Re: [RFC PATCH v2 16/22] sound: usb: Introduce QC USB SND offloading support

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(&params);
• _snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_FORMAT,

• 	(__force int) pcm_format, 0);
  

• _snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_CHANNELS,

• 	channels, 0);
  

• _snd_pcm_hw_param_set(&params, 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,
  

• 		&params, 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, &params, 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.