ASoC: SOF: debug: change runtime PM get/put to correct usuage

[keyonjie]

this should fix #233

ASoC: SOF: debug: change runtime PM get/put to correct usuage

When doing debugFS entry read, we should make sure it won't enter
suspend during the reading, and the reading won't interact the runtime
PM(e.g. if the idle timeout is expired when should enter runtime suspend
ASSP) neither.

Here change to use get_noresume() to make sure not resuming ADSP for
debugFS reading, and put_sync_autosuspend() to ensure we can enter
runtime suspend after reading finished if needed.

Signed-off-by: Keyon Jie [email protected]

[xiulipan]

@keyonjie
In fail cases, the DSP are not able to power off, so this fixes could work.
when runtime pm power off the DSP, then we try to read from memory window. What will happen?

[plbossart]

Humm, in the case "For audio suspend case, reading back old value is reasonable", do you mean to say values that may not be up-to-date but weren't previously read? Or that the debugs info will always be consistent but not necessarily up-to-date. Not sure what you meant by 'old' and whether the data can be inconsistent/misleading?

[lgirdwood]

@plbossart I think "old" means the current values before the FW crash (that prevents runtime PM transitions). Iirc, a FW panic would prevent any D3 entry or core PM so that the SRAM contents should be valid here. @keyonjie can you confirm if this is what you can see.

[keyonjie]

@plbossart @lgirdwood the value depends on what is in DSP MMIO(memory window) at that moment, for DSP crashed or suspended cases, it might be the valid or invalid(e.g. 0xffffffff), but that is what we can try to read back, for analyzing.

The word "old value" is not accurate, now corrected.

[xiulipan]

@keyonjie
Will this have regression about the error trace disappear? If we did not power up the DSP when we try to get etrace, will it show 0xffffffff as it is invalid.

[keyonjie]

@xiulipan when we try to get etrace at suspend, it might show invalid or 0xffffffff, but I don't want to call it regression.
If we want to get etrace, we can do that after ADSP is waked up again, e.g. open alsamixer. So this is only design style choice, if we want(or can bear) to disturb the normal running of ADSP when reading debugFS.

[xiulipan]

@keyonjie
The previous design is to wake up DSP as we want to read the memory window. But every wake up will clean the memory window. So the previous run log will be cleaned after DSP suspend.
But for most debug cases, we want to check what caused the last run to exit or error, the logs are cleared by DSP power on.

For this PR, we can not get the last run log as well.

To achieve the goal to read error trace all the time, we should have more change about trace.

[keyonjie]

@xiulipan agree, this PR can't fix the error trace be cleared after resume issue.

[lgirdwood]

@keyonjie @xiulipan lets have a debubFS file that can be used to enable/disable runtime PM. That way we can disable D3 entry from cmd line if needed.

[plbossart]

@lgirdwood @ranj063 don't we already have existing pm_runtime hooks in /sys to disable D3 entry?

[keyonjie]

@lgirdwood @plbossart we already have that in /sys/bus/pci/device/xxx/power/control for that purpose.

[lgirdwood]

@keyonjie great, lets document that then under debug in public wiki

[plbossart]

@keyonjie I can't figure out if we need this patch merged, or if it contains a design flaw. Can you clarify what the status is, the discussion is not self-explanatory any longer.

[ranj063]

@lgirdwood @plbossart we already have that in /sys/bus/pci/device/xxx/power/control for that purpose.

its actually /sys/devices/pci*/00*0e/power/control

[ranj063]

@plbossart I think we might still need this patch to solve the problem of not being able to read debugfs when dsp panic occurs.

we do not use get/put while reading trace debugfs, so this seems OK to me.

[keyonjie]

@plbossart as Ranjani stated, I think better to apply this, it helps on dsp panic trace/debug, and no harm(or it actually improve to avoid interaction to DSP running during debugFS reading on suspend) for other cases.

[plbossart]

maybe a stupid comment but wouldn't pm_runtime_get_noresume and put_autosuspend be appropriate here? Nothing would happen if the device is already sleeping or crashed.

[ranj063]

@plbossart @keyonjie I think put_autosuspend will still cause errors.

