| Message ID | 20220304145755.2844173-1-cezary.rojewski@intel.com |
|---|---|
| Headers | show |
| Series | ASoC: Intel: AVS - Audio DSP for cAVS | expand |
On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: > Implement the IPC between Intel audio firmware and kernel driver. The > IPC allows transmission of requests, handling of responses as well as > unsolicited (i.e. firmware-generated) notifications. > > A subscription mechanism is added to enable different parts of the > driver to register for specific notifications. > > The part of the DSP boot process that involves sending ROM message > requires an extra step - must be followed by unstall operation of > MAIN_CORE. All other types of messages do not require such specific > handling, so separate set of functions is provided for sending these. > > Signed-off-by: Amadeusz Sławiński < > amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/Makefile | 2 +- > sound/soc/intel/avs/avs.h | 100 +++++++++ > sound/soc/intel/avs/ipc.c | 387 > ++++++++++++++++++++++++++++++++ > sound/soc/intel/avs/messages.h | 170 ++++++++++++++ > sound/soc/intel/avs/registers.h | 45 ++++ > 5 files changed, 703 insertions(+), 1 deletion(-) > create mode 100644 sound/soc/intel/avs/ipc.c > create mode 100644 sound/soc/intel/avs/messages.h > > diff --git a/sound/soc/intel/avs/Makefile > b/sound/soc/intel/avs/Makefile > index 5f7976a95fe2..e243806dd38a 100644 > --- a/sound/soc/intel/avs/Makefile > +++ b/sound/soc/intel/avs/Makefile > @@ -1,5 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0-only > > -snd-soc-avs-objs := dsp.o > +snd-soc-avs-objs := dsp.o ipc.o > > obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o > diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h > index d4e6310e4bf7..841b8541b101 100644 > --- a/sound/soc/intel/avs/avs.h > +++ b/sound/soc/intel/avs/avs.h > @@ -11,13 +11,27 @@ > > #include <linux/device.h> > #include <sound/hda_codec.h> > +#include "messages.h" > > struct avs_dev; > > +/* > + * struct avs_dsp_ops - Platform-specific DSP operations > + * > + * @power: Power on or off DSP cores > + * @reset: Enter or exit reset state on DSP cores > + * @stall: Stall or run DSP cores > + * @irq_handler: Top half of IPC servicing > + * @irq_thread: Bottom half of IPC servicing > + * @int_control: Enable or disable IPC interrupts > + */ > struct avs_dsp_ops { > int (* const power)(struct avs_dev *, u32, bool); > int (* const reset)(struct avs_dev *, u32, bool); > int (* const stall)(struct avs_dev *, u32, bool); > + irqreturn_t (* const irq_handler)(int, void *); > + irqreturn_t (* const irq_thread)(int, void *); > + void (* const int_control)(struct avs_dev *, bool); > }; > > #define avs_dsp_op(adev, op, ...) \ > @@ -34,6 +48,9 @@ struct avs_spec { > > const u32 core_init_mask; /* used during DSP boot */ > const u64 attributes; /* bitmask of AVS_PLATATTR_* > */ > + const u32 sram_base_offset; > + const u32 sram_window_size; > + const u32 rom_status; > }; > > /* > @@ -49,6 +66,9 @@ struct avs_dev { > > void __iomem *dsp_ba; > const struct avs_spec *spec; > + struct avs_ipc *ipc; > + > + struct completion fw_ready; > }; > > /* from hda_bus to avs_dev */ > @@ -68,4 +88,84 @@ int avs_dsp_core_stall(struct avs_dev *adev, u32 > core_mask, bool stall); > int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask); > int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask); > > +/* Inter Process Communication */ > + > +struct avs_ipc_msg { > + union { > + u64 header; > + union avs_global_msg glb; > + union avs_reply_msg rsp; > + }; > + void *data; > + size_t size; > +}; > + > +/* > + * struct avs_ipc - DSP IPC context > + * > + * @dev: PCI device > + * @rx: Reply message cache > + * @default_timeout_ms: default message timeout in MS > + * @ready: whether firmware is ready and communication is open > + * @rx_completed: whether RX for previously sent TX has been > received > + * @rx_lock: for serializating manipulation of rx_* fields > + * @msg_lock: for synchronizing request handling > + * @done_completion: DONE-part of IPC i.e. ROM and ACKs from FW > + * @busy_completion: BUSY-part of IPC i.e. receiving responses from > FW > + */ > +struct avs_ipc { > + struct device *dev; > + > + struct avs_ipc_msg rx; > + u32 default_timeout_ms; > + bool ready; > + > + bool rx_completed; > + spinlock_t rx_lock; > + struct mutex msg_mutex; > + struct completion done_completion; > + struct completion busy_completion; > +}; > + > +#define AVS_EIPC EREMOTEIO > +/* > + * IPC handlers may return positive value (firmware error code) what > denotes > + * successful HOST <-> DSP communication yet failure to process > specific request. > + * > + * Below macro converts returned value to linux kernel error code. > + * All IPC callers MUST use it as soon as firmware error code is > consumed. > + */ > +#define AVS_IPC_RET(ret) \ > + (((ret) <= 0) ? (ret) : -AVS_EIPC) > + > +static inline void avs_ipc_err(struct avs_dev *adev, struct > avs_ipc_msg *tx, > + const char *name, int error) > +{ > + /* > + * If IPC channel is blocked e.g.: due to ongoing recovery, Do you mean firmware recovery? In which cases do you perform a recovery? > + * -EPERM error code is expected and thus it's not an actual > error. And what happens in this case? do you retry the IPC after recovery? > + */ > + if (error == -EPERM) > + dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", > name, > + tx->glb.primary, tx->glb.ext.val, error); > + else > + dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", > name, > + tx->glb.primary, tx->glb.ext.val, error); > +} > + > +irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id); > +irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id); > +void avs_dsp_process_response(struct avs_dev *adev, u64 header); > +int avs_dsp_send_msg_timeout(struct avs_dev *adev, > + struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply, int timeout); > +int avs_dsp_send_msg(struct avs_dev *adev, > + struct avs_ipc_msg *request, struct avs_ipc_msg > *reply); > +int avs_dsp_send_rom_msg_timeout(struct avs_dev *adev, > + struct avs_ipc_msg *request, int > timeout); > +int avs_dsp_send_rom_msg(struct avs_dev *adev, struct avs_ipc_msg > *request); > +void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable); > +int avs_ipc_init(struct avs_ipc *ipc, struct device *dev); > +void avs_ipc_block(struct avs_ipc *ipc); > + > #endif /* __SOUND_SOC_INTEL_AVS_H */ > diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c > new file mode 100644 > index 000000000000..c0722f8b195f > --- /dev/null > +++ b/sound/soc/intel/avs/ipc.c > @@ -0,0 +1,387 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright(c) 2021 Intel Corporation. All rights reserved. > +// > +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> > +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > +// > + > +#include <linux/slab.h> > +#include <sound/hdaudio_ext.h> > +#include "avs.h" > +#include "messages.h" > +#include "registers.h" > + > +#define AVS_IPC_TIMEOUT_MS 300 > + > +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) > +{ > + struct avs_ipc *ipc = adev->ipc; > + union avs_reply_msg msg = AVS_MSG(header); > + > + ipc->rx.header = header; > + /* Abort copying payload if request processing was > unsuccessful. */ This seems misplaced? Why would you called this function is the status showed an error? > + if (!msg.status) > + memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), > + ipc->rx.size); > +} > + > +static void avs_dsp_process_notification(struct avs_dev *adev, u64 > header) > +{ > + struct avs_notify_mod_data mod_data; > + union avs_notify_msg msg = AVS_MSG(header); > + size_t data_size = 0; > + void *data = NULL; > + > + /* Ignore spurious notifications until handshake is > established. */ > + if (!adev->ipc->ready && msg.notify_msg_type != > AVS_NOTIFY_FW_READY) { > + dev_dbg(adev->dev, "FW not ready, skip notification: > 0x%08x\n", > + msg.primary); > + return; > + } > + > + /* Calculate notification payload size. */ > + switch (msg.notify_msg_type) { > + case AVS_NOTIFY_FW_READY: > + break; > + > + case AVS_NOTIFY_PHRASE_DETECTED: > + data_size = sizeof(struct avs_notify_voice_data); > + break; > + > + case AVS_NOTIFY_RESOURCE_EVENT: > + data_size = sizeof(struct avs_notify_res_data); > + break; > + > + case AVS_NOTIFY_MODULE_EVENT: > + /* To know the total payload size, header needs to be > read first. */ > + memcpy_fromio(&mod_data, avs_uplink_addr(adev), > sizeof(mod_data)); > + data_size = sizeof(mod_data) + mod_data.data_size; > + break; > + > + default: > + dev_info(adev->dev, "unknown notification: 0x%08x\n", > + msg.primary); info? should it be a warning? > + break; > + } > + > + if (data_size) { > + data = kmalloc(data_size, GFP_KERNEL); > + if (!data) > + return; Should this function be modified to return the error? If it failed here, all subsequent IPC's rec'd will also fail isnt it? > + > + memcpy_fromio(data, avs_uplink_addr(adev), data_size); > + } > + > + /* Perform notification-specific operations. */ > + switch (msg.notify_msg_type) { > + case AVS_NOTIFY_FW_READY: > + dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary); > + adev->ipc->ready = true; > + complete(&adev->fw_ready); > + break; > + > + default: > + break; > + } > + > + kfree(data); You alloc memory for "data", copy the data and free it? Where is it used? > +} > + > +void avs_dsp_process_response(struct avs_dev *adev, u64 header) > +{ > + struct avs_ipc *ipc = adev->ipc; > + > + /* > + * Response may either be solicited - a reply for a request > that has > + * been sent beforehand - or unsolicited (notification). > + */ > + if (avs_msg_is_reply(header)) { > + /* Response processing is invoked from IRQ thread. */ > + spin_lock_irq(&ipc->rx_lock); > + avs_dsp_receive_rx(adev, header); > + ipc->rx_completed = true; > + spin_unlock_irq(&ipc->rx_lock); > + } else { > + avs_dsp_process_notification(adev, header); > + } > + > + complete(&ipc->busy_completion); > +} > + > +irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id) > +{ > + struct avs_dev *adev = dev_id; > + struct avs_ipc *ipc = adev->ipc; > + u32 adspis, hipc_rsp, hipc_ack; > + irqreturn_t ret = IRQ_NONE; > + > + adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS); > + if (adspis == UINT_MAX || !(adspis & AVS_ADSP_ADSPIS_IPC)) > + return ret; > + > + hipc_ack = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCIE); > + hipc_rsp = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCT); > + > + /* DSP acked host's request */ > + if (hipc_ack & SKL_ADSP_HIPCIE_DONE) { > + /* > + * As an extra precaution, mask done interrupt. Code > executed > + * due to complete() found below does not assume any > masking. > + */ > + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCCTL, > + AVS_ADSP_HIPCCTL_DONE, 0); > + > + complete(&ipc->done_completion); > + > + /* tell DSP it has our attention */ > + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCIE, > + SKL_ADSP_HIPCIE_DONE, > + SKL_ADSP_HIPCIE_DONE); > + /* unmask done interrupt */ > + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCCTL, > + AVS_ADSP_HIPCCTL_DONE, > + AVS_ADSP_HIPCCTL_DONE); > + ret = IRQ_HANDLED; > + } > + > + /* DSP sent new response to process */ > + if (hipc_rsp & SKL_ADSP_HIPCT_BUSY) { > + /* mask busy interrupt */ > + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCCTL, > + AVS_ADSP_HIPCCTL_BUSY, 0); > + > + ret = IRQ_WAKE_THREAD; > + } > + > + return ret; > +} > + > +irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id) > +{ > + struct avs_dev *adev = dev_id; > + union avs_reply_msg msg; > + u32 hipct, hipcte; > + > + hipct = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCT); > + hipcte = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCTE); > + > + /* ensure DSP sent new response to process */ > + if (!(hipct & SKL_ADSP_HIPCT_BUSY)) > + return IRQ_NONE; > + > + msg.primary = hipct; > + msg.ext.val = hipcte; > + avs_dsp_process_response(adev, msg.val); > + > + /* tell DSP we accepted its message */ > + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCT, > + SKL_ADSP_HIPCT_BUSY, > SKL_ADSP_HIPCT_BUSY); > + /* unmask busy interrupt */ > + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCCTL, > + AVS_ADSP_HIPCCTL_BUSY, > AVS_ADSP_HIPCCTL_BUSY); > + > + return IRQ_HANDLED; > +} > + > +static bool avs_ipc_is_busy(struct avs_ipc *ipc) > +{ > + struct avs_dev *adev = to_avs_dev(ipc->dev); > + u32 hipc_rsp; > + > + hipc_rsp = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCT); > + return hipc_rsp & SKL_ADSP_HIPCT_BUSY; > +} > + > +static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int > timeout) > +{ > + u32 repeats_left = 128; /* to avoid infinite looping */ > + int ret; > + > +again: > + ret = wait_for_completion_timeout(&ipc->busy_completion, > + msecs_to_jiffies(timeout)); > + > + /* DSP could be unresponsive at this point. */ > + if (!ipc->ready) > + return -EPERM; > + > + if (!ret) { > + if (!avs_ipc_is_busy(ipc)) > + return -ETIMEDOUT; > + /* > + * Firmware did its job, either notification or reply > + * has been received - now wait until it's processed. > + */ > + wait_for_completion_killable(&ipc->busy_completion); > + } > + > + /* Ongoing notification's bottom-half may cause early wakeup */ > + spin_lock(&ipc->rx_lock); > + if (!ipc->rx_completed) { > + if (repeats_left) { > + /* Reply delayed due to notification. */ > + repeats_left--; > + reinit_completion(&ipc->busy_completion); > + spin_unlock(&ipc->rx_lock); > + goto again; > + } > + > + spin_unlock(&ipc->rx_lock); > + return -ETIMEDOUT; > + } > + > + spin_unlock(&ipc->rx_lock); > + return 0; > +} > + > +static void avs_ipc_msg_init(struct avs_ipc *ipc, struct avs_ipc_msg > *reply) > +{ > + lockdep_assert_held(&ipc->rx_lock); > + > + ipc->rx.header = 0; > + ipc->rx.size = reply ? reply->size : 0; > + ipc->rx_completed = false; > + > + reinit_completion(&ipc->done_completion); > + reinit_completion(&ipc->busy_completion); > +} > + > +static void avs_dsp_send_tx(struct avs_dev *adev, struct avs_ipc_msg > *tx) send_tx? send and tx both imply the same isnt it? maybe just use one or the other? > +{ > + tx->header |= SKL_ADSP_HIPCI_BUSY; > + > + if (tx->size) > + memcpy_toio(avs_downlink_addr(adev), tx->data, tx- > >size); > + snd_hdac_adsp_writel(adev, SKL_ADSP_REG_HIPCIE, tx->header >> > 32); > + snd_hdac_adsp_writel(adev, SKL_ADSP_REG_HIPCI, tx->header & > UINT_MAX); > +} > + > +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct > avs_ipc_msg *request, > + struct avs_ipc_msg *reply, int timeout) > +{ > + struct avs_ipc *ipc = adev->ipc; > + int ret; > + > + if (!ipc->ready) > + return -EPERM; > + > + mutex_lock(&ipc->msg_mutex); > + > + spin_lock(&ipc->rx_lock); > + avs_ipc_msg_init(ipc, reply); > + avs_dsp_send_tx(adev, request); > + spin_unlock(&ipc->rx_lock); > + > + ret = avs_ipc_wait_busy_completion(ipc, timeout); > + if (ret) { > + if (ret == -ETIMEDOUT) { > + dev_crit(adev->dev, "communication severed: %d, > rebooting dsp..\n", Where does this reboot happen? Thanks, Ranjani
On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: > Firmware modules implement processing algorithms. Their lifecycle, > similarly to pipelines is being controlled by IPCs: initialization, > deletion and (un)binding. > > Modules can be configured at runtime - runtime parameters. This is > done > with help of LARGE_CONFIG IPCs: getter and setter. > > Signed-off-by: Amadeusz Sławiński < > amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/ipc.c | 8 +- > sound/soc/intel/avs/messages.c | 262 > +++++++++++++++++++++++++++++++++ > sound/soc/intel/avs/messages.h | 53 +++++++ > 3 files changed, 322 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c > index c0722f8b195f..9770368a898e 100644 > --- a/sound/soc/intel/avs/ipc.c > +++ b/sound/soc/intel/avs/ipc.c > @@ -21,9 +21,15 @@ static void avs_dsp_receive_rx(struct avs_dev > *adev, u64 header) > > ipc->rx.header = header; > /* Abort copying payload if request processing was > unsuccessful. */ > - if (!msg.status) > + if (!msg.status) { > + /* update size in case of LARGE_CONFIG_GET */ > + if (msg.msg_target == AVS_MOD_MSG && > + msg.global_msg_type == AVS_MOD_LARGE_CONFIG_GET) > + ipc->rx.size = > msg.ext.large_config.data_off_size; > + > memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), > ipc->rx.size); > + } > } > > static void avs_dsp_process_notification(struct avs_dev *adev, u64 > header) > diff --git a/sound/soc/intel/avs/messages.c > b/sound/soc/intel/avs/messages.c > index de2d50f8c6b4..613c9452226d 100644 > --- a/sound/soc/intel/avs/messages.c > +++ b/sound/soc/intel/avs/messages.c > @@ -6,6 +6,7 @@ > // Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > // > > +#include <linux/slab.h> > #include "avs.h" > #include "messages.h" > > @@ -139,3 +140,264 @@ int avs_ipc_get_pipeline_state(struct avs_dev > *adev, u8 instance_id, > *state = reply.rsp.ext.get_ppl_state.state; > return ret; > } > + > +/* > + * avs_ipc_init_instance - Initialize module instance > + * > + * @adev: Driver context > + * @module_id: Module-type id > + * @instance_id: Unique module instance id > + * @ppl_id: Parent pipeline id > + * @core_id: DSP core to allocate module on > + * @domain: Processing domain (low latency or data processing) > + * @param: Module-type specific configuration > + * @param_size: Size of @param in bytes > + * > + * Argument verification, as well as pipeline state checks are done > by the > + * firmware. > + * > + * Note: @ppl_id and @core_id are independent of each other as > single pipeline > + * can be composed of module instances located on different DSP > cores. > + */ > +int avs_ipc_init_instance(struct avs_dev *adev, u16 module_id, u8 > instance_id, > + u8 ppl_id, u8 core_id, u8 domain, > + 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; > + > + ret = avs_dsp_send_msg(adev, &request, NULL); > + if (ret) > + avs_ipc_err(adev, &request, "init instance", ret); > + > + 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. Can you please provide an example of such stand-alone modules? If they aren't part of any pipeline, how do they get scheduled? > + */ > +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; > +} > + > +/* > + * avs_ipc_bind - Bind two module instances > + * > + * @adev: Driver context > + * @module_id: Source module-type id > + * @instance_id: Source module instance id > + * @dst_module_id: Sink module-type id > + * @dst_instance_id: Sink module instance id > + * @dst_queue: Sink module pin to bind @src_queue with > + * @src_queue: Source module pin to bind @dst_queue with > + */ > +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) > +{ > + 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; > +} > + > +/* > + * avs_ipc_unbind - Unbind two module instances > + * > + * @adev: Driver context > + * @module_id: Source module-type id > + * @instance_id: Source module instance id > + * @dst_module_id: Sink module-type id > + * @dst_instance_id: Sink module instance id > + * @dst_queue: Sink module pin to unbind @src_queue from > + * @src_queue: Source module pin to unbind @dst_queue from > + */ Are there any rules for unbinding? For example if you have 2 modules connected to a mixer? Can you unbind the module belonging to the host pipeline that is getting stopped while the mixer is still active? > +int avs_ipc_unbind(struct avs_dev *adev, u16 module_id, u8 > instance_id, > + u16 dst_module_id, u8 dst_instance_id, > + u8 dst_queue, u8 src_queue) > +{ > + union avs_module_msg msg = AVS_MODULE_REQUEST(UNBIND); > + 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, "unbind modules", ret); > + > + return ret; > +} > + > +static int __avs_ipc_set_large_config(struct avs_dev *adev, u16 > module_id, u8 instance_id, > + u8 param_id, bool init_block, > bool final_block, > + u8 *request_data, size_t > request_size, size_t off_size) > +{ > + union avs_module_msg msg = > AVS_MODULE_REQUEST(LARGE_CONFIG_SET); > + struct avs_ipc_msg request; > + int ret; > + > + msg.module_id = module_id; > + msg.instance_id = instance_id; > + msg.ext.large_config.data_off_size = off_size; > + msg.ext.large_config.large_param_id = param_id; > + msg.ext.large_config.final_block = final_block; > + msg.ext.large_config.init_block = init_block; > + > + request.header = msg.val; > + request.data = request_data; > + request.size = request_size; > + > + ret = avs_dsp_send_msg(adev, &request, NULL); > + if (ret) > + avs_ipc_err(adev, &request, "large config set", ret); > + > + return ret; > +} > + > +int avs_ipc_set_large_config(struct avs_dev *adev, u16 module_id, > + u8 instance_id, u8 param_id, > + u8 *request, size_t request_size) > +{ > + size_t remaining, tx_size; > + bool final; > + int ret; > + > + remaining = request_size; > + tx_size = min_t(size_t, AVS_MAILBOX_SIZE, remaining); > + final = (tx_size == remaining); > + > + /* Initial request states total payload size. */ > + ret = __avs_ipc_set_large_config(adev, module_id, instance_id, > + param_id, 1, final, request, > tx_size, > + request_size); > + if (ret) > + return ret; > + > + remaining -= tx_size; > + > + /* Loop the rest only when payload exceeds mailbox's size. */ > + while (remaining) { > + size_t offset; > + > + offset = request_size - remaining; > + tx_size = min_t(size_t, AVS_MAILBOX_SIZE, remaining); > + final = (tx_size == remaining); > + > + ret = __avs_ipc_set_large_config(adev, module_id, > instance_id, > + param_id, 0, final, > + request + offset, > tx_size, > + offset); > + if (ret) > + return ret; > + > + remaining -= tx_size; > + } > + > + return 0; > +} > + > +int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 > instance_id, > + u8 param_id, u8 *request_data, size_t > request_size, > + u8 **reply_data, size_t *reply_size) > +{ > + union avs_module_msg msg = > AVS_MODULE_REQUEST(LARGE_CONFIG_GET); > + struct avs_ipc_msg request; > + struct avs_ipc_msg reply = {{0}}; > + size_t size; > + void *buf; > + int ret; > + > + reply.data = kzalloc(AVS_MAILBOX_SIZE, GFP_KERNEL); > + if (!reply.data) > + return -ENOMEM; > + > + msg.module_id = module_id; > + msg.instance_id = instance_id; > + msg.ext.large_config.data_off_size = request_size; > + msg.ext.large_config.large_param_id = param_id; > + /* final_block is always 0 on request. Updated by fw on reply. > */ > + msg.ext.large_config.final_block = 0; > + msg.ext.large_config.init_block = 1; > + > + request.header = msg.val; > + request.data = request_data; > + request.size = request_size; > + reply.size = AVS_MAILBOX_SIZE; > + > + ret = avs_dsp_send_msg(adev, &request, &reply); > + if (ret) { > + avs_ipc_err(adev, &request, "large config get", ret); > + kfree(reply.data); > + return ret; > + } How come you dont have a loop here? What if the rec'd data size if larger than the max size of IP payload? Thanks,Ranjani
On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: > Wrap elementary DSP-core operations and resource control into more > complex handlers. This is done to reduce the number of invocations of > wrapped operations throughout the driver as order of operations > matters - > most flows involve register manipulation and IPCs combined. > > Signed-off-by: Amadeusz Sławiński < > amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/avs.h | 10 +++ > sound/soc/intel/avs/dsp.c | 170 > ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 180 insertions(+) > > diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h > index 02d7591d0eac..0034c075fa64 100644 > --- a/sound/soc/intel/avs/avs.h > +++ b/sound/soc/intel/avs/avs.h > @@ -89,6 +89,7 @@ struct avs_dev { > struct mutex modres_mutex; > struct ida ppl_ida; > struct list_head fw_list; > + int *core_refs; Is this a per core ref_count? a comment or explicitly calling it core_ref_count would help. Thanks,Ranjani
On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: > Compared to SKL and KBL, younger cAVS platforms are meant to re-use > one Younger? you mean newer? > of HDAudio streams during boot procedure causing CLDMA to become > obsolete. Once transferred, given stream is returned to pool > available > for audio streaming. > > Module loading handler is dummy as library and module code became > inseparable in later firmware generations. replace dummy with "stub" maybe? lets use inclusive terms > > Signed-off-by: Amadeusz Sławiński < > amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/Kconfig | 1 + > sound/soc/intel/avs/avs.h | 5 + > sound/soc/intel/avs/loader.c | 211 > +++++++++++++++++++++++++++++++++++ > 3 files changed, 217 insertions(+) > > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig > index e9768c4aa1a9..d025ca0c77fa 100644 > --- a/sound/soc/intel/Kconfig > +++ b/sound/soc/intel/Kconfig > @@ -215,6 +215,7 @@ config SND_SOC_INTEL_AVS > depends on COMMON_CLK > select SND_SOC_ACPI > select SND_HDA_EXT_CORE > + select SND_HDA_DSP_LOADER > help > Enable support for Intel(R) cAVS 1.5 platforms with DSP > capabilities. This includes Skylake, Kabylake, Amberlake and > diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h > index fb3520f32488..6dc5d17ccf21 100644 > --- a/sound/soc/intel/avs/avs.h > +++ b/sound/soc/intel/avs/avs.h > @@ -45,6 +45,7 @@ struct avs_dsp_ops { > ((adev)->spec->dsp_ops->op(adev, ## __VA_ARGS__)) > > #define AVS_PLATATTR_CLDMA BIT_ULL(0) > +#define AVS_PLATATTR_IMR BIT_ULL(1) > > #define avs_platattr_test(adev, attr) \ > ((adev)->spec->attributes & AVS_PLATATTR_##attr) > @@ -239,5 +240,9 @@ int avs_cldma_load_basefw(struct avs_dev *adev, > struct firmware *fw); > int avs_cldma_load_library(struct avs_dev *adev, struct firmware > *lib, u32 id); > int avs_cldma_transfer_modules(struct avs_dev *adev, bool load, > struct avs_module_entry *mods, u32 > num_mods); > +int avs_hda_load_basefw(struct avs_dev *adev, struct firmware *fw); > +int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib, > u32 id); > +int avs_hda_transfer_modules(struct avs_dev *adev, bool load, > + struct avs_module_entry *mods, u32 > num_mods); > > #endif /* __SOUND_SOC_INTEL_AVS_H */ > diff --git a/sound/soc/intel/avs/loader.c > b/sound/soc/intel/avs/loader.c > index 2134aaabe2c6..6520f23fc70e 100644 > --- a/sound/soc/intel/avs/loader.c > +++ b/sound/soc/intel/avs/loader.c > @@ -9,6 +9,7 @@ > #include <linux/firmware.h> > #include <linux/module.h> > #include <linux/slab.h> > +#include <sound/hdaudio.h> > #include <sound/hdaudio_ext.h> > #include "avs.h" > #include "cldma.h" > @@ -18,8 +19,11 @@ > #define AVS_ROM_STS_MASK 0xFF > #define AVS_ROM_INIT_DONE 0x1 > #define SKL_ROM_BASEFW_ENTERED 0xF > +#define APL_ROM_FW_ENTERED 0x5 > #define AVS_ROM_INIT_POLLING_US 5 > #define SKL_ROM_INIT_TIMEOUT_US 1000000 > +#define APL_ROM_INIT_TIMEOUT_US 300000 > +#define APL_ROM_INIT_RETRIES 3 > > #define AVS_FW_INIT_POLLING_US 500 > #define AVS_FW_INIT_TIMEOUT_US 3000000 > @@ -261,6 +265,204 @@ int avs_cldma_transfer_modules(struct avs_dev > *adev, bool load, > return 0; > } > > +static int > +avs_hda_init_rom(struct avs_dev *adev, unsigned int dma_id, bool > purge) > +{ > + const struct avs_spec *const spec = adev->spec; > + unsigned int corex_mask, reg; > + int ret; > + > + corex_mask = spec->core_init_mask & ~AVS_MAIN_CORE_MASK; > + > + ret = avs_dsp_op(adev, power, spec->core_init_mask, true); > + if (ret < 0) > + goto err; > + > + ret = avs_dsp_op(adev, reset, AVS_MAIN_CORE_MASK, false); > + if (ret < 0) > + goto err; > + > + reinit_completion(&adev->fw_ready); > + avs_dsp_op(adev, int_control, true); > + > + /* set boot config */ > + ret = avs_ipc_set_boot_config(adev, dma_id, purge); > + if (ret) { > + ret = AVS_IPC_RET(ret); > + goto err; > + } > + > + /* await ROM init */ > + ret = snd_hdac_adsp_readq_poll(adev, spec->rom_status, reg, > + (reg & 0xF) == AVS_ROM_INIT_DONE > || > + (reg & 0xF) == > APL_ROM_FW_ENTERED, > + AVS_ROM_INIT_POLLING_US, > APL_ROM_INIT_TIMEOUT_US); > + if (ret < 0) { > + dev_err(adev->dev, "rom init timeout: %d\n", ret); > + goto err; > + } > + > + /* power down non-main cores */ > + if (corex_mask) > + avs_dsp_op(adev, power, corex_mask, false); What if this fails? > + > + return 0; > + > +err: > + avs_dsp_core_disable(adev, spec->core_init_mask); > + return ret; > +} > + > +static int avs_imr_load_basefw(struct avs_dev *adev) > +{ > + int ret; > + > + /* DMA id ignored when flashing from IMR as no transfer occurs. > */ > + ret = avs_hda_init_rom(adev, 0, false); > + if (ret < 0) { > + dev_err(adev->dev, "rom init failed: %d\n", ret); > + return ret; > + } > + > + ret = wait_for_completion_timeout(&adev->fw_ready, > + msecs_to_jiffies(AVS_FW_INIT_TIMEOUT_MS > )); > + if (!ret) { > + dev_err(adev->dev, "firmware ready timeout\n"); > + avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK); > + return -ETIMEDOUT; > + } > + > + return 0; > +} > + > +int avs_hda_load_basefw(struct avs_dev *adev, struct firmware *fw) > +{ > + struct snd_pcm_substream substream; > + struct snd_dma_buffer dmab; > + struct hdac_ext_stream *estream; > + struct hdac_stream *hstream; > + struct hdac_bus *bus = &adev->base.core; > + unsigned int sdfmt, reg; > + int ret, i; > + > + /* configure hda dma */ > + memset(&substream, 0, sizeof(substream)); > + substream.stream = SNDRV_PCM_STREAM_PLAYBACK; > + estream = snd_hdac_ext_stream_assign(bus, &substream, > + HDAC_EXT_STREAM_TYPE_HOST) > ; > + if (!estream) > + return -ENODEV; > + hstream = hdac_stream(estream); > + > + /* code loading performed with default format */ > + sdfmt = snd_hdac_calc_stream_format(48000, 1, > SNDRV_PCM_FORMAT_S32_LE, 32, 0); > + ret = snd_hdac_dsp_prepare(hstream, sdfmt, fw->size, &dmab); > + if (ret < 0) > + goto release_stream; > + > + /* enable SPIB for hda stream */ > + snd_hdac_ext_stream_spbcap_enable(bus, true, hstream->index); > + ret = snd_hdac_ext_stream_set_spib(bus, estream, fw->size); > + if (ret) > + goto cleanup_resources; > + > + memcpy(dmab.area, fw->data, fw->size); > + > + for (i = 0; i < APL_ROM_INIT_RETRIES; i++) { > + unsigned int dma_id = hstream->stream_tag - 1; > + > + ret = avs_hda_init_rom(adev, dma_id, true); > + if (!ret) > + break; > + dev_info(adev->dev, "#%d rom init fail: %d\n", i + 1, > ret); > + } > + if (ret < 0) > + goto cleanup_resources; > + > + /* transfer firmware */ > + snd_hdac_dsp_trigger(hstream, true); > + ret = snd_hdac_adsp_readl_poll(adev, AVS_FW_REG_STATUS(adev), > reg, > + (reg & AVS_ROM_STS_MASK) == > APL_ROM_FW_ENTERED, > + AVS_FW_INIT_POLLING_US, > AVS_FW_INIT_TIMEOUT_US); > + snd_hdac_dsp_trigger(hstream, false); > + if (ret < 0) { > + dev_err(adev->dev, "transfer fw failed: %d\n", ret); > + avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK); > + } > + > +cleanup_resources: > + /* disable SPIB for hda stream */ > + snd_hdac_ext_stream_spbcap_enable(bus, false, hstream->index); > + snd_hdac_ext_stream_set_spib(bus, estream, 0); > + > + snd_hdac_dsp_cleanup(hstream, &dmab); > +release_stream: > + snd_hdac_ext_stream_release(estream, > HDAC_EXT_STREAM_TYPE_HOST); > + > + return ret; > +} > + > +int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib, > u32 id) > +{ > + struct snd_pcm_substream substream; > + struct snd_dma_buffer dmab; > + struct hdac_ext_stream *estream; > + struct hdac_stream *stream; > + struct hdac_bus *bus = &adev->base.core; > + unsigned int sdfmt; > + int ret; > + > + /* configure hda dma */ > + memset(&substream, 0, sizeof(substream)); > + substream.stream = SNDRV_PCM_STREAM_PLAYBACK; > + estream = snd_hdac_ext_stream_assign(bus, &substream, > + HDAC_EXT_STREAM_TYPE_HOST) > ; > + if (!estream) > + return -ENODEV; > + stream = hdac_stream(estream); > + > + /* code loading performed with default format */ > + sdfmt = snd_hdac_calc_stream_format(48000, 1, > SNDRV_PCM_FORMAT_S32_LE, 32, 0); > + ret = snd_hdac_dsp_prepare(stream, sdfmt, lib->size, &dmab); > + if (ret < 0) > + goto release_stream; > + > + /* enable SPIB for hda stream */ > + snd_hdac_ext_stream_spbcap_enable(bus, true, stream->index); > + snd_hdac_ext_stream_set_spib(bus, estream, lib->size); > + > + memcpy(dmab.area, lib->data, lib->size); > + > + /* transfer firmware */ > + snd_hdac_dsp_trigger(stream, true); > + ret = avs_ipc_load_library(adev, stream->stream_tag - 1, id); > + snd_hdac_dsp_trigger(stream, false); > + if (ret) { > + dev_err(adev->dev, "transfer lib %d failed: %d\n", id, > ret); > + ret = AVS_IPC_RET(ret); > + } > + > + /* disable SPIB for hda stream */ > + snd_hdac_ext_stream_spbcap_enable(bus, false, stream->index); > + snd_hdac_ext_stream_set_spib(bus, estream, 0); > + > + snd_hdac_dsp_cleanup(stream, &dmab); > +release_stream: > + snd_hdac_ext_stream_release(estream, > HDAC_EXT_STREAM_TYPE_HOST); > + > + return ret; > +} > + > +int avs_hda_transfer_modules(struct avs_dev *adev, bool load, > + struct avs_module_entry *mods, u32 > num_mods) What is the difference between transfer_modules and load_library? Thanks,Ranjani
On 2022-03-04 5:21 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski 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. > Can you please provide an example of such stand-alone modules? If they > aren't part of any pipeline, how do they get scheduled? Thanks for feedback! Consider dropping the unnecessary bits so it is easier to navigate through your responses. Please note: kernel mailing list is not for explaining SW <-> FW communication details. Feel free to contact my colleagues from firmware team if in need of any FW-iface details. That goes for most of the comments found below too. >> +/* >> + * avs_ipc_unbind - Unbind two module instances >> + * >> + * @adev: Driver context >> + * @module_id: Source module-type id >> + * @instance_id: Source module instance id >> + * @dst_module_id: Sink module-type id >> + * @dst_instance_id: Sink module instance id >> + * @dst_queue: Sink module pin to unbind @src_queue from >> + * @src_queue: Source module pin to unbind @dst_queue from >> + */ > Are there any rules for unbinding? For example if you have 2 modules > connected to a mixer? Can you unbind the module belonging to the host > pipeline that is getting stopped while the mixer is still active? Here we have just a delegate. All the rules are defined and enforced by the firmware. >> +int avs_ipc_unbind(struct avs_dev *adev, u16 module_id, u8 >> instance_id, >> + u16 dst_module_id, u8 dst_instance_id, >> + u8 dst_queue, u8 src_queue) >> +{ >> + union avs_module_msg msg = AVS_MODULE_REQUEST(UNBIND); >> + 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, "unbind modules", ret); >> + >> + return ret; >> +} ... >> +int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 >> instance_id, >> + u8 param_id, u8 *request_data, size_t >> request_size, >> + u8 **reply_data, size_t *reply_size) >> +{ >> + union avs_module_msg msg = >> AVS_MODULE_REQUEST(LARGE_CONFIG_GET); >> + struct avs_ipc_msg request; >> + struct avs_ipc_msg reply = {{0}}; >> + size_t size; >> + void *buf; >> + int ret; >> + >> + reply.data = kzalloc(AVS_MAILBOX_SIZE, GFP_KERNEL); >> + if (!reply.data) >> + return -ENOMEM; >> + >> + msg.module_id = module_id; >> + msg.instance_id = instance_id; >> + msg.ext.large_config.data_off_size = request_size; >> + msg.ext.large_config.large_param_id = param_id; >> + /* final_block is always 0 on request. Updated by fw on reply. >> */ >> + msg.ext.large_config.final_block = 0; >> + msg.ext.large_config.init_block = 1; >> + >> + request.header = msg.val; >> + request.data = request_data; >> + request.size = request_size; >> + reply.size = AVS_MAILBOX_SIZE; >> + >> + ret = avs_dsp_send_msg(adev, &request, &reply); >> + if (ret) { >> + avs_ipc_err(adev, &request, "large config get", ret); >> + kfree(reply.data); >> + return ret; >> + } > How come you dont have a loop here? What if the rec'd data size if > larger than the max size of IP payload? That's not how LARGE_CONFIG_GET message works. There is no looping involved or expected by the firmware and so we don't have it here. Regards, Czarek
On 2022-03-04 5:26 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: >> ROM requests are messages initiated by Host to alter firmware early >> boot >> process. They specify whether the next boot should be a fresh start >> or if >> IMR can be used to speed things up. >> >> Signed-off-by: Amadeusz Sławiński < >> amadeuszx.slawinski@linux.intel.com> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> sound/soc/intel/avs/messages.c | 18 ++++++++++++++++++ >> sound/soc/intel/avs/messages.h | 14 ++++++++++++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/sound/soc/intel/avs/messages.c >> b/sound/soc/intel/avs/messages.c >> index e8f441b28d71..f7d00e541323 100644 >> --- a/sound/soc/intel/avs/messages.c >> +++ b/sound/soc/intel/avs/messages.c >> @@ -12,6 +12,24 @@ >> >> #define AVS_CL_TIMEOUT_MS 5000 >> >> +int avs_ipc_set_boot_config(struct avs_dev *adev, u32 dma_id, u32 >> purge) > Does purge set to true indicate a fresh boot and a false indicate IMR > restore? A description would help. Indeed, purge indicates a fresh boot. I believe flow is explained well by the layout of the calling functions, local variable naming and step-ordering which are found later in the series. Regards, Czarek
On 2022-03-04 5:41 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski 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 >> + * @mod_idas: Module instance ID pool, one per module-type >> + * @modres_mutex: For synchronizing any @mods_info updates > Is this mutex really necessary? Can you please elaborate under what > circumstances your will have parallel module updates? Yes, we believe modres_mutex is necessary. All information regarding modules exposed by the firmware are stored within ->mods_info cache. That's just a snapshot though. When a new library gets loaded, new modules may be available for use and so the driver updates the ->mods_info cache to have the latest snapshot. As information found there is used when streaming (e.g.: instantiating modules), we enter a scenario when multiple threads could be reading/updating the ->mods_info at once. To prevent any unwanted behavior, mutex has been added. >> +void avs_module_info_free(struct avs_dev *adev) >> +{ >> + mutex_lock(&adev->modres_mutex); >> + >> + avs_module_ida_destroy(adev); >> + kfree(adev->mods_info); >> + adev->mods_info = NULL; >> + >> + mutex_unlock(&adev->modres_mutex); >> +} >> + >> +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) { > Can you please help me understand when this can happen? If all modules > required by the topology are already initialized, will this ever > happen? I want to help! Just not understanding the meaning of: "all modules required by the topology are already initialized". Will answer best I can though: topology carries just patterns, it may happen that module found within topology file is not actually exposed by the firmware. In such case, we drop an error. This keeps recovery scenarios sane too - when recovering, libraries may have to be re-loaded, depending on the firmware generation and whether basefw recovery was successful or not. Regards, Czarek
On 2022-03-04 5:47 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: >> Wrap elementary DSP-core operations and resource control into more >> complex handlers. This is done to reduce the number of invocations of >> wrapped operations throughout the driver as order of operations >> matters - >> most flows involve register manipulation and IPCs combined. >> >> Signed-off-by: Amadeusz Sławiński < >> amadeuszx.slawinski@linux.intel.com> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> sound/soc/intel/avs/avs.h | 10 +++ >> sound/soc/intel/avs/dsp.c | 170 >> ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 180 insertions(+) >> >> diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h >> index 02d7591d0eac..0034c075fa64 100644 >> --- a/sound/soc/intel/avs/avs.h >> +++ b/sound/soc/intel/avs/avs.h >> @@ -89,6 +89,7 @@ struct avs_dev { >> struct mutex modres_mutex; >> struct ida ppl_ida; >> struct list_head fw_list; >> + int *core_refs; > > Is this a per core ref_count? a comment or explicitly calling it > core_ref_count would help. Hmm.. That's bit of a nitpick. Isn't a 's' enough here? I.e.: here we have an array of core references. Array, as the number of elements is retrieved dynamically via LARGE_CONFIG_GET and only then the array gets allocated. Regards, Czarek
On 2022-03-04 5:54 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote: ... >> +static int avs_fw_manifest_strip_verify(struct avs_dev *adev, struct >> firmware *fw, >> + const struct avs_fw_version >> *min) >> +{ >> + struct avs_fw_manifest *man; >> + int offset, ret; >> + >> + ret = avs_fw_ext_manifest_strip(fw); >> + if (ret) >> + return ret; >> + >> + offset = avs_fw_manifest_offset(fw); >> + if (offset < 0) >> + return offset; >> + >> + if (fw->size < offset + sizeof(*man)) >> + return -EINVAL; >> + if (!min) >> + return 0; >> + >> + man = (struct avs_fw_manifest *)(fw->data + offset); >> + if (man->version.major != min->major || >> + man->version.minor != min->minor || >> + man->version.hotfix != min->hotfix || >> + man->version.build < min->build) { > Isnt this check a bit too strict? Isnt a check major enough? Unfortunately not. I share the similar thinking but the build system has its history and several things which should not happen, had happened. There could be _large_ API changes without any meaningful version updates at all. To prevent any unwanted behavior, this check is as strict as it can get. >> + dev_warn(adev->dev, "bad FW version %d.%d.%d.%d, >> expected %d.%d.%d.%d or newer\n", >> + man->version.major, man->version.minor, >> + man->version.hotfix, man->version.build, >> + min->major, min->minor, min->hotfix, min- >>> build); >> + >> + if (!debug_ignore_fw_version) >> + return -EINVAL; >> + } >> + >> + return 0; >> +} ... >> +int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge) >> +{ >> + int ret, i; >> + >> + /* Full boot, clear cached data except for basefw (slot 0). */ > Does this mean IMR restore is only available for base FW and not for > module libraries? Do I understand this correctly? Loop below just clears the data. The new snapshot will be received once the basefw and libraries get loaded. The execution of library loading is not part of this patch anymore as it is dependent on the avs-soc-component stuff. To make things easier to review, request was to split main series into chucks. I do believe it is easier to read and review indeed. >> + for (i = 1; i < adev->fw_cfg.max_libs_count; i++) >> + memset(adev->lib_names[i], 0, AVS_LIB_NAME_SIZE); >> + >> + avs_hda_clock_gating_enable(adev, false); >> + avs_hda_l1sen_enable(adev, false); >> + >> + ret = avs_dsp_load_basefw(adev); >> + >> + avs_hda_l1sen_enable(adev, true); >> + avs_hda_clock_gating_enable(adev, true); >> + >> + if (ret < 0) >> + return ret; >> + >> + /* With all code loaded, refresh module information. */ >> + ret = avs_module_info_init(adev, true); > It is not clear if this required only after first boot or after a > suspend/resume as well. avs_dsp_boot_firmware() and avs_dsp_first_boot_firmware() (found just below this one) are two separate functions. Only the latter has things done once. Anything else can happen several times throughout the lifetime of the avs-driver. Regards, Czarek
>>> Compared to SKL and KBL, younger cAVS platforms are meant to re-use >>> one >> Younger? you mean newer? > > > Isn't the meaning of the two quite similar in this context? younger doesn't sound quite right to me. 'cAVS platforms more recent that SKL and KBL...' > >>> of HDAudio streams during boot procedure causing CLDMA to become >>> obsolete. Once transferred, given stream is returned to pool >>> available >>> for audio streaming. >>> >>> Module loading handler is dummy as library and module code became >>> inseparable in later firmware generations. >> replace dummy with "stub" maybe? lets use inclusive terms > > > Is 'dummy' categorized as non-inclusive? We have several usages of > 'dummy' even within /sound (e.g. soc-utils.c). Intel and plenty of other companies recommend a different wording. mock-up, stub, placeholder, etc. see e.g. https://developers.google.com/style/inclusive-documentation Yes we have plenty of existing uses of 'dummy', but that doesn't mean we should add new ones. >>> +static int >>> +avs_hda_init_rom(struct avs_dev *adev, unsigned int dma_id, bool >>> purge) >>> +{ >>> + const struct avs_spec *const spec = adev->spec; >>> + unsigned int corex_mask, reg; >>> + int ret; >>> + >>> + corex_mask = spec->core_init_mask & ~AVS_MAIN_CORE_MASK; >>> + >>> + ret = avs_dsp_op(adev, power, spec->core_init_mask, true); >>> + if (ret < 0) >>> + goto err; >>> + >>> + ret = avs_dsp_op(adev, reset, AVS_MAIN_CORE_MASK, false); >>> + if (ret < 0) >>> + goto err; >>> + >>> + reinit_completion(&adev->fw_ready); >>> + avs_dsp_op(adev, int_control, true); >>> + >>> + /* set boot config */ >>> + ret = avs_ipc_set_boot_config(adev, dma_id, purge); >>> + if (ret) { >>> + ret = AVS_IPC_RET(ret); >>> + goto err; >>> + } >>> + >>> + /* await ROM init */ >>> + ret = snd_hdac_adsp_readq_poll(adev, spec->rom_status, reg, >>> + (reg & 0xF) == AVS_ROM_INIT_DONE >>> || >>> + (reg & 0xF) == >>> APL_ROM_FW_ENTERED, >>> + AVS_ROM_INIT_POLLING_US, >>> APL_ROM_INIT_TIMEOUT_US); >>> + if (ret < 0) { >>> + dev_err(adev->dev, "rom init timeout: %d\n", ret); >>> + goto err; >>> + } >>> + >>> + /* power down non-main cores */ >>> + if (corex_mask) >>> + avs_dsp_op(adev, power, corex_mask, false); >> What if this fails? > > > We are still in happy path, worst thing could happen here is increased > power consumption. 'happy path'?
On 2022-03-04 7:56 PM, Pierre-Louis Bossart wrote: > >>>> Compared to SKL and KBL, younger cAVS platforms are meant to re-use >>>> one >>> Younger? you mean newer? >> >> >> Isn't the meaning of the two quite similar in this context? > > younger doesn't sound quite right to me. > > 'cAVS platforms more recent that SKL and KBL...' Sure, can reword. >>>> Module loading handler is dummy as library and module code became >>>> inseparable in later firmware generations. >>> replace dummy with "stub" maybe? lets use inclusive terms >> >> >> Is 'dummy' categorized as non-inclusive? We have several usages of >> 'dummy' even within /sound (e.g. soc-utils.c). > > Intel and plenty of other companies recommend a different wording. > mock-up, stub, placeholder, etc. see e.g. > > https://developers.google.com/style/inclusive-documentation > > Yes we have plenty of existing uses of 'dummy', but that doesn't mean we > should add new ones. No problem with rewording this one in v4. >>>> +static int >>>> +avs_hda_init_rom(struct avs_dev *adev, unsigned int dma_id, bool >>>> purge) >>>> +{ >>>> + const struct avs_spec *const spec = adev->spec; >>>> + unsigned int corex_mask, reg; >>>> + int ret; >>>> + >>>> + corex_mask = spec->core_init_mask & ~AVS_MAIN_CORE_MASK; >>>> + >>>> + ret = avs_dsp_op(adev, power, spec->core_init_mask, true); >>>> + if (ret < 0) >>>> + goto err; >>>> + >>>> + ret = avs_dsp_op(adev, reset, AVS_MAIN_CORE_MASK, false); >>>> + if (ret < 0) >>>> + goto err; >>>> + >>>> + reinit_completion(&adev->fw_ready); >>>> + avs_dsp_op(adev, int_control, true); >>>> + >>>> + /* set boot config */ >>>> + ret = avs_ipc_set_boot_config(adev, dma_id, purge); >>>> + if (ret) { >>>> + ret = AVS_IPC_RET(ret); >>>> + goto err; >>>> + } >>>> + >>>> + /* await ROM init */ >>>> + ret = snd_hdac_adsp_readq_poll(adev, spec->rom_status, reg, >>>> + (reg & 0xF) == AVS_ROM_INIT_DONE >>>> || >>>> + (reg & 0xF) == >>>> APL_ROM_FW_ENTERED, >>>> + AVS_ROM_INIT_POLLING_US, >>>> APL_ROM_INIT_TIMEOUT_US); >>>> + if (ret < 0) { >>>> + dev_err(adev->dev, "rom init timeout: %d\n", ret); >>>> + goto err; >>>> + } >>>> + >>>> + /* power down non-main cores */ >>>> + if (corex_mask) >>>> + avs_dsp_op(adev, power, corex_mask, false); >>> What if this fails? >> >> >> We are still in happy path, worst thing could happen here is increased >> power consumption. > > 'happy path'? The question is whether we value user experience over error-checking this case. I've never seen this step failing and the step itself is quite.. extraordinary : ) It's also invoked on very limited number of cAVS platforms, for everything else it's just: 'return 0'. Regards, Czarek
On Fri, 2022-03-04 at 18:11 +0100, Cezary Rojewski wrote: > > > + * -EPERM error code is expected and thus it's not an actual > > > error. > > And what happens in this case? do you retry the IPC after recovery? > > > > > Not at all. Why would you want retry IPC after recovery in the first > place? So what happens after FW recovery? Will this lead to an xrun and cause usersapce to restart the stream? Thanks,Ranjani
On 2022-03-07 5:15 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 18:11 +0100, Cezary Rojewski wrote: >>>> + * -EPERM error code is expected and thus it's not an actual >>>> error. >>> And what happens in this case? do you retry the IPC after recovery? >> >> >> >> >> Not at all. Why would you want retry IPC after recovery in the first >> place? > So what happens after FW recovery? Will this lead to an xrun and cause > usersapce to restart the stream? If DSP dies so does the stream. Recovery is not guaranteed, it's an attempt. What userspace does with that is for userspace to decide. Regards, Czarek
On Fri, 2022-03-04 at 18:21 +0100, Cezary Rojewski wrote: > > Are there any rules for unbinding? For example if you have 2 > > modules > > connected to a mixer? Can you unbind the module belonging to the > > host > > pipeline that is getting stopped while the mixer is still active? > > > > > Here we have just a delegate. All the rules are defined and enforced > by > > the firmware. I'm not following this, Czarek. If there are rules defined by the FW, the driver has to follow it isnt it? What I am asking is how and where do you enforce this in the AVS driver? ... > > > > > How come you dont have a loop here? What if the rec'd data size if > > larger than the max size of IP payload? > > > > > That's not how LARGE_CONFIG_GET message works. There is no looping > > involved or expected by the firmware and so we don't have it here. So, are you saying that when retrieving data from the FW, the size of the retrieved data can never exceed max IPC payload size? Thanks,Ranjani
> > > + * @modres_mutex: For synchronizing any @mods_info updates > > Is this mutex really necessary? Can you please elaborate under what > > circumstances your will have parallel module updates? > > > > > Yes, we believe modres_mutex is necessary. All information regarding > > modules exposed by the firmware are stored within ->mods_info cache. > > > > That's just a snapshot though. When a new library gets loaded, new > > modules may be available for use and so the driver updates the > > ->mods_info cache to have the latest snapshot. As information found > > there is used when streaming (e.g.: instantiating modules), we enter > a > > scenario when multiple threads could be reading/updating the > ->mods_info > > at once. To prevent any unwanted behavior, mutex has been added. This is the part that's hard to follow without seeing the actual code where this new library is loaded. When does a libray get loaded? When you start streaming and you realize that the stream requires a module that is not built into the base FW? Can this be done during topology loading instead? Thanks, Rnajani
On 2022-03-07 5:39 PM, Ranjani Sridharan wrote: > On Fri, 2022-03-04 at 18:21 +0100, Cezary Rojewski wrote: >>> Are there any rules for unbinding? For example if you have 2 >>> modules >>> connected to a mixer? Can you unbind the module belonging to the >>> host >>> pipeline that is getting stopped while the mixer is still active? >> >> >> >> >> Here we have just a delegate. All the rules are defined and enforced >> by >> >> the firmware. > I'm not following this, Czarek. If there are rules defined by the FW, > the driver has to follow it isnt it? What I am asking is how and where > do you enforce this in the AVS driver? How the stream looks is defined by the topology. Code that translates such patterns into runtime being is not part of this patchset. It's part of avs_path and its collaterals instead. >>>> >>> How come you dont have a loop here? What if the rec'd data size if >>> larger than the max size of IP payload? >> >> >> >> >> That's not how LARGE_CONFIG_GET message works. There is no looping >> >> involved or expected by the firmware and so we don't have it here. > > So, are you saying that when retrieving data from the FW, the size of > the retrieved data can never exceed max IPC payload size? Precisely. Regards, Czarek
On Mon, 2022-03-07 at 17:58 +0100, Cezary Rojewski wrote: > > I'm not following this, Czarek. If there are rules defined by the > > FW, > > the driver has to follow it isnt it? What I am asking is how and > > where > > do you enforce this in the AVS driver? > > > > > How the stream looks is defined by the topology. Code that > translates > > such patterns into runtime being is not part of this patchset. It's > part > > of avs_path and its collaterals instead. Alright, I'll wait for the next patchset for the explanation. > > > > > > > How come you dont have a loop here? What if the rec'd data size > > > > if > > > > larger than the max size of IP payload? > > > That's not how LARGE_CONFIG_GET message works. There is no > > > looping > > > involved or expected by the firmware and so we don't have it > > > here. > > So, are you saying that when retrieving data from the FW, the size > > of > > the retrieved data can never exceed max IPC payload size? > > > > > Precisely. This is fundmentally flawed isnt it? If set_large_config() sets a config that can exceed max IPC size, get_large_config() has to be able to support it. Thanks, Ranjani
On 2022-03-07 5:46 PM, Ranjani Sridharan wrote: >>>> + * @modres_mutex: For synchronizing any @mods_info updates >>> Is this mutex really necessary? Can you please elaborate under what >>> circumstances your will have parallel module updates? >> >> >> >> >> Yes, we believe modres_mutex is necessary. All information regarding >> >> modules exposed by the firmware are stored within ->mods_info cache. >> >> >> >> That's just a snapshot though. When a new library gets loaded, new >> >> modules may be available for use and so the driver updates the >> >> ->mods_info cache to have the latest snapshot. As information found >> >> there is used when streaming (e.g.: instantiating modules), we enter >> a >> >> scenario when multiple threads could be reading/updating the >> ->mods_info >> >> at once. To prevent any unwanted behavior, mutex has been added. > This is the part that's hard to follow without seeing the actual code > where this new library is loaded. When does a libray get loaded? When > you start streaming and you realize that the stream requires a module > that is not built into the base FW? Can this be done during topology > loading instead? But that's already done during topology load! If there is no topology telling the driver: "hey, load this lib for me!", nothing gets loaded regardless of how your /lib/firmware looks like. Libraries get loaded during soc-component's (platform component) ->probe(). This is place where snd_soc_tplg_component_load() is called. However, if platform has no IMR capability, driver has to re-load libraries for all platform components of bound sound cards on every pm_runtime_resume(). Regards, Czarek
On 2022-03-07 6:05 PM, Ranjani Sridharan wrote: > On Mon, 2022-03-07 at 17:58 +0100, Cezary Rojewski wrote: >>> I'm not following this, Czarek. If there are rules defined by the >>> FW, >>> the driver has to follow it isnt it? What I am asking is how and >>> where >>> do you enforce this in the AVS driver? >> >> >> >> >> How the stream looks is defined by the topology. Code that >> translates >> >> such patterns into runtime being is not part of this patchset. It's >> part >> >> of avs_path and its collaterals instead. > Alright, I'll wait for the next patchset for the explanation. > >> >> >> >>>>> How come you dont have a loop here? What if the rec'd data size >>>>> if >>>>> larger than the max size of IP payload? >>>> That's not how LARGE_CONFIG_GET message works. There is no >>>> looping >>>> involved or expected by the firmware and so we don't have it >>>> here. >>> So, are you saying that when retrieving data from the FW, the size >>> of >>> the retrieved data can never exceed max IPC payload size? >> >> >> >> >> Precisely. > This is fundmentally flawed isnt it? If set_large_config() sets a > config that can exceed max IPC size, get_large_config() has to be able > to support it. I could ask people on the list to "not look for a second" then there would be no problem explaining all the *recommended flows*. Simple, honest answer is: Yes, that's fundamentally flawed. Now, as older firmware generations do not expect nor support larger payload sizes, adding such code here is essentially adding dead code so we have decided to add none of it. Regards, Czarek
On Mon, 2022-03-07 at 18:13 +0100, Cezary Rojewski wrote: > > This is the part that's hard to follow without seeing the actual > > code > > where this new library is loaded. When does a libray get loaded? > > When > > you start streaming and you realize that the stream requires a > > module > > that is not built into the base FW? Can this be done during > > topology > > loading instead? > > > But that's already done during topology load! If there is no > topology > > telling the driver: "hey, load this lib for me!", nothing gets > loaded > > regardless of how your /lib/firmware looks like. Libraries get > loaded > > during soc-component's (platform component) ->probe(). This is place > > where snd_soc_tplg_component_load() is called. > > > > However, if platform has no IMR capability, driver has to re-load > > libraries for all platform components of bound sound cards on every > > pm_runtime_resume(). And if it done during pm_runtime_resume(), where would the problem with synchronizing arise from? userspace apps do not get resumed until after the PCI device is resumed isnt it? Thanks, Ranjani
>>>>>> How come you dont have a loop here? What if the rec'd data size >>>>>> if >>>>>> larger than the max size of IP payload? >>>>> That's not how LARGE_CONFIG_GET message works. There is no >>>>> looping >>>>> involved or expected by the firmware and so we don't have it >>>>> here. >>>> So, are you saying that when retrieving data from the FW, the size >>>> of >>>> the retrieved data can never exceed max IPC payload size? >>> >>> >>> >>> >>> Precisely. >> This is fundmentally flawed isnt it? If set_large_config() sets a >> config that can exceed max IPC size, get_large_config() has to be able >> to support it. > > I could ask people on the list to "not look for a second" then there > would be no problem explaining all the *recommended flows*. > > Simple, honest answer is: Yes, that's fundamentally flawed. > Now, as older firmware generations do not expect nor support larger > payload sizes, adding such code here is essentially adding dead code so > we have decided to add none of it. Adding a comment and/or an explanation in the commit message wouldn't hurt then.
On 2022-03-07 6:30 PM, Ranjani Sridharan wrote: > On Mon, 2022-03-07 at 18:13 +0100, Cezary Rojewski wrote: >>> This is the part that's hard to follow without seeing the actual >>> code >>> where this new library is loaded. When does a libray get loaded? >>> When >>> you start streaming and you realize that the stream requires a >>> module >>> that is not built into the base FW? Can this be done during >>> topology >>> loading instead? >> >> >> But that's already done during topology load! If there is no >> topology >> >> telling the driver: "hey, load this lib for me!", nothing gets >> loaded >> >> regardless of how your /lib/firmware looks like. Libraries get >> loaded >> >> during soc-component's (platform component) ->probe(). This is place >> >> where snd_soc_tplg_component_load() is called. >> >> >> >> However, if platform has no IMR capability, driver has to re-load >> >> libraries for all platform components of bound sound cards on every >> >> pm_runtime_resume(). > > And if it done during pm_runtime_resume(), where would the problem with > synchronizing arise from? userspace apps do not get resumed until after > the PCI device is resumed isnt it? Scenario you describe is correct and does not prompt the need for the mutex. However, ->mods_info is accessed through getters found in utils.c (this very patch) during stream creation too. That fragment is part of path management series though - it was requested to split those bits away. So, there is a possibility for a platform-soc-component to have its ->probe() called - and thus triggering ->mods_info update - while a stream is being opened on a different sound card simultaneously. To avoid unwanted behavior e.g.: looping through ->mods_info while it's being updated in separate thread, we lock the array. Regards, Czarek
On Tue, 2022-03-08 at 17:57 +0100, Cezary Rojewski wrote: > Scenario you describe is correct and does not prompt the need for the > mutex. > > > > However, ->mods_info is accessed through getters found in utils.c > (this > > very patch) during stream creation too. That fragment is part of > path > > management series though - it was requested to split those bits away. > > > > So, there is a possibility for a platform-soc-component to have its > > ->probe() called - and thus triggering ->mods_info update - while a > > stream is being opened on a different sound card simultaneously. To > > avoid unwanted behavior e.g.: looping through ->mods_info while it's > > being updated in separate thread, we lock the array. Keeping in mind that this driver is meant for older platforms, how likely are you to support multiple sound cards with those topologies? Thanks, Ranjani
On 2022-03-08 6:22 PM, Ranjani Sridharan wrote: > On Tue, 2022-03-08 at 17:57 +0100, Cezary Rojewski wrote: >> Scenario you describe is correct and does not prompt the need for the >> mutex. >> >> >> >> However, ->mods_info is accessed through getters found in utils.c >> (this >> >> very patch) during stream creation too. That fragment is part of >> path >> >> management series though - it was requested to split those bits away. >> >> >> >> So, there is a possibility for a platform-soc-component to have its >> >> ->probe() called - and thus triggering ->mods_info update - while a >> >> stream is being opened on a different sound card simultaneously. To >> >> avoid unwanted behavior e.g.: looping through ->mods_info while it's >> >> being updated in separate thread, we lock the array. > > Keeping in mind that this driver is meant for older platforms, how > likely are you to support multiple sound cards with those topologies? Not sure what's the question here. Age of the platform has nothing to do with the subject. There is not a single DSP-capable platform that Intel has shipped that would not contain more one audio device onboard. At least I'm not aware of any. Regards, Czarek
On Tue, 2022-03-08 at 19:07 +0100, Cezary Rojewski wrote: > > Keeping in mind that this driver is meant for older platforms, how > > likely are you to support multiple sound cards with those > > topologies? > > > Not sure what's the question here. Age of the platform has nothing to > do > > with the subject. There is not a single DSP-capable platform that > Intel > > has shipped that would not contain more one audio device onboard. At > > least I'm not aware of any. My question was related to your comment about multiple sound cards. What I asked was do you plan to support multiple topologies with modules spread across then with multiple sound component drivers with the AVS driver and firmware? Does this mean you will need multiple topology files and machine driver? And what is the rationale for this? If not, there's no need for the mutex. Thanks, Ranjani
On 2022-03-08 7:26 PM, Ranjani Sridharan wrote: > On Tue, 2022-03-08 at 19:07 +0100, Cezary Rojewski wrote: >>> Keeping in mind that this driver is meant for older platforms, how >>> likely are you to support multiple sound cards with those >>> topologies? >> >> >> Not sure what's the question here. Age of the platform has nothing to >> do >> >> with the subject. There is not a single DSP-capable platform that >> Intel >> >> has shipped that would not contain more one audio device onboard. At >> >> least I'm not aware of any. > > My question was related to your comment about multiple sound cards. > What I asked was do you plan to support multiple topologies with > modules spread across then with multiple sound component drivers with > the AVS driver and firmware? Does this mean you will need multiple > topology files and machine driver? And what is the rationale for this? > > If not, there's no need for the mutex. Yes, avs-driver embraces granular sound card approach as opposed to super card approach. There is one topology file per sound card. That subject is not part of this patchset though. Regards, Czarek
On 3/8/22 12:31, Cezary Rojewski wrote: > On 2022-03-08 7:26 PM, Ranjani Sridharan wrote: >> On Tue, 2022-03-08 at 19:07 +0100, Cezary Rojewski wrote: >>>> Keeping in mind that this driver is meant for older platforms, how >>>> likely are you to support multiple sound cards with those >>>> topologies? >>> >>> >>> Not sure what's the question here. Age of the platform has nothing to >>> do >>> >>> with the subject. There is not a single DSP-capable platform that >>> Intel >>> >>> has shipped that would not contain more one audio device onboard. At >>> >>> least I'm not aware of any. >> >> My question was related to your comment about multiple sound cards. >> What I asked was do you plan to support multiple topologies with >> modules spread across then with multiple sound component drivers with >> the AVS driver and firmware? Does this mean you will need multiple >> topology files and machine driver? And what is the rationale for this? >> >> If not, there's no need for the mutex. > > Yes, avs-driver embraces granular sound card approach as opposed to > super card approach. There is one topology file per sound card. > > That subject is not part of this patchset though. You would still need to clarify how the same set of modules might be accessed or configured when different cards are created, or if there is a 'clean' split with a 1:1 mapping between module and cards. The more I think of this the less practical this 'granular' approach looks, e.g. if you want to route the same stream to different interfaces handled by different cards. An example is playing a notification on local speakers controlled by HDaudio and a Bluetooth headset using I2S. This could be really fun to represent even basic volume control to users: controls are card-specific and some parts may be handled in different cards and thus different UCM files. I really think the best split of a DSP topology is between orthogonal parts. When muxers/demux are used, or multi-input modules such as AEC, the routing complexity outweighs the benefits of a simpler card design.
On 2022-03-08 8:42 PM, Pierre-Louis Bossart wrote: > On 3/8/22 12:31, Cezary Rojewski wrote: ... >> Yes, avs-driver embraces granular sound card approach as opposed to >> super card approach. There is one topology file per sound card. >> >> That subject is not part of this patchset though. > > You would still need to clarify how the same set of modules might be > accessed or configured when different cards are created, or if there is > a 'clean' split with a 1:1 mapping between module and cards. > > The more I think of this the less practical this 'granular' approach > looks, e.g. if you want to route the same stream to different interfaces > handled by different cards. An example is playing a notification on > local speakers controlled by HDaudio and a Bluetooth headset using I2S. > This could be really fun to represent even basic volume control to > users: controls are card-specific and some parts may be handled in > different cards and thus different UCM files. > > I really think the best split of a DSP topology is between orthogonal > parts. When muxers/demux are used, or multi-input modules such as AEC, > the routing complexity outweighs the benefits of a simpler card design. There is a clean split - for all the typical scenarios, topology tied to given sound card defines the stream patterns completely. Even when considering "functional" split as you mention in third paragraph, multiple sound cards are still there. So, as I have described earlier, to prevent any unwanted behaviour, cached module information in form of ->mods_info, is being locked with help of a mutex. We do not duplicate such information per sound card as that would be a waste of memory. No matter how many libraries are mentioned by the topology files and loaded throughout the lifetime of a driver, there is always just one base firmware we sent MODULE_INFO to. In regard to advanced scenarios and more complex routing, it would be good to have that conversation as a part of dedicated patchset which is going to follow this one (sooner or later). Regards, Czarek