| Message ID | 20240801011730.4797-30-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
> +ifneq ($(CONFIG_SND_USB_QC_OFFLOAD_MIXER),) > +snd-usb-audio-qmi-objs += mixer_usb_offload.o > +endif > \ No newline at end of file add one? > diff --git a/sound/usb/qcom/mixer_usb_offload.c b/sound/usb/qcom/mixer_usb_offload.c > new file mode 100644 > index 000000000000..c00770400c67 > --- /dev/null > +++ b/sound/usb/qcom/mixer_usb_offload.c > @@ -0,0 +1,101 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/usb.h> > + > +#include <sound/core.h> > +#include <sound/control.h> > +#include <sound/soc-usb.h> > + > +#include "../card.h" > +#include "../mixer.h" > +#include "../usbaudio.h" > + > +#include "mixer_usb_offload.h" > + > +#define PCM_IDX(n) (n & 0xffff) > +#define CARD_IDX(n) (n >> 16) > + > +static int > +snd_usb_offload_route_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct device *sysdev = snd_kcontrol_chip(kcontrol); > + int card; > + int pcm; > + > + card = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), > + PCM_IDX(kcontrol->private_value), > + SND_SOC_USB_KCTL_CARD_ROUTE); > + > + pcm = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), > + PCM_IDX(kcontrol->private_value), > + SND_SOC_USB_KCTL_PCM_ROUTE); > + if (card < 0 || pcm < 0) { > + card = -1; > + pcm = -1; > + } > + > + ucontrol->value.integer.value[0] = card; > + ucontrol->value.integer.value[1] = pcm; > + > + return 0; > +} see my earlier comment, should those two calls be collapsed to return all the information in one shot? > + > +static int snd_usb_offload_route_info(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_info *uinfo) > +{ > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = 2; > + uinfo->value.integer.min = -1; > + /* Arbitrary max value, as there is no 'limit' on number of PCM devices */ > + uinfo->value.integer.max = 0xff; > + > + return 0; > +} > + > +static struct snd_kcontrol_new snd_usb_offload_mapped_ctl = { > + .iface = SNDRV_CTL_ELEM_IFACE_CARD, > + .access = SNDRV_CTL_ELEM_ACCESS_READ, > + .info = snd_usb_offload_route_info, > + .get = snd_usb_offload_route_get, > +}; > + > +/** > + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer > + * @chip - USB SND chip device > + * > + * Creates a sound control for a USB audio device, so that applications can > + * query for if there is an available USB audio offload path, and which > + * card is managing it. > + */ > +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) > +{ > + struct usb_device *udev = chip->dev; > + struct snd_kcontrol_new *chip_kctl; > + struct snd_usb_stream *as; > + char ctl_name[37]; > + int ret; > + > + list_for_each_entry(as, &chip->pcm_list, list) { > + chip_kctl = &snd_usb_offload_mapped_ctl; > + chip_kctl->count = 1; > + /* > + * Store the associated USB SND card number and PCM index for > + * the kctl. > + */ > + chip_kctl->private_value = as->pcm_index | > + chip->card->number << 16; > + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", > + as->pcm_index); > + chip_kctl->name = ctl_name; > + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, > + udev->bus->sysdev)); > + if (ret < 0) > + break; > + } > + > + return ret; None of this looks Qualcomm-specific, shouldn't this be part of the soc_usb framework instead of being added in the qcom/ stuff?
Hi Pierre, On 8/1/2024 2:02 AM, Pierre-Louis Bossart wrote: > >> +ifneq ($(CONFIG_SND_USB_QC_OFFLOAD_MIXER),) >> +snd-usb-audio-qmi-objs += mixer_usb_offload.o >> +endif >> \ No newline at end of file > add one? > >> diff --git a/sound/usb/qcom/mixer_usb_offload.c b/sound/usb/qcom/mixer_usb_offload.c >> new file mode 100644 >> index 000000000000..c00770400c67 >> --- /dev/null >> +++ b/sound/usb/qcom/mixer_usb_offload.c >> @@ -0,0 +1,101 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/usb.h> >> + >> +#include <sound/core.h> >> +#include <sound/control.h> >> +#include <sound/soc-usb.h> >> + >> +#include "../card.h" >> +#include "../mixer.h" >> +#include "../usbaudio.h" >> + >> +#include "mixer_usb_offload.h" >> + >> +#define PCM_IDX(n) (n & 0xffff) >> +#define CARD_IDX(n) (n >> 16) >> + >> +static int >> +snd_usb_offload_route_get(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct device *sysdev = snd_kcontrol_chip(kcontrol); >> + int card; >> + int pcm; >> + >> + card = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), >> + PCM_IDX(kcontrol->private_value), >> + SND_SOC_USB_KCTL_CARD_ROUTE); >> + >> + pcm = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), >> + PCM_IDX(kcontrol->private_value), >> + SND_SOC_USB_KCTL_PCM_ROUTE); >> + if (card < 0 || pcm < 0) { >> + card = -1; >> + pcm = -1; >> + } >> + >> + ucontrol->value.integer.value[0] = card; >> + ucontrol->value.integer.value[1] = pcm; >> + >> + return 0; >> +} > see my earlier comment, should those two calls be collapsed to return > all the information in one shot? > >> + >> +static int snd_usb_offload_route_info(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_info *uinfo) >> +{ >> + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; >> + uinfo->count = 2; >> + uinfo->value.integer.min = -1; >> + /* Arbitrary max value, as there is no 'limit' on number of PCM devices */ >> + uinfo->value.integer.max = 0xff; >> + >> + return 0; >> +} >> + >> +static struct snd_kcontrol_new snd_usb_offload_mapped_ctl = { >> + .iface = SNDRV_CTL_ELEM_IFACE_CARD, >> + .access = SNDRV_CTL_ELEM_ACCESS_READ, >> + .info = snd_usb_offload_route_info, >> + .get = snd_usb_offload_route_get, >> +}; >> + >> +/** >> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >> + * @chip - USB SND chip device >> + * >> + * Creates a sound control for a USB audio device, so that applications can >> + * query for if there is an available USB audio offload path, and which >> + * card is managing it. >> + */ >> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >> +{ >> + struct usb_device *udev = chip->dev; >> + struct snd_kcontrol_new *chip_kctl; >> + struct snd_usb_stream *as; >> + char ctl_name[37]; >> + int ret; >> + >> + list_for_each_entry(as, &chip->pcm_list, list) { >> + chip_kctl = &snd_usb_offload_mapped_ctl; >> + chip_kctl->count = 1; >> + /* >> + * Store the associated USB SND card number and PCM index for >> + * the kctl. >> + */ >> + chip_kctl->private_value = as->pcm_index | >> + chip->card->number << 16; >> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >> + as->pcm_index); >> + chip_kctl->name = ctl_name; >> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >> + udev->bus->sysdev)); >> + if (ret < 0) >> + break; >> + } >> + >> + return ret; > None of this looks Qualcomm-specific, shouldn't this be part of the > soc_usb framework instead of being added in the qcom/ stuff? Let me see if I can do that. Thanks Wesley Cheng
Hi Pierre, On 8/1/2024 2:02 AM, Pierre-Louis Bossart wrote: > >> +ifneq ($(CONFIG_SND_USB_QC_OFFLOAD_MIXER),) >> +snd-usb-audio-qmi-objs += mixer_usb_offload.o >> +endif >> \ No newline at end of file > add one? > >> diff --git a/sound/usb/qcom/mixer_usb_offload.c b/sound/usb/qcom/mixer_usb_offload.c >> new file mode 100644 >> index 000000000000..c00770400c67 >> --- /dev/null >> +++ b/sound/usb/qcom/mixer_usb_offload.c >> @@ -0,0 +1,101 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/usb.h> >> + >> +#include <sound/core.h> >> +#include <sound/control.h> >> +#include <sound/soc-usb.h> >> + >> +#include "../card.h" >> +#include "../mixer.h" >> +#include "../usbaudio.h" >> + >> +#include "mixer_usb_offload.h" >> + >> +#define PCM_IDX(n) (n & 0xffff) >> +#define CARD_IDX(n) (n >> 16) >> + >> +static int >> +snd_usb_offload_route_get(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct device *sysdev = snd_kcontrol_chip(kcontrol); >> + int card; >> + int pcm; >> + >> + card = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), >> + PCM_IDX(kcontrol->private_value), >> + SND_SOC_USB_KCTL_CARD_ROUTE); >> + >> + pcm = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), >> + PCM_IDX(kcontrol->private_value), >> + SND_SOC_USB_KCTL_PCM_ROUTE); >> + if (card < 0 || pcm < 0) { >> + card = -1; >> + pcm = -1; >> + } >> + >> + ucontrol->value.integer.value[0] = card; >> + ucontrol->value.integer.value[1] = pcm; >> + >> + return 0; >> +} > see my earlier comment, should those two calls be collapsed to return > all the information in one shot? > >> + >> +static int snd_usb_offload_route_info(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_info *uinfo) >> +{ >> + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; >> + uinfo->count = 2; >> + uinfo->value.integer.min = -1; >> + /* Arbitrary max value, as there is no 'limit' on number of PCM devices */ >> + uinfo->value.integer.max = 0xff; >> + >> + return 0; >> +} >> + >> +static struct snd_kcontrol_new snd_usb_offload_mapped_ctl = { >> + .iface = SNDRV_CTL_ELEM_IFACE_CARD, >> + .access = SNDRV_CTL_ELEM_ACCESS_READ, >> + .info = snd_usb_offload_route_info, >> + .get = snd_usb_offload_route_get, >> +}; >> + >> +/** >> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >> + * @chip - USB SND chip device >> + * >> + * Creates a sound control for a USB audio device, so that applications can >> + * query for if there is an available USB audio offload path, and which >> + * card is managing it. >> + */ >> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >> +{ >> + struct usb_device *udev = chip->dev; >> + struct snd_kcontrol_new *chip_kctl; >> + struct snd_usb_stream *as; >> + char ctl_name[37]; >> + int ret; >> + >> + list_for_each_entry(as, &chip->pcm_list, list) { >> + chip_kctl = &snd_usb_offload_mapped_ctl; >> + chip_kctl->count = 1; >> + /* >> + * Store the associated USB SND card number and PCM index for >> + * the kctl. >> + */ >> + chip_kctl->private_value = as->pcm_index | >> + chip->card->number << 16; >> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >> + as->pcm_index); >> + chip_kctl->name = ctl_name; >> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >> + udev->bus->sysdev)); >> + if (ret < 0) >> + break; >> + } >> + >> + return ret; Hi Pierre, > None of this looks Qualcomm-specific, shouldn't this be part of the > soc_usb framework instead of being added in the qcom/ stuff? Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB: 1. We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev. However, I'm not sure if everyone would potentially follow that way. 2. I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip. Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications. It is possible, but definitely not as clean as how we have it now IMO. Thanks Wesley Cheng
>>> +/** >>> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >>> + * @chip - USB SND chip device >>> + * >>> + * Creates a sound control for a USB audio device, so that applications can >>> + * query for if there is an available USB audio offload path, and which >>> + * card is managing it. >>> + */ >>> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >>> +{ >>> + struct usb_device *udev = chip->dev; >>> + struct snd_kcontrol_new *chip_kctl; >>> + struct snd_usb_stream *as; >>> + char ctl_name[37]; >>> + int ret; >>> + >>> + list_for_each_entry(as, &chip->pcm_list, list) { >>> + chip_kctl = &snd_usb_offload_mapped_ctl; >>> + chip_kctl->count = 1; >>> + /* >>> + * Store the associated USB SND card number and PCM index for >>> + * the kctl. >>> + */ >>> + chip_kctl->private_value = as->pcm_index | >>> + chip->card->number << 16; >>> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >>> + as->pcm_index); >>> + chip_kctl->name = ctl_name; >>> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >>> + udev->bus->sysdev)); >>> + if (ret < 0) >>> + break; >>> + } >>> + >>> + return ret; > Hi Pierre, >> None of this looks Qualcomm-specific, shouldn't this be part of the >> soc_usb framework instead of being added in the qcom/ stuff? > > Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB: > > 1. We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev. However, I'm not sure if everyone would potentially follow that way. > > 2. I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip. > > Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications. It is possible, but definitely not as clean as how we have it now IMO. My comment was only referring to the location of the code, it's now in sound/usb/qcom/mixer_usb_offload.c but does not contain anything specific to Qualcomm. I was not asking for any encapsulation inside of soc-usb, I was only suggesting a move of the code to a shared helper library so that this code can be reused as is and not duplicated if the QCOM parts are not compiled in.
Hi Pierre, On 8/19/2024 11:39 PM, Pierre-Louis Bossart wrote: >>>> +/** >>>> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >>>> + * @chip - USB SND chip device >>>> + * >>>> + * Creates a sound control for a USB audio device, so that applications can >>>> + * query for if there is an available USB audio offload path, and which >>>> + * card is managing it. >>>> + */ >>>> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >>>> +{ >>>> + struct usb_device *udev = chip->dev; >>>> + struct snd_kcontrol_new *chip_kctl; >>>> + struct snd_usb_stream *as; >>>> + char ctl_name[37]; >>>> + int ret; >>>> + >>>> + list_for_each_entry(as, &chip->pcm_list, list) { >>>> + chip_kctl = &snd_usb_offload_mapped_ctl; >>>> + chip_kctl->count = 1; >>>> + /* >>>> + * Store the associated USB SND card number and PCM index for >>>> + * the kctl. >>>> + */ >>>> + chip_kctl->private_value = as->pcm_index | >>>> + chip->card->number << 16; >>>> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >>>> + as->pcm_index); >>>> + chip_kctl->name = ctl_name; >>>> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >>>> + udev->bus->sysdev)); >>>> + if (ret < 0) >>>> + break; >>>> + } >>>> + >>>> + return ret; >> Hi Pierre, >>> None of this looks Qualcomm-specific, shouldn't this be part of the >>> soc_usb framework instead of being added in the qcom/ stuff? >> Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB: >> >> 1. We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev. However, I'm not sure if everyone would potentially follow that way. >> >> 2. I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip. >> >> Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications. It is possible, but definitely not as clean as how we have it now IMO. > My comment was only referring to the location of the code, it's now in > sound/usb/qcom/mixer_usb_offload.c but does not contain anything > specific to Qualcomm. I was not asking for any encapsulation inside of > soc-usb, I was only suggesting a move of the code to a shared helper > library so that this code can be reused as is and not duplicated if the > QCOM parts are not compiled in. Ah, great, thanks for the clarification. Let me take a look with that perspective. Thanks Wesley Cheng
Hi Pierre, On 8/20/2024 10:37 AM, Wesley Cheng wrote: > Hi Pierre, > > On 8/19/2024 11:39 PM, Pierre-Louis Bossart wrote: >>>>> +/** >>>>> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >>>>> + * @chip - USB SND chip device >>>>> + * >>>>> + * Creates a sound control for a USB audio device, so that applications can >>>>> + * query for if there is an available USB audio offload path, and which >>>>> + * card is managing it. >>>>> + */ >>>>> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >>>>> +{ >>>>> + struct usb_device *udev = chip->dev; >>>>> + struct snd_kcontrol_new *chip_kctl; >>>>> + struct snd_usb_stream *as; >>>>> + char ctl_name[37]; >>>>> + int ret; >>>>> + >>>>> + list_for_each_entry(as, &chip->pcm_list, list) { >>>>> + chip_kctl = &snd_usb_offload_mapped_ctl; >>>>> + chip_kctl->count = 1; >>>>> + /* >>>>> + * Store the associated USB SND card number and PCM index for >>>>> + * the kctl. >>>>> + */ >>>>> + chip_kctl->private_value = as->pcm_index | >>>>> + chip->card->number << 16; >>>>> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >>>>> + as->pcm_index); >>>>> + chip_kctl->name = ctl_name; >>>>> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >>>>> + udev->bus->sysdev)); >>>>> + if (ret < 0) >>>>> + break; >>>>> + } >>>>> + >>>>> + return ret; >>> Hi Pierre, >>>> None of this looks Qualcomm-specific, shouldn't this be part of the >>>> soc_usb framework instead of being added in the qcom/ stuff? >>> Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB: >>> >>> 1. We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev. However, I'm not sure if everyone would potentially follow that way. >>> >>> 2. I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip. >>> >>> Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications. It is possible, but definitely not as clean as how we have it now IMO. >> My comment was only referring to the location of the code, it's now in >> sound/usb/qcom/mixer_usb_offload.c but does not contain anything >> specific to Qualcomm. I was not asking for any encapsulation inside of >> soc-usb, I was only suggesting a move of the code to a shared helper >> library so that this code can be reused as is and not duplicated if the >> QCOM parts are not compiled in. > Ah, great, thanks for the clarification. Let me take a look with that perspective. > Going back on the history behind moving it into qcom/ was based off feedback that Takashi pointed out in v14[1]. It was mainly due to the fact that we would be adding another hard dependency between USB SND and the offloading components. Hence the reason for moving it to within the QCOM offloading package. Thanks Wesley Cheng [1]: https://lore.kernel.org/linux-usb/87y1bt2acg.wl-tiwai@suse.de/
>>>>>> +/** >>>>>> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >>>>>> + * @chip - USB SND chip device >>>>>> + * >>>>>> + * Creates a sound control for a USB audio device, so that applications can >>>>>> + * query for if there is an available USB audio offload path, and which >>>>>> + * card is managing it. >>>>>> + */ >>>>>> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >>>>>> +{ >>>>>> + struct usb_device *udev = chip->dev; >>>>>> + struct snd_kcontrol_new *chip_kctl; >>>>>> + struct snd_usb_stream *as; >>>>>> + char ctl_name[37]; >>>>>> + int ret; >>>>>> + >>>>>> + list_for_each_entry(as, &chip->pcm_list, list) { >>>>>> + chip_kctl = &snd_usb_offload_mapped_ctl; >>>>>> + chip_kctl->count = 1; >>>>>> + /* >>>>>> + * Store the associated USB SND card number and PCM index for >>>>>> + * the kctl. >>>>>> + */ >>>>>> + chip_kctl->private_value = as->pcm_index | >>>>>> + chip->card->number << 16; >>>>>> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >>>>>> + as->pcm_index); >>>>>> + chip_kctl->name = ctl_name; >>>>>> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >>>>>> + udev->bus->sysdev)); >>>>>> + if (ret < 0) >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + return ret; >>>> Hi Pierre, >>>>> None of this looks Qualcomm-specific, shouldn't this be part of the >>>>> soc_usb framework instead of being added in the qcom/ stuff? >>>> Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB: >>>> >>>> 1. We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev. However, I'm not sure if everyone would potentially follow that way. >>>> >>>> 2. I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip. >>>> >>>> Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications. It is possible, but definitely not as clean as how we have it now IMO. >>> My comment was only referring to the location of the code, it's now in >>> sound/usb/qcom/mixer_usb_offload.c but does not contain anything >>> specific to Qualcomm. I was not asking for any encapsulation inside of >>> soc-usb, I was only suggesting a move of the code to a shared helper >>> library so that this code can be reused as is and not duplicated if the >>> QCOM parts are not compiled in. >> Ah, great, thanks for the clarification. Let me take a look with that perspective. >> > Going back on the history behind moving it into qcom/ was based off feedback that Takashi pointed out in v14[1]. It was mainly due to the fact that we would be adding another hard dependency between USB SND and the offloading components. Hence the reason for moving it to within the QCOM offloading package. > > Thanks > > Wesley Cheng > > [1]: https://lore.kernel.org/linux-usb/87y1bt2acg.wl-tiwai@suse.de/ I don't see anything wrong with the initial proposal +config SND_USB_OFFLOAD_MIXER + bool + config SND_USB_AUDIO_QMI tristate "Qualcomm Audio Offload driver" depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND select SND_PCM + select SND_USB_OFFLOAD_MIXER That would allows the SND_USB_OFFLOAD_MIXER to be build as a module, and it would allow other non-QCON solutions to use the module. Maybe just make it a tristate?
On 8/21/2024 12:02 AM, Pierre-Louis Bossart wrote: > > >>>>>>> +/** >>>>>>> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer >>>>>>> + * @chip - USB SND chip device >>>>>>> + * >>>>>>> + * Creates a sound control for a USB audio device, so that applications can >>>>>>> + * query for if there is an available USB audio offload path, and which >>>>>>> + * card is managing it. >>>>>>> + */ >>>>>>> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) >>>>>>> +{ >>>>>>> + struct usb_device *udev = chip->dev; >>>>>>> + struct snd_kcontrol_new *chip_kctl; >>>>>>> + struct snd_usb_stream *as; >>>>>>> + char ctl_name[37]; >>>>>>> + int ret; >>>>>>> + >>>>>>> + list_for_each_entry(as, &chip->pcm_list, list) { >>>>>>> + chip_kctl = &snd_usb_offload_mapped_ctl; >>>>>>> + chip_kctl->count = 1; >>>>>>> + /* >>>>>>> + * Store the associated USB SND card number and PCM index for >>>>>>> + * the kctl. >>>>>>> + */ >>>>>>> + chip_kctl->private_value = as->pcm_index | >>>>>>> + chip->card->number << 16; >>>>>>> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", >>>>>>> + as->pcm_index); >>>>>>> + chip_kctl->name = ctl_name; >>>>>>> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, >>>>>>> + udev->bus->sysdev)); >>>>>>> + if (ret < 0) >>>>>>> + break; >>>>>>> + } >>>>>>> + >>>>>>> + return ret; >>>>> Hi Pierre, >>>>>> None of this looks Qualcomm-specific, shouldn't this be part of the >>>>>> soc_usb framework instead of being added in the qcom/ stuff? >>>>> Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB: >>>>> >>>>> 1. We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev. However, I'm not sure if everyone would potentially follow that way. >>>>> >>>>> 2. I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip. >>>>> >>>>> Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications. It is possible, but definitely not as clean as how we have it now IMO. >>>> My comment was only referring to the location of the code, it's now in >>>> sound/usb/qcom/mixer_usb_offload.c but does not contain anything >>>> specific to Qualcomm. I was not asking for any encapsulation inside of >>>> soc-usb, I was only suggesting a move of the code to a shared helper >>>> library so that this code can be reused as is and not duplicated if the >>>> QCOM parts are not compiled in. >>> Ah, great, thanks for the clarification. Let me take a look with that perspective. >>> >> Going back on the history behind moving it into qcom/ was based off feedback that Takashi pointed out in v14[1]. It was mainly due to the fact that we would be adding another hard dependency between USB SND and the offloading components. Hence the reason for moving it to within the QCOM offloading package. >> >> Thanks >> >> Wesley Cheng >> >> [1]: https://lore.kernel.org/linux-usb/87y1bt2acg.wl-tiwai@suse.de/ > I don't see anything wrong with the initial proposal > > > +config SND_USB_OFFLOAD_MIXER > + bool > + > config SND_USB_AUDIO_QMI > tristate "Qualcomm Audio Offload driver" > depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND > select SND_PCM > + select SND_USB_OFFLOAD_MIXER > > > That would allows the SND_USB_OFFLOAD_MIXER to be build as a module, and > it would allow other non-QCON solutions to use the module. > Maybe just make it a tristate? I think the main concern was that in the initial suggestion, I made it as part of usb-snd-audio and the call to create the kcontrol was made in the main USB SND mixer driver. Maybe in the new proposal, we'll just have it as its own independent module and allow for other vendors to use the same (kcontrol creation done by vendor USB offload driver)? Takashi, would this be acceptable? Thanks Wesley Cheng
diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig index 5cc3eaf53e6b..1228be3c1f83 100644 --- a/sound/usb/Kconfig +++ b/sound/usb/Kconfig @@ -176,6 +176,16 @@ config SND_BCD2000 To compile this driver as a module, choose M here: the module will be called snd-bcd2000. +config SND_USB_QC_OFFLOAD_MIXER + bool "Qualcomm USB Audio Offload mixer control" + help + Say Y to enable the Qualcomm USB audio offloading mixer controls. + This exposes an USB offload capable kcontrol to signal to + applications about which platform sound card can support USB + audio offload. This can potentially be used to fetch further + information about the offloading status from the platform sound + card. + config SND_USB_AUDIO_QMI tristate "Qualcomm Audio Offload driver" depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND && SND_SOC_USB diff --git a/sound/usb/qcom/Makefile b/sound/usb/qcom/Makefile index a81c9b28d484..eada5cd7b71f 100644 --- a/sound/usb/qcom/Makefile +++ b/sound/usb/qcom/Makefile @@ -1,2 +1,6 @@ snd-usb-audio-qmi-objs := usb_audio_qmi_v01.o qc_audio_offload.o -obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o \ No newline at end of file +obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o + +ifneq ($(CONFIG_SND_USB_QC_OFFLOAD_MIXER),) +snd-usb-audio-qmi-objs += mixer_usb_offload.o +endif \ No newline at end of file diff --git a/sound/usb/qcom/mixer_usb_offload.c b/sound/usb/qcom/mixer_usb_offload.c new file mode 100644 index 000000000000..c00770400c67 --- /dev/null +++ b/sound/usb/qcom/mixer_usb_offload.c @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include <linux/usb.h> + +#include <sound/core.h> +#include <sound/control.h> +#include <sound/soc-usb.h> + +#include "../card.h" +#include "../mixer.h" +#include "../usbaudio.h" + +#include "mixer_usb_offload.h" + +#define PCM_IDX(n) (n & 0xffff) +#define CARD_IDX(n) (n >> 16) + +static int +snd_usb_offload_route_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct device *sysdev = snd_kcontrol_chip(kcontrol); + int card; + int pcm; + + card = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), + PCM_IDX(kcontrol->private_value), + SND_SOC_USB_KCTL_CARD_ROUTE); + + pcm = soc_usb_get_offload_device(sysdev, CARD_IDX(kcontrol->private_value), + PCM_IDX(kcontrol->private_value), + SND_SOC_USB_KCTL_PCM_ROUTE); + if (card < 0 || pcm < 0) { + card = -1; + pcm = -1; + } + + ucontrol->value.integer.value[0] = card; + ucontrol->value.integer.value[1] = pcm; + + return 0; +} + +static int snd_usb_offload_route_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; + uinfo->count = 2; + uinfo->value.integer.min = -1; + /* Arbitrary max value, as there is no 'limit' on number of PCM devices */ + uinfo->value.integer.max = 0xff; + + return 0; +} + +static struct snd_kcontrol_new snd_usb_offload_mapped_ctl = { + .iface = SNDRV_CTL_ELEM_IFACE_CARD, + .access = SNDRV_CTL_ELEM_ACCESS_READ, + .info = snd_usb_offload_route_info, + .get = snd_usb_offload_route_get, +}; + +/** + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer + * @chip - USB SND chip device + * + * Creates a sound control for a USB audio device, so that applications can + * query for if there is an available USB audio offload path, and which + * card is managing it. + */ +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) +{ + struct usb_device *udev = chip->dev; + struct snd_kcontrol_new *chip_kctl; + struct snd_usb_stream *as; + char ctl_name[37]; + int ret; + + list_for_each_entry(as, &chip->pcm_list, list) { + chip_kctl = &snd_usb_offload_mapped_ctl; + chip_kctl->count = 1; + /* + * Store the associated USB SND card number and PCM index for + * the kctl. + */ + chip_kctl->private_value = as->pcm_index | + chip->card->number << 16; + sprintf(ctl_name, "USB Offload Playback Route PCM#%d", + as->pcm_index); + chip_kctl->name = ctl_name; + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl, + udev->bus->sysdev)); + if (ret < 0) + break; + } + + return ret; +} diff --git a/sound/usb/qcom/mixer_usb_offload.h b/sound/usb/qcom/mixer_usb_offload.h new file mode 100644 index 000000000000..0efda52e92b6 --- /dev/null +++ b/sound/usb/qcom/mixer_usb_offload.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef __USB_OFFLOAD_MIXER_H +#define __USB_OFFLOAD_MIXER_H + +#if IS_ENABLED(CONFIG_SND_USB_QC_OFFLOAD_MIXER) +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip); +#else +static inline int snd_usb_offload_create_ctl(struct snd_usb_audio *chip) +{ + return -ENODEV; +} +#endif +#endif /* __USB_OFFLOAD_MIXER_H */ diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c index d9320ebad617..b862aa15aa37 100644 --- a/sound/usb/qcom/qc_audio_offload.c +++ b/sound/usb/qcom/qc_audio_offload.c @@ -37,6 +37,7 @@ #include "../pcm.h" #include "../power.h" +#include "mixer_usb_offload.h" #include "usb_audio_qmi_v01.h" /* Stream disable request timeout during USB device disconnect */ @@ -1678,6 +1679,9 @@ static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip) snd_soc_usb_connect(usb_get_usb_backend(udev), sdev); + if (chip->num_interfaces == 1) + snd_usb_offload_create_ctl(chip); + mutex_unlock(&chip->mutex); mutex_unlock(&qdev_mutex);
In order to allow userspace/applications know about USB offloading status, expose a sound kcontrol that fetches information about which sound card and PCM index the USB device is mapped to for supporting offloading. In the USB audio offloading framework, the ASoC BE DAI link is the entity responsible for registering to the SOC USB layer. It is expected for the USB SND offloading driver to add the kcontrol to the sound card associated with the USB audio device. An example output would look like: tinymix -D 1 get 'USB Offload Playback Route PCM#0' -1, -1 (range -1->255) This example signifies that there is no mapped ASoC path available for the USB SND device. tinymix -D 1 get 'USB Offload Playback Route PCM#0' 0, 0 (range -1->255) This example signifies that the offload path is available over ASoC sound card index#0 and PCM device#0. Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> --- sound/usb/Kconfig | 10 +++ sound/usb/qcom/Makefile | 6 +- sound/usb/qcom/mixer_usb_offload.c | 101 +++++++++++++++++++++++++++++ sound/usb/qcom/mixer_usb_offload.h | 17 +++++ sound/usb/qcom/qc_audio_offload.c | 4 ++ 5 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 sound/usb/qcom/mixer_usb_offload.c create mode 100644 sound/usb/qcom/mixer_usb_offload.h