2022-12-23 23:51:56

by Wesley Cheng

[permalink] [raw]
Subject: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

Create a USB BE component that will register a new USB port to the ASoC USB
framework. This will handle determination on if the requested audio
profile is supported by the USB device currently selected.

Signed-off-by: Wesley Cheng <[email protected]>
---
include/sound/q6usboffload.h | 20 +++
sound/soc/qcom/Kconfig | 4 +
sound/soc/qcom/qdsp6/Makefile | 1 +
sound/soc/qcom/qdsp6/q6usb.c | 232 ++++++++++++++++++++++++++++++++++
4 files changed, 257 insertions(+)
create mode 100644 include/sound/q6usboffload.h
create mode 100644 sound/soc/qcom/qdsp6/q6usb.c

diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
new file mode 100644
index 000000000000..e576808901d9
--- /dev/null
+++ b/include/sound/q6usboffload.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * linux/sound/q6usboffload.h -- QDSP6 USB offload
+ *
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+/**
+ * struct q6usb_offload
+ * @dev - dev handle to usb be
+ * @sid - streamID for iommu
+ * @intr_num - usb interrupter number
+ * @domain - allocated iommu domain
+ **/
+struct q6usb_offload {
+ struct device *dev;
+ u32 sid;
+ u32 intr_num;
+ struct iommu_domain *domain;
+};
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index 8c7398bc1ca8..d65c365116e5 100644
--- a/sound/soc/qcom/Kconfig
+++ b/sound/soc/qcom/Kconfig
@@ -111,6 +111,9 @@ config SND_SOC_QDSP6_APM
config SND_SOC_QDSP6_PRM_LPASS_CLOCKS
tristate

