ASoC: SOF: Intel: reset SPIB in hda_data_stream_cleanup#5718
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.
| /* | ||
| * 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.
| * 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).
There was a problem hiding this comment.
The original code also propagates HDA_DSP_SPIB_DISABLE error. I think we need to propagate the error on capture, too.
| * 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); |
There was a problem hiding this comment.
what this will fix?
hda_dsp_stream_spib_config() will enable/disable SPIB and also writes the size, will reset the size as well.
Also, in hda_dsp_pcm_hw_params() we have this:
/* enable SPIB when rewinds are disabled */
if (hda_disable_rewinds)
hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_ENABLE, 0);
else
hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_DISABLE, 0);So, you might have SPIB disabled but then you enable it?
There was a problem hiding this comment.
And I will eventually disable it.
| int ret; | ||
|
|
||
| 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.
if this is true, that the spib_addr must be set when the SPIB is enabled - which is strange as you are changing an address runtime, but anyways, then would not this be a better and global solution:
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index b5d81a3b8b39..0f8807f14fbd 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -198,13 +198,18 @@ int hda_dsp_stream_spib_config(struct snd_sof_dev *sdev,
mask = (1 << hstream->index);
+ /* Reset the spib_addr before disabling SPIB */
+ if (!enable)
+ sof_io_write(sdev, hstream->spib_addr, 0);
+
/* enable/disable SPIB for the stream */
snd_sof_dsp_update_bits(sdev, HDA_DSP_SPIB_BAR,
SOF_HDA_ADSP_REG_CL_SPBFIFO_SPBFCCTL, mask,
enable << hstream->index);
/* set the SPIB value */
- sof_io_write(sdev, hstream->spib_addr, size);
+ if (enable)
+ sof_io_write(sdev, hstream->spib_addr, size);
return 0;
}
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.