IPC4: add support for generic bytes control and fix LARGE_CONFIG_GET for generic controls. #10235
Conversation
ujfalusi
requested review from
RanderWang,
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
marcinszkudlinski,
mmaka1,
pblaszko,
plbossart and
ranj063
as code owners
ujfalusi
force-pushed
the
peter/pr/generic-bytes-control-01
branch
from
682892d to
2306509
Compare
src/ipc/ipc4/handler.c
Outdated
| /* Copy the control payload header from inbox to outbox */ | ||
| memcpy_s(data_out, data_size, | ||
| data_in, sizeof(struct sof_ipc4_control_msg_payload)); | ||
| } |
There was a problem hiding this comment.
Reads easier if the memcpy is part of the switch {} ie. the action is contained within the decision block
| if (!set) | ||
| msg_reply.extension = config->extension.dat; | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
typos in commit:
- s/fro example/for example/
- s/is coped/is copied/
- s/would not worked/would not have worked/
Add definition for generic bytes control and extend the sof_ipc4_control_msg_payload struct to be able to handle bytes data as well. In other terms, the generic bytes control can be handled in similar way as the ENUM or SWITCH generic control. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi
force-pushed
the
peter/pr/generic-bytes-control-01
branch
from
2306509 to
280336d
Compare
|
Changes since v1:
|
singalsu
left a comment
singalsu
left a comment
There was a problem hiding this comment.
Code works and looks good to me.
src/ipc/ipc4/handler.c
Outdated
| } | ||
|
|
||
| __cold static void | ||
| ipc4_prepare_for_kcontrol_get(struct comp_dev *dev, uint8_t param_id, |
There was a problem hiding this comment.
the two lines can be trivially joined without exceeding the maximum line length
| ipc4_prepare_for_kcontrol_get(struct comp_dev *dev, uint8_t param_id, | ||
| uint32_t data_size, const char *data_in, | ||
| char *data_out) | ||
| { |
There was a problem hiding this comment.
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
ujfalusi
force-pushed
the
peter/pr/generic-bytes-control-01
branch
from
280336d to
8cbbff8
Compare
|
Changes since v2:
|
This version runs without problems in MTL platform. Thanks Peter! |
ujfalusi
force-pushed
the
peter/pr/generic-bytes-control-01
branch
from
8cbbff8 to
5990229
Compare
Some LARGE_CONFIG_GET message includes payload to provide instructions to the handler modules on how to fulfill the request. The generic control messages are such examples (they are not related to VENDOR_CONFIG_PARAM messages): The payload from host includes the sof_ipc4_control_msg_payload struct to describe the ID of the control for example. Alone the type (enum, switch or bytes) is not enough to know _which_ control's information is requested. Similarly, the control's ID does not identify the control's type. When such generic control message is received, the payload is copied from hostbox to the comp_data and the handler module can inspect it before overwriting it with the requested data. Note: currently we don't have modules which would handle generic control get functionality, but they would not have worked, because the information was not available to them to decide which control's data needs to be returned. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
|
Changes since v3:
|
| sizeof(struct sof_ipc4_control_msg_payload)); | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
5 participants
Add definition for generic bytes control and fix the get functionality
The LARGE_CONFIG_GET for generic controls have been broken (no module implements it) because the information about the control's ID was not propagated to the modules.
kernel PR: thesofproject/linux#5531