+config SND_SOC_QDSP6_USB
+ tristate
+
config SND_SOC_QDSP6_PRM
tristate
select SND_SOC_QDSP6_PRM_LPASS_CLOCKS
@@ -131,6 +134,7 @@ config SND_SOC_QDSP6
select SND_SOC_TOPOLOGY
select SND_SOC_QDSP6_APM
select SND_SOC_QDSP6_PRM
+ select SND_SOC_QDSP6_USB
help
To add support for MSM QDSP6 Soc Audio.
This will enable sound soc platform specific
diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile
index 3963bf234664..c9457ee898d0 100644
--- a/sound/soc/qcom/qdsp6/Makefile
+++ b/sound/soc/qcom/qdsp6/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SND_SOC_QDSP6_APM_DAI) += q6apm-dai.o
obj-$(CONFIG_SND_SOC_QDSP6_APM_LPASS_DAI) += q6apm-lpass-dais.o
obj-$(CONFIG_SND_SOC_QDSP6_PRM) += q6prm.o
obj-$(CONFIG_SND_SOC_QDSP6_PRM_LPASS_CLOCKS) += q6prm-clocks.o
+obj-$(CONFIG_SND_SOC_QDSP6_USB) += q6usb.o
diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
new file mode 100644
index 000000000000..a9da6dec6c6f
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/q6usb.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/iommu.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma-map-ops.h>
+
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include <sound/soc-usb.h>
+#include <sound/pcm_params.h>
+#include <sound/asound.h>
+#include <sound/q6usboffload.h>
+
+#include "q6dsp-lpass-ports.h"
+#include "q6afe.h"
+
+struct q6usb_port_data {
+ struct q6afe_usb_cfg usb_cfg;
+ struct snd_soc_usb *usb;
+ struct q6usb_offload priv;
+ int active_idx;
+};
+
+static const struct snd_soc_dapm_widget q6usb_dai_widgets[] = {
+ SND_SOC_DAPM_HP("USB_RX_BE", NULL),
+};
+
+static const struct snd_soc_dapm_route q6usb_dapm_routes[] = {
+ {"USB Playback", NULL, "USB_RX_BE"},
+};
+
+static int q6usb_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ return 0;
+}
+static const struct snd_soc_dai_ops q6usb_ops = {
+ .hw_params = q6usb_hw_params,
+};
+
+static struct snd_soc_dai_driver q6usb_be_dais[] = {
+ {
+ .playback = {
+ .stream_name = "USB BE RX",
+ .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_max = 192000,
+ .rate_min = 8000,
+ },
+ .id = USB_RX,
+ .name = "USB_RX_BE",
+ .ops = &q6usb_ops,
+ },
+};
+
+int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component,
+ const struct of_phandle_args *args,
+ const char **dai_name)
+{
+ int id = args->args[0];
+ int ret = -EINVAL;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(q6usb_be_dais); i++) {
+ if (q6usb_be_dais[i].id == id) {
+ *dai_name = q6usb_be_dais[i].name;
+ ret = 0;
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int q6usb_component_probe(struct snd_soc_component *component)
+{
+ struct q6usb_port_data *data = dev_get_drvdata(component->dev);
+ struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
+
+ data->usb->component = component;
+
+ snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
+ snd_soc_dapm_sync(dapm);
+
+ return 0;
+}
+
+static const struct snd_soc_component_driver q6usb_dai_component = {
+ .probe = q6usb_component_probe,
+ .name = "q6usb-dai-component",
+ .dapm_widgets = q6usb_dai_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets),
+ .dapm_routes = q6usb_dapm_routes,
+ .num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes),
+ .of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name,
+};
+
+int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
+ int connected)
+{
+ struct snd_soc_dapm_context *dapm;
+ struct q6usb_port_data *data;
+
+ if (!usb->component)
+ return 0;
+
+ dapm = snd_soc_component_get_dapm(usb->component);
+ data = dev_get_drvdata(usb->component->dev);
+
+ if (connected) {
+ snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
+ /* We only track the latest USB headset plugged in */
+ data->active_idx = card_idx;
+ } else {
+ snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
+ }
+ snd_soc_dapm_sync(dapm);
+
+ return 0;
+}
+
+static int q6usb_dai_dev_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ struct q6usb_port_data *data;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(node, "qcom,usb-audio-stream-id",
+ &data->priv.sid);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to read sid.\n");
+ return -ENODEV;
+ }
+
+ ret = of_property_read_u32(node, "qcom,usb-audio-intr-num",
+ &data->priv.intr_num);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to read intr num.\n");
+ return -ENODEV;
+ }
+
+ data->priv.domain = iommu_domain_alloc(pdev->dev.bus);
+ if (!data->priv.domain) {
+ dev_err(&pdev->dev, "failed to allocate iommu domain\n");
+ return -ENODEV;
+ }
+
+ /* attach to external processor iommu */
+ ret = iommu_attach_device(data->priv.domain, &pdev->dev);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to attach device ret = %d\n", ret);
+ goto free_domain;
+ }
+
+ data->usb = snd_soc_usb_add_port(dev, q6usb_alsa_connection_cb);
+ if (IS_ERR(data->usb)) {
+ dev_err(&pdev->dev, "failed to add usb port\n");
+ goto detach_device;
+ }
+
+ data->priv.dev = dev;
+ dev_set_drvdata(dev, data);
+ devm_snd_soc_register_component(dev, &q6usb_dai_component,
+ q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais));
+ snd_soc_usb_set_priv_data(&data->priv);
+
+ return 0;
+
+detach_device:
+ iommu_detach_device(data->priv.domain, &pdev->dev);
+free_domain:
+ iommu_domain_free(data->priv.domain);
+
+ return ret;
+}
+
+static int q6usb_dai_dev_remove(struct platform_device *pdev)
+{
+ struct q6usb_port_data *data = platform_get_drvdata(pdev);
+
+ iommu_detach_device(data->priv.domain, &pdev->dev);
+ iommu_domain_free(data->priv.domain);
+
+ snd_soc_usb_remove_port();
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id q6usb_dai_device_id[] = {
+ { .compatible = "qcom,q6usb-dais" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, q6afe_dai_device_id);
+#endif
+
+static struct platform_driver q6usb_dai_platform_driver = {
+ .driver = {
+ .name = "q6usb-dai",
+ .of_match_table = of_match_ptr(q6usb_dai_device_id),
+ },
+ .probe = q6usb_dai_dev_probe,
+ .remove = q6usb_dai_dev_remove,
+};
+module_platform_driver(q6usb_dai_platform_driver);
+
+MODULE_DESCRIPTION("Q6 USB backend dai driver");
+MODULE_LICENSE("GPL");


2022-12-24 09:21:12

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

On Fri, Dec 23, 2022 at 03:31:49PM -0800, Wesley Cheng wrote:
> Create a USB BE component that will register a new USB port to the ASoC USB
> framework. This will handle determination on if the requested audio
> profile is supported by the USB device currently selected.
>
> Signed-off-by: Wesley Cheng <[email protected]>
> ---
> include/sound/q6usboffload.h | 20 +++
> sound/soc/qcom/Kconfig | 4 +
> sound/soc/qcom/qdsp6/Makefile | 1 +
> sound/soc/qcom/qdsp6/q6usb.c | 232 ++++++++++++++++++++++++++++++++++
> 4 files changed, 257 insertions(+)
> create mode 100644 include/sound/q6usboffload.h
> create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
>
> diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
> new file mode 100644
> index 000000000000..e576808901d9
> --- /dev/null
> +++ b/include/sound/q6usboffload.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * linux/sound/q6usboffload.h -- QDSP6 USB offload
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/**
> + * struct q6usb_offload
> + * @dev - dev handle to usb be

"be"? What is that?

> + * @sid - streamID for iommu
> + * @intr_num - usb interrupter number
> + * @domain - allocated iommu domain
> + **/
> +struct q6usb_offload {
> + struct device *dev;

Do you properly reference count this?

> + u32 sid;
> + u32 intr_num;
> + struct iommu_domain *domain;
> +};

What is the lifetime of this structure? Who owns it? Where is the lock
for accessing it?

> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> index 8c7398bc1ca8..d65c365116e5 100644
> --- a/sound/soc/qcom/Kconfig
> +++ b/sound/soc/qcom/Kconfig
> @@ -111,6 +111,9 @@ config SND_SOC_QDSP6_APM
> config SND_SOC_QDSP6_PRM_LPASS_CLOCKS
> tristate
>
> +config SND_SOC_QDSP6_USB
> + tristate
> +
> config SND_SOC_QDSP6_PRM
> tristate
> select SND_SOC_QDSP6_PRM_LPASS_CLOCKS
> @@ -131,6 +134,7 @@ config SND_SOC_QDSP6
> select SND_SOC_TOPOLOGY
> select SND_SOC_QDSP6_APM
> select SND_SOC_QDSP6_PRM
> + select SND_SOC_QDSP6_USB
> help
> To add support for MSM QDSP6 Soc Audio.
> This will enable sound soc platform specific
> diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile
> index 3963bf234664..c9457ee898d0 100644
> --- a/sound/soc/qcom/qdsp6/Makefile
> +++ b/sound/soc/qcom/qdsp6/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_SND_SOC_QDSP6_APM_DAI) += q6apm-dai.o
> obj-$(CONFIG_SND_SOC_QDSP6_APM_LPASS_DAI) += q6apm-lpass-dais.o
> obj-$(CONFIG_SND_SOC_QDSP6_PRM) += q6prm.o
> obj-$(CONFIG_SND_SOC_QDSP6_PRM_LPASS_CLOCKS) += q6prm-clocks.o
> +obj-$(CONFIG_SND_SOC_QDSP6_USB) += q6usb.o
> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
> new file mode 100644
> index 000000000000..a9da6dec6c6f
> --- /dev/null
> +++ b/sound/soc/qcom/qdsp6/q6usb.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.

All of the code in this patch series is older than 2022 as I know it has
been in shipping devices for many years. Please use the proper
copyright year to make your lawyers happy...

> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/iommu.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dma-map-ops.h>
> +
> +#include <sound/pcm.h>
> +#include <sound/soc.h>
> +#include <sound/soc-usb.h>
> +#include <sound/pcm_params.h>
> +#include <sound/asound.h>
> +#include <sound/q6usboffload.h>
> +
> +#include "q6dsp-lpass-ports.h"
> +#include "q6afe.h"
> +
> +struct q6usb_port_data {
> + struct q6afe_usb_cfg usb_cfg;
> + struct snd_soc_usb *usb;
> + struct q6usb_offload priv;
> + int active_idx;
> +};
> +
> +static const struct snd_soc_dapm_widget q6usb_dai_widgets[] = {
> + SND_SOC_DAPM_HP("USB_RX_BE", NULL),
> +};
> +
> +static const struct snd_soc_dapm_route q6usb_dapm_routes[] = {
> + {"USB Playback", NULL, "USB_RX_BE"},
> +};

No terminating entry? How does this not break? Why do you need to
specify the size of the array, that feels like a design bug somewhere.


> +
> +static int q6usb_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + return 0;
> +}
> +static const struct snd_soc_dai_ops q6usb_ops = {
> + .hw_params = q6usb_hw_params,
> +};
> +
> +static struct snd_soc_dai_driver q6usb_be_dais[] = {
> + {
> + .playback = {
> + .stream_name = "USB BE RX",
> + .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_max = 192000,
> + .rate_min = 8000,
> + },
> + .id = USB_RX,
> + .name = "USB_RX_BE",
> + .ops = &q6usb_ops,
> + },
> +};
> +
> +int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component,
> + const struct of_phandle_args *args,
> + const char **dai_name)
> +{
> + int id = args->args[0];
> + int ret = -EINVAL;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(q6usb_be_dais); i++) {
> + if (q6usb_be_dais[i].id == id) {
> + *dai_name = q6usb_be_dais[i].name;
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int q6usb_component_probe(struct snd_soc_component *component)
> +{
> + struct q6usb_port_data *data = dev_get_drvdata(component->dev);
> + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> +
> + data->usb->component = component;
> +
> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
> + snd_soc_dapm_sync(dapm);
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_component_driver q6usb_dai_component = {
> + .probe = q6usb_component_probe,
> + .name = "q6usb-dai-component",
> + .dapm_widgets = q6usb_dai_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets),
> + .dapm_routes = q6usb_dapm_routes,
> + .num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes),
> + .of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name,
> +};
> +
> +int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
> + int connected)
> +{
> + struct snd_soc_dapm_context *dapm;
> + struct q6usb_port_data *data;
> +
> + if (!usb->component)
> + return 0;

How can this happen?

Why is this not an error?

> +
> + dapm = snd_soc_component_get_dapm(usb->component);
> + data = dev_get_drvdata(usb->component->dev);
> +
> + if (connected) {
> + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
> + /* We only track the latest USB headset plugged in */
> + data->active_idx = card_idx;
> + } else {
> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
> + }
> + snd_soc_dapm_sync(dapm);
> +
> + return 0;
> +}
> +
> +static int q6usb_dai_dev_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + struct q6usb_port_data *data;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(node, "qcom,usb-audio-stream-id",
> + &data->priv.sid);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to read sid.\n");
> + return -ENODEV;
> + }
> +
> + ret = of_property_read_u32(node, "qcom,usb-audio-intr-num",
> + &data->priv.intr_num);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to read intr num.\n");
> + return -ENODEV;
> + }
> +
> + data->priv.domain = iommu_domain_alloc(pdev->dev.bus);
> + if (!data->priv.domain) {
> + dev_err(&pdev->dev, "failed to allocate iommu domain\n");
> + return -ENODEV;
> + }
> +
> + /* attach to external processor iommu */
> + ret = iommu_attach_device(data->priv.domain, &pdev->dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to attach device ret = %d\n", ret);
> + goto free_domain;
> + }
> +
> + data->usb = snd_soc_usb_add_port(dev, q6usb_alsa_connection_cb);
> + if (IS_ERR(data->usb)) {
> + dev_err(&pdev->dev, "failed to add usb port\n");
> + goto detach_device;
> + }
> +
> + data->priv.dev = dev;
> + dev_set_drvdata(dev, data);
> + devm_snd_soc_register_component(dev, &q6usb_dai_component,
> + q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais));

