Skip to content

Support for volume controls in SOF plugin#9446

Merged
kv2019i merged 7 commits intothesofproject:mainfrom
ranj063:fix/plugin_kcontrol
Sep 13, 2024
Merged

Support for volume controls in SOF plugin#9446
kv2019i merged 7 commits intothesofproject:mainfrom
ranj063:fix/plugin_kcontrol

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 ranj063 force-pushed the fix/plugin_kcontrol branch from 261ac41 to c0459d0 Compare September 6, 2024 15:44
@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

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....

/* 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

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.

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>
@ranj063 ranj063 force-pushed the fix/plugin_kcontrol branch from c0459d0 to 525d875 Compare September 11, 2024 18:11
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>
@ranj063 ranj063 force-pushed the fix/plugin_kcontrol branch from 525d875 to d5e2a94 Compare September 11, 2024 20:25
@ranj063 ranj063 requested a review from lyakh September 12, 2024 13:12
@kv2019i kv2019i merged commit 89e417a into thesofproject:main Sep 13, 2024
@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