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

[Cezary Rojewski]

On 2022-02-25 2:27 AM, Pierre-Louis Bossart wrote:
>> +int avs_ipc_init_instance(struct avs_dev *adev, u16 module_id, u8 instance_id,
>> +			  u8 ppl_id, u8 core_id, u8 domain,
> 
> you should explain the relationship between ppl_id and core_id. It seems
> that in the same pipeline different modules instances can be pegged to
> different cores, which isn't very intuitive given the previous
> explanation that a pipeline is a scheduling unit.
> 
> The domain as a u8 is not very clear either, I was under the impression
> there were only two domains (LL and EDF)?


Hmm.. such explanations are supposed to be part of HW or FW 
specifications. I don't believe kernel is a place for that. Fields found 
here are needed to provide all the necessary information firmware 
expects when requesting INIT_INSTANCE. What's possible and how's 
everything handled internally is for firmware to decide and explain. 
There are no if-statements in the driver's code that force 
ppl_id/core_id relation so I don't see why reader would get an 
impression there is some dependency. What's in the topology gets routed 
to firmware with help of above function.

Just to confirm: yes, you can have multiple cores engaged in servicing 
modules found in single pipelines.

In regard to field name/sizes: again, these match firmware equivalents 
1:1 so it's easy to switch back and forth.

>> +			  void *param, u32 param_size)
>> +{
>> +	union avs_module_msg msg = AVS_MODULE_REQUEST(INIT_INSTANCE);
>> +	struct avs_ipc_msg request;
>> +	int ret;
>> +
>> +	msg.module_id = module_id;
>> +	msg.instance_id = instance_id;
>> +	/* firmware expects size provided in dwords */
>> +	msg.ext.init_instance.param_block_size =
>> +			DIV_ROUND_UP(param_size, sizeof(u32));
>> +	msg.ext.init_instance.ppl_instance_id = ppl_id;
>> +	msg.ext.init_instance.core_id = core_id;
>> +	msg.ext.init_instance.proc_domain = domain;
>> +
>> +	request.header = msg.val;
>> +	request.data = param;
>> +	request.size = param_size;
> 
> isn't there a need to check if the module can be initialized? there's
> got to be some dependency on pipeline state?


IPC handlers found in message.c have one and only one purpose only: send 
a message. Firmware will return an error if arguments passed are invalid.

Also, note that ALSA/ASoC already have a working state machine for 
streaming. There is no reason to re-implement it here.

>> +
>> +	ret = avs_dsp_send_msg(adev, &request, NULL);
>> +	if (ret)
>> +		avs_ipc_err(adev, &request, "init instance", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +int avs_ipc_delete_instance(struct avs_dev *adev, u16 module_id, u8 instance_id)
>> +{
>> +	union avs_module_msg msg = AVS_MODULE_REQUEST(DELETE_INSTANCE);
>> +	struct avs_ipc_msg request = {0};
>> +	int ret;
>> +
>> +	msg.module_id = module_id;
>> +	msg.instance_id = instance_id;
>> +	request.header = msg.val;
>> +
>> +	ret = avs_dsp_send_msg(adev, &request, NULL);
>> +	if (ret)
>> +		avs_ipc_err(adev, &request, "delete instance", ret);
>> +
>> +	return ret;
> 
> same here, can this be used in any pipeline state?


Ditto.

>> +}
>> +
>> +int avs_ipc_bind(struct avs_dev *adev, u16 module_id, u8 instance_id,
>> +		 u16 dst_module_id, u8 dst_instance_id,
>> +		 u8 dst_queue, u8 src_queue)
> 
> what does a queue represent?


In firmware's nomenclature pin/index/queue are synonyms when speaking 
about module instances.

>> +{
>> +	union avs_module_msg msg = AVS_MODULE_REQUEST(BIND);
>> +	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, "bind modules", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +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;
> 
> can this be merged with the bind in a helper, the code looks
> quasi-identical with just two lines different.


We had these two coupled together in the past just like you mention. 
Lately we'd decided that having two if-statements (one for message type 
and the other for error message) just two reduce file by just few lines 
is not worth it. So we chose the readability over small file-size gain.