START / RELEASE from task by lyakh · Pull Request #4559 · thesofproject/sof

lyakh · 2021-07-27T20:25:00Z

move START and RELEASE triggers to the pipeline task

lgirdwood

Have you managed to test the xrun logic yet by injecting xruns ?

lgirdwood · 2021-07-28T07:28:12Z

src/audio/pipeline/pipeline-stream.c

This looks like it should be a separate function ?

sorry, missed this one, let me do that

lgirdwood · 2021-07-28T07:33:13Z

src/audio/pipeline/pipeline-schedule.c

{ should be on the line above.

can you alos comment on what we are doing here when we iterate the list.

lyakh · 2021-07-28T10:04:16Z

Have you managed to test the xrun logic yet by injecting xruns ?

@lgirdwood I just tried that - the test passes but I see no xruns in firmware trace. The way I came across and debugged this is with Zephyr, where I was regularly getting xruns. I can re-try that. Also note, that that part of this PR has been moved over to #4562

lyakh · 2021-08-05T06:04:11Z

This is an experimental push: only for testing of preparatory patches, because even they are rather intrusive. Note, that I expect topologies with mux/demux pipelines to fail at least pause-resume tests.

lyakh · 2021-08-05T14:46:49Z

Can these CI kernel errors on CML_RVP_SDW be related?

[ 1195.770174] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 0
[ 1195.771229] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 1
[ 1195.772831] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 2
[ 1195.774208] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 3
[ 1195.775841] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 4
[ 1195.777187] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 5
[ 1195.778814] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 6
[ 1195.780151] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 7
[ 1195.781778] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 8
[ 1195.783108] kernel: soundwire_intel soundwire_intel.link.1: intel_resume: MCP_CONTROL_HW_RST is not cleared at iteration 9
[ 1195.784736] kernel: soundwire_intel soundwire_intel.link.1: intel_resume failed: MCP_CONTROL_HW_RST is not cleared
[ 1200.127240] kernel: soundwire_intel soundwire_intel.link.1: sdw_cdns_clock_stop failed: MCP_CONTROL_HW_RST is not cleared

lgirdwood · 2021-08-05T16:52:02Z

@lyakh I'm seeing lots og GH action internal errors trying to run your tests. Will try again later.

lgirdwood · 2021-08-05T21:05:56Z

src/include/sof/audio/component.h

STANDBY probably not best naming PRE_ACTIVE is better (also means we can have POST_ACTIVE if needed).

STANDBY is definitely not the best :-)

lgirdwood · 2021-08-05T21:39:32Z

src/ipc/ipc3/handler.c

We should call this PRE_START aligns with PRE_ACTIVE

yeah, I thought the names weren't perfect. And I thought about "pre_start" too, but there's already ssp_pre_start() at least which I now call from ssp_early_start(). I could rename both eventually of course, it's just a static function

src/audio/pipeline/pipeline-stream.c

src/ipc/ipc3/handler.c

lgirdwood · 2021-08-06T15:07:38Z

@kkarask we are struggling to figure out the failures in internal CI around muxdemux tests. Could you post the topology used in both failing tests. This will help us work out the issue. Thanks !

lyakh · 2021-08-06T15:07:55Z

I see internal Intel MUX and DEMUX CI tests failing. The reason is a non-trivial mux state machine in mux_trigger(). mixer_trigger() is also non-trivial and indeed I had to update it for this PR. However, I tested with upstream DEMUX pipelines and they worked with no modifications needed to mux_trigger(). So, I think there might be a problem with those internal tests themselves.

wszypelt · 2021-08-10T08:38:27Z

@lgirdwood
Topology for 14_00 mux simple:

    pipe_plb
+----------------------+
| +-------+   +------+ |
| | Host1 |-->| Buff |----+
| +-------+   +------+ |  |      pipe_plb
+----------------------+  |  +--------------------------------+
                          +---->+-----+   +------+   +-----+  |     +------+
                             |  | Mux |-->| Buff |-->| Dai |------->| SSPx |
    pipe_plb              +---->+-----+   +------+   +-----+  |     +------+
