Skip to content

dma: hda: enable xrun handling#53327

Merged
nashif merged 1 commit intozephyrproject-rtos:mainfrom
makarukp:hda_dma_xrun_handling
Jan 17, 2023
Merged

dma: hda: enable xrun handling#53327
nashif merged 1 commit intozephyrproject-rtos:mainfrom
makarukp:hda_dma_xrun_handling

Conversation

@makarukp
Copy link
Copy Markdown
Contributor

@makarukp makarukp commented Dec 23, 2022 •
edited by nashif
Loading

This is proposition of enabling link under/overruns handling and reporting such events in dma status.

Signed-off-by: Piotr Makaruk piotr.makaruk@intel.com

@makarukp makarukp mentioned this pull request Dec 23, 2022
Merged
@carlescufi carlescufi added the DNM This PR should not be merged (Do Not Merge) label Dec 23, 2022
@makarukp makarukp force-pushed the hda_dma_xrun_handling branch from 8ecb99a to 4e9424d Compare January 10, 2023 13:10
@makarukp makarukp marked this pull request as ready for review January 10, 2023 13:19
@zephyrbot zephyrbot added platform: Intel ADSP Intel Audio platforms area: DMA Direct Memory Access labels Jan 10, 2023
@nashif nashif assigned teburd and unassigned nashif Jan 11, 2023
@makarukp makarukp changed the title [DO NOT MERGE]dma: hda: enable xrun handling dma: hda: enable xrun handling Jan 11, 2023
teburd
teburd requested changes Jan 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@teburd teburd left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In looking at the sof code for HDA this is logged, cleared, and then ignored. See https://github.com/thesofproject/sof/blob/main/src/drivers/intel/hda/hda-dma.c#L933

GPDMA does not have specific flagging to say there was an under/over run, but that the transfer (an endless loop of them in sof) has stopped by checking the enable bit, signaling an under/over run. https://github.com/thesofproject/sof/blob/main/src/drivers/dw/dma.c#L1126

I think returning -ENODATA here from get_status is good though! The error return is enough to signal an under/over run as the stream direction should be known by the caller.

So lets remove the bool additions from the status struct here, which are going to be unused by all other DMAs in Zephyr today and document the -ENODATA return.

Doing that, +1 from me easy

@makarukp
Copy link
Copy Markdown
Contributor Author

makarukp commented Jan 16, 2023 •
edited
Loading

@teburd
I agree, it can be deduced by SOF from stream direction and such approach seems to be enough.
According to https://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html shouldn't we change -ENODATA to -EPIPE ?

@kv2019i
Copy link
Copy Markdown
Contributor

kv2019i commented Jan 16, 2023

@teburd I agree, it can be deduced by SOF from stream direction and such approach seems to be enough. According to https://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html shouldn't we change -ENODATA to -EPIPE ?

For the error code, SOF has used ENODATA quite a bit, but I do agree EPIPE is probably better. ENODATA is more often used in cases, where the condition is not a fatal error yet, there's just not data yet. EPIPE seems more fitting as signals a more severe problem, which applies here as the DMA did run out of data leading to data loss.

@makarukp
dma: hda: enable xrun handling
1afcc20 
Enable link under/overruns handling and reporting such events in dma
status

Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
@makarukp makarukp force-pushed the hda_dma_xrun_handling branch from 4e9424d to 1afcc20 Compare January 16, 2023 14:45
dabekjakub
dabekjakub reviewed Jan 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@dabekjakub dabekjakub left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this report generic for HDA link? In any case information in the comment on report functions describing where we actually reporting it to would be good I think.

Maybe to be more specific: If I was reading this not knowing the context I would ask myself why we clear on the xrun detect.

@makarukp
Copy link
Copy Markdown
Contributor Author

makarukp commented Jan 17, 2023

Is this report generic for HDA link? In any case information in the comment on report functions describing where we actually reporting it to would be good I think.

Maybe to be more specific: If I was reading this not knowing the context I would ask myself why we clear on the xrun detect.