Very odd indentation. Please do this properly everywhere.


> + snd_soc_usb_set_priv_data(&data->priv);
> +
> + return 0;
> +
> +detach_device:
> + iommu_detach_device(data->priv.domain, &pdev->dev);
> +free_domain:
> + iommu_domain_free(data->priv.domain);
> +
> + return ret;
> +}
> +
> +static int q6usb_dai_dev_remove(struct platform_device *pdev)
> +{
> + struct q6usb_port_data *data = platform_get_drvdata(pdev);
> +
> + iommu_detach_device(data->priv.domain, &pdev->dev);
> + iommu_domain_free(data->priv.domain);
> +
> + snd_soc_usb_remove_port();
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF

Is this really needed still?

thanks,

greg k-h

2022-12-27 13:45:01

by Mark Brown

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

On Sat, Dec 24, 2022 at 10:02:59AM +0100, Greg KH wrote:
> On Fri, Dec 23, 2022 at 03:31:49PM -0800, Wesley Cheng wrote:

> > + * struct q6usb_offload
> > + * @dev - dev handle to usb be

> "be"? What is that?

Back end. This is a concept in DPCM which should be reasonably
discoverable to people working on the audio portions of this code.

> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.

> All of the code in this patch series is older than 2022 as I know it has
> been in shipping devices for many years. Please use the proper
> copyright year to make your lawyers happy...

Are you *positive* about this. Based on some preparatory discussions
the Qualcomm people had with Takashi and I it seemed like this was a new
version of existing concepts. I'm sure they had something already but
it's not obvious to me that they're just posting the same code.

> > +static const struct snd_soc_dapm_route q6usb_dapm_routes[] = {
> > + {"USB Playback", NULL, "USB_RX_BE"},
> > +};

> No terminating entry? How does this not break? Why do you need to
> specify the size of the array, that feels like a design bug somewhere.

It's how the interface works, the number of entries is passed in when
adding routes.


Attachments:
(No filename) (1.29 kB)
signature.asc (499.00 B)
Download all attachments

2022-12-27 14:25:00

by Takashi Iwai

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

On Tue, 27 Dec 2022 14:45:13 +0100,
Greg KH wrote:
>
> On Tue, Dec 27, 2022 at 01:04:55PM +0000, Mark Brown wrote:
> > On Sat, Dec 24, 2022 at 10:02:59AM +0100, Greg KH wrote:
> > > On Fri, Dec 23, 2022 at 03:31:49PM -0800, Wesley Cheng wrote:
> >
> > > > + * struct q6usb_offload
> > > > + * @dev - dev handle to usb be
> >
> > > "be"? What is that?
> >
> > Back end. This is a concept in DPCM which should be reasonably
> > discoverable to people working on the audio portions of this code.
>
> Ok, then how is the reference counting logic handled here? USB devices
> can be removed from the system at any point in time...

The whole picture is fairly complex, and this patch is a part
belonging to the ASoC machine driver -- that is, it's bound to the
Qualcomm host, and there can be only one on a system.

OTOH, USB audio devices are still managed by the existing USB audio
driver as is, and they can be multiple and any devices. The basic
idea here is a hijack of the USB data processing in USB audio driver
with the offloading mechanism by this ASoC driver (only if the
condition met).


thanks,

Takashi

2022-12-27 14:41:30

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

On Tue, Dec 27, 2022 at 01:04:55PM +0000, Mark Brown wrote:
> On Sat, Dec 24, 2022 at 10:02:59AM +0100, Greg KH wrote:
> > On Fri, Dec 23, 2022 at 03:31:49PM -0800, Wesley Cheng wrote:
>
> > > + * struct q6usb_offload
> > > + * @dev - dev handle to usb be
>
> > "be"? What is that?
>
> Back end. This is a concept in DPCM which should be reasonably
> discoverable to people working on the audio portions of this code.

Ok, then how is the reference counting logic handled here? USB devices
can be removed from the system at any point in time...

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>
> > All of the code in this patch series is older than 2022 as I know it has
> > been in shipping devices for many years. Please use the proper
> > copyright year to make your lawyers happy...
>
> Are you *positive* about this. Based on some preparatory discussions
> the Qualcomm people had with Takashi and I it seemed like this was a new
> version of existing concepts. I'm sure they had something already but
> it's not obvious to me that they're just posting the same code.

I thought that this same code has been shipping in devices for a few
years now in the last few Samsung phone models. Is this not the same
code that is in those devices?

If not, why not, what happened to that codebase that makes it not worthy
of being submitted upstream?

> > > +static const struct snd_soc_dapm_route q6usb_dapm_routes[] = {
> > > + {"USB Playback", NULL, "USB_RX_BE"},
> > > +};
>
> > No terminating entry? How does this not break? Why do you need to
> > specify the size of the array, that feels like a design bug somewhere.
>
> It's how the interface works, the number of entries is passed in when
> adding routes.

Ah, you all might want to change that to make it simpler :)