+----------------------+  |  +--------------------------------+        |
| +-------+   +------+ |  |                                            |
| | Host2 |-->| Buff |----+                                            |
| +-------+   +------+ |                                               |
+----------------------+                                               |
                                                                       |
                                                                       |
    pipe_cap                                                           |
+-------------------------------+                                      |
| +------+   +------+   +-----+ |   +------+                           |
| | Host |<--| Buff |<--| Dai |<----| SSPx |<--------------------------<
| +------+   +------+   +-----+ |   +------+
+-------------------------------+

Topology for 14_04 demux synchronized capture:

    pipe_plb
+-------------------------------+
| +------+   +------+   +-----+ |                                                 +------+
| | Host |-->| Buff |-->| Dai |-------------------------------------------------->| SSPx |
| +------+   +------+   +-----+ |                                                 +------+
+-------------------------------+                                                    |
                                                                                     |
                                                                                     |
    pipe_cap                                                                         |
+----------------------------------------------------------------------------+       |
| +-------+   +------+   +-----+   +------+   +-------+   +------+   +-----+ |    +------+
| | Host1 |<--| Buff |<--| Vol |<--| Buff |<--| DeMux |<--| Buff |<--| Dai |<-----| SSPx |
| +-------+   +------+   +-----+   +------+   +-------+   +------+   +-----+ |    +------+
+-------------------------------------------------|--------------------------+
                                                  |
    pipe_cap                                      |
+-------------------------------------------+     |
| +-------+   +------+   +-----+   +------+ |     |
| | Host2 |<--| Buff |<--| Vol |<--| Buff |<------+
| +-------+   +------+   +-----+   +------+ |
+-------------------------------------------+

lyakh · 2021-08-26T19:52:37Z

SOFCI TEST

plbossart

@lyakh I have a limited understanding of this PR, specifically why you need the init_delay, but the fundamental question is why we add support for PRE without having a matching POST set of operations.

src/audio/component.c

src/audio/mixer.c

plbossart · 2021-08-27T12:51:51Z

src/audio/pipeline/pipeline-stream.c

no following what this does.

we're adding an intermediate "PRE" trigger command to the "START" and "RELEASE" IPCs.

plbossart · 2021-08-27T12:54:38Z

src/drivers/intel/ssp/ssp.c

well, if we are going to fix things: does it make sense to use a spin_lock around a wait_delay?

of course, this is done when the delay is removed from the SSP driver into the scheduler

The test should be that 2 consecutive stream wont extend the delay or cancel the delay. i.e. opening playback then capture on SSP0 codec.

bingo, this is actually a fun one! I confess - I didn't think about it, and I didn't know what happens in such a case. So, I ran playback and simultaneously capture on the same SSP. And I see with the current tip of the tree, that in such a case ssp_start() is called for both streams and therefore, if .bclk_delay != 0 the driver will wait twice. The same would happen after my patches. This probably doesn't make sense. This isn't a bug introduced by this PR but I guess I need to fix this...

ok, lets track this with a feature

src/drivers/intel/ssp/ssp.c

plbossart · 2021-08-27T12:58:00Z

src/include/sof/lib/dai.h

I don't understand if this represents a measured delay or a specified delay. If this is measured, why was it needed?

me neither. It's there before this PR and IIRC removing it breaks SSP on some platforms

well in the initial code it is a specified delay. The topology asks the firmware, using the kernel driver as a proxy, to keep a delay between the SSP start and DMA start.

Now it's unclear to me what you want to do with the delay. Is this just about moving the wait outside of the driver? Or is this to compensate for the delay somehow, in which case you'd want the measured and actual wait.

This is the spin_for_many_miliseconds_and_block_other_work delay. It's being folded into the higher level synchronised LL work. i.e. the LL pipeline will schedule the BCLK before the rest of the DAI and the pipeline start (and unblock all other work on the core).

early_start_length_ms should also be a uint32_t too.

length doesn't seem like a good name for this. early_start_delay_ms?

plbossart · 2021-08-27T12:58:43Z

src/drivers/intel/ssp/ssp.c

what are you going to do with the information?

see pipeline_comp_trigger() and pipeline_trigger_run() in https://github.com/thesofproject/sof/blob/8856f664e9511c410e530271328e08bc9764abe3/src/audio/pipeline/pipeline-stream.c specifically:

