mbox series

[RFC,00/14] Introduce QC USB SND audio offloading support

Message ID 20221223233200.26089-1-quic_wcheng@quicinc.com
Headers show
Series Introduce QC USB SND audio offloading support | expand

Message

Wesley Cheng Dec. 23, 2022, 11:31 p.m. UTC
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

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

Wesley Cheng (14):
  ASoC: Add SOC USB APIs for adding an USB backend
  ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp
  ASoC: qcom: Add USB backend ASoC driver for Q6
  sound: usb: card: Introduce USB SND vendor op callbacks
  sound: usb: Export USB SND APIs for modules
  usb: core: hcd: Introduce USB HCD APIs for interrupter management
  usb: host: xhci: Add XHCI secondary interrupter support
  usb: dwc3: Add DT parameter to specify maximum number of interrupters
  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
  ASoC: dt-bindings: Add Q6USB backend bindings
  ASoC: dt-bindings: Update example for enabling USB offload on SM8250

 .../bindings/sound/qcom,q6usb-dais.yaml       |   55 +
 .../bindings/sound/qcom,sm8250.yaml           |   13 +
 drivers/usb/core/hcd.c                        |   86 +
 drivers/usb/dwc3/core.c                       |   12 +
 drivers/usb/dwc3/core.h                       |    2 +
 drivers/usb/dwc3/host.c                       |    5 +-
 drivers/usb/host/xhci-mem.c                   |  219 ++-
 drivers/usb/host/xhci-plat.c                  |    2 +
 drivers/usb/host/xhci.c                       |  169 +-
 drivers/usb/host/xhci.h                       |   15 +
 .../sound/qcom,q6dsp-lpass-ports.h            |    1 +
 include/linux/usb.h                           |    7 +
 include/linux/usb/hcd.h                       |   16 +-
 include/sound/q6usboffload.h                  |   20 +
 include/sound/soc-usb.h                       |   34 +
 sound/soc/Makefile                            |    2 +-
 sound/soc/qcom/Kconfig                        |    4 +
 sound/soc/qcom/qdsp6/Makefile                 |    1 +
 sound/soc/qcom/qdsp6/q6afe-dai.c              |   47 +
 sound/soc/qcom/qdsp6/q6afe.c                  |  183 ++
 sound/soc/qcom/qdsp6/q6afe.h                  |   46 +-
 sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c      |   23 +
 sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h      |    1 +
 sound/soc/qcom/qdsp6/q6routing.c              |    8 +
 sound/soc/qcom/qdsp6/q6usb.c                  |  239 +++
 sound/soc/soc-usb.c                           |   79 +
 sound/usb/Kconfig                             |   14 +
 sound/usb/Makefile                            |    2 +-
 sound/usb/card.c                              |   43 +
 sound/usb/card.h                              |   10 +
 sound/usb/endpoint.c                          |    2 +
 sound/usb/helper.c                            |    1 +
 sound/usb/pcm.c                               |    9 +-
 sound/usb/pcm.h                               |   12 +
 sound/usb/qcom/Makefile                       |    2 +
 sound/usb/qcom/qc_audio_offload.c             | 1610 +++++++++++++++++
 sound/usb/qcom/usb_audio_qmi_v01.c            |  892 +++++++++
 sound/usb/qcom/usb_audio_qmi_v01.h            |  162 ++
 38 files changed, 3998 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml
 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

Comments

Greg KH Dec. 24, 2022, 6:45 a.m. UTC | #1
On Fri, Dec 23, 2022 at 03:31:46PM -0800, 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
> 
> 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

This looks to duplicate a bunch of the same things that a number of
different google developers have posted recently.  Please work with them
to come up with a unified set of patches that you all can agree with,
AND get them to sign off on the changes before resubmitting them.

This uncoordinated drip of patches from different people doing the same
thing is almost impossible to review from our side, as I'm sure you can
imagine.

That being said, thank you finally for at least submitting all of the
needed changes together as one patch set.  That's a first, and something
we had been asking for for years.

Have a good holiday break,

greg k-h
Greg KH Dec. 24, 2022, 6:48 a.m. UTC | #2
On Fri, Dec 23, 2022 at 03:31:47PM -0800, Wesley Cheng wrote:
> diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c
> new file mode 100644
> index 000000000000..c6c376960e4d
> --- /dev/null
> +++ b/sound/soc/soc-usb.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#include <linux/usb.h>
> +#include <sound/soc.h>
> +#include <sound/soc-usb.h>
> +#include "../usb/card.h"
> +
> +struct snd_soc_usb *ctx;

Note, this will not work.  You can not only have "one" state for a
system like this.  That just broke any system with more than one
controller, of which we have millions.

This has to be dynamic for any number of controllers in the system, like
the sound and USB core can handle.  Any requirement of "there can be
only one!" will obviously never be acceptable as that is not how Linux
works.

thanks,

greg k-h
Wesley Cheng Dec. 24, 2022, 8:49 a.m. UTC | #3
Hi Greg,