thanks,

greg k-h

2022-12-27 15:13:12

by Mark Brown

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

On Tue, Dec 27, 2022 at 03:02:31PM +0100, Takashi Iwai wrote:
> Greg KH wrote:
> > On Tue, Dec 27, 2022 at 01:04:55PM +0000, Mark Brown wrote:
> > > On Sat, Dec 24, 2022 at 10:02:59AM +0100, Greg KH wrote:

> > > > "be"? What is that?

> > > Back end. This is a concept in DPCM which should be reasonably
> > > discoverable to people working on the audio portions of this code.

> > Ok, then how is the reference counting logic handled here? USB devices
> > can be removed from the system at any point in time...

> The whole picture is fairly complex, and this patch is a part
> belonging to the ASoC machine driver -- that is, it's bound to the
> Qualcomm host, and there can be only one on a system.

> OTOH, USB audio devices are still managed by the existing USB audio
> driver as is, and they can be multiple and any devices. The basic
> idea here is a hijack of the USB data processing in USB audio driver
> with the offloading mechanism by this ASoC driver (only if the
> condition met).

Right. I haven't even begun to look at the actual code here, just
triaging my inbox, so I've got no thoughts on if things work or not.


Attachments:
(No filename) (1.14 kB)
signature.asc (499.00 B)
Download all attachments

