pipeline param negotiation #2788

keyonjie · 2020-04-17T09:40:59Z

pipeline: add negotiation before propagating params

For different connected pipelines, they are usually connected via a
component with >1 source or sink buffers, as most of those components
don't have ability to converse frame format or sample rate, we need add
a params negotiation mechanism between those pipelines.

If all buffers are iterated in the params propagation direction, no
negotiation needed, otherwise we need to iterate all other buffers in
the opposite direction, to make sure all connected buffers have chance
to take part in the negotiation.

The negotiation should happen before we can propagate the params
further, when doing negotiation in a branch/buffer, we should check:
If a branch is active, check if params is matched, return error to
reject the whole .params() requirement if not.
If a branch is inactive, force update the params to its calling buffer,
to make sure all branches are aligned on the format.

We are propagating the params to branched buffer, and the subsequent
component's .params() or .prepare() should be responsible to calibrate
the buffer params if needed. For example, a component who has different
channels buffers should explicitly configure the channels for its
branched buffers (the ones connected to another pipeline).

lgirdwood

We are missing some documentation here for sof-docs on how this works. @kv2019i is creating a set of concrete pipeline rules and this should be included if approved. @kv2019i how are your pipeline rules coming along ?

lgirdwood · 2020-04-19T18:56:40Z

src/audio/pipeline.c

Can you put the calling_buf in ctx

Sorry, after think it more, @lgirdwood I think it's better to keep this calling_buf outside of the ctx, as the all the other ctx members are constant during the whole path walking, while this calling_buf is keep changing.

So better not to store it in the ctx otherwise it will be lost as the deeper layer will overwrite and break the upper layer value.

Ok, what's the usage of calling_buf. Is it the current or next or previous source/sink buffer in the pipeline ? It sounds like we need to rename to better reflect the usage.

Ok, what's the usage of calling_buf. Is it the current or next or previous source/sink buffer in the pipeline ? It sounds like we need to rename to better reflect the usage.

It is the buffer ahead of the 'current' component, it could be one of the current component's source/sink buffer in the pipeline, depends on what direction we are walking in. the calling_buf is used to check and make sure we are not iterating to the path which we have done to avoid dead loop, for example, in below path, we are walking .params() in this way: b1-> comp 1 -> params negotiation with b2, b3(b1 is excluded here for it is the calling_buf) done --> b3:

b3--------| v b1 -> comp 1 --> b3 ^ b2--------|

Ok, this is really then the "next_buf". To avoid re-walking the same components you should add a member to component or buffer that is a flag for "walked already"

Ok, this is really then the "next_buf". To avoid re-walking the same components you should add a member to component or buffer that is a flag for "walked already"

I think it is not the "next", it should be the "previous"? adding a flag could be a choice, but then we might need to manage state of this flag (e.g. we need to do one more iteration to reset all of them once the walking is done, we even need to do this several times in one .params() of the pipeline, as you may know that the pipeline_comp_hw_params() is doing one more iteration before the real pipeline_comp_hw_params() is called).

DAPM uses a "walked" flag when traversing the graph. It's reset before every walk. Please do the same here, sinec the DAPM version also survives loops (and this PR wont)

We can add this flag to buffer only (no need for component), but to reset before every walk(e.g. .params(), .prepare(), .trigger(), .reset(), .copy()), we need to add a full iteration (to all buffers belong to or connected to the pipeline under iterated) ahead respectively, is this what you expected @lgirdwood ?

keyonjie · 2020-04-20T05:55:54Z

We are missing some documentation here for sof-docs on how this works. @kv2019i is creating a set of concrete pipeline rules and this should be included if approved. @kv2019i how are your pipeline rules coming along ?

Sure, I am preparing documentation on how this works, I can add it to sof-docs when @kv2019i's one is available.

mmaka1 · 2020-04-27T14:55:02Z

src/audio/pipeline.c

It runs in the ppl_data->p context, so use pipe_dbg(ppl_data->p, "....", ...) to include the pipeline id in the trace messages.

