ASoC: SOF: Intel: reset SPIB in hda_data_stream_cleanup#5718
ASoC: SOF: Intel: reset SPIB in hda_data_stream_cleanup#5718bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Intel SOF HDA stream cleanup path to reset the SPIB (Software Position In Buffer) register to 0 during hda_data_stream_cleanup(), based on the requirement that the SPIB value is only honored by host DMA when SPIB is enabled.
Changes:
- Add an SPIB “reset to 0” step in
hda_data_stream_cleanup()usingHDA_DSP_SPIB_ENABLE. - Keep the existing playback-specific SPIB disable call after cleanup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sound/soc/sof/intel/hda-stream.c
Outdated
| /* | ||
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | ||
| * That's why we need to set the value with HDA_DSP_SPIB_ENABLE | ||
| */ | ||
| hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_ENABLE, 0); | ||
|
|
||
| if (hstream->direction == SNDRV_PCM_STREAM_PLAYBACK) | ||
| ret = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_DISABLE, 0); |
There was a problem hiding this comment.
The new SPIB reset call enables SPIB unconditionally, but SPIB is only disabled again for playback streams. For capture streams (notably the ICCMAX capture stream path, which doesn’t enable SPIB in hda_data_stream_prepare()), this will leave SPIB enabled after cleanup and changes the stream’s register state unexpectedly. Consider limiting the reset to the playback case (where SPIB is disabled right after), or preserving/restoring the previous SPIB enable state (or explicitly disabling again for capture) so cleanup doesn’t introduce a persistent SPIB enable side effect.
sound/soc/sof/intel/hda-stream.c
Outdated
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | ||
| * That's why we need to set the value with HDA_DSP_SPIB_ENABLE |
There was a problem hiding this comment.
Nit: comment grammar/casing — the sentence after the comma starts with "That's"; consider splitting into two sentences and avoiding the contraction (e.g., "That is why...") to match typical kernel comment style and improve readability.
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | |
| * That's why we need to set the value with HDA_DSP_SPIB_ENABLE | |
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled. | |
| * That is why we need to set the value with HDA_DSP_SPIB_ENABLE. |
The register will indicate to the host DMA where the position is in the buffer currently processed by host SW. Reset it to 0 in hda_data_stream_cleanup. The register is ignored by the host DMA if SPIB is disabled. That's why the "enable" parameter needs to be HDA_DSP_SPIB_ENABLE. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_ENABLE, 0); | ||
| if (ret < 0) { | ||
| dev_err(sdev->dev, "%s: failed to reset SPIB: %d\n", __func__, ret); | ||
| } | ||
|
|
||
| ret1 = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_DISABLE, 0); | ||
| if (!ret) | ||
| ret = ret1; | ||
|
|
There was a problem hiding this comment.
hda_data_stream_cleanup() now returns an error if the SPIB reset/disable fails, which changes behavior for capture streams (previously ret stayed 0 unless direction was PLAYBACK). Since SPIB appears optional and other call sites treat SPIB config as best-effort (e.g. hda_dsp_stream_hw_free() calls hda_dsp_stream_spib_config(...DISABLE...) and still returns 0 at hda-stream.c:770-774), consider not propagating SPIB failures from cleanup (e.g. only attempt when sdev->bar[HDA_DSP_SPIB_BAR] is present, and/or keep ret at 0 unless later cleanup steps fail).
| int ret1; | ||
| int ret; | ||
|
|
||
| if (hstream->direction == SNDRV_PCM_STREAM_PLAYBACK) | ||
| ret = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_DISABLE, 0); | ||
| else | ||
| /* | ||
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | ||
| * That is why we need to set the value with HDA_DSP_SPIB_ENABLE | ||
| */ | ||
| ret = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_ENABLE, 0); | ||
| if (ret < 0) { | ||
| dev_err(sdev->dev, "%s: failed to reset SPIB: %d\n", __func__, ret); | ||
| } | ||
|
|
||
| ret1 = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_DISABLE, 0); | ||
| if (!ret) | ||
| ret = ret1; |
There was a problem hiding this comment.
ret1 is non-descriptive in this context. Renaming it to reflect the operation (e.g. ret_disable) would make the error-handling logic easier to follow and reduce the chance of mistakes during future edits.
The register will indicate to the host DMA where the position is in the buffer currently processed by host SW. Reset it to 0 in hda_data_stream_cleanup. The register is ignored by the host DMA if SPIB is disabled. That's why the "enable" parameter needs to be HDA_DSP_SPIB_ENABLE.