ASoC: SOF: ipc4: Add support for generic bytes control to support notifications #5531
Conversation
ujfalusi
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
plbossart and
ranj063
as code owners
|
@ujfalusi what is the intent for the notifications and the generic bytes control here? Why does the firmware need to send a notification when the data changes? Why can't it simply update the bytes control data directly instead of sending a notification to the kernel and then the kernel to modify the control data based on it? |
It is for similar use cases as for the enum and switch type: user space does not need to poll for changes, it is notified when the content of the control changes, then it can read the changed data. How would user space know that the data is changed if there is no notification? How can the kernel know if the data is changed if there is no notification? |
ujfalusi
force-pushed
the
peter/sof/pr/ipc4-generic-bytes-control-01
branch
from
1ca22ea to
1b03a2d
Compare
When the bytes control have no data (payload) then there is no need to send an IPC message as there is nothing to send. Fixes: a062c88 ("ASoC: SOF: ipc4-control: Add support for bytes control get and put") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The size of the data behind of scontrol->ipc_control_data for bytes controls is: [1] sizeof(struct sof_ipc4_control_data) + // kernel only struct [2] sizeof(struct sof_abi_hdr)) + payload The max_size specifies the size of [2] and it is coming from topology. Change the function to take this into account and allocate adequate amount of memory behind scontrol->ipc_control_data. With the change we will allocate [1] amount more memory to be able to hold the full size of data. Fixes: 1bfde58 ("ASoC: SOF: ipc4-topology: add byte kcontrol support") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ol_data The size of the data behind scontrol->ipc_control_data is stored in scontrol->size, use this when copying data for backup/restore. Fixes: 03f5376 ("ASoC: sof: Improve sof_ipc4_bytes_ext_put function") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
When the bytes data is read from the firmware, the size of the payload can be different than what it was previously. For example when the topology did not contained payload data at all for the control, the data size was 0. For get operation allow maximum size of payload to be read and then update the sizes according to the completed message. Similarly, keep the size in sync when updating the data in firmware. With the change we will be able to read data from firmware for bytes controls which did not had initial payload defined in topology. Fixes: a062c88 ("ASoC: SOF: ipc4-control: Add support for bytes control get and put") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Set the param_id in extension based on the information we got from the topology. If the payload did not present then the param_id will remain 0. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
There are message types when we would need to send a payload along with the LARGE_CONFIG_GET message to provide information to the firmware on what data is requested. Such cases are the ALSA Kcontrol related messages when the high level param_id tells only the type of the control, but the ID/index of the exact control is specified in the payload area. The caller must place the payload for TX before calling the set_get_data() and this payload will be sent alongside with the message to the firmware. The data area will be overwritten by the received data from firmware. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Currently IPC4 only supports module specific custom bytes controls, where each control's param_id is module specific. These bytes controls cannot be handled in a generic way, there is no clean way to support for example notifications from firmware when their data has been changed. Add definition for generic bytes control which should be handled in a similar way as the enum/switch controls. The generic param id for BYTES is selected to be 202 Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The generic byte control can be used in cases when the bytes data can be changed by the firmware and it sends a notification about the change, similarly to the enum and switch controls. The generic control support is needed as from the param_id itself it is not possible to know which control has changed. The needed information is only available via generic control change notification. Generic bytes controls use param_id 202 and their change notification can contain payload with the change embedded or just the header message as notification. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
|
Changes since v1:
|
singalsu
left a comment
singalsu
left a comment
There was a problem hiding this comment.
Just tested this new version after small update, works well as did the previous. This is a very useful and important update for bytes control feature. Thanks Peter!
|
SOFCI TEST |
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Thanks for the verbose comments in the early patches. The multiple places where size of messages (and allocated buffers) are tracked are hard to follow, but as far as I can tell, this is all correct and fixes multiple issues in existing implementation.
| */ | ||
| memcpy(scontrol->ipc_control_data, scontrol->old_ipc_control_data, | ||
| scontrol->max_size); | ||
| scontrol->size); |
There was a problem hiding this comment.
Ok this took a while to parse, but I think this is correct. So without this patch, incorrect amount was copied as:
- too little copied as scontrol->max_size doesn't cover the internal header struct
- too much copied as scontrol->size may be smaller than max_size
There was a problem hiding this comment.
yes, around those lines, we should copy what is available and yes, max_size is the payload's maximum, not the kernel internal buffer's size.
It is really confusing and took quite a while to unravel this for fixing.
There was a problem hiding this comment.
fwiw, the max_size was always bigger than the real data+header, so it was working OK, but it was incorrect at the same time.
5 participants
We support bytes control type for set and get, but these are module specific
controls and there is no way to handle notifications from them in a generic way.
Each control have module specific param_id and this param_id is only valid in
the module's scope, other modules might use the same id for different functions
for example.
This series will add a new generic control type, similar to the existing ones
for ENUM and SWITCH, which can be used to create bytes controls which can send
notifications from firmware on change.
The new param_id is 202 and the sof_ipc4_control_msg_payload is updated to
describe bytes payload also.
On set, the payload must include the sof_ipc4_control_msg_payload struct with
the control's ID and data size, followed by the data.
On get, the kernel needs to send the sof_ipc4_control_msg_payload struct along
with the LARGE_CONFIG_GET message as payload with the ID of the control that
needs to be retrieved. The raw data is received back without additional header.
A notification might contain data, in this case the num_elems reflects the size
in bytes, or without data. If no data is received then the control is marked as
dirty and on read the kernel will refresh the data from firmware.
The series includes mandatory fixes for existing code and adds support for
sending payload with LARGE_CONFIG_GET when the param_id is either generic ENUM,
SWITCH or BYTES control.