ok, I just followed the existed ones in pipeline_comp_params() and pipeline_comp_hw_params(), fwiw, let me change it to pipe_dbg.

mmaka1 · 2020-04-27T14:55:44Z

src/audio/pipeline.c

Use pipe_err()

mmaka1 · 2020-04-27T14:59:52Z

src/include/sof/audio/buffer.h

Trace message does not explain what happened. I am also not sure whether this is real run-time error, expected scenario (the message should be a warning at most then), or a logic error (and assert() should be used here)?

@mmaka1 Asserts cannot be used as sole verification, because they can be disabled. Not yet, but once we will move to Zephyr we will be able to disable asserts. So if it's a logic error, we shouldn't have just assert but also this trace and return.

And actually... We shouldn't assert on parameters that are controlled by the Linux driver, but only internal values. Asserting on an external guarantee (something the Linux driver "guarantees" is never a good idea, because mismatched versions will occur).

@paulstelian97 I assume that asserts may be compiled out of the production code, so they should be use to catch logic errors only (those coming from coding issues) during the code bringup and debug phase. Any data coming from the driver should be verified at run-time. However I asked about this particular case, afair the parameters in this flow are internally computed by the fw, so I asked to double check whether tracing run-time error makes sense in this case. Still not sure what response is expected when seeing this error message in the trace log.

If we don't know for certain this is internally computed, assume it isn't. That's the thing.

@mmaka1 @paulstelian97 Thanks for the comments.

I was not sure how the helper will be used and what params and buffer it could be passed in, so I treated it as run-time error here.

But after thought about it more now, as @mmaka1 stated, here what we tried to catch here is the code logic error(e.g. some calling to buffer_params_match() with NULL buffer or params) but not the run-time params errors from the Linux kernel side, the response should be halting the execution of the FW but not just returning an false when this error happen, so changing it to 'assert' handling looks good to me.

keyonjie · 2020-05-07T01:48:51Z

@paulstelian97 @mmaka1 @lgirdwood comments are addressed, please help to re-review, thanks.

lgirdwood · 2020-05-07T16:54:59Z

@keyonjie is this aligned with the pipline params rules that are being written by @mmaka1 ?

keyonjie · 2020-05-08T14:05:57Z

@keyonjie is this aligned with the pipline params rules that are being written by @mmaka1 ?

I think so, @mmaka1 will work for more complicate cases on top of this.

bkokoszx

@keyonjie
We can see that our TestDemuxSynchronizedCapture48000Hz24b32b4ch are failing. Those tests are using following topology:

     pipe_plb
    +-------------------------------+
    | +------+   +------+   +-----+ |                                                 +------+
    | | Host |-->| Buff |-->| Dai |-------------------------------------------------->| SSPx |
    | +------+   +------+   +-----+ |                                                 +------+
    +-------------------------------+                                                    |
                                                                                         |
                                                                                         |
        pipe_cap                                                                         |
    +----------------------------------------------------------------------------+       |
    | +-------+   +------+   +-----+   +------+   +-------+   +------+   +-----+ |    +------+
    | | Host1 |<--| B1.0 |<--| Vol |<--| B1.1 |<--| DeMux |<--| B1.2 |<--| Dai |<-----| SSPx |
    | +-------+   +------+   +-----+   +------+   +-------+   +------+   +-----+ |    +------+
    +-------------------------------------------------|--------------------------+
                                                      |
        pipe_cap                                      |
    +-------------------------------------------+     |
    | +-------+   +------+   +-----+   +------+ |     |
    | | Host2 |<--| B2.0 |<--| Vol |<--| B2.1 |<------+
    | +-------+   +------+   +-----+   +------+ |
    +-------------------------------------------+

