Simplify S0ix implementation #1481
Conversation
|
@keyonjie I dont have a S0ix-enabled platform to test this. Can you please look into it? |
ranj063
force-pushed
the
fix/S0ix
branch
3 times, most recently
from
b38c4b5 to
68e160b
Compare
Simplify the logic to determine whether the DSP should be put in D0i3 or D3 by keeping count of the number of D0i3-compatible streams that have been kept running during suspend. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
| } | ||
|
|
||
| sdev->dsp_power_state = SOF_DSP_D3; | ||
|
|
There was a problem hiding this comment.
Better to set D3 in the core driver, make D0 and D3 be the more generic states?
There was a problem hiding this comment.
I think it makes sense here because the core requests the state and here's where the actual assignement happens isnt it?
There was a problem hiding this comment.
I think it makes sense here because the core requests the state and here's where the actual assignement happens isnt it?
My point is that for platforms which don't have platform .suspend() ops, the dsp_power_state will not be changed then?
There was a problem hiding this comment.
Yes, it will not and thats correct right? You never change the DSP state for those platforms.
| SOF_DSP_D0I3, /* DSP D0i3(low power) substate*/ | ||
| /* DSP power state */ | ||
| enum sof_dsp_power_state { | ||
| SOF_DSP_D0I0 = 0, |
There was a problem hiding this comment.
rename this to SOF_DSP_D0 which sounds more generic.
| bool s0_suspend; | ||
|
|
||
| /* number of pipelines running when the DSP is in D0i3 */ | ||
| u32 D0i3_pipeline_count; |
There was a problem hiding this comment.
pipeline or stream? d0i3-compatible or anyone? make it clear in the comment.
There was a problem hiding this comment.
"number of pipelines running when the DSP is in D0i3". Isnt it clear enough that only D0i3-compatible ones can run in D0i3?
There was a problem hiding this comment.
I specifically chose the word pipeline because the streams are suspended from ALSA point of view. Only the pipelines are running in the DSP
There was a problem hiding this comment.
I specifically chose the word pipeline because the streams are suspended from ALSA point of view. Only the pipelines are running in the DSP
But the count here is of stream, e.g. though KWD stream includes 2 pipelines, we only increase the count by one here.
There was a problem hiding this comment.
Yes, but that's because the KWD pipeline does not contain a host PCM and it never receives the suspend trigger.
| * as well. | ||
| */ | ||
| if (spcm->stream[stream].d0i3_compatible && | ||
| sdev->D0i3_pipeline_count) { |
There was a problem hiding this comment.
the logic here is becoming convoluted?
There was a problem hiding this comment.
How? There are 2 checks. How is it convoluted? In fact the first one could possibly be removed because the KWD stream is meant to be D0i3 compatible
There was a problem hiding this comment.
um, the wording D0i3_pipeline_count make it uneasy to follow to me, as you know, D0I3 stream/pipeline can be not suspend related, how about using 'suspend_ignored_count'?
There was a problem hiding this comment.
@keyonjie "suspend_ignored" is what makes me uneasy. From an ALSA point of view, none of the PCM streams ignore suspend.They do get "suspended" but the DSP pipeline remains running.
ranj063
force-pushed
the
fix/S0ix
branch
2 times, most recently
from
af5cf38 to
82d663b
Compare
ranj063
changed the title
|
SOFCI TEST |
|
SOFCI TEST |
This patch simplifies the suspend/resume callback in the SOF device by passing the required DSP state as an argument to the platform-specific suspend/resume ops. In order to support this, the d0_substate member of struct snd_sof_dev is renames to dsp_power_state to include the D3 state as well. The enum sof_d0_substate is accordingly expanded to include SOF_DSP_D3 state and renamed to sof_dsp_power_state. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
|
SOFCI TEST |
Doing that now, but looks my hatch doesn't create sound card with the latest code base, let me fix it first. |
|
@ranj063 KWD works fine on my Hatch Ubuntu, don't have chance to run it on the Helios yet. |
|
closing will be replaced by @keyonjie |
2 participants
@keyonjie @plbossart This is my proposal to simplify the S0ix/D0ix implementation.
At the top-level PM callbacks, we only need to worry about whether the DSP should be set to D0i3 or D3. Everything else should be handled in the platform-specific suspend callback.
The decision to enter D0i3 is made simpler by loooking at the number of D0i3-compatible pipelines active.