Message ID | 20240208231406.27397-1-quic_wcheng@quicinc.com |
---|---|
Headers | show |
Series | Introduce QC USB SND audio offloading support | expand |
Hi Mark/Takashi, On 2/8/2024 3:13 PM, Wesley Cheng wrote: <snip> Would it be possible to see if we could start pulling some of these non offloading dependent changes into your repos? It would really be helpful since the # of patches is getting a little cumbersome to maintain. If we need to make any follow ups, I can address them as a separate patch and add it to the series w/ the other changes that are still pending. > Mathias Nyman (13): > xhci: fix possible null pointer dereference at secondary interrupter > removal > xhci: fix off by one check when adding a secondary interrupter. > xhci: Add interrupt pending autoclear flag to each interrupter > xhci: Add helper to set an interrupters interrupt moderation interval > xhci: make isoc_bei_interval variable interrupter specific. > xhci: remove unnecessary event_ring_deq parameter from > xhci_handle_event() > xhci: update event ring dequeue pointer position to controller > correctly > xhci: move event processing for one interrupter to a separate function > xhci: add helper that checks for unhandled events on a event ring > xhci: Don't check if the event ring is valid before every event TRB > xhci: Decouple handling an event from checking for unhandled events > xhci: add helper to stop endpoint and wait for completion > xhci: sideband: add initial api to register a sideband entity > Will work with Mathias on the XHCI stuff when he gets some time to review the set of changes added for the XHCI interrupters. Some of the series in USB SND are dependent on that, such as the qc_audio_offload driver, since it makes calls into the XHCI sideband that was added, so won't be able to pull that in yet. (This is the only driver that will interact w/ XHCI sideband, all ASoC and general USB SND changes are independent) Thanks Wesley Cheng > Wesley Cheng (40): > usb: host: xhci: Export enable and disable interrupter APIs > usb: host: xhci: Repurpose event handler for skipping interrupter > events > xhci: export XHCI IMOD setting helper for interrupters > usb: host: xhci-sideband: Expose a sideband interrupter enable API > usb: host: xhci-mem: Cleanup pending secondary event ring events > usb: host: xhci-mem: Allow for interrupter clients to choose specific > index > ASoC: Add SOC USB APIs for adding an USB backend > ASoC: dt-bindings: qcom,q6dsp-lpass-ports: Add USB_RX port > ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp > ASoC: qdsp6: q6afe: Increase APR timeout > ASoC: qcom: qdsp6: Add USB backend ASoC driver for Q6> ALSA: usb-audio: Introduce USB SND platform op callbacks > ALSA: usb-audio: Export USB SND APIs for modules > ALSA: usb-audio: Save UAC sample size information > usb: dwc3: Specify maximum number of XHCI interrupters > usb: host: xhci-plat: Set XHCI max interrupters if property is present > ALSA: usb-audio: qcom: Add USB QMI definitions > ALSA: usb-audio: qcom: Introduce QC USB SND offloading support > ALSA: usb-audio: Check for support for requested audio format > ASoC: usb: Add PCM format check API for USB backend > ASoC: qcom: qdsp6: Ensure PCM format is supported by USB audio device > ALSA: usb-audio: Prevent starting of audio stream if in use > ALSA: usb-audio: Do not allow USB offload path if PCM device is in use > ASoC: dt-bindings: Add Q6USB backend > ASoC: dt-bindings: Update example for enabling USB offload on SM8250 > ALSA: usb-audio: qcom: Populate PCM and USB chip information > ASoC: qcom: qdsp6: Add support to track available USB PCM devices > ASoC: Introduce SND kcontrols to select sound card and PCM device > ASoC: qcom: qdsp6: Add SOC USB offload select get/put callbacks > ASoC: Add SND kcontrol for fetching USB offload status > ASoC: qcom: qdsp6: Add PCM ops to track current state > ASoC: usb: Create SOC USB SND jack kcontrol > ASoC: qcom: qdsp6: Add headphone jack for offload connection status > ASoC: usb: Fetch ASoC sound card information > ALSA: usb-audio: mixer: Add USB offloading mixer control > ALSA: usb-audio: qcom: Use card and PCM index from QMI request > ALSA: usb-audio: Allow for rediscovery of connected USB SND devices > ASoC: usb: Rediscover USB SND devices on USB port add > ASoC: qcom: Populate SoC components string > ASoC: doc: Add documentation for SOC USB > > .../devicetree/bindings/sound/qcom,q6usb.yaml | 55 + > .../bindings/sound/qcom,sm8250.yaml | 15 + > Documentation/sound/soc/index.rst | 1 + > Documentation/sound/soc/usb.rst | 611 ++++++ > drivers/usb/dwc3/core.c | 12 + > drivers/usb/dwc3/core.h | 2 + > drivers/usb/dwc3/host.c | 5 +- > drivers/usb/host/Kconfig | 9 + > drivers/usb/host/Makefile | 2 + > drivers/usb/host/xhci-mem.c | 53 +- > drivers/usb/host/xhci-plat.c | 2 + > drivers/usb/host/xhci-ring.c | 240 ++- > drivers/usb/host/xhci-sideband.c | 439 ++++ > drivers/usb/host/xhci.c | 97 +- > drivers/usb/host/xhci.h | 21 +- > .../sound/qcom,q6dsp-lpass-ports.h | 1 + > include/linux/usb/xhci-sideband.h | 70 + > include/sound/q6usboffload.h | 20 + > include/sound/soc-usb.h | 90 + > sound/soc/Makefile | 2 +- > sound/soc/qcom/Kconfig | 4 + > sound/soc/qcom/common.c | 41 + > sound/soc/qcom/common.h | 4 +- > sound/soc/qcom/qdsp6/Makefile | 1 + > sound/soc/qcom/qdsp6/q6afe-dai.c | 60 + > sound/soc/qcom/qdsp6/q6afe.c | 193 +- > sound/soc/qcom/qdsp6/q6afe.h | 36 +- > sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c | 23 + > sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h | 1 + > sound/soc/qcom/qdsp6/q6routing.c | 9 + > sound/soc/qcom/qdsp6/q6usb.c | 401 ++++ > sound/soc/qcom/sm8250.c | 14 +- > sound/soc/soc-usb.c | 538 +++++ > sound/usb/Kconfig | 19 + > sound/usb/Makefile | 3 +- > sound/usb/card.c | 109 + > sound/usb/card.h | 24 + > sound/usb/endpoint.c | 1 + > sound/usb/format.c | 1 + > sound/usb/helper.c | 1 + > sound/usb/mixer.c | 5 + > sound/usb/mixer_usb_offload.c | 72 + > sound/usb/mixer_usb_offload.h | 17 + > sound/usb/pcm.c | 104 +- > sound/usb/pcm.h | 11 + > sound/usb/qcom/Makefile | 2 + > sound/usb/qcom/qc_audio_offload.c | 1910 +++++++++++++++++ > sound/usb/qcom/usb_audio_qmi_v01.c | 892 ++++++++ > sound/usb/qcom/usb_audio_qmi_v01.h | 162 ++ > 49 files changed, 6228 insertions(+), 177 deletions(-) > create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb.yaml > create mode 100644 Documentation/sound/soc/usb.rst > create mode 100644 drivers/usb/host/xhci-sideband.c > create mode 100644 include/linux/usb/xhci-sideband.h > create mode 100644 include/sound/q6usboffload.h > create mode 100644 include/sound/soc-usb.h > create mode 100644 sound/soc/qcom/qdsp6/q6usb.c > create mode 100644 sound/soc/soc-usb.c > create mode 100644 sound/usb/mixer_usb_offload.c > create mode 100644 sound/usb/mixer_usb_offload.h > create mode 100644 sound/usb/qcom/Makefile > create mode 100644 sound/usb/qcom/qc_audio_offload.c > create mode 100644 sound/usb/qcom/usb_audio_qmi_v01.c > create mode 100644 sound/usb/qcom/usb_audio_qmi_v01.h >
On Thu, Feb 08, 2024 at 03:33:06PM -0800, Wesley Cheng wrote: > Hi Mark/Takashi, > > On 2/8/2024 3:13 PM, Wesley Cheng wrote: > <snip> > > Would it be possible to see if we could start pulling some of these non > offloading dependent changes into your repos? It would really be helpful > since the # of patches is getting a little cumbersome to maintain. If we > need to make any follow ups, I can address them as a separate patch and add > it to the series w/ the other changes that are still pending. Or if someone gives me acks, I can take them through my usb tree as well. thanks, greg k-h
On Thu, Feb 08, 2024 at 03:13:14PM -0800, Wesley Cheng wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Don't try to remove a secondary interrupter that is known to be invalid. > Also check if the interrupter is valid inside the spinlock that protects > the array of interrupters. > > Found by smatch static checker > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/linux-usb/ffaa0a1b-5984-4a1f-bfd3-9184630a97b9@moroto.mountain/ > Fixes: c99b38c41234 ("xhci: add support to allocate several interrupters") > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > Link: https://lore.kernel.org/r/20240125152737.2983959-2-mathias.nyman@linux.intel.com > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> Wait, this is already in my tree, right? Why keep sending it? confused, greg k-h
On Fri, 09 Feb 2024 00:14:01 +0100, 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 > index is associated with the ASoC platform card supporting offloading. In > the USB audio offloading framework, the ASoC BE DAI link is the entity > responsible for registering to the SOC USB layer. SOC USB will expose more > details about the current offloading status, which includes the USB sound > card and USB PCM device indexes currently being used. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> Now looking at this again, I noticed that this will bring the hard-dependency on ASoC stuff to USB-audio driver, since it adds the call of snd_soc_usb_device_offload_available(). Maybe we can let the add-on platform adding/removing the control element on the fly instead? thanks, Takashi > --- > sound/usb/Kconfig | 4 ++ > sound/usb/Makefile | 1 + > sound/usb/mixer.c | 5 +++ > sound/usb/mixer_usb_offload.c | 72 +++++++++++++++++++++++++++++++++++ > sound/usb/mixer_usb_offload.h | 17 +++++++++ > 5 files changed, 99 insertions(+) > 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 4c842fbe6365..3e7be258d0e3 100644 > --- a/sound/usb/Kconfig > +++ b/sound/usb/Kconfig > @@ -176,10 +176,14 @@ 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 > + bool > + > config SND_USB_AUDIO_QMI > tristate "Qualcomm Audio Offload driver" > depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND > 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 246788268ddd..8c54660a11b0 100644 > --- a/sound/usb/Makefile > +++ b/sound/usb/Makefile > @@ -22,6 +22,7 @@ snd-usb-audio-objs := card.o \ > stream.o \ > validate.o > > +snd-usb-audio-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o > snd-usb-audio-$(CONFIG_SND_USB_AUDIO_MIDI_V2) += midi2.o > snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c > index 409fc1164694..09229e623469 100644 > --- a/sound/usb/mixer.c > +++ b/sound/usb/mixer.c > @@ -48,6 +48,7 @@ > #include "mixer.h" > #include "helper.h" > #include "mixer_quirks.h" > +#include "mixer_usb_offload.h" > #include "power.h" > > #define MAX_ID_ELEMS 256 > @@ -3609,6 +3610,10 @@ int snd_usb_create_mixer(struct snd_usb_audio *chip, int ctrlif) > if (err < 0) > goto _error; > > + err = snd_usb_offload_init_mixer(mixer); > + if (err < 0) > + goto _error; > + > err = snd_device_new(chip->card, SNDRV_DEV_CODEC, mixer, &dev_ops); > if (err < 0) > goto _error; > diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c > new file mode 100644 > index 000000000000..61b17359b987 > --- /dev/null > +++ b/sound/usb/mixer_usb_offload.c > @@ -0,0 +1,72 @@ > +// 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 "card.h" > +#include "mixer.h" > +#include "mixer_usb_offload.h" > +#include "usbaudio.h" > + > +static int > +snd_usb_offload_create_mixer(struct usb_mixer_interface *mixer, > + const struct snd_kcontrol_new *new_kctl) > +{ > + struct snd_usb_audio *chip = mixer->chip; > + struct usb_device *udev = chip->dev; > + > + return snd_ctl_add(chip->card, > + snd_ctl_new1(new_kctl, udev->bus->sysdev)); > +} > + > +static int > +snd_usb_offload_available_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct device *sysdev = snd_kcontrol_chip(kcontrol); > + int ret; > + > + ret = snd_soc_usb_device_offload_available(sysdev); > + ucontrol->value.integer.value[0] = ret < 0 ? -1 : ret; > + > + return ret < 0 ? ret : 0; > +} > + > +static int snd_usb_offload_available_info(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_info *uinfo) > +{ > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = 1; > + uinfo->value.integer.min = -1; > + uinfo->value.integer.max = SNDRV_CARDS; > + > + return 0; > +} > + > +static const struct snd_kcontrol_new snd_usb_offload_available_ctl = { > + .iface = SNDRV_CTL_ELEM_IFACE_CARD, > + .access = SNDRV_CTL_ELEM_ACCESS_READ, > + .name = "USB Offload Playback Capable Card", > + .info = snd_usb_offload_available_info, > + .get = snd_usb_offload_available_get, > +}; > + > +/** > + * snd_usb_offload_init_mixer() - Add USB offload bounded mixer > + * @mixer - USB mixer > + * > + * 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_init_mixer(struct usb_mixer_interface *mixer) > +{ > + return snd_usb_offload_create_mixer(mixer, &snd_usb_offload_available_ctl); > +} > +EXPORT_SYMBOL_GPL(snd_usb_offload_init_mixer); > diff --git a/sound/usb/mixer_usb_offload.h b/sound/usb/mixer_usb_offload.h > new file mode 100644 > index 000000000000..fb88e872d8fa > --- /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_init_mixer(struct usb_mixer_interface *mixer); > +#else > +static int snd_usb_offload_init_mixer(struct usb_mixer_interface *mixer) > +{ > + return 0; > +} > +#endif > +#endif /* __USB_OFFLOAD_MIXER_H */
On Fri, 09 Feb 2024 00:13:54 +0100, Wesley Cheng wrote: > > Add SND kcontrol to SOC USB, which will allow for userpsace to determine > which USB card number and PCM device to offload. This allows for userspace > to potentially tag an alternate path for a specific USB SND card and PCM > device. Previously, control was absent, and the offload path would be > enabled on the last USB SND device which was connected. This logic will > continue to be applicable if no mixer input is received for specific device > selection. > > An example to configure the offload device using tinymix: > tinymix -D 0 set 'SNDUSB OFFLD device select' 1 0 As I mentioned in another patch, the control element name should be more understandable. The same applied even for ASoC stuff. The current name is way too cryptic. thanks, Takashi
On Fri, 09 Feb 2024 00:13:45 +0100, 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> > --- > sound/usb/card.c | 31 +++++++++++++++++++++++++++++++ > sound/usb/card.h | 11 +++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 7dc8007ba839..1ad99a462038 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void) > } > EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); > > +/* > + * Checks to see if requested audio profile, i.e sample rate, # of > + * channels, etc... is supported by the substream associated to the > + * USB audio device. > + */ > +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > + struct snd_pcm_hw_params *params, int direction) > +{ > + struct snd_usb_audio *chip; > + struct snd_usb_substream *subs; > + struct snd_usb_stream *as; > + > + /* > + * Register mutex is held when populating and clearing usb_chip > + * array. > + */ > + guard(mutex)(®ister_mutex); > + chip = usb_chip[card_idx]; > + > + if (chip && enable[card_idx]) { > + list_for_each_entry(as, &chip->pcm_list, list) { > + subs = &as->substream[direction]; > + if (snd_usb_find_substream_format(subs, params)) > + return as; > + } > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > + > /* > * disconnect streams > * called from usb_audio_disconnect() > diff --git a/sound/usb/card.h b/sound/usb/card.h > index 02e4ea898db5..ed4a664e24e5 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -217,4 +217,15 @@ struct snd_usb_platform_ops { > > int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); > int snd_usb_unregister_platform_ops(void); > + > +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) > +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > + struct snd_pcm_hw_params *params, int direction); > +#else > +static struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > + struct snd_pcm_hw_params *params, int direction) > +{ > + return NULL; > +} > +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ The usefulness of ifdef guard here is doubtful, IMO. This header is only for USB-audio driver enablement, and not seen as generic helpers. So, just add the new function declarations without dummy definitions. thanks, Takashi
On Fri, 09 Feb 2024 00:13:33 +0100, Wesley Cheng wrote: > > Some platforms may have support for offloading USB audio devices to a > dedicated audio DSP. Introduce a set of APIs that allow for management of > USB sound card and PCM devices enumerated by the USB SND class driver. > This allows for the ASoC components to be aware of what USB devices are > available for offloading. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> (snip) > --- a/sound/soc/Makefile > +++ b/sound/soc/Makefile > @@ -1,5 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > -snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-utils.o soc-dai.o soc-component.o > +snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-usb.o soc-utils.o soc-dai.o soc-component.o > snd-soc-core-objs += soc-pcm.o soc-devres.o soc-ops.o soc-link.o soc-card.o > snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o Do we really want to build this into ASoC core unconditionally? This is very specific to Qualcomm USB-offload stuff, so it's better to factor out. thanks, Takashi
On Fri, 09 Feb 2024 00:13:58 +0100, Wesley Cheng wrote: > > Expose API for creation of a jack control for notifying of available > devices that are plugged in/discovered, and that support offloading. This > allows for control names to be standardized across implementations of USB > audio offloading. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> Again, use a more intuitive control element name. thanks, Takashi
Hi Greg, On 2/9/2024 2:22 AM, Greg KH wrote: > On Thu, Feb 08, 2024 at 03:13:14PM -0800, Wesley Cheng wrote: >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> >> Don't try to remove a secondary interrupter that is known to be invalid. >> Also check if the interrupter is valid inside the spinlock that protects >> the array of interrupters. >> >> Found by smatch static checker >> >> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >> Closes: https://lore.kernel.org/linux-usb/ffaa0a1b-5984-4a1f-bfd3-9184630a97b9@moroto.mountain/ >> Fixes: c99b38c41234 ("xhci: add support to allocate several interrupters") >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> Link: https://lore.kernel.org/r/20240125152737.2983959-2-mathias.nyman@linux.intel.com >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > Wait, this is already in my tree, right? Why keep sending it? > Sorry, I noticed this yesterday night as well when I was preparing some changes to push elsewhere. Will remove the ones I saw that were already present on usb-next. Thanks Wesley Cheng
Hi Takashi, On 2/9/2024 2:54 AM, Takashi Iwai wrote: > On Fri, 09 Feb 2024 00:13:33 +0100, > Wesley Cheng wrote: >> >> Some platforms may have support for offloading USB audio devices to a >> dedicated audio DSP. Introduce a set of APIs that allow for management of >> USB sound card and PCM devices enumerated by the USB SND class driver. >> This allows for the ASoC components to be aware of what USB devices are >> available for offloading. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > (snip) >> --- a/sound/soc/Makefile >> +++ b/sound/soc/Makefile >> @@ -1,5 +1,5 @@ >> # SPDX-License-Identifier: GPL-2.0 >> -snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-utils.o soc-dai.o soc-component.o >> +snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-usb.o soc-utils.o soc-dai.o soc-component.o >> snd-soc-core-objs += soc-pcm.o soc-devres.o soc-ops.o soc-link.o soc-card.o >> snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o > > Do we really want to build this into ASoC core unconditionally? > This is very specific to Qualcomm USB-offload stuff, so it's better to > factor out. > Ideally, the SOC USB part shouldn't be Qualcomm specific. Since I don't have access or insight into how other vendors are achieving the same thing, I can only base the soc-usb layer to work with the information that is required to get the audio stream up and running on the QC platforms. In its simplest form, its basically just a SW entity that notifies ASoC components about changes occurring from USB SND, and I think all vendors that have an ASoC based platform card handling the offload will need this notification. Thanks Wesley Cheng
Hi Takashi, On 2/9/2024 2:42 AM, Takashi Iwai wrote: > On Fri, 09 Feb 2024 00:13:45 +0100, > 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> >> --- >> sound/usb/card.c | 31 +++++++++++++++++++++++++++++++ >> sound/usb/card.h | 11 +++++++++++ >> 2 files changed, 42 insertions(+) >> >> diff --git a/sound/usb/card.c b/sound/usb/card.c >> index 7dc8007ba839..1ad99a462038 100644 >> --- a/sound/usb/card.c >> +++ b/sound/usb/card.c >> @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void) >> } >> EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); >> >> +/* >> + * Checks to see if requested audio profile, i.e sample rate, # of >> + * channels, etc... is supported by the substream associated to the >> + * USB audio device. >> + */ >> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >> + struct snd_pcm_hw_params *params, int direction) >> +{ >> + struct snd_usb_audio *chip; >> + struct snd_usb_substream *subs; >> + struct snd_usb_stream *as; >> + >> + /* >> + * Register mutex is held when populating and clearing usb_chip >> + * array. >> + */ >> + guard(mutex)(®ister_mutex); >> + chip = usb_chip[card_idx]; >> + >> + if (chip && enable[card_idx]) { >> + list_for_each_entry(as, &chip->pcm_list, list) { >> + subs = &as->substream[direction]; >> + if (snd_usb_find_substream_format(subs, params)) >> + return as; >> + } >> + } >> + >> + return NULL; >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >> + >> /* >> * disconnect streams >> * called from usb_audio_disconnect() >> diff --git a/sound/usb/card.h b/sound/usb/card.h >> index 02e4ea898db5..ed4a664e24e5 100644 >> --- a/sound/usb/card.h >> +++ b/sound/usb/card.h >> @@ -217,4 +217,15 @@ struct snd_usb_platform_ops { >> >> int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); >> int snd_usb_unregister_platform_ops(void); >> + >> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) >> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >> + struct snd_pcm_hw_params *params, int direction); >> +#else >> +static struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >> + struct snd_pcm_hw_params *params, int direction) >> +{ >> + return NULL; >> +} >> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ > > The usefulness of ifdef guard here is doubtful, IMO. This header is > only for USB-audio driver enablement, and not seen as generic > helpers. So, just add the new function declarations without dummy > definitions. > Got it, will remove it. We also have a dependency in place for the qc_audio_offload driver and SND USB AUDIO in the Kconfig. Thanks Wesley Cheng
Hi Takashi, On 2/9/2024 3:02 AM, Takashi Iwai wrote: > On Fri, 09 Feb 2024 00:13:58 +0100, > Wesley Cheng wrote: >> >> Expose API for creation of a jack control for notifying of available >> devices that are plugged in/discovered, and that support offloading. This >> allows for control names to be standardized across implementations of USB >> audio offloading. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > Again, use a more intuitive control element name. > Sorry, missed these. Will fix. Thanks Wesley Cheng
Hi Takashi, On 2/9/2024 2:36 AM, Takashi Iwai wrote: > On Fri, 09 Feb 2024 00:14:01 +0100, > 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 >> index is associated with the ASoC platform card supporting offloading. In >> the USB audio offloading framework, the ASoC BE DAI link is the entity >> responsible for registering to the SOC USB layer. SOC USB will expose more >> details about the current offloading status, which includes the USB sound >> card and USB PCM device indexes currently being used. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > Now looking at this again, I noticed that this will bring the > hard-dependency on ASoC stuff to USB-audio driver, since it adds the > call of snd_soc_usb_device_offload_available(). > > Maybe we can let the add-on platform adding/removing the control > element on the fly instead? > Sure, I'll move it into the QC offload driver. As long as we have a standard API within USB SND that other vendors can also add to their offload drivers, I think that is sufficient. Thanks Wesley Cheng > > thanks, > > Takashi > >> --- >> sound/usb/Kconfig | 4 ++ >> sound/usb/Makefile | 1 + >> sound/usb/mixer.c | 5 +++ >> sound/usb/mixer_usb_offload.c | 72 +++++++++++++++++++++++++++++++++++ >> sound/usb/mixer_usb_offload.h | 17 +++++++++ >> 5 files changed, 99 insertions(+) >> 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 4c842fbe6365..3e7be258d0e3 100644 >> --- a/sound/usb/Kconfig >> +++ b/sound/usb/Kconfig >> @@ -176,10 +176,14 @@ 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 >> + bool >> + >> config SND_USB_AUDIO_QMI >> tristate "Qualcomm Audio Offload driver" >> depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND >> 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 246788268ddd..8c54660a11b0 100644 >> --- a/sound/usb/Makefile >> +++ b/sound/usb/Makefile >> @@ -22,6 +22,7 @@ snd-usb-audio-objs := card.o \ >> stream.o \ >> validate.o >> >> +snd-usb-audio-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o >> snd-usb-audio-$(CONFIG_SND_USB_AUDIO_MIDI_V2) += midi2.o >> snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o >> >> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c >> index 409fc1164694..09229e623469 100644 >> --- a/sound/usb/mixer.c >> +++ b/sound/usb/mixer.c >> @@ -48,6 +48,7 @@ >> #include "mixer.h" >> #include "helper.h" >> #include "mixer_quirks.h" >> +#include "mixer_usb_offload.h" >> #include "power.h" >> >> #define MAX_ID_ELEMS 256 >> @@ -3609,6 +3610,10 @@ int snd_usb_create_mixer(struct snd_usb_audio *chip, int ctrlif) >> if (err < 0) >> goto _error; >> >> + err = snd_usb_offload_init_mixer(mixer); >> + if (err < 0) >> + goto _error; >> + >> err = snd_device_new(chip->card, SNDRV_DEV_CODEC, mixer, &dev_ops); >> if (err < 0) >> goto _error; >> diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c >> new file mode 100644 >> index 000000000000..61b17359b987 >> --- /dev/null >> +++ b/sound/usb/mixer_usb_offload.c >> @@ -0,0 +1,72 @@ >> +// 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 "card.h" >> +#include "mixer.h" >> +#include "mixer_usb_offload.h" >> +#include "usbaudio.h" >> + >> +static int >> +snd_usb_offload_create_mixer(struct usb_mixer_interface *mixer, >> + const struct snd_kcontrol_new *new_kctl) >> +{ >> + struct snd_usb_audio *chip = mixer->chip; >> + struct usb_device *udev = chip->dev; >> + >> + return snd_ctl_add(chip->card, >> + snd_ctl_new1(new_kctl, udev->bus->sysdev)); >> +} >> + >> +static int >> +snd_usb_offload_available_get(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct device *sysdev = snd_kcontrol_chip(kcontrol); >> + int ret; >> + >> + ret = snd_soc_usb_device_offload_available(sysdev); >> + ucontrol->value.integer.value[0] = ret < 0 ? -1 : ret; >> + >> + return ret < 0 ? ret : 0; >> +} >> + >> +static int snd_usb_offload_available_info(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_info *uinfo) >> +{ >> + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; >> + uinfo->count = 1; >> + uinfo->value.integer.min = -1; >> + uinfo->value.integer.max = SNDRV_CARDS; >> + >> + return 0; >> +} >> + >> +static const struct snd_kcontrol_new snd_usb_offload_available_ctl = { >> + .iface = SNDRV_CTL_ELEM_IFACE_CARD, >> + .access = SNDRV_CTL_ELEM_ACCESS_READ, >> + .name = "USB Offload Playback Capable Card", >> + .info = snd_usb_offload_available_info, >> + .get = snd_usb_offload_available_get, >> +}; >> + >> +/** >> + * snd_usb_offload_init_mixer() - Add USB offload bounded mixer >> + * @mixer - USB mixer >> + * >> + * 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_init_mixer(struct usb_mixer_interface *mixer) >> +{ >> + return snd_usb_offload_create_mixer(mixer, &snd_usb_offload_available_ctl); >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_offload_init_mixer); >> diff --git a/sound/usb/mixer_usb_offload.h b/sound/usb/mixer_usb_offload.h >> new file mode 100644 >> index 000000000000..fb88e872d8fa >> --- /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_init_mixer(struct usb_mixer_interface *mixer); >> +#else >> +static int snd_usb_offload_init_mixer(struct usb_mixer_interface *mixer) >> +{ >> + return 0; >> +} >> +#endif >> +#endif /* __USB_OFFLOAD_MIXER_H */
On Fri, 09 Feb 2024 21:34:39 +0100, Wesley Cheng wrote: > > Hi Takashi, > > On 2/9/2024 2:54 AM, Takashi Iwai wrote: > > On Fri, 09 Feb 2024 00:13:33 +0100, > > Wesley Cheng wrote: > >> > >> Some platforms may have support for offloading USB audio devices to a > >> dedicated audio DSP. Introduce a set of APIs that allow for management of > >> USB sound card and PCM devices enumerated by the USB SND class driver. > >> This allows for the ASoC components to be aware of what USB devices are > >> available for offloading. > >> > >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > (snip) > >> --- a/sound/soc/Makefile > >> +++ b/sound/soc/Makefile > >> @@ -1,5 +1,5 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> -snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-utils.o soc-dai.o soc-component.o > >> +snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-usb.o soc-utils.o soc-dai.o soc-component.o > >> snd-soc-core-objs += soc-pcm.o soc-devres.o soc-ops.o soc-link.o soc-card.o > >> snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o > > > > Do we really want to build this into ASoC core unconditionally? > > This is very specific to Qualcomm USB-offload stuff, so it's better to > > factor out. > > > > Ideally, the SOC USB part shouldn't be Qualcomm specific. Since I > don't have access or insight into how other vendors are achieving the > same thing, I can only base the soc-usb layer to work with the > information that is required to get the audio stream up and running on > the QC platforms. In its simplest form, its basically just a SW > entity that notifies ASoC components about changes occurring from USB > SND, and I think all vendors that have an ASoC based platform card > handling the offload will need this notification. Yes, but it's not necessarily built into the snd-soc-core module at all, but can be split to another module, right? Otherwise all machines must load this code even if it doesn't use at all. If this were common among various chips, it'd be worth to be merged into the default common module. But I don't think that's the case. thanks, Takashi
Hi Takashi, On 2/10/2024 12:08 AM, Takashi Iwai wrote: > On Fri, 09 Feb 2024 21:34:39 +0100, > Wesley Cheng wrote: >> >> Hi Takashi, >> >> On 2/9/2024 2:54 AM, Takashi Iwai wrote: >>> On Fri, 09 Feb 2024 00:13:33 +0100, >>> Wesley Cheng wrote: >>>> >>>> Some platforms may have support for offloading USB audio devices to a >>>> dedicated audio DSP. Introduce a set of APIs that allow for management of >>>> USB sound card and PCM devices enumerated by the USB SND class driver. >>>> This allows for the ASoC components to be aware of what USB devices are >>>> available for offloading. >>>> >>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>> (snip) >>>> --- a/sound/soc/Makefile >>>> +++ b/sound/soc/Makefile >>>> @@ -1,5 +1,5 @@ >>>> # SPDX-License-Identifier: GPL-2.0 >>>> -snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-utils.o soc-dai.o soc-component.o >>>> +snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-usb.o soc-utils.o soc-dai.o soc-component.o >>>> snd-soc-core-objs += soc-pcm.o soc-devres.o soc-ops.o soc-link.o soc-card.o >>>> snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o >>> >>> Do we really want to build this into ASoC core unconditionally? >>> This is very specific to Qualcomm USB-offload stuff, so it's better to >>> factor out. >>> >> >> Ideally, the SOC USB part shouldn't be Qualcomm specific. Since I >> don't have access or insight into how other vendors are achieving the >> same thing, I can only base the soc-usb layer to work with the >> information that is required to get the audio stream up and running on >> the QC platforms. In its simplest form, its basically just a SW >> entity that notifies ASoC components about changes occurring from USB >> SND, and I think all vendors that have an ASoC based platform card >> handling the offload will need this notification. > > Yes, but it's not necessarily built into the snd-soc-core module at > all, but can be split to another module, right? Otherwise all > machines must load this code even if it doesn't use at all. > If this were common among various chips, it'd be worth to be merged > into the default common module. But I don't think that's the case. > That's fair. I'll make it a separate module and upload v15 tomorrow. Thanks for the explanation. Thanks Wesley Cheng
Hi Takashi, On 2/9/2024 1:34 PM, Wesley Cheng wrote: > Hi Takashi, > > On 2/9/2024 2:42 AM, Takashi Iwai wrote: >> On Fri, 09 Feb 2024 00:13:45 +0100, >> 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> >>> --- >>> sound/usb/card.c | 31 +++++++++++++++++++++++++++++++ >>> sound/usb/card.h | 11 +++++++++++ >>> 2 files changed, 42 insertions(+) >>> >>> diff --git a/sound/usb/card.c b/sound/usb/card.c >>> index 7dc8007ba839..1ad99a462038 100644 >>> --- a/sound/usb/card.c >>> +++ b/sound/usb/card.c >>> @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void) >>> } >>> EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); >>> +/* >>> + * Checks to see if requested audio profile, i.e sample rate, # of >>> + * channels, etc... is supported by the substream associated to the >>> + * USB audio device. >>> + */ >>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >>> + struct snd_pcm_hw_params *params, int direction) >>> +{ >>> + struct snd_usb_audio *chip; >>> + struct snd_usb_substream *subs; >>> + struct snd_usb_stream *as; >>> + >>> + /* >>> + * Register mutex is held when populating and clearing usb_chip >>> + * array. >>> + */ >>> + guard(mutex)(®ister_mutex); >>> + chip = usb_chip[card_idx]; >>> + >>> + if (chip && enable[card_idx]) { >>> + list_for_each_entry(as, &chip->pcm_list, list) { >>> + subs = &as->substream[direction]; >>> + if (snd_usb_find_substream_format(subs, params)) >>> + return as; >>> + } >>> + } >>> + >>> + return NULL; >>> +} >>> +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >>> + >>> /* >>> * disconnect streams >>> * called from usb_audio_disconnect() >>> diff --git a/sound/usb/card.h b/sound/usb/card.h >>> index 02e4ea898db5..ed4a664e24e5 100644 >>> --- a/sound/usb/card.h >>> +++ b/sound/usb/card.h >>> @@ -217,4 +217,15 @@ struct snd_usb_platform_ops { >>> int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); >>> int snd_usb_unregister_platform_ops(void); >>> + >>> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) >>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >>> + struct snd_pcm_hw_params *params, int direction); >>> +#else >>> +static struct snd_usb_stream *snd_usb_find_suppported_substream(int >>> card_idx, >>> + struct snd_pcm_hw_params *params, int direction) >>> +{ >>> + return NULL; >>> +} >>> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ >> >> The usefulness of ifdef guard here is doubtful, IMO. This header is >> only for USB-audio driver enablement, and not seen as generic >> helpers. So, just add the new function declarations without dummy >> definitions. >> > > Got it, will remove it. We also have a dependency in place for the > qc_audio_offload driver and SND USB AUDIO in the Kconfig. > Looking at this again after trying some mixed Kconfig settings. These declarations aren't specific for USB-audio. They are helpers that are exposed to soc usb, so that it can do some basic verification with soc usb before allowing the enable stream to continue. Since the ASoC layer doesn't have insight on what audio profiles are supported by the usb device, this API will ensure that the request profile is supported. Issues are seen when we disable SND USB audio config and enable the ASoC parts. Thanks Wesley Cheng
On Sat, 17 Feb 2024 00:42:18 +0100, Wesley Cheng wrote: > > Hi Takashi, > > On 2/9/2024 1:34 PM, Wesley Cheng wrote: > > Hi Takashi, > > > > On 2/9/2024 2:42 AM, Takashi Iwai wrote: > >> On Fri, 09 Feb 2024 00:13:45 +0100, > >> 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> > >>> --- > >>> sound/usb/card.c | 31 +++++++++++++++++++++++++++++++ > >>> sound/usb/card.h | 11 +++++++++++ > >>> 2 files changed, 42 insertions(+) > >>> > >>> diff --git a/sound/usb/card.c b/sound/usb/card.c > >>> index 7dc8007ba839..1ad99a462038 100644 > >>> --- a/sound/usb/card.c > >>> +++ b/sound/usb/card.c > >>> @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void) > >>> } > >>> EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); > >>> +/* > >>> + * Checks to see if requested audio profile, i.e sample rate, # of > >>> + * channels, etc... is supported by the substream associated to the > >>> + * USB audio device. > >>> + */ > >>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > >>> + struct snd_pcm_hw_params *params, int direction) > >>> +{ > >>> + struct snd_usb_audio *chip; > >>> + struct snd_usb_substream *subs; > >>> + struct snd_usb_stream *as; > >>> + > >>> + /* > >>> + * Register mutex is held when populating and clearing usb_chip > >>> + * array. > >>> + */ > >>> + guard(mutex)(®ister_mutex); > >>> + chip = usb_chip[card_idx]; > >>> + > >>> + if (chip && enable[card_idx]) { > >>> + list_for_each_entry(as, &chip->pcm_list, list) { > >>> + subs = &as->substream[direction]; > >>> + if (snd_usb_find_substream_format(subs, params)) > >>> + return as; > >>> + } > >>> + } > >>> + > >>> + return NULL; > >>> +} > >>> +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > >>> + > >>> /* > >>> * disconnect streams > >>> * called from usb_audio_disconnect() > >>> diff --git a/sound/usb/card.h b/sound/usb/card.h > >>> index 02e4ea898db5..ed4a664e24e5 100644 > >>> --- a/sound/usb/card.h > >>> +++ b/sound/usb/card.h > >>> @@ -217,4 +217,15 @@ struct snd_usb_platform_ops { > >>> int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); > >>> int snd_usb_unregister_platform_ops(void); > >>> + > >>> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) > >>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > >>> + struct snd_pcm_hw_params *params, int direction); > >>> +#else > >>> +static struct snd_usb_stream > >>> *snd_usb_find_suppported_substream(int card_idx, > >>> + struct snd_pcm_hw_params *params, int direction) > >>> +{ > >>> + return NULL; > >>> +} > >>> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ > >> > >> The usefulness of ifdef guard here is doubtful, IMO. This header is > >> only for USB-audio driver enablement, and not seen as generic > >> helpers. So, just add the new function declarations without dummy > >> definitions. > >> > > > > Got it, will remove it. We also have a dependency in place for the > > qc_audio_offload driver and SND USB AUDIO in the Kconfig. > > > > Looking at this again after trying some mixed Kconfig settings. These > declarations aren't specific for USB-audio. They are helpers that are > exposed to soc usb, so that it can do some basic verification with soc > usb before allowing the enable stream to continue. Then rather the question is why snd-soc-usb calls those functions *unconditionally*. No matter whether we have dependencies in Kconfig, calling the function means that the callee shall be drug when the corresponding code is running. If it were generic core API stuff such as power-management or ACPI, it'd make sense to define dummy functions without the enablement, as many code may have optional calls. If the API is enabled, it's anyway in the core. If not, it's optional. That'll be fine. OTOH, the stuff you're calling certainly belongs to snd-usb-audio. Even if the call is really optional, it means that you'll have a hard dependency when snd-usb-audio is built, no matter whether you need or not. > Since the ASoC > layer doesn't have insight on what audio profiles are supported by the > usb device, this API will ensure that the request profile is > supported. > > Issues are seen when we disable SND USB audio config and enable the > ASoC parts. If snd-usb-audio is disabled, what snd-soc-usb would serve at all? Does it still make sense to keep it enabled? That said, the statement above (building snd-soc-usb without snd-usb-audio) looks already dubious; isn't it better to have a proper dependency in Kconfig, instead? thanks, Takashi
Hi Takashi, On 2/17/2024 2:08 AM, Takashi Iwai wrote: > On Sat, 17 Feb 2024 00:42:18 +0100, > Wesley Cheng wrote: >> >> Hi Takashi, >> >> On 2/9/2024 1:34 PM, Wesley Cheng wrote: >>> Hi Takashi, >>> >>> On 2/9/2024 2:42 AM, Takashi Iwai wrote: >>>> On Fri, 09 Feb 2024 00:13:45 +0100, >>>> 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> >>>>> --- >>>>> sound/usb/card.c | 31 +++++++++++++++++++++++++++++++ >>>>> sound/usb/card.h | 11 +++++++++++ >>>>> 2 files changed, 42 insertions(+) >>>>> >>>>> diff --git a/sound/usb/card.c b/sound/usb/card.c >>>>> index 7dc8007ba839..1ad99a462038 100644 >>>>> --- a/sound/usb/card.c >>>>> +++ b/sound/usb/card.c >>>>> @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void) >>>>> } >>>>> EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); >>>>> +/* >>>>> + * Checks to see if requested audio profile, i.e sample rate, # of >>>>> + * channels, etc... is supported by the substream associated to the >>>>> + * USB audio device. >>>>> + */ >>>>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >>>>> + struct snd_pcm_hw_params *params, int direction) >>>>> +{ >>>>> + struct snd_usb_audio *chip; >>>>> + struct snd_usb_substream *subs; >>>>> + struct snd_usb_stream *as; >>>>> + >>>>> + /* >>>>> + * Register mutex is held when populating and clearing usb_chip >>>>> + * array. >>>>> + */ >>>>> + guard(mutex)(®ister_mutex); >>>>> + chip = usb_chip[card_idx]; >>>>> + >>>>> + if (chip && enable[card_idx]) { >>>>> + list_for_each_entry(as, &chip->pcm_list, list) { >>>>> + subs = &as->substream[direction]; >>>>> + if (snd_usb_find_substream_format(subs, params)) >>>>> + return as; >>>>> + } >>>>> + } >>>>> + >>>>> + return NULL; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >>>>> + >>>>> /* >>>>> * disconnect streams >>>>> * called from usb_audio_disconnect() >>>>> diff --git a/sound/usb/card.h b/sound/usb/card.h >>>>> index 02e4ea898db5..ed4a664e24e5 100644 >>>>> --- a/sound/usb/card.h >>>>> +++ b/sound/usb/card.h >>>>> @@ -217,4 +217,15 @@ struct snd_usb_platform_ops { >>>>> int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); >>>>> int snd_usb_unregister_platform_ops(void); >>>>> + >>>>> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) >>>>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >>>>> + struct snd_pcm_hw_params *params, int direction); >>>>> +#else >>>>> +static struct snd_usb_stream >>>>> *snd_usb_find_suppported_substream(int card_idx, >>>>> + struct snd_pcm_hw_params *params, int direction) >>>>> +{ >>>>> + return NULL; >>>>> +} >>>>> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ >>>> >>>> The usefulness of ifdef guard here is doubtful, IMO. This header is >>>> only for USB-audio driver enablement, and not seen as generic >>>> helpers. So, just add the new function declarations without dummy >>>> definitions. >>>> >>> >>> Got it, will remove it. We also have a dependency in place for the >>> qc_audio_offload driver and SND USB AUDIO in the Kconfig. >>> >> >> Looking at this again after trying some mixed Kconfig settings. These >> declarations aren't specific for USB-audio. They are helpers that are >> exposed to soc usb, so that it can do some basic verification with soc >> usb before allowing the enable stream to continue. > > Then rather the question is why snd-soc-usb calls those functions > *unconditionally*. No matter whether we have dependencies in Kconfig, > calling the function means that the callee shall be drug when the > corresponding code is running. > > If it were generic core API stuff such as power-management or ACPI, > it'd make sense to define dummy functions without the enablement, as > many code may have optional calls. If the API is enabled, it's anyway > in the core. If not, it's optional. That'll be fine. > > OTOH, the stuff you're calling certainly belongs to snd-usb-audio. > Even if the call is really optional, it means that you'll have a hard > dependency when snd-usb-audio is built, no matter whether you need or > not. > >> Since the ASoC >> layer doesn't have insight on what audio profiles are supported by the >> usb device, this API will ensure that the request profile is >> supported. >> >> Issues are seen when we disable SND USB audio config and enable the >> ASoC parts. > > If snd-usb-audio is disabled, what snd-soc-usb would serve at all? > Does it still make sense to keep it enabled? > That said, the statement above (building snd-soc-usb without > snd-usb-audio) looks already dubious; isn't it better to have a proper > dependency in Kconfig, instead? > Ok, took a look at it a bit more and should have gotten all the dependencies addressed through Kconfigs. Thanks for the review comments and feedback. Thanks Wesley Cheng