Re: [PATCH v3 07/17] ASoC: Intel: avs: Add module management requests

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-03-04 5:21 PM, Ranjani Sridharan wrote:
> On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote:

...

>> +/*
>> + * avs_ipc_delete_instance - Delete module instance
>> + *
>> + * @adev: Driver context
>> + * @module_id: Module-type id
>> + * @instance_id: Unique module instance id
>> + *
>> + * Argument verification, as well as pipeline state checks are done
>> by the
>> + * firmware.
>> + *
>> + * Note: only standalone modules i.e. without a parent pipeline
>> shall be
>> + * deleted using this IPC message. In all other cases, pipeline
>> owning the
>> + * modules peforms cleanup automatically when it is deleted.
> Can you please provide an example of such stand-alone modules? If they
> aren't part of any pipeline, how do they get scheduled?


Thanks for feedback! Consider dropping the unnecessary bits so it is 
easier to navigate through your responses.

Please note: kernel mailing list is not for explaining SW <-> FW 
communication details. Feel free to contact my colleagues from firmware 
team if in need of any FW-iface details.

That goes for most of the comments found below too.

>> +/*
>> + * avs_ipc_unbind - Unbind two module instances
>> + *
>> + * @adev: Driver context
>> + * @module_id: Source module-type id
>> + * @instance_id: Source module instance id
>> + * @dst_module_id: Sink module-type id
>> + * @dst_instance_id: Sink module instance id
>> + * @dst_queue: Sink module pin to unbind @src_queue from
>> + * @src_queue: Source module pin to unbind @dst_queue from
>> + */
> Are there any rules for unbinding? For example if you have 2 modules
> connected to a mixer? Can you unbind the module belonging to the host
> pipeline that is getting stopped while the mixer is still active?


Here we have just a delegate. All the rules are defined and enforced by 
the firmware.

>> +int avs_ipc_unbind(struct avs_dev *adev, u16 module_id, u8
>> instance_id,
>> +		   u16 dst_module_id, u8 dst_instance_id,
>> +		   u8 dst_queue, u8 src_queue)
>> +{
>> +	union avs_module_msg msg = AVS_MODULE_REQUEST(UNBIND);
>> +	struct avs_ipc_msg request = {{0}};
>> +	int ret;
>> +
>> +	msg.module_id = module_id;
>> +	msg.instance_id = instance_id;
>> +	msg.ext.bind_unbind.dst_module_id = dst_module_id;
>> +	msg.ext.bind_unbind.dst_instance_id = dst_instance_id;
>> +	msg.ext.bind_unbind.dst_queue = dst_queue;
>> +	msg.ext.bind_unbind.src_queue = src_queue;
>> +	request.header = msg.val;
>> +
>> +	ret = avs_dsp_send_msg(adev, &request, NULL);
>> +	if (ret)
>> +		avs_ipc_err(adev, &request, "unbind modules", ret);
>> +
>> +	return ret;
>> +}

...

>> +int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8
>> instance_id,
>> +			     u8 param_id, u8 *request_data, size_t
>> request_size,
>> +			     u8 **reply_data, size_t *reply_size)
>> +{
>> +	union avs_module_msg msg =
>> AVS_MODULE_REQUEST(LARGE_CONFIG_GET);
>> +	struct avs_ipc_msg request;
>> +	struct avs_ipc_msg reply = {{0}};
>> +	size_t size;
>> +	void *buf;
>> +	int ret;
>> +
>> +	reply.data = kzalloc(AVS_MAILBOX_SIZE, GFP_KERNEL);
>> +	if (!reply.data)
>> +		return -ENOMEM;
>> +
>> +	msg.module_id = module_id;
>> +	msg.instance_id = instance_id;
>> +	msg.ext.large_config.data_off_size = request_size;
>> +	msg.ext.large_config.large_param_id = param_id;
>> +	/* final_block is always 0 on request. Updated by fw on reply.
>> */
>> +	msg.ext.large_config.final_block = 0;
>> +	msg.ext.large_config.init_block = 1;
>> +
>> +	request.header = msg.val;
>> +	request.data = request_data;
>> +	request.size = request_size;
>> +	reply.size = AVS_MAILBOX_SIZE;
>> +
>> +	ret = avs_dsp_send_msg(adev, &request, &reply);
>> +	if (ret) {
>> +		avs_ipc_err(adev, &request, "large config get", ret);
>> +		kfree(reply.data);
>> +		return ret;
>> +	}
> How come you dont have a loop here? What if the rec'd data size if
> larger than the max size of IP payload?


That's not how LARGE_CONFIG_GET message works. There is no looping 
involved or expected by the firmware and so we don't have it here.


Regards,
Czarek