2022-12-27 15:18:14

by Mark Brown

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

On Tue, Dec 27, 2022 at 02:45:13PM +0100, Greg KH wrote:
> On Tue, Dec 27, 2022 at 01:04:55PM +0000, Mark Brown wrote:
> > On Sat, Dec 24, 2022 at 10:02:59AM +0100, Greg KH wrote:

> > > > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.

> > > All of the code in this patch series is older than 2022 as I know it has
> > > been in shipping devices for many years. Please use the proper
> > > copyright year to make your lawyers happy...

> > Are you *positive* about this. Based on some preparatory discussions
> > the Qualcomm people had with Takashi and I it seemed like this was a new
> > version of existing concepts. I'm sure they had something already but
> > it's not obvious to me that they're just posting the same code.

> I thought that this same code has been shipping in devices for a few
> years now in the last few Samsung phone models. Is this not the same
> code that is in those devices?

> If not, why not, what happened to that codebase that makes it not worthy
> of being submitted upstream?

I don't specifically know anything about that code but I'd expect that
for out of tree code breaking new ground like this there'd be a strong
likelyhood that there'd be design level issues and that's what the pre
submission discussions were all about - how to fit the concept into the
kernel subsystems in a way that might be maintainable. There's also
been the whole transition to their new DSP firmware going on. It's
possible that what was shipped was implemented in the same way with the
same code but I'd not assume that this is the case without actually
comparing the two.


Attachments:
(No filename) (1.62 kB)
signature.asc (499.00 B)
Download all attachments

2022-12-27 21:47:29

by Wesley Cheng

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

Hi Greg,

On 12/24/2022 1:02 AM, Greg KH wrote:
> On Fri, Dec 23, 2022 at 03:31:49PM -0800, Wesley Cheng wrote:
>> Create a USB BE component that will register a new USB port to the ASoC USB
>> framework. This will handle determination on if the requested audio
>> profile is supported by the USB device currently selected.
>>
>> Signed-off-by: Wesley Cheng <[email protected]>
>> ---
>> include/sound/q6usboffload.h | 20 +++
>> sound/soc/qcom/Kconfig | 4 +
>> sound/soc/qcom/qdsp6/Makefile | 1 +
>> sound/soc/qcom/qdsp6/q6usb.c | 232 ++++++++++++++++++++++++++++++++++
>> 4 files changed, 257 insertions(+)
>> create mode 100644 include/sound/q6usboffload.h
>> create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
>>
>> diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
>> new file mode 100644
>> index 000000000000..e576808901d9
>> --- /dev/null
>> +++ b/include/sound/q6usboffload.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * linux/sound/q6usboffload.h -- QDSP6 USB offload
>> + *
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/**
>> + * struct q6usb_offload
>> + * @dev - dev handle to usb be
>
> "be"? What is that?
>
>> + * @sid - streamID for iommu
>> + * @intr_num - usb interrupter number
>> + * @domain - allocated iommu domain
>> + **/
>> +struct q6usb_offload {
>> + struct device *dev;
>
> Do you properly reference count this?
>
>> + u32 sid;
>> + u32 intr_num;
>> + struct iommu_domain *domain;
>> +};
>
> What is the lifetime of this structure? Who owns it? Where is the lock
> for accessing it?
>

