Split data blob functionality out of component code and add single data blob mode #5509

jsarha · 2022-03-10T09:03:45Z

The motive behind this PR is to provide means to save DSP memory with components that require very large configurations.

Bit surprising - when using single blob mode - that even if a set_cmd() is rejected because state == active, the configuration is sent again right after the component is stopped and started again. I guess this is the Linux side drivers doing. Otherwise the code appears to work based on my brief tests.

If the two first commits look Ok, I'll drop the others out and change this from draft to a real PR.

In the end this was much simpler that I anticipated.

jsarha · 2022-03-10T10:29:59Z

I dropped the extraneous example and testing commits, but they are still available here: https://github.com/jsarha/sof/tree/single-data-blob-test

jsarha · 2022-03-11T08:16:27Z

Fixed exotic component and zephyr builds.

jsarha · 2022-03-11T10:44:33Z

Fix cmocka builds.

jsarha · 2022-03-11T12:25:54Z

Fix couple of typos.

The data blob handler functionality is quite independent from the rest of the generic component code. The component.c and component.h are already too big so it is better to split the data blob handler functionality out before adding more features to it. Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2022-03-14T14:55:36Z

@mwasko and @mmaka1 does this PR do what you asked or is there something more needed here?

src/audio/data_blob.c

src/include/sof/audio/data_blob.h

mwasko · 2022-03-15T10:16:57Z

@mwasko and @mmaka1 does this PR do what you asked or is there something more needed here?

looks good to me, minor changes requested in comments

cujomalainey · 2022-03-15T20:56:46Z

src/audio/data_blob.c