But get_noresume() and put_noidle() will just increment and decrement the usage counters and it might be useful to have this to prevent the dsp from entering D3 (if it were in D0 before) during debugfs read.

[keyonjie]

@ranj063 sounds good to use get_noresume() which will help prevent dsp to enter D3,
but not suitable to use put_noidle() fore return as it won't decrease to 0 where we need it actually.

@plbossart comparing to put_autosuspend(), I think we should use put_sync_autosuspend(), the latter one will use sync mode which may lead to suspend ASAP which means our debugFS reading won't influence runtime PM too much.

@lgirdwood I think you are familiar with this, any comments?

[keyonjie]

@lgirdwood @plbossart @ranj063 updated as stated above, please help review.

[ranj063]

@keyonjie if the dsp has crashed and you request a put_sync_autosuspend(), what happens then?

[keyonjie]

@keyonjie if the dsp has crashed and you request a put_sync_autosuspend(), what happens then?

it just put the count we got above, if it is planed to go suspend, then let it go suspend.
It don't change the behavior, that means, keep what it should do as that without the debugFS read, keep what the driver will do autosuspend or not at DSP panic.

[plbossart]

There are a couple of points that are completely unclear to me.

using pm_runtime_get_noresume will only not have any effect if the DSP is in D3. If it is in D0, it will prevent entry in D3. What about when the DSP is crashed? What is the effect? Does this solve the problem that was initially reported?

When you use autosuspend you typically use mark_last_busy to provide a reference point. If you use pm_runtime_put_sync_autosuspend, when exactly will the transition to D3 take place. And what's the difference with pm_runtime_put_sync if you haven't used the mark-last-busy (which possibly makes no sense if you are already in D3)?

If we don't have the answers then I'll need experimental evidence that this patch solves the initial problem and has no side effects before merging it.

[keyonjie]

There are a couple of points that are completely unclear to me.

using pm_runtime_get_noresume will only not have any effect if the DSP is in D3. If it is in D0, it will prevent entry in D3. What about when the DSP is crashed? What is the effect? Does this solve the problem that was initially reported?

Currently, if DSP is crashed, usually we are still in D0 in driver side(we may do improvement in future, e.g. unload/reload modules to try to recover, or disable runtime PM when DSP panic detected..), the patch here will solved the #233, I have tried we can read etrace at this case, we actually treat it as normal D0.

When you use autosuspend you typically use mark_last_busy to provide a reference point. If you use pm_runtime_put_sync_autosuspend, when exactly will the transition to D3 take place. And what's the difference with pm_runtime_put_sync if you haven't used the mark-last-busy (which possibly makes no sense if you are already in D3)?

I am not sure about this, I intent to don't mark-busy here which means don't change the normal audio device runtime suspend/resume per my understanding, just raise question on PM ML.

If we don't have the answers then I'll need experimental evidence that this patch solves the initial problem and has no side effects before merging it.

I had done experiment:
for DSP panic case, issue #233 is fixed, we can read etrace and other memory windows values.
for D0 case, no regression found.
for D3 case, we can also read back entries without resuming DSP.

[plbossart]

When you use autosuspend you typically use mark_last_busy to provide a reference point. If you use pm_runtime_put_sync_autosuspend, when exactly will the transition to D3 take place. And what's the difference with pm_runtime_put_sync if you haven't used the mark-last-busy (which possibly makes no sense if you are already in D3)?

I am not sure about this, I intent to don't mark-busy here which means don't change the normal audio device runtime suspend/resume per my understanding, just raise question on PM ML.

Can you add a TODO comment in the code to track this is an open point so that we can merge this first and deal with this later.

[keyonjie]

When you use autosuspend you typically use mark_last_busy to provide a reference point. If you use pm_runtime_put_sync_autosuspend, when exactly will the transition to D3 take place. And what's the difference with pm_runtime_put_sync if you haven't used the mark-last-busy (which possibly makes no sense if you are already in D3)?

I am not sure about this, I intent to don't mark-busy here which means don't change the normal audio device runtime suspend/resume per my understanding, just raise question on PM ML.

Can you add a TODO comment in the code to track this is an open point so that we can merge this first and deal with this later.

Sure, let me add this now.

[keyonjie]

@plbossart updated.