| Message ID | 20230126031424.14582-1-quic_wcheng@quicinc.com |
|---|---|
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
On Wed, Jan 25, 2023 at 07:14:04PM -0800, Wesley Cheng wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Time to remove this test trb in td math check that was added > in early stage of xhci driver deveopment. > > It verified that the size, alignment and boundaries of the event and > command rings the driver itself allocated are correct. > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- Note, if you pass on a patch from someone else, you HAVE to also sign-off on it as well. For that reason alone this series would have to be rejected :( thanks, greg k-h
On 26/01/2023 04:14, Wesley Cheng wrote: > Q6DSP supports handling of USB playback audio data if USB audio offloading > is enabled. Add a new definition for the USB_RX AFE port, which is > referenced when the AFE port is started. Subject prefix: ASoC: dt-bindings: qcom,q6dsp-lpass-ports: because you are not adding USB_RX port to all bindings in ASoC. With subject fixes: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 26/01/2023 04:14, Wesley Cheng wrote: > Add a dt-binding to describe the definition of enabling the Q6 USB backend > device for audio offloading. The node carries information, which is passed > along to the QC USB SND class driver counterpart. These parameters will be > utilized during QMI stream enable requests. Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > .../bindings/sound/qcom,q6usb-dais.yaml | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > > diff --git a/Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml b/Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > new file mode 100644 > index 000000000000..e24b4d52fa7e > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > @@ -0,0 +1,55 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/qcom,q6usb-dais.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm ASoC USB backend DAI > + > +maintainers: > + - Wesley Cheng <quic_wcheng@quicinc.com> > + > +description: > + The Q6USB backend is a supported AFE port on the Q6DSP. This backend > + driver will communicate the required settings to the QC USB SND class > + driver for properly enabling the audio stream. Parameters defined > + under this node will carry settings, which will be passed along during > + the QMI stream enable request. > + > +properties: > + compatible: > + enum: > + - qcom,q6usb-dais > + > + iommus: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 1 > + > + qcom,usb-audio-stream-id: > + description: > + SID for the Q6DSP processor for IOMMU mapping. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + qcom,usb-audio-intr-num: > + description: > + Desired XHCI interrupter number to use. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > +required: > + - compatible > + - '#sound-dai-cells' Use consistent quotes - either " or ' > + - qcom,usb-audio-intr-num > + > +additionalProperties: false > + > +examples: > + - | > + usbdai: usbd { Generic node name, so: dais Drop also label, not needed/used in example. > + compatible = "qcom,q6usb-dais"; > + #sound-dai-cells = <1>; > + iommus = <&apps_smmu 0x180f 0x0>; > + qcom,usb-audio-stream-id = <0xf>; > + qcom,usb-audio-intr-num = <2>; > + }; Best regards, Krzysztof
Thanks Wesley for the work, Minor nits. On 26/01/2023 03: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. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/soc/qcom/qdsp6/q6afe-dai.c | 48 ++++++ > 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 + > 6 files changed, 308 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c b/sound/soc/qcom/qdsp6/q6afe-dai.c > index 8bb7452b8f18..0773a0882d9b 100644 > --- a/sound/soc/qcom/qdsp6/q6afe-dai.c > +++ b/sound/soc/qcom/qdsp6/q6afe-dai.c > @@ -111,6 +111,40 @@ static int q6hdmi_hw_params(struct snd_pcm_substream *substream, > return 0; > } > <--- > +static int q6usb_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + struct q6afe_dai_data *dai_data = dev_get_drvdata(dai->dev); > + int channels = params_channels(params); > + int rate = params_rate(params); > + struct q6afe_usb_cfg *usb = &dai_data->port_config[dai->id].usb_audio; > + > + usb->sample_rate = rate; > + usb->num_channels = channels; > + > + switch (params_format(params)) { > + case SNDRV_PCM_FORMAT_U16_LE: > + case SNDRV_PCM_FORMAT_S16_LE: > + case SNDRV_PCM_FORMAT_SPECIAL: > + usb->bit_width = 16; > + break; > + case SNDRV_PCM_FORMAT_S24_LE: > + case SNDRV_PCM_FORMAT_S24_3LE: > + usb->bit_width = 24; > + break; > + case SNDRV_PCM_FORMAT_S32_LE: > + usb->bit_width = 32; > + break; > + default: > + dev_err(dai->dev, "%s: invalid format %d\n", > + __func__, params_format(params)); > + return -EINVAL; > + } > + > + return 0; > +} --> This one looks like duplicate of q6slim_hw_params, you could probably use it. > + > static int q6i2s_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params, > struct snd_soc_dai *dai) > @@ -411,6 +445,10 @@ static int q6afe_dai_prepare(struct snd_pcm_substream *substream, > q6afe_cdc_dma_port_prepare(dai_data->port[dai->id], > &dai_data->port_config[dai->id].dma_cfg); > break; > + case USB_RX: > + q6afe_usb_port_prepare(dai_data->port[dai->id], > + &dai_data->port_config[dai->id].usb_audio); indentation. > + break; > default: > return -EINVAL; > } > @@ -495,6 +533,8 @@ static int q6afe_mi2s_set_sysclk(struct snd_soc_dai *dai, > } > > static const struct snd_soc_dapm_route q6afe_dapm_routes[] = { > + /* USB playback AFE port receives data for playback, hence use the RX port */ > + {"USB Playback", NULL, "USB_RX"}, Normally we add new entries at the end of this list. > {"HDMI Playback", NULL, "HDMI_RX"}, > {"Display Port Playback", NULL, "DISPLAY_PORT_RX"}, > {"Slimbus Playback", NULL, "SLIMBUS_0_RX"}, > @@ -639,6 +679,12 @@ static const struct snd_soc_dapm_route q6afe_dapm_routes[] = { > {"RX_CODEC_DMA_RX_7 Playback", NULL, "RX_CODEC_DMA_RX_7"}, > }; > > +static const struct snd_soc_dai_ops q6usb_ops = { > + .prepare = q6afe_dai_prepare, > + .hw_params = q6usb_hw_params, > + .shutdown = q6afe_dai_shutdown, > +}; > + > static const struct snd_soc_dai_ops q6hdmi_ops = { > .prepare = q6afe_dai_prepare, > .hw_params = q6hdmi_hw_params, > @@ -703,6 +749,7 @@ static int msm_dai_q6_dai_remove(struct snd_soc_dai *dai) > } > > static const struct snd_soc_dapm_widget q6afe_dai_widgets[] = { > + SND_SOC_DAPM_AIF_IN("USB_RX", NULL, 0, SND_SOC_NOPM, 0, 0), same here. > SND_SOC_DAPM_AIF_IN("HDMI_RX", NULL, 0, SND_SOC_NOPM, 0, 0), > SND_SOC_DAPM_AIF_IN("SLIMBUS_0_RX", NULL, 0, SND_SOC_NOPM, 0, 0), > SND_SOC_DAPM_AIF_IN("SLIMBUS_1_RX", NULL, 0, SND_SOC_NOPM, 0, 0), > @@ -1068,6 +1115,7 @@ static int q6afe_dai_dev_probe(struct platform_device *pdev) > cfg.q6i2s_ops = &q6i2s_ops; > cfg.q6tdm_ops = &q6tdm_ops; > cfg.q6dma_ops = &q6dma_ops; > + cfg.q6usb_ops = &q6usb_ops; > dais = q6dsp_audio_ports_set_config(dev, &cfg, &num_dais); > > return devm_snd_soc_register_component(dev, &q6afe_dai_component, dais, num_dais); > diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c > index 919e326b9462..ca799fc3820e 100644 > --- a/sound/soc/qcom/qdsp6/q6afe.c > +++ b/sound/soc/qcom/qdsp6/q6afe.c > @@ -34,6 +34,8 @@ > #define AFE_MODULE_TDM 0x0001028A > > #define AFE_PARAM_ID_CDC_SLIMBUS_SLAVE_CFG 0x00010235 > +#define AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS 0x000102A5 > +#define AFE_PARAM_ID_USB_AUDIO_DEV_LPCM_FMT 0x000102AA > > #define AFE_PARAM_ID_LPAIF_CLK_CONFIG 0x00010238 > #define AFE_PARAM_ID_INT_DIGITAL_CDC_CLK_CONFIG 0x00010239 > @@ -43,6 +45,7 @@ > #define AFE_PARAM_ID_TDM_CONFIG 0x0001029D > #define AFE_PARAM_ID_PORT_SLOT_MAPPING_CONFIG 0x00010297 > #define AFE_PARAM_ID_CODEC_DMA_CONFIG 0x000102B8 > +#define AFE_PARAM_ID_USB_AUDIO_CONFIG 0x000102A4 > #define AFE_CMD_REMOTE_LPASS_CORE_HW_VOTE_REQUEST 0x000100f4 > #define AFE_CMD_RSP_REMOTE_LPASS_CORE_HW_VOTE_REQUEST 0x000100f5 > #define AFE_CMD_REMOTE_LPASS_CORE_HW_DEVOTE_REQUEST 0x000100f6 > @@ -71,12 +74,16 @@ > #define AFE_PORT_CONFIG_I2S_WS_SRC_INTERNAL 0x1 > #define AFE_LINEAR_PCM_DATA 0x0 > > +#define AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG 0x1 > > /* Port IDs */ > #define AFE_API_VERSION_HDMI_CONFIG 0x1 > #define AFE_PORT_ID_MULTICHAN_HDMI_RX 0x100E > #define AFE_PORT_ID_HDMI_OVER_DP_RX 0x6020 > > +/* USB AFE port */ > +#define AFE_PORT_ID_USB_RX 0x7000 > + > #define AFE_API_VERSION_SLIMBUS_CONFIG 0x1 > /* Clock set API version */ > #define AFE_API_VERSION_CLOCK_SET 1 > @@ -512,12 +519,109 @@ struct afe_param_id_cdc_dma_cfg { > u16 active_channels_mask; > } __packed; > > +struct afe_param_id_usb_cfg { > +/* Minor version used for tracking USB audio device configuration. > + * Supported values: AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + */ > + u32 cfg_minor_version; > +/* Sampling rate of the port. > + * Supported values: > + * - AFE_PORT_SAMPLE_RATE_8K > + * - AFE_PORT_SAMPLE_RATE_11025 > + * - AFE_PORT_SAMPLE_RATE_12K > + * - AFE_PORT_SAMPLE_RATE_16K > + * - AFE_PORT_SAMPLE_RATE_22050 > + * - AFE_PORT_SAMPLE_RATE_24K > + * - AFE_PORT_SAMPLE_RATE_32K > + * - AFE_PORT_SAMPLE_RATE_44P1K > + * - AFE_PORT_SAMPLE_RATE_48K > + * - AFE_PORT_SAMPLE_RATE_96K > + * - AFE_PORT_SAMPLE_RATE_192K > + */ > + u32 sample_rate; > +/* Bit width of the sample. > + * Supported values: 16, 24 > + */ > + u16 bit_width; > +/* Number of channels. > + * Supported values: 1 and 2 > + */ > + u16 num_channels; > +/* Data format supported by the USB. The supported value is > + * 0 (#AFE_USB_AUDIO_DATA_FORMAT_LINEAR_PCM). > + */ > + u16 data_format; > +/* this field must be 0 */ > + u16 reserved; > +/* device token of actual end USB aduio device */ > + u32 dev_token; > +/* endianness of this interface */ > + u32 endian; > +/* service interval */ > + u32 service_interval; > +} __packed; > + > +/** > + * struct afe_param_id_usb_audio_dev_params > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + * @dev_token: device token of actual end USB aduio device > + **/ > +struct afe_param_id_usb_audio_dev_params { > + u32 cfg_minor_version; > + u32 dev_token; > +} __packed; > + > +/** > + * struct afe_param_id_usb_audio_dev_lpcm_fmt > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + * @endian: endianness of this interface > + **/ > +struct afe_param_id_usb_audio_dev_lpcm_fmt { > + u32 cfg_minor_version; > + u32 endian; > +} __packed; > + > +/** > + * struct afe_param_id_usb_audio_dev_latency_mode > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_LATENCY_MODE > + * @mode: latency mode for the USB audio device > + **/ > +struct afe_param_id_usb_audio_dev_latency_mode { > + u32 minor_version; > + u32 mode; > +} __packed; > + > +#define AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL 0x000102B7 > + > +/** > + * struct afe_param_id_usb_audio_svc_interval > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + * @svc_interval: service interval > + **/ > +struct afe_param_id_usb_audio_svc_interval { > + u32 cfg_minor_version; > + u32 svc_interval; > +} __packed; > + > union afe_port_config { > struct afe_param_id_hdmi_multi_chan_audio_cfg hdmi_multi_ch; > struct afe_param_id_slimbus_cfg slim_cfg; > struct afe_param_id_i2s_cfg i2s_cfg; > struct afe_param_id_tdm_cfg tdm_cfg; > struct afe_param_id_cdc_dma_cfg dma_cfg; > + struct afe_param_id_usb_cfg usb_cfg; > } __packed; > > > @@ -577,6 +681,7 @@ struct afe_port_map { > */ > > static struct afe_port_map port_maps[AFE_PORT_MAX] = { > + [USB_RX] = { AFE_PORT_ID_USB_RX, USB_RX, 1, 1}, same > [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}, > @@ -1289,6 +1394,82 @@ void q6afe_tdm_port_prepare(struct q6afe_port *port, > } > EXPORT_SYMBOL_GPL(q6afe_tdm_port_prepare); > > +static int afe_port_send_usb_dev_param(struct q6afe_port *port, struct q6afe_usb_cfg *cfg) > +{ > + union afe_port_config *pcfg = &port->port_cfg; > + struct afe_param_id_usb_audio_dev_params usb_dev; > + struct afe_param_id_usb_audio_dev_lpcm_fmt lpcm_fmt; > + struct afe_param_id_usb_audio_svc_interval svc_int; > + int ret = 0; > + > + if (!pcfg) { > + dev_err(port->afe->dev, "%s: Error, no configuration data\n", __func__); > + ret = -EINVAL; > + goto exit; > + } > + > + memset(&usb_dev, 0, sizeof(usb_dev)); > + memset(&lpcm_fmt, 0, sizeof(lpcm_fmt)); > + > + usb_dev.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; > + q6afe_port_set_param_v2(port, &usb_dev, > + AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS, > + AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(usb_dev)); indentation. > + if (ret) { > + dev_err(port->afe->dev, "%s: AFE device param cmd failed %d\n", > + __func__, ret); > + goto exit; > + } > + > + lpcm_fmt.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; > + lpcm_fmt.endian = pcfg->usb_cfg.endian; > + ret = q6afe_port_set_param_v2(port, &lpcm_fmt, > + AFE_PARAM_ID_USB_AUDIO_DEV_LPCM_FMT, > + AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(lpcm_fmt)); > + if (ret) { > + dev_err(port->afe->dev, "%s: AFE device param cmd LPCM_FMT failed %d\n", > + __func__, ret); > + goto exit; > + } > + > + svc_int.cfg_minor_version = > + AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; this can go in 100 chars line. > + svc_int.svc_interval = pcfg->usb_cfg.service_interval; > + ret = q6afe_port_set_param_v2(port, &svc_int, > + AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL, > + AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(svc_int)); > + if (ret) { > + dev_err(port->afe->dev, "%s: AFE device param cmd svc_interval failed %d\n", > + __func__, ret); > + ret = -EINVAL; > + goto exit; > + } > +exit: > + return ret; > +} > + > +/** > + * q6afe_usb_port_prepare() - Prepare usb afe port. > + * > + * @port: Instance of afe port > + * @cfg: USB configuration for the afe port > + * > + */ > +void q6afe_usb_port_prepare(struct q6afe_port *port, > + struct q6afe_usb_cfg *cfg) > +{ > + union afe_port_config *pcfg = &port->port_cfg; > + > + pcfg->usb_cfg.cfg_minor_version = > + AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG; same here single line. > + pcfg->usb_cfg.sample_rate = cfg->sample_rate; > + pcfg->usb_cfg.num_channels = cfg->num_channels; > + pcfg->usb_cfg.bit_width = cfg->bit_width; > + > + afe_port_send_usb_dev_param(port, cfg); > +} > +EXPORT_SYMBOL_GPL(q6afe_usb_port_prepare); > + > /** > * q6afe_hdmi_port_prepare() - Prepare hdmi afe port. > * > @@ -1611,6 +1792,8 @@ struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id) > break; > case AFE_PORT_ID_WSA_CODEC_DMA_RX_0 ... AFE_PORT_ID_RX_CODEC_DMA_RX_7: > cfg_type = AFE_PARAM_ID_CODEC_DMA_CONFIG; break seems to be missing. > + case AFE_PORT_ID_USB_RX: > + cfg_type = AFE_PARAM_ID_USB_AUDIO_CONFIG; > break; > default: > dev_err(dev, "Invalid port id 0x%x\n", port_id); > diff --git a/sound/soc/qcom/qdsp6/q6afe.h b/sound/soc/qcom/qdsp6/q6afe.h > index 30fd77e2f458..88550a08e57d 100644 > --- a/sound/soc/qcom/qdsp6/q6afe.h > +++ b/sound/soc/qcom/qdsp6/q6afe.h > @@ -5,7 +5,7 @@ > > #include <dt-bindings/sound/qcom,q6afe.h> > > -#define AFE_PORT_MAX 129 > +#define AFE_PORT_MAX 130 > > #define MSM_AFE_PORT_TYPE_RX 0 > #define MSM_AFE_PORT_TYPE_TX 1 > @@ -205,6 +205,47 @@ struct q6afe_cdc_dma_cfg { > u16 active_channels_mask; > }; > > +/** > + * struct q6afe_usb_cfg > + * @cfg_minor_version: Minor version used for tracking USB audio device > + * configuration. > + * Supported values: > + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG > + * @sample_rate: Sampling rate of the port > + * Supported values: > + * AFE_PORT_SAMPLE_RATE_8K > + * AFE_PORT_SAMPLE_RATE_11025 > + * AFE_PORT_SAMPLE_RATE_12K > + * AFE_PORT_SAMPLE_RATE_16K > + * AFE_PORT_SAMPLE_RATE_22050 > + * AFE_PORT_SAMPLE_RATE_24K > + * AFE_PORT_SAMPLE_RATE_32K > + * AFE_PORT_SAMPLE_RATE_44P1K > + * AFE_PORT_SAMPLE_RATE_48K > + * AFE_PORT_SAMPLE_RATE_96K > + * AFE_PORT_SAMPLE_RATE_192K > + * @bit_width: Bit width of the sample. > + * Supported values: 16, 24 > + * @num_channels: Number of channels > + * Supported values: 1, 2 > + * @data_format: Data format supported by the USB > + * Supported values: 0 > + * @reserved: this field must be 0 > + * @dev_token: device token of actual end USB aduio device > + * @endian: endianness of this interface > + * @service_interval: service interval > + **/ > +struct q6afe_usb_cfg { > + u32 cfg_minor_version; > + u32 sample_rate; > + u16 bit_width; > + u16 num_channels; > + u16 data_format; > + u16 reserved; > + u32 dev_token; > + u32 endian; > + u32 service_interval; > +}; > > struct q6afe_port_config { > struct q6afe_hdmi_cfg hdmi; > @@ -212,6 +253,7 @@ struct q6afe_port_config { > struct q6afe_i2s_cfg i2s_cfg; > struct q6afe_tdm_cfg tdm; > struct q6afe_cdc_dma_cfg dma_cfg; > + struct q6afe_usb_cfg usb_audio; > }; > > struct q6afe_port; > @@ -221,6 +263,8 @@ int q6afe_port_start(struct q6afe_port *port); > int q6afe_port_stop(struct q6afe_port *port); > void q6afe_port_put(struct q6afe_port *port); > int q6afe_get_port_id(int index); > +void q6afe_usb_port_prepare(struct q6afe_port *port, > + struct q6afe_usb_cfg *cfg); > void q6afe_hdmi_port_prepare(struct q6afe_port *port, > struct q6afe_hdmi_cfg *cfg); > void q6afe_slim_port_prepare(struct q6afe_port *port, > diff --git a/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c b/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c > index f67c16fd90b9..39719c3f1767 100644 > --- a/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c > +++ b/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c > @@ -81,6 +81,26 @@ > > > static struct snd_soc_dai_driver q6dsp_audio_fe_dais[] = { > + { > + .playback = { > + .stream_name = "USB Playback", > + .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | > + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | > + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | > + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | > + SNDRV_PCM_RATE_192000, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE | > + SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE | > + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE | > + SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE, > + .channels_min = 1, > + .channels_max = 2, > + .rate_min = 8000, > + .rate_max = 192000, > + }, > + .id = USB_RX, > + .name = "USB_RX", > + }, > { > .playback = { > .stream_name = "HDMI Playback", > @@ -616,6 +636,9 @@ struct snd_soc_dai_driver *q6dsp_audio_ports_set_config(struct device *dev, > case WSA_CODEC_DMA_RX_0 ... RX_CODEC_DMA_RX_7: > q6dsp_audio_fe_dais[i].ops = cfg->q6dma_ops; > break; > + case USB_RX: > + q6dsp_audio_fe_dais[i].ops = cfg->q6usb_ops; > + break; > default: > break; > } > diff --git a/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h b/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h > index 7f052c8a1257..d8dde6dd0aca 100644 > --- a/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h > +++ b/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h > @@ -11,6 +11,7 @@ struct q6dsp_audio_port_dai_driver_config { > const struct snd_soc_dai_ops *q6i2s_ops; > const struct snd_soc_dai_ops *q6tdm_ops; > const struct snd_soc_dai_ops *q6dma_ops; > + const struct snd_soc_dai_ops *q6usb_ops; > }; > > struct snd_soc_dai_driver *q6dsp_audio_ports_set_config(struct device *dev, > diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c > index 928fd23e2c27..683ae2ae8e50 100644 > --- a/sound/soc/qcom/qdsp6/q6routing.c > +++ b/sound/soc/qcom/qdsp6/q6routing.c > @@ -514,6 +514,9 @@ static int msm_routing_put_audio_mixer(struct snd_kcontrol *kcontrol, > return 1; > } > > +static const struct snd_kcontrol_new usb_mixer_controls[] = { > + Q6ROUTING_RX_MIXERS(USB_RX) }; > + > static const struct snd_kcontrol_new hdmi_mixer_controls[] = { > Q6ROUTING_RX_MIXERS(HDMI_RX) }; > > @@ -733,6 +736,10 @@ static const struct snd_kcontrol_new mmul8_mixer_controls[] = { > > static const struct snd_soc_dapm_widget msm_qdsp6_widgets[] = { > /* Mixer definitions */ > + SND_SOC_DAPM_MIXER("USB Mixer", SND_SOC_NOPM, 0, 0, > + usb_mixer_controls, > + ARRAY_SIZE(usb_mixer_controls)), > + at the end of the list. > SND_SOC_DAPM_MIXER("HDMI Mixer", SND_SOC_NOPM, 0, 0, > hdmi_mixer_controls, > ARRAY_SIZE(hdmi_mixer_controls)), > @@ -952,6 +959,7 @@ static const struct snd_soc_dapm_widget msm_qdsp6_widgets[] = { > }; > > static const struct snd_soc_dapm_route intercon[] = { > + Q6ROUTING_RX_DAPM_ROUTE("USB Mixer", "USB_RX"), same. > Q6ROUTING_RX_DAPM_ROUTE("HDMI Mixer", "HDMI_RX"), > Q6ROUTING_RX_DAPM_ROUTE("DISPLAY_PORT_RX Audio Mixer", > "DISPLAY_PORT_RX"), --srini
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? 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. > 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?
> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) > +{ > + if (platform_ops) > + return -EEXIST; > + > + platform_ops = ops; > + return 0; > +} > +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops); > + > +int snd_usb_unregister_platform_ops(void) > +{ > + platform_ops = NULL; > + return 0; > +} > +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); I find this super-racy. If the this function is called just before ... > > /* > * disconnect streams > @@ -910,6 +928,10 @@ static int usb_audio_probe(struct usb_interface *intf, > usb_set_intfdata(intf, chip); > atomic_dec(&chip->active); > mutex_unlock(®ister_mutex); > + > + if (platform_ops->connect_cb) > + platform_ops->connect_cb(intf, chip); > + ... this, then you have a risk of using a dandling pointer. You also didn't test that the platform_ops != NULL, so there's a risk of dereferencing a NULL pointer. Not so good, eh? It's a classic (I've had the same sort of issues with SoundWire), when you export ops from one driver than can be removed, then additional protection is needed when using those callbacks.
On 26/01/2023 03:14, Wesley Cheng wrote: > Add a dt-binding to describe the definition of enabling the Q6 USB backend > device for audio offloading. The node carries information, which is passed > along to the QC USB SND class driver counterpart. These parameters will be > utilized during QMI stream enable requests. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > .../bindings/sound/qcom,q6usb-dais.yaml | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > > diff --git a/Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml b/Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > new file mode 100644 > index 000000000000..e24b4d52fa7e > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/qcom,q6usb-dais.yaml > @@ -0,0 +1,55 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/qcom,q6usb-dais.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm ASoC USB backend DAI > + > +maintainers: > + - Wesley Cheng <quic_wcheng@quicinc.com> > + > +description: > + The Q6USB backend is a supported AFE port on the Q6DSP. This backend > + driver will communicate the required settings to the QC USB SND class > + driver for properly enabling the audio stream. Parameters defined > + under this node will carry settings, which will be passed along during > + the QMI stream enable request. > + > +properties: > + compatible: > + enum: > + - qcom,q6usb-dais > + > + iommus: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 1 > + > + qcom,usb-audio-stream-id: > + description: > + SID for the Q6DSP processor for IOMMU mapping. > + $ref: /schemas/types.yaml#/definitions/uint32 We could derive this directly from iommus property as we do it like q6asm-dai.c --srini > + > + qcom,usb-audio-intr-num: > + description: > + Desired XHCI interrupter number to use. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > +required: > + - compatible > + - '#sound-dai-cells' > + - qcom,usb-audio-intr-num > + > +additionalProperties: false > + > +examples: > + - | > + usbdai: usbd { > + compatible = "qcom,q6usb-dais"; > + #sound-dai-cells = <1>; > + iommus = <&apps_smmu 0x180f 0x0>; > + qcom,usb-audio-stream-id = <0xf>; > + qcom,usb-audio-intr-num = <2>; > + };
On 1/25/23 21:14, Wesley Cheng wrote: > Several Qualcomm SoCs have a dedicated audio DSP, which has the ability to > support USB sound devices. This vendor driver will implement the required > handshaking with the DSP, in order to pass along required resources that > will be utilized by the DSP's USB SW. The communication channel used for > this handshaking will be using the QMI protocol. Required resources > include: > - Allocated secondary event ring address > - EP transfer ring address > - Interrupter number > > The above information will allow for the audio DSP to execute USB transfers > over the USB bus. It will also be able to support devices that have an > implicit feedback and sync endpoint as well. Offloading these data > transfers will allow the main/applications processor to enter lower CPU > power modes, and sustain a longer duration in those modes. > > Audio offloading is initiated with the following sequence: > 1. Userspace configures to route audio playback to USB backend and starts > playback on the platform soundcard. what happens if the DSP driver is probed after the USB one? Or vice-versa? Userspace needs to be notified of what is detected at the kernel level, I don't see how we can assume a specific route is always present. > +config QC_USB_AUDIO_OFFLOAD > + tristate "Qualcomm Audio Offload driver" USB Audio Offload > +struct uaudio_dev { > + struct usb_device *udev; > + /* audio control interface */ > + struct usb_host_interface *ctrl_intf; > + unsigned int card_num; > + unsigned int usb_core_id; > + atomic_t in_use; > + struct kref kref; > + wait_queue_head_t disconnect_wq; > + > + /* interface specific */ > + int num_intf; > + struct intf_info *info; > + struct snd_usb_audio *chip; > +}; > + > +static struct uaudio_dev uadev[SNDRV_CARDS]; I don't follow what this array is? Does this shadow all possible cards, even non-USB ones? > +static struct uaudio_qmi_dev *uaudio_qdev; > +static struct uaudio_qmi_svc *uaudio_svc; > +static DEFINE_MUTEX(qdev_mutex); > +/** > + * disable_audio_stream() - disable usb snd endpoints > + * @subs: usb substream > + * > + * Closes the USB SND endpoints associated with the current audio stream > + * used. This will decrement the USB SND endpoint opened reference count. > + * > + */ > +static void disable_audio_stream(struct snd_usb_substream *subs) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + > + if (subs->data_endpoint || subs->sync_endpoint) { > + close_endpoints(chip, subs); > + > + mutex_lock(&chip->mutex); > + subs->cur_audiofmt = NULL; > + mutex_unlock(&chip->mutex); can you explain why the format selection is protected by a mutex? I don't quite get what level of concurrency might happen here? > + } > + > + snd_usb_autosuspend(chip); > +} > + > +/** > + * enable_audio_stream() - enable usb snd endpoints > + * @subs: usb substream > + * @pcm_format: pcm format requested > + * @channels: number of channels > + * @cur_rate: sample rate > + * @datainterval: interval > + * > + * Opens all USB SND endpoints used for the data interface. This will increment > + * the USB SND endpoint's opened count. Requests to keep the interface resumed > + * until the audio stream is stopped. Will issue the USB set interface control > + * message to enable the data interface. > + * > + */ > +static int enable_audio_stream(struct snd_usb_substream *subs, > + snd_pcm_format_t pcm_format, > + unsigned int channels, unsigned int cur_rate, > + int datainterval) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + struct snd_pcm_hw_params params; > + const struct audioformat *fmt; > + int ret; > + > + _snd_pcm_hw_params_any(¶ms); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_FORMAT, > + (__force int) pcm_format, 0); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_CHANNELS, > + channels, 0); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_RATE, > + cur_rate, 0); > + > + pm_runtime_barrier(&chip->intf[0]->dev); > + snd_usb_autoresume(chip); > + > + fmt = find_format(&subs->fmt_list, pcm_format, cur_rate, > + channels, datainterval, subs); > + if (!fmt) { > + dev_err(uaudio_qdev->dev, > + "cannot find format: format = %#x, rate = %d, ch = %d\n", > + pcm_format, cur_rate, channels); > + return -EINVAL; > + } > + > + if (atomic_read(&chip->shutdown)) { > + dev_err(uaudio_qdev->dev, "chip already shutdown\n"); > + ret = -ENODEV; > + } else { > + if (subs->data_endpoint) > + close_endpoints(chip, subs); > + > + subs->data_endpoint = snd_usb_endpoint_open(chip, fmt, > + ¶ms, false); > + if (!subs->data_endpoint) { > + dev_err(uaudio_qdev->dev, "failed to open data endpoint\n"); > + return -EINVAL; > + } > + > + if (fmt->sync_ep) { > + subs->sync_endpoint = snd_usb_endpoint_open(chip, > + fmt, ¶ms, true); > + if (!subs->sync_endpoint) { > + dev_err(uaudio_qdev->dev, > + "failed to open sync endpoint\n"); > + return -EINVAL; > + } > + > + subs->data_endpoint->sync_source = subs->sync_endpoint; > + } > + > + mutex_lock(&chip->mutex); > + subs->cur_audiofmt = fmt; > + mutex_unlock(&chip->mutex); > + > + if (subs->sync_endpoint) { > + ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint); > + if (ret < 0) > + return ret; > + } > + > + ret = snd_usb_endpoint_prepare(chip, subs->data_endpoint); > + if (ret < 0) > + return ret; what happens in those two error cases? Should the format selected above remain set even though the prepare failed? > + > + dev_dbg(uaudio_qdev->dev, > + "selected %s iface:%d altsetting:%d datainterval:%dus\n", > + subs->direction ? "capture" : "playback", > + fmt->iface, fmt->altsetting, > + (1 << fmt->datainterval) * > + (subs->dev->speed >= USB_SPEED_HIGH ? > + BUS_INTERVAL_HIGHSPEED_AND_ABOVE : > + BUS_INTERVAL_FULL_SPEED)); > + } > + > + return 0; > +} <snip> > diff --git a/sound/usb/qcom/usb_audio_qmi_v01.c b/sound/usb/qcom/usb_audio_qmi_v01.c > new file mode 100644 > index 000000000000..95ae434f0a41 > --- /dev/null > +++ b/sound/usb/qcom/usb_audio_qmi_v01.c > @@ -0,0 +1,892 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/soc/qcom/qmi.h> > + > +#include "usb_audio_qmi_v01.h" > + > +static struct qmi_elem_info mem_info_v01_ei[] = { > + { > + .data_type = QMI_UNSIGNED_8_BYTE, > + .elem_len = 1, > + .elem_size = sizeof(u64), > + .array_type = NO_ARRAY, > + .tlv_type = 0, > + .offset = offsetof(struct mem_info_v01, va), > + }, > + { > + .data_type = QMI_UNSIGNED_8_BYTE, > + .elem_len = 1, > + .elem_size = sizeof(u64), > + .array_type = NO_ARRAY, > + .tlv_type = 0, > + .offset = offsetof(struct mem_info_v01, pa), > + }, > + { > + .data_type = QMI_UNSIGNED_4_BYTE, > + .elem_len = 1, > + .elem_size = sizeof(u32), > + .array_type = NO_ARRAY, > + .tlv_type = 0, > + .offset = offsetof(struct mem_info_v01, size), > + }, > + { > + .data_type = QMI_EOTI, > + .array_type = NO_ARRAY, > + .tlv_type = QMI_COMMON_TLV_TYPE, > + }, <snip> > + { > + .data_type = QMI_EOTI, > + .array_type = NO_ARRAY, > + .tlv_type = QMI_COMMON_TLV_TYPE, > + }, > +}; Are those dozens of descriptors needed? They look mostly the same, not sure how anyone could review this.
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. > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/usb/card.h | 1 + > sound/usb/pcm.c | 19 +++++++++++++++++-- > sound/usb/qcom/qc_audio_offload.c | 15 ++++++++++++++- > 3 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/sound/usb/card.h b/sound/usb/card.h > index 410a4ffad98e..ff6d4695e727 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -163,6 +163,7 @@ struct snd_usb_substream { > unsigned int pkt_offset_adj; /* Bytes to drop from beginning of packets (for non-compliant devices) */ > unsigned int stream_offset_adj; /* Bytes to drop from beginning of stream (for non-compliant devices) */ > > + unsigned int opened:1; /* pcm device opened */ > unsigned int running: 1; /* running status */ > unsigned int period_elapsed_pending; /* delay period handling */ > > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index 0b01a5dfcb73..8946f8ddb892 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -1114,8 +1114,15 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream) > struct snd_usb_stream *as = snd_pcm_substream_chip(substream); > struct snd_pcm_runtime *runtime = substream->runtime; > struct snd_usb_substream *subs = &as->substream[direction]; > + struct snd_usb_audio *chip = subs->stream->chip; > int ret; > > + mutex_lock(&chip->mutex); > + if (subs->opened) { > + mutex_unlock(&chip->mutex); > + return -EBUSY; > + } > + > runtime->hw = snd_usb_hardware; > /* need an explicit sync to catch applptr update in low-latency mode */ > if (direction == SNDRV_PCM_STREAM_PLAYBACK && > @@ -1132,13 +1139,17 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream) > > ret = setup_hw_info(runtime, subs); > if (ret < 0) > - return ret; > + goto out; > ret = snd_usb_autoresume(subs->stream->chip); > if (ret < 0) > - return ret; > + goto out; > ret = snd_media_stream_init(subs, as->pcm, direction); > if (ret < 0) > snd_usb_autosuspend(subs->stream->chip); > + subs->opened = 1; > +out: > + mutex_unlock(&chip->mutex); > + > return ret; > } > > @@ -1147,6 +1158,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream) > int direction = substream->stream; > struct snd_usb_stream *as = snd_pcm_substream_chip(substream); > struct snd_usb_substream *subs = &as->substream[direction]; > + struct snd_usb_audio *chip = subs->stream->chip; > int ret; > > snd_media_stop_pipeline(subs); > @@ -1160,6 +1172,9 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream) > > subs->pcm_substream = NULL; > snd_usb_autosuspend(subs->stream->chip); > + mutex_lock(&chip->mutex); > + subs->opened = 0; > + mutex_unlock(&chip->mutex); > > return 0; > } > diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c > index c1254d5f680d..9bd09282e70d 100644 > --- a/sound/usb/qcom/qc_audio_offload.c > +++ b/sound/usb/qcom/qc_audio_offload.c > @@ -1365,12 +1365,17 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle, > goto response; > } > > + mutex_lock(&chip->mutex); > if (req_msg->enable) { > - if (info_idx < 0 || chip->system_suspend) { > + if (info_idx < 0 || chip->system_suspend || subs->opened) { > ret = -EBUSY; > + mutex_unlock(&chip->mutex); > + > goto response; > } > + subs->opened = 1; > } > + mutex_unlock(&chip->mutex); > > if (req_msg->service_interval_valid) { > ret = get_data_interval_from_si(subs, > @@ -1392,6 +1397,11 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle, > if (!ret) > ret = prepare_qmi_response(subs, req_msg, &resp, > info_idx); > + if (ret < 0) { > + mutex_lock(&chip->mutex); > + subs->opened = 0; > + mutex_unlock(&chip->mutex); > + } > } else { > info = &uadev[pcm_card_num].info[info_idx]; > if (info->data_ep_pipe) { > @@ -1413,6 +1423,9 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle, > } > > disable_audio_stream(subs); > + mutex_lock(&chip->mutex); > + subs->opened = 0; > + mutex_unlock(&chip->mutex); > } > > response:
This version has lots of improvements, but I am concerned about hard-coded ops/callbacks that look racy and assume dependencies between driver probes. How does this work if the probe is delayed on one side for some reason? What happens is a driver is 'blacklisted' and manually added later? The code has to deal with this sort of known unknowns. I also still have a bit of heartburn with the notion that there would be a completely separate card with all the control for volume/mute/etc having to be duplicated. It's still a lot of good work so thanks for sharing and pushing for this capability.
On Wed, Jan 25, 2023 at 07:14:18PM -0800, Wesley Cheng wrote: > Several Qualcomm SoCs have a dedicated audio DSP, which has the ability to > support USB sound devices. This vendor driver will implement the required > handshaking with the DSP, in order to pass along required resources that > will be utilized by the DSP's USB SW. The communication channel used for > this handshaking will be using the QMI protocol. Required resources > include: > - Allocated secondary event ring address > - EP transfer ring address > - Interrupter number > > The above information will allow for the audio DSP to execute USB transfers > over the USB bus. It will also be able to support devices that have an > implicit feedback and sync endpoint as well. Offloading these data > transfers will allow the main/applications processor to enter lower CPU > power modes, and sustain a longer duration in those modes. > > Audio offloading is initiated with the following sequence: > 1. Userspace configures to route audio playback to USB backend and starts > playback on the platform soundcard. > 2. The Q6DSP AFE will communicate to the audio DSP to start the USB AFE > port. > 3. This results in a QMI packet with a STREAM enable command. > 4. The QC audio offload driver will fetch the required resources, and pass > this information as part of the QMI response to the STREAM enable command. > 5. Once the QMI response is received the audio DSP will start queuing data > on the USB bus. A real driver, finally!!! Thank you for posting this. That being said, some comments: > --- /dev/null > +++ b/sound/usb/qcom/qc_audio_offload.c > @@ -0,0 +1,1775 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. It's 2023 :) > + */ > + > +#include <linux/ctype.h> > +#include <linux/moduleparam.h> > +#include <linux/module.h> > +#include <linux/usb.h> > +#include <linux/init.h> > +#include <linux/usb/hcd.h> > +#include <linux/usb/xhci-intr.h> > +#include <linux/usb/quirks.h> > +#include <linux/usb/audio.h> > +#include <linux/usb/audio-v2.h> > +#include <linux/usb/audio-v3.h> > +#include <linux/soc/qcom/qmi.h> > +#include <linux/iommu.h> > +#include <linux/dma-mapping.h> > +#include <linux/dma-map-ops.h> > +#include <sound/q6usboffload.h> > + > +#include <sound/control.h> > +#include <sound/core.h> > +#include <sound/info.h> > +#include <sound/pcm.h> > +#include <sound/pcm_params.h> > +#include <sound/initval.h> > + > +#include <sound/soc.h> > +#include <sound/soc-usb.h> > +#include "../usbaudio.h" > +#include "../card.h" > +#include "../midi.h" > +#include "../mixer.h" > +#include "../proc.h" > +#include "../quirks.h" > +#include "../endpoint.h" > +#include "../helper.h" > +#include "../pcm.h" > +#include "../format.h" > +#include "../power.h" > +#include "../stream.h" > +#include "../media.h" > +#include "usb_audio_qmi_v01.h" > + > +/* Stream disable request timeout during USB device disconnect */ > +#define DEV_RELEASE_WAIT_TIMEOUT 10000 /* in ms */ > + > +/* Data interval calculation parameters */ > +#define BUS_INTERVAL_FULL_SPEED 1000 /* in us */ > +#define BUS_INTERVAL_HIGHSPEED_AND_ABOVE 125 /* in us */ > +#define MAX_BINTERVAL_ISOC_EP 16 > + > +#define SND_PCM_CARD_NUM_MASK 0xffff0000 > +#define SND_PCM_DEV_NUM_MASK 0xff00 > +#define SND_PCM_STREAM_DIRECTION 0xff > + > +/* iommu resource parameters and management */ > +#define PREPEND_SID_TO_IOVA(iova, sid) ((u64)(((u64)(iova)) | \ > + (((u64)sid) << 32))) > +#define IOVA_BASE 0x1000 > +#define IOVA_XFER_RING_BASE (IOVA_BASE + PAGE_SIZE * (SNDRV_CARDS + 1)) > +#define IOVA_XFER_BUF_BASE (IOVA_XFER_RING_BASE + PAGE_SIZE * SNDRV_CARDS * 32) > +#define IOVA_XFER_RING_MAX (IOVA_XFER_BUF_BASE - PAGE_SIZE) > +#define IOVA_XFER_BUF_MAX (0xfffff000 - PAGE_SIZE) > + > +#define MAX_XFER_BUFF_LEN (24 * PAGE_SIZE) > + > +struct iova_info { > + struct list_head list; > + unsigned long start_iova; > + size_t size; > + bool in_use; > +}; > + > +struct intf_info { > + unsigned long data_xfer_ring_va; > + size_t data_xfer_ring_size; > + unsigned long sync_xfer_ring_va; > + size_t sync_xfer_ring_size; > + unsigned long xfer_buf_va; > + size_t xfer_buf_size; > + phys_addr_t xfer_buf_pa; > + unsigned int data_ep_pipe; > + unsigned int sync_ep_pipe; > + u8 *xfer_buf; > + u8 intf_num; > + u8 pcm_card_num; > + u8 pcm_dev_num; > + u8 direction; > + bool in_use; > +}; > + > +struct uaudio_qmi_dev { > + struct device *dev; > + u32 sid; > + u32 intr_num; > + struct xhci_interrupter *ir; > + struct xhci_ring *sec_ring; > + struct iommu_domain *domain; > + > + /* list to keep track of available iova */ > + struct list_head xfer_ring_list; > + size_t xfer_ring_iova_size; > + unsigned long curr_xfer_ring_iova; > + struct list_head xfer_buf_list; > + size_t xfer_buf_iova_size; > + unsigned long curr_xfer_buf_iova; > + > + /* bit fields representing pcm card enabled */ > + unsigned long card_slot; > + /* indicate event ring mapped or not */ > + bool er_mapped; > + /* reference count to number of possible consumers */ > + atomic_t qdev_in_use; > + /* idx to last udev card number plugged in */ > + unsigned int last_card_num; > +}; > + > +struct uaudio_dev { > + struct usb_device *udev; > + /* audio control interface */ > + struct usb_host_interface *ctrl_intf; > + unsigned int card_num; > + unsigned int usb_core_id; > + atomic_t in_use; Why is this atomic? What happens if it changes right after you read it? > + struct kref kref; Why is this structure not a "real" device? Why are you faking it out? It should be in the device tree as you have a reference count and a lifespan, don't make it harder by trying to roll your own logic here. > + wait_queue_head_t disconnect_wq; > + > + /* interface specific */ > + int num_intf; > + struct intf_info *info; > + struct snd_usb_audio *chip; > +}; > + > +static struct uaudio_dev uadev[SNDRV_CARDS]; > +static struct uaudio_qmi_dev *uaudio_qdev; > +static struct uaudio_qmi_svc *uaudio_svc; Why are these all limited? These should all be dynamic and tied to the device that the driver is bound to. No static limits here please. > +static DEFINE_MUTEX(qdev_mutex); What does this lock? > + > +struct uaudio_qmi_svc { > + struct qmi_handle *uaudio_svc_hdl; > + struct work_struct qmi_disconnect_work; > + struct workqueue_struct *uaudio_wq; > + struct sockaddr_qrtr client_sq; > + bool client_connected; > +}; > + > +enum mem_type { > + MEM_EVENT_RING, > + MEM_XFER_RING, > + MEM_XFER_BUF, > +}; > + > +/* Supported audio formats */ > +enum usb_qmi_audio_format { > + USB_QMI_PCM_FORMAT_S8 = 0, > + USB_QMI_PCM_FORMAT_U8, > + USB_QMI_PCM_FORMAT_S16_LE, > + USB_QMI_PCM_FORMAT_S16_BE, > + USB_QMI_PCM_FORMAT_U16_LE, > + USB_QMI_PCM_FORMAT_U16_BE, > + USB_QMI_PCM_FORMAT_S24_LE, > + USB_QMI_PCM_FORMAT_S24_BE, > + USB_QMI_PCM_FORMAT_U24_LE, > + USB_QMI_PCM_FORMAT_U24_BE, > + USB_QMI_PCM_FORMAT_S24_3LE, > + USB_QMI_PCM_FORMAT_S24_3BE, > + USB_QMI_PCM_FORMAT_U24_3LE, > + USB_QMI_PCM_FORMAT_U24_3BE, > + USB_QMI_PCM_FORMAT_S32_LE, > + USB_QMI_PCM_FORMAT_S32_BE, > + USB_QMI_PCM_FORMAT_U32_LE, > + USB_QMI_PCM_FORMAT_U32_BE, > +}; > + > +static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long va, > + size_t iova_size, size_t mapped_iova_size); > +static void uaudio_dev_cleanup(struct uaudio_dev *dev); > +static void disable_audio_stream(struct snd_usb_substream *subs); > +static struct snd_usb_substream *find_substream(unsigned int card_num, > + unsigned int pcm_idx, unsigned int direction); Why not reorder the code to not need predeclarations? > + > +/* QMI service disconnect handlers */ > +static void qmi_disconnect_work(struct work_struct *w) > +{ > + struct intf_info *info; > + int idx, if_idx; > + struct snd_usb_substream *subs; > + struct snd_usb_audio *chip; > + > + /* find all active intf for set alt 0 and cleanup usb audio dev */ > + for (idx = 0; idx < SNDRV_CARDS; idx++) { > + if (!atomic_read(&uadev[idx].in_use)) > + continue; > + > + chip = uadev[idx].chip; > + for (if_idx = 0; if_idx < uadev[idx].num_intf; if_idx++) { > + if (!uadev[idx].info || !uadev[idx].info[if_idx].in_use) > + continue; > + info = &uadev[idx].info[if_idx]; > + subs = find_substream(info->pcm_card_num, > + info->pcm_dev_num, > + info->direction); > + if (!subs || !chip || atomic_read(&chip->shutdown)) { > + dev_err(&subs->dev->dev, > + "no sub for c#%u dev#%u dir%u\n", > + info->pcm_card_num, > + info->pcm_dev_num, > + info->direction); > + continue; > + } > + disable_audio_stream(subs); > + } > + atomic_set(&uadev[idx].in_use, 0); > + mutex_lock(&chip->mutex); > + uaudio_dev_cleanup(&uadev[idx]); > + mutex_unlock(&chip->mutex); > + } > +} > + > +/** > + * qmi_bye_cb() - qmi bye message callback > + * @handle: QMI handle > + * @node: id of the dying node > + * > + * This callback is invoked when the QMI bye control message is received > + * from the QMI client. Handle the message accordingly by ensuring that > + * the USB offload path is disabled and cleaned up. At this point, ADSP > + * is not utilizing the USB bus. > + * > + */ > +static void qmi_bye_cb(struct qmi_handle *handle, unsigned int node) > +{ > + struct uaudio_qmi_svc *svc = uaudio_svc; > + > + if (svc->uaudio_svc_hdl != handle) > + return; > + > + if (svc->client_connected && svc->client_sq.sq_node == node) { > + queue_work(svc->uaudio_wq, &svc->qmi_disconnect_work); > + svc->client_sq.sq_node = 0; > + svc->client_sq.sq_port = 0; > + svc->client_sq.sq_family = 0; > + svc->client_connected = false; > + } > +} > + > +/** > + * qmi_svc_disconnect_cb() - qmi client disconnected > + * @handle: QMI handle > + * @node: id of the dying node > + * @port: port of the dying client > + * > + * Invoked when the remote QMI client is disconnected. Handle this event > + * the same way as when the QMI bye message is received. This will ensure > + * the USB offloading path is disabled and cleaned up. > + * > + */ > +static void qmi_svc_disconnect_cb(struct qmi_handle *handle, > + unsigned int node, unsigned int port) > +{ > + struct uaudio_qmi_svc *svc; > + > + if (uaudio_svc == NULL) > + return; > + > + svc = uaudio_svc; > + if (svc->uaudio_svc_hdl != handle) > + return; > + > + if (svc->client_connected && svc->client_sq.sq_node == node && > + svc->client_sq.sq_port == port) { > + queue_work(svc->uaudio_wq, &svc->qmi_disconnect_work); > + svc->client_sq.sq_node = 0; > + svc->client_sq.sq_port = 0; > + svc->client_sq.sq_family = 0; > + svc->client_connected = false; > + } > +} > + > +/* QMI client callback handlers from QMI interface */ > +static struct qmi_ops uaudio_svc_ops_options = { > + .bye = qmi_bye_cb, > + .del_client = qmi_svc_disconnect_cb, > +}; > + > +static enum usb_audio_device_speed_enum_v01 > +get_speed_info(enum usb_device_speed udev_speed) > +{ > + switch (udev_speed) { > + case USB_SPEED_LOW: > + return USB_AUDIO_DEVICE_SPEED_LOW_V01; > + case USB_SPEED_FULL: > + return USB_AUDIO_DEVICE_SPEED_FULL_V01; > + case USB_SPEED_HIGH: > + return USB_AUDIO_DEVICE_SPEED_HIGH_V01; > + case USB_SPEED_SUPER: > + return USB_AUDIO_DEVICE_SPEED_SUPER_V01; > + case USB_SPEED_SUPER_PLUS: > + return USB_AUDIO_DEVICE_SPEED_SUPER_PLUS_V01; > + default: > + return USB_AUDIO_DEVICE_SPEED_INVALID_V01; > + } > +} > + > +/* Offloading IOMMU management */ > +static unsigned long uaudio_get_iova(unsigned long *curr_iova, > + size_t *curr_iova_size, struct list_head *head, size_t size) > +{ > + struct iova_info *info, *new_info = NULL; > + struct list_head *curr_head; > + unsigned long va = 0; > + size_t tmp_size = size; > + bool found = false; > + > + if (size % PAGE_SIZE) { > + dev_err(uaudio_qdev->dev, "size %zu is not page size multiple\n", > + size); > + goto done; > + } > + > + if (size > *curr_iova_size) { > + dev_err(uaudio_qdev->dev, "size %zu > curr size %zu\n", > + size, *curr_iova_size); > + goto done; > + } > + if (*curr_iova_size == 0) { > + dev_err(uaudio_qdev->dev, "iova mapping is full\n"); > + goto done; > + } > + > + list_for_each_entry(info, head, list) { > + /* exact size iova_info */ > + if (!info->in_use && info->size == size) { > + info->in_use = true; > + va = info->start_iova; > + *curr_iova_size -= size; > + found = true; > + dev_dbg(uaudio_qdev->dev, "exact size: %zu found\n", size); > + goto done; > + } else if (!info->in_use && tmp_size >= info->size) { > + if (!new_info) > + new_info = info; > + dev_dbg(uaudio_qdev->dev, "partial size: %zu found\n", > + info->size); > + tmp_size -= info->size; > + if (tmp_size) > + continue; > + > + va = new_info->start_iova; > + for (curr_head = &new_info->list; curr_head != > + &info->list; curr_head = curr_head->next) { > + new_info = list_entry(curr_head, struct > + iova_info, list); > + new_info->in_use = true; > + } > + info->in_use = true; > + *curr_iova_size -= size; > + found = true; > + goto done; > + } else { > + /* iova region in use */ > + new_info = NULL; > + tmp_size = size; > + } > + } > + > + info = kzalloc(sizeof(struct iova_info), GFP_KERNEL); > + if (!info) { > + va = 0; > + goto done; > + } > + > + va = info->start_iova = *curr_iova; > + info->size = size; > + info->in_use = true; > + *curr_iova += size; > + *curr_iova_size -= size; > + found = true; > + list_add_tail(&info->list, head); > + > +done: > + if (!found) > + dev_err(uaudio_qdev->dev, "unable to find %zu size iova\n", > + size); > + else > + dev_dbg(uaudio_qdev->dev, > + "va:0x%08lx curr_iova:0x%08lx curr_iova_size:%zu\n", > + va, *curr_iova, *curr_iova_size); > + > + return va; > +} > + > +/** > + * uaudio_iommu_map() - maps iommu memory for adsp > + * @mtype: ring type > + * @dma_coherent: dma coherent > + * @pa: physical address for ring/buffer > + * @size: size of memory region > + * @sgt: sg table for memory region > + * > + * Maps the XHCI related resources to a memory region that is assigned to be > + * used by the adsp. This will be mapped to the domain, which is created by > + * the ASoC USB backend driver. > + * > + */ > +static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent, > + phys_addr_t pa, size_t size, struct sg_table *sgt) > +{ > + unsigned long va_sg, va = 0; > + bool map = true; > + int i, ret; > + size_t sg_len, total_len = 0; > + struct scatterlist *sg; > + phys_addr_t pa_sg; > + int prot = IOMMU_READ | IOMMU_WRITE; > + > + if (dma_coherent) > + prot |= IOMMU_CACHE; > + > + switch (mtype) { > + case MEM_EVENT_RING: > + va = IOVA_BASE; > + /* er already mapped */ > + if (uaudio_qdev->er_mapped) > + map = false; > + break; > + case MEM_XFER_RING: > + va = uaudio_get_iova(&uaudio_qdev->curr_xfer_ring_iova, > + &uaudio_qdev->xfer_ring_iova_size, &uaudio_qdev->xfer_ring_list, > + size); > + break; > + case MEM_XFER_BUF: > + va = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova, > + &uaudio_qdev->xfer_buf_iova_size, &uaudio_qdev->xfer_buf_list, > + size); > + break; > + default: > + dev_err(uaudio_qdev->dev, "unknown mem type %d\n", mtype); > + } > + > + if (!va || !map) > + goto done; > + > + if (!sgt) > + goto skip_sgt_map; > + > + va_sg = va; > + for_each_sg(sgt->sgl, sg, sgt->nents, i) { > + sg_len = PAGE_ALIGN(sg->offset + sg->length); > + pa_sg = page_to_phys(sg_page(sg)); > + ret = iommu_map(uaudio_qdev->domain, va_sg, pa_sg, sg_len, > + prot); > + if (ret) { > + dev_err(uaudio_qdev->dev, "mapping failed ret%d\n", ret); > + dev_err(uaudio_qdev->dev, > + "type:%d, pa:%pa iova:0x%08lx sg_len:%zu\n", > + mtype, &pa_sg, va_sg, sg_len); > + uaudio_iommu_unmap(MEM_XFER_BUF, va, size, total_len); > + va = 0; > + goto done; > + } > + dev_dbg(uaudio_qdev->dev, > + "type:%d map pa:%pa to iova:0x%08lx len:%zu offset:%u\n", > + mtype, &pa_sg, va_sg, sg_len, sg->offset); > + va_sg += sg_len; > + total_len += sg_len; > + } > + > + if (size != total_len) { > + dev_err(uaudio_qdev->dev, "iova size %zu != mapped iova size %zu\n", > + size, total_len); > + uaudio_iommu_unmap(MEM_XFER_BUF, va, size, total_len); > + va = 0; > + } > + return va; > + > +skip_sgt_map: > + dev_dbg(uaudio_qdev->dev, "type:%d map pa:%pa to iova:0x%08lx size:%zu\n", > + mtype, &pa, va, size); > + > + ret = iommu_map(uaudio_qdev->domain, va, pa, size, prot); > + if (ret) > + dev_err(uaudio_qdev->dev, > + "failed to map pa:%pa iova:0x%lx type:%d ret:%d\n", > + &pa, va, mtype, ret); > +done: > + return va; > +} > + > +static void uaudio_put_iova(unsigned long va, size_t size, struct list_head > + *head, size_t *curr_iova_size) > +{ > + struct iova_info *info; > + size_t tmp_size = size; > + bool found = false; > + > + list_for_each_entry(info, head, list) { > + if (info->start_iova == va) { > + if (!info->in_use) { > + dev_err(uaudio_qdev->dev, "va %lu is not in use\n", > + va); > + return; > + } > + found = true; > + info->in_use = false; > + if (info->size == size) > + goto done; > + } > + > + if (found && tmp_size >= info->size) { > + info->in_use = false; > + tmp_size -= info->size; > + if (!tmp_size) > + goto done; > + } > + } > + > + if (!found) { > + dev_err(uaudio_qdev->dev, "unable to find the va %lu\n", va); > + return; > + } > +done: > + *curr_iova_size += size; > + dev_dbg(uaudio_qdev->dev, "curr_iova_size %zu\n", *curr_iova_size); > +} > + > +/** > + * uaudio_iommu_unmap() - unmaps iommu memory for adsp > + * @mtype: ring type > + * @va: virtual address to unmap > + * @iova_size: region size > + * @mapped_iova_size: mapped region size > + * > + * Unmaps the memory region that was previously assigned to the adsp. > + * > + */ > +static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long va, > + size_t iova_size, size_t mapped_iova_size) > +{ > + size_t umap_size; > + bool unmap = true; > + > + if (!va || !iova_size) > + return; > + > + switch (mtype) { > + case MEM_EVENT_RING: > + if (uaudio_qdev->er_mapped) > + uaudio_qdev->er_mapped = false; > + else > + unmap = false; > + break; > + > + case MEM_XFER_RING: > + uaudio_put_iova(va, iova_size, &uaudio_qdev->xfer_ring_list, > + &uaudio_qdev->xfer_ring_iova_size); > + break; > + case MEM_XFER_BUF: > + uaudio_put_iova(va, iova_size, &uaudio_qdev->xfer_buf_list, > + &uaudio_qdev->xfer_buf_iova_size); > + break; > + default: > + dev_err(uaudio_qdev->dev, "unknown mem type %d\n", mtype); > + unmap = false; > + } > + > + if (!unmap || !mapped_iova_size) > + return; > + > + dev_dbg(uaudio_qdev->dev, "type %d: unmap iova 0x%08lx size %zu\n", > + mtype, va, mapped_iova_size); > + > + umap_size = iommu_unmap(uaudio_qdev->domain, va, mapped_iova_size); > + if (umap_size != mapped_iova_size) > + dev_err(uaudio_qdev->dev, > + "unmapped size %zu for iova 0x%08lx of mapped size %zu\n", > + umap_size, va, mapped_iova_size); > +} > + > +/* looks up alias, if any, for controller DT node and returns the index */ > +static int usb_get_controller_id(struct usb_device *udev) > +{ > + if (udev->bus->sysdev && udev->bus->sysdev->of_node) > + return of_alias_get_id(udev->bus->sysdev->of_node, "usb"); > + > + return -ENODEV; > +} > + > +/** > + * uaudio_dev_intf_cleanup() - cleanup transfer resources > + * @udev: usb device > + * @info: usb offloading interface > + * > + * Cleans up the transfer ring related resources which are assigned per > + * endpoint from XHCI. This is invoked when the USB endpoints are no > + * longer in use by the adsp. > + * > + */ > +static void uaudio_dev_intf_cleanup(struct usb_device *udev, > + struct intf_info *info) > +{ > + uaudio_iommu_unmap(MEM_XFER_RING, info->data_xfer_ring_va, > + info->data_xfer_ring_size, info->data_xfer_ring_size); > + info->data_xfer_ring_va = 0; > + info->data_xfer_ring_size = 0; > + > + uaudio_iommu_unmap(MEM_XFER_RING, info->sync_xfer_ring_va, > + info->sync_xfer_ring_size, info->sync_xfer_ring_size); > + info->sync_xfer_ring_va = 0; > + info->sync_xfer_ring_size = 0; > + > + uaudio_iommu_unmap(MEM_XFER_BUF, info->xfer_buf_va, > + info->xfer_buf_size, info->xfer_buf_size); > + info->xfer_buf_va = 0; > + > + usb_free_coherent(udev, info->xfer_buf_size, > + info->xfer_buf, info->xfer_buf_pa); > + info->xfer_buf_size = 0; > + info->xfer_buf = NULL; > + info->xfer_buf_pa = 0; > + > + info->in_use = false; > +} > + > +/** > + * uaudio_event_ring_cleanup_free() - cleanup secondary event ring > + * @dev: usb offload device > + * > + * Cleans up the secondary event ring that was requested. This will > + * occur when the adsp is no longer transferring data on the USB bus > + * across all endpoints. > + * > + */ > +static void uaudio_event_ring_cleanup_free(struct uaudio_dev *dev) > +{ > + struct usb_hcd *hcd = bus_to_hcd(dev->udev->bus); > + > + clear_bit(dev->card_num, &uaudio_qdev->card_slot); > + /* all audio devices are disconnected */ > + if (!uaudio_qdev->card_slot) { > + uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE, > + PAGE_SIZE); > + xhci_remove_secondary_interrupter(hcd, uaudio_qdev->ir); > + uaudio_qdev->ir = NULL; > + } > +} > + > +/* kref release callback when all streams are disabled */ > +static void uaudio_dev_release(struct kref *kref) > +{ > + struct uaudio_dev *dev = container_of(kref, struct uaudio_dev, kref); > + > + uaudio_event_ring_cleanup_free(dev); > + atomic_set(&dev->in_use, 0); > + wake_up(&dev->disconnect_wq); > +} > + > +static struct snd_usb_substream *find_substream(unsigned int card_num, > + unsigned int pcm_idx, unsigned int direction) > +{ > + struct snd_usb_stream *as; > + struct snd_usb_substream *subs = NULL; > + struct snd_usb_audio *chip; > + > + chip = uadev[card_num].chip; > + if (!chip || atomic_read(&chip->shutdown)) > + goto done; What happens if this atomic value changes right after you read it? See, don't use them, use a proper lock correctly, it's much simpler and will actually work. thanks, greg k-h
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 Krysztof, On 1/26/2023 3:55 AM, Krzysztof Kozlowski wrote: > On 26/01/2023 04:14, Wesley Cheng wrote: >> Q6DSP supports handling of USB playback audio data if USB audio offloading >> is enabled. Add a new definition for the USB_RX AFE port, which is >> referenced when the AFE port is started. > > Subject prefix: > ASoC: dt-bindings: qcom,q6dsp-lpass-ports: > > because you are not adding USB_RX port to all bindings in ASoC. > Thanks will fix this on the next revision. 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
Hi Pierre, On 1/26/2023 7:50 AM, Pierre-Louis Bossart wrote: > > > >> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) >> +{ >> + if (platform_ops) >> + return -EEXIST; >> + >> + platform_ops = ops; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops); >> + >> +int snd_usb_unregister_platform_ops(void) >> +{ >> + platform_ops = NULL; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); > > I find this super-racy. > > If the this function is called just before ... > >> >> /* >> * disconnect streams >> @@ -910,6 +928,10 @@ static int usb_audio_probe(struct usb_interface *intf, >> usb_set_intfdata(intf, chip); >> atomic_dec(&chip->active); >> mutex_unlock(®ister_mutex); >> + >> + if (platform_ops->connect_cb) >> + platform_ops->connect_cb(intf, chip); >> + > > ... this, then you have a risk of using a dandling pointer. > > You also didn't test that the platform_ops != NULL, so there's a risk of > dereferencing a NULL pointer. > > Not so good, eh? > > It's a classic (I've had the same sort of issues with SoundWire), when > you export ops from one driver than can be removed, then additional > protection is needed when using those callbacks. > > Yep, will take a look at this a bit more to improve it. 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?
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. >>> 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? The mutex is only for registering the card, and ensuring atomic access to the list. I don't see how that would block support for having multiple devices being registered to soc-usb. ops are stored per backend device. Greg did want me to re-look at the soc-usb device management, so I will have to rework some of these things. It would be nice to see if we can get it to work like how the headphone jack works, ie interaction between soc-jack and core/jack.c. Thanks Wesley Cheng
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 Pierre, On 1/31/2023 7:02 PM, Pierre-Louis Bossart wrote: > > > 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. So the reason I wanted to mention the max number of EPs within the audio streaming descriptor is because the USB SND driver currently creates streams based off of the number of AS desc: static int snd_usb_create_streams(struct snd_usb_audio *chip, int ctrlif) { ... for (i = 0; i < assoc->bInterfaceCount; i++) { int intf = assoc->bFirstInterface + i; if (intf != ctrlif) snd_usb_create_stream(chip, ctrlif, intf); } "assoc" is the audio control interface desc. In the end, when userspace initiates a playback session, it operates on the streams created (which contains at max 1 isoc and 1 feedback ep) In short, the audio DSP doesn't need to consider handling more than 1 isoc ep (and potentially 1 feedback). I believe that each audio stream creates a separate PCM device, so userspace is still free to attempt to activate another audio stream. I believe # of PCM devices created matches the # of streams, so when userspace does activate another session, it would be on an entirely different substream, and can be handled through the USB SND (non-offload) path. If attempted to open the substream used by the offload path, then we would reject is based on the new change. Thanks Wesley Cheng
Hi Pierre, On 2/2/2023 5:23 PM, Wesley Cheng wrote: > Hi Pierre, > > On 1/31/2023 7:02 PM, Pierre-Louis Bossart wrote: >> >> >> 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. Getting back to passing in a format/profile that the USB BE doesn't support. It looks like ASoC doesn't actually check against the PCM HW params received (for components), so the audio playback does still occur even though its outside of what we support. Will need to add changes to specifically check for # of channels, format, etc... before we allow the session to proceed. Thanks Wesley Cheng
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. Thanks Wesley Cheng
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.
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