if (data.delay && pipeline_is_timer_driven(p)) { /* The task will skip .delay periods before processing the next command */ p->trigger.delay = (data.delay * 1000 + p->period - 1) / p->period; p->trigger.cmd = data.cmd; return ret; }

and pipeline_task() in https://github.com/thesofproject/sof/blob/8856f664e9511c410e530271328e08bc9764abe3/src/audio/pipeline/pipeline-schedule.c:

if (p->trigger.delay) { p->trigger.delay--; return SOF_TASK_STATE_RESCHEDULE; }

lyakh · 2021-08-27T13:10:18Z

@lyakh I have a limited understanding of this PR, specifically why you need the init_delay, but the fundamental question is why we add support for PRE without having a matching POST set of operations.

@plbossart because that's what the SSP driver currently has - a delay in its initialisation path. The initial version of this PR didn't touch that delay, it only moved the START / RELEASE triggers into the pipeline task. But that delay then broke tests because the whole scheduler was blocked on it. To solve that we added a new "PRE_ACTIVE" state. Now there's no ad-hoc waiting in SSP any more, the SSP driver just tells the framework that it needs a delay between PRE_START and START and the scheduler counts how many ticks it has to skip between those states.

lgirdwood

CI looking good for internal CI now :)

lgirdwood · 2021-08-27T16:06:23Z

src/include/sof/audio/component.h

We should have a big comment here describing the usage and have the POST conditions too (even if we dont use them today).

lgirdwood

Almost there, some opens though but mostly minor.

src/include/sof/audio/component_ext.h

tools/testbench/common_test.c

src/drivers/imx/sai.c

src/drivers/intel/ssp/ssp.c

lgirdwood · 2021-08-30T15:14:33Z

src/include/sof/audio/pipeline.h

what delay - can we have acomment.

lgirdwood · 2021-08-30T15:16:08Z

src/include/sof/lib/dai.h

early_start_length_ms should also be a uint32_t too.

lgirdwood · 2021-08-30T15:18:37Z

src/drivers/intel/ssp/ssp.c

The test should be that 2 consecutive stream wont extend the delay or cancel the delay. i.e. opening playback then capture on SSP0 codec.

src/ipc/ipc3/handler.c

We need to split the START trigger command into two commands because some components have a long delay inside their START handling. This patch introduces two new trigger commands: PRE_START and PRE_RELEASE and a new state PRE_ACTIVE to prepare for that split. For simmetry POST_STOP and POST_PAUSE are also added, however they aren't used yet. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Some DAI drivers have to perform additional preparatory operations before START and RELEASE triggers, pass PRE_* triggers down to them for that purpose. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

The SSP trigger handler has a potentially long delay when processing START and RELEASE triggers. Split off an early start trigger to extract that delay out of the SSP driver into the trigger state machine. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Some DAI devices need a delay between their PRE_START and START trigger commands, and similarly between PRE_RELEASE and RELEASE. Add a DAI driver operation to get that delay time and use it between the two commands. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Implement BCLK initialisation delay using the .get_init_delay() operation. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

When the firmware receives a START or RELEASE IPC message, it immediately triggers all involved components, which starts DMA. Then it schedules the pipeline task, but since the scheduler can be already running at that time, the task might be scheduled when DMA data isn't available yet or has already overflowed. To fix this change the control flow to also trigger all components from the task during its first run. Actual data processing then begins with the next period. Note, that this is currently only possible with pipelines, using timer-based scheduling. Pipelines, using DMA interrupts for scheduling are unaffected. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

comp_set_state() always sets the component state to requested state, which is calculated at the top of the function. Re-use the variable instead of hard-coding each value again. Also return immediately in case of errors, which eliminates the need for the return code variable. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Trace prints often contain open-coded function names in them. When functions get renamed or trace prints are moved to other functions, those strings sometimes are forgotten. Fix two such cases. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

The initialisation delay in SSP start-up sequence is only needed when the first stream initialises the SSP. If an SSP port is used for both playback and capture, the second stream doesn't need to delay start up again. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lgirdwood · 2021-08-31T20:14:30Z

