Skip to content

[RFC]selector: need treat selector as endpoint for command handling - #1365

Merged
lgirdwood merged 1 commit into
thesofproject:masterfrom
keyonjie:selector_endpoint_rfc
May 15, 2019
Merged

lgirdwood merged 1 commit into
thesofproject:masterfrom
keyonjie:selector_endpoint_rfc

Conversation

@keyonjie

@keyonjie keyonjie commented May 5, 2019

Copy link
Copy Markdown
Contributor

For hw_params(), prepare(), trigger(), reset() commands, we should treat
seletor as endpoint(don't propagate them upstream anymore), otherwise,
those upstream components(e.g. kpb and dai) will complain about double
handling and status might be wrong, errors like "source component buffer
has not enough data available" will happen.

This is only RFC to raise attention of this issue, it is not good fix,
please help comment.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

@keyonjie
keyonjie requested a review from tlauda May 5, 2019 10:28
For hw_params(), prepare(), trigger(), reset() commands, we should treat
seletor as endpoint(don't propagate them upstream anymore), otherwise,
those upstream components(e.g. kpb and dai) will complain about double
handling and status might be wrong, errors like "source component buffer
has not enough data available" will happen.

This is only RFC to raise attention of this issue, it is not good fix,
please help comment.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

@lgirdwood lgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not following, can you describe use case where this happens.

@keyonjie

keyonjie commented May 6, 2019

Copy link
Copy Markdown
Contributor Author

Not following, can you describe use case where this happens.

@lgirdwood for the detect pipeline(pipeline 9), those commands(e.g. hw_params, trigger, reset) should stop before KPB(that is, at selector), otherwise, it will interact the running of capture pipeline(pipeline 8), here the solution to fix it is not decent, I draft the RFC patch here which verified work, to try to get better solution of it especially from @tlauda who have done refinement for pipeline recently.

Hi @tlauda any comment?

@lgirdwood

Copy link
Copy Markdown
Member

@keyonjie ok, this is how mixer and mux work, they will stop path walk at any unconnected paths. So your patch is using the correct constructs as the selector actually determines which sink paths are valid.

@keyonjie

keyonjie commented May 7, 2019

Copy link
Copy Markdown
Contributor Author

@keyonjie ok, this is how mixer and mux work, they will stop path walk at any unconnected paths. So your patch is using the correct constructs as the selector actually determines which sink paths are valid.

The slight difference here is that the path is actually connected, so...

@tlauda

tlauda commented May 7, 2019

Copy link
Copy Markdown
Contributor

@keyonjie @lgirdwood We need to think about some more generic solution, because there is no guarantee that the selector will always be the first component in the pipeline. This is specific for our current topology. The best and most error-proof would be to treat every pipeline in FW as separate entity, but then kernel would need to "know" about backend pipelines.

@keyonjie

keyonjie commented May 7, 2019

Copy link
Copy Markdown
Contributor Author

@keyonjie @lgirdwood We need to think about some more generic solution, because there is no guarantee that the selector will always be the first component in the pipeline. This is specific for our current topology. The best and most error-proof would be to treat every pipeline in FW as separate entity, but then kernel would need to "know" about backend pipelines.

That's what I concern exactly, thanks @tlauda for your appearance. :)

@tlauda

tlauda commented May 7, 2019

Copy link
Copy Markdown
Contributor

@keyonjie This is one of the things I wanted to discuss for a while now. Currently some of the parameters are propagated to connected pipelines and some aren't and with so many different types of topologies our logic is not good enough. We have mixer, mux and kwd topologies and every one of them expects different behavior from FW. And also I don't like the idea of putting this logic inside of the module, because it takes away from our flexibility. I still think that controlling every pipeline separately from the kernel would be the best solution. Simultaneous trigger of pipelines can be handled using compound IPCs.

@keyonjie

keyonjie commented May 7, 2019

Copy link
Copy Markdown
Contributor Author

@keyonjie This is one of the things I wanted to discuss for a while now. Currently some of the parameters are propagated to connected pipelines and some aren't and with so many different types of topologies our logic is not good enough. We have mixer, mux and kwd topologies and every one of them expects different behavior from FW. And also I don't like the idea of putting this logic inside of the module, because it takes away from our flexibility. I still think that controlling every pipeline separately from the kernel would be the best solution. Simultaneous trigger of pipelines can be handled using compound IPCs.

I can totally feel your feeling, I talked this to @lbetlej in Gdansk also, this might need discussion among wider range(e.g. at least @plbossart need to be involved), and the good solution with it will be our long-term goal(won't be available in days).

So here, I prefix it with [RFC] to try to get alignment for short-term solution firstly.

@keyonjie

keyonjie commented May 8, 2019

Copy link
Copy Markdown
Contributor Author

@tlauda @lgirdwood as we can expect we won't have solution for this in short time, I suggest we take the solution in my PR firstly and refine it to long-term solution once that is available, here the change is only inside selector component, the risk here is more controlable comparing to any pipeline short-term fixes IMO.

@lgirdwood

Copy link
Copy Markdown
Member

@keyonjie @tlauda please see #1280 on propagating params through pipeline.
@tlauda kernel should not be managing individual pipelines but only kernel controllable endpoints (like PCMs). The kernel has no concept of what a pipeline is in reality, it really doesn't care because pipeline is low level, it only passes topology data for a pipeline from FS to FW. It is up to the FW to decide when to start scheduling pipelines based on the topology graph status.

@keyonjie

Copy link
Copy Markdown
Contributor Author

@keyonjie @tlauda please see #1280 on propagating params through pipeline.
@tlauda kernel should not be managing individual pipelines but only kernel controllable endpoints (like PCMs). The kernel has no concept of what a pipeline is in reality, it really doesn't care because pipeline is low level, it only passes topology data for a pipeline from FS to FW. It is up to the FW to decide when to start scheduling pipelines based on the topology graph status.

@lgirdwood I totally appreciate what you described on #1280, what I concern only is when will we get this feature supported in the FW, now it is block for both KWD and demux what @juimonen is working on.

@wenqingfu

Copy link
Copy Markdown

@zrombel please check the CI if it's a false negative report on the CodeCheck?

@lgirdwood
lgirdwood merged commit 12ea3a0 into thesofproject:master May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants