Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Suspend-target handling, DAPM endpoint coverage, and ACE2.x teardown currently prevent safe path retention.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Adds suspend retention for D0i3-compatible SOF SoundWire audio paths.
Changes:
- Propagates suspend retention through DAPM and SoundWire streams.
- Keeps Intel SoundWire buses active during suspend.
- Adjusts trigger handling for retained streams.
| File | Description |
|---|---|
sound/soc/sof/pcm.c |
Marks connected DAPM widgets for retention. |
sound/soc/sof/intel/hda.c |
Propagates D0i3 compatibility to SoundWire. |
sound/soc/sof/intel/hda-dai-ops.c |
Handles resume triggers. |
sound/soc/sdw_utils/soc_sdw_utils.c |
Preserves retained streams across suspend. |
include/linux/soundwire/sdw.h |
Adds retention state and bus API. |
drivers/soundwire/stream.c |
Implements bus retention detection. |
drivers/soundwire/intel.c |
Avoids suspending retained controller state. |
drivers/soundwire/intel_auxdevice.c |
Skips bus power transitions when retained. |
drivers/soundwire/intel_ace2x.c |
Applies retention to ACE2.x controllers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ujfalusi
left a comment
There was a problem hiding this comment.
@bardliao, how about the codec drivers themself? I think that the one you use only marks the regcache cache only, but it would not be far fetched if a codec driver would actually do the right thing and powers down things in their PM suspend callback?
| swidget->spipe->started_count = 0; | ||
| break; | ||
| case SNDRV_PCM_TRIGGER_PAUSE_PUSH: | ||
| case SNDRV_PCM_TRIGGER_RESUME: |
There was a problem hiding this comment.
with this patch: 8d544a0
you are not going to receive the RESUME trigger.
| break; | ||
| } | ||
| if (!list_entry_is_head(spcm, &sdev->pcm_list, list) && | ||
| spcm->stream[dir].d0i3_compatible) |
There was a problem hiding this comment.
this will mark DeepBuffer playback and capture also, which is not correct.
The definition to use is:
9e47005
There was a problem hiding this comment.
@ujfalusi Do you mean just test spcm->stream[substream->stream].suspend_ignored?
There was a problem hiding this comment.
* WoV streams can be indetified by:
* They are capture streams and
* They have the d0i3_compatible flag set and
* They don't use Deep Buffer
| return ret; | ||
| } | ||
|
|
||
| if (spcm->stream[dir].d0i3_compatible) { |
There was a problem hiding this comment.
this will break deepbuffer playback and capture during suspend, the definition to use is:
9e47005
Most SDW codec drivers don't do power down in their suspend callback. IOW the current solution applies to most codecs. We can handle the codecs that need to skip the device suspend in the future if needed. |
Operation whack-a-mole? |
Some streams, such as those used for wake-on-voice, must remain active during system suspend. Add an ignore_suspend flag to sdw_stream_runtime and export sdw_bus_ignore_suspend() so bus drivers can check whether any stream on the bus requires it to remain active. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Honor the SoundWire bus ignore-suspend state in the Intel drivers. Preserve the master across system suspend when a stream requires the bus to remain active. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Do not disable a SoundWire stream on suspend when it is marked to remain active. If the stream is already enabled on resume, no further action is needed. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Keep a SoundWire stream active during system suspend when it is used by a WoV stream. WoV streams can be indetified by: They are capture streams and. They have the d0i3_compatible flag set and They don't use Deep Buffer. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
0377bcf to
d247a69
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Suspend policy is applied too broadly and required DAPM endpoints are omitted, potentially breaking wake-on-voice and deeper suspend states.
Review effort: Balanced
Findings: 6
Open (6)
Propagate suspend policy only for the active SOF suspend target · New Mark the starting DAI endpoint before walking connected widgets · New Avoid permanently ignoring suspend based only on topology compatibility Derive D0i3 retention from the actual suspend target Skip SoundWire teardown on suspend for retained streams Do not retain the bus for stopped D0i3 streams
d247a69 to
558587f
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Suspend-ignore state is incomplete and persists beyond its valid lifecycle, causing both WoV failures and unintended bus retention.
Review effort: Balanced
Findings: 7
Open (7)
Clear or refcount widget suspend-ignore state after resume · New Mark the starting DAI endpoint before walking connected widgets Propagate suspend policy only for the active SOF suspend target Avoid permanently ignoring suspend based only on topology compatibility Derive D0i3 retention from the actual suspend target Skip SoundWire teardown on suspend for retained streams Do not retain the bus for stopped D0i3 streams
558587f to
9e0e7c2
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Suspend-target mismatches and incomplete DAPM lifecycle handling can disable WoV paths or incorrectly keep inactive buses powered.
Review effort: Balanced
Findings: 2
Open (2)
Resolved since last review (6)
Mark the starting DAI endpoint before walking connected widgets Propagate suspend policy only for the active SOF suspend target Avoid permanently ignoring suspend based only on topology compatibility Derive D0i3 retention from the actual suspend target Skip SoundWire teardown on suspend for retained streams Do not retain the bus for stopped D0i3 streams
The SOF DSP remains active for wake on voice streams, but DAPM may still power down widgets on the PCM path. Mark the connected endpoint widgets to ignore suspend so DAPM keeps the path powered. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
9e0e7c2 to
191e953
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Suspend-ignore state is applied too broadly and DAPM endpoint state is not preserved safely.
1 open finding
2 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| dev_dbg(sdev->dev, | ||
| "WoV PCM %s ignore suspend for widget %s\n", | ||
| spcm->pcm.caps[dir].name, widget->name); | ||
| widget->ignore_suspend = ignore_suspend; |

Wake-on-voice requires the DSP audio path to remain available while the
system is suspended. Although SOF can keep a D0i3-compatible PCM running,
SoundWire stream and Intel controller suspend handling may still disable
the stream or its bus, while DAPM may power down widgets on the path.
This series propagates the ignore-suspend requirement from SOF to the
SoundWire stream and connected DAPM widgets and keep the required
components alive during system suspend.