-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASoC: SOF: Intel: hda: clean up link DMA during stop for IPC4 #5197
base: topic/sof-dev
Are you sure you want to change the base?
Conversation
SOFCI TEST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for the metallic sound as we will operate like IPC3 but breaks IPC4 delay reporting when the LLP is host read.
sound/soc/sof/intel/hda-dai.c
Outdated
!hda_stream->dma_cleanup_during_stop) | ||
break; | ||
hda_stream->dma_cleanup_during_stop = false; | ||
fallthrough; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063, this is essentially equals to:
diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index 484c76147885..dffe9e145a1e 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -512,7 +512,6 @@ static const struct hda_dai_widget_dma_ops sdw_ipc4_chain_dma_ops = {
static int hda_ipc3_post_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cpu_dai,
struct snd_pcm_substream *substream, int cmd)
{
- struct hdac_ext_stream *hext_stream = hda_get_hext_stream(sdev, cpu_dai, substream);
struct snd_soc_dapm_widget *w = snd_soc_dai_get_widget(cpu_dai, substream->stream);
switch (cmd) {
@@ -527,9 +526,6 @@ static int hda_ipc3_post_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *c
if (ret < 0)
return ret;
- if (cmd == SNDRV_PCM_TRIGGER_STOP)
- return hda_link_dma_cleanup(substream, hext_stream, cpu_dai);
-
break;
}
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index f1a8491bba9e..df8bf497e224 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -303,6 +303,7 @@ static int __maybe_unused hda_dai_trigger(struct snd_pcm_substream *substream, i
switch (cmd) {
case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_STOP:
ret = hda_link_dma_cleanup(substream, hext_stream, dai);
if (ret < 0) {
dev_err(sdev->dev, "%s: failed to clean up link DMA\n", __func__);
I don't see why we need to exclude capture, apart from that we did not tested. It has the same issue.
At the end we will do the same as we do with IPC3, right? Afaik we have most of these hda-ops complication to not do the same thing for IPC4, there must be a reason why we did this in the first place and diverged from IPC3 flow.
This is one note, the second is: with this change you need to revert (at the same time):
1abc264 ("ASoC: SOF: Intel: hda: Compensate LLP in case it is not reset")
f9eeb6b ("ALSA: hda: Add pplcllpl/u members to hdac_ext_stream")
To not break the delay reporting when the firmware is not reporting LLP (I need to test the firmware reporting case still)
The third and might not be applicable is that we have hda_link_dma_cleanup() called in hda_dai_hw_free(), so we clean up twice, I'm not sure if that is a problem or not, but something to keep in mind.
Right, it might handling the case when PCM is opened, configured, not triggered and then closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the firmware reported LLP works, but I have not tested LNL, where the HD-DMA link LLP is also reported by firmware...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one note, the second is: with this change you need to revert (at the same time):
1abc264 ("ASoC: SOF: Intel: hda: Compensate LLP in case it is not reset")
f9eeb6b ("ALSA: hda: Add pplcllpl/u members to hdac_ext_stream")
To not break the delay reporting when the firmware is not reporting LLP (I need to test the firmware reporting case still)
@ujfalusi should I really revert the commit. We still dont stop the DMAs during pause. So I should only exclude stop from it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, on LNL SDW, second iteration fails to:
[ 49.455927] sof-audio-pci-intel-lnl 0000:00:1f.3: ipc tx : 0x45010006|0x10004: MOD_BIND
[ 49.456812] sof-audio-pci-intel-lnl 0000:00:1f.3: ipc tx reply: 0x65000000|0x10004: MOD_BIND
[ 49.456836] sof-audio-pci-intel-lnl 0000:00:1f.3: ipc tx done : 0x45010006|0x10004: MOD_BIND
[ 49.458755] soundwire_intel soundwire_intel.link.0: cmd=1 dai SDW0 Pin2 direction 0
[ 49.458761] soundwire_intel soundwire_intel.link.0: ASoC: error at soc_dai_trigger on SDW0 Pin2: -22
[ 49.460886] Jack Out: ASoC: trigger FE cmd: 1 failed: -22
@ranj063, I think we can do this in a different way by introducing the missing logic to track the pending_stop on the Intel-HDA side, something like this: diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index 484c76147885..157cdbebd2ad 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -413,8 +413,16 @@ static int hda_ipc4_post_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *c
goto out;
pipeline->state = SOF_IPC4_PIPE_RUNNING;
break;
- case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
+ {
+ struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream);
+ struct sof_intel_hda_stream *hda_stream = hstream_to_sof_hda_stream(hext_stream);
+
+ hda_stream->pending_stop = true;
+ }
+
+ fallthrough;
+ case SNDRV_PCM_TRIGGER_SUSPEND:
/*
* STOP/SUSPEND trigger is invoked only once when all users of this pipeline have
* been stopped. So, clear the started_count so that the pipeline can be reset
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index f1a8491bba9e..d0f973e70d60 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -136,6 +136,7 @@ int hda_link_dma_cleanup(struct snd_pcm_substream *substream, struct hdac_ext_st
/* free the host DMA channel reserved by hostless streams */
hda_stream = hstream_to_sof_hda_stream(hext_stream);
hda_stream->host_reserved = 0;
+ hda_stream->pending_stop = false;
return 0;
}
@@ -232,8 +233,16 @@ static int __maybe_unused hda_dai_hw_params_data(struct snd_pcm_substream *subst
}
hext_stream = ops->get_hext_stream(sdev, dai, substream);
- if (hext_stream && hext_stream->link_prepared)
- return 0;
+ if (hext_stream && hext_stream->link_prepared) {
+ struct sof_intel_hda_stream *hda_stream;
+
+ hda_stream = hstream_to_sof_hda_stream(hext_stream);
+
+ if (!hda_stream->pending_stop)
+ return 0;
+
+ hda_dai_hw_free(substream, dai);
+ }
ret = hda_link_dma_hw_params(substream, params, dai);
if (ret < 0)
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 32ae04b0a1e7..09e7f7fddebd 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -571,6 +571,7 @@ struct sof_intel_hda_stream {
struct sof_intel_stream sof_intel_stream;
int host_reserved; /* reserve host DMA channel */
u32 flags;
+ bool pending_stop;
struct completion ioc;
};
the two HDA delay patch still needs to be reverted. And we need to check the LNL SDW (and SSP/DMIC?) also, it relies on the fact that we don't release the stream... |
7a81ca9
to
03f5352
Compare
@ranj063, for the delay calculation I suggest something like this: diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index d015664b76d8..92681ca7f24d 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -346,22 +346,21 @@ static int hda_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cpu_dai,
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
snd_hdac_ext_stream_start(hext_stream);
break;
- case SNDRV_PCM_TRIGGER_SUSPEND:
- case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
- snd_hdac_ext_stream_clear(hext_stream);
-
/*
- * Save the LLP registers in case the stream is
- * restarting due PAUSE_RELEASE, or START without a pcm
- * close/open since in this case the LLP register is not reset
- * to 0 and the delay calculation will return with invalid
- * results.
+ * Save the LLP registers since in case of PAUSE the LLP
+ * register are not reset to 0, the delay calculation will use
+ * the saved offsets for compensating the delay calculation.
*/
- if (cmd == SNDRV_PCM_TRIGGER_PAUSE_PUSH) {
- hext_stream->pplcllpl = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPL);
- hext_stream->pplcllpu = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPU);
- }
+ hext_stream->pplcllpl = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPL);
+ hext_stream->pplcllpu = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPU);
+ snd_hdac_ext_stream_clear(hext_stream);
+ break;
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_STOP:
+ hext_stream->pplcllpl = 0;
+ hext_stream->pplcllpu = 0;
+ snd_hdac_ext_stream_clear(hext_stream);
break;
default:
dev_err(sdev->dev, "unknown trigger command %d\n", cmd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works now on LNL-SDW machine (no EINVAL and audio is good). Some comments about code inline.
return ret; | ||
/* Inform DSP about PDI stream number */ | ||
return intel_params_stream(sdw, substream, dai, hw_params, sdw->instance, | ||
dai_runtime->pdi->intel_alh_id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i Yes, we can have soundwire/asoc combined patches especially when they have dependency. Usually, it will be applied to the tree that the main change is in with the other maintainer's ack-by tag.
Update: the delay reporting is broken and needs the addition @ujfalusi added in a comment. Now with start-stop-start-stop cycle, the saved AZX_REG_PPLCLLPL/U is added to delay. The error is not huge as the error doesn't compound, but the delay reporting still is wrong in this case. |
For aggregated DAIs, the node ID is set to the group_id during the DAI widget's ipc_prepare op. With the current logic, setting the dai_index for node_id in the dai_config is redundant as it will be overwritten with the group_id anyway. Removing it will also prevent any accidental clearing/resetting of the group_id for aggregated DAIs due to the dai_config calls could that happen before the allocated group_id is freed. Signed-off-by: Ranjani Sridharan <[email protected]>
03f5352
to
aa19fcd
Compare
@ujfalusi fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always best to add more inline comments when we have complex use case around transitions and FW/kernel interactions. Lets state all assumptions about FW so that anyone can follow.
return ret; | ||
/* Inform DSP about PDI stream number */ | ||
return intel_params_stream(sdw, substream, dai, hw_params, sdw->instance, | ||
dai_runtime->pdi->intel_alh_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the same change in drivers/soundwire/intel.c for the older platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory this is only needed when HD-DMA is used on the link side, so ACE2+, but I'm not sure, I guess tests will tell if needed or not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good assumption to capture in our inline comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not needed for platform up until MTL because we dont use the link DMA for Soundwire in those platforms. Thats why this is isolated to ACE2+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063 , apart from the typo in commit message it looks good to me.
return ret; | ||
/* Inform DSP about PDI stream number */ | ||
return intel_params_stream(sdw, substream, dai, hw_params, sdw->instance, | ||
dai_runtime->pdi->intel_alh_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory this is only needed when HD-DMA is used on the link side, so ACE2+, but I'm not sure, I guess tests will tell if needed or not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on LNL-SDW and works great. Some comments on the git commits still and split into series, please see inline. I think the 3rd/4th patches would need to be linked more strongly together.
@@ -383,11 +383,12 @@ static int intel_hw_params(struct snd_pcm_substream *substream, | |||
static int intel_prepare(struct snd_pcm_substream *substream, | |||
struct snd_soc_dai *dai) | |||
{ | |||
struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The " after a call to snd_pcm_drain/snd_pcm_drop" is duplicated in commit message (second time "This is needed for the case that the DMA data is cleared when the PCM is stopped due to xruns or snd_pcm_drain/drop()."
@@ -383,11 +383,12 @@ static int intel_hw_params(struct snd_pcm_substream *substream, | |||
static int intel_prepare(struct snd_pcm_substream *substream, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the commit is a bit hard to understand alone (and in drivers/soundwire this is potentially view as a standalone commit). Without "ASoC: SOF: Intel: hda: Always clean up link DMA during stop ", this patch has not been needed but there a audio corruption was hit (with Soundwire as well). So this commit is inherently linked to the 4th patch and at minimum the bug needs to be mentioned. I wonder if this could be combined as a single patch or at least a depends or some-such tag is need to ensure these two patches always go together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message is good now, but how about the series dependencies @ranj063 @ujfalusi @bardliao ? Should a "Depends-on: " tag be added between the SOF and soundwire patches, or just have a link to the bug in all. If a partial series is cherry-picked to a stable tree, the bugfix will not be complete.
When a PCM is restarted after a snd_pcm_drain/snd_pcm_drop(), the prepare callback will be invoked and the hw_params will be set again. For the HDA DAI's, the hw_params function handles this case already but not for the non-HDA DAI's. So, add the check for link_prepared to verify if the hw_params should be done again or not. Additionally, for SDW DAI's reset the PCMSyCM registers as would be done in the case of a start after a hw_free. Signed-off-by: Ranjani Sridharan <[email protected]>
In the case of a prepare callback after an xrun or when the PCM is restarted after a call to snd_pcm_drain/snd_pcm_drop, avoid reprogramming the SHIM registers but send the PDI stream number so that the link DMA data can be set. This is needed for the case that the DMA data is cleared when the PCM is stopped and restarted without being closed. Link: https://github.com/thesofproject/sof/issues/9502 Signed-off-by: Ranjani Sridharan <[email protected]>
This is required to reset the DMA read/write pointers when the stream is prepared and restarted after a call to snd_pcm_drain()/snd_pcm_drop(). Also, now that the stream is reset during stop, do not save LLP registers in the case of STOP/suspend to avoid erroneous delay reporting. Link: https://github.com/thesofproject/sof/issues/9502 Signed-off-by: Ranjani Sridharan <[email protected]>
aa19fcd
to
5db9c7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063, looks good to my eyes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ranj063 , looks good now. I do have one remaining question on how to mark the patches in the series in order to ensure downstream users handle the series correctly. See comment inline, Depends-on perhaps?
@@ -383,11 +383,12 @@ static int intel_hw_params(struct snd_pcm_substream *substream, | |||
static int intel_prepare(struct snd_pcm_substream *substream, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message is good now, but how about the series dependencies @ranj063 @ujfalusi @bardliao ? Should a "Depends-on: " tag be added between the SOF and soundwire patches, or just have a link to the bug in all. If a partial series is cherry-picked to a stable tree, the bugfix will not be complete.
We should submit the series in a patch set and all those commits will land in the same kernel version. Should this be cherry-picked to a stable tree? There is no "Fixes" tag in the comment message, so I don't expect that will be cherry-picked to a stable tree. |
@bardliao I dont think I can add a fixes tag to this because this has always existed as far back as IPC4 support was added. |
SOFCI TEST |
Clean up the link DMA for playback during stop for IPC4. This is required to reset the DMA read/write pointers when the stream is prepared and restarted after a call to snd_pcm_drain()/snd_pcm_drop(). Add a new field to struct sof_intel_hda_stream that will be set only in the case of IPC4.
Link: #5198