Message ID | 20240829194105.1504814-1-quic_wcheng@quicinc.com |
---|---|
Headers | show |
Series | Introduce QC USB SND audio offloading support | expand |
On 8/29/24 21:40, Wesley Cheng wrote: > Allow for checks on a specific USB audio device to see if a requested PCM > format is supported. This is needed for support when playback is > initiated by the ASoC USB backend path. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
On 8/29/24 21:40, Wesley Cheng wrote: > USB SND needs to know how the USB offload path is being routed. This would > allow for applications to open the corresponding sound card and pcm device > when it wants to take the audio offload path. This callback should return > the mapped indexes based on the USB SND device information. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h > index 5c788cbfa82e..86876098a2b7 100644 > --- a/include/sound/soc-usb.h > +++ b/include/sound/soc-usb.h > @@ -36,6 +36,7 @@ struct snd_soc_usb_device { > * @list - list head for SND SOC struct list > * @component - reference to ASoC component > * @connection_status_cb - callback to notify connection events > + * @update_offload_route_info - callback to fetch mapped ASoC device nit-pick: explain that 'route' refers to the (card, device) pair and isn't related to the concept of DAPM route. > * @priv_data - driver data > **/ > struct snd_soc_usb { > @@ -44,6 +45,9 @@ struct snd_soc_usb { > int (*connection_status_cb)(struct snd_soc_usb *usb, > struct snd_soc_usb_device *sdev, > bool connected); > + int (*update_offload_route_info)(struct snd_soc_component *component, > + int card, int pcm, int direction, > + long *route); nit-pick: also explain the format used for the (card, device) pair stored in the long > void *priv_data; > }; > > @@ -59,6 +63,8 @@ void *snd_soc_usb_find_priv_data(struct device *dev); > int snd_soc_usb_setup_offload_jack(struct snd_soc_component *component, > struct snd_soc_jack *jack); > int snd_soc_usb_disable_offload_jack(struct snd_soc_component *component); > +int snd_soc_usb_update_offload_route(struct device *dev, int card, int pcm, > + int direction, long *route); > > struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, > void *data); > @@ -101,6 +107,12 @@ static inline int snd_soc_usb_disable_offload_jack(struct snd_soc_component *com > return 0; > } > > +static int snd_soc_usb_update_offload_route(struct device *dev, int card, int pcm, > + int direction, long *route) > +{ > + return -ENODEV; > +} > + > static inline struct snd_soc_usb * > snd_soc_usb_allocate_port(struct snd_soc_component *component, void *data) > { > diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c > index 3d5354298206..de249e8a28cb 100644 > --- a/sound/soc/soc-usb.c > +++ b/sound/soc/soc-usb.c > @@ -117,6 +117,37 @@ int snd_soc_usb_disable_offload_jack(struct snd_soc_component *component) > } > EXPORT_SYMBOL_GPL(snd_soc_usb_disable_offload_jack); > > +/** > + * snd_soc_usb_update_offload_route - Find active USB offload path > + * @dev - USB device to get offload status > + * @card - USB card index > + * @pcm - USB PCM device index > + * @direction - playback or capture direction > + * @route - pointer to route output array > + * > + * Fetch the current status for the USB SND card and PCM device indexes > + * specified. > + */ > +int snd_soc_usb_update_offload_route(struct device *dev, int card, int pcm, > + int direction, long *route) > +{ > + struct snd_soc_usb *ctx; > + int ret; > + > + ctx = snd_soc_find_usb_ctx(dev); > + if (!ctx) > + return -ENODEV; > + > + mutex_lock(&ctx_mutex); > + if (ctx && ctx->update_offload_route_info) > + ret = ctx->update_offload_route_info(ctx->component, card, pcm, > + direction, route); > + mutex_unlock(&ctx_mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_update_offload_route); > + > /** > * snd_soc_usb_find_priv_data() - Retrieve private data stored > * @dev: device reference
On 8/29/24 21:40, Wesley Cheng wrote: > The QC ADSP is able to support USB playback endpoints, so that the main > application processor can be placed into lower CPU power modes. This adds > the required AFE port configurations and port start command to start an > audio session. > > Specifically, the QC ADSP can support all potential endpoints that are > exposed by the audio data interface. This includes, feedback endpoints > (both implicit and explicit) as well as the isochronous (data) endpoints. I think you meant " this includes isochronous data endpoints, in either synchronous mode or asynchronous mode. In the latter case both implicit or explicit feedback endpoints are supported. " And now I don't remember how *controls* are handled. Is this the case that all controls exposed by endpoint zero are visible in both the regular USB-audio card AND the offloaded card, with changes mirrored? It's important to explain so that the volumes are consistent no matter which path is used. This should be added to the documentation. > +static const struct snd_soc_dai_ops q6afe_usb_ops = { > + .probe = msm_dai_q6_dai_probe, > + .prepare = q6afe_dai_prepare, > + .hw_params = q6afe_usb_hw_params, > + /* > + * Shutdown callback required to stop the USB AFE port, which is enabled > + * by the prepare() stage. This stops the audio traffic on the USB AFE > + * port on the Q6DSP. > + */ > + .shutdown = q6afe_dai_shutdown, > + /* > + * Startup callback not needed, as AFE port start command passes the PCM > + * parameters within the AFE command, which is provided by the PCM core > + * during the prepare() stage. > + */ Humm, now this is a bit confusing. Why would you need a shutdown, can't you use the hw_free() callback for symmetry with prepare()?
On 8/29/24 21:40, Wesley Cheng wrote: > Create a USB BE component that will register a new USB port to the ASoC USB > framework. This will handle determination on if the requested audio > profile is supported by the USB device currently selected. > > Check for if the PCM format is supported during the hw_params callback. If > the profile is not supported then the userspace ALSA entity will receive an > error, and can take further action. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > include/sound/q6usboffload.h | 20 +++ > sound/soc/qcom/Kconfig | 10 ++ > sound/soc/qcom/qdsp6/Makefile | 1 + > sound/soc/qcom/qdsp6/q6usb.c | 246 ++++++++++++++++++++++++++++++++++ > 4 files changed, 277 insertions(+) > create mode 100644 include/sound/q6usboffload.h > create mode 100644 sound/soc/qcom/qdsp6/q6usb.c > > diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h > new file mode 100644 > index 000000000000..49ab2c34b84c > --- /dev/null > +++ b/include/sound/q6usboffload.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * linux/sound/q6usboffload.h -- QDSP6 USB offload not sure about the linux/ prefix? > + * > + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +/** > + * struct q6usb_offload > + * @dev - dev handle to usb be > + * @sid - streamID for iommu > + * @intr_num - usb interrupter number > + * @domain - allocated iommu domain > + **/ > +struct q6usb_offload { > + struct device *dev; > + long long sid; > + u16 intr_num; > + struct iommu_domain *domain; > +}; consider reordering to avoid holes/alignment issues, e.g. all pointers first, then long long then u16 > +static int q6usb_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + struct q6usb_port_data *data = dev_get_drvdata(dai->dev); > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); > + int direction = substream->stream; > + struct q6afe_port *q6usb_afe; > + struct snd_soc_usb_device *sdev; > + int ret = -EINVAL; > + > + mutex_lock(&data->mutex); > + > + /* No active chip index */ > + if (list_empty(&data->devices)) > + goto out; -ENODEV for the default return value>? > + > + sdev = list_last_entry(&data->devices, struct snd_soc_usb_device, list); > + > + ret = snd_soc_usb_find_supported_format(sdev->chip_idx, params, direction); > + if (ret < 0) > + goto out; > + > + q6usb_afe = q6afe_port_get_from_id(cpu_dai->dev, USB_RX); > + if (IS_ERR(q6usb_afe)) > + goto out; > + > + /* Notify audio DSP about the devices being offloaded */ > + ret = afe_port_send_usb_dev_param(q6usb_afe, sdev->card_idx, > + sdev->ppcm_idx[sdev->num_playback - 1]); don't you need a symmetrical notification upon hw_free()? Also what happens if there are multiple calls to hw_params, which is quite legit in ALSA/ASoC?
On 8/29/24 21:40, Wesley Cheng wrote: > The USB SND path may need to know how the USB offload path is routed, so > that applications can open the proper sound card and PCM device. The > implementation for the QC ASoC design has a "USB Mixer" kcontrol for each > possible FE (Q6ASM) DAI, which can be utilized to know which front end link > is enabled. > > When an application/userspace queries for the mapped offload devices, the > logic will lookup the USB mixer status though the following path: > > MultiMedia* <-> MM_DL* <-> USB Mixer* > > The "USB Mixer" is a DAPM widget, and the q6routing entity will set the > DAPM connect status accordingly if the USB mixer is enabled. If enabled, > the Q6USB backend link can fetch the PCM device number from the FE DAI > link (Multimedia*). With respects to the card number, that is > straightforward, as the ASoC components have direct references to the ASoC > platform sound card. > > An example output can be shown below: > > Number of controls: 9 > name value > Capture Channel Map 0, 0 (range 0->36) > Playback Channel Map 0, 0 (range 0->36) > Headset Capture Switch On > Headset Capture Volume 1 (range 0->4) > Sidetone Playback Switch On > Sidetone Playback Volume 4096 (range 0->8192) > Headset Playback Switch On > Headset Playback Volume 20, 20 (range 0->24) > USB Offload Playback Route PCM#0 0, 1 (range -1->255) > > The "USB Offload Playback Route PCM#*" kcontrol will signify the > corresponding card and pcm device it is offload to. (card#0 pcm - device#1) > If the USB SND device supports multiple audio interfaces, then it will > contain several PCM streams, hence in those situations, it is expected > that there will be multiple playback route kcontrols created. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/soc/qcom/qdsp6/q6usb.c | 104 +++++++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > > diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c > index 10337d70eb27..c2fc0dedf430 100644 > --- a/sound/soc/qcom/qdsp6/q6usb.c > +++ b/sound/soc/qcom/qdsp6/q6usb.c > @@ -132,6 +132,109 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone > return ret; > } > > +static int q6usb_get_pcm_id_from_widget(struct snd_soc_dapm_widget *w) > +{ > + struct snd_soc_pcm_runtime *rtd; > + struct snd_soc_dai *dai; > + > + for_each_card_rtds(w->dapm->card, rtd) { > + dai = snd_soc_rtd_to_cpu(rtd, 0); > + /* > + * Only look for playback widget. RTD number carries the assigned > + * PCM index. > + */ > + if (dai->stream[0].widget == w) > + return rtd->num; > + } > + > + return -1; > +} > + > +static int q6usb_usb_mixer_enabled(struct snd_soc_dapm_widget *w) > +{ > + struct snd_soc_dapm_path *p; > + > + /* Checks to ensure USB path is enabled/connected */ > + snd_soc_dapm_widget_for_each_sink_path(w, p) > + if (!strcmp(p->sink->name, "USB Mixer") && p->connect) > + return 1; > + > + return 0; > +} > + > +static int q6usb_get_pcm_id(struct snd_soc_component *component) > +{ > + struct snd_soc_dapm_widget *w; > + struct snd_soc_dapm_path *p; > + int pidx; > + > + /* > + * Traverse widgets to find corresponding FE widget. The DAI links are > + * built like the following: > + * MultiMedia* <-> MM_DL* <-> USB Mixer* > + */ > + for_each_card_widgets(component->card, w) { > + if (!strncmp(w->name, "MultiMedia", 10)) { > + /* > + * Look up all paths associated with the FE widget to see if > + * the USB BE is enabled. The sink widget is responsible to > + * link with the USB mixers. > + */ > + snd_soc_dapm_widget_for_each_sink_path(w, p) { > + if (q6usb_usb_mixer_enabled(p->sink)) { > + pidx = q6usb_get_pcm_id_from_widget(w); > + return pidx; > + } > + } Humm, there should be a note that the design assumes that the USB offload path exposes a single PCM per endpoints - same as the non-offloaded path. If the ASoC card has multiple PCMs for each endpoint, possibly with different processing on each PCM, then the mapping would fail. The other question is whether you need to walk in the DAPM graph, in theory DPCM has helpers to find which FEs are connected to which BE.
On 8/29/24 21:41, Wesley Cheng wrote: > Utilize the card and PCM index coming from the USB QMI stream request. > This field follows what is set by the ASoC USB backend, and could > potentially carry information about a specific device selected through the > ASoC USB backend. The backend also has information about the last USB > sound device plugged in, so it can choose to select the last device plugged > in, accordingly. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/usb/qcom/qc_audio_offload.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c > index 0bd533f539e4..a7ad15404fd1 100644 > --- a/sound/usb/qcom/qc_audio_offload.c > +++ b/sound/usb/qcom/qc_audio_offload.c > @@ -106,8 +106,6 @@ struct uaudio_qmi_dev { > bool er_mapped; > /* reference count to number of possible consumers */ > atomic_t qdev_in_use; > - /* idx to last udev card number plugged in */ > - unsigned int last_card_num; > }; > > struct uaudio_dev { > @@ -1261,7 +1259,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs, > > pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8; > xfer_buf_len = req_msg->xfer_buff_size; > - card_num = uaudio_qdev->last_card_num; > + card_num = (req_msg->usb_token & QMI_STREAM_REQ_CARD_NUM_MASK) >> 16; > > if (!uadev[card_num].ctrl_intf) { > dev_err(&subs->dev->dev, "audio ctrl intf info not cached\n"); > @@ -1455,8 +1453,7 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle, > > direction = (req_msg->usb_token & QMI_STREAM_REQ_DIRECTION); > pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8; > - pcm_card_num = req_msg->enable ? uaudio_qdev->last_card_num : > - ffs(uaudio_qdev->card_slot) - 1; > + pcm_card_num = (req_msg->usb_token & QMI_STREAM_REQ_CARD_NUM_MASK) >> 16; > if (pcm_card_num >= SNDRV_CARDS) { > ret = -EINVAL; > goto response; > @@ -1706,7 +1703,6 @@ static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip) > sdev->card_idx = chip->card->number; > sdev->chip_idx = chip->index; > > - uaudio_qdev->last_card_num = chip->card->number; > snd_soc_usb_connect(usb_get_usb_backend(udev), sdev); > } This entire path seems like a bad split/merge, it removes stuff that was done earlier. Also it's not clear what this has to do with 'QMI', card and PCM device management is usually done at a higher level. not following, please be mindful of reviewer fatigue when adding such changes in patch 30/33....
On 8/30/2024 2:12 AM, Pierre-Louis Bossart wrote: > > On 8/29/24 21:40, Wesley Cheng wrote: >> The QC ADSP is able to support USB playback endpoints, so that the main >> application processor can be placed into lower CPU power modes. This adds >> the required AFE port configurations and port start command to start an >> audio session. >> >> Specifically, the QC ADSP can support all potential endpoints that are >> exposed by the audio data interface. This includes, feedback endpoints >> (both implicit and explicit) as well as the isochronous (data) endpoints. > I think you meant > > " > this includes isochronous data endpoints, in either synchronous mode or > asynchronous mode. In the latter case both implicit or explicit feedback > endpoints are supported. > " > > And now I don't remember how *controls* are handled. > > Is this the case that all controls exposed by endpoint zero are visible > in both the regular USB-audio card AND the offloaded card, with changes > mirrored? I think we had a discussion about this previously, but can't seem to find it. The USB volume controls (ie controls still handled by the control EP) are going to still reside only within the USB SND card. As of now, there aren't any kcontrols that are being added to the ASoC platform card for offload. > It's important to explain so that the volumes are consistent no matter > which path is used. This should be added to the documentation. > > >> +static const struct snd_soc_dai_ops q6afe_usb_ops = { >> + .probe = msm_dai_q6_dai_probe, >> + .prepare = q6afe_dai_prepare, >> + .hw_params = q6afe_usb_hw_params, >> + /* >> + * Shutdown callback required to stop the USB AFE port, which is enabled >> + * by the prepare() stage. This stops the audio traffic on the USB AFE >> + * port on the Q6DSP. >> + */ >> + .shutdown = q6afe_dai_shutdown, >> + /* >> + * Startup callback not needed, as AFE port start command passes the PCM >> + * parameters within the AFE command, which is provided by the PCM core >> + * during the prepare() stage. >> + */ > Humm, now this is a bit confusing. Why would you need a shutdown, can't > you use the hw_free() callback for symmetry with prepare()? I thought that it was hw_params() -- hw_free() and prepare() -- shutdown()? "DPCM runs the PCM trigger(stop), hw_free(), shutdown() operations on DAI0 for headset since the path is now disabled. DPCM runs the PCM ops for startup(), hw_params(), prepare() and trigger(start) for DAI1 Speakers since the path is enabled." https://www.kernel.org/doc/html/v6.10/sound/soc/dpcm.html Thanks Wesley Cheng >
Hi Pierre, On 8/30/2024 2:21 AM, Pierre-Louis Bossart wrote: > > On 8/29/24 21:40, Wesley Cheng wrote: >> Create a USB BE component that will register a new USB port to the ASoC USB >> framework. This will handle determination on if the requested audio >> profile is supported by the USB device currently selected. >> >> Check for if the PCM format is supported during the hw_params callback. If >> the profile is not supported then the userspace ALSA entity will receive an >> error, and can take further action. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> include/sound/q6usboffload.h | 20 +++ >> sound/soc/qcom/Kconfig | 10 ++ >> sound/soc/qcom/qdsp6/Makefile | 1 + >> sound/soc/qcom/qdsp6/q6usb.c | 246 ++++++++++++++++++++++++++++++++++ >> 4 files changed, 277 insertions(+) >> create mode 100644 include/sound/q6usboffload.h >> create mode 100644 sound/soc/qcom/qdsp6/q6usb.c >> >> diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h >> new file mode 100644 >> index 000000000000..49ab2c34b84c >> --- /dev/null >> +++ b/include/sound/q6usboffload.h >> @@ -0,0 +1,20 @@ >> +/* SPDX-License-Identifier: GPL-2.0 >> + * >> + * linux/sound/q6usboffload.h -- QDSP6 USB offload > not sure about the linux/ prefix? > >> + * >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +/** >> + * struct q6usb_offload >> + * @dev - dev handle to usb be >> + * @sid - streamID for iommu >> + * @intr_num - usb interrupter number >> + * @domain - allocated iommu domain >> + **/ >> +struct q6usb_offload { >> + struct device *dev; >> + long long sid; >> + u16 intr_num; >> + struct iommu_domain *domain; >> +}; > consider reordering to avoid holes/alignment issues, e.g. all pointers > first, then long long then u16 > Will fix these. >> +static int q6usb_hw_params(struct snd_pcm_substream *substream, >> + struct snd_pcm_hw_params *params, >> + struct snd_soc_dai *dai) >> +{ >> + struct q6usb_port_data *data = dev_get_drvdata(dai->dev); >> + struct snd_soc_pcm_runtime *rtd = substream->private_data; >> + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); >> + int direction = substream->stream; >> + struct q6afe_port *q6usb_afe; >> + struct snd_soc_usb_device *sdev; >> + int ret = -EINVAL; >> + >> + mutex_lock(&data->mutex); >> + >> + /* No active chip index */ >> + if (list_empty(&data->devices)) >> + goto out; > -ENODEV for the default return value>? Sure. >> + >> + sdev = list_last_entry(&data->devices, struct snd_soc_usb_device, list); >> + >> + ret = snd_soc_usb_find_supported_format(sdev->chip_idx, params, direction); >> + if (ret < 0) >> + goto out; >> + >> + q6usb_afe = q6afe_port_get_from_id(cpu_dai->dev, USB_RX); >> + if (IS_ERR(q6usb_afe)) >> + goto out; >> + >> + /* Notify audio DSP about the devices being offloaded */ >> + ret = afe_port_send_usb_dev_param(q6usb_afe, sdev->card_idx, >> + sdev->ppcm_idx[sdev->num_playback - 1]); > don't you need a symmetrical notification upon hw_free()? > > Also what happens if there are multiple calls to hw_params, which is > quite legit in ALSA/ASoC? The afe_port_send_usb_dev_param() is meant to just update the device selected for offload on the audio DSP end, and this won't be referenced until our Q6AFE DAI sends the port start command in its prepare() callback. Don't think we need to handle anything specific in the hw_free() case. As long as the hw_params() callback is called before any audio session is started, then we'll ensure that the device selected is always updated to the audio DSP. Thanks Wesley Cheng
Hi Pierre, On 8/30/2024 2:34 AM, Pierre-Louis Bossart wrote: > > On 8/29/24 21:40, Wesley Cheng wrote: >> The USB SND path may need to know how the USB offload path is routed, so >> that applications can open the proper sound card and PCM device. The >> implementation for the QC ASoC design has a "USB Mixer" kcontrol for each >> possible FE (Q6ASM) DAI, which can be utilized to know which front end link >> is enabled. >> >> When an application/userspace queries for the mapped offload devices, the >> logic will lookup the USB mixer status though the following path: >> >> MultiMedia* <-> MM_DL* <-> USB Mixer* >> >> The "USB Mixer" is a DAPM widget, and the q6routing entity will set the >> DAPM connect status accordingly if the USB mixer is enabled. If enabled, >> the Q6USB backend link can fetch the PCM device number from the FE DAI >> link (Multimedia*). With respects to the card number, that is >> straightforward, as the ASoC components have direct references to the ASoC >> platform sound card. >> >> An example output can be shown below: >> >> Number of controls: 9 >> name value >> Capture Channel Map 0, 0 (range 0->36) >> Playback Channel Map 0, 0 (range 0->36) >> Headset Capture Switch On >> Headset Capture Volume 1 (range 0->4) >> Sidetone Playback Switch On >> Sidetone Playback Volume 4096 (range 0->8192) >> Headset Playback Switch On >> Headset Playback Volume 20, 20 (range 0->24) >> USB Offload Playback Route PCM#0 0, 1 (range -1->255) >> >> The "USB Offload Playback Route PCM#*" kcontrol will signify the >> corresponding card and pcm device it is offload to. (card#0 pcm - device#1) >> If the USB SND device supports multiple audio interfaces, then it will >> contain several PCM streams, hence in those situations, it is expected >> that there will be multiple playback route kcontrols created. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> sound/soc/qcom/qdsp6/q6usb.c | 104 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 104 insertions(+) >> >> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c >> index 10337d70eb27..c2fc0dedf430 100644 >> --- a/sound/soc/qcom/qdsp6/q6usb.c >> +++ b/sound/soc/qcom/qdsp6/q6usb.c >> @@ -132,6 +132,109 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone >> return ret; >> } >> >> +static int q6usb_get_pcm_id_from_widget(struct snd_soc_dapm_widget *w) >> +{ >> + struct snd_soc_pcm_runtime *rtd; >> + struct snd_soc_dai *dai; >> + >> + for_each_card_rtds(w->dapm->card, rtd) { >> + dai = snd_soc_rtd_to_cpu(rtd, 0); >> + /* >> + * Only look for playback widget. RTD number carries the assigned >> + * PCM index. >> + */ >> + if (dai->stream[0].widget == w) >> + return rtd->num; >> + } >> + >> + return -1; >> +} >> + >> +static int q6usb_usb_mixer_enabled(struct snd_soc_dapm_widget *w) >> +{ >> + struct snd_soc_dapm_path *p; >> + >> + /* Checks to ensure USB path is enabled/connected */ >> + snd_soc_dapm_widget_for_each_sink_path(w, p) >> + if (!strcmp(p->sink->name, "USB Mixer") && p->connect) >> + return 1; >> + >> + return 0; >> +} >> + >> +static int q6usb_get_pcm_id(struct snd_soc_component *component) >> +{ >> + struct snd_soc_dapm_widget *w; >> + struct snd_soc_dapm_path *p; >> + int pidx; >> + >> + /* >> + * Traverse widgets to find corresponding FE widget. The DAI links are >> + * built like the following: >> + * MultiMedia* <-> MM_DL* <-> USB Mixer* >> + */ >> + for_each_card_widgets(component->card, w) { >> + if (!strncmp(w->name, "MultiMedia", 10)) { >> + /* >> + * Look up all paths associated with the FE widget to see if >> + * the USB BE is enabled. The sink widget is responsible to >> + * link with the USB mixers. >> + */ >> + snd_soc_dapm_widget_for_each_sink_path(w, p) { >> + if (q6usb_usb_mixer_enabled(p->sink)) { >> + pidx = q6usb_get_pcm_id_from_widget(w); >> + return pidx; >> + } >> + } > Humm, there should be a note that the design assumes that the USB > offload path exposes a single PCM per endpoints - same as the > non-offloaded path. If the ASoC card has multiple PCMs for each > endpoint, possibly with different processing on each PCM, then the > mapping would fail. Sure I'll add a note within the comments on the above. > The other question is whether you need to walk in the DAPM graph, in > theory DPCM has helpers to find which FEs are connected to which BE. Hmm...Yes, I've tried to see if I could utilize some of the helpers that were present, but none of them was able to fetch the DAPM widget that was associated with the FE, hence why I had to implement the lookup that would work for our current design. Thanks Wesley Cheng
Hi Pierre, On 8/30/2024 3:05 AM, Pierre-Louis Bossart wrote: > > On 8/29/24 21:41, Wesley Cheng wrote: >> In order to allow userspace/applications know about USB offloading status, >> expose a sound kcontrol that fetches information about which sound card >> and PCM index the USB device is mapped to for supporting offloading. In >> the USB audio offloading framework, the ASoC BE DAI link is the entity >> responsible for registering to the SOC USB layer. >> >> It is expected for the USB SND offloading driver to add the kcontrol to the >> sound card associated with the USB audio device. An example output would >> look like: > this is very odd, the offloading driver adds a control to another card? > > That seems like a rather important layering violation. > > I thought it was done the other way, the USB audio card created a > control that would point to the other card/device. The USB SND offloading vendor driver (qc_audio_offload), which technically co-exists with USB SND core, is the entity that will add the offload kcontrol to the USB audio device's sound card. Initially, I had the kcontrol creation as part of the USB SND mixer, but I believe Takashi preferred if the vendor driver did it instead. > >> tinymix -D 1 get 'USB Offload Playback Route PCM#0' >> -1, -1 (range -1->255) >> >> This example signifies that there is no mapped ASoC path available for the >> USB SND device. > but that control would not even exist if the ASoC-based driver isn't probed. > > It's become really hard to follow, I've been on all this for 1.5hrs and > losing track of the design. Hence why it probably is a good idea to leave it within the USB offload vendor driver. There are checks to ensure that the ASoC based driver is probed/available before the kcontrols are created for a device. If there are devices that were identified before the ASoC components were probed, then the snd_usb_rediscover_devices() is triggered as part of soc-usb, and that would call the connect_cb() callback to the USB SND vendor offload driver to create the kcontrol. Thanks Wesley Cheng >> tinymix -D 1 get 'USB Offload Playback Route PCM#0' >> 0, 0 (range -1->255) >> >> This example signifies that the offload path is available over ASoC sound >> card index#0 and PCM device#0. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> sound/usb/Kconfig | 10 +++ >> sound/usb/Makefile | 2 + >> sound/usb/mixer_usb_offload.c | 102 ++++++++++++++++++++++++++++++ >> sound/usb/mixer_usb_offload.h | 17 +++++ >> sound/usb/qcom/Makefile | 2 +- >> sound/usb/qcom/qc_audio_offload.c | 2 + >> 6 files changed, 134 insertions(+), 1 deletion(-) >> create mode 100644 sound/usb/mixer_usb_offload.c >> create mode 100644 sound/usb/mixer_usb_offload.h >> >> diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig >> index 5cc3eaf53e6b..e3fbf9541d0b 100644 >> --- a/sound/usb/Kconfig >> +++ b/sound/usb/Kconfig >> @@ -176,10 +176,20 @@ config SND_BCD2000 >> To compile this driver as a module, choose M here: the module >> will be called snd-bcd2000. >> >> +config SND_USB_OFFLOAD_MIXER >> + tristate "USB Audio Offload mixer control" >> + help >> + Say Y to enable the USB audio offloading mixer controls. This >> + exposes an USB offload capable kcontrol to signal to applications >> + about which platform sound card can support USB audio offload. >> + The returning values specify the mapped ASoC card and PCM device >> + the USB audio device is associated to. >> + >> config SND_USB_AUDIO_QMI >> tristate "Qualcomm Audio Offload driver" >> depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND && SND_SOC_USB >> select SND_PCM >> + select SND_USB_OFFLOAD_MIXER >> help >> Say Y here to enable the Qualcomm USB audio offloading feature. >> >> diff --git a/sound/usb/Makefile b/sound/usb/Makefile >> index 54a06a2f73ca..2f19f854944c 100644 >> --- a/sound/usb/Makefile >> +++ b/sound/usb/Makefile >> @@ -36,3 +36,5 @@ obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o >> >> obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ qcom/ >> obj-$(CONFIG_SND_USB_LINE6) += line6/ >> + >> +obj-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o >> diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c >> new file mode 100644 >> index 000000000000..16e2fd634684 >> --- /dev/null >> +++ b/sound/usb/mixer_usb_offload.c >> @@ -0,0 +1,102 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/usb.h> >> + >> +#include <sound/core.h> >> +#include <sound/control.h> >> +#include <sound/soc-usb.h> >> + >> +#include "usbaudio.h" >> +#include "card.h" >> +#include "helper.h" >> +#include "mixer.h" >> + >> +#include "mixer_usb_offload.h" >> + >> +#define PCM_IDX(n) ((n) & 0xffff) >> +#define CARD_IDX(n) ((n) >> 16) >> + >> +static int >> +snd_usb_offload_route_get(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct device *sysdev = snd_kcontrol_chip(kcontrol); >> + int ret; >> + >> + ret = snd_soc_usb_update_offload_route(sysdev, >> + CARD_IDX(kcontrol->private_value), >> + PCM_IDX(kcontrol->private_value), >> + SNDRV_PCM_STREAM_PLAYBACK, >> + ucontrol->value.integer.value); >> + if (ret < 0) { >> + ucontrol->value.integer.value[0] = -1; >> + ucontrol->value.integer.value[1] = -1; >> + } >> + >> + return 0; >> +} >> + >> +static int snd_usb_offload_route_info(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_info *uinfo) >> +{ >> + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; >> + uinfo->count = 2; >> + uinfo->value.integer.min = -1; >> + /* Arbitrary max value, as there is no 'limit' on number of PCM devices */ >> + uinfo->value.integer.max = 0xff; >> + >> + return 0; >> +} >> + >> +static struct snd_kcontrol_new snd_usb_offload_mapped_ctl = { >> + .iface = SNDRV_CTL_ELEM_IFACE_CARD, >> + .access = SNDRV_CTL_ELEM_ACCESS_READ, >> + .info = snd_usb_offload_route_info, >> + .get = snd_usb_offload_route_get, >> +}; >> + >> +/** >> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >> + * @chip - USB SND chip device >> + * >> + * Creates a sound control for a USB audio device, so that applications can >> + * query for if there is an available USB audio offload path, and which >> + * card is managing it. >> + */ >> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >> +{ >> + struct usb_device *udev = chip->dev; >> + struct snd_kcontrol_new *chip_kctl; >> + struct snd_usb_substream *subs; >> + struct snd_usb_stream *as; >> + char ctl_name[37]; >> + int ret; >> + >> + list_for_each_entry(as, &chip->pcm_list, list) { >> + subs = &as->substream[SNDRV_PCM_STREAM_PLAYBACK]; >> + if (!subs->ep_num) >> + continue; >> + >> + chip_kctl = &snd_usb_offload_mapped_ctl; >> + chip_kctl->count = 1; >> + /* >> + * Store the associated USB SND card number and PCM index for >> + * the kctl. >> + */ >> + chip_kctl->private_value = as->pcm_index | >> + chip->card->number << 16; >> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >> + as->pcm_index); >> + chip_kctl->name = ctl_name; >> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >> + udev->bus->sysdev)); >> + if (ret < 0) >> + break; >> + } >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_offload_create_ctl); >> diff --git a/sound/usb/mixer_usb_offload.h b/sound/usb/mixer_usb_offload.h >> new file mode 100644 >> index 000000000000..3f6e2a8b19c8 >> --- /dev/null >> +++ b/sound/usb/mixer_usb_offload.h >> @@ -0,0 +1,17 @@ >> +/* SPDX-License-Identifier: GPL-2.0 >> + * >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#ifndef __USB_OFFLOAD_MIXER_H >> +#define __USB_OFFLOAD_MIXER_H >> + >> +#if IS_ENABLED(CONFIG_SND_USB_OFFLOAD_MIXER) >> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip); >> +#else >> +static inline int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >> +{ >> + return 0; >> +} >> +#endif >> +#endif /* __USB_OFFLOAD_MIXER_H */ >> diff --git a/sound/usb/qcom/Makefile b/sound/usb/qcom/Makefile >> index a81c9b28d484..4005e8391ab9 100644 >> --- a/sound/usb/qcom/Makefile >> +++ b/sound/usb/qcom/Makefile >> @@ -1,2 +1,2 @@ >> snd-usb-audio-qmi-objs := usb_audio_qmi_v01.o qc_audio_offload.o >> -obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o >> \ No newline at end of file >> +obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o >> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c >> index a7ad15404fd1..5b9262a116be 100644 >> --- a/sound/usb/qcom/qc_audio_offload.c >> +++ b/sound/usb/qcom/qc_audio_offload.c >> @@ -36,6 +36,7 @@ >> #include "../helper.h" >> #include "../pcm.h" >> #include "../power.h" >> +#include "../mixer_usb_offload.h" >> >> #include "usb_audio_qmi_v01.h" >> >> @@ -1703,6 +1704,7 @@ static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip) >> sdev->card_idx = chip->card->number; >> sdev->chip_idx = chip->index; >> >> + snd_usb_offload_create_ctl(chip); >> snd_soc_usb_connect(usb_get_usb_backend(udev), sdev); >> } >>
Hi Pierre, On 8/30/2024 2:58 AM, Pierre-Louis Bossart wrote: > > On 8/29/24 21:41, Wesley Cheng wrote: >> Utilize the card and PCM index coming from the USB QMI stream request. >> This field follows what is set by the ASoC USB backend, and could >> potentially carry information about a specific device selected through the >> ASoC USB backend. The backend also has information about the last USB >> sound device plugged in, so it can choose to select the last device plugged >> in, accordingly. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> sound/usb/qcom/qc_audio_offload.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c >> index 0bd533f539e4..a7ad15404fd1 100644 >> --- a/sound/usb/qcom/qc_audio_offload.c >> +++ b/sound/usb/qcom/qc_audio_offload.c >> @@ -106,8 +106,6 @@ struct uaudio_qmi_dev { >> bool er_mapped; >> /* reference count to number of possible consumers */ >> atomic_t qdev_in_use; >> - /* idx to last udev card number plugged in */ >> - unsigned int last_card_num; >> }; >> >> struct uaudio_dev { >> @@ -1261,7 +1259,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs, >> >> pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8; >> xfer_buf_len = req_msg->xfer_buff_size; >> - card_num = uaudio_qdev->last_card_num; >> + card_num = (req_msg->usb_token & QMI_STREAM_REQ_CARD_NUM_MASK) >> 16; >> >> if (!uadev[card_num].ctrl_intf) { >> dev_err(&subs->dev->dev, "audio ctrl intf info not cached\n"); >> @@ -1455,8 +1453,7 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle, >> >> direction = (req_msg->usb_token & QMI_STREAM_REQ_DIRECTION); >> pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8; >> - pcm_card_num = req_msg->enable ? uaudio_qdev->last_card_num : >> - ffs(uaudio_qdev->card_slot) - 1; >> + pcm_card_num = (req_msg->usb_token & QMI_STREAM_REQ_CARD_NUM_MASK) >> 16; >> if (pcm_card_num >= SNDRV_CARDS) { >> ret = -EINVAL; >> goto response; >> @@ -1706,7 +1703,6 @@ static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip) >> sdev->card_idx = chip->card->number; >> sdev->chip_idx = chip->index; >> >> - uaudio_qdev->last_card_num = chip->card->number; >> snd_soc_usb_connect(usb_get_usb_backend(udev), sdev); >> } > This entire path seems like a bad split/merge, it removes stuff that was > done earlier. Also it's not clear what this has to do with 'QMI', card > and PCM device management is usually done at a higher level. > > not following, please be mindful of reviewer fatigue when adding such > changes in patch 30/33.... I'll just add this as part of patch#28. I think before I did the reordering of the series, this made a bit more sense to have as a patch on its own. Now that the entire framework for the audio dsp to know about the card and pcm index is already done in previous patches, the plumbing is done for the qc_audio_offload to utilize the fields coming from the audio DSP, as they will carry valid values. Thanks Wesley Cheng