| Message ID | 20230725023416.11205-1-quic_wcheng@quicinc.com |
|---|---|
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
Apologies for the long delay in addressing some of your comments and feedback. I understand it is difficult to refresh a lot of the material after such a long gap, but hopefully I have addressed some of the concerns in the latest revision. Appreciate all the suggestions/reviews! Thanks Wesley Cheng On 7/24/2023 7:33 PM, Wesley Cheng wrote: > Several Qualcomm based chipsets can support USB audio offloading to a > dedicated audio DSP, which can take over issuing transfers to the USB > host controller. The intention is to reduce the load on the main > processors in the SoC, and allow them to be placed into lower power modes. > There are several parts to this design: > 1. Adding ASoC binding layer > 2. Create a USB backend for Q6DSP > 3. Introduce XHCI interrupter support > 4. Create vendor ops for the USB SND driver > > USB | ASoC > -------------------------------------------------------------------- > | _________________________ > | |sm8250 platform card | > | |_________________________| > | | | > | ___V____ ____V____ > | |Q6USB | |Q6AFE | > | |"codec" | |"cpu" | > | |________| |_________| > | ^ ^ ^ > | | |________| > | ___V____ | > | |SOC-USB | | > ________ ________ | | | > |USB SND |<--->|QC offld|<------------>|________| | > |(card.c)| | |<---------- | > |________| |________|___ | | | > ^ ^ | | | ____________V_________ > | | | | | |APR/GLINK | > __ V_______________V_____ | | | |______________________| > |USB SND (endpoint.c) | | | | ^ > |_________________________| | | | | > ^ | | | ___________V___________ > | | | |->|audio DSP | > ___________V_____________ | | |_______________________| > |XHCI HCD |<- | > |_________________________| | > > > Adding ASoC binding layer: > soc-usb: Intention is to treat a USB port similar to a headphone jack. > The port is always present on the device, but cable/pin status can be > enabled/disabled. Expose mechanisms for USB backend ASoC drivers to > communicate with USB SND. > > Create a USB backend for Q6DSP: > q6usb: Basic backend driver that will be responsible for maintaining the > resources needed to initiate a playback stream using the Q6DSP. Will > be the entity that checks to make sure the connected USB audio device > supports the requested PCM format. If it does not, the PCM open call will > fail, and userpsace ALSA can take action accordingly. > > Introduce XHCI interrupter support: > XHCI HCD supports multiple interrupters, which allows for events to be routed > to different event rings. This is determined by "Interrupter Target" field > specified in Section "6.4.1.1 Normal TRB" of the XHCI specification. > > Events in the offloading case will be routed to an event ring that is assigned > to the audio DSP. > > Create vendor ops for the USB SND driver: > qc_audio_offload: This particular driver has several components associated > with it: > - QMI stream request handler > - XHCI interrupter and resource management > - audio DSP memory management > > When the audio DSP wants to enable a playback stream, the request is first > received by the ASoC platform sound card. Depending on the selected route, > ASoC will bring up the individual DAIs in the path. The Q6USB backend DAI > will send an AFE port start command (with enabling the USB playback path), and > the audio DSP will handle the request accordingly. > > Part of the AFE USB port start handling will have an exchange of control > messages using the QMI protocol. The qc_audio_offload driver will populate the > buffer information: > - Event ring base address > - EP transfer ring base address > > and pass it along to the audio DSP. All endpoint management will now be handed > over to the DSP, and the main processor is not involved in transfers. > > Overall, implementing this feature will still expose separate sound card and PCM > devices for both the platorm card and USB audio device: > 0 [SM8250MTPWCD938]: sm8250 - SM8250-MTP-WCD9380-WSA8810-VA-D > SM8250-MTP-WCD9380-WSA8810-VA-DMIC > 1 [Audio ]: USB-Audio - USB Audio > Generic USB Audio at usb-xhci-hcd.1.auto-1.4, high speed > > This is to ensure that userspace ALSA entities can decide which route to take > when executing the audio playback. In the above, if card#1 is selected, then > USB audio data will take the legacy path over the USB PCM drivers, etc... > > This feature was validated using: > - tinymix: set/enable the multimedia path to route to USB backend > - tinyplay: issue playback on platform card > > Changelog > -------------------------------------------- > Changes in v4: > - Rebased to xhci/for-usb-next > - Addressed some dt-bindings comments > > XHCI: > - Pulled in latest changes from Mathias' feature_interrupters branch: > https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters > > - Fixed commit text and signage for the XHCI sideband/interrupter related changes > - Added some logic to address the FIXME tags mentioned throughout the commits, such > as handling multi segment rings and building the SGT, locking concerns, and ep > cleanup operations. > - Removed some fixme tags for conditions that may not be needed/addressed. > - Repurposed the new endpoint stop sync API to be utilized in other places. > - Fixed potential compile issue if XHCI sideband config is not defined. > > ASoC: > - Added sound jack control into the Q6USB driver. Allows for userpsace to know when > an offload capable device is connected. > > USB SND: > - Avoided exporting _snd_pcm_hw_param_set based on Takashi's recommendation. > - Split USB QMI packet header definitions into a separate commit. This is used to > properly allow the QMI interface driver to parse and route QMI packets accordingly > - Added a "depends on" entry when enabling QC audio offload to avoid compile time > issues. > > Changes in v3: > - Changed prefix from RFC to PATCH > - Rebased entire series to usb-next > - Updated copyright years > > XHCI: > - Rebased changes on top of XHCI changes merged into usb-next, and only added > changes that were still under discussion. > - Added change to read in the "num-hc-interrupters" device property. > > ASoC: > - qusb6 USB backend > - Incorporated suggestions to fetch iommu information with existing APIs > - Added two new sound kcontrols to fetch offload status and offload device > selection. > - offload status - will return the card and pcm device in use > tinymix -D 0 get 1 --> 1, 0 (offload in progress on card#1 pcm#0) > > - device selection - set the card and pcm device to enable offload on. Ex.: > tinymix -D 0 set 1 2 0 --> sets offload on card#2 pcm#0 > (this should be the USB card) > > USB SND: > - Fixed up some locking related concerns for registering platform ops. > - Moved callbacks under the register_mutex, so that > - Modified APIs to properly pass more information about the USB SND device, so > that the Q6USB backend can build a device list/map, in order to monitor offload > status and device selection. > > Changes in v2: > > XHCI: > - Replaced XHCI and HCD changes with Mathias' XHCI interrupter changes > in his tree: > https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters > > Adjustments made to Mathias' changes: > - Created xhci-intr.h to export/expose interrupter APIs versus exposing xhci.h. > Moved dependent structures to this file as well. (so clients can parse out > information from "struct xhci_interrupter") > - Added some basic locking when requesting interrupters. > - Fixed up some sanity checks. > - Removed clearing of the ERSTBA during freeing of the interrupter. (pending > issue where SMMU fault occurs if DMA addr returned is 64b - TODO) > > - Clean up pending events in the XHCI secondary interrupter. While testing USB > bus suspend, it was seen that on bus resume, the xHCI HC would run into a command > timeout. > - Added offloading APIs to xHCI to fetch transfer and event ring information. > > ASoC: > - Modified soc-usb to allow for multiple USB port additions. For this to work, > the USB offload driver has to have a reference to the USB backend by adding > a "usb-soc-be" DT entry to the device saved into XHCI sysdev. > - Created separate dt-bindings for defining USB_RX port. > - Increased APR timeout to accommodate the situation where the AFE port start > command could be delayed due to having to issue a USB bus resume while > handling the QMI stream start command. > > USB SND: > - Added a platform ops during usb_audio_suspend(). This allows for the USB > offload driver to halt the audio stream when system enters PM suspend. This > ensures the audio DSP is not issuing transfers on the USB bus. > - Do not override platform ops if they are already populated. > - Introduce a shared status variable between the USB offload and USB SND layers, > to ensure that only one path is active at a time. If the USB bus is occupied, > then userspace is notified that the path is busy. > > Mathias Nyman (3): > xhci: add support to allocate several interrupters > xhci: add helper to stop endpoint and wait for completion > xhci: sideband: add initial api to register a sideband entity > > Wesley Cheng (29): > 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: Add USB backend ASoC driver for Q6 > sound: usb: card: Introduce USB SND platform op callbacks > sound: usb: Export USB SND APIs for modules > dt-bindings: usb: dwc3: Add snps,num-hc-interrupters definition > usb: dwc3: Add DT parameter to specify maximum number of interrupters > usb: host: xhci-plat: Set XHCI max interrupters if property is present > sound: usb: pcm: Export fixed rate check USB SND API > sound: usb: qcom: Add USB QMI definitions > sound: usb: Introduce QC USB SND offloading support > sound: usb: card: Check for support for requested audio format > sound: soc: soc-usb: Add PCM format check API for USB backend > sound: soc: qcom: qusb6: Ensure PCM format is supported by USB audio > device > sound: usb: Prevent starting of audio stream if in use > ASoC: dt-bindings: Add Q6USB backend bindings > ASoC: dt-bindings: Update example for enabling USB offload on SM8250 > ASoC: qcom: qdsp6: q6afe: Split USB AFE dev_token param into separate > API > sound: Pass USB SND card and PCM information to SOC USB > sound: soc: qdsp6: Add SND kcontrol to select offload device > sound: soc: qdsp6: Add SND kcontrol for fetching offload status > sound: soc: qcom: q6usb: Add headphone jack for offload connection > status > sound: usb: qc_audio_offload: Use card and PCM index from QMI request > sound: usb: card: Allow for rediscovery of connected USB SND devices > sound: soc: soc-usb: Rediscover USB SND devices on USB port add > > .../bindings/sound/qcom,q6usb-dais.yaml | 49 + > .../bindings/sound/qcom,sm8250.yaml | 15 + > .../devicetree/bindings/usb/snps,dwc3.yaml | 13 + > arch/arm64/boot/dts/qcom/sm8350.dtsi.rej | 14 + > arch/arm64/configs/defconfig.rej | 24 + > 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 | 4 + > drivers/usb/host/xhci-debugfs.c | 2 +- > drivers/usb/host/xhci-hub.c | 29 +- > drivers/usb/host/xhci-mem.c | 167 +- > drivers/usb/host/xhci-plat.c | 2 + > drivers/usb/host/xhci-ring.c | 2 +- > drivers/usb/host/xhci-sideband.c | 195 ++ > drivers/usb/host/xhci.c | 109 +- > drivers/usb/host/xhci.h | 102 +- > .../sound/qcom,q6dsp-lpass-ports.h | 1 + > include/linux/usb/xhci-intr.h | 86 + > include/linux/usb/xhci-sideband.h | 90 + > include/sound/q6usboffload.h | 20 + > include/sound/soc-usb.h | 39 + > sound/core/oss/pcm_oss.c | 4 +- > sound/soc/Makefile | 2 +- > sound/soc/qcom/Kconfig | 4 + > sound/soc/qcom/qdsp6/Makefile | 1 + > sound/soc/qcom/qdsp6/q6afe-dai.c | 50 + > sound/soc/qcom/qdsp6/q6afe.c | 208 +- > sound/soc/qcom/qdsp6/q6afe.h | 47 +- > 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 | 458 +++++ > sound/soc/soc-usb.c | 197 ++ > sound/usb/Kconfig | 15 + > sound/usb/Makefile | 2 +- > sound/usb/card.c | 85 + > sound/usb/card.h | 32 + > sound/usb/endpoint.c | 2 + > sound/usb/helper.c | 1 + > sound/usb/pcm.c | 29 +- > sound/usb/pcm.h | 12 + > sound/usb/qcom/Makefile | 2 + > sound/usb/qcom/qc_audio_offload.c | 1820 +++++++++++++++++ > sound/usb/qcom/usb_audio_qmi_v01.c | 892 ++++++++ > sound/usb/qcom/usb_audio_qmi_v01.h | 162 ++ > 47 files changed, 4870 insertions(+), 179 deletions(-) > create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > create mode 100644 arch/arm64/boot/dts/qcom/sm8350.dtsi.rej > create mode 100644 arch/arm64/configs/defconfig.rej > create mode 100644 drivers/usb/host/xhci-sideband.c > create mode 100644 include/linux/usb/xhci-intr.h > 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/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 7/24/23 19:34, Wesley Cheng wrote: > diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig > index 059242f15d75..44b0fa92b6cc 100644 > --- a/sound/usb/Kconfig > +++ b/sound/usb/Kconfig > @@ -165,6 +165,21 @@ config SND_BCD2000 > To compile this driver as a module, choose M here: the module > will be called snd-bcd2000. > > +config QC_USB_AUDIO_OFFLOAD > + tristate "Qualcomm Audio Offload driver" > + depends on QCOM_QMI_HELPERS > + select SND_PCM > + help > + Say Y here to enable the Qualcomm USB audio offloading feature feature. > + > + This module sets up the required QMI stream enable/disable > + responses to requests generated by the audio DSP. It passes the > + USB transfer resource references, so that the audio DSP can issue > + USB transfers to the host controller. > + > + To compile this driver as a module, choose M here: the module > + will be called qc-audio-offload.
On 7/24/23 19:33, Wesley Cheng wrote: > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index c170672f847e..d9dc92bea525 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -104,6 +104,15 @@ config USB_XHCI_RZV2M > Say 'Y' to enable the support for the xHCI host controller > found in Renesas RZ/V2M SoC. > > +config USB_XHCI_SIDEBAND > + bool "xHCI support for sideband" > + help > + Say 'Y' to enable the support for the xHCI sideband capability. > + provide a mechanism for a sideband datapath for payload associated Provide > + with audio class endpoints. This allows for an audio DSP to use > + xHCI USB endpoints directly, allowing CPU to sleep while playing > + audio audio.
On 7/24/2023 7:33 PM, Wesley Cheng wrote: > Some vendor modules will utilize useful parsing and endpoint management > APIs to start audio playback/capture. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/core/oss/pcm_oss.c | 4 ++-- > sound/usb/card.c | 2 ++ > sound/usb/endpoint.c | 2 ++ > sound/usb/helper.c | 1 + > sound/usb/pcm.c | 9 ++++++--- > sound/usb/pcm.h | 12 ++++++++++++ > 6 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c > index 728c211142d1..7773b5362e1a 100644 > --- a/sound/core/oss/pcm_oss.c > +++ b/sound/core/oss/pcm_oss.c > @@ -444,8 +444,8 @@ static int snd_pcm_hw_param_near(struct snd_pcm_substream *pcm, > } > > static int _snd_pcm_hw_param_set(struct snd_pcm_hw_params *params, > - snd_pcm_hw_param_t var, unsigned int val, > - int dir) > + snd_pcm_hw_param_t var, unsigned int val, > + int dir) > { > int changed; > if (hw_is_mask(var)) { > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 9365d1e17836..a3fad66a3337 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -1076,6 +1076,7 @@ int snd_usb_autoresume(struct snd_usb_audio *chip) > } > return 0; > } > +EXPORT_SYMBOL_GPL(snd_usb_autoresume); > > void snd_usb_autosuspend(struct snd_usb_audio *chip) > { > @@ -1089,6 +1090,7 @@ void snd_usb_autosuspend(struct snd_usb_audio *chip) > for (i = 0; i < chip->num_interfaces; i++) > usb_autopm_put_interface(chip->intf[i]); > } > +EXPORT_SYMBOL_GPL(snd_usb_autosuspend); > > static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message) > { > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index a385e85c4650..0757499599c8 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -866,6 +866,7 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip, > mutex_unlock(&chip->mutex); > return ep; > } > +EXPORT_SYMBOL_GPL(snd_usb_endpoint_open); > > /* > * snd_usb_endpoint_set_sync: Link data and sync endpoints > @@ -1503,6 +1504,7 @@ int snd_usb_endpoint_prepare(struct snd_usb_audio *chip, > mutex_unlock(&chip->mutex); > return err; > } > +EXPORT_SYMBOL_GPL(snd_usb_endpoint_prepare); > > /* get the current rate set to the given clock by any endpoint */ > int snd_usb_endpoint_get_clock_rate(struct snd_usb_audio *chip, int clock) > diff --git a/sound/usb/helper.c b/sound/usb/helper.c > index bf80e55d013a..4322ae3738e6 100644 > --- a/sound/usb/helper.c > +++ b/sound/usb/helper.c > @@ -62,6 +62,7 @@ void *snd_usb_find_csint_desc(void *buffer, int buflen, void *after, u8 dsubtype > } > return NULL; > } > +EXPORT_SYMBOL_GPL(snd_usb_find_csint_desc); > > /* > * Wrapper for usb_control_msg(). > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index eec5232f9fb2..b6fb6d84d9eb 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -87,7 +87,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream > /* > * find a matching audio format > */ > -static const struct audioformat * > +const struct audioformat * > find_format(struct list_head *fmt_list_head, snd_pcm_format_t format, > unsigned int rate, unsigned int channels, bool strict_match, > struct snd_usb_substream *subs) > @@ -147,8 +147,9 @@ find_format(struct list_head *fmt_list_head, snd_pcm_format_t format, > } > return found; > } > +EXPORT_SYMBOL_GPL(find_format); > > -static const struct audioformat * > +const struct audioformat * > find_substream_format(struct snd_usb_substream *subs, > const struct snd_pcm_hw_params *params) > { > @@ -156,6 +157,7 @@ find_substream_format(struct snd_usb_substream *subs, > params_rate(params), params_channels(params), > true, subs); > } > +EXPORT_SYMBOL_GPL(find_substream_format); > > bool snd_usb_pcm_has_fixed_rate(struct snd_usb_substream *subs) > { > @@ -446,7 +448,7 @@ int snd_usb_pcm_resume(struct snd_usb_stream *as) > return 0; > } > > -static void close_endpoints(struct snd_usb_audio *chip, > +void close_endpoints(struct snd_usb_audio *chip, > struct snd_usb_substream *subs) > { > if (subs->data_endpoint) { > @@ -460,6 +462,7 @@ static void close_endpoints(struct snd_usb_audio *chip, > subs->sync_endpoint = NULL; > } > } > +EXPORT_SYMBOL(close_endpoints); EXPORT_SYMBOL_GPL here as well?
On Mon, Jul 24, 2023 at 07:33:56PM -0700, Wesley Cheng wrote: > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -87,7 +87,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream > /* > * find a matching audio format > */ > -static const struct audioformat * > +const struct audioformat * > find_format(struct list_head *fmt_list_head, snd_pcm_format_t format, > unsigned int rate, unsigned int channels, bool strict_match, > struct snd_usb_substream *subs) > @@ -147,8 +147,9 @@ find_format(struct list_head *fmt_list_head, snd_pcm_format_t format, > } > return found; > } > +EXPORT_SYMBOL_GPL(find_format); This is a horrible symbol name for a global function, same for the other ones in this file. If you really want to export them, please put them in the proper "snd_" prefix namespace, or better yet, use a module namespace as well to ensure that we know who is using them. thanks, greg k-h
On Mon, Jul 24, 2023 at 07:33:45PM -0700, Wesley Cheng wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Modify the XHCI drivers to accommodate for handling multiple event rings in > case there are multiple interrupters. Add the required APIs so clients are > able to allocate/request for an interrupter ring, and pass this information > back to the client driver. This allows for users to handle the resource > accordingly, such as passing the event ring base address to an audio DSP. > There is no actual support for multiple MSI/MSI-X vectors. > > Factoring out XHCI interrupter APIs and structures done by Wesley Cheng, in > order to allow for USB class drivers to utilze them. > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8350.dtsi.rej | 14 ++++ > arch/arm64/configs/defconfig.rej | 24 ++++++ > drivers/usb/host/xhci-debugfs.c | 2 +- > drivers/usb/host/xhci-mem.c | 93 ++++++++++++++++++++++-- > drivers/usb/host/xhci-ring.c | 2 +- > drivers/usb/host/xhci.c | 49 ++++++++----- > drivers/usb/host/xhci.h | 77 +------------------- > include/linux/usb/xhci-intr.h | 86 ++++++++++++++++++++++ > 8 files changed, 245 insertions(+), 102 deletions(-) > create mode 100644 arch/arm64/boot/dts/qcom/sm8350.dtsi.rej > create mode 100644 arch/arm64/configs/defconfig.rej > create mode 100644 include/linux/usb/xhci-intr.h > > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi.rej b/arch/arm64/boot/dts/qcom/sm8350.dtsi.rej > new file mode 100644 > index 000000000000..e96ef1120160 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi.rej > @@ -0,0 +1,14 @@ > +--- arch/arm64/boot/dts/qcom/sm8350.dtsi > ++++ arch/arm64/boot/dts/qcom/sm8350.dtsi > +@@ -2254,9 +2254,9 @@ > + iommus = <&apps_smmu 0x0 0x0>; > + snps,dis_u2_susphy_quirk; > + snps,dis_enblslpm_quirk; > +- phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>; > ++ phys = <&usb_1_hsphy>, <&usb_1_qmpphy>; > + phy-names = "usb2-phy", "usb3-phy"; > +- > ++ dr_mode = "host"; > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > diff --git a/arch/arm64/configs/defconfig.rej b/arch/arm64/configs/defconfig.rej <snip> I think you need to look at this patch series a bit better and clean it up before sending it out. These files obviously are not going to work, and prevent any of this from being able to be applied or tested by any bots or test systems :( I've stopped here in my review of this series (except for the obvious global symbol name comment.) thanks, greg k-h
On Tue, 25 Jul 2023 07:33:57 +0200, Greg KH wrote: > > On Mon, Jul 24, 2023 at 07:33:56PM -0700, Wesley Cheng wrote: > > --- a/sound/usb/pcm.c > > +++ b/sound/usb/pcm.c > > @@ -87,7 +87,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream > > /* > > * find a matching audio format > > */ > > -static const struct audioformat * > > +const struct audioformat * > > find_format(struct list_head *fmt_list_head, snd_pcm_format_t format, > > unsigned int rate, unsigned int channels, bool strict_match, > > struct snd_usb_substream *subs) > > @@ -147,8 +147,9 @@ find_format(struct list_head *fmt_list_head, snd_pcm_format_t format, > > } > > return found; > > } > > +EXPORT_SYMBOL_GPL(find_format); > > This is a horrible symbol name for a global function, same for the other > ones in this file. If you really want to export them, please put them > in the proper "snd_" prefix namespace, or better yet, use a module > namespace as well to ensure that we know who is using them. Yes, please add snd_usb_ prefix for all exported symbols from USB-audio driver code. Also, make sure that EXPORT_SYMBOL_GPL() is used for those, too. thanks, Takashi
On Tue, 25 Jul 2023 04:33:55 +0200, Wesley Cheng wrote: > > Allow for different platforms to be notified on USB SND connect/disconnect > seqeunces. This allows for platform USB SND modules to properly initialize > and populate internal structures with references to the USB SND chip > device. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/usb/card.c | 36 ++++++++++++++++++++++++++++++++++++ > sound/usb/card.h | 20 ++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index f6e99ced8068..9365d1e17836 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -117,6 +117,30 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no) > static DEFINE_MUTEX(register_mutex); > static struct snd_usb_audio *usb_chip[SNDRV_CARDS]; > static struct usb_driver usb_audio_driver; > +static struct snd_usb_platform_ops *platform_ops; > + > +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) > +{ > + if (platform_ops) > + return -EEXIST; > + mutex_lock(®ister_mutex); > + platform_ops = ops; > + mutex_unlock(®ister_mutex); The check should be inside register_mutex lock, too. Otherwise it's racy. > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -207,4 +207,24 @@ struct snd_usb_stream { > struct list_head list; > }; > > +struct snd_usb_platform_ops { > + void (*connect_cb)(struct snd_usb_audio *chip); > + void (*disconnect_cb)(struct snd_usb_audio *chip); > + void (*suspend_cb)(struct usb_interface *intf, pm_message_t message); > +}; Don't we need resume_cb? Even if it's unused for your platform, it'd make sense to be a pair. > +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) > +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); > +int snd_usb_unregister_platform_ops(void); > +#else > +static int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) > +{ > + return -EOPNOTSUPP; > +} > + > +static int snd_usb_unregister_platform_ops(void) > +{ > + return -EOPNOTSUPP; > +} > +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ Any need for dummy function definitions? I thought those exported functions are always with CONFIG_SND_USB_AUDIO enabled? IOW, are they use of those functions without USB audio driver? thanks, Takashi
On Tue, 25 Jul 2023 04:34:03 +0200, 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 for when playback is > initiated by the ASoC USB backend path. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/usb/card.c | 28 ++++++++++++++++++++++++++++ > sound/usb/card.h | 8 ++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index a3fad66a3337..365f6d978608 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -142,6 +142,34 @@ int snd_usb_unregister_platform_ops(void) > } > EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); > > +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > + struct snd_pcm_hw_params *params, int direction) > +{ Please add a comment what this function does. > + struct snd_usb_audio *chip = usb_chip[card_idx]; Is the dereference safe without locking? If the call is supposed to be allowed only in a safe situation, it should be mentioned in the function description. thanks, Takashi
On Tue, 25 Jul 2023 04:34:02 +0200, Wesley Cheng wrote: > > --- a/sound/usb/Kconfig > +++ b/sound/usb/Kconfig > @@ -165,6 +165,21 @@ config SND_BCD2000 > To compile this driver as a module, choose M here: the module > will be called snd-bcd2000. > > +config QC_USB_AUDIO_OFFLOAD > + tristate "Qualcomm Audio Offload driver" > + depends on QCOM_QMI_HELPERS > + select SND_PCM So the driver can be enabled without CONFIG_SND_USB_AUDIO? It makes little sense without it. Or is it set so intentionally for testing purpose? About the code: > +/* Offloading IOMMU management */ > +static unsigned long uaudio_get_iova(unsigned long *curr_iova, > + size_t *curr_iova_size, struct list_head *head, size_t size) > +{ > + struct iova_info *info, *new_info = NULL; > + struct list_head *curr_head; > + unsigned long va = 0; > + size_t tmp_size = size; > + bool found = false; > + > + if (size % PAGE_SIZE) { > + dev_err(uaudio_qdev->dev, "size %zu is not page size multiple\n", > + size); > + goto done; This can be easily triggered by user-space as it's passed directly from the mmap call, and it implies that you can fill up the messages easily. It's safer to make it debug message or add the rate limit. Ditto for other error messages. > +static void disable_audio_stream(struct snd_usb_substream *subs) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + > + if (subs->data_endpoint || subs->sync_endpoint) { > + close_endpoints(chip, subs); > + > + mutex_lock(&chip->mutex); > + subs->cur_audiofmt = NULL; > + mutex_unlock(&chip->mutex); > + } Now looking at this and... > +static int enable_audio_stream(struct snd_usb_substream *subs, > + snd_pcm_format_t pcm_format, > + unsigned int channels, unsigned int cur_rate, > + int datainterval) > +{ ... this implementation, I wonder whether it'd be better to modify and export snd_usb_hw_params() snd snd_usb_hw_free() to fit with qcom driver. Then you can avoid lots of open code. In general, if you see a direct use of chip->mutex, it can be often done better in a different form. The use of an internal lock or such from an external driver is always fragile and error-prone. Also, the current open-code misses the potential race against the disconnection during the operation. In snd-usb-audio, it protects with snd_usb_lock_shutdown() and snd_usb_unlock_shutdown() pairs. > +static int __init qc_usb_audio_offload_init(void) > +{ > + struct uaudio_qmi_svc *svc; > + int ret; > + > + ret = snd_usb_register_platform_ops(&offload_ops); > + if (ret < 0) > + return ret; Registering the ops at the very first opens a potential access to the uninitialized stuff. Imagine a suspend happens right after this point. As the ops is already registered, it'll enter to the suspend_cb callback and straight to Oops. > +static void __exit qc_usb_audio_offload_exit(void) > +{ > + struct uaudio_qmi_svc *svc = uaudio_svc; > + > + qmi_handle_release(svc->uaudio_svc_hdl); > + flush_workqueue(svc->uaudio_wq); > + destroy_workqueue(svc->uaudio_wq); > + kfree(svc); > + uaudio_svc = NULL; > + snd_usb_unregister_platform_ops(); Similarly, the unregister order has to be careful, too. thanks, Takashi
> +/** > + * struct snd_soc_usb > + * @list - list head for SND SOC struct list > + * @dev - USB backend device reference > + * @component - reference to DAPM component ASoC component, not DAPM. > + * @connection_status_cb - callback to notify connection events > + * @priv_data - driver data > + **/ > +struct snd_soc_usb { > + struct list_head list; > + struct device *dev; > + struct snd_soc_component *component; > + int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx, > + int connected); It's not clear what 'connected' really refers to, is this a boolean really or is this a "connection_event? > + void *priv_data; > +}; > + > +int snd_soc_usb_connect(struct device *usbdev, int card_idx); > +int snd_soc_usb_disconnect(struct device *usbdev); > +void snd_soc_usb_set_priv_data(struct device *dev, void *priv); this function is not part of this patch, is this intentional to have a get but not a set? > +void *snd_soc_usb_get_priv_data(struct device *usbdev); you are using 'usbdev' and 'dev' for the same type of parameters, why not align on one set of definition with a consistent naming. > +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev) > +{ > + struct device_node *node; > + struct snd_soc_usb *ctx = NULL; this init doesn't seem required? > + > + node = snd_soc_find_phandle(dev); > + if (IS_ERR(node)) > + return NULL; > + > + mutex_lock(&ctx_mutex); > + list_for_each_entry(ctx, &usb_ctx_list, list) { > + if (ctx->dev->of_node == node) { > + of_node_put(node); > + mutex_unlock(&ctx_mutex); > + return ctx; > + } > + } > + of_node_put(node); > + mutex_unlock(&ctx_mutex); > + > + return NULL; > +} > + > +/** > + * snd_soc_usb_get_priv_data() - Retrieve private data stored > + * @dev: device reference > + * > + * Fetch the private data stored in the USB SND SOC structure. > + * > + */ > +void *snd_soc_usb_get_priv_data(struct device *dev) > +{ > + struct snd_soc_usb *ctx; > + > + ctx = snd_soc_find_usb_ctx(dev); so in this function you walk through the usb_ctx_list with locking... > + if (!ctx) { > + /* Check if backend device */ > + list_for_each_entry(ctx, &usb_ctx_list, list) { ... and here you walk again through the list without locking. Something's weird here, if this was intentional you need to add comments. > + if (dev->of_node == ctx->dev->of_node) > + goto out; > + } > + ctx = NULL; > + } > +out: > + return ctx ? ctx->priv_data : NULL; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data); > + > +/** > + * snd_soc_usb_add_port() - Add a USB backend port > + * @dev: USB backend device > + * @priv: private data > + * @connection_cb: connection status callback > + * > + * Register a USB backend device to the SND USB SOC framework. Memory is > + * allocated as part of the USB backend device. > + * > + */ > +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv, > + int (*connection_cb)(struct snd_soc_usb *usb, int card_idx, > + int connected)) > +{ > + struct snd_soc_usb *usb; > + > + usb = devm_kzalloc(dev, sizeof(*usb), GFP_KERNEL); > + if (!usb) > + return ERR_PTR(-ENOMEM); > + > + usb->connection_status_cb = connection_cb; > + usb->dev = dev; > + usb->priv_data = priv; back to my comment above, you don't seem to need the set_priv_data() ? > + > + mutex_lock(&ctx_mutex); > + list_add_tail(&usb->list, &usb_ctx_list); > + mutex_unlock(&ctx_mutex); > + > + return usb; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port); > +/** > + * snd_soc_usb_connect() - Notification of USB device connection > + * @usbdev: USB bus device > + * @card_idx: USB SND card instance > + * > + * Notify of a new USB SND device connection. The card_idx can be used to > + * handle how the USB backend selects, which device to enable offloading on. "USB backend" is confusing, not sure if this is the same concept as DPCM "backend" or something else. Please try to avoid overloaded terms. > + * > + */ > +int snd_soc_usb_connect(struct device *usbdev, int card_idx) > +{ > + struct snd_soc_usb *ctx; > + > + if (!usbdev) > + return -ENODEV; > + > + ctx = snd_soc_find_usb_ctx(usbdev); > + if (!ctx) > + return -ENODEV; > + > + if (ctx->connection_status_cb) > + ctx->connection_status_cb(ctx, card_idx, 1); so either the 'connected' value is 1... > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_connect); > + > +/** > + * snd_soc_usb_disconnect() - Notification of USB device disconnection > + * @usbdev: USB bus device > + * > + * Notify of a new USB SND device disconnection to the USB backend. > + * > + */ > +int snd_soc_usb_disconnect(struct device *usbdev) > +{ > + struct snd_soc_usb *ctx; > + > + if (!usbdev) > + return -ENODEV; > + > + ctx = snd_soc_find_usb_ctx(usbdev); > + if (!ctx) > + return -ENODEV; > + > + if (ctx->connection_status_cb) > + ctx->connection_status_cb(ctx, -1, 0); ...and here it's zero. should this 'connected' parameter be a boolean then with true/false value, or do you want to add enums/defines for more flexibility down the road? > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect);
> +static const struct snd_soc_dai_ops q6usb_ops = { > + .prepare = q6afe_dai_prepare, > + .hw_params = q6usb_hw_params, > + .shutdown = q6afe_dai_shutdown, it's a bit odd to see a .shutdown without a .startup? Is this intentional and should a comment be added? > +/* device token of actual end USB aduio device */ audio > + u32 dev_token; > +/* endianness of this interface */ > + u32 endian; > +/* service interval */ > + u32 service_interval; > +} __packed; > + > +/** > + * struct afe_param_id_usb_audio_dev_params > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + * @dev_token: device token of actual end USB aduio device audio. please run a spell-checker. > + svc_int.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; > + svc_int.svc_interval = pcfg->usb_cfg.service_interval; > + ret = q6afe_port_set_param_v2(port, &svc_int, > + AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL, > + AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(svc_int)); > + if (ret) { > + dev_err(port->afe->dev, "%s: AFE device param cmd svc_interval failed %d\n", > + __func__, ret); > + ret = -EINVAL; why do you override the return value? > + goto exit; not necessary, this is a jump to the next line. Looks like copy-paste ... > + } > +exit: > + return ret; > +} > + > +/** > + * q6afe_usb_port_prepare() - Prepare usb afe port. > + * > + * @port: Instance of afe port > + * @cfg: USB configuration for the afe port > + * > + */ > +void q6afe_usb_port_prepare(struct q6afe_port *port, > + struct q6afe_usb_cfg *cfg) > +{ > + union afe_port_config *pcfg = &port->port_cfg; > + > + pcfg->usb_cfg.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; > + pcfg->usb_cfg.sample_rate = cfg->sample_rate; > + pcfg->usb_cfg.num_channels = cfg->num_channels; > + pcfg->usb_cfg.bit_width = cfg->bit_width; > + > + afe_port_send_usb_dev_param(port, cfg); > +} > +EXPORT_SYMBOL_GPL(q6afe_usb_port_prepare); > + > /** > * q6afe_hdmi_port_prepare() - Prepare hdmi afe port. > * > @@ -1611,7 +1791,10 @@ struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id) > break; > case AFE_PORT_ID_WSA_CODEC_DMA_RX_0 ... AFE_PORT_ID_RX_CODEC_DMA_RX_7: > cfg_type = AFE_PARAM_ID_CODEC_DMA_CONFIG; > - break; > + break; > + case AFE_PORT_ID_USB_RX: > + cfg_type = AFE_PARAM_ID_USB_AUDIO_CONFIG; > + break; > default: > dev_err(dev, "Invalid port id 0x%x\n", port_id); > return ERR_PTR(-EINVAL); > diff --git a/sound/soc/qcom/qdsp6/q6afe.h b/sound/soc/qcom/qdsp6/q6afe.h > index 30fd77e2f458..e098a3e15135 100644 > --- a/sound/soc/qcom/qdsp6/q6afe.h > +++ b/sound/soc/qcom/qdsp6/q6afe.h > @@ -5,7 +5,7 @@ > > #include <dt-bindings/sound/qcom,q6afe.h> > > -#define AFE_PORT_MAX 129 > +#define AFE_PORT_MAX 130 > > #define MSM_AFE_PORT_TYPE_RX 0 > #define MSM_AFE_PORT_TYPE_TX 1 > @@ -205,6 +205,47 @@ struct q6afe_cdc_dma_cfg { > u16 active_channels_mask; > }; > > +/** > + * struct q6afe_usb_cfg > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + * @sample_rate: Sampling rate of the port > + * Supported values: > + * AFE_PORT_SAMPLE_RATE_8K > + * AFE_PORT_SAMPLE_RATE_11025 > + * AFE_PORT_SAMPLE_RATE_12K > + * AFE_PORT_SAMPLE_RATE_16K > + * AFE_PORT_SAMPLE_RATE_22050 > + * AFE_PORT_SAMPLE_RATE_24K > + * AFE_PORT_SAMPLE_RATE_32K > + * AFE_PORT_SAMPLE_RATE_44P1K > + * AFE_PORT_SAMPLE_RATE_48K > + * AFE_PORT_SAMPLE_RATE_96K > + * AFE_PORT_SAMPLE_RATE_192K > + * @bit_width: Bit width of the sample. > + * Supported values: 16, 24 > + * @num_channels: Number of channels > + * Supported values: 1, 2 > + * @data_format: Data format supported by the USB > + * Supported values: 0 > + * @reserved: this field must be 0 > + * @dev_token: device token of actual end USB audio device > + * @endian: endianness of this interface > + * @service_interval: service interval > + **/ > +struct q6afe_usb_cfg { > + u32 cfg_minor_version; > + u32 sample_rate; > + u16 bit_width; > + u16 num_channels; > + u16 data_format; > + u16 reserved; > + u32 dev_token; > + u32 endian; > + u32 service_interval; > +}; this definition looks exactly the same as struct afe_param_id_usb_cfg ??
> +struct q6usb_port_data { > + struct q6afe_usb_cfg usb_cfg; > + struct snd_soc_usb *usb; > + struct q6usb_offload priv; > + int active_idx; what is an 'active_idx' ? > +static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > + int connected) > +{ > + struct snd_soc_dapm_context *dapm; > + struct q6usb_port_data *data; > + > + dapm = snd_soc_component_get_dapm(usb->component); > + data = dev_get_drvdata(usb->component->dev); shouldn't you test that 'dapm' and 'data' are not NULL ? > + > + if (connected) { this goes back to my earlier comment that you treat 'connected' as a boolean. > + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); > + /* We only track the latest USB headset plugged in */ > + data->active_idx = card_idx; > + } else { > + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > + } > + snd_soc_dapm_sync(dapm); > + > + return 0; > +} > + > +static int q6usb_component_probe(struct snd_soc_component *component) > +{ > + struct q6usb_port_data *data = dev_get_drvdata(component->dev); > + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); > + > + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > + snd_soc_dapm_sync(dapm); > + > + data->usb = snd_soc_usb_add_port(component->dev, &data->priv, q6usb_alsa_connection_cb); > + if (IS_ERR(data->usb)) { > + dev_err(component->dev, "failed to add usb port\n"); > + return -ENODEV; > + } > + > + data->usb->component = component; > + > + return 0; > +} > + > +static const struct snd_soc_component_driver q6usb_dai_component = { > + .probe = q6usb_component_probe, erm, if you have a .probe that adds a port, don't you need a remove that removes the same port, and sets the pin state as well? > + .name = "q6usb-dai-component", > + .dapm_widgets = q6usb_dai_widgets, > + .num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets), > + .dapm_routes = q6usb_dapm_routes, > + .num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes), > + .of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name, > +}; > + > +static int q6usb_dai_dev_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + struct q6usb_port_data *data; > + struct device *dev = &pdev->dev; > + struct of_phandle_args args; > + int ret; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + ret = of_property_read_u32(node, "qcom,usb-audio-intr-num", > + &data->priv.intr_num); > + if (ret) { > + dev_err(&pdev->dev, "failed to read intr num.\n"); > + return ret; > + } > + > + ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args); > + if (ret < 0) > + data->priv.sid = -1; > + else > + data->priv.sid = args.args[0] & SID_MASK; > + > + data->priv.domain = iommu_get_domain_for_dev(&pdev->dev); > + > + data->priv.dev = dev; > + dev_set_drvdata(dev, data); > + > + ret = devm_snd_soc_register_component(dev, &q6usb_dai_component, > + q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais)); > + if (ret < 0) > + return ret; > + > + return 0; return devm_snd_soc_register_component > +} > + > +static int q6usb_dai_dev_remove(struct platform_device *pdev) > +{ > + snd_soc_usb_remove_port(&pdev->dev); that seems wrong, the port is added in the component probe, not the platform device probe. > + > + return 0; > +}> +
On 7/25/23 04:34, Wesley Cheng wrote: > Currently, only the index to the USB SND card array is passed to the USB > backend. Pass through more information, specifically the USB SND card > number and the number of PCM devices available. The USB backend should > know about which sound resources are being shared between the ASoC and USB > SND paths. This can be utilized to properly select and maintain the > offloading devices. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > include/sound/soc-usb.h | 9 +++++---- > sound/soc/qcom/qdsp6/q6usb.c | 20 ++++++++++++++++++-- > sound/soc/soc-usb.c | 12 +++++++----- > sound/usb/qcom/qc_audio_offload.c | 9 +++++---- > 4 files changed, 35 insertions(+), 15 deletions(-) > > diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h > index 71e6e75e600a..606128332044 100644 > --- a/include/sound/soc-usb.h > +++ b/include/sound/soc-usb.h > @@ -19,20 +19,21 @@ struct snd_soc_usb { > struct device *dev; > struct snd_soc_component *component; > int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx, > - int connected); > + int chip_idx, int num_pcm, int connected); I don't know what 'chip_idx' is. The 'num_pcm' sounds problematic if there are different devices for playback and capture. I would guess this is for playback only, but this doesn't scale. > void *priv_data; > }; > +struct q6usb_status { > + unsigned int num_pcm; > + unsigned int chip_index; > + unsigned int pcm_index; > +}; > + > struct q6usb_port_data { > struct q6afe_usb_cfg usb_cfg; > struct snd_soc_usb *usb; > struct q6usb_offload priv; > + unsigned long available_card_slot; what is a card slot? > + struct q6usb_status status[SNDRV_CARDS]; > int active_idx; > }; > > @@ -97,7 +105,7 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone > } > > static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > - int connected) > + int chip_idx, int num_pcm, int connected) > { > struct snd_soc_dapm_context *dapm; > struct q6usb_port_data *data; > @@ -109,8 +117,16 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); > /* We only track the latest USB headset plugged in */ > data->active_idx = card_idx; > + > + set_bit(card_idx, &data->available_card_slot); > + data->status[card_idx].num_pcm = num_pcm; > + data->status[card_idx].chip_index = chip_idx; > } else { > - snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > + clear_bit(card_idx, &data->available_card_slot); > + data->status[card_idx].num_pcm = 0; > + data->status[card_idx].chip_index = 0; > + if (!data->available_card_slot) > + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); not able to follow what this does, this patch is rather unclear and lacks comments. > } > snd_soc_dapm_sync(dapm);
On 7/25/23 04:34, Wesley Cheng wrote: > The headphone jack framework has a well defined intrastructure for infrastructure > notifying userspace entities through input devices. Expose a jack device > that carries information about if an offload capable device is connected. > Applications can further identify specific offloading information through > other SND kcontrols. What if you connect a set of USB speakers? Would they show as a headphone/headset? > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/soc/qcom/qdsp6/q6usb.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c > index e4ccb9d912b0..860dff8c1438 100644 > --- a/sound/soc/qcom/qdsp6/q6usb.c > +++ b/sound/soc/qcom/qdsp6/q6usb.c > @@ -20,6 +20,7 @@ > #include <sound/pcm_params.h> > #include <sound/asound.h> > #include <sound/q6usboffload.h> > +#include <sound/jack.h> > > #include "q6dsp-lpass-ports.h" > #include "q6afe.h" > @@ -37,6 +38,7 @@ struct q6usb_status { > struct q6usb_port_data { > struct q6afe_usb_cfg usb_cfg; > struct snd_soc_usb *usb; > + struct snd_soc_jack hs_jack; > struct q6usb_offload priv; > struct mutex mutex; > unsigned long available_card_slot; > @@ -279,6 +281,7 @@ static const struct snd_kcontrol_new q6usb_offload_control = { > /* Build a mixer control for a UAC connector control (jack-detect) */ > static void q6usb_connector_control_init(struct snd_soc_component *component) > { > + struct q6usb_port_data *data = dev_get_drvdata(component->dev); > int ret; > > ret = snd_ctl_add(component->card->snd_card, > @@ -290,6 +293,11 @@ static void q6usb_connector_control_init(struct snd_soc_component *component) > snd_ctl_new1(&q6usb_offload_dev_ctrl, component)); > if (ret < 0) > return; > + > + ret = snd_soc_card_jack_new(component->card, "USB offload", > + SND_JACK_HEADSET, &data->hs_jack); not all USB devices are headsets... > + if (ret) > + return; > } > > static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component, > @@ -322,7 +330,10 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > > mutex_lock(&data->mutex); > if (connected) { > - snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); > + if (!data->available_card_slot) { > + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); > + snd_jack_report(data->hs_jack.jack, 1); > + } > /* > * Update the latest USB headset plugged in, if session is > * idle. > @@ -338,8 +349,10 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > clear_bit(card_idx, &data->available_card_slot); > data->status[card_idx].num_pcm = 0; > data->status[card_idx].chip_index = 0; > - if (!data->available_card_slot) > + if (!data->available_card_slot) { > snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > + snd_jack_report(data->hs_jack.jack, 0); > + } > } > snd_soc_dapm_sync(dapm); > mutex_unlock(&data->mutex);
On 7/25/23 04:34, Wesley Cheng wrote: > In case of notifying SND platform drivers of connection events, some of > these use cases, such as offloading, require an ASoC USB backend device to > be initialized before the events can be handled. If the USB backend device > has not yet been probed, this leads to missing initial USB audio device > connection events. > > Expose an API that traverses the usb_chip array for connected devices, and > to call the respective connection callback registered to the SND platform > driver. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/usb/card.c | 19 +++++++++++++++++++ > sound/usb/card.h | 2 ++ > 2 files changed, 21 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 365f6d978608..27a89aaa0bf3 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > } > EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > > +/* > + * in case the platform driver was not ready at the time of USB SND > + * device connect, expose an API to discover all connected USB devices > + * so it can populate any dependent resources/structures. > + */ > +void snd_usb_rediscover_devices(void) > +{ > + int i; > + > + mutex_lock(®ister_mutex); > + for (i = 0; i < SNDRV_CARDS; i++) { > + if (usb_chip[i]) > + if (platform_ops && platform_ops->connect_cb) > + platform_ops->connect_cb(usb_chip[i]); what happens if the USB device is removed while the platform device adds a port? This sounds super-racy to me. It's the same set of problems we're having between audio and display/DRM, I would be surprised if this function dealt with all corner cases of insertion/removal, bind/unbind. > + } > + mutex_unlock(®ister_mutex); > +} > +EXPORT_SYMBOL_GPL(snd_usb_rediscover_devices); > + > /* > * disconnect streams > * called from usb_audio_disconnect() > diff --git a/sound/usb/card.h b/sound/usb/card.h > index db735680d155..dfd241334e3d 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -219,6 +219,7 @@ int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); > int snd_usb_unregister_platform_ops(void); > struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > struct snd_pcm_hw_params *params, int direction); > +void snd_usb_rediscover_devices(void); > #else > static int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) > { > @@ -235,5 +236,6 @@ static struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > { > return NULL; > } > +void snd_usb_rediscover_devices(void) { } > #endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ > #endif /* __USBAUDIO_CARD_H */
On 7/25/23 04:34, Wesley Cheng wrote: > Expose a kcontrol on the platform sound card, which will allow for > userspace 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 'Q6USB offload SND device select' 1 0 > > The above will set the Q6AFE device token to choose offload on card#1 and > pcm#0. Device selection is made possible by setting the Q6AFE device > token. The audio DSP utilizes this parameter, and will pass this field > back to the USB offload driver within the QMI stream requests. I must be missing something... If you have a card 0 which exposes a control to change what the card1 does, then it means you have a card0 with a PCM device what can potentially be used concurrently with the card1 exposing an offload device. Is there any sort of mutual exclusion to make sure the same USB endpoint is not used twice? One would hope that when a device is opened the matching non-offloaded one (or ones in the case of implicit feedback) is disabled or marked as used? I would guess in your Android setup you have control on such behavior at the HAL level, but in the more generic Linux use I don't see what would orchestrate the use of two devices, and at the kernel level what would prevent it. > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/soc/qcom/qdsp6/q6usb.c | 126 ++++++++++++++++++++++++++++++++++- > 1 file changed, 123 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c > index 2fb6159bd2a8..87e88dafd8ad 100644 > --- a/sound/soc/qcom/qdsp6/q6usb.c > +++ b/sound/soc/qcom/qdsp6/q6usb.c > @@ -14,6 +14,7 @@ > #include <linux/dma-map-ops.h> > > #include <sound/pcm.h> > +#include <sound/control.h> > #include <sound/soc.h> > #include <sound/soc-usb.h> > #include <sound/pcm_params.h> > @@ -35,9 +36,12 @@ struct q6usb_port_data { > struct q6afe_usb_cfg usb_cfg; > struct snd_soc_usb *usb; > struct q6usb_offload priv; > + struct mutex mutex; > unsigned long available_card_slot; > struct q6usb_status status[SNDRV_CARDS]; > - int active_idx; > + bool idx_valid; > + int sel_card_idx; > + int sel_pcm_idx; > }; > > static const struct snd_soc_dapm_widget q6usb_dai_widgets[] = { > @@ -53,10 +57,36 @@ static int q6usb_hw_params(struct snd_pcm_substream *substream, > 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 = asoc_rtd_to_cpu(rtd, 0); > + struct q6afe_port *q6usb_afe; > int direction = substream->stream; > + int chip_idx; > + int ret; > + > + mutex_lock(&data->mutex); > + chip_idx = data->status[data->sel_card_idx].chip_index; > + > + ret = snd_soc_usb_find_format(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; > + > + ret = afe_port_send_usb_dev_param(q6usb_afe, data->sel_card_idx, > + data->sel_pcm_idx); > + if (ret < 0) > + goto out; > + > + data->status[data->sel_card_idx].pcm_index = data->sel_pcm_idx; > +out: > + mutex_unlock(&data->mutex); > > - return snd_soc_usb_find_format(data->active_idx, params, direction); > + return ret; > } > + > static const struct snd_soc_dai_ops q6usb_ops = { > .hw_params = q6usb_hw_params, > }; > @@ -85,6 +115,89 @@ static struct snd_soc_dai_driver q6usb_be_dais[] = { > }, > }; > > +static int q6usb_get_offload_dev(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); > + struct q6usb_port_data *data = dev_get_drvdata(component->dev); > + int pcm_idx; > + int card_idx; > + > + mutex_lock(&data->mutex); > + if (!data->idx_valid) { > + card_idx = -1; > + pcm_idx = -1; > + } else { > + card_idx = data->sel_card_idx; > + pcm_idx = data->sel_pcm_idx; > + } > + > + ucontrol->value.integer.value[0] = card_idx; > + ucontrol->value.integer.value[1] = pcm_idx; > + mutex_unlock(&data->mutex); > + > + return 0; > +} > + > +static int q6usb_put_offload_dev(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); > + struct q6usb_port_data *data = dev_get_drvdata(component->dev); > + int changed = 0; > + int pcmidx; > + int cardidx; > + > + cardidx = ucontrol->value.integer.value[0]; > + pcmidx = ucontrol->value.integer.value[1]; > + > + mutex_lock(&data->mutex); > + if ((cardidx >= 0 && test_bit(cardidx, &data->available_card_slot))) { > + data->sel_card_idx = cardidx; > + changed = 1; > + } > + > + if ((pcmidx >= 0 && pcmidx < data->status[cardidx].num_pcm)) { > + data->sel_pcm_idx = pcmidx; > + data->idx_valid = true; > + changed = 1; > + } > + mutex_unlock(&data->mutex); > + > + return changed; > +} > + > +static int q6usb_offload_dev_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; > + uinfo->value.integer.max = SNDRV_CARDS; > + > + return 0; > +} > + > +static const struct snd_kcontrol_new q6usb_offload_dev_ctrl = { > + .iface = SNDRV_CTL_ELEM_IFACE_CARD, > + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE, > + .name = "Q6USB offload SND device select", > + .info = q6usb_offload_dev_info, > + .get = q6usb_get_offload_dev, > + .put = q6usb_put_offload_dev, > +}; > + > +/* Build a mixer control for a UAC connector control (jack-detect) */ > +static void q6usb_connector_control_init(struct snd_soc_component *component) > +{ > + int ret; > + > + ret = snd_ctl_add(component->card->snd_card, > + snd_ctl_new1(&q6usb_offload_dev_ctrl, component)); > + if (ret < 0) > + return; > +} > + > static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component, > const struct of_phandle_args *args, > const char **dai_name) > @@ -113,10 +226,12 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > dapm = snd_soc_component_get_dapm(usb->component); > data = dev_get_drvdata(usb->component->dev); > > + mutex_lock(&data->mutex); > if (connected) { > snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); > /* We only track the latest USB headset plugged in */ > - data->active_idx = card_idx; > + if (!data->idx_valid || data->sel_card_idx < 0) > + data->sel_card_idx = card_idx; > > set_bit(card_idx, &data->available_card_slot); > data->status[card_idx].num_pcm = num_pcm; > @@ -129,6 +244,7 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, > snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > } > snd_soc_dapm_sync(dapm); > + mutex_unlock(&data->mutex); > > return 0; > } > @@ -141,6 +257,8 @@ static int q6usb_component_probe(struct snd_soc_component *component) > snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > snd_soc_dapm_sync(dapm); > > + q6usb_connector_control_init(component); > + > data->usb = snd_soc_usb_add_port(component->dev, &data->priv, q6usb_alsa_connection_cb); > if (IS_ERR(data->usb)) { > dev_err(component->dev, "failed to add usb port\n"); > @@ -189,6 +307,8 @@ static int q6usb_dai_dev_probe(struct platform_device *pdev) > > data->priv.domain = iommu_get_domain_for_dev(&pdev->dev); > > + mutex_init(&data->mutex); > + > data->priv.dev = dev; > dev_set_drvdata(dev, data); >
On Tue, 25 Jul 2023 11:15:11 +0200, Pierre-Louis Bossart wrote: > > > > On 7/25/23 04:34, Wesley Cheng wrote: > > In case of notifying SND platform drivers of connection events, some of > > these use cases, such as offloading, require an ASoC USB backend device to > > be initialized before the events can be handled. If the USB backend device > > has not yet been probed, this leads to missing initial USB audio device > > connection events. > > > > Expose an API that traverses the usb_chip array for connected devices, and > > to call the respective connection callback registered to the SND platform > > driver. > > > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > --- > > sound/usb/card.c | 19 +++++++++++++++++++ > > sound/usb/card.h | 2 ++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/sound/usb/card.c b/sound/usb/card.c > > index 365f6d978608..27a89aaa0bf3 100644 > > --- a/sound/usb/card.c > > +++ b/sound/usb/card.c > > @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > > } > > EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > > > > +/* > > + * in case the platform driver was not ready at the time of USB SND > > + * device connect, expose an API to discover all connected USB devices > > + * so it can populate any dependent resources/structures. > > + */ > > +void snd_usb_rediscover_devices(void) > > +{ > > + int i; > > + > > + mutex_lock(®ister_mutex); > > + for (i = 0; i < SNDRV_CARDS; i++) { > > + if (usb_chip[i]) > > + if (platform_ops && platform_ops->connect_cb) > > + platform_ops->connect_cb(usb_chip[i]); > > what happens if the USB device is removed while the platform device adds > a port? That should be protected by the register_mutex. But there can be other races (see below :) > This sounds super-racy to me. It's the same set of problems we're having > between audio and display/DRM, I would be surprised if this function > dealt with all corner cases of insertion/removal, bind/unbind. Yes, we need to be more careful about binding. For example, in the current patch set, I see no way to prevent unloading snd-usb-audio-qmi module, and it allows user to cut off the stuff during operation, which may break things while the kernel is running the code of the unloaded module. You need to have a proper module refcount management for avoiding such a scenario. Most of drivers don't need it because ALSA core part already takes care of it. But in this case, it requires a manual adjustment. Takashi
On 25.07.23 04:33, Wesley Cheng wrote: Hi, this is more of a stylistic remark, but this: [..] > + /* Find available secondary interrupter, interrupter 0 is reserverd for primary */ > + for (i = 1; i < xhci->max_interrupters; i++) { > + if (xhci->interrupters[i] == NULL) { > + err = xhci_add_interrupter(xhci, ir, i); > + break; > + } > + } > + > + spin_unlock_irq(&xhci->lock); > + > + if (!err) { Making the non error case a conditional return is ... > + xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters %d\n", > + i, xhci->max_interrupters); > + return ir; > + } > + > + xhci_warn(xhci, "Failed to add secondary interrupter, max interrupters %d\n", > + xhci->max_interrupters); > + > + xhci_free_interrupter(xhci, ir); > + > + return NULL; > +} Was this taken out of a style guide that teaches to avoid "goto" at any cost? Regards Oliver
Hi Takashi, On 7/25/2023 12:26 AM, Takashi Iwai wrote: > On Tue, 25 Jul 2023 04:34:02 +0200, > Wesley Cheng wrote: >> >> --- a/sound/usb/Kconfig >> +++ b/sound/usb/Kconfig >> @@ -165,6 +165,21 @@ config SND_BCD2000 >> To compile this driver as a module, choose M here: the module >> will be called snd-bcd2000. >> >> +config QC_USB_AUDIO_OFFLOAD >> + tristate "Qualcomm Audio Offload driver" >> + depends on QCOM_QMI_HELPERS >> + select SND_PCM > > So the driver can be enabled without CONFIG_SND_USB_AUDIO? It makes > little sense without it. > Or is it set so intentionally for testing purpose? > Thanks for the review. I'll change this to be dependent on CONFIG_SND_USB_AUDIO...it shouldn't exist in the end use case w/o it. > About the code: > >> +/* Offloading IOMMU management */ >> +static unsigned long uaudio_get_iova(unsigned long *curr_iova, >> + size_t *curr_iova_size, struct list_head *head, size_t size) >> +{ >> + struct iova_info *info, *new_info = NULL; >> + struct list_head *curr_head; >> + unsigned long va = 0; >> + size_t tmp_size = size; >> + bool found = false; >> + >> + if (size % PAGE_SIZE) { >> + dev_err(uaudio_qdev->dev, "size %zu is not page size multiple\n", >> + size); >> + goto done; > > This can be easily triggered by user-space as it's passed directly > from the mmap call, and it implies that you can fill up the messages > easily. It's safer to make it debug message or add the rate limit. > > Ditto for other error messages. > Got it, I'll make sure to address the above dev_err(). >> +static void disable_audio_stream(struct snd_usb_substream *subs) >> +{ >> + struct snd_usb_audio *chip = subs->stream->chip; >> + >> + if (subs->data_endpoint || subs->sync_endpoint) { >> + close_endpoints(chip, subs); >> + >> + mutex_lock(&chip->mutex); >> + subs->cur_audiofmt = NULL; >> + mutex_unlock(&chip->mutex); >> + } > > Now looking at this and... > >> +static int enable_audio_stream(struct snd_usb_substream *subs, >> + snd_pcm_format_t pcm_format, >> + unsigned int channels, unsigned int cur_rate, >> + int datainterval) >> +{ > > ... this implementation, I wonder whether it'd be better to modify and > export snd_usb_hw_params() snd snd_usb_hw_free() to fit with qcom > driver. Then you can avoid lots of open code. > I think the problem is that snd_usb_hw_params assumes that we've already done a PCM open on the PCM device created by USB SND. However, with the offload path, we don't reference the USB PCM device, but the one created by the platform sound card. Hence, I don't have access to the snd_pcm_substream. I attempted to derive snd_pcm_substream from snd_usb_substream, but since PCM open isn't run, it doesn't provide a valid structure. What do you think about adding a wrapper to snd_usb_hw_params? Have a version that will take in snd_usb_substream, and another that is registered to hw_params(). > In general, if you see a direct use of chip->mutex, it can be often > done better in a different form. The use of an internal lock or such > from an external driver is always fragile and error-prone. > > Also, the current open-code misses the potential race against the > disconnection during the operation. In snd-usb-audio, it protects > with snd_usb_lock_shutdown() and snd_usb_unlock_shutdown() pairs. > I agree...I think then the best approach would be something like the above, ie: int snd_usb_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *hw_params) { struct snd_usb_substream *subs = substream->runtime->private_data; snd_usb_ep_attach(subs, hw_params); ... int snd_usb_ep_attach(...) { //implementation of current code in snd_usb_hw_params() } EXPORT_SYMBOL(snd_usb_ep_attach); >> +static int __init qc_usb_audio_offload_init(void) >> +{ >> + struct uaudio_qmi_svc *svc; >> + int ret; >> + >> + ret = snd_usb_register_platform_ops(&offload_ops); >> + if (ret < 0) >> + return ret; > > Registering the ops at the very first opens a potential access to the > uninitialized stuff. Imagine a suspend happens right after this > point. As the ops is already registered, it'll enter to the > suspend_cb callback and straight to Oops. > >> +static void __exit qc_usb_audio_offload_exit(void) >> +{ >> + struct uaudio_qmi_svc *svc = uaudio_svc; >> + >> + qmi_handle_release(svc->uaudio_svc_hdl); >> + flush_workqueue(svc->uaudio_wq); >> + destroy_workqueue(svc->uaudio_wq); >> + kfree(svc); >> + uaudio_svc = NULL; >> + snd_usb_unregister_platform_ops(); > > Similarly, the unregister order has to be careful, too. > Let me re-organize it a bit more. Thanks Wesley Cheng
Hi Pierre, On 7/25/2023 1:10 AM, Pierre-Louis Bossart wrote: > >> +/** >> + * struct snd_soc_usb >> + * @list - list head for SND SOC struct list >> + * @dev - USB backend device reference >> + * @component - reference to DAPM component > > ASoC component, not DAPM. > Thanks for the detailed review. Will fix this. >> + * @connection_status_cb - callback to notify connection events >> + * @priv_data - driver data >> + **/ >> +struct snd_soc_usb { >> + struct list_head list; >> + struct device *dev; >> + struct snd_soc_component *component; >> + int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx, >> + int connected); > > It's not clear what 'connected' really refers to, is this a boolean > really or is this a "connection_event? > > I'll change it to boolean for now, since its currently only connected/disconnected. If more states are required, then we can add that in the future. >> + void *priv_data; >> +}; >> + >> +int snd_soc_usb_connect(struct device *usbdev, int card_idx); >> +int snd_soc_usb_disconnect(struct device *usbdev); >> +void snd_soc_usb_set_priv_data(struct device *dev, void *priv); > > this function is not part of this patch, is this intentional to have a > get but not a set? > >> +void *snd_soc_usb_get_priv_data(struct device *usbdev); > > you are using 'usbdev' and 'dev' for the same type of parameters, why > not align on one set of definition with a consistent naming. > > I'll take a look at this and see what happened. I think Greg mentioned something similar and I made the change to remove the set priv, and moved it elsewhere. >> +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev) >> +{ >> + struct device_node *node; >> + struct snd_soc_usb *ctx = NULL; > > this init doesn't seem required? > Yes, not needed. >> + >> + node = snd_soc_find_phandle(dev); >> + if (IS_ERR(node)) >> + return NULL; >> + >> + mutex_lock(&ctx_mutex); >> + list_for_each_entry(ctx, &usb_ctx_list, list) { >> + if (ctx->dev->of_node == node) { >> + of_node_put(node); >> + mutex_unlock(&ctx_mutex); >> + return ctx; >> + } >> + } >> + of_node_put(node); >> + mutex_unlock(&ctx_mutex); >> + >> + return NULL; >> +} >> + >> +/** >> + * snd_soc_usb_get_priv_data() - Retrieve private data stored >> + * @dev: device reference >> + * >> + * Fetch the private data stored in the USB SND SOC structure. >> + * >> + */ >> +void *snd_soc_usb_get_priv_data(struct device *dev) >> +{ >> + struct snd_soc_usb *ctx; >> + >> + ctx = snd_soc_find_usb_ctx(dev); > > so in this function you walk through the usb_ctx_list with locking... > >> + if (!ctx) { >> + /* Check if backend device */ >> + list_for_each_entry(ctx, &usb_ctx_list, list) { > > ... and here you walk again through the list without locking. > > Something's weird here, if this was intentional you need to add comments. > Yes, needs a lock here. >> + if (dev->of_node == ctx->dev->of_node) >> + goto out; >> + } >> + ctx = NULL; >> + } >> +out: >> + return ctx ? ctx->priv_data : NULL; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data); >> + >> +/** >> + * snd_soc_usb_add_port() - Add a USB backend port >> + * @dev: USB backend device >> + * @priv: private data >> + * @connection_cb: connection status callback >> + * >> + * Register a USB backend device to the SND USB SOC framework. Memory is >> + * allocated as part of the USB backend device. >> + * >> + */ >> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv, >> + int (*connection_cb)(struct snd_soc_usb *usb, int card_idx, >> + int connected)) >> +{ >> + struct snd_soc_usb *usb; >> + >> + usb = devm_kzalloc(dev, sizeof(*usb), GFP_KERNEL); >> + if (!usb) >> + return ERR_PTR(-ENOMEM); >> + >> + usb->connection_status_cb = connection_cb; >> + usb->dev = dev; >> + usb->priv_data = priv; > > back to my comment above, you don't seem to need the set_priv_data() ? > And yes, this is where I moved the priv data setting. >> + >> + mutex_lock(&ctx_mutex); >> + list_add_tail(&usb->list, &usb_ctx_list); >> + mutex_unlock(&ctx_mutex); >> + >> + return usb; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port); > >> +/** >> + * snd_soc_usb_connect() - Notification of USB device connection >> + * @usbdev: USB bus device >> + * @card_idx: USB SND card instance >> + * >> + * Notify of a new USB SND device connection. The card_idx can be used to >> + * handle how the USB backend selects, which device to enable offloading on. > > "USB backend" is confusing, not sure if this is the same concept as DPCM > "backend" or something else. Please try to avoid overloaded terms. > Sure, I meant DPCM backend. >> + * >> + */ >> +int snd_soc_usb_connect(struct device *usbdev, int card_idx) >> +{ >> + struct snd_soc_usb *ctx; >> + >> + if (!usbdev) >> + return -ENODEV; >> + >> + ctx = snd_soc_find_usb_ctx(usbdev); >> + if (!ctx) >> + return -ENODEV; >> + >> + if (ctx->connection_status_cb) >> + ctx->connection_status_cb(ctx, card_idx, 1); > > so either the 'connected' value is 1... >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect); >> + >> +/** >> + * snd_soc_usb_disconnect() - Notification of USB device disconnection >> + * @usbdev: USB bus device >> + * >> + * Notify of a new USB SND device disconnection to the USB backend. >> + * >> + */ >> +int snd_soc_usb_disconnect(struct device *usbdev) >> +{ >> + struct snd_soc_usb *ctx; >> + >> + if (!usbdev) >> + return -ENODEV; >> + >> + ctx = snd_soc_find_usb_ctx(usbdev); >> + if (!ctx) >> + return -ENODEV; >> + >> + if (ctx->connection_status_cb) >> + ctx->connection_status_cb(ctx, -1, 0); > > ...and here it's zero. > > should this 'connected' parameter be a boolean then with true/false > value, or do you want to add enums/defines for more flexibility down the > road? > As mentioned above, will switch to boolean for now. I don't see a need to have other enums. Thanks Wesley Cheng
On Wed, 26 Jul 2023 00:59:57 +0200, Wesley Cheng wrote: > > >> +static int enable_audio_stream(struct snd_usb_substream *subs, > >> + snd_pcm_format_t pcm_format, > >> + unsigned int channels, unsigned int cur_rate, > >> + int datainterval) > >> +{ > > > > ... this implementation, I wonder whether it'd be better to modify and > > export snd_usb_hw_params() snd snd_usb_hw_free() to fit with qcom > > driver. Then you can avoid lots of open code. > > > > I think the problem is that snd_usb_hw_params assumes that we've > already done a PCM open on the PCM device created by USB SND. > However, with the offload path, we don't reference the USB PCM device, > but the one created by the platform sound card. Hence, I don't have > access to the snd_pcm_substream. > > I attempted to derive snd_pcm_substream from snd_usb_substream, but > since PCM open isn't run, it doesn't provide a valid structure. > > What do you think about adding a wrapper to snd_usb_hw_params? Have a > version that will take in snd_usb_substream, and another that is > registered to hw_params(). Yes, that's what I had in mind, too. > > In general, if you see a direct use of chip->mutex, it can be often > > done better in a different form. The use of an internal lock or such > > from an external driver is always fragile and error-prone. > > > > Also, the current open-code misses the potential race against the > > disconnection during the operation. In snd-usb-audio, it protects > > with snd_usb_lock_shutdown() and snd_usb_unlock_shutdown() pairs. > > > > I agree...I think then the best approach would be something like the > above, ie: > > int snd_usb_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *hw_params) > { > struct snd_usb_substream *subs = substream->runtime->private_data; > > snd_usb_ep_attach(subs, hw_params); > ... > > int snd_usb_ep_attach(...) > { > //implementation of current code in snd_usb_hw_params() > } > EXPORT_SYMBOL(snd_usb_ep_attach); Yes, exactly something like that ;) thanks, Takashi
Hi Pierre, On 7/25/2023 2:16 AM, Pierre-Louis Bossart wrote: > > > On 7/25/23 04:34, Wesley Cheng wrote: >> Expose a kcontrol on the platform sound card, which will allow for >> userspace 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 'Q6USB offload SND device select' 1 0 >> >> The above will set the Q6AFE device token to choose offload on card#1 and >> pcm#0. Device selection is made possible by setting the Q6AFE device >> token. The audio DSP utilizes this parameter, and will pass this field >> back to the USB offload driver within the QMI stream requests. > > I must be missing something... If you have a card 0 which exposes a > control to change what the card1 does, then it means you have a card0 > with a PCM device what can potentially be used concurrently with the > card1 exposing an offload device. > > Is there any sort of mutual exclusion to make sure the same USB endpoint > is not used twice? > > One would hope that when a device is opened the matching non-offloaded > one (or ones in the case of implicit feedback) is disabled or marked as > used? > > I would guess in your Android setup you have control on such behavior at > the HAL level, but in the more generic Linux use I don't see what > would orchestrate the use of two devices, and at the kernel level what > would prevent it. > Still going through the comments and trying to address the suggestions in the code, so will reply pack to those as I make the needed changes. As for the above question, the following change was made with the intention to prevent the above scenario. https://lore.kernel.org/linux-usb/20230725023416.11205-23-quic_wcheng@quicinc.com/ Thanks Wesley Cheng
On 25.7.2023 5.33, Wesley Cheng wrote: > As part of xHCI bus suspend, the XHCI is halted. However, if there are > pending events in the secondary event ring, it is observed that the xHCI > controller stops responding to further commands upon host or device > initiated bus resume. Iterate through all pending events and updating the > dequeue pointer to the last pending event trb. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > drivers/usb/host/xhci-mem.c | 74 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 69 insertions(+), 5 deletions(-) This sounds more like ring handling code. Maybe xhci-ring.c would be a better place > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index c51150af22f2..6b01d56c176f 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -1799,17 +1799,85 @@ int xhci_alloc_erst(struct xhci_hcd *xhci, > return 0; > } > > +static void xhci_handle_sec_intr_events(struct xhci_hcd *xhci, > + struct xhci_ring *ring, struct xhci_intr_reg __iomem *ir_set, > + struct xhci_erst *erst) > +{ The function name is a bit misleading as it doesn't handle any of the pending events, it just marks them all handled. > + union xhci_trb *erdp_trb, *current_trb; > + struct xhci_segment *seg; > + u64 erdp_reg; > + u32 iman_reg; > + dma_addr_t deq; > + unsigned long segment_offset; > + > + /* disable irq, ack pending interrupt and ack all pending events */ > + iman_reg = readl_relaxed(&ir_set->irq_pending); > + iman_reg &= ~IMAN_IE; > + writel_relaxed(iman_reg, &ir_set->irq_pending); > + iman_reg = readl_relaxed(&ir_set->irq_pending); > + if (iman_reg & IMAN_IP) > + writel_relaxed(iman_reg, &ir_set->irq_pending); maybe use xhci_disable_interrupter() helper, it does most of this already. > + > + /* last acked event trb is in erdp reg */ > + erdp_reg = xhci_read_64(xhci, &ir_set->erst_dequeue); > + deq = (dma_addr_t)(erdp_reg & ~ERST_PTR_MASK); > + if (!deq) { > + xhci_dbg(xhci, "event ring handling not required\n"); > + return; > + } > + > + seg = ring->first_seg; > + segment_offset = deq - seg->dma; > + > + /* find out virtual address of the last acked event trb */ > + erdp_trb = current_trb = &seg->trbs[0] + > + (segment_offset/sizeof(*current_trb)); > + > + /* read cycle state of the last acked trb to find out CCS */ > + ring->cycle_state = le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE; > + > + while (1) { > + /* last trb of the event ring: toggle cycle state */ > + if (current_trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) { > + ring->cycle_state ^= 1; > + current_trb = &seg->trbs[0]; > + } else { > + current_trb++; > + } > + > + /* cycle state transition */ > + if ((le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE) != > + ring->cycle_state) > + break; > + } > + > + if (erdp_trb != current_trb) { > + deq = xhci_trb_virt_to_dma(ring->deq_seg, current_trb); > + if (deq == 0) > + xhci_warn(xhci, > + "WARN invalid SW event ring dequeue ptr.\n"); > + /* Update HC event ring dequeue pointer */ > + erdp_reg &= ERST_PTR_MASK; > + erdp_reg |= ((u64) deq & (u64) ~ERST_PTR_MASK); > + } > + > + /* Clear the event handler busy flag (RW1C); event ring is empty. */ > + erdp_reg |= ERST_EHB; > + xhci_write_64(xhci, erdp_reg, &ir_set->erst_dequeue); There are some helpers like inc_deq() and xhci_update_erst_dequeue() that could be used here. > +} > + > static void > xhci_free_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir) > { > struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > size_t erst_size; > - u64 tmp64; > u32 tmp; > > if (!ir) > return; > > + xhci_handle_sec_intr_events(xhci, ir->event_ring, ir->ir_set, &ir->erst); Cleaning up the interrupter event ring should be called earlier. Probably from xhci_remove_secondary_interrupter(), before it calls xhci_free_interrupter() Thanks -Mathias
Hi Pierre, On 7/25/2023 1:27 AM, Pierre-Louis Bossart wrote: > >> +static const struct snd_soc_dai_ops q6usb_ops = { >> + .prepare = q6afe_dai_prepare, >> + .hw_params = q6usb_hw_params, >> + .shutdown = q6afe_dai_shutdown, > > it's a bit odd to see a .shutdown without a .startup? > > Is this intentional and should a comment be added? > > That is correct, because the Q6AFE port start command needs to also pass in the PCM params, so we wait for the hw_params() callback before actually starting the port. I will add a comment. >> +/* device token of actual end USB aduio device */ > > audio > >> + u32 dev_token; >> +/* endianness of this interface */ >> + u32 endian; >> +/* service interval */ >> + u32 service_interval; >> +} __packed; >> + >> +/** >> + * struct afe_param_id_usb_audio_dev_params >> + * @cfg_minor_version: Minor version used for tracking USB audio device >> + * configuration. >> + * Supported values: >> + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG >> + * @dev_token: device token of actual end USB aduio device > > audio. please run a spell-checker. > > Will fix the typos. >> + svc_int.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; >> + svc_int.svc_interval = pcfg->usb_cfg.service_interval; >> + ret = q6afe_port_set_param_v2(port, &svc_int, >> + AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL, >> + AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(svc_int)); >> + if (ret) { >> + dev_err(port->afe->dev, "%s: AFE device param cmd svc_interval failed %d\n", >> + __func__, ret); >> + ret = -EINVAL; > > why do you override the return value? > >> + goto exit; > > not necessary, this is a jump to the next line. Looks like copy-paste ... > Thanks, will fix. >> + } >> +exit: >> + return ret; >> +} >> + >> +/** >> + * q6afe_usb_port_prepare() - Prepare usb afe port. >> + * >> + * @port: Instance of afe port >> + * @cfg: USB configuration for the afe port >> + * >> + */ >> +void q6afe_usb_port_prepare(struct q6afe_port *port, >> + struct q6afe_usb_cfg *cfg) >> +{ >> + union afe_port_config *pcfg = &port->port_cfg; >> + >> + pcfg->usb_cfg.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; >> + pcfg->usb_cfg.sample_rate = cfg->sample_rate; >> + pcfg->usb_cfg.num_channels = cfg->num_channels; >> + pcfg->usb_cfg.bit_width = cfg->bit_width; >> + >> + afe_port_send_usb_dev_param(port, cfg); >> +} >> +EXPORT_SYMBOL_GPL(q6afe_usb_port_prepare); >> + >> /** >> * q6afe_hdmi_port_prepare() - Prepare hdmi afe port. >> * >> @@ -1611,7 +1791,10 @@ struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id) >> break; >> case AFE_PORT_ID_WSA_CODEC_DMA_RX_0 ... AFE_PORT_ID_RX_CODEC_DMA_RX_7: >> cfg_type = AFE_PARAM_ID_CODEC_DMA_CONFIG; >> - break; >> + break; >> + case AFE_PORT_ID_USB_RX: >> + cfg_type = AFE_PARAM_ID_USB_AUDIO_CONFIG; >> + break; >> default: >> dev_err(dev, "Invalid port id 0x%x\n", port_id); >> return ERR_PTR(-EINVAL); >> diff --git a/sound/soc/qcom/qdsp6/q6afe.h b/sound/soc/qcom/qdsp6/q6afe.h >> index 30fd77e2f458..e098a3e15135 100644 >> --- a/sound/soc/qcom/qdsp6/q6afe.h >> +++ b/sound/soc/qcom/qdsp6/q6afe.h >> @@ -5,7 +5,7 @@ >> >> #include <dt-bindings/sound/qcom,q6afe.h> >> >> -#define AFE_PORT_MAX 129 >> +#define AFE_PORT_MAX 130 >> >> #define MSM_AFE_PORT_TYPE_RX 0 >> #define MSM_AFE_PORT_TYPE_TX 1 >> @@ -205,6 +205,47 @@ struct q6afe_cdc_dma_cfg { >> u16 active_channels_mask; >> }; >> >> +/** >> + * struct q6afe_usb_cfg >> + * @cfg_minor_version: Minor version used for tracking USB audio device >> + * configuration. >> + * Supported values: >> + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG >> + * @sample_rate: Sampling rate of the port >> + * Supported values: >> + * AFE_PORT_SAMPLE_RATE_8K >> + * AFE_PORT_SAMPLE_RATE_11025 >> + * AFE_PORT_SAMPLE_RATE_12K >> + * AFE_PORT_SAMPLE_RATE_16K >> + * AFE_PORT_SAMPLE_RATE_22050 >> + * AFE_PORT_SAMPLE_RATE_24K >> + * AFE_PORT_SAMPLE_RATE_32K >> + * AFE_PORT_SAMPLE_RATE_44P1K >> + * AFE_PORT_SAMPLE_RATE_48K >> + * AFE_PORT_SAMPLE_RATE_96K >> + * AFE_PORT_SAMPLE_RATE_192K >> + * @bit_width: Bit width of the sample. >> + * Supported values: 16, 24 >> + * @num_channels: Number of channels >> + * Supported values: 1, 2 >> + * @data_format: Data format supported by the USB >> + * Supported values: 0 >> + * @reserved: this field must be 0 >> + * @dev_token: device token of actual end USB audio device >> + * @endian: endianness of this interface >> + * @service_interval: service interval >> + **/ >> +struct q6afe_usb_cfg { >> + u32 cfg_minor_version; >> + u32 sample_rate; >> + u16 bit_width; >> + u16 num_channels; >> + u16 data_format; >> + u16 reserved; >> + u32 dev_token; >> + u32 endian; >> + u32 service_interval; >> +}; > > this definition looks exactly the same as > struct afe_param_id_usb_cfg > ?? > > I'll remove some of the params that we aren't utilizing. Thanks Wesley Cheng
Hi Pierre, On 7/25/2023 1:45 AM, Pierre-Louis Bossart wrote: > >> +struct q6usb_port_data { >> + struct q6afe_usb_cfg usb_cfg; >> + struct snd_soc_usb *usb; >> + struct q6usb_offload priv; >> + int active_idx; > > what is an 'active_idx' ? > > active_idx carries the USB sound card we're going to be offloading. >> +static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, >> + int connected) >> +{ >> + struct snd_soc_dapm_context *dapm; >> + struct q6usb_port_data *data; >> + >> + dapm = snd_soc_component_get_dapm(usb->component); >> + data = dev_get_drvdata(usb->component->dev); > > shouldn't you test that 'dapm' and 'data' are not NULL ? > q6usb_component_probe() would be the one that registers to SOC USB to add this callback. At that time, the component's dev and dapm references should be populated, so that should ensure that those are valid. However, we could see that usb->component to be NULL, as that assignment happens after adding the port. Instead I will add a check for usb->component before attempting to access the dapm/data params. Another thing I will modify is to add a component removal callback, which will remove the SOC USB port. That will ensure that no connection_cb() events are issued, so we don't run into any NULL pointer issues during the remove path. >> + >> + if (connected) { > > this goes back to my earlier comment that you treat 'connected' as a > boolean. > Done, changed to boolean. >> + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); >> + /* We only track the latest USB headset plugged in */ >> + data->active_idx = card_idx; >> + } else { >> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); >> + } >> + snd_soc_dapm_sync(dapm); >> + >> + return 0; >> +} >> + >> +static int q6usb_component_probe(struct snd_soc_component *component) >> +{ >> + struct q6usb_port_data *data = dev_get_drvdata(component->dev); >> + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); >> + >> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); >> + snd_soc_dapm_sync(dapm); >> + >> + data->usb = snd_soc_usb_add_port(component->dev, &data->priv, q6usb_alsa_connection_cb); >> + if (IS_ERR(data->usb)) { >> + dev_err(component->dev, "failed to add usb port\n"); >> + return -ENODEV; >> + } >> + >> + data->usb->component = component; >> + >> + return 0; >> +} >> + >> +static const struct snd_soc_component_driver q6usb_dai_component = { >> + .probe = q6usb_component_probe, > > erm, if you have a .probe that adds a port, don't you need a remove that > removes the same port, and sets the pin state as well? > Will add this as mentioned above. >> + .name = "q6usb-dai-component", >> + .dapm_widgets = q6usb_dai_widgets, >> + .num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets), >> + .dapm_routes = q6usb_dapm_routes, >> + .num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes), >> + .of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name, >> +}; >> + >> +static int q6usb_dai_dev_probe(struct platform_device *pdev) >> +{ >> + struct device_node *node = pdev->dev.of_node; >> + struct q6usb_port_data *data; >> + struct device *dev = &pdev->dev; >> + struct of_phandle_args args; >> + int ret; >> + >> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + ret = of_property_read_u32(node, "qcom,usb-audio-intr-num", >> + &data->priv.intr_num); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to read intr num.\n"); >> + return ret; >> + } >> + >> + ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args); >> + if (ret < 0) >> + data->priv.sid = -1; >> + else >> + data->priv.sid = args.args[0] & SID_MASK; >> + >> + data->priv.domain = iommu_get_domain_for_dev(&pdev->dev); >> + >> + data->priv.dev = dev; >> + dev_set_drvdata(dev, data); >> + >> + ret = devm_snd_soc_register_component(dev, &q6usb_dai_component, >> + q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais)); >> + if (ret < 0) >> + return ret; >> + >> + return 0; > > return devm_snd_soc_register_component > >> +} >> + >> +static int q6usb_dai_dev_remove(struct platform_device *pdev) >> +{ >> + snd_soc_usb_remove_port(&pdev->dev); > > that seems wrong, the port is added in the component probe, not the > platform device probe. > Will fix this. Thanks Wesley Cheng
Hi Pierre, On 7/25/2023 1:59 AM, Pierre-Louis Bossart wrote: > > > On 7/25/23 04:34, Wesley Cheng wrote: >> Currently, only the index to the USB SND card array is passed to the USB >> backend. Pass through more information, specifically the USB SND card >> number and the number of PCM devices available. The USB backend should >> know about which sound resources are being shared between the ASoC and USB >> SND paths. This can be utilized to properly select and maintain the >> offloading devices. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> include/sound/soc-usb.h | 9 +++++---- >> sound/soc/qcom/qdsp6/q6usb.c | 20 ++++++++++++++++++-- >> sound/soc/soc-usb.c | 12 +++++++----- >> sound/usb/qcom/qc_audio_offload.c | 9 +++++---- >> 4 files changed, 35 insertions(+), 15 deletions(-) >> >> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h >> index 71e6e75e600a..606128332044 100644 >> --- a/include/sound/soc-usb.h >> +++ b/include/sound/soc-usb.h >> @@ -19,20 +19,21 @@ struct snd_soc_usb { >> struct device *dev; >> struct snd_soc_component *component; >> int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx, >> - int connected); >> + int chip_idx, int num_pcm, int connected); > > I don't know what 'chip_idx' is. > chip_idx is the index to the USB SND chip array which carries information about each udev (USB device) connected to USB SND) > The 'num_pcm' sounds problematic if there are different devices for > playback and capture. I would guess this is for playback only, but this > doesn't scale. > I agree. My plan is to address this by having another SND SOC USB structure that is created by the USB class driver, ie qc_usb_offload, which would populate the required parameters. Main thing I wanted to avoid is to have to expose the main USB SND structure if we're only going to only have a few variables to check for. >> void *priv_data; >> }; > >> +struct q6usb_status { >> + unsigned int num_pcm; >> + unsigned int chip_index; >> + unsigned int pcm_index; >> +}; >> + >> struct q6usb_port_data { >> struct q6afe_usb_cfg usb_cfg; >> struct snd_soc_usb *usb; >> struct q6usb_offload priv; >> + unsigned long available_card_slot; > > what is a card slot? > Idea was just to have a bitmapped value of all offload capable audio devices that are currently connected. Then we can take the necessary actions when everything has been disconnected. >> + struct q6usb_status status[SNDRV_CARDS]; >> int active_idx; >> }; >> >> @@ -97,7 +105,7 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone >> } >> >> static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, >> - int connected) >> + int chip_idx, int num_pcm, int connected) >> { >> struct snd_soc_dapm_context *dapm; >> struct q6usb_port_data *data; >> @@ -109,8 +117,16 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, >> snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); >> /* We only track the latest USB headset plugged in */ >> data->active_idx = card_idx; >> + >> + set_bit(card_idx, &data->available_card_slot); >> + data->status[card_idx].num_pcm = num_pcm; >> + data->status[card_idx].chip_index = chip_idx; >> } else { >> - snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); >> + clear_bit(card_idx, &data->available_card_slot); >> + data->status[card_idx].num_pcm = 0; >> + data->status[card_idx].chip_index = 0; >> + if (!data->available_card_slot) >> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); > > not able to follow what this does, this patch is rather unclear and > lacks comments. > I will add comments, but it basically will populate some of the limits that we'll end up using for controlling the ksndcontrols. Thanks Wesley Cheng
Hi Pierre, On 7/25/2023 2:10 AM, Pierre-Louis Bossart wrote: > > > On 7/25/23 04:34, Wesley Cheng wrote: >> The headphone jack framework has a well defined intrastructure for > > infrastructure > >> notifying userspace entities through input devices. Expose a jack device >> that carries information about if an offload capable device is connected. >> Applications can further identify specific offloading information through >> other SND kcontrols. > > What if you connect a set of USB speakers? Would they show as a > headphone/headset? > For now, let me modify the patch to send a HEADPHONE event. We don't support the capture/record path as of now, so it doesn't make sense to generate a HEADSET event (which exposes both MIC and HEADPHONE). When you plug in any USB audio device we'd generate this snd jack event. Main purpose was to notify that the offload path is potentially available. Thanks Wesley Cheng >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> sound/soc/qcom/qdsp6/q6usb.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c >> index e4ccb9d912b0..860dff8c1438 100644 >> --- a/sound/soc/qcom/qdsp6/q6usb.c >> +++ b/sound/soc/qcom/qdsp6/q6usb.c >> @@ -20,6 +20,7 @@ >> #include <sound/pcm_params.h> >> #include <sound/asound.h> >> #include <sound/q6usboffload.h> >> +#include <sound/jack.h> >> >> #include "q6dsp-lpass-ports.h" >> #include "q6afe.h" >> @@ -37,6 +38,7 @@ struct q6usb_status { >> struct q6usb_port_data { >> struct q6afe_usb_cfg usb_cfg; >> struct snd_soc_usb *usb; >> + struct snd_soc_jack hs_jack; >> struct q6usb_offload priv; >> struct mutex mutex; >> unsigned long available_card_slot; >> @@ -279,6 +281,7 @@ static const struct snd_kcontrol_new q6usb_offload_control = { >> /* Build a mixer control for a UAC connector control (jack-detect) */ >> static void q6usb_connector_control_init(struct snd_soc_component *component) >> { >> + struct q6usb_port_data *data = dev_get_drvdata(component->dev); >> int ret; >> >> ret = snd_ctl_add(component->card->snd_card, >> @@ -290,6 +293,11 @@ static void q6usb_connector_control_init(struct snd_soc_component *component) >> snd_ctl_new1(&q6usb_offload_dev_ctrl, component)); >> if (ret < 0) >> return; >> + >> + ret = snd_soc_card_jack_new(component->card, "USB offload", >> + SND_JACK_HEADSET, &data->hs_jack); > > not all USB devices are headsets... > >> + if (ret) >> + return; >> } >> >> static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component, >> @@ -322,7 +330,10 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, >> >> mutex_lock(&data->mutex); >> if (connected) { >> - snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); >> + if (!data->available_card_slot) { >> + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE"); >> + snd_jack_report(data->hs_jack.jack, 1); >> + } >> /* >> * Update the latest USB headset plugged in, if session is >> * idle. >> @@ -338,8 +349,10 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx, >> clear_bit(card_idx, &data->available_card_slot); >> data->status[card_idx].num_pcm = 0; >> data->status[card_idx].chip_index = 0; >> - if (!data->available_card_slot) >> + if (!data->available_card_slot) { >> snd_soc_dapm_disable_pin(dapm, "USB_RX_BE"); >> + snd_jack_report(data->hs_jack.jack, 0); >> + } >> } >> snd_soc_dapm_sync(dapm); >> mutex_unlock(&data->mutex);
Hi Pierre, On 7/25/2023 2:15 AM, Pierre-Louis Bossart wrote: > > > On 7/25/23 04:34, Wesley Cheng wrote: >> In case of notifying SND platform drivers of connection events, some of >> these use cases, such as offloading, require an ASoC USB backend device to >> be initialized before the events can be handled. If the USB backend device >> has not yet been probed, this leads to missing initial USB audio device >> connection events. >> >> Expose an API that traverses the usb_chip array for connected devices, and >> to call the respective connection callback registered to the SND platform >> driver. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> sound/usb/card.c | 19 +++++++++++++++++++ >> sound/usb/card.h | 2 ++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/sound/usb/card.c b/sound/usb/card.c >> index 365f6d978608..27a89aaa0bf3 100644 >> --- a/sound/usb/card.c >> +++ b/sound/usb/card.c >> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >> } >> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >> >> +/* >> + * in case the platform driver was not ready at the time of USB SND >> + * device connect, expose an API to discover all connected USB devices >> + * so it can populate any dependent resources/structures. >> + */ >> +void snd_usb_rediscover_devices(void) >> +{ >> + int i; >> + >> + mutex_lock(®ister_mutex); >> + for (i = 0; i < SNDRV_CARDS; i++) { >> + if (usb_chip[i]) >> + if (platform_ops && platform_ops->connect_cb) >> + platform_ops->connect_cb(usb_chip[i]); > > what happens if the USB device is removed while the platform device adds > a port? > > This sounds super-racy to me. It's the same set of problems we're having > between audio and display/DRM, I would be surprised if this function > dealt with all corner cases of insertion/removal, bind/unbind. > The chip array entries are all populated and removed while under the register_mutex, so going over your race condition, we should see: Thread#1: q6usb_component_probe() --> snd_soc_usb_add_port() --> snd_usb_rediscover_devices() --> mutex_lock(register_mutex) Thread#2 --> usb_audio_disconnect() --> mutex_lock(register_mutex) So either thread#1 or thread#2 will complete first. If Thread#1 completes before thread#2: SOC USB will notify DPCM backend of the device connection. Shortly after, once thread#2 runs, we will get a disconnect event for the connected device. Thread#2 completes before thread#1: Then during snd_usb_rediscover_devices() we won't notify of any connection for that particular chip index. Thanks Wesley Cheng
On 8/15/23 20:38, Wesley Cheng wrote: > Hi Pierre, > > On 7/25/2023 2:15 AM, Pierre-Louis Bossart wrote: >> >> >> On 7/25/23 04:34, Wesley Cheng wrote: >>> In case of notifying SND platform drivers of connection events, some of >>> these use cases, such as offloading, require an ASoC USB backend >>> device to >>> be initialized before the events can be handled. If the USB backend >>> device >>> has not yet been probed, this leads to missing initial USB audio device >>> connection events. >>> >>> Expose an API that traverses the usb_chip array for connected >>> devices, and >>> to call the respective connection callback registered to the SND >>> platform >>> driver. >>> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>> --- >>> sound/usb/card.c | 19 +++++++++++++++++++ >>> sound/usb/card.h | 2 ++ >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/sound/usb/card.c b/sound/usb/card.c >>> index 365f6d978608..27a89aaa0bf3 100644 >>> --- a/sound/usb/card.c >>> +++ b/sound/usb/card.c >>> @@ -170,6 +170,25 @@ struct snd_usb_stream >>> *snd_usb_find_suppported_substream(int card_idx, >>> } >>> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >>> +/* >>> + * in case the platform driver was not ready at the time of USB SND >>> + * device connect, expose an API to discover all connected USB devices >>> + * so it can populate any dependent resources/structures. >>> + */ >>> +void snd_usb_rediscover_devices(void) >>> +{ >>> + int i; >>> + >>> + mutex_lock(®ister_mutex); >>> + for (i = 0; i < SNDRV_CARDS; i++) { >>> + if (usb_chip[i]) >>> + if (platform_ops && platform_ops->connect_cb) >>> + platform_ops->connect_cb(usb_chip[i]); >> >> what happens if the USB device is removed while the platform device adds >> a port? >> >> This sounds super-racy to me. It's the same set of problems we're having >> between audio and display/DRM, I would be surprised if this function >> dealt with all corner cases of insertion/removal, bind/unbind. >> > > The chip array entries are all populated and removed while under the > register_mutex, so going over your race condition, we should see: > > Thread#1: > q6usb_component_probe() > --> snd_soc_usb_add_port() > --> snd_usb_rediscover_devices() > --> mutex_lock(register_mutex) > > Thread#2 > --> usb_audio_disconnect() > --> mutex_lock(register_mutex) > > So either thread#1 or thread#2 will complete first. If > > Thread#1 completes before thread#2: > SOC USB will notify DPCM backend of the device connection. Shortly > after, once thread#2 runs, we will get a disconnect event for the > connected device. > > Thread#2 completes before thread#1: > Then during snd_usb_rediscover_devices() we won't notify of any > connection for that particular chip index. You really want to capture this locking model as part of the commit messages or code, it'll help reviewers figure things out.
Hi Takashi, On 7/25/2023 2:27 AM, Takashi Iwai wrote: > On Tue, 25 Jul 2023 11:15:11 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 7/25/23 04:34, Wesley Cheng wrote: >>> In case of notifying SND platform drivers of connection events, some of >>> these use cases, such as offloading, require an ASoC USB backend device to >>> be initialized before the events can be handled. If the USB backend device >>> has not yet been probed, this leads to missing initial USB audio device >>> connection events. >>> >>> Expose an API that traverses the usb_chip array for connected devices, and >>> to call the respective connection callback registered to the SND platform >>> driver. >>> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>> --- >>> sound/usb/card.c | 19 +++++++++++++++++++ >>> sound/usb/card.h | 2 ++ >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/sound/usb/card.c b/sound/usb/card.c >>> index 365f6d978608..27a89aaa0bf3 100644 >>> --- a/sound/usb/card.c >>> +++ b/sound/usb/card.c >>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >>> } >>> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >>> >>> +/* >>> + * in case the platform driver was not ready at the time of USB SND >>> + * device connect, expose an API to discover all connected USB devices >>> + * so it can populate any dependent resources/structures. >>> + */ >>> +void snd_usb_rediscover_devices(void) >>> +{ >>> + int i; >>> + >>> + mutex_lock(®ister_mutex); >>> + for (i = 0; i < SNDRV_CARDS; i++) { >>> + if (usb_chip[i]) >>> + if (platform_ops && platform_ops->connect_cb) >>> + platform_ops->connect_cb(usb_chip[i]); >> >> what happens if the USB device is removed while the platform device adds >> a port? > > That should be protected by the register_mutex. But there can be > other races (see below :) > >> This sounds super-racy to me. It's the same set of problems we're having >> between audio and display/DRM, I would be surprised if this function >> dealt with all corner cases of insertion/removal, bind/unbind. > > Yes, we need to be more careful about binding. > > For example, in the current patch set, I see no way to prevent > unloading snd-usb-audio-qmi module, and it allows user to cut off the > stuff during operation, which may break things while the kernel is > running the code of the unloaded module. You need to have a proper > module refcount management for avoiding such a scenario. Most of > drivers don't need it because ALSA core part already takes care of > it. But in this case, it requires a manual adjustment. > Sorry for the delayed response...this was routed to another folder, and missed it. I see, that is a valid situation, so I think the best way to address it is to do something like the following: static void __exit qc_usb_audio_offload_exit(void) { ... snd_usb_unregister_platform_ops(); for (idx = 0; idx < SNDRV_CARDS; idx++) qc_usb_audio_offload_disconnect(uadev[idx].chip); We'll first unregister the platform ops, so that we get no further connect/disconnect CBs, and then we'll forcefully ensure that all devices are removed/cleaned. Part of the USB offload disconnect sequence will also forcefully stop the offload path on the external DSP by issuing a disconnect indication QMI message. Then we can safely clean up the rest of the resources. We do have refcounting in place for several of the other structures, but I think in the module exit case, we need to ensure the offload path is stopped fully. Thanks Wesley Cheng
On Mon, 28 Aug 2023 23:25:14 +0200, Wesley Cheng wrote: > > Hi Takashi, > > On 7/25/2023 2:27 AM, Takashi Iwai wrote: > > On Tue, 25 Jul 2023 11:15:11 +0200, > > Pierre-Louis Bossart wrote: > >> > >> > >> > >> On 7/25/23 04:34, Wesley Cheng wrote: > >>> In case of notifying SND platform drivers of connection events, some of > >>> these use cases, such as offloading, require an ASoC USB backend device to > >>> be initialized before the events can be handled. If the USB backend device > >>> has not yet been probed, this leads to missing initial USB audio device > >>> connection events. > >>> > >>> Expose an API that traverses the usb_chip array for connected devices, and > >>> to call the respective connection callback registered to the SND platform > >>> driver. > >>> > >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > >>> --- > >>> sound/usb/card.c | 19 +++++++++++++++++++ > >>> sound/usb/card.h | 2 ++ > >>> 2 files changed, 21 insertions(+) > >>> > >>> diff --git a/sound/usb/card.c b/sound/usb/card.c > >>> index 365f6d978608..27a89aaa0bf3 100644 > >>> --- a/sound/usb/card.c > >>> +++ b/sound/usb/card.c > >>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > >>> } > >>> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > >>> +/* > >>> + * in case the platform driver was not ready at the time of USB SND > >>> + * device connect, expose an API to discover all connected USB devices > >>> + * so it can populate any dependent resources/structures. > >>> + */ > >>> +void snd_usb_rediscover_devices(void) > >>> +{ > >>> + int i; > >>> + > >>> + mutex_lock(®ister_mutex); > >>> + for (i = 0; i < SNDRV_CARDS; i++) { > >>> + if (usb_chip[i]) > >>> + if (platform_ops && platform_ops->connect_cb) > >>> + platform_ops->connect_cb(usb_chip[i]); > >> > >> what happens if the USB device is removed while the platform device adds > >> a port? > > > > That should be protected by the register_mutex. But there can be > > other races (see below :) > > > >> This sounds super-racy to me. It's the same set of problems we're having > >> between audio and display/DRM, I would be surprised if this function > >> dealt with all corner cases of insertion/removal, bind/unbind. > > > > Yes, we need to be more careful about binding. > > > > For example, in the current patch set, I see no way to prevent > > unloading snd-usb-audio-qmi module, and it allows user to cut off the > > stuff during operation, which may break things while the kernel is > > running the code of the unloaded module. You need to have a proper > > module refcount management for avoiding such a scenario. Most of > > drivers don't need it because ALSA core part already takes care of > > it. But in this case, it requires a manual adjustment. > > > > Sorry for the delayed response...this was routed to another folder, > and missed it. > > I see, that is a valid situation, so I think the best way to address > it is to do something like the following: > > static void __exit qc_usb_audio_offload_exit(void) > { > ... > snd_usb_unregister_platform_ops(); > for (idx = 0; idx < SNDRV_CARDS; idx++) > qc_usb_audio_offload_disconnect(uadev[idx].chip); > > We'll first unregister the platform ops, so that we get no further > connect/disconnect CBs, and then we'll forcefully ensure that all > devices are removed/cleaned. Part of the USB offload disconnect > sequence will also forcefully stop the offload path on the external > DSP by issuing a disconnect indication QMI message. > > Then we can safely clean up the rest of the resources. We do have > refcounting in place for several of the other structures, but I think > in the module exit case, we need to ensure the offload path is stopped > fully. Yes, the proper disconnection procedure is mandatory. In addition, you can have a module refcount increment upon the card binding, too. This may prevent the unexpected exit (although it's often still possible to unbind via sysfs). thanks, Takashi
Several Qualcomm based chipsets can support USB audio offloading to a dedicated audio DSP, which can take over issuing transfers to the USB host controller. The intention is to reduce the load on the main processors in the SoC, and allow them to be placed into lower power modes. There are several parts to this design: 1. Adding ASoC binding layer 2. Create a USB backend for Q6DSP 3. Introduce XHCI interrupter support 4. Create vendor ops for the USB SND driver USB | ASoC -------------------------------------------------------------------- | _________________________ | |sm8250 platform card | | |_________________________| | | | | ___V____ ____V____ | |Q6USB | |Q6AFE | | |"codec" | |"cpu" | | |________| |_________| | ^ ^ ^ | | |________| | ___V____ | | |SOC-USB | | ________ ________ | | | |USB SND |<--->|QC offld|<------------>|________| | |(card.c)| | |<---------- | |________| |________|___ | | | ^ ^ | | | ____________V_________ | | | | | |APR/GLINK | __ V_______________V_____ | | | |______________________| |USB SND (endpoint.c) | | | | ^ |_________________________| | | | | ^ | | | ___________V___________ | | | |->|audio DSP | ___________V_____________ | | |_______________________| |XHCI HCD |<- | |_________________________| | Adding ASoC binding layer: soc-usb: Intention is to treat a USB port similar to a headphone jack. The port is always present on the device, but cable/pin status can be enabled/disabled. Expose mechanisms for USB backend ASoC drivers to communicate with USB SND. Create a USB backend for Q6DSP: q6usb: Basic backend driver that will be responsible for maintaining the resources needed to initiate a playback stream using the Q6DSP. Will be the entity that checks to make sure the connected USB audio device supports the requested PCM format. If it does not, the PCM open call will fail, and userpsace ALSA can take action accordingly. Introduce XHCI interrupter support: XHCI HCD supports multiple interrupters, which allows for events to be routed to different event rings. This is determined by "Interrupter Target" field specified in Section "6.4.1.1 Normal TRB" of the XHCI specification. Events in the offloading case will be routed to an event ring that is assigned to the audio DSP. Create vendor ops for the USB SND driver: qc_audio_offload: This particular driver has several components associated with it: - QMI stream request handler - XHCI interrupter and resource management - audio DSP memory management When the audio DSP wants to enable a playback stream, the request is first received by the ASoC platform sound card. Depending on the selected route, ASoC will bring up the individual DAIs in the path. The Q6USB backend DAI will send an AFE port start command (with enabling the USB playback path), and the audio DSP will handle the request accordingly. Part of the AFE USB port start handling will have an exchange of control messages using the QMI protocol. The qc_audio_offload driver will populate the buffer information: - Event ring base address - EP transfer ring base address and pass it along to the audio DSP. All endpoint management will now be handed over to the DSP, and the main processor is not involved in transfers. Overall, implementing this feature will still expose separate sound card and PCM devices for both the platorm card and USB audio device: 0 [SM8250MTPWCD938]: sm8250 - SM8250-MTP-WCD9380-WSA8810-VA-D SM8250-MTP-WCD9380-WSA8810-VA-DMIC 1 [Audio ]: USB-Audio - USB Audio Generic USB Audio at usb-xhci-hcd.1.auto-1.4, high speed This is to ensure that userspace ALSA entities can decide which route to take when executing the audio playback. In the above, if card#1 is selected, then USB audio data will take the legacy path over the USB PCM drivers, etc... This feature was validated using: - tinymix: set/enable the multimedia path to route to USB backend - tinyplay: issue playback on platform card Changelog -------------------------------------------- Changes in v4: - Rebased to xhci/for-usb-next - Addressed some dt-bindings comments XHCI: - Pulled in latest changes from Mathias' feature_interrupters branch: https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters - Fixed commit text and signage for the XHCI sideband/interrupter related changes - Added some logic to address the FIXME tags mentioned throughout the commits, such as handling multi segment rings and building the SGT, locking concerns, and ep cleanup operations. - Removed some fixme tags for conditions that may not be needed/addressed. - Repurposed the new endpoint stop sync API to be utilized in other places. - Fixed potential compile issue if XHCI sideband config is not defined. ASoC: - Added sound jack control into the Q6USB driver. Allows for userpsace to know when an offload capable device is connected. USB SND: - Avoided exporting _snd_pcm_hw_param_set based on Takashi's recommendation. - Split USB QMI packet header definitions into a separate commit. This is used to properly allow the QMI interface driver to parse and route QMI packets accordingly - Added a "depends on" entry when enabling QC audio offload to avoid compile time issues. Changes in v3: - Changed prefix from RFC to PATCH - Rebased entire series to usb-next - Updated copyright years XHCI: - Rebased changes on top of XHCI changes merged into usb-next, and only added changes that were still under discussion. - Added change to read in the "num-hc-interrupters" device property. ASoC: - qusb6 USB backend - Incorporated suggestions to fetch iommu information with existing APIs - Added two new sound kcontrols to fetch offload status and offload device selection. - offload status - will return the card and pcm device in use tinymix -D 0 get 1 --> 1, 0 (offload in progress on card#1 pcm#0) - device selection - set the card and pcm device to enable offload on. Ex.: tinymix -D 0 set 1 2 0 --> sets offload on card#2 pcm#0 (this should be the USB card) USB SND: - Fixed up some locking related concerns for registering platform ops. - Moved callbacks under the register_mutex, so that - Modified APIs to properly pass more information about the USB SND device, so that the Q6USB backend can build a device list/map, in order to monitor offload status and device selection. Changes in v2: XHCI: - Replaced XHCI and HCD changes with Mathias' XHCI interrupter changes in his tree: https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters Adjustments made to Mathias' changes: - Created xhci-intr.h to export/expose interrupter APIs versus exposing xhci.h. Moved dependent structures to this file as well. (so clients can parse out information from "struct xhci_interrupter") - Added some basic locking when requesting interrupters. - Fixed up some sanity checks. - Removed clearing of the ERSTBA during freeing of the interrupter. (pending issue where SMMU fault occurs if DMA addr returned is 64b - TODO) - Clean up pending events in the XHCI secondary interrupter. While testing USB bus suspend, it was seen that on bus resume, the xHCI HC would run into a command timeout. - Added offloading APIs to xHCI to fetch transfer and event ring information. ASoC: - Modified soc-usb to allow for multiple USB port additions. For this to work, the USB offload driver has to have a reference to the USB backend by adding a "usb-soc-be" DT entry to the device saved into XHCI sysdev. - Created separate dt-bindings for defining USB_RX port. - Increased APR timeout to accommodate the situation where the AFE port start command could be delayed due to having to issue a USB bus resume while handling the QMI stream start command. USB SND: - Added a platform ops during usb_audio_suspend(). This allows for the USB offload driver to halt the audio stream when system enters PM suspend. This ensures the audio DSP is not issuing transfers on the USB bus. - Do not override platform ops if they are already populated. - Introduce a shared status variable between the USB offload and USB SND layers, to ensure that only one path is active at a time. If the USB bus is occupied, then userspace is notified that the path is busy. Mathias Nyman (3): xhci: add support to allocate several interrupters xhci: add helper to stop endpoint and wait for completion xhci: sideband: add initial api to register a sideband entity Wesley Cheng (29): 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: Add USB backend ASoC driver for Q6 sound: usb: card: Introduce USB SND platform op callbacks sound: usb: Export USB SND APIs for modules dt-bindings: usb: dwc3: Add snps,num-hc-interrupters definition usb: dwc3: Add DT parameter to specify maximum number of interrupters usb: host: xhci-plat: Set XHCI max interrupters if property is present sound: usb: pcm: Export fixed rate check USB SND API sound: usb: qcom: Add USB QMI definitions sound: usb: Introduce QC USB SND offloading support sound: usb: card: Check for support for requested audio format sound: soc: soc-usb: Add PCM format check API for USB backend sound: soc: qcom: qusb6: Ensure PCM format is supported by USB audio device sound: usb: Prevent starting of audio stream if in use ASoC: dt-bindings: Add Q6USB backend bindings ASoC: dt-bindings: Update example for enabling USB offload on SM8250 ASoC: qcom: qdsp6: q6afe: Split USB AFE dev_token param into separate API sound: Pass USB SND card and PCM information to SOC USB sound: soc: qdsp6: Add SND kcontrol to select offload device sound: soc: qdsp6: Add SND kcontrol for fetching offload status sound: soc: qcom: q6usb: Add headphone jack for offload connection status sound: usb: qc_audio_offload: Use card and PCM index from QMI request sound: usb: card: Allow for rediscovery of connected USB SND devices sound: soc: soc-usb: Rediscover USB SND devices on USB port add .../bindings/sound/qcom,q6usb-dais.yaml | 49 + .../bindings/sound/qcom,sm8250.yaml | 15 + .../devicetree/bindings/usb/snps,dwc3.yaml | 13 + arch/arm64/boot/dts/qcom/sm8350.dtsi.rej | 14 + arch/arm64/configs/defconfig.rej | 24 + 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 | 4 + drivers/usb/host/xhci-debugfs.c | 2 +- drivers/usb/host/xhci-hub.c | 29 +- drivers/usb/host/xhci-mem.c | 167 +- drivers/usb/host/xhci-plat.c | 2 + drivers/usb/host/xhci-ring.c | 2 +- drivers/usb/host/xhci-sideband.c | 195 ++ drivers/usb/host/xhci.c | 109 +- drivers/usb/host/xhci.h | 102 +- .../sound/qcom,q6dsp-lpass-ports.h | 1 + include/linux/usb/xhci-intr.h | 86 + include/linux/usb/xhci-sideband.h | 90 + include/sound/q6usboffload.h | 20 + include/sound/soc-usb.h | 39 + sound/core/oss/pcm_oss.c | 4 +- sound/soc/Makefile | 2 +- sound/soc/qcom/Kconfig | 4 + sound/soc/qcom/qdsp6/Makefile | 1 + sound/soc/qcom/qdsp6/q6afe-dai.c | 50 + sound/soc/qcom/qdsp6/q6afe.c | 208 +- sound/soc/qcom/qdsp6/q6afe.h | 47 +- 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 | 458 +++++ sound/soc/soc-usb.c | 197 ++ sound/usb/Kconfig | 15 + sound/usb/Makefile | 2 +- sound/usb/card.c | 85 + sound/usb/card.h | 32 + sound/usb/endpoint.c | 2 + sound/usb/helper.c | 1 + sound/usb/pcm.c | 29 +- sound/usb/pcm.h | 12 + sound/usb/qcom/Makefile | 2 + sound/usb/qcom/qc_audio_offload.c | 1820 +++++++++++++++++ sound/usb/qcom/usb_audio_qmi_v01.c | 892 ++++++++ sound/usb/qcom/usb_audio_qmi_v01.h | 162 ++ 47 files changed, 4870 insertions(+), 179 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml create mode 100644 arch/arm64/boot/dts/qcom/sm8350.dtsi.rej create mode 100644 arch/arm64/configs/defconfig.rej create mode 100644 drivers/usb/host/xhci-sideband.c create mode 100644 include/linux/usb/xhci-intr.h 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/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