, where capture Host1 has 48 kHz, 16/16 frame_fmt, 4 channels stream and Host2 has 48 kHz, 16/16 frame_fmt, 8 channels.
It is failing on a captured streams verification. This happens, because we do not have B1.1 well configured. Firstly, we send pcm params to Host1 and configure B1.1 with 4 channels stream, next we send pcm params to Host2 and in pipeline_comp_params_neg() we overwrite ```B1.1`` with 8 channels, which results in a mismatch in channels parameter.
In my opinion, we should add condition ensuring that we wouldn't negotiate params between pipelines working in the same direction. Something like:

diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c
index c381174b2..d7a8701eb 100644
--- a/src/audio/pipeline.c
+++ b/src/audio/pipeline.c
@@ -388,6 +388,10 @@ static int pipeline_comp_params_neg(struct comp_dev *current,
        pipe_dbg(ppl_data->p, "pipeline_comp_params_neg(), current->comp.id = %u, dir = %u",
                 dev_comp_id(current), dir);

+       if (current->state == COMP_STATE_PREPARE &&
+           current->direction == ppl_data->params->params.direction)
+               return err;
+
        /* return if current is running */
        if (current->state == COMP_STATE_ACTIVE) {
                if (buffer_params_match(calling_buf,

keyonjie · 2020-05-12T04:52:38Z

@keyonjie
We can see that our TestDemuxSynchronizedCapture48000Hz24b32b4ch are failing. Those tests are using following topology:

     pipe_plb
    +-------------------------------+
    | +------+   +------+   +-----+ |                                                 +------+
    | | Host |-->| Buff |-->| Dai |-------------------------------------------------->| SSPx |
    | +------+   +------+   +-----+ |                                                 +------+
    +-------------------------------+                                                    |
                                                                                         |
                                                                                         |
        pipe_cap                                                                         |
    +----------------------------------------------------------------------------+       |
    | +-------+   +------+   +-----+   +------+   +-------+   +------+   +-----+ |    +------+
    | | Host1 |<--| B1.0 |<--| Vol |<--| B1.1 |<--| DeMux |<--| B1.2 |<--| Dai |<-----| SSPx |
    | +-------+   +------+   +-----+   +------+   +-------+   +------+   +-----+ |    +------+
    +-------------------------------------------------|--------------------------+
                                                      |
        pipe_cap                                      |
    +-------------------------------------------+     |
    | +-------+   +------+   +-----+   +------+ |     |
    | | Host2 |<--| B2.0 |<--| Vol |<--| B2.1 |<------+
    | +-------+   +------+   +-----+   +------+ |
    +-------------------------------------------+

, where capture Host1 has 48 kHz, 16/16 frame_fmt, 4 channels stream and Host2 has 48 kHz, 16/16 frame_fmt, 8 channels.
It is failing on a captured streams verification. This happens, because we do not have B1.1 well configured. Firstly, we send pcm params to Host1 and configure B1.1 with 4 channels stream, next we send pcm params to Host2 and in pipeline_comp_params_neg() we overwrite ```B1.1`` with 8 channels, which results in a mismatch in channels parameter.

Thanks for trying this, I think you params the 2nd stream before the 1st one is started, right, this do uncover a bug in the state check, can you try if below change help?

diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c
index c381174b2..4f78bd619 100644
--- a/src/audio/pipeline.c
+++ b/src/audio/pipeline.c
@@ -388,8 +388,9 @@ static int pipeline_comp_params_neg(struct comp_dev *current,
        pipe_dbg(ppl_data->p, "pipeline_comp_params_neg(), current->comp.id = %u, dir = %u",
                 dev_comp_id(current), dir);
 
-       /* return if current is running */
-       if (current->state == COMP_STATE_ACTIVE) {
+       /* return if current is already configured */
+       if (current->state != COMP_STATE_INIT &&
+           current->state != COMP_STATE_READY) {
                if (buffer_params_match(calling_buf,
                                        &ppl_data->params->params))
                        return 0;

keyonjie · 2020-05-13T03:49:14Z

@bkokoszx the new component state coverage is added.

lgirdwood · 2020-05-13T13:19:29Z

@keyonjie @mmaka1 @kv2019i @lbetlej how can we check this PR is aligned with our new rules ?

keyonjie · 2020-05-14T01:38:22Z

@keyonjie @mmaka1 @kv2019i @lbetlej how can we check this PR is aligned with our new rules ?

I would leave this to be answered from @mmaka1 who is designing the new rules.

src/include/sof/audio/buffer.h

src/audio/pipeline.c

lgirdwood · 2020-06-01T09:36:57Z

src/audio/pipeline.c

"further" what ?, can you provide context.

typo, should be the propagation "go further".

"further" what ?, can you provide context.

fixed.

src/audio/pipeline.c

src/include/sof/audio/buffer.h

src/audio/pipeline.c

zrombel · 2020-07-31T08:19:14Z

Due to recent PR3194 merge, please rebase for TGL Multicore tests to Pass on Internal CI.

lgirdwood

@keyonjie ping

keyonjie · 2020-08-10T07:22:14Z

@lgirdwood @mmaka1 @bkokoszx I am not sure if we still need this change for demux/smart_amp usage scenarios?

lgirdwood · 2020-08-10T12:50:25Z

@keyonjie dont think so. Lets close, we can re-open if needed.

lgirdwood · 2020-09-01T12:18:21Z

@keyonjie @bkokoszx do we need to reopen this ? Been speaking with @mwasko and it sounds like we have a dependency ?

bkokoszx · 2020-09-01T14:01:49Z

@lgirdwood
Yes, we need to reopen it in order to fix #3249 issue.

keyonjie · 2020-09-02T10:15:26Z

@lgirdwood rebased to the latest base. How should we proceed for this now?

keyonjie

@lgirdwood we still need this fix for components like demux which could have multiple inputs/outputs, just updated it and addressed what remained, please help to review.

src/include/sof/audio/buffer.h

src/audio/pipeline.c

keyonjie · 2020-09-07T09:15:54Z

src/audio/pipeline.c

"further" what ?, can you provide context.

fixed.

src/include/sof/audio/buffer.h

lgirdwood · 2020-09-07T10:49:24Z

src/audio/pipeline.c

This comment does not align with the code below

src/audio/pipeline.c

lgirdwood · 2020-09-07T10:55:52Z

src/audio/pipeline.c

Do you mean walk the new pipeline here. I think at this point the connected pipeline source buffer will have the correct params configured so no further action would be needed. This can be removed.

Yes, we stop here after the connected (but not paramed) pipeline source buffer is propagated.
We might need to propagate it forward in the future, but no this requirement for now.

OK, let me remove this comment.

It's still there.

lgirdwood

Just minor.

src/include/sof/audio/buffer.h

src/audio/pipeline.c

lgirdwood · 2020-09-09T10:58:33Z

src/audio/pipeline.c

It's still there.

Add a helper buffer_param_match() to check if the configured runtime buffer frame format/rate/channels are matched to the param ones Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Add a flag 'walking' to the buffer struct, to indicate if the buffer is just being traversed while walking through the graph, and don't go back to the buffer which is already walked. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

For different connected pipelines, they are usually connected via a component with >1 source or sink buffers, as most of those components don't have ability to converse frame format or sample rate, we need add a params negotiation mechanism between those pipelines. If all buffers are iterated in the params propagation direction, no negotiation needed, otherwise we need to iterate all other buffers in the opposite direction, to make sure all connected buffers have chance to take part in the negotiation. The negotiation should happen before we can propagate the params further, when doing negotiation in a branch/buffer, we should check: If a branch is active, check if params is matched, return error to reject the whole .params() requirement if not. If a branch is inactive, force update the params to its calling buffer, to make sure all branches are aligned on the format. We are propagating the params to branched buffer, and the subsequent component's .params() or .prepare() should be responsible to calibrate the buffer params if needed. For example, a component who has different channels buffers should explicitly configure the channels for its branched buffers (the ones connected to another pipeline). Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie requested review from akloniex, bardliao, bkokoszx, jajanusz, juimonen, lbetlej, lgirdwood, mmaka1 and tlauda April 17, 2020 09:40

keyonjie requested review from dbaluta and plbossart as code owners April 17, 2020 09:40

lgirdwood requested changes Apr 19, 2020

View reviewed changes

keyonjie mentioned this pull request Apr 26, 2020

mux: add separated .params() for demux to fix noise issue #2653

Closed

mmaka1 requested changes Apr 27, 2020

View reviewed changes

keyonjie force-pushed the topic/pipeline_refine branch from 82129fa to 16a9b5c Compare May 6, 2020 04:49

keyonjie requested review from lgirdwood, mmaka1 and paulstelian97 May 6, 2020 04:50

bkokoszx mentioned this pull request May 8, 2020

mux: select demux proc function based on source buffer #2789

Merged

bkokoszx requested changes May 11, 2020

View reviewed changes

keyonjie force-pushed the topic/pipeline_refine branch from 16a9b5c to fe42edb Compare May 13, 2020 03:46

keyonjie requested a review from bkokoszx May 13, 2020 03:48

lgirdwood requested changes Jun 1, 2020

View reviewed changes

lgirdwood reviewed Jun 3, 2020

View reviewed changes

src/include/sof/audio/buffer.h Outdated Show resolved Hide resolved

src/audio/pipeline.c Outdated Show resolved Hide resolved

lgirdwood requested changes Aug 5, 2020

View reviewed changes

lgirdwood closed this Aug 10, 2020

keyonjie mentioned this pull request Aug 20, 2020

[BUG][Smart_Amp] ipc timeout in smart_amp pipelines test #3249

Closed

bkokoszx mentioned this pull request Aug 24, 2020

smart amp: added Maxim DSM audio component #3248

Merged

mengdonglin added this to the v1.6 milestone Sep 1, 2020

lgirdwood reopened this Sep 1, 2020

keyonjie force-pushed the topic/pipeline_refine branch from 91d864d to 5a033f8 Compare September 2, 2020 10:11

keyonjie force-pushed the topic/pipeline_refine branch from 5a033f8 to 5bec139 Compare September 7, 2020 09:15

keyonjie commented Sep 7, 2020

View reviewed changes

lgirdwood requested changes Sep 7, 2020

View reviewed changes

keyonjie force-pushed the topic/pipeline_refine branch from 5bec139 to d779ae5 Compare September 8, 2020 07:25

keyonjie requested review from lgirdwood and removed request for tlauda September 8, 2020 07:27

lgirdwood requested changes Sep 9, 2020

View reviewed changes

keyonjie added 3 commits September 10, 2020 13:30

buffer: add helper to check if buffer format is matched to param

d94eafd

Add a helper buffer_param_match() to check if the configured runtime buffer frame format/rate/channels are matched to the param ones Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie force-pushed the topic/pipeline_refine branch from d779ae5 to 49e9c72 Compare September 10, 2020 05:32

keyonjie requested a review from lgirdwood September 10, 2020 05:33

lgirdwood approved these changes Sep 10, 2020

View reviewed changes

lgirdwood merged commit 64a1657 into thesofproject:master Sep 10, 2020

zrombel mentioned this pull request Sep 14, 2020

smart_amp: remove hardcoded frame_fmt value #3404

Merged

pipeline param negotiation #2788

pipeline param negotiation #2788

Uh oh!

Conversation

keyonjie commented Apr 17, 2020

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!

keyonjie Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

keyonjie commented Apr 20, 2020

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!

keyonjie May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented May 7, 2020

Uh oh!

lgirdwood commented May 7, 2020

Uh oh!

keyonjie commented May 8, 2020

Uh oh!

bkokoszx left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie commented May 12, 2020

Uh oh!

keyonjie commented May 13, 2020

Uh oh!

lgirdwood commented May 13, 2020

Uh oh!

keyonjie commented May 14, 2020

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!

keyonjie Apr 20, 2020 •

edited

Loading

keyonjie May 6, 2020 •

edited

Loading

lgirdwood commented Aug 10, 2020 •

edited

Loading