stop all connected pipelines on XRUNs

[lyakh]

with connected pipelines, if an XRUN occurs on one of them, all of them should be stopped

[mrajwa]

Hmm I like to return "ret" more TBH this way we are consistent of always returning the same variable and not magic numbers.

[lyakh]

I usually approach this firstly from the PoV of minimalism, secondly of locality. You don't do

int f(void)
{
	int ret = 0;
	return ret;
}

you just do

int f(void)
{
	return 0;
}

I think this is similar. Locality - when I look at that line I immediately know what happens there. With the previous version I have to check 9 lines up to see where ret comes from and whether it was modified in between. So, sorry, no, I prefer this version.

[mrajwa]

Both approaches can be considered better or worse... I think we should create a macro or enum which maps 0 with some meaningful success constant like in CAVS we have ADSP_SUCCESS. Anyway, if you feel like returning 0 instead of defined ret is more appropriate then OK, this is not critically important tbh.

[mrajwa]

magic number

[lyakh]

any non-zero should do, the value isn't used anyway. I can add a comment.

[mrajwa]

I see but can't we just assign exact XRUN_BYTES here? Even though currently we treat it as a boolean value at some point we may need exact value - this veriable was defined as 32 bit one so this suggest me that was the original intention.

[mrajwa]

there should be a comment informing that fall thru is deliberate

[lyakh]

no, in such trivial cases of several case statements following each other with no code between them, no COMPILER_FALLTHROUGH is needed - see line 304 below.

[mrajwa]

error message is not logical, "No active pipeline found to continue pipeline processing" maybe?

[lyakh]

well, firstly "this error should never happen" (TM), if anyone ever sees it, there's a problem. We could put an assert() there but I don't think we do that for this kind of errors. And no, it isn't processing, it's rather linking. I'll adjust

[lyakh]

re-tested with Zephyr xrun recipe and found out that a change to my previous PR #4471 conflicted with this one, so I had to account for it. Now actually tested to work again. Before this PR an XRUN looked like:

78965599 dw_dma_get_data_size(): xrun detected
78966958 dai_report_xrun(): underrun due to no data available
78968638 comp_underrun(): dev->comp.id = 26, source->avail = 768, copy_bytes = 0
78970933 pipeline_trigger_run(): ret = -22, host->comp.id = 24, cmd = 8
78973461 pipeline_xrun(): Pipelines notification about XRUN failed, ret = -22
78975623 perf comp_copy peak plat 10048 cpu 10050
78977209 pipeline_copy(): ret = -61, start->comp.id = 26, dir = 1
78979110 pipeline_task(): xrun recovery failed! pipeline is stopped.
78980952 task complete 0x9e0563e8 0x1fffa360U
78982507 num_tasks 7 total_num_tasks 7

now it looks like

6768997 dw_dma_get_data_size(): xrun detected
6770339 dai_report_xrun(): underrun due to no data available
6771992 comp_underrun(): dev->comp.id = 26, source->avail = 768, copy_bytes = 0
6774244 pipe trigger cmd 8
6775569 perf comp_copy peak plat 6621 cpu 6621
6777100 pipeline_copy(): ret = -61, start->comp.id = 26, dir = 1
6778977 pipeline_task(): xrun recovery failed! pipeline is stopped.
6781251 task complete 0x9e056648 0x1fffa360U
6782827 num_tasks 8 total_num_tasks 8
6805856 task complete 0x9e0565a0 0x1fffa360U
6807443 num_tasks 7 total_num_tasks 7

Note, how in the new version 2 tasks are completed as a result of the XRUN

[lyakh]

@mrajwa could you re-review, please?

[mrajwa]

@lyakh done, in general it is fine for me, only small concern about the xrun_bytes stored as boolean value instead of exact bytes count.

[lyakh]

@lyakh done, in general it is fine for me, only small concern about the xrun_bytes stored as boolean value instead of exact bytes count.

@mrajwa well, if that's a problem, I could try to copy the same xrun_bytes value from the original pipeline to all connected ones. It can be done with scanning the list twice - which shouldn't be a performance issue because the list usually consists of 1 or 2 members. But it's kind of ugly. @lgirdwood what do you think?

[lgirdwood]

@lyakh done, in general it is fine for me, only small concern about the xrun_bytes stored as boolean value instead of exact bytes count.

@mrajwa well, if that's a problem, I could try to copy the same xrun_bytes value from the original pipeline to all connected ones. It can be done with scanning the list twice - which shouldn't be a performance issue because the list usually consists of 1 or 2 members. But it's kind of ugly. @lgirdwood what do you think?

To be honest I dont think the host really uses the xrun size within the individual pipelines but uses the host DMA position to determine where to restart the stream. So probably useful for debug, but not useful for host.

[lyakh]

@lyakh done, in general it is fine for me, only small concern about the xrun_bytes stored as boolean value instead of exact bytes count.

@mrajwa well, if that's a problem, I could try to copy the same xrun_bytes value from the original pipeline to all connected ones. It can be done with scanning the list twice - which shouldn't be a performance issue because the list usually consists of 1 or 2 members. But it's kind of ugly. @lgirdwood what do you think?

To be honest I dont think the host really uses the xrun size within the individual pipelines but uses the host DMA position to determine where to restart the stream. So probably useful for debug, but not useful for host.

@lgirdwood @mrajwa I've just grepped the sources and I only see ->xrun_bytes being used as a flag, compared to 0, never as an actual value. So, unless I'm missing something, setting it to 1 seems safe to me.

[lgirdwood]

@lgirdwood @mrajwa I've just grepped the sources and I only see ->xrun_bytes being used as a flag, compared to 0, never as an actual value. So, unless I'm missing something, setting it to 1 seems safe to me.

Can you check IPC4 messages for any fields relating to glitches or xruns. It may be that IPC4 uses a byte or frame value in reporting stream errors and if so we need to keep.

stale review

[lyakh]

SOFCI TEST

[lyakh]

tests have been hanging since last week, kick them again. Also re-tested locally, it still does the right thing

[lgirdwood]

CI showing one CML DUT is not running but other CML DUT is all green.