[RFC]ASoC: SOF: remove runtime PM calls during debugfs read

[ranj063]

runtime PM calls during debugfs reads are not necessary because:

1. If the DSP is active, there's no need to wake it up.
2. If the DSP is suspended, there's no updates to its
   status, so there's no need to wake it up. Besides
   pm_runtime_get_noresume() doesn't really resume the DSP
   but just increments the device counter.
3. If the DSP is in panic, runtime PM is pointless. We
   cannot resume/suspend it anyway.
4. The runtime_pm_put_autosuspend() call at the end of memcpy
   cannot be paired with a pm_runtime_get_noresume().
5. Finally, this I'm not entirely sure about this but I think
   when we read a debugfs memory, we're only reading the shared
   memory between the host and DSP. We don't really copy memory
   from the DSP's SRAM to the shared memory.

Signed-off-by: Ranjani Sridharan [email protected]

[plbossart]

I don't know either why pm_runtime was enabled for debugfs. This was modified in #237 but there's nothing that tells me why this was needed in the first place.

[plbossart]

Actually no, this won't work.
The buffer in DDR is for trace.
For debugfs, we do copy data from mmio.

static const struct snd_sof_debugfs_map byt_debugfs[] = {
	{"dmac0", BYT_DSP_BAR, DMAC0_OFFSET, DMAC_SIZE},
	{"dmac1", BYT_DSP_BAR,  DMAC1_OFFSET, DMAC_SIZE},
	{"ssp0",  BYT_DSP_BAR, SSP0_OFFSET, SSP_SIZE},
	{"ssp1", BYT_DSP_BAR, SSP1_OFFSET, SSP_SIZE},
	{"ssp2", BYT_DSP_BAR, SSP2_OFFSET, SSP_SIZE},
	{"iram", BYT_DSP_BAR, IRAM_OFFSET, IRAM_SIZE},
	{"dram", BYT_DSP_BAR, DRAM_OFFSET, DRAM_SIZE},
	{"shim", BYT_DSP_BAR, SHIM_OFFSET, SHIM_SIZE},
};

BYT is a bad example since PM is not enabled, but in general some of those resources will not be available when the device is suspended.

[ranj063]

Actually no, this won't work.
The buffer in DDR is for trace.
For debugfs, we do copy data from mmio.

static const struct snd_sof_debugfs_map byt_debugfs[] = {
	{"dmac0", BYT_DSP_BAR, DMAC0_OFFSET, DMAC_SIZE},
	{"dmac1", BYT_DSP_BAR,  DMAC1_OFFSET, DMAC_SIZE},
	{"ssp0",  BYT_DSP_BAR, SSP0_OFFSET, SSP_SIZE},
	{"ssp1", BYT_DSP_BAR, SSP1_OFFSET, SSP_SIZE},
	{"ssp2", BYT_DSP_BAR, SSP2_OFFSET, SSP_SIZE},
	{"iram", BYT_DSP_BAR, IRAM_OFFSET, IRAM_SIZE},
	{"dram", BYT_DSP_BAR, DRAM_OFFSET, DRAM_SIZE},
	{"shim", BYT_DSP_BAR, SHIM_OFFSET, SHIM_SIZE},
};

BYT is a bad example since PM is not enabled, but in general some of those resources will not be available when the device is suspended.

@plbossart just to confirm then we do need to resume the DSP to read the debugfs entries if the device in runtime suspended? Is this true for all the memory windows including exception?

[plbossart]

@plbossart <https://github.com/plbossart> just to confirm then we do need to resume the DSP to read the debugfs entries if the device in runtime suspended? Is this true for all the memory windows including exception?

That's not clear to me. Some of the registers exposed through debugfs are not tied to the DSP state, so the answer is no in general. We might need a flag in the debugfs_map which states is the mmio survives a suspend case. @lgirdwood, thoughts?

[ranj063]

@plbossart I had a discussion with Liam and Lech about this today. What they told me was that once the DSP is in D3, the contents of the memory windows are invalid. So their suggestion was to have a debugfs entry that prevents the DSP from entering D3 before reading the memory windows.
The reason for this was that even if we do resume the DSP from D3 to read the memory windows, we'd have lost all context from before.

Also, the last case would be when reading the memory windows when the DSP is in D3. In this case, Liam suggested we should let the userspace know that the contents might be invalid as the DSP is suspended.

Please let me know what you think and i'll change the PR accordingly.

[plbossart]

@ranj063 I still believe we need to add a flag for each debugfs file that states if it's affected or not by a transition to a D3 state. yes the memory windows are probably not correct in D3 (even D0i1), so you'd want to copy it to some mirror before suspending, but others such as SSP registers or PP parts can be accessed as desired.
In other words, not all debugfs information is related to memory windows so we should not assume the solution for memory windows is generic and applies in all cases.

[ranj063]

@plbossart ok that makes sense. When you say mirror it, do you mean cache it in the driver? And we would do this every time prior to the DSP entering D3?

[plbossart]

@plbossart ok that makes sense. When you say mirror it, do you mean cache it in the driver? And we would do this every time prior to the DSP entering D3?

yes. On suspend you mirror, and you only update the mirror and pass the data to debugfs if the device is active.

[ranj063]

@plbossart I will update and submit a new PR based on our discussion. Closing this one.