Skip to content

codec_adapter: cadence: fix reset procedure + improve prepare #4263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dbaluta merged 4 commits into from
Jul 6, 2021
Merged

codec_adapter: cadence: fix reset procedure + improve prepare #4263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1c5f03c
codec_adapter: generic: fix comments
mrajwa May 29, 2021
b0a1551
codec_adapter: generic: simplify reset procedure
mrajwa May 29, 2021
f7d94d3
codec_adapter: cadence: change reset procedure
mrajwa May 29, 2021
9c9642a
codec_adapter: cadence: get init_done status on prepare
mrajwa Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions src/audio/codec_adapter/codec/cadence.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,26 @@ int cadence_codec_prepare(struct comp_dev *dev)
ret);
goto free;
}

/* Check init done status. Note, it may happen that init_done flag will return
* false value, this is normal since some codec variants needs input in order to
* fully finish initialization. That's why at codec_adapter_copy() we call
* codec_init_process() base on result obtained below.
*/
#ifdef CONFIG_CADENCE_CODEC_WRAPPER
/* TODO: remove the "#ifdef CONFIG_CADENCE_CODEC_WRAPPER" once cadence fixes the bug
* in the init/prepare sequence. Basically below API_CALL shall return 1 for
* PCM streams and 0 for compress ones. As it turns out currently it returns 1
* in both cases so in turn compress stream won't finish its prepare during first copy
* in codec_adapter_copy().
*/
API_CALL(cd, XA_API_CMD_INIT, XA_CMD_TYPE_INIT_DONE_QUERY,
&codec->cpd.init_done, ret);

@lyakh lyakh Jul 6, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

@dbaluta dbaluta Jul 6, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

if (ret != LIB_NO_ERROR) {
comp_err(dev, "cadence_codec_init_process() error %x: failed to get lib init status",
ret);
return ret;
}
#endif
comp_dbg(dev, "cadence_codec_prepare() done");

@dbaluta dbaluta May 31, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mrajwa mrajwa May 31, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dbaluta dbaluta May 31, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

@mrajwa mrajwa May 31, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dbaluta dbaluta May 31, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mrajwa mrajwa Jun 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dbaluta dbaluta Jun 11, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dbaluta dbaluta Jun 14, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@lgirdwood lgirdwood Jun 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@dbaluta dbaluta Jun 15, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood true.

return 0;
free:
Expand Down Expand Up @@ -504,14 +523,22 @@ int cadence_codec_apply_config(struct comp_dev *dev)
int cadence_codec_reset(struct comp_dev *dev)
{
int ret;
struct codec_data *codec = comp_get_codec(dev);
struct cadence_codec_data *cd = codec->private;

API_CALL(cd, XA_API_CMD_INIT, XA_CMD_TYPE_INIT_API_PRE_CONFIG_PARAMS,
NULL, ret);
if (ret != LIB_NO_ERROR) {
comp_err(dev, "cadence_codec_init(): error %x: failed to reset to default values.",
ret);
/* Current CADENCE API doesn't support reset of codec's
* runtime parameters therefore we need to free all the resources
* and start over.
*/
codec_free_all_memory(dev);
ret = cadence_codec_init(dev);
if (ret) {
comp_err(dev, "cadence_codec_reset() error %x: could not re-initialize codec after reset",
ret);
}

ret = cadence_codec_prepare(dev);
if (ret) {
comp_err(dev, "cadence_codec_reset() error %x: could not re-prepare codec after reset",
ret);
}

return ret;
Expand Down
6 changes: 3 additions & 3 deletions src/audio/codec_adapter/codec/generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,14 +262,14 @@ int codec_process(struct comp_dev *dev)
comp_dbg(dev, "codec_process() start");

if (cd->codec.state != CODEC_IDLE) {
comp_err(dev, "codec_prepare(): wrong state of codec %x, state %d",
comp_err(dev, "codec_process(): wrong state of codec %x, state %d",
cd->ca_config.codec_id, codec->state);
return -EPERM;
}

ret = codec->ops->process(dev);
if (ret) {
comp_err(dev, "codec_prepare() error %d: codec process failed for codec_id %x",
comp_err(dev, "codec_process() error %d: codec process failed for codec_id %x",
ret, codec_id);
goto out;
}
Expand Down Expand Up @@ -339,7 +339,7 @@ int codec_reset(struct comp_dev *dev)
/* Codec reset itself to the initial condition after prepare()
* so let's change its state to reflect that.
*/
codec->state = CODEC_INITIALIZED;
codec->state = CODEC_IDLE;
Comment thread
lgirdwood marked this conversation as resolved.
Resolved

return 0;
}
Expand Down