waves: store config blob in a cache in waves.c #8448

barry-waves · 2023-11-06T11:18:08Z

Store/apply config blob in a cache to avoid that
cfg.data will be released after prepare.

btian1 · 2023-11-07T03:07:56Z

src/audio/module_adapter/module/waves/waves.c

+						ret);
+				module_free_memory(mod, waves_codec->config_blob);
+				waves_codec->config_blob_size = 0;
+				return ret;


replace line 497 with
ret = -ENOMEM;

this can:

keep same with if part.

one return in line 502 is enough for this case.

btian1 · 2023-11-07T03:15:10Z

src/audio/module_adapter/module/waves/waves.c

+
+	if (waves_codec->config_blob) {
+		ret = waves_effect_message(mod, waves_codec->config_blob,
+								waves_codec->config_blob_size);


line 734 should align with line 733

src/audio/module_adapter/module/waves/waves.c

lyakh · 2023-11-07T13:21:02Z

src/audio/module_adapter/module/waves/waves.c

+			comp_err(dev,
+				"waves_effect_cache_config_blob() failed to allocate %d bytes for config blob",
+				size);
+			ret = -ENOMEM;


I'd just do return -ENOMEM here (and below) and also avoid a done debug print in an error case, remove the ret variable

lyakh · 2023-11-07T13:21:14Z

src/audio/module_adapter/module/waves/waves.c

+		}
+	}
+	comp_dbg(dev, "waves_effect_cache_config_blob() done");
+	return ret;


and just return 0 here

lyakh · 2023-11-07T13:22:59Z

src/audio/module_adapter/module/waves/waves.c

