[PATCH 07/17] ASoC: Intel: avs: Add module management requests
Cezary Rojewski
cezary.rojewski at intel.com
Mon Feb 28 16:26:22 CET 2022
On 2022-02-25 9:44 PM, Pierre-Louis Bossart wrote:
> 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.
Added kernel-doc for said function to address this.
>> 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.
Ack.
>>
>>>> + 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.
Ack.
>>>> +}
>>>> +
>>>> +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...
Ack.
More information about the Alsa-devel
mailing list