| Message ID | 20230126031424.14582-1-quic_wcheng@quicinc.com |
|---|---|
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
On 2023/1/29 15:09, Greg KH wrote: > On Sun, Jan 29, 2023 at 02:54:43PM +0800, Zhou Furong wrote: >> >> >>>> +void *snd_soc_usb_get_priv_data(struct device *usbdev) >>>> +{ >>>> + struct snd_soc_usb *ctx; >>>> + >>>> + if (!usbdev) >>>> + return NULL; >>> >>> How could usbdev ever be NULL? >> The method is exported to public, valid check should be reasonable >> as someone may call it by mistake > > We do not protect the kernel from itself like this, no need to check > things that should never happen. If the caller gets it wrong, their > code will break :) > > thanks, > > greg k-h Thank you Greg! This has been confused me for long time when I found Linux kernel don't check input even for public method.
Hi Mathias, On 1/26/2023 1:23 AM, Mathias Nyman wrote: > On 26.1.2023 5.14, Wesley Cheng wrote: >> 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 >> >> > > I'll submit the first three patches from that branch myself to usb-next, > might modify > them slightly. > Just need to make sure they don't cause regression. Those are changes I > want done anyway. > Sounds good! Thanks! >> Adjustments made to Mathias' changes: >> - Created xhci-intr.h to export/expose interrupter APIs versus >> exposing xhci.h. Do you think using the xhci-intr.h is a viable solution for class drivers to request for a secondary interrupter? >> 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) > > Was this solvable by first clearing high 32 bits and then low 32 bits? > During the freeing of the secondary interrupter, the SMMU fault wasn't resolvable with clearing the high bits first. This does somewhat give me the notion that the xHC is attempting to access the event ring base address every time the ERSTBA is written. I believe the hi-lo write didn't work, as this time we are zero'ing out the base address. (SMMU FAR=0x0) As stated in Table 5-40 in the XHCI spec, when we write a 0 to the secondary interrupter ERSTSZ, it should disable that event ring. In this case, do we really need to explicitly clear the base address register? If I don't clear the ERSTBA (during free), then I don't see a SMMU fault even after the event ring has been freed. (ie event ring memory has been unmapped from the SMMU) So this should mean the xHC hasn't attempted to access that unmapped region for the memory address stored in the ERSTBA. Likewise, we'll write the ERSTBA again during the alloc phase to a valid and mapped address. Thanks Wesley Cheng
Hi Krzysztof, On 1/26/2023 4:01 AM, Krzysztof Kozlowski wrote: > On 26/01/2023 04:14, Wesley Cheng wrote: >> Add a new definition for specifying how many XHCI secondary interrupters >> can be allocated. XHCI in general can potentially support up to 1024 >> interrupters, which some uses may want to limit depending on how many >> users utilize the interrupters. > > I cannot find in the code any user of this. Your next patch stores it, > but which other patch uses stored value? > > What I still don't get how is this exactly hardware property, not policy > or driver choice. > Sorry I must have missed that patchset when rebasing over Mathias' xHCI changes. It was there previously in my initial submission where the property is carried over into xhci-plat from dwc3/host.c. So the xHC controller has a HCSPARAMs field that defines the number of interrupters it can support. It does potentially have the capability of having 1024 interrupters. Each interrupter has to have its own set of interrupt register sets, which depending on the vendor implementing it could limit the maximum. For example, as stated below, DWC3 only allows for 8 interrupters to be allocated. The purpose for this property is to allow the user/driver to not have to allocate memory for supporting 1024 event rings, if they are only going to utilize one. Likewise, if the user attempts to allocate more than what is supported by the HW, then Mathias' SW will cross check to ensure that isn't allowed. (by checking the HCSPARAMs against the DT property below) >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> index 6d78048c4613..4faaec9655e0 100644 >> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> @@ -349,6 +349,18 @@ properties: >> items: >> enum: [1, 4, 8, 16, 32, 64, 128, 256] >> >> + snps,num-hc-interrupters: >> + description: >> + Defines the maximum number of XHCI host controller interrupters that can >> + be supported. The XHCI host controller has support to allocate multiple >> + event rings, which can be assigned to different clients/users. The DWC3 >> + controller has a maximum of 8 interrupters. If this is not defined then >> + the value will be defaulted to 1. This parameter is used only when >> + operating in host mode. >> + $ref: /schemas/types.yaml#/definitions/uint8 >> + minimum: 1 >> + maximum: 8 > > default: 1 > Got it. Thanks Wesley Cheng
Hi Pierre, On 1/26/2023 7:32 AM, Pierre-Louis Bossart wrote: > > > On 1/25/23 21:14, Wesley Cheng wrote: >> Some platforms may want to register its USB port to be handled by the ASoC >> framework. Audio playback/capture support is also handled entirely by the >> vendor ASoC drivers. > > Can you clarify what you mean by 'port'? > Ideally, port is intended to represent how many USB audio devices the audio DSP can support. >> +/** >> + * snd_soc_usb_add_port() - Add a USB backend port >> + * @dev: USB backend device >> + * @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, >> + 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; >> + >> + 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); > > Can a backend have more than one ports? > The intended model is one BE per usb device. > Is there any relationship between port and USB endpoint, and if yes > where is this determined? > Might need some more clarification on this question. I mean, whichever port the USB device is connected to will be the USB endpoint(s) being utilized. Maybe the confusion is in the "port" label itself? You can think of port meaning the same thing as a udev. (struct usb_device) >> + >> +/** >> + * snd_soc_usb_remove_port() - Remove a USB backend port >> + * @dev: USB backend device >> + * >> + * Remove a USB backend device from USB SND SOC. Memory is freed when USB >> + * backend is removed. >> + * >> + */ >> +int snd_soc_usb_remove_port(struct device *dev) >> +{ >> + struct snd_soc_usb *ctx, *tmp; >> + >> + mutex_lock(&ctx_mutex); >> + list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) { >> + if (ctx->dev == dev) { >> + list_del(&ctx->list); >> + break; >> + } >> + } >> + mutex_unlock(&ctx_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port); > > What happens if the ASoC driver probes/initialize AFTER the USB device > is plugged? > Good point, that's one thing I was seeing how we could address initially, but never got around to adding it. Currently the code would basically not enable the DAPM pins/path for that backend device. (when we probe the USB backend, we disable the USB_RX_BE pin) If the USB SND driver gets the connection before the USB BE is up, then q6usb_alsa_connection_cb() would not be called, which sets the USB_RX_BE pin state (to enable). Then when the Q6USB backend is probed, the USB_RX_BE pin state would be set to disabled. Will see if I can maybe cache the connection state somewhere and pass it along when the USB BE is up. > Likewise, can the ASoC driver be removed 'safely' with a fallback to > normal non-offloaded operation happening on remove? > As of now, there is no automatic fallback for that scenario. If I compile all our Q6 dai drivers as modules as well as the platform card, then we won't be able to individually remove ASoC component modules since they are being used by the platform soundcard device. The only way to remove the USB backend driver is first to remove the platform sound card, which will tear down the current audio session and remove the sound card that was created. >> + >> +/** >> + * 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. >> + * >> + */ >> +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); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect); >> + >> +/** >> + * snd_soc_usb_connect() - Notification of USB device connection >> + * @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); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect); > > Similar concern on connect/disconnect, does this assume any specific > order for the driver probe? I think the above explanation clarifies the order which is currently going to cause us to potentially miss a device connection. Thanks Wesley Cheng
Hi Pierre, On 1/26/2023 7:38 AM, Pierre-Louis Bossart wrote: > > > On 1/25/23 21:14, Wesley Cheng wrote: >> The QC ADSP is able to support USB playback endpoints, so that the main >> application processor can be placed into lower CPU power modes. This adds >> the required AFE port configurations and port start command to start an >> audio session. >> >> Specifically, the QC ADSP can support all potential endpoints that are >> exposed by the audio data interface. This includes, feedback endpoints >> (both implicit and explicit) as well as the isochronous (data) endpoints. >> The size of audio samples sent per USB frame (microframe) will be adjusted >> based on information received on the feedback endpoint. > > I think you meant "support all potential endpoint types" > > It's likely that some USB devices have more endpoints than what the DSP > can handle, no? > True, as we discussed before, we only handle the endpoints for the audio interface. Other endpoints, such as HID, or control is still handled by the main processor. > And that brings me back to the question: what is a port and the > relationship between port/backend/endpoints? > > Sorry for being picky on terminology, but if I learned something in days > in standardization it's that there shouldn't be any ambiguity on > concepts, otherwise everyone is lost at some point. > No worries, I can understand where you're coming from :). After re-reading some of the notations used, I can see where people may be confused. > >> static struct afe_port_map port_maps[AFE_PORT_MAX] = { >> + [USB_RX] = { AFE_PORT_ID_USB_RX, USB_RX, 1, 1}, >> [HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX, HDMI_RX, 1, 1}, >> [SLIMBUS_0_RX] = { AFE_PORT_ID_SLIMBUS_MULTI_CHAN_0_RX, >> SLIMBUS_0_RX, 1, 1}, > > And if I look here a port seems to be a very specific AFE concept > related to interface type? Do we even need to refer to a port in the USB > parts? > Well, this is a design specific to how the Q6 AFE is implemented. There is a concept for an AFE port to be opened. However, as mentioned earlier, the "port" term used in soc-usb should be more for how many USB devices can be supported. If there was a case the audio DSP would support more than one USB device, I believe another AFE port would need to be added. Thanks Wesley Cheng
On 1/30/23 16:54, Wesley Cheng wrote: > Hi Pierre, > > On 1/26/2023 7:38 AM, Pierre-Louis Bossart wrote: >> >> >> On 1/25/23 21:14, Wesley Cheng wrote: >>> The QC ADSP is able to support USB playback endpoints, so that the main >>> application processor can be placed into lower CPU power modes. This >>> adds >>> the required AFE port configurations and port start command to start an >>> audio session. >>> >>> Specifically, the QC ADSP can support all potential endpoints that are >>> exposed by the audio data interface. This includes, feedback endpoints >>> (both implicit and explicit) as well as the isochronous (data) >>> endpoints. >>> The size of audio samples sent per USB frame (microframe) will be >>> adjusted >>> based on information received on the feedback endpoint. >> >> I think you meant "support all potential endpoint types" >> >> It's likely that some USB devices have more endpoints than what the DSP >> can handle, no? >> > > True, as we discussed before, we only handle the endpoints for the audio > interface. Other endpoints, such as HID, or control is still handled by > the main processor. The number of isoc/audio endpoints can be larger than 1 per direction, it's not uncommon for a USB device to have multiple connectors on the front side for instruments, mics, monitor speakers, you name it. Just google 'motu' or 'rme usb' and you'll see examples of USB devices that are very different from plain vanilla headsets. >> And that brings me back to the question: what is a port and the >> relationship between port/backend/endpoints? >> >> Sorry for being picky on terminology, but if I learned something in days >> in standardization it's that there shouldn't be any ambiguity on >> concepts, otherwise everyone is lost at some point. >> > > No worries, I can understand where you're coming from :). After > re-reading some of the notations used, I can see where people may be > confused. > >> >>> static struct afe_port_map port_maps[AFE_PORT_MAX] = { >>> + [USB_RX] = { AFE_PORT_ID_USB_RX, USB_RX, 1, 1}, >>> [HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX, HDMI_RX, 1, 1}, >>> [SLIMBUS_0_RX] = { AFE_PORT_ID_SLIMBUS_MULTI_CHAN_0_RX, >>> SLIMBUS_0_RX, 1, 1}, >> >> And if I look here a port seems to be a very specific AFE concept >> related to interface type? Do we even need to refer to a port in the USB >> parts? >> > > Well, this is a design specific to how the Q6 AFE is implemented. There > is a concept for an AFE port to be opened. However, as mentioned > earlier, the "port" term used in soc-usb should be more for how many USB > devices can be supported. > > If there was a case the audio DSP would support more than one USB > device, I believe another AFE port would need to be added. would the suggested infrastructure work though, even if the DSP could deal with multiple endpoints on different devices ? You have static mutexes and ops, can that scale to more than one USB device?
On 1/31/23 20:40, Wesley Cheng wrote: > Hi Pierre, > > On 1/30/2023 3:59 PM, Pierre-Louis Bossart wrote: >> >> >> On 1/30/23 16:54, Wesley Cheng wrote: >>> Hi Pierre, >>> >>> On 1/26/2023 7:38 AM, Pierre-Louis Bossart wrote: >>>> >>>> >>>> On 1/25/23 21:14, Wesley Cheng wrote: >>>>> The QC ADSP is able to support USB playback endpoints, so that the >>>>> main >>>>> application processor can be placed into lower CPU power modes. This >>>>> adds >>>>> the required AFE port configurations and port start command to >>>>> start an >>>>> audio session. >>>>> >>>>> Specifically, the QC ADSP can support all potential endpoints that are >>>>> exposed by the audio data interface. This includes, feedback >>>>> endpoints >>>>> (both implicit and explicit) as well as the isochronous (data) >>>>> endpoints. >>>>> The size of audio samples sent per USB frame (microframe) will be >>>>> adjusted >>>>> based on information received on the feedback endpoint. >>>> >>>> I think you meant "support all potential endpoint types" >>>> >>>> It's likely that some USB devices have more endpoints than what the DSP >>>> can handle, no? >>>> >>> >>> True, as we discussed before, we only handle the endpoints for the audio >>> interface. Other endpoints, such as HID, or control is still handled by >>> the main processor. >> >> The number of isoc/audio endpoints can be larger than 1 per direction, >> it's not uncommon for a USB device to have multiple connectors on the >> front side for instruments, mics, monitor speakers, you name it. Just >> google 'motu' or 'rme usb' and you'll see examples of USB devices that >> are very different from plain vanilla headsets. >> > > Thanks for the reference. > > I tried to do some research on the RME USB audio devices, and they > mentioned that they do have a "class compliant mode," which is for > compatibility w/ Linux hosts. I didn't see a vendor specific USB SND > driver matching the USB VID/PID either, so I am assuming that it uses > the USB SND driver as is.(and that Linux doesn't currently support their > vendor specific mode) In that case, the device should conform to the > UAC2.0 spec (same statement seen on UAC3.0), which states in Section > 4.9.1 Standard AS Interface Descriptor Table 4-26: > > "4 bNumEndpoints 1 Number Number of endpoints used by this > interface (excluding endpoint 0). Must be > either 0 (no data endpoint), 1 (data > endpoint) or 2 (data and explicit feedback > endpoint)." > > So each audio streaming interface should only have 1 data and > potentially 1 feedback. However, this device does expose a large number > of channels (I saw up to 18 channels), which the USB backend won't be > able to support. I still need to check how ASoC behaves if I pass in a > profile that the backend can't support. > > Maybe in the non-class compliant/vendor based class driver, they have > the support for multiple EPs per data interface? I don't have one of > these devices on hand, so I can't confirm that. Look at Figure 3-1 in the UAC2 spec, it shows it's perfectly legal to have multiple Audio Streaming interfaces - but one Audio Control interface only. The fact that there is a restriction to 1 or 2 endpoints per Audio Streaming interface does not really matter if in the end there are multiple endpoints and concurrent isoc transfers happening to/from the same USB device.
Hi Greg, On 1/28/2023 5:26 AM, Greg KH wrote: > On Wed, Jan 25, 2023 at 07:14:09PM -0800, Wesley Cheng wrote: >> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h >> new file mode 100644 >> index 000000000000..ec422a8a834f >> --- /dev/null >> +++ b/include/sound/soc-usb.h >> @@ -0,0 +1,33 @@ >> +/* SPDX-License-Identifier: GPL-2.0 >> + * >> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. > > It is now 2023 :) > Sorry for the delayed response. Will change the year for all the patches...time flies. >> + */ >> + >> +#ifndef __LINUX_SND_SOC_USB_H >> +#define __LINUX_SND_SOC_USB_H >> + >> +/** >> + * struct snd_soc_usb >> + * @component - Reference to DAPM component >> + * @connection_status_cb - callback to notify connection events >> + * @priv_data - vendor data > > You do not document all items in the structure so you will get build > warnings :( > > And what exactly is "vendor data"? You use that term in a few places in > this series, there is no such thing as a "vendor" in the kernel. This > could be a device or driver specific data, but not a "vendor". > The term vendor data can be renamed to something else. It essentially signifies that each platform can potentially have a different implementation of how these callbacks behave. It makes sense to rename it into driver specific data, since it depends on how the offload driver is added. >> --- /dev/null >> +++ b/sound/soc/soc-usb.c >> @@ -0,0 +1,202 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> +#include <linux/of.h> >> +#include <linux/usb.h> >> +#include <sound/soc.h> >> +#include <sound/soc-usb.h> >> +#include "../usb/card.h" >> + >> +static DEFINE_MUTEX(ctx_mutex); >> +static LIST_HEAD(usb_ctx_list); > > What is this a list of? Why a list? This should be dynamic and tied to > the device itself somehow, not a separate list you have to walk. > This is a list of USB backends that have been registered. At the moment we only have one USB backend, as the audio DSP only supports playback on a single device through the offload path (on our chipset). Potentially, if there are other platforms that can support multiple, they can register several USB backends to control each offload path accordingly. It was difficult to tie the "struct snd_soc_usb" into a device, because of how different the device lifetime is for the USB audio device (udev) and the USB backend (one is dynamically created/freed based on USB device plugged into the port, the other exists until the backend is removed), and the fact that communication has to happen both ways. This warrented a need to have this structure exist as a separate entity, hence the reason why I went with a list. >> + >> +#define for_each_usb_ctx(ctx) \ >> + list_for_each_entry(ctx, &usb_ctx_list, list) > > No need for a #define like this, just spell it out. > > Sure. >> + >> +static struct device_node *snd_soc_find_phandle(struct device *dev) >> +{ >> + struct device_node *node; >> + >> + node = of_parse_phandle(dev->of_node, "usb-soc-be", 0); >> + if (!node) >> + return ERR_PTR(-ENODEV); >> + >> + return node; >> +} >> + >> +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev) >> +{ >> + struct device_node *node; >> + struct snd_soc_usb *ctx = NULL; >> + >> + node = snd_soc_find_phandle(dev); >> + if (IS_ERR(node)) >> + return NULL; >> + >> + mutex_lock(&ctx_mutex); >> + for_each_usb_ctx(ctx) { >> + 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 >> + * @usbdev: USB bus sysdev >> + * >> + * Fetch the private data stored in the USB SND SOC structure. This is >> + * intended to be called by the USB offloading class driver, in order to >> + * attain parameters about the USB backend device. >> + * >> + */ >> +void *snd_soc_usb_get_priv_data(struct device *usbdev) >> +{ >> + struct snd_soc_usb *ctx; >> + >> + if (!usbdev) >> + return NULL; > > How could usbdev ever be NULL? > >> + >> + ctx = snd_soc_find_usb_ctx(usbdev); >> + >> + return ctx ? ctx->priv_data : NULL; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data); >> + >> +/** >> + * snd_soc_usb_set_priv_data() - Set private data stored >> + * @dev: USB backend device >> + * @priv: private data to store >> + * >> + * Save data describing the USB backend device parameters. This is intended >> + * to be called by the ASoC USB backend driver. >> + * >> + */ >> +void snd_soc_usb_set_priv_data(struct device *dev, void *priv) >> +{ >> + struct snd_soc_usb *ctx; > > > Why does this function take a "struct device" but the get function take > a USB device? > I can modify that. It was done this way with the intention to not have the USB SND offload driver worry about finding the USB backend device. But in a way, it does indirectly place a design where the USB SND offload is the consumer of the platform data, and the USB backend is the source. >> + >> + mutex_lock(&ctx_mutex); >> + for_each_usb_ctx(ctx) { >> + if (dev->of_node == ctx->dev->of_node) { >> + ctx->priv_data = priv; >> + break; >> + } >> + } >> + mutex_unlock(&ctx_mutex); >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_set_priv_data); >> + >> +/** >> + * snd_soc_usb_add_port() - Add a USB backend port >> + * @dev: USB backend device >> + * @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, >> + 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; >> + >> + mutex_lock(&ctx_mutex); >> + list_add_tail(&usb->list, &usb_ctx_list); >> + mutex_unlock(&ctx_mutex); > > Again, why a list? > > >> + >> + return usb; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port); >> + >> +/** >> + * snd_soc_usb_remove_port() - Remove a USB backend port >> + * @dev: USB backend device >> + * >> + * Remove a USB backend device from USB SND SOC. Memory is freed when USB >> + * backend is removed. >> + * >> + */ >> +int snd_soc_usb_remove_port(struct device *dev) >> +{ >> + struct snd_soc_usb *ctx, *tmp; >> + >> + mutex_lock(&ctx_mutex); >> + list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) { >> + if (ctx->dev == dev) { >> + list_del(&ctx->list); >> + break; >> + } >> + } >> + mutex_unlock(&ctx_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_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. >> + * >> + */ >> +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); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect); >> + >> +/** >> + * snd_soc_usb_connect() - Notification of USB device connection >> + * @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); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect); > > Meta-comment, why are all of these in the sound directory? They are > only operating on USB devices, nothing else. So why here? > The USB SND exists in the sound directory as well as the ASoC framework. The main goal of this is to abstract USB SND to components that ASoC requires, versus having to expose all of USB SND. Thanks Wesley Cheng
Hi Pierre, On 2/7/2023 5:29 AM, Pierre-Louis Bossart wrote: > > > On 2/6/23 19:15, Wesley Cheng wrote: >> Hi Pierre, >> >> On 1/26/2023 8:12 AM, Pierre-Louis Bossart wrote: >>> >>> >>> On 1/25/23 21:14, Wesley Cheng wrote: >>>> With USB audio offloading, an audio session is started from the ASoC >>>> platform sound card and PCM devices. Likewise, the USB SND path is >>>> still >>>> readily available for use, in case the non-offload path is desired. In >>>> order to prevent the two entities from attempting to use the USB bus, >>>> introduce a flag that determines when either paths are in use. >>>> >>>> If a PCM device is already in use, the check will return an error to >>>> userspace notifying that the stream is currently busy. This ensures >>>> that >>>> only one path is using the USB substream. >>> >>> It's good to maintain mutual exclusion, but it's still very hard for an >>> application to figure out which card can be used when. >>> >>> Returning -EBUSY is not super helpful. There should be something like a >>> notification or connection status so that routing decisions can be made >>> without trial-and-error. >>> >> >> The USB offload driver does have access to the USB substream that is >> being utilized/offloaded. Maybe in addition to this check, we can also >> set the PCM runtime state as well (for that particular substream)? That >> way userspace can fetch information about if the stream is busy or not. > > You're missing the point. When a card is exposed but the PCM devices may > or may not be usable (consuming data with no sound rendered or returning > an error), it's much better to provide a clear connection status to > userspace. > > Let me give you an example. Intel drivers can expose 3 HDMI/DP PCM > devices. Userspace has no idea which one to use, so there's a jack > control that tells userspace whether there is a receiver connected so > that the audio server can use the relevant PCM device. > > Audio routing based on trial and error is really problematic, errors can > happen but they should be exceptional (e.g. xruns), not a means of > driver-userspace communication on the device status. Thanks for clarifying. The example helped me understand a bit more on how the potential use of the SND control interface. Since we're dealing with multiple sound cards here (platform sound card (offload) and USB SND card (legacy)), what do you think about creating a SND control on both the USB backend (platform card) and the USB SND card listing the PCM device status? That way at least userspace can have the information about which PCM dev (USB substream) is available (and not offloaded, or vice versa). So the USB SND control will contain the PCM devices (exposed by the card) and if any are offloaded (if so mark them as unavailable). Likewise, for the USB backend, if the legacy path is being used, mark them as unavailable for offloading. Thanks Wesley Cheng
On 2/11/23 03:52, Wesley Cheng wrote: > Hi Pierre, > > On 2/7/2023 5:29 AM, Pierre-Louis Bossart wrote: >> >> >> On 2/6/23 19:15, Wesley Cheng wrote: >>> Hi Pierre, >>> >>> On 1/26/2023 8:12 AM, Pierre-Louis Bossart wrote: >>>> >>>> >>>> On 1/25/23 21:14, Wesley Cheng wrote: >>>>> With USB audio offloading, an audio session is started from the ASoC >>>>> platform sound card and PCM devices. Likewise, the USB SND path is >>>>> still >>>>> readily available for use, in case the non-offload path is >>>>> desired. In >>>>> order to prevent the two entities from attempting to use the USB bus, >>>>> introduce a flag that determines when either paths are in use. >>>>> >>>>> If a PCM device is already in use, the check will return an error to >>>>> userspace notifying that the stream is currently busy. This ensures >>>>> that >>>>> only one path is using the USB substream. >>>> >>>> It's good to maintain mutual exclusion, but it's still very hard for an >>>> application to figure out which card can be used when. >>>> >>>> Returning -EBUSY is not super helpful. There should be something like a >>>> notification or connection status so that routing decisions can be made >>>> without trial-and-error. >>>> >>> >>> The USB offload driver does have access to the USB substream that is >>> being utilized/offloaded. Maybe in addition to this check, we can also >>> set the PCM runtime state as well (for that particular substream)? That >>> way userspace can fetch information about if the stream is busy or not. >> >> You're missing the point. When a card is exposed but the PCM devices may >> or may not be usable (consuming data with no sound rendered or returning >> an error), it's much better to provide a clear connection status to >> userspace. >> >> Let me give you an example. Intel drivers can expose 3 HDMI/DP PCM >> devices. Userspace has no idea which one to use, so there's a jack >> control that tells userspace whether there is a receiver connected so >> that the audio server can use the relevant PCM device. >> >> Audio routing based on trial and error is really problematic, errors can >> happen but they should be exceptional (e.g. xruns), not a means of >> driver-userspace communication on the device status. > > Thanks for clarifying. The example helped me understand a bit more on > how the potential use of the SND control interface. Since we're dealing > with multiple sound cards here (platform sound card (offload) and USB > SND card (legacy)), what do you think about creating a SND control on > both the USB backend (platform card) and the USB SND card listing the > PCM device status? > > That way at least userspace can have the information about which PCM dev > (USB substream) is available (and not offloaded, or vice versa). So the > USB SND control will contain the PCM devices (exposed by the card) and > if any are offloaded (if so mark them as unavailable). Likewise, for > the USB backend, if the legacy path is being used, mark them as > unavailable for offloading. We definitively need a control to indicate that a PCM offload device is available or not. There's still a very large open with the notion of having separate cards for the same audio device. Not only would it duplicate the control parts for e.g. volume control, but it would introduce the need to tag devices across two cards are being the same physical device. I still think the least-bad option is to have a single card and an optional PCM device for offload.
Hi Pierre, On 2/13/2023 7:22 AM, Pierre-Louis Bossart wrote: > > > On 2/11/23 03:52, Wesley Cheng wrote: >> Hi Pierre, >> >> On 2/7/2023 5:29 AM, Pierre-Louis Bossart wrote: >>> >>> >>> On 2/6/23 19:15, Wesley Cheng wrote: >>>> Hi Pierre, >>>> >>>> On 1/26/2023 8:12 AM, Pierre-Louis Bossart wrote: >>>>> >>>>> >>>>> On 1/25/23 21:14, Wesley Cheng wrote: >>>>>> With USB audio offloading, an audio session is started from the ASoC >>>>>> platform sound card and PCM devices. Likewise, the USB SND path is >>>>>> still >>>>>> readily available for use, in case the non-offload path is >>>>>> desired. In >>>>>> order to prevent the two entities from attempting to use the USB bus, >>>>>> introduce a flag that determines when either paths are in use. >>>>>> >>>>>> If a PCM device is already in use, the check will return an error to >>>>>> userspace notifying that the stream is currently busy. This ensures >>>>>> that >>>>>> only one path is using the USB substream. >>>>> >>>>> It's good to maintain mutual exclusion, but it's still very hard for an >>>>> application to figure out which card can be used when. >>>>> >>>>> Returning -EBUSY is not super helpful. There should be something like a >>>>> notification or connection status so that routing decisions can be made >>>>> without trial-and-error. >>>>> >>>> >>>> The USB offload driver does have access to the USB substream that is >>>> being utilized/offloaded. Maybe in addition to this check, we can also >>>> set the PCM runtime state as well (for that particular substream)? That >>>> way userspace can fetch information about if the stream is busy or not. >>> >>> You're missing the point. When a card is exposed but the PCM devices may >>> or may not be usable (consuming data with no sound rendered or returning >>> an error), it's much better to provide a clear connection status to >>> userspace. >>> >>> Let me give you an example. Intel drivers can expose 3 HDMI/DP PCM >>> devices. Userspace has no idea which one to use, so there's a jack >>> control that tells userspace whether there is a receiver connected so >>> that the audio server can use the relevant PCM device. >>> >>> Audio routing based on trial and error is really problematic, errors can >>> happen but they should be exceptional (e.g. xruns), not a means of >>> driver-userspace communication on the device status. >> >> Thanks for clarifying. The example helped me understand a bit more on >> how the potential use of the SND control interface. Since we're dealing >> with multiple sound cards here (platform sound card (offload) and USB >> SND card (legacy)), what do you think about creating a SND control on >> both the USB backend (platform card) and the USB SND card listing the >> PCM device status? >> >> That way at least userspace can have the information about which PCM dev >> (USB substream) is available (and not offloaded, or vice versa). So the >> USB SND control will contain the PCM devices (exposed by the card) and >> if any are offloaded (if so mark them as unavailable). Likewise, for >> the USB backend, if the legacy path is being used, mark them as >> unavailable for offloading. > > We definitively need a control to indicate that a PCM offload device is > available or not. > There's still a very large open with the notion of having separate cards > for the same audio device. Not only would it duplicate the control parts > for e.g. volume control, but it would introduce the need to tag devices > across two cards are being the same physical device. The volume control would still be done through the card that is exposed by the USB SND card (even for the offload path)[no vol control option for the USB device on the platform card]. In the last discussion, you did mention that maybe we can tag the offload path as the "power saving" option for a particular USB stream. Although I'm not sure how intricate the logic is, but if userspace marks to use the power saving path, then would it already know which card and PCM devices are involved? Although, that part is missing, ie to select the card and pcm device that we want to offload. It may be possible to do this with another control on the USB ASoC backend driver. I believe the audio DSP can support device selection. > I still think the least-bad option is to have a single card and an > optional PCM device for offload. This is most likely the end goal, but as mentioned previously, its going to be a large effort to slowly decouple some of the PCM related operations from USB SND. IMO, that would most likely be another significant patch series in itself. Thanks Wesley Cheng
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. 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 Mathias Nyman (4): xhci: fix event ring segment table related masks and variables in header xhci: remove xhci_test_trb_in_td_math early development check xhci: Refactor interrupter code for initial multi interrupter support. xhci: Add support to allocate several interrupters Wesley Cheng (18): usb: xhci: Add XHCI APIs to support USB offloading usb: host: xhci-mem: Cleanup pending secondary event ring events ASoC: Add SOC USB APIs for adding an USB backend ASoC: dt-bindings: 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 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 .../bindings/sound/qcom,q6usb-dais.yaml | 55 + .../bindings/sound/qcom,sm8250.yaml | 13 + .../devicetree/bindings/usb/snps,dwc3.yaml | 12 + drivers/usb/dwc3/core.c | 12 + drivers/usb/dwc3/core.h | 2 + drivers/usb/dwc3/host.c | 5 +- drivers/usb/host/xhci-debugfs.c | 2 +- drivers/usb/host/xhci-mem.c | 471 +++-- drivers/usb/host/xhci-ring.c | 68 +- drivers/usb/host/xhci.c | 199 +- drivers/usb/host/xhci.h | 81 +- .../sound/qcom,q6dsp-lpass-ports.h | 1 + include/linux/usb/xhci-intr.h | 94 + include/sound/pcm_params.h | 4 + include/sound/q6usboffload.h | 20 + include/sound/soc-usb.h | 36 + sound/core/oss/pcm_oss.c | 58 - sound/core/pcm_lib.c | 65 + sound/soc/Makefile | 2 +- sound/soc/qcom/Kconfig | 4 + sound/soc/qcom/qdsp6/Makefile | 1 + sound/soc/qcom/qdsp6/q6afe-dai.c | 48 + sound/soc/qcom/qdsp6/q6afe.c | 185 +- 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 | 234 +++ sound/soc/soc-usb.c | 215 ++ sound/usb/Kconfig | 14 + sound/usb/Makefile | 2 +- sound/usb/card.c | 58 + sound/usb/card.h | 29 + sound/usb/endpoint.c | 2 + sound/usb/helper.c | 1 + sound/usb/pcm.c | 28 +- sound/usb/pcm.h | 12 + sound/usb/qcom/Makefile | 2 + sound/usb/qcom/qc_audio_offload.c | 1789 +++++++++++++++++ sound/usb/qcom/usb_audio_qmi_v01.c | 892 ++++++++ sound/usb/qcom/usb_audio_qmi_v01.h | 162 ++ 41 files changed, 4546 insertions(+), 410 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml create mode 100644 include/linux/usb/xhci-intr.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