Skip to content

IPC4: add support for generic bytes control and fix LARGE_CONFIG_GET for generic controls. #10235

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
lgirdwood merged 2 commits into from
Sep 16, 2025
Merged

IPC4: add support for generic bytes control and fix LARGE_CONFIG_GET for generic controls. #10235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44b00d2
ipc4: header: Add support for generic bytes control
ujfalusi Sep 12, 2025
5990229
ipc4: handler: Add support for rx payload with LARGE_CONFIG_GET messages
ujfalusi Sep 12, 2025
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
13 changes: 9 additions & 4 deletions src/include/ipc4/header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,9 @@ struct ipc4_message_reply {
} extension;
} __attribute((packed, aligned(4)));

#define SOF_IPC4_SWITCH_CONTROL_PARAM_ID 200
#define SOF_IPC4_ENUM_CONTROL_PARAM_ID 201
#define SOF_IPC4_SWITCH_CONTROL_PARAM_ID 200
#define SOF_IPC4_ENUM_CONTROL_PARAM_ID 201
#define SOF_IPC4_BYTES_CONTROL_PARAM_ID 202
#define SOF_IPC4_NOTIFY_MODULE_EVENTID_ALSA_MAGIC_VAL ((uint32_t)(0xA15A << 16))

/**
Expand All @@ -190,15 +191,19 @@ struct sof_ipc4_ctrl_value_chan {
/**
* struct sof_ipc4_control_msg_payload - IPC payload for kcontrol parameters
* @id: unique id of the control
* @num_elems: Number of elememnts in the chanv array
* @num_elems: Number of elements in the chanv array or number of bytes in data
* @reserved: reserved for future use, must be set to 0
* @chanv: channel ID and value array
* @data: binary payload
*/
struct sof_ipc4_control_msg_payload {
uint16_t id;
uint16_t num_elems;
uint32_t reserved[4];
struct sof_ipc4_ctrl_value_chan chanv[];
union {
struct sof_ipc4_ctrl_value_chan chanv[0];
uint8_t data[0];
};
} __attribute((packed, aligned(4)));

/**
Expand Down
33 changes: 33 additions & 0 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,36 @@ static int ipc4_set_get_config_module_instance(struct ipc4_message_request *ipc4
return ret;
Copy link
Collaborator

@kv2019i kv2019i Sep 12, 2025

Choose a reason for hiding this comment

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

typos in commit:

  • s/fro example/for example/
  • s/is coped/is copied/
  • s/would not worked/would not have worked/

}

__cold static void ipc4_prepare_for_kcontrol_get(struct comp_dev *dev, uint8_t param_id,
char *data_out, uint32_t data_size)
{
Copy link
Collaborator

@lyakh lyakh Sep 15, 2025

Choose a reason for hiding this comment

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

it's ok not to call assert_can_be_cold() here because this is a static function and its only caller already calls it. But maybe we should add a comment here to explain that, so that somebody in the future doesn't re-add it here by mistake

const char *hostbox;

#if CONFIG_LIBRARY
hostbox = (const char *)ipc_get()->comp_data + sizeof(struct ipc4_module_large_config);
#else
hostbox = (const char *)MAILBOX_HOSTBOX_BASE;
#endif

assert_can_be_cold();

switch (param_id) {
case SOF_IPC4_SWITCH_CONTROL_PARAM_ID:
case SOF_IPC4_ENUM_CONTROL_PARAM_ID:
case SOF_IPC4_BYTES_CONTROL_PARAM_ID:
/* We have payload in hostbox */
dcache_invalidate_region((__sparse_force void __sparse_cache *)MAILBOX_HOSTBOX_BASE,
data_size);

/* Copy the control payload header from inbox to outbox */
memcpy_s(data_out, data_size, hostbox,
sizeof(struct sof_ipc4_control_msg_payload));
break;
default:
break;
Copy link
Collaborator

@lyakh lyakh Sep 16, 2025

Choose a reason for hiding this comment

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

would we more often hit one of the three cases above or end up in default case here? If default is rare, we could unite the dcache_invalidate_region() calls above and in line 1175 to put it directly below line 1170. This is optional and can also be an incremental PR. But if we usually take the default path, then maybe it isn't a good idea.

Copy link
Contributor Author

@ujfalusi ujfalusi Sep 16, 2025

Choose a reason for hiding this comment

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

This will be highly topology dependent and we will only hit these where there is TDFB used (and it is used by user space as well) or we have sound dose enabled and actively used.

During runtime we are likely not even receiving GET_LARGE_CONFIG messages at all. I cannot say which will be the most frequent use in the future.

}
}

__cold static int ipc4_get_vendor_config_module_instance(struct comp_dev *dev,
const struct comp_driver *drv,
bool init_block,
Expand Down Expand Up @@ -1154,6 +1184,9 @@ __cold static int ipc4_get_large_config_module_instance(struct ipc4_message_requ
#if CONFIG_LIBRARY
data += sizeof(reply);
#endif
ipc4_prepare_for_kcontrol_get(dev, config.extension.r.large_param_id,
data, data_offset);

ret = drv->ops.get_large_config(dev, config.extension.r.large_param_id,
config.extension.r.init_block,
config.extension.r.final_block,
Expand Down
Loading