codec_adapter: cadence: fix reset procedure + improve prepare by mrajwa · Pull Request #4263 · thesofproject/sof

mrajwa · 2021-05-29T18:28:19Z

This patch set:

improves prepare procedure by adding INIT_PROCESS and INIT_DONE API calls
fixes the reset procedure to address [BUG] Post Processing XRUN when playing multiple times on same topology #4104
Cadence documentation doesn't document reset procedure at all so for a long time we were just freeing all the memory codec allocated and start over. However this is very inefficient way of performing simple reset of parameters. Hence this patch https://github.com/thesofproject/sof/pull/4162/files tried to simplify it and while it does reset parameters it also breaks the flow since XA_CMD_TYPE_INIT_API_PRE_CONFIG_PARAMS is only allowed during INIT phase. After code inspection of the codec library XA_CMD_TYPE_INIT_PROCESS seems to do what we need to perform the reset and hence this patch relies on that API call.
minor cleanup of wrong comments

This patch fixes wrong comment in generic processing function. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

After the reset codec should not need to be prepared again. We want reset procedure to just set all the parameters to their default values. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

dbaluta · 2021-05-31T11:05:30Z

I don't think this will work.

cadence_codec_init_prcess will call API_CALL(cd, XA_API_CMD_INIT, XA_CMD_TYPE_INIT_PROCESS, NULL, ret);

This will fail because at cadence_codec_prepare() time there is no data in the input buffer. Does this work for Intel DAP codec?

@dbaluta but do you think XA_CMD_TYPE_INIT_PROCESS requires data in the input buffer?

@mrajwa yes. Here is what DAP VLLDP Programmers Guide says:

Same for mp3, aac or any other cadence codec. That's why i added the cadence_codec_init_process() call inside the copy() function

We need to clarify this with cadence, namely what is the correct way of performing the reset - this is nowhere defined. I noticed INIT_PROCESS does reset the parameters by DAP lib code inspection.

Does this work for Intel DAP codec?

Yes it works for DAP

Ok, i see. It doesn't work for mp3 for example. It needs to have some input data in the buffers. Care to start a thread with Cadence support?
I will be on vacation starting tomorrow returning on June, 9 but will still check my emails.

Can you, for the testing purposes call the codec API with XA_CMD_TYPE_DO_RUNTIME_INIT command before the update of init_done at the end of the prepare() function? I have a feeling this may update the flag for compress streams.

@mrajwa Ack. Will get back to you when get some time for testing.

@mrajwa I checked with your suggestion. But no luck. The init_done is still set at 1.

API_CALL(cd, XA_API_CMD_EXECUTE, XA_CMD_TYPE_DO_RUNTIME_INIT, NULL, ret); API_CALL(cd, XA_API_CMD_INIT, XA_CMD_TYPE_INIT_DONE_QUERY, &codec->cpd.init_done, ret);

I checked with Cadence support team and they confirm that this happens and might be a library bug. They will forward the problem to engineering.

Anyhow, what is exactly our problem to fix with this reset procedure. I'm sorry I'm still after vacation and is hard for me to navigate through the sea of emails. Can you provide a simple description of the problem we are trying to solve.

On another note, I'm having a problem with second cplay run. It's always get stuck. Anyhow, if I leave enough time between 1st run and 2nd run of cplay everything works (system runtime suspend/resume get into action).

Cc: @paulstelian97

On another note, I'm having a problem with second cplay run. It's always get stuck. Anyhow, if I leave enough time between 1st run and 2nd run of cplay everything works (system runtime suspend/resume get into action).

Sounds like kernel runtime PM is rebooting FW and reseting the state ?

@lgirdwood true.

This patch simplifies reset procedure of cadence codecs by only calling the init_process() function in which XA_CMD_TYPE_INIT_PROCESS is the API call responsible for the actual reset of codec parameters. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

lgirdwood · 2021-06-14T18:03:04Z

@mrajwa @dbaluta I will merge unless any objections ? We can figure out when Cadence get back to us.

mrajwa · 2021-06-14T19:02:50Z

@lgirdwood yes I am ok with that. The bug only affects compress streams.

dbaluta · 2021-06-15T07:15:00Z

@lgirdwood please don't merge this. This breaks existing working usecases.

Actually, only the last commit ( codec_adapter: cadence: get init_done status on prepare ) is problematic. This breaks compress streams.
I am also looking at this to find a better fix.
@mrajwa can you explain in a simple usecase what is the problem with existing code? I will try to replicate the bug on compress streams and fix it.

Later edit:

@mrajwa can you please confirm that the problem we are trying to fix is that running aplay/cplay multiple time fails at the second run?

e.g

$ aplay -Dhw:0,0 sample.wav # works fine
$ aplay -Dhw:0,0 sample.wav # fails
$ aplay -Dhw:0,0, sample.wav # fails

@paulstelian97 I now you also looked at this. Do you have any insight?

dbaluta

@lgirdwood @mrajwa leave us time until Wednesday to fix it.

mrajwa · 2021-06-15T07:32:52Z

@mrajwa can you please confirm that the problem we are trying to fix is that running aplay/cplay multiple time fails at the second run?

