Re: [PATCH v4 11/17] ASoC: Intel: avs: Firmware resources management utilities

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 3/11/22 11:20, Cezary Rojewski wrote:
> On 2022-03-11 4:59 PM, Pierre-Louis Bossart wrote:
>> On 3/11/22 09:46, Cezary Rojewski wrote:
>>> On 2022-03-09 11:36 PM, Pierre-Louis Bossart wrote:
>>>>
>>>>>   /*
>>>>>    * struct avs_dev - Intel HD-Audio driver data
>>>>>    *
>>>>>    * @dev: PCI device
>>>>>    * @dsp_ba: DSP bar address
>>>>>    * @spec: platform-specific descriptor
>>>>> + * @fw_cfg: Firmware configuration, obtained through FW_CONFIG 
>>>>> message
>>>>> + * @hw_cfg: Hardware configuration, obtained through HW_CONFIG 
>>>>> message
>>>>> + * @mods_info: Available module-types, obtained through 
>>>>> MODULES_INFO message
>>>>
>>>> is this just for the base firmware? If this includes the extensions, 
>>>> how are the module types defined?
>>>
>>>
>>> Only base firmware is able to process MODULE_INFO getter. So, every 
>>> time driver loads a library, this info gets updated internally on the 
>>> firmware side. We make use of said getter to retrieve up-to-date 
>>> information and cache in ->mods_info for later use. ->mods_info is a 
>>> member of type struct avs_mods_info with each enter represented by 
>>> struct avs_module_info. These are introduced with all the basefw 
> 
> Sorry for the typo: s/avs_module_info/avs_module_entry/.
> 
>>> runtime parameters.
>>
>> you clarified the mechanism but not the definition of 'module-type'?
>>
>> the definition doesn't really help.
>>
>> struct avs_module_type {
>>      u32 load_type:4;
>>      u32 auto_start:1;
>>      u32 domain_ll:1;
>>      u32 domain_dp:1;
>>      u32 lib_code:1;
>>      u32 rsvd:24;
>> } __packed;
>>
>> I see nothing that would e.g. identify a mixer from a gain. The 
>> definition of 'type' seems to refer to low-level properties, not what 
>> the module actually does?
> 
> 
> There is no "module-type" enum that software can rely on. We rely on 
> hardcoded GUIDs instead. "module-type" is represented by struct 
> avs_module_entry (per type) in context of MODULE_INFO IPC. All these 
> names are indented to match firmware equivalents to make it easier to 
> switch between the two worlds.

So the initial kernel-doc I commented on is still quite convoluted, you 
are referring to a 'module-type' that's not really well defined or 
useful for a driver.

Given the definition:

struct avs_mods_info {
	u32 count;
	struct avs_module_entry entries[];
} __packed;


wouldn't this be simpler/less confusing:

"
@mods_info: Available array of module entries, obtained through
MODULES_INFO message
"

?