Owner of this structure is the USB backend. If the USB backend is
removed, then the qc_audio_offload driver would never receive the QMI
request to enable the audio stream path from the audio dsp. (where this
is referenced) It will exist as long as the USB BE device exists.

One thing I will need to follow up on is if an ASoC backend device is
removed while an audio playback is happening how it would handle it.

>> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
>> index 8c7398bc1ca8..d65c365116e5 100644
>> --- a/sound/soc/qcom/Kconfig
>> +++ b/sound/soc/qcom/Kconfig
>> @@ -111,6 +111,9 @@ config SND_SOC_QDSP6_APM
>> config SND_SOC_QDSP6_PRM_LPASS_CLOCKS
>> tristate
>>
>> +config SND_SOC_QDSP6_USB
>> + tristate
>> +
>> config SND_SOC_QDSP6_PRM
>> tristate
>> select SND_SOC_QDSP6_PRM_LPASS_CLOCKS
>> @@ -131,6 +134,7 @@ config SND_SOC_QDSP6
>> select SND_SOC_TOPOLOGY
>> select SND_SOC_QDSP6_APM
>> select SND_SOC_QDSP6_PRM
>> + select SND_SOC_QDSP6_USB
>> help
>> To add support for MSM QDSP6 Soc Audio.
>> This will enable sound soc platform specific
>> diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile
>> index 3963bf234664..c9457ee898d0 100644
>> --- a/sound/soc/qcom/qdsp6/Makefile
>> +++ b/sound/soc/qcom/qdsp6/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_SND_SOC_QDSP6_APM_DAI) += q6apm-dai.o
>> obj-$(CONFIG_SND_SOC_QDSP6_APM_LPASS_DAI) += q6apm-lpass-dais.o
>> obj-$(CONFIG_SND_SOC_QDSP6_PRM) += q6prm.o
>> obj-$(CONFIG_SND_SOC_QDSP6_PRM_LPASS_CLOCKS) += q6prm-clocks.o
>> +obj-$(CONFIG_SND_SOC_QDSP6_USB) += q6usb.o
>> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
>> new file mode 100644
>> index 000000000000..a9da6dec6c6f
>> --- /dev/null
>> +++ b/sound/soc/qcom/qdsp6/q6usb.c
>> @@ -0,0 +1,232 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>
> All of the code in this patch series is older than 2022 as I know it has
> been in shipping devices for many years. Please use the proper
> copyright year to make your lawyers happy...
>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/iommu.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dma-map-ops.h>
>> +
>> +#include <sound/pcm.h>
>> +#include <sound/soc.h>
>> +#include <sound/soc-usb.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/asound.h>
>> +#include <sound/q6usboffload.h>
>> +
>> +#include "q6dsp-lpass-ports.h"
>> +#include "q6afe.h"
>> +
>> +struct q6usb_port_data {
>> + struct q6afe_usb_cfg usb_cfg;
>> + struct snd_soc_usb *usb;
>> + struct q6usb_offload priv;
>> + int active_idx;
>> +};
>> +
>> +static const struct snd_soc_dapm_widget q6usb_dai_widgets[] = {
>> + SND_SOC_DAPM_HP("USB_RX_BE", NULL),
>> +};
>> +
>> +static const struct snd_soc_dapm_route q6usb_dapm_routes[] = {
>> + {"USB Playback", NULL, "USB_RX_BE"},
>> +};
>
> No terminating entry? How does this not break? Why do you need to
> specify the size of the array, that feels like a design bug somewhere.
>
>
>> +
>> +static int q6usb_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + return 0;
>> +}
>> +static const struct snd_soc_dai_ops q6usb_ops = {
>> + .hw_params = q6usb_hw_params,
>> +};
>> +
>> +static struct snd_soc_dai_driver q6usb_be_dais[] = {
>> + {
>> + .playback = {
>> + .stream_name = "USB BE RX",
>> + .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_max = 192000,
>> + .rate_min = 8000,
>> + },
>> + .id = USB_RX,
>> + .name = "USB_RX_BE",
>> + .ops = &q6usb_ops,
>> + },
>> +};
>> +
>> +int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component,
>> + const struct of_phandle_args *args,
>> + const char **dai_name)
>> +{
>> + int id = args->args[0];
>> + int ret = -EINVAL;
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(q6usb_be_dais); i++) {
>> + if (q6usb_be_dais[i].id == id) {
>> + *dai_name = q6usb_be_dais[i].name;
>> + ret = 0;
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int q6usb_component_probe(struct snd_soc_component *component)
>> +{
>> + struct q6usb_port_data *data = dev_get_drvdata(component->dev);
>> + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>> +
>> + data->usb->component = component;
>> +
>> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> + snd_soc_dapm_sync(dapm);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct snd_soc_component_driver q6usb_dai_component = {
>> + .probe = q6usb_component_probe,
>> + .name = "q6usb-dai-component",
>> + .dapm_widgets = q6usb_dai_widgets,
>> + .num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets),
>> + .dapm_routes = q6usb_dapm_routes,
>> + .num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes),
>> + .of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name,
>> +};
>> +
>> +int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>> + int connected)
>> +{
>> + struct snd_soc_dapm_context *dapm;
>> + struct q6usb_port_data *data;
>> +
>> + if (!usb->component)
>> + return 0;
>
> How can this happen?
>
> Why is this not an error?
>

If the ASoC platform card that this USB BE is a child of, is not yet
registered then the component probe hasn't happened. However, that is
independent of when the USB connect/disconnect events can happen.

>> +
>> + dapm = snd_soc_component_get_dapm(usb->component);
>> + data = dev_get_drvdata(usb->component->dev);
>> +
>> + if (connected) {
>> + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>> + /* We only track the latest USB headset plugged in */
>> + data->active_idx = card_idx;
>> + } else {
>> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> + }
>> + snd_soc_dapm_sync(dapm);
>> +
>> + return 0;
>> +}
>> +
>> +static int q6usb_dai_dev_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *node = pdev->dev.of_node;
>> + struct q6usb_port_data *data;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + ret = of_property_read_u32(node, "qcom,usb-audio-stream-id",
>> + &data->priv.sid);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to read sid.\n");
>> + return -ENODEV;
>> + }
>> +
>> + ret = of_property_read_u32(node, "qcom,usb-audio-intr-num",
>> + &data->priv.intr_num);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to read intr num.\n");
>> + return -ENODEV;
>> + }
>> +
>> + data->priv.domain = iommu_domain_alloc(pdev->dev.bus);
>> + if (!data->priv.domain) {
>> + dev_err(&pdev->dev, "failed to allocate iommu domain\n");
>> + return -ENODEV;
>> + }
>> +
>> + /* attach to external processor iommu */
>> + ret = iommu_attach_device(data->priv.domain, &pdev->dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to attach device ret = %d\n", ret);
>> + goto free_domain;
>> + }
>> +
>> + data->usb = snd_soc_usb_add_port(dev, q6usb_alsa_connection_cb);
>> + if (IS_ERR(data->usb)) {
>> + dev_err(&pdev->dev, "failed to add usb port\n");
>> + goto detach_device;
>> + }
>> +
>> + data->priv.dev = dev;
>> + dev_set_drvdata(dev, data);
>> + devm_snd_soc_register_component(dev, &q6usb_dai_component,
>> + q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais));
>
> Very odd indentation. Please do this properly everywhere.
>
>