One CML DUT not booting but other CML passing.

lgirdwood reviewed Jul 28, 2021

View reviewed changes

lyakh force-pushed the pipe_2 branch from 845f9c5 to 9f080e4 Compare July 28, 2021 10:08

lyakh changed the title ~~XRUN, STOP / PAUSE from task~~ STOP / PAUSE from task Jul 28, 2021

lyakh force-pushed the pipe_2 branch from 9f080e4 to fe5381f Compare August 5, 2021 06:02

lyakh force-pushed the pipe_2 branch from c6111b9 to cd47b45 Compare August 5, 2021 15:13

lyakh force-pushed the pipe_2 branch from ff2c8d5 to f871916 Compare August 5, 2021 20:33

lgirdwood reviewed Aug 5, 2021

View reviewed changes

lyakh mentioned this pull request Aug 6, 2021

compile: use __attribute__((fallthrough)) with GCC #4607

Merged

lyakh force-pushed the pipe_2 branch 6 times, most recently from d3f7d5e to 8d9eac1 Compare August 6, 2021 14:55

lyakh changed the title ~~STOP / PAUSE from task~~ START / RELEASE from task Aug 6, 2021

lyakh marked this pull request as ready for review August 6, 2021 14:56

lyakh requested review from bardliao, dbaluta, lbetlej, mmaka1 and plbossart as code owners August 6, 2021 14:56

lgirdwood added this to the v1.9 milestone Aug 6, 2021

lgirdwood added the P1 Blocker bugs or important features label Aug 6, 2021

lyakh force-pushed the pipe_2 branch from 8d9eac1 to 8c1f30a Compare August 25, 2021 14:18

lyakh force-pushed the pipe_2 branch from 8c1f30a to fa78009 Compare August 27, 2021 09:21

lyakh requested a review from ranj063 as a code owner August 27, 2021 09:21

lyakh force-pushed the pipe_2 branch from fa78009 to 8eac26e Compare August 27, 2021 12:59

lyakh requested a review from akloniex as a code owner August 27, 2021 12:59

plbossart requested changes Aug 27, 2021

View reviewed changes

lyakh force-pushed the pipe_2 branch from 8eac26e to d73f365 Compare August 27, 2021 14:46

lyakh requested a review from plbossart August 27, 2021 15:26

lgirdwood reviewed Aug 27, 2021

View reviewed changes

lyakh force-pushed the pipe_2 branch from d73f365 to cf0faf3 Compare August 30, 2021 10:19

lgirdwood reviewed Aug 30, 2021

View reviewed changes

lyakh added 3 commits August 31, 2021 15:42

dai: pass PRE_START and PRE_RELEASE down to DAI drivers

4e24e69

Some DAI drivers have to perform additional preparatory operations before START and RELEASE triggers, pass PRE_* triggers down to them for that purpose. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the pipe_2 branch from cf0faf3 to 4acbb7f Compare August 31, 2021 14:15

lyakh added 6 commits August 31, 2021 16:53

ssp: move initialisation delay to .get_init_delay()

74bc0b1

Implement BCLK initialisation delay using the .get_init_delay() operation. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the pipe_2 branch from 4acbb7f to f02c2da Compare August 31, 2021 14:53

lgirdwood approved these changes Aug 31, 2021

View reviewed changes

lgirdwood merged commit 8fa5ff5 into thesofproject:main Aug 31, 2021

lyakh deleted the pipe_2 branch September 1, 2021 06:05

This was referenced Sep 1, 2021

[TEST] Trig test #4685

Closed

[BUG]underrun due to no data available when multiple-pipeline on CML_HEL_RT5682 #4560

Closed

Conversation

lyakh commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jul 28, 2021

Uh oh!

lyakh commented Aug 5, 2021

Uh oh!

lyakh commented Aug 5, 2021

Uh oh!

lgirdwood commented Aug 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Aug 6, 2021

Uh oh!

lyakh commented Aug 6, 2021

Uh oh!

wszypelt commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Aug 26, 2021

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Aug 27, 2021

Uh oh!

lyakh commented Jul 27, 2021 •

edited

Loading

wszypelt commented Aug 10, 2021 •

edited

Loading