| Message ID | 20220309204029.89040-1-cezary.rojewski@intel.com |
|---|---|
| Headers | show |
| Series | ASoC: Intel: AVS - Audio DSP for cAVS | expand |
> diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile > new file mode 100644 > index 000000000000..5f7976a95fe2 > --- /dev/null > +++ b/sound/soc/intel/avs/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +snd-soc-avs-objs := dsp.o > + > +obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o nit-pick: snd-soc-intel-avs? avs is not a clear mapping to Intel platforms > +/* > + * struct avs_dev - Intel HD-Audio driver data > + * > + * @dev: PCI device > + * @dsp_ba: DSP bar address not sure it's only limited to DSP, is it? > + * @spec: platform-specific descriptor > + */ > +struct avs_dev { > + struct hda_bus base; > + struct device *dev; > + > + void __iomem *dsp_ba; > + const struct avs_spec *spec; > +}; > +int avs_dsp_core_reset(struct avs_dev *adev, u32 core_mask, bool reset) > +{ > + u32 value, mask, reg; > + int ret; > + > + mask = AVS_ADSPCS_CRST_MASK(core_mask); > + value = reset ? mask : 0; > + > + snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPCS, mask, value); > + > + ret = snd_hdac_adsp_readl_poll(adev, AVS_ADSP_REG_ADSPCS, > + reg, (reg & mask) == value, > + AVS_ADSPCS_INTERVAL_US, > + AVS_ADSPCS_TIMEOUT_US); > + if (ret) > + dev_err(adev->dev, "core_mask %d %s reset failed: %d\n", > + core_mask, reset ? "enter" : "exit", ret); nit-pick: reset %s "entry" "exit" > + > + return ret; > +} > +
> +/* > + * avs_ipc_delete_instance - Delete module instance > + * > + * @adev: Driver context > + * @module_id: Module-type id > + * @instance_id: Unique module instance id > + * > + * Argument verification, as well as pipeline state checks are done by the > + * firmware. > + * > + * Note: only standalone modules i.e. without a parent pipeline shall be > + * deleted using this IPC message. In all other cases, pipeline owning the > + * modules peforms cleanup automatically when it is deleted. typo: performs checkpatch.pl --strict --codespell would detect all this for you. > + */ > +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; > +}
> /* > * 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? > + * @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; > }; > +/* Caller responsible for holding adev->modres_mutex. */ > +static int > +avs_module_ida_alloc(struct avs_dev *adev, struct avs_mods_info *newinfo, bool purge) > +{ > + struct avs_mods_info *oldinfo = adev->mods_info; > + struct ida **ida_ptrs; > + u32 tocopy_count = 0; > + int i; > + > + if (!purge && oldinfo) { > + if (oldinfo->count >= newinfo->count) > + dev_warn(adev->dev, "refreshing %d modules info with %d\n", > + oldinfo->count, newinfo->count); > + tocopy_count = oldinfo->count; > + } > + > + ida_ptrs = kcalloc(newinfo->count, sizeof(*ida_ptrs), GFP_KERNEL); > + if (!ida_ptrs) > + return -ENOMEM; > + > + if (tocopy_count) > + memcpy(ida_ptrs, adev->mod_idas, tocopy_count * sizeof(*ida_ptrs)); > + > + for (i = tocopy_count; i < newinfo->count; i++) { > + ida_ptrs[i] = kzalloc(sizeof(**ida_ptrs), GFP_KERNEL); > + if (!ida_ptrs[i]) { > + while (i--) > + kfree(ida_ptrs[i]); it's a bit hairy to play with the loop counter, I would jump to an error handling label to make it clearer. > + > + kfree(ida_ptrs); > + return -ENOMEM; > + } > + > + ida_init(ida_ptrs[i]); > + } > + > + /* If old elements have been reused, don't wipe them. */ the comment is very odd, there's either a free() or a destroy() happening below... > + 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. > + goto exit; > + } > + > + ida_free(adev->mod_idas[idx], instance_id); > +exit: > + mutex_unlock(&adev->modres_mutex); > +} > + > +/* I am running out of time and will resume this review next week.
On Wed, 09 Mar 2022 21:40:13 +0100, Cezary Rojewski wrote: > > HDAudio drivers make heavy use of I/O operations. Declare a range of > update, read and write helpers similar to those available for HDAudio > legacy driver. These macros are used by AVS driver to improve code > readability. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> Acked-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi > --- > include/sound/hdaudio.h | 2 ++ > include/sound/hdaudio_ext.h | 50 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h > index 6a90ce405e60..69907260b9ce 100644 > --- a/include/sound/hdaudio.h > +++ b/include/sound/hdaudio.h > @@ -448,6 +448,8 @@ static inline u16 snd_hdac_reg_readw(struct hdac_bus *bus, void __iomem *addr) > > #define snd_hdac_reg_writel(bus, addr, val) writel(val, addr) > #define snd_hdac_reg_readl(bus, addr) readl(addr) > +#define snd_hdac_reg_writeq(bus, addr, val) writeq(val, addr) > +#define snd_hdac_reg_readq(bus, addr) readq(addr) > > /* > * macros for easy use > diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h > index b0c8e4936168..d26234f9ee46 100644 > --- a/include/sound/hdaudio_ext.h > +++ b/include/sound/hdaudio_ext.h > @@ -2,6 +2,8 @@ > #ifndef __SOUND_HDAUDIO_EXT_H > #define __SOUND_HDAUDIO_EXT_H > > +#include <linux/io-64-nonatomic-lo-hi.h> > +#include <linux/iopoll.h> > #include <sound/hdaudio.h> > > int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev, > @@ -144,6 +146,54 @@ void snd_hdac_ext_bus_link_power(struct hdac_device *codec, bool enable); > writew(((readw(addr + reg) & ~(mask)) | (val)), \ > addr + reg) > > +#define snd_hdac_adsp_writeb(chip, reg, value) \ > + snd_hdac_reg_writeb(chip, (chip)->dsp_ba + (reg), value) > +#define snd_hdac_adsp_readb(chip, reg) \ > + snd_hdac_reg_readb(chip, (chip)->dsp_ba + (reg)) > +#define snd_hdac_adsp_writew(chip, reg, value) \ > + snd_hdac_reg_writew(chip, (chip)->dsp_ba + (reg), value) > +#define snd_hdac_adsp_readw(chip, reg) \ > + snd_hdac_reg_readw(chip, (chip)->dsp_ba + (reg)) > +#define snd_hdac_adsp_writel(chip, reg, value) \ > + snd_hdac_reg_writel(chip, (chip)->dsp_ba + (reg), value) > +#define snd_hdac_adsp_readl(chip, reg) \ > + snd_hdac_reg_readl(chip, (chip)->dsp_ba + (reg)) > +#define snd_hdac_adsp_writeq(chip, reg, value) \ > + snd_hdac_reg_writeq(chip, (chip)->dsp_ba + (reg), value) > +#define snd_hdac_adsp_readq(chip, reg) \ > + snd_hdac_reg_readq(chip, (chip)->dsp_ba + (reg)) > + > +#define snd_hdac_adsp_updateb(chip, reg, mask, val) \ > + snd_hdac_adsp_writeb(chip, reg, \ > + (snd_hdac_adsp_readb(chip, reg) & ~(mask)) | (val)) > +#define snd_hdac_adsp_updatew(chip, reg, mask, val) \ > + snd_hdac_adsp_writew(chip, reg, \ > + (snd_hdac_adsp_readw(chip, reg) & ~(mask)) | (val)) > +#define snd_hdac_adsp_updatel(chip, reg, mask, val) \ > + snd_hdac_adsp_writel(chip, reg, \ > + (snd_hdac_adsp_readl(chip, reg) & ~(mask)) | (val)) > +#define snd_hdac_adsp_updateq(chip, reg, mask, val) \ > + snd_hdac_adsp_writeq(chip, reg, \ > + (snd_hdac_adsp_readq(chip, reg) & ~(mask)) | (val)) > + > +#define snd_hdac_adsp_readb_poll(chip, reg, val, cond, delay_us, timeout_us) \ > + readb_poll_timeout((chip)->dsp_ba + (reg), val, cond, \ > + delay_us, timeout_us) > +#define snd_hdac_adsp_readw_poll(chip, reg, val, cond, delay_us, timeout_us) \ > + readw_poll_timeout((chip)->dsp_ba + (reg), val, cond, \ > + delay_us, timeout_us) > +#define snd_hdac_adsp_readl_poll(chip, reg, val, cond, delay_us, timeout_us) \ > + readl_poll_timeout((chip)->dsp_ba + (reg), val, cond, \ > + delay_us, timeout_us) > +#define snd_hdac_adsp_readq_poll(chip, reg, val, cond, delay_us, timeout_us) \ > + readq_poll_timeout((chip)->dsp_ba + (reg), val, cond, \ > + delay_us, timeout_us) > +#define snd_hdac_stream_readb_poll(strm, reg, val, cond, delay_us, timeout_us) \ > + readb_poll_timeout((strm)->sd_addr + AZX_REG_ ## reg, val, cond, \ > + delay_us, timeout_us) > +#define snd_hdac_stream_readl_poll(strm, reg, val, cond, delay_us, timeout_us) \ > + readl_poll_timeout((strm)->sd_addr + AZX_REG_ ## reg, val, cond, \ > + delay_us, timeout_us) > > struct hdac_ext_device; > > -- > 2.25.1 >
On 2022-03-09 11:36 PM, Pierre-Louis Bossart wrote:
> I am running out of time and will resume this review next week.
Hello,
I don't believe any of the comments provided are a strong reason for
re-send. Given that last revisions were mostly related to addressing
comments, rewords and explaining stuff, series is in good shape and
ready for merge. As I already stated, team continues to work on the
subject and there are more patch-series to come.
Regards,
Czarek
On Thu, Mar 10, 2022 at 06:11:31PM +0100, Cezary Rojewski wrote: > I don't believe any of the comments provided are a strong reason for > re-send. Given that last revisions were mostly related to addressing > comments, rewords and explaining stuff, series is in good shape and ready > for merge. As I already stated, team continues to work on the subject and > there are more patch-series to come. It'd be good to get the WARN()s fixed.
On 2022-03-11 1:10 PM, Mark Brown wrote: > On Thu, Mar 10, 2022 at 06:11:31PM +0100, Cezary Rojewski wrote: > >> I don't believe any of the comments provided are a strong reason for >> re-send. Given that last revisions were mostly related to addressing >> comments, rewords and explaining stuff, series is in good shape and ready >> for merge. As I already stated, team continues to work on the subject and >> there are more patch-series to come. > > It'd be good to get the WARN()s fixed. Ack. Addressed in v5.
On 2022-03-09 10:58 PM, Pierre-Louis Bossart wrote: >> diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile >> new file mode 100644 >> index 000000000000..5f7976a95fe2 >> --- /dev/null >> +++ b/sound/soc/intel/avs/Makefile >> @@ -0,0 +1,5 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> + >> +snd-soc-avs-objs := dsp.o >> + >> +obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o > > nit-pick: snd-soc-intel-avs? > > avs is not a clear mapping to Intel platforms We already have snd_soc_catpt : ( >> +/* >> + * struct avs_dev - Intel HD-Audio driver data >> + * >> + * @dev: PCI device >> + * @dsp_ba: DSP bar address > > not sure it's only limited to DSP, is it? Well, this space is classified as: "Host Device Memory Space (Audio DSP)". >> + * @spec: platform-specific descriptor >> + */ >> +struct avs_dev { >> + struct hda_bus base; >> + struct device *dev; >> + >> + void __iomem *dsp_ba; >> + const struct avs_spec *spec; >> +}; > >> +int avs_dsp_core_reset(struct avs_dev *adev, u32 core_mask, bool reset) >> +{ >> + u32 value, mask, reg; >> + int ret; >> + >> + mask = AVS_ADSPCS_CRST_MASK(core_mask); >> + value = reset ? mask : 0; >> + >> + snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPCS, mask, value); >> + >> + ret = snd_hdac_adsp_readl_poll(adev, AVS_ADSP_REG_ADSPCS, >> + reg, (reg & mask) == value, >> + AVS_ADSPCS_INTERVAL_US, >> + AVS_ADSPCS_TIMEOUT_US); >> + if (ret) >> + dev_err(adev->dev, "core_mask %d %s reset failed: %d\n", >> + core_mask, reset ? "enter" : "exit", ret); > > nit-pick: reset %s "entry" "exit" I did align with what's already present in sound/hda/hdac_controller.c. It's done the other way around there. My personal opinion is: it sounds more logically the way it is currently - but I'm not a native English speaker. >> + >> + return ret; >> +} >> + >
On 2022-03-09 11:16 PM, Pierre-Louis Bossart wrote: > >> +/* >> + * avs_ipc_delete_instance - Delete module instance >> + * >> + * @adev: Driver context >> + * @module_id: Module-type id >> + * @instance_id: Unique module instance id >> + * >> + * Argument verification, as well as pipeline state checks are done >> by the >> + * firmware. >> + * >> + * Note: only standalone modules i.e. without a parent pipeline shall be >> + * deleted using this IPC message. In all other cases, pipeline >> owning the >> + * modules peforms cleanup automatically when it is deleted. > > typo: performs > > checkpatch.pl --strict --codespell would detect all this for you. Ack.
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
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 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?
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. Regards, Czarek
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 " ?
On 2022-03-11 9:30 PM, Pierre-Louis Bossart wrote: > On 3/11/22 11:20, Cezary Rojewski wrote: ... >> 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 > " The major problem is the convoluted naming within the firmware itself. The decision for the naming all the members as they are is dictated by the fact that there is much, much higher chance for Intel audio software or firmware developer to do some major/meaningful changes to the avs-driver as the kernel grows than a person outside that circle. Not everyone might agree, but that's the democratic decision made by the team in the early days of this driver. And thus, having close name-matching between the driver and the firmware makes it easier for given developer to switch between the two projects. Agree on the rewording. There are probably more of such wordings within the code so this might span more than 1 file. Regards, Czarek