Skip to content
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.

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

Support for volume controls in SOF plugin #9446

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Sep 6, 2024

This PR adds the support for parsing kcontrols in the IPC4 topology and the kcontrol IO ops to volume controls in the plugin topology.

@witkowsx
Copy link

witkowsx commented Sep 6, 2024

@ranj063 in Internal Intel CI System/merge/build west compilation failed probably due to lack of ';'
In handler.c in 1252:56
image

@ranj063
Copy link
Collaborator Author

ranj063 commented Sep 6, 2024

@ranj063 in Internal Intel CI System/merge/build west compilation failed probably due to lack of ';' In handler.c in 1252:56 image

Thanks @witkowsx

@@ -89,6 +89,8 @@ static inline void ipc4_send_reply(struct ipc4_message_reply *reply)
{
struct ipc *ipc = ipc_get();

/* copy the extension from the message reply */
reply->extension.dat = msg_reply.extension;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change will affect the "normal" on-DSP use-case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lyakh This is under ifdef CONFIG_LIBRARY....

tools/plugin/alsaplug/ctl.c Outdated Show resolved Hide resolved
tools/plugin/alsaplug/ctl.c Show resolved Hide resolved
@@ -270,7 +270,7 @@ static int plug_new_pga(snd_sof_plug_t *plug)

/* skip kcontrols for now */
if (tplg_create_controls(ctx, ctx->widget->num_kcontrols,
tplg_ctl, ctx->hdr->payload_size, &volume) < 0) {
tplg_ctl, ctx->hdr->payload_size, comp_info) < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

usually when a parameter is changed the function should be changed too, unless the parameter is ignored there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lyakh this parameter has been unused so far. It will only be used in the following patch

tools/plugin/alsaplug/tplg_ctl.c Outdated Show resolved Hide resolved
tools/plugin/alsaplug/tplg_ctl.c Outdated Show resolved Hide resolved
tools/plugin/alsaplug/tplg_ctl.c Outdated Show resolved Hide resolved
uint64_t q31val = ((uint64_t)val) << 15;

ctl->volume_table[i] = q31val > SOF_IPC4_VOL_ZERO_DB ?
SOF_IPC4_VOL_ZERO_DB : q31val;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is some kind of a MIN() macro available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code is copied as is from the linux driver. Is it OK to leave it as is for the sake of consistency?

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Minor comments inline, but no blockers really.

src/ipc/ipc4/handler.c Outdated Show resolved Hide resolved
@@ -89,6 +89,8 @@ static inline void ipc4_send_reply(struct ipc4_message_reply *reply)
{
struct ipc *ipc = ipc_get();

/* copy the extension from the message reply */
reply->extension.dat = msg_reply.extension;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lyakh This is under ifdef CONFIG_LIBRARY....

if (err < 0) {
SNDERR("error: invalid name for IPC rx mq %s\n", plug->tplg_file);
goto error;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit surprising to see the msg queue implementation change as part of this commit -- so to support volume controls, you need to change the IPC machinery quite a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kv2019i the msg queue implementation has not been changed in this commit. The original implementation never worked. So it has been fixed to work here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ack @ranj063 . Info like this is important to catch in the git commit messages as they are the permenant history and vital info anyone who needs to maintain this code later on. I see you already update the commit, so good with me.

Add the logic to copy the data to/from the mailbox when CONFIG_LIBRARY
is enabled with the set_large_config and get_large_config IPCs.
Also, make sure to copy the data to the mailbox along with the replay
message.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
In preparation for parsing just the topology file from the command line
for kcontrols, modify the signature of plug_parse_conf() to add an
argument to specify it.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Fix the read/write integer operations for IPC4 and the IPC messages
queues in the plugin kcontrol. Convert the bytes/enum ops implementation
to stubs. Support for these will be added later.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Initialize the shared memory for the global context during init so that
it can be used to store the kcontrol info during topology parsing. Move
the glb_ctx field from struct snd_sof_pcm to struct snd_sof_plug so that
it can be accessed during topology parsing.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
This will be used to store the module id and instance ID when creating
the volume controls.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Define the callback for setting up kcontrols in the plugin. Add a few
new fields in struct plug_shm_ctl to store the module info and the
volume table for converting mixer values to linear volume gain.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
…s for playback

This will be useful to test volume controls with the plugin.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@kv2019i kv2019i merged commit 89e417a into thesofproject:main Sep 13, 2024
44 of 47 checks passed
@ranj063 ranj063 deleted the fix/plugin_kcontrol branch September 13, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants