Re: [PATCH v3 15/28] sound: usb: Introduce QC USB SND offloading support

+config QC_USB_AUDIO_OFFLOAD

• tristate "Qualcomm Audio Offload driver"

"Qualcomm USB Audio Offload driver"

• select SND_PCM

depends on SND_USB_AUDIO ?

• help

•  Say Y here to enable the Qualcomm USB audio offloading feature
  

•  This module sets up the required QMI stream enable/disable
  

•  responses to requests generated by the audio DSP.  It passes the
  

•  USB transfer resource references, so that the audio DSP can issue
  

•  USB transfers to the host controller.
  

•  To compile this driver as a module, choose M here: the module
  

•  will be called qc-audio-offload.
  

+#include <linux/ctype.h> +#include <linux/moduleparam.h> +#include <linux/module.h> +#include <linux/usb.h> +#include <linux/init.h>

alphabetical order?

+#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"

this looks weird, are all these needed?

+#define SND_PCM_CARD_NUM_MASK 0xffff0000 +#define SND_PCM_DEV_NUM_MASK 0xff00 +#define SND_PCM_STREAM_DIRECTION 0xff

The prefix looks wrong, or it duplicates SND_PCM stuff?

+/* 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;
• struct kref kref;
• wait_queue_head_t disconnect_wq;
• /* interface specific */
• int num_intf;
• struct intf_info *info;
• struct snd_usb_audio *chip;

+};

This looks too detailed, isnt' that a duplication of other structures that exist in the regular USB audio part?

There's nothing that looks QCOM-specific anyways.

+static struct uaudio_dev uadev[SNDRV_CARDS]; +static struct uaudio_qmi_dev *uaudio_qdev; +static struct uaudio_qmi_svc *uaudio_svc; +static DEFINE_MUTEX(qdev_mutex);

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

+};

prefixes better than 'MEM'?

+/* 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 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;
  

• }

+}

this doesn't look qcom-specific, why is this here?

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

• if (pcm_idx >= chip->pcm_devs)

• goto done;
  

• if (direction > SNDRV_PCM_STREAM_CAPTURE)

• goto done;
  

• list_for_each_entry(as, &chip->pcm_list, list) {

• if (as->pcm_index == pcm_idx) {
  

• 	subs = &as->substream[direction];
  

• 	goto done;
  

• }
  

• }

+done:

• return subs;

+}

+static int info_idx_from_ifnum(int card_num, int intf_num, bool enable) +{

• int i;
• /*

• * default index 0 is used when info is allocated upon
  

• * first enable audio stream req for a pcm device
  

• */
  

• if (enable && !uadev[card_num].info)

• return 0;
  

• for (i = 0; i < uadev[card_num].num_intf; i++) {

• if (enable && !uadev[card_num].info[i].in_use)
  

• 	return i;
  

• else if (!enable &&
  

• 		uadev[card_num].info[i].intf_num == intf_num)
  

• 	return i;
  

• }
• return -EINVAL;

+}

+static int get_data_interval_from_si(struct snd_usb_substream *subs,

• u32 service_interval)

+{

• unsigned int bus_intval, bus_intval_mult, binterval;
• if (subs->dev->speed >= USB_SPEED_HIGH)

• bus_intval = BUS_INTERVAL_HIGHSPEED_AND_ABOVE;
  

• else

• bus_intval = BUS_INTERVAL_FULL_SPEED;
  

• if (service_interval % bus_intval)

• return -EINVAL;
  

• bus_intval_mult = service_interval / bus_intval;
• binterval = ffs(bus_intval_mult);
• if (!binterval || binterval > MAX_BINTERVAL_ISOC_EP)

• return -EINVAL;
  

• /* check if another bit is set then bail out */
• bus_intval_mult = bus_intval_mult >> binterval;
• if (bus_intval_mult)

• return -EINVAL;
  

• return (binterval - 1);

+}

this doesn't look qcom-specific either, is this a set of helpers that need to be moved somewhere else?

+/**

• • 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);
  

• }
• snd_usb_autosuspend(chip);

+}

also not qcom-specific?

+/* kref release callback when all streams are disabled */

what does "all streams disabled" mean, and when does this happen?

+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 int __init qc_usb_audio_offload_init(void) +{

• struct uaudio_qmi_svc *svc;
• int ret;
• ret = snd_usb_register_platform_ops(&offload_ops);
• if (ret < 0)

• return ret;
  

so what happens if snd_usb is blacklisted or takes time to probe?

This seems to assume a timing dependency when USB is initialized/probed first?

• svc = kzalloc(sizeof(struct uaudio_qmi_svc), GFP_KERNEL);
• if (!svc) {

• ret = -ENOMEM;
  

• goto unreg_ops;
  

• }
• svc->uaudio_wq = create_singlethread_workqueue("uaudio_svc");
• if (!svc->uaudio_wq) {

• ret = -ENOMEM;
  

• goto free_svc;
  

• }
• svc->uaudio_svc_hdl = kzalloc(sizeof(struct qmi_handle), GFP_KERNEL);
• if (!svc->uaudio_svc_hdl) {

• ret = -ENOMEM;
  

• goto free_wq;
  

• }
• ret = qmi_handle_init(svc->uaudio_svc_hdl,

• 		QMI_UAUDIO_STREAM_REQ_MSG_V01_MAX_MSG_LEN,
  

• 		&uaudio_svc_ops_options,
  

• 		&uaudio_stream_req_handlers);
  

• ret = qmi_add_server(svc->uaudio_svc_hdl, UAUDIO_STREAM_SERVICE_ID_V01,

• 			UAUDIO_STREAM_SERVICE_VERS_V01, 0);
  

• INIT_WORK(&svc->qmi_disconnect_work, qmi_disconnect_work);
• uaudio_svc = svc;
• return 0;

+free_wq:

• destroy_workqueue(svc->uaudio_wq);

+free_svc:

• kfree(svc);

+unreg_ops:

• snd_usb_unregister_platform_ops();
• return ret;

+}

+static void __exit qc_usb_audio_offload_exit(void) +{

• struct uaudio_qmi_svc *svc = uaudio_svc;
• qmi_handle_release(svc->uaudio_svc_hdl);
• flush_workqueue(svc->uaudio_wq);
• destroy_workqueue(svc->uaudio_wq);
• kfree(svc);
• uaudio_svc = NULL;
• snd_usb_unregister_platform_ops();

+}

+module_init(qc_usb_audio_offload_init); +module_exit(qc_usb_audio_offload_exit);

what causes this module to be loaded?

+MODULE_DESCRIPTION("QC USB Audio Offloading"); +MODULE_LICENSE("GPL"); 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..1a0f2f579786 --- /dev/null +++ b/sound/usb/qcom/usb_audio_qmi_v01.c @@ -0,0 +1,892 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2022-2023 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),
  

• },

maybe move these arrays to a different patch that we don't need to scroll-through or review...

• {

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

• },

+};