audio: dai-zephyr: Do not reset the DMA buffer cursor for HD-DMA on T… #8995
Conversation
…RIGGER_RELEASE During DMA stop/config/start the read/write pointer of HD-DMA is not reset unlike other DMAs (GPDMA, DMAC). Only call the audio_stream_reset() if the link is not serviced by HD-DMA. Link: thesofproject#8986 Fixes: 9831a9d ("audio: dai-zephyr: reset DMA buffer cursors on TRIGGER_RELEASE") Signed-off-by: Peter Ujfalusi <[email protected]>
ujfalusi
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
|
imx build fail with |
Caused by zephyrproject-rtos/zephyr#70219. Fix in #8945 |
only with the Zephyr main branch ("zmain"). Manifest build is fine: https://github.com/thesofproject/sof/actions/runs/8468096003/job/23200277967?pr=8995 |
| */ | ||
| audio_stream_reset(&dd->dma_buffer->stream); | ||
| if (!(dd->dai->dma_caps & DMA_CAP_HDA)) | ||
| audio_stream_reset(&dd->dma_buffer->stream); |
There was a problem hiding this comment.
@LaurentiuM1234 @dbaluta With this fixup in place, this does not look like a solid design anymore. Either SOF can assume something about Zephyr DMA driver behaviour, or it cannot. It does sound like instead of audio_stream_reset, SOF should synchronize its view of DMA driver state (either at start or stop) and we could again have code that works for all (then it's upto DMA drivers how they behave for pause/reset).
Not blocking this PR as this fixes a rather nasty bug with Intel HDA devices, but I think this needs further work. The HW details should be handled on Zephyr side and SOF code should be agnostic.
There was a problem hiding this comment.
ACK. Just curious, why do we need the dma_stop() and dma_config() operations on release? If I recall correctly, an alternative to 9831a9d was to just, well, not do those, which worked on NXP platforms at least? This would have been another way of keeping the buffers consistent with each other (i.e: SOF view and DMA view)
There was a problem hiding this comment.
@LaurentiuM1234, HD-DMA zephyr driver does not support dma_resume(), other than that, I don't know why we do the stop/start here.. We do clear the buffer, so it would kind of make sense to stop/start the DMA, but why we clear the buffer on pause/resume? I don't know.
There was a problem hiding this comment.
Might be worth creating an issue for the stuff @kv2019i mentioned so we can come back to it later on?
|
@dbaluta, @LaurentiuM1234, should we move the call to stream_reset() where we call dma_config() ? |
You mean do it if there's no XRUN? Not sure if I can be of much help here as I've never messed around with the XRUN stuff :( |
OK, let's leave it where it is and move it if needed. |
|
#9002 filed to track the follow-up. Merging the fix. |
…RIGGER_RELEASE
During DMA stop/config/start the read/write pointer of HD-DMA is not reset unlike other DMAs (GPDMA, DMAC).
Only call the audio_stream_reset() if the link is not serviced by HD-DMA.
Link: #8986
Fixes: 9831a9d ("audio: dai-zephyr: reset DMA buffer cursors on TRIGGER_RELEASE")