On 12/23/2022 10:45 PM, Greg KH wrote:
> On Fri, Dec 23, 2022 at 03:31:46PM -0800, 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
>>
>> 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
> 
> This looks to duplicate a bunch of the same things that a number of
> different google developers have posted recently.  Please work with them
> to come up with a unified set of patches that you all can agree with,
> AND get them to sign off on the changes before resubmitting them.
> 
> This uncoordinated drip of patches from different people doing the same
> thing is almost impossible to review from our side, as I'm sure you can
> imagine.

I saw some of the Google patchsets submitted awhile back, but didn't 
really get a chance to look at them in detail.  Let me reach out to 
Albert Wang to see if we can come to a solution that works for both 
implementations.

 From the looks of it (at least from the XHCI HCD changes), it seems 
that a different set of resources is required for the Google 
implementation to work.  I'll need to ask for a bit more details before 
I can comment further...

> 
> That being said, thank you finally for at least submitting all of the
> needed changes together as one patch set.  That's a first, and something
> we had been asking for for years.
> 
> Have a good holiday break,

Thanks for the quick in-depth review, and the feedback.  Gives me some 
more things to think about improving over the break :).  Happy holidays!

Thanks
Wesley Cheng
Greg KH Dec. 24, 2022, 8:54 a.m. UTC | #4
On Fri, Dec 23, 2022 at 03:31:52PM -0800, Wesley Cheng wrote:
> For USB HCDs that can support multiple USB interrupters, expose functions
> that class drivers can utilize for setting up secondary interrupters.
> Class drivers can pass this information to its respective clients, i.e.
> a dedicated DSP.

Where is the locking here that seems to be required when a hcd is
removed from the system and you have data in flight?  What am I missing
here in the design of this?

And yes, HCDs get removed all the time, and will happen more and more in
the future with the design of more systems moving to Thunderbolt/PCIe
designs due to the simplicity of it.

> +/**
> + * usb_set_interruper - Reserve an interrupter

Where is an "interrupter" defined?  I don't know what this term means
sorry, is this in the USB specification somewhere?


> + * @udev: usb device which requested the interrupter
> + * @intr_num: interrupter number to reserve
> + * @dma: iova address to event ring
> + *
> + * Request for a specific interrupter to be reserved for a USB class driver.
> + * This will return the base address to the event ring that was allocated to
> + * the specific interrupter.
> + **/
> +phys_addr_t usb_set_interruper(struct usb_device *udev, int intr_num,
> +							dma_addr_t *dma)
> +{
> +	struct usb_hcd *hcd;
> +	phys_addr_t pa = 0;
> +
> +	hcd = bus_to_hcd(udev->bus);
> +	if (hcd->driver->update_interrupter)
> +		pa = hcd->driver->update_interrupter(hcd, intr_num, dma);
> +
> +	return pa;

Wait, you return a physical address?  What are you going to do with
that?  And what guarantees that the address is valid after you return it
(again, remember memory and devices can be removed at any point in time.

thanks,

greg k-h
Greg KH Dec. 24, 2022, 8:55 a.m. UTC | #5
On Fri, Dec 23, 2022 at 03:31:53PM -0800, Wesley Cheng wrote:
> Implement the XHCI operations for allocating and requesting for a secondary
> interrupter.  The secondary interrupter can allow for events for a
> particular endpoint to be routed to a separate event ring.  The event
> routing is defined when submitting a transfer descriptor to the USB HW.
> There is a specific field which denotes which interrupter ring to route the
> event to when the transfer is completed.
> 
> An example use case, such as audio packet offloading can utilize a separate
> event ring, so that these events can be routed to a different processor
> within the system.  The processor would be able to independently submit
> transfers and handle its completions without intervention from the main
> processor.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/host/xhci-mem.c  | 219 ++++++++++++++++++++++++++++-------
>  drivers/usb/host/xhci-plat.c |   2 +
>  drivers/usb/host/xhci.c      | 169 ++++++++++++++++++++++++++-
>  drivers/usb/host/xhci.h      |  15 +++
>  4 files changed, 363 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 81ca2bc1f0be..d5cb4b82ad3d 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1835,6 +1835,7 @@ void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
>  void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  {
>  	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
> +	struct xhci_sec *sec, *tmp;
>  	int i, j, num_ports;
>  
>  	cancel_delayed_work_sync(&xhci->cmd_timer);
> @@ -1846,6 +1847,16 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  	xhci->event_ring = NULL;
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed event ring");
>  
> +	list_for_each_entry_safe(sec, tmp, &xhci->xhci_sec_list, list) {
> +		list_del(&sec->list);
> +		if (sec->event_ring) {
> +			xhci_ring_free(xhci, sec->event_ring);
> +			xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> +						"Freed secondary ring %d", sec->intr_num);

Odd indentation :(
Greg KH Dec. 24, 2022, 8:59 a.m. UTC | #6
On Fri, Dec 23, 2022 at 03:31:56PM -0800, 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 | 19 +++++++++++++++++++
>  sound/usb/card.h |  3 +++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 396e5a34e23b..9b8d2ed308c8 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -133,6 +133,25 @@ int snd_usb_unregister_vendor_ops(void)
>  }
>  EXPORT_SYMBOL_GPL(snd_usb_unregister_vendor_ops);
>  
> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
> +			struct snd_pcm_hw_params *params, int direction)
> +{
> +	struct snd_usb_stream *as;
> +	struct snd_usb_substream *subs = NULL;
> +	const struct audioformat *fmt;
> +
> +	if (usb_chip[card_idx] && enable[card_idx]) {
> +		list_for_each_entry(as, &usb_chip[card_idx]->pcm_list, list) {
> +			subs = &as->substream[direction];
> +			fmt = find_substream_format(subs, params);
> +			if (fmt)
> +				return as;
> +		}
> +	}

Where is the locking here?  How can you walk a list that can be changed
as you walk it?

And what about reference counting?  You are returning a pointer to a
structure, who now "owns" it?  What happens if it is removed from the
system after you return it?

> +	return 0;

Didn't sparse complain about this?  You can't return "0" as a pointer,
it should be NULL.

Always run basic tools like sparse on code before submitting it so that
we don't have to find errors like this.

thanks,

greg k-h
Dmitry Baryshkov Dec. 24, 2022, 11:13 a.m. UTC | #7
On Sat, 24 Dec 2022 at 01:33, Wesley Cheng <quic_wcheng@quicinc.com> wrote:
>
> Allow for the DWC3 host driver to pass along a XHCI property that defines
> how many interrupters to allocate.  This is in relation for the number of
> event rings that can be potentially used by other processors within the
> system.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 12 ++++++++++++
>  drivers/usb/dwc3/core.h |  2 ++
>  drivers/usb/dwc3/host.c |  5 ++++-
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..67d6f0ae81d2 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1446,6 +1446,7 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>         u8                      tx_thr_num_pkt_prd = 0;
>         u8                      tx_max_burst_prd = 0;
>         u8                      tx_fifo_resize_max_num;
> +       u8                      num_hc_interrupters;
>         const char              *usb_psy_name;
>         int                     ret;
>
> @@ -1468,6 +1469,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>          */
>         tx_fifo_resize_max_num = 6;
>
> +       /* default to a single XHCI interrupter */
> +       num_hc_interrupters = 1;
> +
>         dwc->maximum_speed = usb_get_maximum_speed(dev);
>         dwc->max_ssp_rate = usb_get_maximum_ssp_rate(dev);
>         dwc->dr_mode = usb_get_dr_mode(dev);
> @@ -1511,6 +1515,12 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>                                 &tx_thr_num_pkt_prd);
>         device_property_read_u8(dev, "snps,tx-max-burst-prd",
>                                 &tx_max_burst_prd);
> +       device_property_read_u8(dev, "snps,num-hc-interrupters",
> +                               &num_hc_interrupters);

bindings change?

> +       /* DWC3 core allowed to have a max of 8 interrupters */
> +       if (num_hc_interrupters > 8)
> +               num_hc_interrupters = 8;
> +
>         dwc->do_fifo_resize = device_property_read_bool(dev,
>                                                         "tx-fifo-resize");
>         if (dwc->do_fifo_resize)
> @@ -1589,6 +1599,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>         dwc->imod_interval = 0;
>
>         dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num;
> +
> +       dwc->num_hc_interrupters = num_hc_interrupters;
>  }
>
>  /* check whether the core supports IMOD */
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8f9959ba9fd4..09037299da53 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1050,6 +1050,7 @@ struct dwc3_scratchpad_array {
>   * @tx_max_burst_prd: max periodic ESS transmit burst size
>   * @tx_fifo_resize_max_num: max number of fifos allocated during txfifo resize
>   * @clear_stall_protocol: endpoint number that requires a delayed status phase
> + * @num_hc_interrupters: number of host controller interrupters
>   * @hsphy_interface: "utmi" or "ulpi"
>   * @connected: true when we're connected to a host, false otherwise
>   * @softconnect: true when gadget connect is called, false when disconnect runs
> @@ -1275,6 +1276,7 @@ struct dwc3 {
>         u8                      tx_max_burst_prd;
>         u8                      tx_fifo_resize_max_num;
>         u8                      clear_stall_protocol;
> +       u8                      num_hc_interrupters;
>
>         const char              *hsphy_interface;
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index f6f13e7f1ba1..52a284fdd704 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -66,7 +66,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
>
>  int dwc3_host_init(struct dwc3 *dwc)
>  {
> -       struct property_entry   props[4];
> +       struct property_entry   props[5];
>         struct platform_device  *xhci;
>         int                     ret, irq;
>         int                     prop_idx = 0;
> @@ -112,6 +112,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>         if (DWC3_VER_IS_WITHIN(DWC3, ANY, 300A))
>                 props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
>
> +       props[prop_idx++] = PROPERTY_ENTRY_U8("num-hc-interrupters",
> +                                                               dwc->num_hc_interrupters);
> +
>         if (prop_idx) {
>                 ret = device_create_managed_software_node(&xhci->dev, props, NULL);
>                 if (ret) {
Alan Stern Dec. 24, 2022, 3:29 p.m. UTC | #8
On Fri, Dec 23, 2022 at 03:31:52PM -0800, Wesley Cheng wrote:
> For USB HCDs that can support multiple USB interrupters, expose functions
> that class drivers can utilize for setting up secondary interrupters.
> Class drivers can pass this information to its respective clients, i.e.
> a dedicated DSP.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/core/hcd.c  | 86 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h     |  7 ++++
>  include/linux/usb/hcd.h | 16 +++++++-
>  3 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 8300baedafd2..90ead90faf1d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c

> +/**
> + * usb_hcd_stop_endpoint - Halt USB EP transfers
> + * @udev: usb device
> + * @ep: usb ep to stop
> + *
> + * Stop pending transfers on a specific USB endpoint.
> + **/
> +int usb_hcd_stop_endpoint(struct usb_device *udev,
> +					struct usb_host_endpoint *ep)
> +{
> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +	int ret = 0;
> +
> +	if (hcd->driver->stop_endpoint)
> +		ret = hcd->driver->stop_endpoint(hcd, udev, ep);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_stop_endpoint);

You know, there already is a function that does this.  It's named 
usb_hcd_flush_endpoint().  No need to add another function that does the 
same thing.

Alan Stern
Krzysztof Kozlowski Dec. 26, 2022, 12:27 p.m. UTC | #9
On 24/12/2022 00:32, Wesley Cheng wrote:
> Add an example on enabling of USB offload for the Q6DSP.  The routing can
> be done by the mixer, which can pass the multimedia stream to the USB
> backend.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  .../devicetree/bindings/sound/qcom,sm8250.yaml      | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
> index 70080d04ddc9..60cd84e6727a 100644
> --- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
> +++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
> @@ -216,6 +216,19 @@ examples:
>                  sound-dai = <&vamacro 0>;
>              };
>          };
> +
> +        usb-dai-link {
> +            link-name = "USB Playback";
> +            cpu {
> +                sound-dai = <&q6afedai USB_RX>;

Hmm, that makes me wonder if you really tested the bindings before
sending? If yes, where is the USB_RX defined?

Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 26, 2022, 12:28 p.m. UTC | #10
On 24/12/2022 12:13, Dmitry Baryshkov wrote:
>> @@ -1468,6 +1469,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>          */
>>         tx_fifo_resize_max_num = 6;
>>
>> +       /* default to a single XHCI interrupter */
>> +       num_hc_interrupters = 1;
>> +
>>         dwc->maximum_speed = usb_get_maximum_speed(dev);
>>         dwc->max_ssp_rate = usb_get_maximum_ssp_rate(dev);
>>         dwc->dr_mode = usb_get_dr_mode(dev);
>> @@ -1511,6 +1515,12 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>                                 &tx_thr_num_pkt_prd);
>>         device_property_read_u8(dev, "snps,tx-max-burst-prd",
>>                                 &tx_max_burst_prd);
>> +       device_property_read_u8(dev, "snps,num-hc-interrupters",
>> +                               &num_hc_interrupters);
> 
> bindings change?

Undocumented bindings change :(

Best regards,
Krzysztof
Mark Brown Dec. 27, 2022, 2:36 p.m. UTC | #11
On Sat, Dec 24, 2022 at 07:45:38AM +0100, Greg KH wrote:
> On Fri, Dec 23, 2022 at 03:31:46PM -0800, Wesley Cheng wrote:

> > 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

> This looks to duplicate a bunch of the same things that a number of
> different google developers have posted recently.  Please work with them
> to come up with a unified set of patches that you all can agree with,
> AND get them to sign off on the changes before resubmitting them.

> This uncoordinated drip of patches from different people doing the same
> thing is almost impossible to review from our side, as I'm sure you can
> imagine.

I have to say this is the first I've heard of any such patches other
than from the Qualcomm people and I can't immediately see anything that
was on the list either, though I might be missing something since I
don't have the subject or anything.  If other people send things again
it's probably good to suggest they copy in audio people and lists.
Wesley Cheng Dec. 27, 2022, 9:07 p.m. UTC | #12
Hi Greg,

On 12/24/2022 12:59 AM, Greg KH wrote:
> On Fri, Dec 23, 2022 at 03:31:56PM -0800, 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 | 19 +++++++++++++++++++
>>   sound/usb/card.h |  3 +++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 396e5a34e23b..9b8d2ed308c8 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -133,6 +133,25 @@ int snd_usb_unregister_vendor_ops(void)
>>   }
>>   EXPORT_SYMBOL_GPL(snd_usb_unregister_vendor_ops);
>>   
>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>> +			struct snd_pcm_hw_params *params, int direction)
>> +{
>> +	struct snd_usb_stream *as;
>> +	struct snd_usb_substream *subs = NULL;
>> +	const struct audioformat *fmt;
>> +
>> +	if (usb_chip[card_idx] && enable[card_idx]) {
>> +		list_for_each_entry(as, &usb_chip[card_idx]->pcm_list, list) {
>> +			subs = &as->substream[direction];
>> +			fmt = find_substream_format(subs, params);
>> +			if (fmt)
>> +				return as;
>> +		}
>> +	}
> 
> Where is the locking here?  How can you walk a list that can be changed
> as you walk it?
> 
> And what about reference counting?  You are returning a pointer to a
> structure, who now "owns" it?  What happens if it is removed from the
> system after you return it?
> 
>> +	return 0;
> 
> Didn't sparse complain about this?  You can't return "0" as a pointer,
> it should be NULL.
> 
> Always run basic tools like sparse on code before submitting it so that
> we don't have to find errors like this.
> 

Got it...I didn't get a chance to run that, but will do it on future 
submissions.  Will also address the locking and pointer reference you 
mentioned.

Thanks
Wesley Cheng
Wesley Cheng Dec. 28, 2022, 8:31 p.m. UTC | #13
Hi Alan,

On 12/28/2022 7:16 AM, Alan Stern wrote:
> On Wed, Dec 28, 2022 at 09:59:03AM +0100, Oliver Neukum wrote:
>>
>>
>> On 27.12.22 22:07, Wesley Cheng wrote:
>>
>>>
>>> Hmmm...maybe I should change the name of the API then to avoid the confusion.  Yes, usb_hcd_flush_endpoint() does ensure that URBs submitted to the EP are stopped.  However, with this offloading concept, we aren't actually submitting URBs from the main processor, so the ep->urb_list will be empty.
>>>
>>> This means the usb_hcd_flush_endpoint() API won't actually do anything.  What we need is to ensure that we send a XHCI stop ep command to the controller.
>>
>> That is a concept specific to XHCI, yet you are adding a generic
>> API. The namin should reflect that. usb_quiesce_endpoint() ?
> 
> Or even xhci_send_stop_ep_cmd(), which is what the routine is intended
> to do.
> 

Just to clarify, you're talking about renaming the API that was added in 
the XHCI driver, correct?

Thanks
Wesley Cheng
Mark Brown Jan. 3, 2023, 5:46 p.m. UTC | #14
On Mon, Dec 26, 2022 at 01:27:21PM +0100, Krzysztof Kozlowski wrote:
> On 24/12/2022 00:32, Wesley Cheng wrote:

> > +            link-name = "USB Playback";
> > +            cpu {
> > +                sound-dai = <&q6afedai USB_RX>;

> Hmm, that makes me wonder if you really tested the bindings before
> sending? If yes, where is the USB_RX defined?

It was added in patch 2, it's in include/dt-bindings.
Pierre-Louis Bossart Jan. 4, 2023, 11:19 p.m. UTC | #15
On 12/23/22 17:31, 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.

It would be nice to clarify what you want to offload
a) audio data transfers for isoc ports
b) control for e.g. volume settings (those go to endpoint 0 IIRC)
c) Both?

This has a lot of implications on the design. ASoC/DPCM is mainly
intended for audio data transfers, control is a separate problem with
configurations handled with register settings or bus-specific commands.

> 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
> 
> 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...

You would still need some sort of mutual exclusion to make sure the isoc
endpoints are not used concurrently by the two cards. Relying on
userspace intelligence to enforce that exclusion is not safe IMHO.

Intel looked at this sort of offload support a while ago and our
directions were very different - for a variety of reasons USB offload is
enabled on Windows platforms but remains a TODO for Linux. Rather than
having two cards, you could have a single card and addition subdevices
that expose the paths through the DSP. The benefits were that there was
a single set of controls that userspace needed to know about, and volume
settings were the same no matter which path you used (legacy or
DSP-optimized paths). That's consistent with the directions to use 'Deep
Buffer' PCM paths for local playback, it's the same idea of reducing
power consumption with optimized routing.

Another point is that there may be cases where the DSP paths are not
available if the DSP memory and MCPS budget is exceeded. In those cases,
the DSP parts needs the ability to notify userspace that the legacy path
should be used.

Another case to handle is that some USB devices can handle way more data
than DSPs can chew, for example Pro audio boxes that can deal with 8ch
192kHz will typically use the legacy paths. Some also handle specific
formats such as DSD over PCM. So it's quite likely that PCM devices for
card0 and card1 above do NOT expose support for the same formats, or put
differently that only a subset of the USB device capabilities are
handled through the DSP.

And last, power optimizations with DSPs typically come from additional
latency helping put the SoC in low-power modes. That's not necessarily
ideal for all usages, e.g. for music recording and mixing I am not
convinced the DSP path would help at all.

> This feature was validated using:
> - tinymix: set/enable the multimedia path to route to USB backend
> - tinyplay: issue playback on platform card
Krzysztof Kozlowski Jan. 5, 2023, 6:09 p.m. UTC | #16
On 03/01/2023 18:46, Mark Brown wrote:
> On Mon, Dec 26, 2022 at 01:27:21PM +0100, Krzysztof Kozlowski wrote:
>> On 24/12/2022 00:32, Wesley Cheng wrote:
> 
>>> +            link-name = "USB Playback";
>>> +            cpu {
>>> +                sound-dai = <&q6afedai USB_RX>;
> 
>> Hmm, that makes me wonder if you really tested the bindings before
>> sending? If yes, where is the USB_RX defined?
> 
> It was added in patch 2, it's in include/dt-bindings.

Thanks, indeed, I was looking for another bindings patch but this was
squashed with a driver.

Best regards,
Krzysztof
Pierre-Louis Bossart Jan. 6, 2023, 3:57 p.m. UTC | #17
>> On 12/23/22 17:31, 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.
>>
>> It would be nice to clarify what you want to offload
>> a) audio data transfers for isoc ports
>> b) control for e.g. volume settings (those go to endpoint 0 IIRC)
>> c) Both?
>>
> 
> Thanks for sharing your experience, and inputs!
> 
> It would be the audio related endpoints only, so ISOC and potentially
> feedback ep.

That's good, that means there's a common basis for at least two separate
hardware implementations.
>> This has a lot of implications on the design. ASoC/DPCM is mainly
>> intended for audio data transfers, control is a separate problem with
>> configurations handled with register settings or bus-specific commands.
>>
> 
> Control would still be handled by the main processor.

Excellent, one more thing in common. Maintainers like this sort of
alignment :-)

>>> 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
>>>
>>> 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.

To the best of my knowledge this isn't needed on Intel platforms, but
that's something we would need to double-check.
>>> 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...
>>
>> You would still need some sort of mutual exclusion to make sure the isoc
>> endpoints are not used concurrently by the two cards. Relying on
>> userspace intelligence to enforce that exclusion is not safe IMHO.
>>
> 
> Sure, I think we can make the USB card as being used if the offloading
> path is currently being enabled.  Kernel could return an error to
> userspace when this situation happens.

It's problematic for servers such as PipeWire/PulseAudio that open all
possible PCMs to figure out what they support in terms of formats. I am
not sure we can enforce a user-space serialization when discovering
capabilities?

> 
>> Intel looked at this sort of offload support a while ago and our
>> directions were very different - for a variety of reasons USB offload is
>> enabled on Windows platforms but remains a TODO for Linux. Rather than
>> having two cards, you could have a single card and addition subdevices
>> that expose the paths through the DSP. The benefits were that there was
>> a single set of controls that userspace needed to know about, and volume
>> settings were the same no matter which path you used (legacy or
>> DSP-optimized paths). That's consistent with the directions to use 'Deep
>> Buffer' PCM paths for local playback, it's the same idea of reducing
>> power consumption with optimized routing.
>>
> 
> Volume control would still be done through the legacy path as mentioned
> above.  For example, if a USB headset w/ a HID interface exposed (for
> volume control) was connected, those HID events would be routed to
> userspace to adjust volume accordingly on the main processor. (although
> you're right about having separate controls still present - one for the
> ASoC card and another for USB card)

The two sets of controls implied by the use of two cards is really
problematic IMHO. This adds complexity for userspace to figure out that
the controls are really the same and synchronize/mirror changes.

The premise of offload is that it should really not get in the way of
user-experience, design constructs that result in delayed starts/stop,
changed volumes or quality differences should be avoided, or
users/distros will disable this optimization.

One card with additional DSP-based PCM devices seems simpler to me in
terms of usage, but it's not without technical challenges either: with
the use of the ASoC topology framework we only know what the DSP
supports when registering a card and probing the ASoC components.

The interaction between USB audio and ASoC would also be at least as
complicated as display audio, in that it needs to work no matter what
the probe order is, and even survive the Linux device/driver model
requirement that there are no timing dependencies in the driver
bind/unbind sequences.

>> Another point is that there may be cases where the DSP paths are not
>> available if the DSP memory and MCPS budget is exceeded. In those cases,
>> the DSP parts needs the ability to notify userspace that the legacy path
>> should be used.
> 
> If we ran into this scenario, the audio DSP AFE port start command would
> fail, and this would be propagated to the userspace entity.  It could
> then potentially re-route to the legacy/non-offload path.

'start' or 'open'? This is a rather important design difference. Usually
we try to make decisions in the .open or .hw_params stage. The 'start'
or 'trigger' are usually not meant to fail due to unavailable resources
in ALSA.
>> Another case to handle is that some USB devices can handle way more data
>> than DSPs can chew, for example Pro audio boxes that can deal with 8ch
>> 192kHz will typically use the legacy paths. Some also handle specific
>> formats such as DSD over PCM. So it's quite likely that PCM devices for
>> card0 and card1 above do NOT expose support for the same formats, or put
>> differently that only a subset of the USB device capabilities are
>> handled through the DSP.
> 
> Same as the above.  We have programmed the USB backend to support the
> profiles that the audio DSP can handle.  I assume if there was any other
> request, the userspace entity would fail the PCM open for that requested
> profile.

What's not clear to me is whether there's any matching process between
the DSP capabilities and what the USB device exposes? if the USB device
is already way more complicated that what the ASoC back-end can deal
with, why expose a card?

>> And last, power optimizations with DSPs typically come from additional
>> latency helping put the SoC in low-power modes. That's not necessarily
>> ideal for all usages, e.g. for music recording and mixing I am not
>> convinced the DSP path would help at all.
>>
> 
> That's true.  At the same time, this feature is more for power related
> benefits, not specifically for performance. (although we haven't seen
> any performance related issues w/ this approach on the audio profiles
> the DSP supports)  I think if its an audio profile that supports a high
> sample rate and large number of channels, then the DSP wouldn't be able
> to support it anyway, and userspace could still use the legacy path.
> This would allow for those high-performance audio devices to not be
> affected.

ok, we are aligned as well here. Excellent. With the on-going work to
introduce 'Deep Buffer' capabilities, we'll have a need to tag PCM
devices with a usage or 'modifier', or have this information in
UCM/topology. Logic will have to be introduced in userspace to use the
best routing, I would think this work can be reused for USB cases to
indicate the offload solution is geared to power optimizations.
Wesley Cheng Jan. 7, 2023, 12:46 a.m. UTC | #18
Hi Pierre,

On 1/6/2023 7:57 AM, Pierre-Louis Bossart wrote:
> 
>>> On 12/23/22 17:31, 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.
>>>
>>> It would be nice to clarify what you want to offload
>>> a) audio data transfers for isoc ports
>>> b) control for e.g. volume settings (those go to endpoint 0 IIRC)
>>> c) Both?
>>>
>>
>> Thanks for sharing your experience, and inputs!
>>
>> It would be the audio related endpoints only, so ISOC and potentially
>> feedback ep.
> 
> That's good, that means there's a common basis for at least two separate
> hardware implementations.
>>> This has a lot of implications on the design. ASoC/DPCM is mainly
>>> intended for audio data transfers, control is a separate problem with
>>> configurations handled with register settings or bus-specific commands.
>>>
>>
>> Control would still be handled by the main processor.
> 
> Excellent, one more thing in common. Maintainers like this sort of
> alignment :-)
> 
>>>> 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
>>>>
>>>> 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.
> 
> To the best of my knowledge this isn't needed on Intel platforms, but
> that's something we would need to double-check.

I think Mathias mentioned that he was looking into adding some XHCI 
secondary interrupter support as well.  However, it did have some 
slightly different requirements compared to what this offloading feature 
is trying to do.

I'll first have to split up the XHCI/HCD changes into separate parts 
(interrupter specific and offloading specific), and then I'll work with 
him to see what can be improved from there.

>>>> 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...
>>>
>>> You would still need some sort of mutual exclusion to make sure the isoc
>>> endpoints are not used concurrently by the two cards. Relying on
>>> userspace intelligence to enforce that exclusion is not safe IMHO.
>>>
>>
>> Sure, I think we can make the USB card as being used if the offloading
>> path is currently being enabled.  Kernel could return an error to
>> userspace when this situation happens.
> 
> It's problematic for servers such as PipeWire/PulseAudio that open all
> possible PCMs to figure out what they support in terms of formats. I am
> not sure we can enforce a user-space serialization when discovering
> capabilities?
> 

I see...I'm not too familiar yet with all the different implementations 
from userspace yet, so that is something I'll need to look up on the 
side.  Takashi, would you happen to have any inputs with regards to how 
flexible PCM device selection can be from the userspace level?  If the 
offload PCM device can't be supported, can it fallback to another PCM 
device?

>>
>>> Intel looked at this sort of offload support a while ago and our
>>> directions were very different - for a variety of reasons USB offload is
>>> enabled on Windows platforms but remains a TODO for Linux. Rather than
>>> having two cards, you could have a single card and addition subdevices
>>> that expose the paths through the DSP. The benefits were that there was
>>> a single set of controls that userspace needed to know about, and volume
>>> settings were the same no matter which path you used (legacy or
>>> DSP-optimized paths). That's consistent with the directions to use 'Deep
>>> Buffer' PCM paths for local playback, it's the same idea of reducing
>>> power consumption with optimized routing.
>>>
>>
>> Volume control would still be done through the legacy path as mentioned
>> above.  For example, if a USB headset w/ a HID interface exposed (for
>> volume control) was connected, those HID events would be routed to
>> userspace to adjust volume accordingly on the main processor. (although
>> you're right about having separate controls still present - one for the
>> ASoC card and another for USB card)
> 
> The two sets of controls implied by the use of two cards is really
> problematic IMHO. This adds complexity for userspace to figure out that
> the controls are really the same and synchronize/mirror changes.
>  > The premise of offload is that it should really not get in the way of
> user-experience, design constructs that result in delayed starts/stop,
> changed volumes or quality differences should be avoided, or
> users/distros will disable this optimization.
>

