Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 150
Add extraction of new component properties from topology and pass them to modules with new ext_init object array.#5460
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
5f553a092ffbcd064fd55b7e8825File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -353,6 +353,10 @@ struct sof_ipc4_base_module_cfg { | ||
| #define SOF_IPC4_MOD_EXT_DOMAIN_MASK BIT(28) | ||
| #define SOF_IPC4_MOD_EXT_DOMAIN(x) ((x) << SOF_IPC4_MOD_EXT_DOMAIN_SHIFT) | ||
| #define SOF_IPC4_MOD_EXT_EXTENDED_INIT_SHIFT 29 | ||
| #define SOF_IPC4_MOD_EXT_EXTENDED_INIT_MASK BIT(29) | ||
| #define SOF_IPC4_MOD_EXT_EXTENDED_INIT(x) ((x) << SOF_IPC4_MOD_EXT_EXTENDED_SHIFT) | ||
| /* bind/unbind module ipc msg */ | ||
| #define SOF_IPC4_MOD_EXT_DST_MOD_ID_SHIFT 0 | ||
| #define SOF_IPC4_MOD_EXT_DST_MOD_ID_MASK GENMASK(15, 0) | ||
| @@ -616,6 +620,77 @@ struct sof_ipc4_notify_module_data { | ||
| #define SOF_IPC4_NOTIFY_MODULE_EVENTID_ALSA_MAGIC_VAL 0xA15A0000 | ||
| #define SOF_IPC4_NOTIFY_MODULE_EVENTID_ALSA_PARAMID_MASK GENMASK(15, 0) | ||
| /* | ||
| * Macros for creating struct sof_ipc4_module_init_ext_init payload | ||
| * with its associated data. ext_init payload should be the first | ||
| * piece of payload following SOF_IPC4_MOD_INIT_INSTANCE msg, and its | ||
| * existence is indicated with SOF_IPC4_MOD_EXT_EXTENDED-bit. | ||
| * | ||
| * The macros below apply to sof_ipc4_module_init_ext_init.word0 | ||
| */ | ||
| #define SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN_SHIFT 0 | ||
| #define SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN_MASK BIT(0) | ||
| #define SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN(x) ((x) << SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN_SHIFT) | ||
| #define SOF_IPC4_MOD_INIT_EXT_GNA_USED_SHIFT 1 | ||
| #define SOF_IPC4_MOD_INIT_EXT_GNA_USED_MASK BIT(1) | ||
| #define SOF_IPC4_MOD_INIT_EXT_GNA_USED(x) ((x) << SOF_IPC4_MOD_INIT_EXT_GNA_USED_SHIFT) | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_SHIFT 2 | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_MASK BIT(2) | ||
| #define SOF_IPC4_MOD_INIT_EXT_DATA_ARRAY(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_SHIFT) | ||
ranj063 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| struct sof_ipc4_module_init_ext_init { | ||
| u32 word0; | ||
| u32 rsvd1; | ||
| u32 rsvd2; | ||
| } __packed __aligned(4); | ||
| /* | ||
| * SOF_IPC4_MOD_EXT_EXTENDED payload may be followed by arbitrary | ||
| * number of object array objects. SOF_IPC4_MOD_INIT_EXT_DATA_ARRAY | ||
| * -bit indicates that an array object follows struct | ||
| * sof_ipc4_module_init_ext_init. | ||
| * | ||
| * The object header's SOF_IPC4_MOD_INIT_EXT_OBJ_LAST-bit in struct | ||
| * sof_ipc4_module_init_ext_object indicates if the array is continued | ||
| * with another object. The header has also fields to identify the | ||
| * object, SOF_IPC4_MOD_INIT_EXT_OBJ_ID, and to indicate the object's | ||
| * size in 32-bit words, SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS, not | ||
| * including the header itself. | ||
| * | ||
| * The macros below apply to sof_ipc4_module_init_ext_object.header | ||
| */ | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_SHIFT 0 | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_MASK BIT(0) | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_LAST(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_SHIFT) | ||
ranj063 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_ID_SHIFT 1 | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_ID_MASK GENMASK(15, 1) | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_ID(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_ID_SHIFT) | ||
ujfalusi marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS_SHIFT 16 | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS_MASK GENMASK(31, 16) | ||
| #define SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS_SHIFT) | ||
ujfalusi marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| struct sof_ipc4_module_init_ext_object { | ||
| u32 header; | ||
| u32 data[]; | ||
| } __packed __aligned(4); | ||
| enum sof_ipc4_mod_init_ext_obj_id { | ||
| SOF_IPC4_MOD_INIT_DATA_ID_INVALID = 0, | ||
| SOF_IPC4_MOD_INIT_DATA_ID_DP_DATA, | ||
| SOF_IPC4_MOD_INIT_DATA_ID_MAX = SOF_IPC4_MOD_INIT_DATA_ID_DP_DATA, | ||
| }; | ||
| /* DP module memory configuration data object for ext_init object array */ | ||
| struct sof_ipc4_mod_init_ext_dp_memory_data { | ||
| u32 domain_id; /* userspace domain ID */ | ||
| u32 stack_bytes; /* stack size in bytes, 0 means default size */ | ||
| u32 heap_bytes; /* stack size in bytes, 0 means default size */ | ||
| } __packed __aligned(4); | ||
| /** @}*/ | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -159,6 +159,12 @@ static const struct sof_topology_token comp_ext_tokens[] = { | ||
| offsetof(struct snd_sof_widget, core)}, | ||
| {SOF_TKN_COMP_SCHED_DOMAIN, SND_SOC_TPLG_TUPLE_TYPE_STRING, get_token_comp_domain, | ||
| offsetof(struct snd_sof_widget, comp_domain)}, | ||
| {SOF_TKN_COMP_DOMAIN_ID, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32, | ||
| offsetof(struct snd_sof_widget, dp_domain_id)}, | ||
| {SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32, | ||
| offsetof(struct snd_sof_widget, dp_heap_bytes)}, | ||
| {SOF_TKN_COMP_STACK_BYTES_REQUIREMENT, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32, | ||
| offsetof(struct snd_sof_widget, dp_stack_bytes)}, | ||
| }; | ||
| static const struct sof_topology_token gain_tokens[] = { | ||
| @@ -2946,13 +2952,85 @@ static int sof_ipc4_control_setup(struct snd_sof_dev *sdev, struct snd_sof_contr | ||
| return 0; | ||
| } | ||
| static int sof_ipc4_widget_setup_msg_payload(struct snd_sof_dev *sdev, | ||
jsarha marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. ranj063 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| struct snd_sof_widget *swidget, | ||
| struct sof_ipc4_msg *msg, | ||
| void *ipc_data, u32 ipc_size, | ||
| void **new_data) | ||
| { | ||
| struct sof_ipc4_mod_init_ext_dp_memory_data *dp_mem_data; | ||
| struct sof_ipc4_module_init_ext_init *ext_init; | ||
| struct sof_ipc4_module_init_ext_object *hdr; | ||
| int new_size; | ||
| u32 *payload; | ||
| u32 ext_pos; | ||
| /* For the moment the only reason for adding init_ext_init payload is DP | ||
| * memory data. If both stack and heap size are 0 (= use default), then | ||
| * there is no need for init_ext_init payload. | ||
| */ | ||
| if (swidget->comp_domain != SOF_COMP_DOMAIN_DP) { | ||
| msg->extension &= ~SOF_IPC4_MOD_EXT_EXTENDED_INIT_MASK; | ||
| return 0; | ||
| } | ||
| payload = kzalloc(sdev->ipc->max_payload_size, GFP_KERNEL); | ||
| if (!payload) | ||
| return -ENOMEM; | ||
| /* Add ext_init first and set objects array flag to 1 */ | ||
| ext_init = (struct sof_ipc4_module_init_ext_init *)payload; | ||
| ext_init->word0 |= SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_MASK; | ||
| ext_pos = DIV_ROUND_UP(sizeof(*ext_init), sizeof(u32)); | ||
| /* Add object array objects after ext_init */ | ||
| /* Add dp_memory_data if comp_domain indicates DP */ | ||
| if (swidget->comp_domain == SOF_COMP_DOMAIN_DP) { | ||
| hdr = (struct sof_ipc4_module_init_ext_object *)&payload[ext_pos]; | ||
| hdr->header = SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_MASK | | ||
| SOF_IPC4_MOD_INIT_EXT_OBJ_ID(SOF_IPC4_MOD_INIT_DATA_ID_DP_DATA) | | ||
| SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS(DIV_ROUND_UP(sizeof(*dp_mem_data), | ||
| sizeof(u32))); | ||
| ext_pos += DIV_ROUND_UP(sizeof(*hdr), sizeof(u32)); | ||
| dp_mem_data = (struct sof_ipc4_mod_init_ext_dp_memory_data *)&payload[ext_pos]; | ||
| dp_mem_data->domain_id = swidget->dp_domain_id; | ||
| dp_mem_data->stack_bytes = swidget->dp_stack_bytes; | ||
| dp_mem_data->heap_bytes = swidget->dp_heap_bytes; | ||
| ext_pos += DIV_ROUND_UP(sizeof(*dp_mem_data), sizeof(u32)); | ||
| } | ||
| /* If another array object is added, remember clear previous OBJ_LAST bit */ | ||
| /* Calculate final size and check that it fits to max payload size */ | ||
| new_size = ext_pos * sizeof(u32) + ipc_size; | ||
| if (new_size > sdev->ipc->max_payload_size) { | ||
| dev_err(sdev->dev, "Max ipc payload size %lu exceeded: %u", | ||
| sdev->ipc->max_payload_size, new_size); | ||
| kfree(payload); | ||
| return -EINVAL; | ||
jsarha marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
ujfalusi marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| *new_data = payload; | ||
| /* Copy module specific ipc_payload to end */ | ||
| memcpy(&payload[ext_pos], ipc_data, ipc_size); | ||
| /* Update msg extension bits according to the payload changes */ | ||
| msg->extension |= SOF_IPC4_MOD_EXT_EXTENDED_INIT_MASK; | ||
| msg->extension &= ~SOF_IPC4_MOD_EXT_PARAM_SIZE_MASK; | ||
| msg->extension |= SOF_IPC4_MOD_EXT_PARAM_SIZE(DIV_ROUND_UP(new_size, sizeof(u32))); | ||
| return new_size; | ||
| } | ||
| static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget) | ||
| { | ||
| struct snd_sof_widget *pipe_widget = swidget->spipe->pipe_widget; | ||
| struct sof_ipc4_fw_data *ipc4_data = sdev->private; | ||
| struct sof_ipc4_pipeline *pipeline; | ||
| struct sof_ipc4_msg *msg; | ||
| void *ipc_data = NULL; | ||
| void *ext_data = NULL; | ||
| u32 ipc_size = 0; | ||
| int ret; | ||
| @@ -3097,6 +3175,16 @@ static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget | ||
| dev_dbg(sdev->dev, "Create widget %s (pipe %d) - ID %d, instance %d, core %d\n", | ||
| swidget->widget->name, swidget->pipeline_id, module_id, | ||
| swidget->instance_id, swidget->core); | ||
| ret = sof_ipc4_widget_setup_msg_payload(sdev, swidget, msg, ipc_data, ipc_size, | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jsarha does it make sense to move this to each widget's setup op? I mean this is static data parsed from the topology right? So it isnt going to change from every instance of start/stop isnt it? CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it there only because that is where the IPC payload is put together. The ext_init payload could be created already at sof_ipc4_widget_setup_msg() and store it somewhere, but we would still have to allocate memory and concatenate different payloads together in sof_ipc4_widget_setup(), so the benefit would be mostly cosmetic. That is unless I take on a bigger rework, find a standard place for setup msg payloads in snd_sof_widget for all modules (except pipelines), and put custom made payloads there. That would still only be a trade from saved cycles to consumed memory. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jsarha why would you need to allocate memory only here? You can do the same earlier as well. CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the payloads are module specific, initialized where ever, and not having common place in sof_widget. The ext_init payload however would not depend on the module its using. It would be relatively straight forward to add a pointer for storing the ext_init payload to sof_widget in setup_msg(). However, the ext_init payload should still be concatenated with the current module specific payload in widget_setup(). I guess it would be possible to create the full IPC payloads also already at topology read phase. That would however require fixing the current situation, where there is no common place for init message payloads, and each module code stores it somewhere in its own private memory space. | ||
| &ext_data); | ||
| if (ret < 0) | ||
| goto fail; | ||
| if (ret > 0) { | ||
| ipc_size = ret; | ||
| ipc_data = ext_data; | ||
| } | ||
| } else { | ||
| dev_dbg(sdev->dev, "Create pipeline %s (pipe %d) - instance %d, core %d\n", | ||
| swidget->widget->name, swidget->pipeline_id, | ||
| @@ -3107,6 +3195,8 @@ static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget | ||
| msg->data_ptr = ipc_data; | ||
| ret = sof_ipc_tx_message_no_reply(sdev->ipc, msg, ipc_size); | ||
| fail: | ||
| if (ret < 0) { | ||
| dev_err(sdev->dev, "failed to create module %s\n", swidget->widget->name); | ||
| @@ -3119,6 +3209,7 @@ static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget | ||
| } | ||
| } | ||
| kfree(ext_data); | ||
| return ret; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as to FW 10064. We already have a concept of "extended init". E.g. we have:
All modules have base_config, but some have base_config_ext additionally. This new PR (and the bit29) seems to add a way to extend the module init data, but still keeps the old "base_config_ext" around as well. Having too "extended" module configs is highly confusing. Can we call this new variant something clearly different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I address all the other comments first and push a new version. I take on this tomorrow, when I am certain we have an agreement on what to do. Let's continue the discussion on FW PR side.