Got it...will fix everywhere

>> + snd_soc_usb_set_priv_data(&data->priv);
>> +
>> + return 0;
>> +
>> +detach_device:
>> + iommu_detach_device(data->priv.domain, &pdev->dev);
>> +free_domain:
>> + iommu_domain_free(data->priv.domain);
>> +
>> + return ret;
>> +}
>> +
>> +static int q6usb_dai_dev_remove(struct platform_device *pdev)
>> +{
>> + struct q6usb_port_data *data = platform_get_drvdata(pdev);
>> +
>> + iommu_detach_device(data->priv.domain, &pdev->dev);
>> + iommu_domain_free(data->priv.domain);
>> +
>> + snd_soc_usb_remove_port();
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>
> Is this really needed still?
>

Will remove this

Thanks
Wesley Cheng

2022-12-27 21:48:54

by Wesley Cheng

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

Hi Mark/Greg,

On 12/27/2022 7:11 AM, Mark Brown wrote:
> On Tue, Dec 27, 2022 at 02:45:13PM +0100, Greg KH wrote:
>> On Tue, Dec 27, 2022 at 01:04:55PM +0000, Mark Brown wrote:
>>> On Sat, Dec 24, 2022 at 10:02:59AM +0100, Greg KH wrote:
>
>>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>
>>>> All of the code in this patch series is older than 2022 as I know it has
>>>> been in shipping devices for many years. Please use the proper
>>>> copyright year to make your lawyers happy...
>
>>> Are you *positive* about this. Based on some preparatory discussions
>>> the Qualcomm people had with Takashi and I it seemed like this was a new
>>> version of existing concepts. I'm sure they had something already but
>>> it's not obvious to me that they're just posting the same code.
>
>> I thought that this same code has been shipping in devices for a few
>> years now in the last few Samsung phone models. Is this not the same
>> code that is in those devices?
>
>> If not, why not, what happened to that codebase that makes it not worthy
>> of being submitted upstream?
>
> I don't specifically know anything about that code but I'd expect that
> for out of tree code breaking new ground like this there'd be a strong
> likelyhood that there'd be design level issues and that's what the pre
> submission discussions were all about - how to fit the concept into the
> kernel subsystems in a way that might be maintainable. There's also
> been the whole transition to their new DSP firmware going on. It's
> possible that what was shipped was implemented in the same way with the
> same code but I'd not assume that this is the case without actually
> comparing the two.

It's correct that all the ASoC related patches are new, and didn't exist
previously in QC products. It is due to the fact that Android has a
different ALSA userspace concept which allowed for a lot of the Q6 audio
front end (AFE) communication to be done from userspace, I believe.
This is the reason that we had to introduce this new ASoC based design.

I can't comment much more about the Android ALSA design, maybe @Patrick
Lai can.

Thanks
Wesley Cheng

2023-01-05 00:06:36

by Pierre-Louis Bossart

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6


> +int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
> + int connected)
> +{
> + struct snd_soc_dapm_context *dapm;
> + struct q6usb_port_data *data;
> +
> + if (!usb->component)
> + return 0;
> +
> + dapm = snd_soc_component_get_dapm(usb->component);
> + data = dev_get_drvdata(usb->component->dev);
> +
> + if (connected) {
> + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
> + /* We only track the latest USB headset plugged in */

that answers to my earlier question on how to deal with multiple
devices, but is this a desirable policy? This could lead to a lot of
confusion. If there are restrictions to a single device, then it might
be more interesting for userspace or the user to indicate which USB
device gets to use USB offload and all others use legacy.

> + data->active_idx = card_idx;
> + } else {
> + snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
> + }
> + snd_soc_dapm_sync(dapm);
> +
> + return 0;
> +}

