Repository navigation
pipeline frame count and pcm range setting - #842
Conversation
|
Here's some documentation related to this pr, if someone has time to read: |
xiulipan
left a comment
xiulipan
left a comment
There was a problem hiding this comment.
@juimonen Finish the huge patches review, I have some question summary here:
- How the frame_rate is setting here. I see your example in discussion. But seems no related code here.
Correct me if I missed something. - The changes to topology is very huge. Any simple way?
If there are any side effect if no src is pipeline?
others are small issues inline.
e97a5f9 to
5531d56
Compare
|
@lgirdwood @xiulipan thanks for the review. I updated the PR. I tried to incorporate the suggestions, but there are still couple of comments I did not totally understand. This PR should be really tested by QA with all possible platforms and topologies as I'm limited to up2... |
5531d56 to
b31e71e
Compare
|
The test results of PR842 are as follows:
Test env: |
|
@lgirdwood I'll rebase, however it goes to tomorrow, there was tomasz' patch for pipeline refactoring, which I have to resolve... |
b31e71e to
19a9ee1
Compare
|
@lgirdwood, rebased and updated. The frame setting code now changed, because of re-factored pipeline code. I did test basic src topologies and they seem to work. I tried to test also 8-192khz range to 48kHz dai, but there's some problem. I think the kernel sof/pcm.c is currently tied to dai max rate, which is not going to work in the case mentioned before. I don't know however is it a problem for this pr now... |
|
this PR is failing in cmocka test: test_audio_pipeline_complete_connect_downstream_full |
|
@juimonen just a minor comment and looks like a UT failure. |
6f67178 to
2494dbe
Compare
|
@lgirdwood I fixed the unit test, I hope the fix was ok. I'm still seeing strange build errors, some time outs and klockwork "indexing" errors... |
|
don't merge this yet, as I need to test this better after pipeline refactoring! |
|
On QuickBuild in python regression test for this PR we are encountering DSP panic when sending ipc_stream_pcm_params to FW. This problem applies to APL, CNL and ICL builds. IPC log (after creating topology when trying to create stream): Creating stream (48000Hz, 2, 32b/32b). Notification Ipc (8DEA0000, 00040800) Full IPC log can be found on QuickBuild or I can provide it. All builds was made with xtensa-smp architecture. I will try to run test on APL with xtensa-up to see if problem is architecture dependent. |
edcc57f to
c887140
Compare
503d5d4 to
5630a3c
Compare
|
@lgirdwood @mengdonglin I'd still like to have QA test round as this is very invasive... I can't test most platforms on my own. |
|
@juimonen Can we get this rebased and maybe finally merged? |
5630a3c to
8e685e1
Compare
|
@tlauda rebased. I think @mengdonglin is on vacation this week and I would like to have the extended testing for this... This is touching all topology files and I can't test them myself. |
|
@lgirdwood @xiulipan Please rereview ASAP to avoid another rebase. |
8e685e1 to
7983166
Compare
xiulipan
left a comment
xiulipan
left a comment
There was a problem hiding this comment.
Some simple question about the pipeline rate setting.
It should be always need to be used for SRC involved pipeine right?
There was a problem hiding this comment.
Do we miss some setting to
W_VENDORTUPLES(media_src_tokens, sof_src_tokens, LIST( ', SOF_TKN_SRC_RATE_OUT "PIPELINE_RATE"'))
in this file?
There was a problem hiding this comment.
yes, you are right. As this PR has been lingering for almost a year, topologies come an go...
There was a problem hiding this comment.
dnl PIPELINE_PCM_ADD(pipeline,
dnl pipe id, pcm, max channels, format,
dnl period, priority, core,
dnl pcm_min_rate, pcm_max_rate, pipeline_rate,
dnl time_domain, sched_comp)```
There was a problem hiding this comment.
Do we also need to change the comments here?
PS: I think this commit should go after the topology: enable pcm range and pipeline rate and remove frame count
Previously frame count was coming as topology pipeline parameter. As the frame count might change because of some components have fixed samplerate, we might as well calculate it from the stream parameters. Component base struct will have new member output_rate, which the component should set if it has fixed output rate (like SRC). Not setting it or 0 means it can be whatever the pipeline decides. Components with 0 output_rate will have frames = (stream) sample_rate / (pipeline) deadline. Components downstream to fixed output rate will obey the fixed rate / period. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>
|
@xiulipan it could/should be used with any/all pipelines. The old "frames" is going away, because we can calculate it automatically from samplerate&scheduletime. Of course difference on these new rates doesn't make much sense with pipeline without SRC. |
Enable setting pcm min and max rate from top level m4 pipeline macro. This way it is possible to configure the whole pipeline to correct samplerate range in 1 file. Previously you needed to modify the pipeline macros where the rate was hardcoded. As the frame count is calculated from pcm/dai rate and scheduling time the frame count is obsolete. Introduce pipeline rate parameter to help configuring components with fixed output rate. We can't deduce this from pcm range since for example src might accept bigger max rate than the following dai. Even though the parameter is named "pipeline rate" it essentially means the "final" output rate to which this pipeline is connected to (dai or other pipeline). In capture pipelines it means the originating fixed dai rate. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>
Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>
cmocka pipeline connect unit test is checking the successful connection from frames count set in components. However the frames are not set in pipeline complete if we set the frames from stream parameters. So check now that the pipeline pointer has been set and matches the pipeline in high level test function. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>
7983166 to
86cae1a
Compare
|
@xiulipan pushed new version. I tried to address your comments. |
|
SOFCI TEST |
|
@juimonen Do we need any additional testing or you think CI is enough? I would like to merge it as soon as possible and avoid another rebase. |
|
@tlauda I'm not sure if all use cases with kwd and more exotic things work as intended... I'm not sure is anyone testing those. Anyway, it seems basic stuff is working and I'm fine with reverting or fixing if things arise. |
|
@juimonen CI passed (BYT machine with codec is currently down). |
ok, this is a little bit monster, but the patches are more or less related. Idea is to set pipeline component frame count from stream samplerate and scheduling time. Because of stream sample rate range is also set in the topology, there's patches to set that on top level m4. Because frame count also affects buffer size, there's also patches related to buffer re-size inside firmware. 50kHz topology is the only "non-related", but it is also good for testing this thing.