dai: fix xrun handling #7044

makarukp · 2023-02-06T09:31:24Z

dai copy shuld continue flow if status return xrun warning

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

softwarecki · 2023-02-06T10:45:54Z

dw dma driver also reports -EPIPE when it detect xrun (https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/dma/dma_dw_common.c#L804). Shouldn't we call dai_report_xrun for this driver? In my opinion, in default case should be a general status read error handling, not xrun handling.

softwarecki · 2023-02-06T10:47:01Z

Similar PR: #6965

kv2019i

Some comments inline, added @juimonen to review.

kv2019i · 2023-02-06T11:40:28Z

src/audio/dai-zephyr.c

+			comp_warn(dev, "dai_copy(): dma_get_status() overrun occurred, ret = %u",
+				  ret);
+		break;
+	default:
 		dai_report_xrun(dev, 0);


Hmm, something is still off here. It doesn't make sense to not call "dai_report_xrun()" when "-EPIPE" is returned.

I was looking at the old non-Zephyr driver sof/src/drivers/intel/hda/hda-dma.c:hda_dma_data_size() is checking for overrun/underrun exactly the same way as the new Zephyr driver (checking DGCS_BUR and DGCS_BOR).
And in dai-lecy.c:dai_copy(), the logic is similar. dma_get_data_size_legacy() is called and if xrun is detected, an error is reported.

And with this stack, tests are passing. So I think we need to understand why the difference here.

@kv2019i IIRC calling dai_report_xrun(dev, 0); would cause the XRUN handler on SOF Linux driver to run (and stop/restart the stream) ? Should this not be something to enable/disable (default) at runtime ?

kv2019i · 2023-02-06T13:25:08Z

@makarup @softwarecki I think I now understand why we have native dai-zephyr.c.

If I run test to a non-connected HDMI PCM node, with non-native HD-DMA driver I get:

[   47.628023] <wrn> dai_comp: comp:1 0x40001 dai_copy(): nothing to copy
[   47.629000] <err> hda_dma: hda_dma_link_check_xrun(): underrun detected
[   47.729056] <err> hda_dma: hda_dma_link_check_xrun(): underrun detected
[   48.648050] <inf> ll_schedule: ll timer avg 2251, max 5396, overruns 0

With native Zephyr driver I get:

[    4.125471] <wrn> dai_comp: comp:1 0x40001 dai_copy(): nothing to copy
[    4.225553] <err> dai_comp: comp:1 0x40001 dai_report_xrun(): underrun due to no data available
[    4.225565] <err> component: comp:1 0x40001 comp_underrun(): dev->comp.id = 262145, source->avail = 384, copy_bytes = 0
[    4.225573] <inf> pipe: pipe:1 0x0 pipe trigger cmd 6
[    4.225588] <err> pipe: pipe:1 0x0 pipeline_copy(): ret = -32, start->comp.id = 262145, dir = 1

So in both cases the xrun is observed (makes sense as display side is not connected in this case), but in non-native case the pipeline is not stopped. Took a while but then a realized hda_dma_link_check_xrun() in the old src/drivers/intel/hda/hda-dma.c
always returns 0 for success, even if xrun is found.

I traced this back to this old commit from 2020:

commit 293dfe2469cb2f29895847cb3dd2bc831e3ea7c3
Author: Tomasz Lauda <tomasz.lauda@linux.intel.com>
Date:   Tue Mar 10 10:09:30 2020 +0100

    hda-dma: refactor xrun handling
    
    Changes behaviour of HDA Link overruns and underruns handling.
    Let's no longer stop the stream, but just report an error.
    It might happen that just after the release buffer is still
    full/not yet empty after the previous run, but it shouldn't
    affect the data, since stream has been paused anyway.

The Linux stack is still depending on this behaviour (the Linux audio servers open all the PCM nodes and for the HDMI nodes, the expectation is that PCM streaming can be started without an error even if no display is connected at the time).

So I think we need to modify xrun handling so that for the HDA link, we just log a warning, but do not stop the pipeline. Other xrun handling should be as before.

makarukp · 2023-02-06T14:51:24Z

@kv2019i @softwarecki
I have similar conclusion, however, now I am not 100% sure if dw_dma_driver xrun meaning is the same as hda dma DGCS_BUR and DGCS_BOR. Is it really xrun or error ???
'
#if CONFIG_DMA_DW_HW_LLI
if (!(dw_read(dev_cfg->base, DW_DMA_CHAN_EN) & DW_CHAN(channel))) {
LOG_ERR("xrun detected");
return -EPIPE;
}
#endif
'

I see that in legacy it was only warning for hda but error for dw_dma. Maybe in cases different than standard xrun resulting from delay it should be reported as an error.

What do you think ?

kv2019i · 2023-02-06T18:13:35Z

@makarukp Ack, I can confirm that. With non-native DW-DMA driver, xruns are reported and will stop the pipe. So this special handling is only for the HD-DMA link DMA. Now for Linux, we do need to keep the special handling, and the move of drivers to Zephyr does pose a new challenge how to maintain. Hmm, maybe we put the xrun-raises-an-error logic behind a Kconfig option and otherwise just emit a warning to FW log...?

lgirdwood

Just some minor comments

lgirdwood · 2023-02-07T13:05:39Z

src/audio/dai-zephyr.c

+		/* DMA status can return -EPIPE and current status content if xrun occurs */
+		if (ret == -EPIPE)
+#if XRUN_RAISES_AN_ERROR
+			dai_report_xrun(dev, 0);


and this will eventually call the IPC3/4 version of the notification in subsequent PRs.

no action needed here, btw, just making an observation.

It would be good to enclose the contents of if in {}. We have several levels of if that depends on #if. It's easy to make a mistake.

Hmm. @makarukp @softwarecki @lgirdwood I wonder if we should start by just making the xrun handling here emit warnings and not call dai_report_xrun() yet. That would allow to unblock Zephyr upstream updated.

The CONFIG_NOTIFY_HOST_ON_XRUN is very misleading as no notifies are sent to host in current code. What dai_report_xrun() is to escalate the problem to pipeline level, which will try to restart the pipeline. There is also another layer of conditional logic after dai_report_xrun(). E.g. "NO_XRUN_RECOVERY" option in src/audio/pipeline/pipeline-xrun.c at the pipeline level.

So seems more time is needed to sort out how we introduce the IPC4 notification mechanisms (which are new) and how we tackle the problem that HDA link xruns happen in common use-cases and in the past we have ignored them.

We still have xrun logic at the pipeline level driven by buffering status, so it's not like we are disabling xrun handling completely.

src/audio/dai-zephyr.c

src/audio/Kconfig

kv2019i

I propose we simplify even further and just emit warnings and do not call dai_xrun_report() yet. There seems to be more opens (see inline).

kv2019i · 2023-02-07T15:41:35Z

src/audio/dai-zephyr.c

+		/* DMA status can return -EPIPE and current status content if xrun occurs */
+		if (ret == -EPIPE)
+#if XRUN_RAISES_AN_ERROR
+			dai_report_xrun(dev, 0);


Hmm. @makarukp @softwarecki @lgirdwood I wonder if we should start by just making the xrun handling here emit warnings and not call dai_report_xrun() yet. That would allow to unblock Zephyr upstream updated.

The CONFIG_NOTIFY_HOST_ON_XRUN is very misleading as no notifies are sent to host in current code. What dai_report_xrun() is to escalate the problem to pipeline level, which will try to restart the pipeline. There is also another layer of conditional logic after dai_report_xrun(). E.g. "NO_XRUN_RECOVERY" option in src/audio/pipeline/pipeline-xrun.c at the pipeline level.

So seems more time is needed to sort out how we introduce the IPC4 notification mechanisms (which are new) and how we tackle the problem that HDA link xruns happen in common use-cases and in the past we have ignored them.

We still have xrun logic at the pipeline level driven by buffering status, so it's not like we are disabling xrun handling completely.

lgirdwood · 2023-02-07T16:31:40Z

I propose we simplify even further and just emit warnings and do not call dai_xrun_report() yet. There seems to be more opens (see inline).

Fine by me.

lyakh · 2023-02-08T09:16:11Z

src/audio/dai-zephyr.c

+					  "ret = %u", ret);
+			else
+				comp_warn(dev, "dai_copy(): dma_get_status() overrun occurred, "
+					  "ret = %u", ret);


I think you need braces here. GCC used to complain about constructs like

if (a) if (b) do_x(); else do_y();

because of the potential ambiguity of the else (this ain't no python). This probably wasn't compiled with CONFIG_NOTIFY_HOST_ON_XRUN disabled

kv2019i

Ok with the fix now, but if you are refreshing, a few style comments to the Kconfig option to make it more understandable.

kv2019i · 2023-02-08T09:33:37Z

src/audio/Kconfig

+	help
+	  Select for handling DMA xruns and reporting them as errors.
+	  Otherwise, only warnings are emitted.
+


I won't block on this, but if we have the Kconfig, I'd do:

put it at the end of audio/Kconfig after WRAP_ACTUAL_POSITION (now it's in middle of SRC related options)

name it DAI_REPORT_XRUNS

description: "Report DAI overrun/underruns to pipeline"

This would better reflect what this does.

makarukp · 2023-02-08T12:09:53Z

I propose we simplify even further and just emit warnings and do not call dai_xrun_report() yet. There seems to be more opens (see inline).
@lgirdwood @kv2019i
Does it mean i could remove Kconfig dependency and simplify it to below example ?

switch (ret) {
case 0:
        break;
case -EPIPE:
        tr_warn()
default:
        return ret

kv2019i

@makarukp You have unrelated rimage changes in the updated commit.

Otherwise looks good to go, thanks!

lgirdwood · 2023-02-09T12:31:09Z

@makarukp can you check the CI. Thanks !

src/audio/dai-zephyr.c

comment addressed. Since I personally haven't followed closely the decision to remove xrun reporting completely in that case, I won't explicitly approve, but just remove my request for change

lgirdwood · 2023-02-10T15:42:47Z

@makarukp can you check CI, thanks !

kv2019i · 2023-02-13T11:59:22Z

@wszypelt This one seems to have hit unexpected errors as well.

src/audio/dai-zephyr.c

dai copy should continue with flow if status returns xrun warning Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>

kv2019i · 2023-02-15T12:46:10Z

@wszypelt @makarukp This time it seems an unrelated fail running "git clone" , can you check and agree how to best handle (repush or rerun of CI).

kv2019i · 2023-02-15T16:09:16Z

Now merged! Thank you @wszypelt @makarukp @lyakh for helping to get this across the finish line.

makarukp requested review from lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners February 6, 2023 09:31

makarukp requested a review from pblaszko February 6, 2023 09:33

makarukp force-pushed the dai_xrun_handling branch from 830837f to dcb70d1 Compare February 6, 2023 09:37

makarukp mentioned this pull request Feb 6, 2023

dma: hda: enable xrun handling zephyrproject-rtos/zephyr#53327

Merged

makarukp force-pushed the dai_xrun_handling branch from dcb70d1 to 8c2e99f Compare February 6, 2023 09:57

kv2019i requested a review from juimonen February 6, 2023 11:35

kv2019i requested changes Feb 6, 2023

View reviewed changes

kv2019i mentioned this pull request Feb 6, 2023

[UPDATE ZEPHYR] zephyr: dai: Check error code before xrun report #6965

Closed

makarukp force-pushed the dai_xrun_handling branch from 8c2e99f to 3f2bbe8 Compare February 7, 2023 12:22

lgirdwood reviewed Feb 7, 2023

View reviewed changes

makarukp force-pushed the dai_xrun_handling branch 3 times, most recently from 7c26530 to 2f9e198 Compare February 7, 2023 15:10

kv2019i requested changes Feb 7, 2023

View reviewed changes

lyakh reviewed Feb 8, 2023

View reviewed changes

kv2019i approved these changes Feb 8, 2023

View reviewed changes

kv2019i mentioned this pull request Feb 8, 2023

app: zephyr: add CONFIG_ASSERT debug overlay #6530

Merged

13 tasks

marc-hb added bug Something isn't working as expected P1 Blocker bugs or important features labels Feb 8, 2023

makarukp force-pushed the dai_xrun_handling branch 2 times, most recently from 27b1f4c to ade5593 Compare February 9, 2023 08:30

kv2019i reviewed Feb 9, 2023

View reviewed changes

makarukp force-pushed the dai_xrun_handling branch from ade5593 to 2a2c213 Compare February 9, 2023 08:41

softwarecki approved these changes Feb 9, 2023

View reviewed changes

dnikodem approved these changes Feb 9, 2023

View reviewed changes

lgirdwood approved these changes Feb 9, 2023

View reviewed changes

tmleman approved these changes Feb 9, 2023

View reviewed changes

makarukp force-pushed the dai_xrun_handling branch from 2a2c213 to c9722df Compare February 9, 2023 14:01

lyakh previously requested changes Feb 10, 2023

View reviewed changes

src/audio/dai-zephyr.c Show resolved Hide resolved

makarukp force-pushed the dai_xrun_handling branch from c9722df to 0ab49b0 Compare February 10, 2023 09:21

kv2019i requested a review from lyakh February 10, 2023 11:15

lgirdwood approved these changes Feb 10, 2023

View reviewed changes

makarukp force-pushed the dai_xrun_handling branch from 0ab49b0 to 1de9083 Compare February 14, 2023 09:51

lyakh requested changes Feb 14, 2023

View reviewed changes

src/audio/dai-zephyr.c Outdated Show resolved Hide resolved

dai: fix xrun handling

485bd6d

dai copy should continue with flow if status returns xrun warning Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>

makarukp force-pushed the dai_xrun_handling branch from 1de9083 to 485bd6d Compare February 14, 2023 12:21

lyakh approved these changes Feb 14, 2023

View reviewed changes

kv2019i merged commit 5b8c622 into thesofproject:main Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dai: fix xrun handling #7044

dai: fix xrun handling #7044

makarukp commented Feb 6, 2023

softwarecki commented Feb 6, 2023

softwarecki commented Feb 6, 2023

kv2019i left a comment

kv2019i Feb 6, 2023

lgirdwood Feb 6, 2023

kv2019i commented Feb 6, 2023

makarukp commented Feb 6, 2023

kv2019i commented Feb 6, 2023

lgirdwood left a comment

lgirdwood Feb 7, 2023

lgirdwood Feb 7, 2023

softwarecki Feb 7, 2023 •

edited

kv2019i Feb 7, 2023

kv2019i left a comment

kv2019i Feb 7, 2023

lgirdwood commented Feb 7, 2023

lyakh Feb 8, 2023

kv2019i left a comment

kv2019i Feb 8, 2023

makarukp commented Feb 8, 2023 •

edited

kv2019i left a comment

lgirdwood commented Feb 9, 2023

lgirdwood commented Feb 10, 2023

kv2019i commented Feb 13, 2023

kv2019i commented Feb 15, 2023

kv2019i commented Feb 15, 2023

dai: fix xrun handling #7044

dai: fix xrun handling #7044

Conversation

makarukp commented Feb 6, 2023

softwarecki commented Feb 6, 2023

softwarecki commented Feb 6, 2023

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Feb 6, 2023

makarukp commented Feb 6, 2023

kv2019i commented Feb 6, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarecki Feb 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Feb 7, 2023

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makarukp commented Feb 8, 2023 • edited

kv2019i left a comment

Choose a reason for hiding this comment

lgirdwood commented Feb 9, 2023

lgirdwood commented Feb 10, 2023

kv2019i commented Feb 13, 2023

kv2019i commented Feb 15, 2023

kv2019i commented Feb 15, 2023

softwarecki Feb 7, 2023 •

edited

makarukp commented Feb 8, 2023 •

edited