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

[Cezary Rojewski]

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 runtime 
parameters.

>> + * @mod_idas: Module instance ID pool, one per module-type
>> + * @modres_mutex: For synchronizing any @mods_info updates
>> + * @ppl_ida: Pipeline instance ID pool
>> + * @fw_list: List of libraries loaded, including base firmware
>>    */
>>   struct avs_dev {
>>       struct hda_bus base;
>> @@ -68,6 +82,14 @@ struct avs_dev {
>>       const struct avs_spec *spec;
>>       struct avs_ipc *ipc;
>> +    struct avs_fw_cfg fw_cfg;
>> +    struct avs_hw_cfg hw_cfg;
>> +    struct avs_mods_info *mods_info;
>> +    struct ida **mod_idas;
>> +    struct mutex modres_mutex;
>> +    struct ida ppl_ida;
>> +    struct list_head fw_list;
>> +
>>       struct completion fw_ready;
>>   };

...

>> +    if (tocopy_count)
>> +        kfree(adev->mod_idas);
>> +    else
>> +        avs_module_ida_destroy(adev);
>> +
>> +    adev->mod_idas = ida_ptrs;
>> +    return 0;
>> +}
> 
>> +int avs_module_id_alloc(struct avs_dev *adev, u16 module_id)
>> +{
>> +    int ret, idx, max_id;
>> +
>> +    mutex_lock(&adev->modres_mutex);
>> +
>> +    idx = avs_module_id_entry_index(adev, module_id);
>> +    if (idx == -ENOENT) {
>> +        WARN(1, "invalid module id: %d", module_id);
> 
> dev_err() seems to be more than enough, why would you add a complete 
> call trace?
> 
>> +        ret = -EINVAL;
>> +        goto exit;
>> +    }
>> +    max_id = adev->mods_info->entries[idx].instance_max_count - 1;
>> +    ret = ida_alloc_max(adev->mod_idas[idx], max_id, GFP_KERNEL);
>> +exit:
>> +    mutex_unlock(&adev->modres_mutex);
>> +    return ret;
>> +}
>> +
>> +void avs_module_id_free(struct avs_dev *adev, u16 module_id, u8 
>> instance_id)
>> +{
>> +    int idx;
>> +
>> +    mutex_lock(&adev->modres_mutex);
>> +
>> +    idx = avs_module_id_entry_index(adev, module_id);
>> +    if (idx == -ENOENT) {
>> +        WARN(1, "invalid module id: %d", module_id);
> 
> same WARN is over-engineered.


Ack for both.


Regards,
Czarek