[DNM (for v1.8-rc2) ] CA: cadence: update stream params during config apply by mrajwa · Pull Request #4008 · thesofproject/sof

mrajwa · 2021-04-07T10:40:42Z

This patch updates configuration parameters read from the topology
or during runtime with the stream parameters. Hence settings like
sampling rate or sample width are aligned with stream parameters.
The mismatch in those parameters may result in audio glitches or
stream stalls, just to name a few potential issues.

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

lgirdwood

Need some comments to help guide other developers through the Cadence differences.

lgirdwood · 2021-04-07T13:03:50Z

I would also have a big comment here explaining which Cadence codecs this applies to and state other may differ.

lgirdwood · 2021-04-12T15:18:17Z

@dbaluta I'm aligned if you need to take over whilst @mrajwa is busy.

dbaluta · 2021-04-12T15:37:14Z

@lgirdwood thanks will take over this as it also overlaps with some patches I need to send for somehow similar issue.

mrajwa · 2021-04-13T11:57:05Z

@lgirdwood thanks will take over this as it also overlaps with some patches I need to send for somehow similar issue.

Hmm I'll update this PR later today, please don't close

This patch assigns IDs of each stream parameter that needs to be updated in runtime. This is because each codec uses different params and we need a consistent method of update. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch takes care of the update of stream parameters during runtime. It is important to start stream base on its runtime params and not those read from the topology which may not mach those of stream. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2021-04-27T10:47:44Z

@dbaluta I have finally updated. I would appreciate if you could review :)

mrajwa · 2021-04-29T08:22:47Z

@lgirdwood @paulstelian97 @dbaluta updated. Are we good to merge?

paulstelian97

Other than the fact that we are using comp_err for something that's not an error this looks good now.

zrombel · 2021-04-29T09:41:00Z

CI is all green so PR is good to merge for me :)

dbaluta · 2021-05-04T15:24:22Z

@lgirdwood Yes, I figured out the problem. update_params() tries to do invalid set parameters configuration for MP3 for example and prepare fails.

E.g setting sample rate for MP3 is an invalid operation! sample rate can only be read with a call to Cadence API.

cadence_codec_prepare()
   => update_stream_params
       =>        API_CALL(cd, XA_API_CMD_SET_CONFIG_PARAM, cd->sample_rate_id, &sample_rate, ret)
                 if (LIB_IS_FATAL_ERROR(ret)) {
                      comp_err(dev, "update_stream_params(): failed to set sample rate."); <===== API fails here!
                      goto err;
                  }

lgirdwood · 2021-05-04T18:56:40Z

@dbaluta ok, does this mean this is good to merge or can it wait for rc2 ? (it also need a conflict fixed)

dbaluta · 2021-05-04T19:10:40Z

@lgirdwood it's no good to merge. lets wait for rc2

mrajwa · 2021-05-05T08:29:10Z

@dbaluta are you going to fix that mp3 issue or you want me to do this? Although I don't have that codec on my disposal.. ;/

dbaluta · 2021-05-05T08:33:26Z

@mrajwa let me send you my version of this patch.

dbaluta · 2021-05-05T09:13:49Z

@mrajwa what do you think about this? #4139

mrajwa · 2021-05-07T08:18:04Z

@dbaluta #4139 seems to be just a copy and rework of this patch. Could you please tell why the update of sample rate fails for MP3 codec? Maybe we can make a define which keep record of updates that need to be performed? I am thinking about some sort of a mask like 0x03 -> first n bits can define parameters to be updated.

dbaluta · 2021-05-10T11:46:09Z

@mrajwa I will provide you a detailed answer to this on Tuesday. As I said above Cadence algorithms for MP3 (for example) will result in a fatal error when one tries to set sample rate or number of channels.

The Cadence algorithm will figure out by itself these parameters by parsing the mp3 header.

lgirdwood

Thanks for the update @mrajwa !
@dbaluta I guess you can test this with your Cadence library too ?

dbaluta · 2021-07-28T05:37:46Z

@lgirdwood will test this today.

lyakh · 2021-07-28T08:50:46Z