Makes sense.  I think in terms of controls, we know that for an USB 
audio device, anything will still be handled through the USB card. 
Again, since I'm not too familiar yet with all the userspace 
implementations, does it have mechanisms to treat the control and data 
interfaces separately?

> One card with additional DSP-based PCM devices seems simpler to me in
> terms of usage, but it's not without technical challenges either: with
> the use of the ASoC topology framework we only know what the DSP
> supports when registering a card and probing the ASoC components.
> 
> The interaction between USB audio and ASoC would also be at least as
> complicated as display audio, in that it needs to work no matter what
> the probe order is, and even survive the Linux device/driver model
> requirement that there are no timing dependencies in the driver
> bind/unbind sequences.
> 

Yes, this was my initial approach as well, but from the technical 
perspective it was very very messy, and potentially could have affected 
functionality on certain devices if not handled correctly.  I think the 
difficult part was that the USB SND framework itself is an independent 
entity, and it was tough to dissect the portions which created PCM/sound 
card devices.

I don't think that was something which would have gone well if 
introduced all at once.  It would require a lot of discussion before 
getting the proper implementation.  At least this series introduces the 
initial communication between ASoC and USB SND, and maybe as use cases 
become clearer we can always improve/build on top of it.

>>> Another point is that there may be cases where the DSP paths are not
>>> available if the DSP memory and MCPS budget is exceeded. In those cases,
>>> the DSP parts needs the ability to notify userspace that the legacy path
>>> should be used.
>>
>> If we ran into this scenario, the audio DSP AFE port start command would
>> fail, and this would be propagated to the userspace entity.  It could
>> then potentially re-route to the legacy/non-offload path.
> 
> 'start' or 'open'? This is a rather important design difference. Usually
> we try to make decisions in the .open or .hw_params stage. The 'start'
> or 'trigger' are usually not meant to fail due to unavailable resources
> in ALSA.