@dabekjakub Good point. Yes, this is generic for HDA link. From SOF point of view link status is retrieved each time new data is copied from/to link in DAI or Chain DMA interfaces. Once we had a chance to get its status, we clear stat register and this is the only chance to handle xrun. After retrieving xrun we have to clean it to be able to check it during next chunk processing. However, I am a little bit concerning if we should not move reg clearing it to dma_reload... What do you think ?

@teburd teburd removed the DNM This PR should not be merged (Do Not Merge) label Jan 17, 2023
@fabiobaltieri
Copy link
Copy Markdown
Member

fabiobaltieri commented Jan 17, 2023

@makarukp can you update the PR comment as well? Still says "please do not merge"

@nashif nashif merged commit 00b5114 into zephyrproject-rtos:main Jan 17, 2023
@makarukp makarukp mentioned this pull request Jan 19, 2023
Closed
@softwarecki softwarecki mentioned this pull request Jan 19, 2023
Merged
@kv2019i
Copy link
Copy Markdown
Contributor

kv2019i commented Jan 27, 2023

@makarukp @softwarecki @mwasko This causes HDMI tests to fail in SOF CI when we attempt to move to newer Zephyr mainline -> thesofproject/sof#6965

@makarukp
Copy link
Copy Markdown
Contributor Author

makarukp commented Jan 27, 2023

@kv2019i Could you provide any IPC + FW logs for analysis and reproduction?

@kv2019i
Copy link
Copy Markdown
Contributor

kv2019i commented Jan 27, 2023

@makarukp wrote:

@kv2019i Could you provide any IPC + FW logs for analysis and reproduction?

https://sof-ci.01.org/sofpr/PR6965/build3542/devicetest/index.html?model=ADLP_RVP_SDW_IPC4ZPH&testcase=check-playback-3times

@marc-hb
Copy link
Copy Markdown
Contributor

marc-hb commented Jan 27, 2023 •
edited
Loading

Thanks to sof/west.yml it's trivial to submit zephyr PRs to SOF CI even BEFORE they are merged in Zephyr, see recent example in #50136 (comment)

@marc-hb
Copy link
Copy Markdown
Contributor

marc-hb commented Feb 2, 2023 •
edited
Loading

Could this be reverted for now? This is now blocking zephyr upgrades and testing like:

A bit problematic considering Zephyr is now in a pre-release, bug fixing phase.

@mwasko
Copy link
Copy Markdown
Contributor

mwasko commented Feb 3, 2023

Could this be reverted for now? This is now blocking zephyr upgrades and testing like:

A bit problematic considering Zephyr is now in a pre-release, bug fixing phase.

@marc-hb we are currently working to fix this issue. In case of no progress in a next few days we will revert changes.

@makarukp
Copy link
Copy Markdown
Contributor Author

makarukp commented Feb 6, 2023 •
edited by marc-hb
Loading

PR in OSF should solve the issue:

@marc-hb marc-hb mentioned this pull request Feb 8, 2023
Closed
@marc-hb marc-hb mentioned this pull request Feb 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teburd teburd teburd approved these changes

@nashif nashif Awaiting requested review from nashif

@carlescufi carlescufi Awaiting requested review from carlescufi

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@abonislawski abonislawski Awaiting requested review from abonislawski

@dcpleung dcpleung Awaiting requested review from dcpleung

@andyross andyross Awaiting requested review from andyross

@aborisovich aborisovich Awaiting requested review from aborisovich

@ceolin ceolin Awaiting requested review from ceolin

@kv2019i kv2019i Awaiting requested review from kv2019i

@lgirdwood lgirdwood Awaiting requested review from lgirdwood

@lyakh lyakh Awaiting requested review from lyakh

@marc-hb marc-hb Awaiting requested review from marc-hb

@tmleman tmleman Awaiting requested review from tmleman

+2 more reviewers

@dabekjakub dabekjakub dabekjakub left review comments

@mwasko mwasko mwasko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@teburd teburd

Labels

area: DMA Direct Memory Access platform: Intel ADSP Intel Audio platforms

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@makarukp @kv2019i @fabiobaltieri @marc-hb @mwasko @teburd @dabekjakub @nashif @carlescufi @zephyrbot