+	comp_dbg(dev, "waves_effect_apply_config_blob_cache()");
+
+	if (waves_codec->config_blob) {
+		ret = waves_effect_message(mod, waves_codec->config_blob,


return waves_effect_message(...) here and return 0 below

src/audio/module_adapter/module/waves/waves.c

johnylin76 · 2023-11-09T11:56:01Z

src/audio/module_adapter/module/waves/waves.c

+				"waves_effect_cache_config_blob(): failed to copy config blob %d",
+				ret);
+			module_free_memory(mod, waves_codec->config_blob);
+			waves_codec->config_blob_size = 0;


waves_codec->config_blob = NULL; accordingly

johnylin76 · 2023-11-09T12:42:15Z

src/audio/module_adapter/module/waves/waves.c

@@ -492,6 +536,16 @@ static int waves_effect_message(struct processing_module *mod, void *data, uint3
 	return 0;
 }

+static int waves_handle_param_message(struct processing_module *mod, void *data, uint32_t size)
+{
+	int ret = waves_effect_message(mod, data, size);


why not

int ret = waves_effect_cache_config_blob(mod, data, size); if (!ret) ret = waves_effect_message_from_cache(mod);

or even clearer, decouple these two actions:

when apply_config() is called which means the external config is ready --> call waves_effect_cache_config_blob() to store into cache

when prepare() is called --> call waves_effect_message_from_cache() to load config blob from cache and set into module

(Note we don't always set config from external every time before prepare)

May need to consider if it is possible that the external config comes latter than prepare? The same for the current design as well (it seems to early return with error if config comes late for the current design, which can be acceptable by the cases we have now in reality) It is always the best practice that takes the above question into account.

johnylin76 · 2023-11-09T13:28:30Z

src/audio/module_adapter/module/waves/waves.c

@@ -659,6 +717,22 @@ static int waves_codec_init(struct processing_module *mod)
 	return ret;
 }

+/* apply config blob */
+static int waves_effect_apply_config_blob_cache(struct processing_module *mod)


I beg to state that the commit complicates the design. After that there will be both waves_effect_apply_config() and waves_effect_apply_config_cache(). Moreover, waves_effect_config(), waves_effect_cache_config(), and waves_effect_message().

(Please correct me if there is devil in the details.) I do believe the logic is fair enough as I mentioned in the former comment. That is, two reaction chain in total which can be triggered from external:

set config blob from external:

(...) -> waves_codec_set_configuration() -> waves_effect_apply_config() -> waves_effect_config_save_to_cache()

start module (trigger prepare) from external:

(...) -> waves_codec_prepare() -> waves_effect_config_load_from_cache()

then handle what if...

cache is empty when calling waves_effect_config_load_from_cache()?

module is processing when calling waves_effect_config_save_to_cache()?

johnylin76 · 2023-11-09T13:33:02Z

src/audio/module_adapter/module/waves/waves.c

@@ -41,7 +41,9 @@ struct waves_codec_data {
 	uint32_t                response_max_bytes;
 	uint32_t                request_max_bytes;
 	void                    *response;
-	struct module_config setup_cfg;
+	struct module_config    setup_cfg;


I feel certain that setup_cfg is no longer used

src/audio/module_adapter/module/waves/waves.c

lyakh · 2023-11-14T08:58:43Z

src/audio/module_adapter/module/waves/waves.c

 	if (ret)
-		comp_err(dev, "waves_codec_prepare() failed %d", ret);
+		goto error;

 	comp_dbg(dev, "waves_codec_prepare() done");
 	return ret;


this can be return 0 now

src/audio/module_adapter/module/waves/waves.c

Store/apply config blob in a cache to avoid that cfg.data will be released after prepare. Signed-off-by: barry.jan <[email protected]>

lgirdwood · 2023-11-24T13:23:23Z

@johnylin76 are all review comments aligned with the latest update ?

johnylin76 · 2023-11-27T04:01:10Z

@johnylin76 are all review comments aligned with the latest update ?

Yes. Thanks

kv2019i · 2023-11-27T11:33:03Z

Seems ready to go, let me kick the SOF CI once more to get a clean CI sheet.

kv2019i · 2023-11-27T11:33:15Z

SOFCI TEST

barry-waves requested review from lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners November 6, 2023 11:18

btian1 reviewed Nov 7, 2023

View reviewed changes

barry-waves force-pushed the main branch 2 times, most recently from 10f7210 to 739a583 Compare November 7, 2023 06:18

lyakh reviewed Nov 7, 2023

View reviewed changes

barry-waves force-pushed the main branch from 739a583 to ec6d9f3 Compare November 8, 2023 01:42

lyakh approved these changes Nov 8, 2023

View reviewed changes

johnylin76 requested review from johnylin76 and cujomalainey November 9, 2023 02:27

btian1 approved these changes Nov 9, 2023

View reviewed changes

lyakh requested changes Nov 9, 2023

View reviewed changes

src/audio/module_adapter/module/waves/waves.c Outdated Show resolved Hide resolved

johnylin76 reviewed Nov 9, 2023

View reviewed changes

barry-waves force-pushed the main branch from ec6d9f3 to 6465926 Compare November 14, 2023 01:25

lyakh reviewed Nov 14, 2023

View reviewed changes

src/audio/module_adapter/module/waves/waves.c Show resolved Hide resolved

lyakh reviewed Nov 14, 2023

View reviewed changes

barry-waves force-pushed the main branch from 6465926 to 19e74b0 Compare November 15, 2023 04:20

johnylin76 reviewed Nov 21, 2023

View reviewed changes

waves: store config blob in a cache in waves.c

7731eca

Store/apply config blob in a cache to avoid that cfg.data will be released after prepare. Signed-off-by: barry.jan <[email protected]>

barry-waves force-pushed the main branch from 19e74b0 to 7731eca Compare November 22, 2023 03:30

barry-waves requested a review from lyakh November 23, 2023 08:45

lyakh approved these changes Nov 23, 2023

View reviewed changes

johnylin76 approved these changes Nov 27, 2023

View reviewed changes

kv2019i merged commit 3917308 into thesofproject:main Nov 27, 2023
42 of 46 checks passed

This was referenced Dec 1, 2023

[cavs2.5-001] waves: store config blob in a cache in waves.c #8555

Merged

Mt8186/v0.2 #8556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

waves: store config blob in a cache in waves.c #8448

waves: store config blob in a cache in waves.c #8448

barry-waves commented Nov 6, 2023

btian1 Nov 7, 2023

btian1 Nov 7, 2023

lyakh Nov 7, 2023

lyakh Nov 7, 2023

lyakh Nov 7, 2023

johnylin76 Nov 9, 2023

johnylin76 Nov 9, 2023

johnylin76 Nov 9, 2023

johnylin76 Nov 9, 2023

lyakh Nov 14, 2023

lgirdwood commented Nov 24, 2023

johnylin76 commented Nov 27, 2023

kv2019i commented Nov 27, 2023

kv2019i commented Nov 27, 2023

waves: store config blob in a cache in waves.c #8448

waves: store config blob in a cache in waves.c #8448

Conversation

barry-waves commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Nov 24, 2023

johnylin76 commented Nov 27, 2023

kv2019i commented Nov 27, 2023

kv2019i commented Nov 27, 2023