This happens during the .prepare() phase.

>>> Another case to handle is that some USB devices can handle way more data
>>> than DSPs can chew, for example Pro audio boxes that can deal with 8ch
>>> 192kHz will typically use the legacy paths. Some also handle specific
>>> formats such as DSD over PCM. So it's quite likely that PCM devices for
>>> card0 and card1 above do NOT expose support for the same formats, or put
>>> differently that only a subset of the USB device capabilities are
>>> handled through the DSP.
>>
>> Same as the above.  We have programmed the USB backend to support the
>> profiles that the audio DSP can handle.  I assume if there was any other
>> request, the userspace entity would fail the PCM open for that requested
>> profile.
> 
> What's not clear to me is whether there's any matching process between
> the DSP capabilities and what the USB device exposes? if the USB device
> is already way more complicated that what the ASoC back-end can deal
> with, why expose a card?
> 

That's something I thought was done by the ASoC core.  I can check that 
and see if that's the case.  There is a check added in hw_params of our 
ASoC component where we do query the USB audio descriptors to ensure 
that the PCM format being used is supported by the device.  I guess this 
is when the DSP capabilities are better than what the headset can 
support :).

>>> And last, power optimizations with DSPs typically come from additional
>>> latency helping put the SoC in low-power modes. That's not necessarily
>>> ideal for all usages, e.g. for music recording and mixing I am not
>>> convinced the DSP path would help at all.
>>>
>>
>> That's true.  At the same time, this feature is more for power related
>> benefits, not specifically for performance. (although we haven't seen
>> any performance related issues w/ this approach on the audio profiles
>> the DSP supports)  I think if its an audio profile that supports a high
>> sample rate and large number of channels, then the DSP wouldn't be able
>> to support it anyway, and userspace could still use the legacy path.
>> This would allow for those high-performance audio devices to not be
>> affected.
> 
> ok, we are aligned as well here. Excellent. With the on-going work to
> introduce 'Deep Buffer' capabilities, we'll have a need to tag PCM
> devices with a usage or 'modifier', or have this information in
> UCM/topology. Logic will have to be introduced in userspace to use the
> best routing, I would think this work can be reused for USB cases to
> indicate the offload solution is geared to power optimizations.

Great, I like that idea to see if we can help userspace choose the 
desired path based on what the overall system is looking for.  I wonder 
if that would also potentially help with some of the PCM device 
selection complexities you brought up as well.  If the system just wants 
best possible performance then it would just completely ignore the power 
optimized (offload) path for any device.

Thanks
Wesley Cheng