On 2/25/22 12:50, Cezary Rojewski wrote:
> 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
how do you expect people with no access to those specs to understand
this code then?
You have to describe the concepts in vague-enough terms that someone
familiar with DSPs can understand.
> 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.
add comments then.
>
>>> + 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.
add a comment then.
>
>>> +
>>> + 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.
well, that's worthy of a comment...
>
>>> +{
>>> + 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;
>>> +}