2023-01-06 01:14:14

by Wesley Cheng

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6

Hi Pierre,

On 1/4/2023 3:41 PM, Pierre-Louis Bossart wrote:
>
>> +int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>> + int connected)
>> +{
>> + struct snd_soc_dapm_context *dapm;
>> + struct q6usb_port_data *data;
>> +
>> + if (!usb->component)
>> + return 0;
>> +
>> + dapm = snd_soc_component_get_dapm(usb->component);
>> + data = dev_get_drvdata(usb->component->dev);
>> +
>> + if (connected) {
>> + snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>> + /* We only track the latest USB headset plugged in */
>
> that answers to my earlier question on how to deal with multiple
> devices, but is this a desirable policy? This could lead to a lot of
> confusion. If there are restrictions to a single device, then it might
> be more interesting for userspace or the user to indicate which USB
> device gets to use USB offload and all others use legacy.
>

Yeah, as mentioned its still pretty open ended. I think from the
feedback received from Mark/Takashi, this was a viable option for now.
Would you happen to have any insight/input on how the userspace can pass
down that selection to the ASoC framework? Maybe some kind of PCM IOCTL
call?

Thanks
Wesley Cheng

2023-01-07 00:35:40

by Pierre-Louis Bossart

[permalink] [raw]
Subject: Re: [RFC PATCH 03/14] ASoC: qcom: Add USB backend ASoC driver for Q6



On 1/5/23 19:05, Wesley Cheng wrote:
> Hi Pierre,
>
> On 1/4/2023 3:41 PM, Pierre-Louis Bossart wrote:
>>
>>> +int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>>> +            int connected)
>>> +{
>>> +    struct snd_soc_dapm_context *dapm;
>>> +    struct q6usb_port_data *data;
>>> +
>>> +    if (!usb->component)
>>> +        return 0;
>>> +
>>> +    dapm = snd_soc_component_get_dapm(usb->component);
>>> +    data = dev_get_drvdata(usb->component->dev);
>>> +
>>> +    if (connected) {
>>> +        snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>>> +        /* We only track the latest USB headset plugged in */
>>
>> that answers to my earlier question on how to deal with multiple
>> devices, but is this a desirable policy? This could lead to a lot of
>> confusion. If there are restrictions to a single device, then it might
>> be more interesting for userspace or the user to indicate which USB
>> device gets to use USB offload and all others use legacy.
>>
>
> Yeah, as mentioned its still pretty open ended.  I think from the
> feedback received from Mark/Takashi, this was a viable option for now.
> Would you happen to have any insight/input on how the userspace can pass
> down that selection to the ASoC framework?  Maybe some kind of PCM IOCTL
> call?

I don't have a turn-key solution either :-)
We'd need userspace to make one device as 'preferred' or 'optimized' and
give it a priority somehow. It can't be a PCM IOCTL, it has to be at the
device level.

It's really a second-level optimization that can be parked for now, the
bulk of the work is really the interaction between USB audio and ASoC
stacks, we should probably focus on that BIG topic with a design that
can be shared across implementations.