Yes that's about it, plus keep in mind real time parameters like i.e post gain shall be reset to their default values as well. Current implementation allows for consecutive play but it doesn't reset parameters as it should.
We struggle with reset for quite a bit.. the solution proposed in this PR try to cover missing API in cadence codec but it turns our there is a bug in their logic as well (affects compress steams only).

I am also looking at this to find a better fix.

Well, it's not us who should come up with a fix but Cadence... What did they tell you, are they working to fix that?

dbaluta · 2021-06-15T07:38:20Z

@mrajwa they forwarded the problem to engineering and they are working for a fix. Will send them another mail to ask for status.

mrajwa · 2021-06-15T07:47:07Z

@dbaluta okay, I would propose them dedicated API for parameters reset - the fact that we need to free all codec memory and allocate again shows the API is defective in the first place.

dbaluta · 2021-06-15T07:51:28Z

@mrajwa I agree. Can you continue the discussion with Cadence on email via thread stared by me?

mrajwa · 2021-06-15T08:53:41Z

@dbaluta sure but I didn't receive any thread, could you please forward?

lgirdwood · 2021-06-15T08:56:32Z

Ok, lets delay this until Cadence provide us with guidance. We can make this a hotfix when we are all aligned with each other and Cadence.

dbaluta · 2021-06-16T06:50:02Z

@dbaluta sure but I didn't receive any thread, could you please forward?

@mrajwa I have added you to the thread since the first email with your linux.intel.com address. Now I also forwarded you the emails using your intel.com address. Let me know if you got them.

This patch adds init_done status check at the very end of cadence_codec_prepare() function. This is needed to know if codec is fully prepared for the processing or not. Note we don't do anything with the value read at prepare, we postpone final verification until copy/process time since some codec variants require input in order to finish its initialization. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2021-07-05T19:18:38Z

@dbaluta I have just updated with temporary solution for the init done bit. Is that OK for you?

lyakh · 2021-07-06T06:16:57Z

+	 * in codec_adapter_copy().
+	 */
+	API_CALL(cd, XA_API_CMD_INIT, XA_CMD_TYPE_INIT_DONE_QUERY,
+		 &codec->cpd.init_done, ret);


side note unrelated to this PR: I think it would be good to switch API_CALL() to

#define API_CALL(cd, cmd, sub_cmd, value) \ (cd)->api((cd)->self, cmd, sub_cmd, value)

and then do explicitly

ret = API_CALL(cd, cmd, sub_cmd, value);

@lyakh not a bad idea, but I don't see a draw back from the current implementation either. It is just a mater of taste. We all know that API_CALL is a macro.

Remember spin_lock_irqsave in the Linux kernel?

dbaluta · 2021-07-06T08:15:16Z

@dbaluta I have just updated with temporary solution for the init done bit. Is that OK for you?

@mrajwa all good from my side now! Ready to merge.

mrajwa added 2 commits May 29, 2021 19:59

codec_adapter: generic: fix comments

1c5f03c

This patch fixes wrong comment in generic processing function. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

codec_adapter: generic: simplify reset procedure

b0a1551

After the reset codec should not need to be prepared again. We want reset procedure to just set all the parameters to their default values. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners May 29, 2021 18:28

mrajwa mentioned this pull request May 29, 2021

[BUG] Post Processing XRUN when playing multiple times on same topology #4104

Closed

lgirdwood reviewed May 31, 2021

View reviewed changes

Comment thread src/audio/codec_adapter/codec/generic.c

dbaluta reviewed May 31, 2021

View reviewed changes

mrajwa force-pushed the ca_xrun_after_reset branch from e091652 to 2e1fe1d Compare June 9, 2021 09:42

lgirdwood approved these changes Jun 9, 2021

View reviewed changes

dbaluta requested changes Jun 15, 2021

View reviewed changes

mrajwa mentioned this pull request Jun 23, 2021

codec_adapter: cadence: Reset init_done at init time #4394

Merged

mrajwa force-pushed the ca_xrun_after_reset branch from 2e1fe1d to 9c9642a Compare July 5, 2021 19:16

lyakh reviewed Jul 6, 2021

View reviewed changes

dbaluta approved these changes Jul 6, 2021

View reviewed changes

dbaluta merged commit e05fbff into thesofproject:main Jul 6, 2021

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jun 14, 2021

Uh oh!

mrajwa commented Jun 14, 2021

Uh oh!

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

Uh oh!

Uh oh!

dbaluta left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

dbaluta commented Jun 15, 2021

Uh oh!

mrajwa commented Jun 15, 2021

Uh oh!

dbaluta commented Jun 15, 2021

Uh oh!

mrajwa commented Jun 15, 2021

Uh oh!

lgirdwood commented Jun 15, 2021

Uh oh!

dbaluta commented Jun 16, 2021

Uh oh!

mrajwa commented Jul 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Jul 6, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mrajwa commented May 29, 2021 •

edited

Loading

dbaluta commented Jun 15, 2021 •

edited

Loading

mrajwa commented Jun 15, 2021 •

edited

Loading