+static int update_stream_params(struct comp_dev *dev)
+{
+	int ret = 0;


IIRC API_CALL() was ret = ... in which case this initialisation is superfluous

dbaluta · 2021-07-28T10:53:30Z

@lgirdwood @mrajwa what branch should I test this on? It looks like it cannot be cleanly applied on main / or v1.8 branch.

dbaluta · 2021-07-28T13:49:04Z

@lgirdwood @mrajwa this patch fails for AAC and MP3 because some parameters are not supported for these codecs and the error is fatal.

For example:

setting sample rate for MP3 codec returns XA_API_FATAL_INVALID_CMD_TYPE.

We can merge this patch with the following compromise. Do not return an error if setting a parameter fails:

e.g

        /* sample rate */
        API_CALL(cd, XA_API_CMD_SET_CONFIG_PARAM, cd->sample_rate_id,
                 &sample_rate, ret);
-       if (LIB_IS_FATAL_ERROR(ret)) {
-               comp_err(dev, "update_stream_params(): failed to set sample rate.");
-               goto err;
-       }
+       if (LIB_IS_FATAL_ERROR(ret))
+               comp_err(dev, "update_stream_params(): failed to set sample rate, err = %x", ret);
+

there are few other comments which i will add inline.

dbaluta · 2021-07-28T13:51:42Z

+	CADENCE_SAMPLE_WIDTH_ID = 0x02,
+	CADENCE_CHANNELS_COUNT_ID = 0x04
+}
+


Here we need a semicolon:

CADENCE_SAMPLE_WIDTH_ID = 0x02, CADENCE_CHANNELS_COUNT_ID = 0x04 -} +};

dbaluta · 2021-07-28T13:51:49Z


+
+
 /*****************************************************************************/


remove the new lines from here. Also, add the following changes:

--- a/src/audio/codec_adapter/CMakeLists.txt +++ b/src/audio/codec_adapter/CMakeLists.txt @@ -4,6 +4,7 @@ add_local_sources(sof codec_adapter.c codec/generic.c) if(CONFIG_CADENCE_CODEC) add_local_sources(sof codec/cadence.c) +target_include_directories(sof PUBLIC ${CMAKE_CURRENT_LIST_DIR}/../../include/sof/audio/cadence)

--- a/src/include/sof/audio/codec_adapter/codec/cadence.h +++ b/src/include/sof/audio/codec_adapter/codec/cadence.h @@ -13,6 +13,9 @@ #include <sof/audio/cadence/xa_error_standards.h> #include <sof/audio/cadence/xa_memory_standards.h> +#include <sof/audio/cadence/mp3_dec/xa_mp3_dec_api.h> +#include <sof/audio/cadence/aac_dec/xa_aac_dec_api.h>

I will add a PR with these changes later if you want.

dbaluta · 2021-07-28T13:52:27Z

+	if (LIB_IS_FATAL_ERROR(ret)) {
+		comp_err(dev, "update_stream_params(): failed to set sample width.");
+		goto err;
+	}


Here only print a warning if there is an error but do not return err from this function.

lgirdwood · 2021-09-20T11:56:56Z

@mrajwa @dbaluta any updates here ? Do we need this or does the latest Cadence code have a fix ?

mrajwa · 2021-09-20T13:32:15Z

@dbaluta do you happen to know if Cadence fixed the reset bug in their library? If no let's resurrect our workaround for this here.

dbaluta · 2021-09-20T14:06:06Z

@dbaluta do you happen to know if Cadence fixed the reset bug in their library? If no let's resurrect our workaround for this here.

No news from Cadence. Also, @mrajwa can you remind me what was the reset bug? Instead of current PR I remember I prepared this PR #4139 which should be similar for Intel uses cases but doesn't break IMX usecases.

mrajwa · 2021-09-22T08:09:09Z

@daniel, ohh sorry I confused, this is not about the reset this is about the update of stream params, we really need this patch. Unfortunately I no longer have my test board, can you retest it on your side?

dbaluta · 2021-09-22T12:21:25Z

@mrajwa OK, will retest it until EOW. But I will retest the equivalent #4139 which should be equivalent with your PR but without breaking IMX usecases.

Unfortunately, I cannot test your DAP algorithm. Could we assign someone from Intel to do it?

mrajwa · 2021-09-22T12:32:15Z

Ok I will update this patch as it is unmergable now and ask our internal validation to test it.

CC: @wszypelt

kv2019i · 2022-04-20T15:38:43Z

No activity in almost half a year, should we close? @mrajwa @dbaluta @lgirdwood

mrajwa requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners April 7, 2021 10:40

mrajwa mentioned this pull request Apr 7, 2021

[BUG] [TGL] [tgl-012-drop-stable] Audio corruption and unexpected fault with codec adapter #3965

Closed

dbaluta reviewed Apr 7, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/cadence.c Outdated

dbaluta reviewed Apr 7, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/cadence.c Outdated

lgirdwood requested changes Apr 7, 2021

View reviewed changes

mrajwa force-pushed the dev branch 2 times, most recently from f75d28f to f7742a4 Compare April 8, 2021 21:25

dbaluta reviewed Apr 9, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/cadence.c Outdated

dbaluta reviewed Apr 9, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/cadence.c Outdated

dbaluta reviewed Apr 9, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/cadence.c Outdated

lgirdwood approved these changes Apr 12, 2021

View reviewed changes

mrajwa force-pushed the dev branch from f7742a4 to 94226e5 Compare April 27, 2021 10:29

mrajwa added 2 commits April 27, 2021 12:34

cadence codecs: assign stream params IDS

a775a94

This patch assigns IDs of each stream parameter that needs to be updated in runtime. This is because each codec uses different params and we need a consistent method of update. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the dev branch 3 times, most recently from 549d261 to b8ba597 Compare April 27, 2021 10:47

paulstelian97 reviewed Apr 27, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/cadence.c Outdated

mrajwa force-pushed the dev branch from b8ba597 to 3d79f62 Compare April 29, 2021 08:22

paulstelian97 reviewed Apr 29, 2021

View reviewed changes

lgirdwood changed the title ~~CA: cadence: update stream params during config apply~~ [DNM (for v1.8-rc2) ] CA: cadence: update stream params during config apply May 5, 2021

dbaluta mentioned this pull request May 5, 2021

cadence codecs: update stream params #4139

Closed

mrajwa force-pushed the dev branch 3 times, most recently from 11da945 to 27afe5d Compare July 27, 2021 18:25

lgirdwood reviewed Jul 27, 2021

View reviewed changes

lyakh reviewed Jul 28, 2021

View reviewed changes

dbaluta reviewed Jul 28, 2021

View reviewed changes

iuliana-prodan self-requested a review September 20, 2021 13:44

lgirdwood closed this Apr 20, 2022




		/*****************************************************************************/

Conversation

mrajwa commented Apr 7, 2021

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Apr 12, 2021

Uh oh!

dbaluta commented Apr 12, 2021

Uh oh!

mrajwa commented Apr 13, 2021

Uh oh!

mrajwa commented Apr 27, 2021

Uh oh!

Uh oh!

mrajwa commented Apr 29, 2021

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

zrombel commented Apr 29, 2021

Uh oh!

dbaluta commented May 4, 2021

Uh oh!

lgirdwood commented May 4, 2021

Uh oh!

dbaluta commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented May 5, 2021

Uh oh!

dbaluta commented May 5, 2021

Uh oh!

dbaluta commented May 5, 2021

Uh oh!

mrajwa commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbaluta commented May 10, 2021

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Jul 28, 2021

Uh oh!

lyakh Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Jul 28, 2021

Uh oh!

dbaluta commented Jul 28, 2021

Uh oh!

dbaluta Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

dbaluta Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

dbaluta Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented Sep 20, 2021

Uh oh!

dbaluta commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented Sep 22, 2021

dbaluta commented May 4, 2021 •

edited

Loading

mrajwa commented May 7, 2021 •

edited

Loading

lgirdwood commented Sep 20, 2021 •

edited

Loading

dbaluta commented Sep 20, 2021 •

edited

Loading