ASoC: SOF: Intel: Make sure that link and host DMA is coupled after use #5525
Conversation
sound/soc/sof/intel/hda-stream.c
Outdated
|
|
||
| if (!sdev->dspless_mode_selected && found) { | ||
| /* couple host and link DMA if link DMA channel is idle */ | ||
| if (!hext_stream->link_locked) |
There was a problem hiding this comment.
Not sure if it is correct. The list_for_each_entry(s, &bus->stream_list, list) loop doesn't break when if find a stream. In other words, hext_stream will be the last stream of the bus->stream_list.
There was a problem hiding this comment.
Yes, I need to save the hext_stream as well...
sound/soc/sof/intel/hda-stream.c
Outdated
| continue; | ||
|
|
||
| if (s->direction == direction && s->stream_tag == stream_tag) { | ||
| mask = BIT(hext_stream->hstream.index); |
There was a problem hiding this comment.
Should it be |=? Can we assume that there is only one stream found?
There was a problem hiding this comment.
It should be = we cannot have multiple streams with the same stream_tag
ujfalusi
force-pushed
the
peter/sof/pr/intel-couple-dmas-01
branch
from
68eaed3 to
baa31e4
Compare
|
Changes since v1:
|
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Looks good, one idea on reusing HDA generic code for this, see inline
| SOF_HDA_REG_PP_PPCTL, mask, 0); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
what happens if there are two link DMAs for one host DMA, e.g. for a splitter on playback or merge on capture?
There was a problem hiding this comment.
That I'm not sure, I cannot find a topology which has that config, but in that case I would assume that when we are at this point, both link DMAs have been released and we can couple them back.
I also think that in that case we would have reserved two host DMA channels, each using distinct stream_tag, but only use one of them on the host side, so we would couple back them eventually one-by-one?
There was a problem hiding this comment.
@bardliao in the case of amplifier aggregation, don't we have two link DMAs, one per PDI/port/link? IIRC we had to change the DMA handling because it didn't support amplifier aggregation with a single DMA transfer across two links.
There was a problem hiding this comment.
@plbossart @ujfalusi I wonder what we missed but we do the coupling back in 2 places both in the host side (during hw_free)and the link side (in snd_hdac_ext_stream_release). If it is the case that during FE hw_free, the link is still locked, we should couple it back when the BE hw_free happens isnt it?
There was a problem hiding this comment.
Maybe what we need to do is to make the coupling/decoupling match on both the BE and FE DAI to be done in the hw_params/hw_free instead of open/close for the FE and hw_params/hw_free for the BE DAI
There was a problem hiding this comment.
We also need to handle the code loader DMA and BRA.
Just loading the driver in DSP mode and never doing audio activity, removing, switching to DSPless/Legacy mode will fail. Same with BRA.
I think our callback sequencing got changed over time and this got broken. Not a frequently used feature, but it hurts when it is not working as expected.
There was a problem hiding this comment.
yeah but just doing it at the FE side during stream_put will not be enough to handle 1host DMA connected to 2 link DMAs like Pierre said. We need to do this at both ends whenever the last endpoint of the connected is released
There was a problem hiding this comment.
@ujfalusi this will definitely not suffice with aggregated amps with SDW.
There was a problem hiding this comment.
Yes, I will revisit as soon as have time for it. Switching from DSP to DSPless/hda-legacy has been broken for a long time now, it is a developer feature and bugs only me ;)
There was a problem hiding this comment.
Checked the code and the channel will be coupled if the link is released later or in case of multiple link DMA use.
We just missing this from the host side.
@ujfalusi what issue are we trying to fix with this PR? |
switching from DSP mode to DSPless mode (or to legacy HDA) does not work without rebooting the device because we leave random channels decoupled. |
Couple the Host and Link DMA when the stream is put. When the hda_dsp_stream_hw_free() is called the Link DMA might be still locked and we would leave the DMAs decoupled. hda_dsp_stream_hw_free() is not called for code loader use or in case of probes or trace use for example. Signed-off-by: Peter Ujfalusi <[email protected]>
ujfalusi
force-pushed
the
peter/sof/pr/intel-couple-dmas-01
branch
from
baa31e4 to
79dc660
Compare
|
Changes since v2:
|
5 participants
Couple the Host and Link DMA when the stream is put. When the hda_dsp_stream_hw_free() is called the Link DMA might be still locked and we would leave the DMAs decoupled.