@@ -22,17 +22,40 @@ struct comp_data_blob_handler {
 	uint32_t data_pos;	/**< indicates a data position in data
 				  *  sending/receiving process
 				  */
+	uint32_t single_blob:1; /**< Allocate only one blob. Module can not
+				  *  be active while reconfguring.


im curious, why can it not be active?

The whole feature is for components with very big configurations. In those cases we can not afford to have two copies of the configuration blob in the DSP memory. This also means that when we receive a new configuration - in multiple IPC fragments - we must release the old configuration first (or reuse the old buffer). This means that we do not have a valid configuration during the update so the audio pipeline must be inactive.

Ack, can we get that clarified in either comments or some docs?

There is already something in comp_data_blob_handler_new_ext() doxygen documentation, but the motivation for the feature is missing. I'll add one more sentence about saving DSP memory.

@MingJenTai FYI, so you don't need to do it in your own blob. This is also safer as we can fuzz this

src/audio/data_blob.c

cujomalainey · 2022-03-15T21:01:35Z

I recommend you stress test your code on a chromebook with this script #4769 (comment)

The single blob mode keeps only one configuration blob available at one time and saves the memory otherwise consumed by the second blob. The down side of this behaviour is that the audio pipeline must be stopped for the configuration to be updated. Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2022-03-18T09:38:29Z

I recommend you stress test your code on a chromebook with this script #4769 (comment)

@juimonen is there an easy way for me to get my hands on a chromebook?

cujomalainey · 2022-03-20T00:10:23Z

I recommend you stress test your code on a chromebook with this script #4769 (comment)

@juimonen is there an easy way for me to get my hands on a chromebook?

You may need to get @sathya-nujella or @sathyap-chrome to generate a build for you with the hotword library in it

juimonen · 2022-03-21T06:52:48Z

I recommend you stress test your code on a chromebook with this script #4769 (comment)

@juimonen is there an easy way for me to get my hands on a chromebook?

@jsarha there is a quite easy physical way that we drive with our cars the the office and I'll give you one laptop :)

jsarha · 2022-03-21T08:53:46Z

I recommend you stress test your code on a chromebook with this script #4769 (comment)

@juimonen is there an easy way for me to get my hands on a chromebook?

Actually this PR does not do much in itself since it defaults to the old behaviour. Can you tell me which component I should modify to take the single blob feature into use for it to affect the referred case? google-hotword-detect?

cujomalainey · 2022-03-25T20:56:24Z

I recommend you stress test your code on a chromebook with this script #4769 (comment)

@juimonen is there an easy way for me to get my hands on a chromebook?

Actually this PR does not do much in itself since it defaults to the old behaviour. Can you tell me which component I should modify to take the single blob feature into use for it to affect the referred case? google-hotword-detect?

I have no clue if CRAS would abide by the requirements. That being said, RTNR is trying to add something that this could use.

lgirdwood · 2022-03-28T14:50:23Z

@cujomalainey fwiw - we may have also reproduced the CRAS blob loading issue on testbench and valgrind. We do a lot of blob loading and then we get an inconsistent state and then a freed memory deref.....

https://github.com/thesofproject/sof/runs/5664281753?check_suite_focus=true

(component.c:235) comp_is_new_data_blob_available()
(pipeline-schedule.c:189) pipeline_task() sched
(pipeline-schedule.c:124) pipeline_task()
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 4, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 1, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 0, dir = 1
(eq_iir.c:819) eq_iir_copy()
(component.c:235) comp_is_new_data_blob_available()
(pipeline-schedule.c:189) pipeline_task() sched
(pipeline-schedule.c:124) pipeline_task()
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 4, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 1, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 0, dir = 1
(eq_iir.c:819) eq_iir_copy()
(component.c:235) comp_is_new_data_blob_available()
(pipeline-schedule.c:189) pipeline_task() sched
(pipeline-schedule.c:124) pipeline_task()
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 4, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 1, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 0, dir = 1
(eq_iir.c:819) eq_iir_copy()
(component.c:235) comp_is_new_data_blob_available()
(pipeline-schedule.c:189) pipeline_task() sched
(pipeline-schedule.c:124) pipeline_task()
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 4, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 1, dir = 1
(pipeline-stream.c:72) pipeline_comp_copy(), current->comp.id = 0, dir = 1
(file.c:791) file_copy(): copies 54 max 0 eof 1
(eq_iir.c:819) eq_iir_copy()(pipeline-stream.c:261) pipe trigger cmd 0
(pipeline-graph.c:342) pipe reset
(pipeline-graph.c:297) pipeline_comp_reset(), current->comp.id = 0, dir = 0
(file.c:885) file_reset()
(component.c:103) comp_set_state(): wrong state = 5, COMP_TRIGGER_RESET
(pipeline-graph.c:297) pipeline_comp_reset(), current->comp.id = 1, dir = 0
(eq_iir.c:928) eq_iir_reset()
(component.c:103) comp_set_state(): wrong state = 5, COMP_TRIGGER_RESET
(file.c:644) file_free()
(eq_iir.c:654) eq_iir_free()
(pipeline-graph.c:184) disconnect buffer 3 as source
(ipc-helper.c:289) ipc_comp_free(): comp id: 4 state is 5 cannot be freed
failed to free comp 4
(pipeline-graph.c:195) pipeline_free()

==20329== Thread 3:
==20329== Invalid read of size 8
==20329==    at 0x484CA9E: eq_iir_copy (eq_iir.c:821)
==20329==    by 0x49BF704: pipeline_for_each_comp (pipeline-graph.c:392)
==20329==    by 0x49C031F: pipeline_comp_copy (pipeline-stream.c:93)
==20329==    by 0x49C031F: pipeline_copy (pipeline-stream.c:131)
==20329==    by 0x49C1C2A: pipeline_task (pipeline-schedule.c:178)
==20329==    by 0x49B5AF5: ll_thread (ll_schedule.c:131)
==20329==    by 0x4A0D608: start_thread (pthread_create.c:477)
==20329==    by 0x4B[471](https://github.com/thesofproject/sof/runs/5664281753?check_suite_focus=true#step:7:471)62: clone (clone.S:95)
==20329==  Address 0x4c23580 is 128 bytes inside a block of size 168 free'd
==20329==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==20329==    by 0x49BCD7C: comp_free (component_ext.h:55)
==20329==    by 0x49BCD7C: ipc_comp_free (ipc-helper.c:295)
==20329==    by 0x10CD6F: test_pipeline_free_comps (testbench.c:237)
==20329==    by 0x10CD6F: test_pipeline_free (testbench.c:564)
==20329==    by 0x10CD6F: pipline_test (testbench.c:728)
==20329==    by 0x4A0D608: start_thread (pthread_create.c:[477](https://github.com/thesofproject/sof/runs/5664281753?check_suite_focus=true#step:7:477))
==20329==    by 0x4B47162: clone (clone.S:95)
==20329==  Block was alloc'd at
==20329==    at 0x[483](https://github.com/thesofproject/sof/runs/5664281753?check_suite_focus=true#step:7:483)DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==20329==    by 0x[484](https://github.com/thesofproject/sof/runs/5664281753?check_suite_focus=true#step:7:484)C826: comp_alloc (component.h:619)
==20329==    by 0x484C826: eq_iir_new (eq_iir.c:605)
==20329==    by 0x49B9A4F: comp_new (helper.c:303)
==20329==    by 0x49BA85F: ipc_comp_new (helper.c:624)
==20329==    by 0x1105A4: load_process (topology.c:672)
==20329==    by 0x49FF5E0: load_widget (tplg_parser.c:1366)
==20329==    by 0x110AC7: parse_topology (topology.c:824)
==20329==    by 0x10C6F5: test_pipeline_load (testbench.c:554)
==20329==    by 0x10C6F5: pipline_test (testbench.c:662)
==20329==    by 0x4A0D608: start_thread (pthread_create.c:477)
==20329==    by

cujomalainey · 2022-03-28T17:44:46Z

@cujomalainey fwiw - we may have also reproduced the CRAS blob loading issue on testbench and valgrind. We do a lot of blob loading and then we get an inconsistent state and then a freed memory deref.....

That is great to hear, but do we have an understanding of the root cause yet?

lgirdwood · 2022-03-30T18:58:12Z

@cujomalainey fwiw - we may have also reproduced the CRAS blob loading issue on testbench and valgrind. We do a lot of blob loading and then we get an inconsistent state and then a freed memory deref.....

That is great to hear, but do we have an understanding of the root cause yet?

Not yet - looks like above was my misunderstanding of the log, i.e. the log was only showing a check for blob and not processing it.

lgirdwood

@jsarha I assume by default we use the existing mode i.e. no existing topology needs updated ?

lgirdwood · 2022-03-30T19:01:47Z

@mwasko good for you now ?

jsarha · 2022-03-31T08:46:57Z

@jsarha I assume by default we use the existing mode i.e. no existing topology needs updated ?

Yes, no changes to current behavior until some component uses comp_data_blob_handler_new_ext() and its extended parameters.

jsarha force-pushed the single-data-blob branch from 24889dd to e6efe61 Compare March 10, 2022 10:28

jsarha marked this pull request as ready for review March 10, 2022 10:34

jsarha requested review from cujomalainey, chiang831, johnylin76, singalsu, lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners March 10, 2022 10:34

jsarha force-pushed the single-data-blob branch from e6efe61 to aca9e0d Compare March 11, 2022 08:14

jsarha force-pushed the single-data-blob branch from aca9e0d to 70e85a4 Compare March 11, 2022 10:43

jsarha force-pushed the single-data-blob branch from 70e85a4 to 4d05644 Compare March 11, 2022 12:20

jsarha force-pushed the single-data-blob branch 2 times, most recently from 231268d to ed7df93 Compare March 11, 2022 14:28

mwasko reviewed Mar 15, 2022

View reviewed changes

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

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

cujomalainey reviewed Mar 15, 2022

View reviewed changes

jsarha force-pushed the single-data-blob branch 3 times, most recently from 2e286b5 to 69ddca2 Compare March 17, 2022 10:05

cujomalainey mentioned this pull request Mar 20, 2022

[RTNR] Pass binary data to RTNR #5544

Closed

lgirdwood approved these changes Mar 30, 2022

View reviewed changes

mwasko approved these changes Mar 31, 2022

View reviewed changes

lgirdwood merged commit ad0d4fb into thesofproject:main Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split data blob functionality out of component code and add single data blob mode #5509

Split data blob functionality out of component code and add single data blob mode #5509

jsarha commented Mar 10, 2022 •

edited

Loading

jsarha commented Mar 10, 2022

jsarha commented Mar 11, 2022

jsarha commented Mar 11, 2022

jsarha commented Mar 11, 2022

jsarha commented Mar 14, 2022

mwasko commented Mar 15, 2022

cujomalainey Mar 15, 2022

jsarha Mar 16, 2022

cujomalainey Mar 16, 2022

jsarha Mar 17, 2022

cujomalainey Mar 17, 2022

cujomalainey commented Mar 15, 2022

jsarha commented Mar 18, 2022

cujomalainey commented Mar 20, 2022

juimonen commented Mar 21, 2022

jsarha commented Mar 21, 2022

cujomalainey commented Mar 25, 2022

lgirdwood commented Mar 28, 2022

cujomalainey commented Mar 28, 2022

lgirdwood commented Mar 30, 2022

lgirdwood left a comment

lgirdwood commented Mar 30, 2022

jsarha commented Mar 31, 2022

Split data blob functionality out of component code and add single data blob mode #5509

Split data blob functionality out of component code and add single data blob mode #5509

Conversation

jsarha commented Mar 10, 2022 • edited Loading

jsarha commented Mar 10, 2022

jsarha commented Mar 11, 2022

jsarha commented Mar 11, 2022

jsarha commented Mar 11, 2022

jsarha commented Mar 14, 2022

mwasko commented Mar 15, 2022

cujomalainey Mar 15, 2022

Choose a reason for hiding this comment

jsarha Mar 16, 2022

Choose a reason for hiding this comment

cujomalainey Mar 16, 2022

Choose a reason for hiding this comment

jsarha Mar 17, 2022

Choose a reason for hiding this comment

cujomalainey Mar 17, 2022

Choose a reason for hiding this comment

cujomalainey commented Mar 15, 2022

jsarha commented Mar 18, 2022

cujomalainey commented Mar 20, 2022

juimonen commented Mar 21, 2022

jsarha commented Mar 21, 2022

cujomalainey commented Mar 25, 2022

lgirdwood commented Mar 28, 2022

cujomalainey commented Mar 28, 2022

lgirdwood commented Mar 30, 2022

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood commented Mar 30, 2022

jsarha commented Mar 31, 2022

jsarha commented Mar 10, 2022 •

edited

Loading