diff mbox series

[v30,28/30] ALSA: usb-audio: Add USB offload route kcontrol

Message ID 20241106193413.1730413-29-quic_wcheng@quicinc.com
State New
Headers show
Series Introduce QC USB SND audio offloading support | expand

Commit Message

Wesley Cheng Nov. 6, 2024, 7:34 p.m. UTC
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.

The USB offload kcontrol will be added in addition to the existing
kcontrols identified by the USB SND mixer.  The kcontrols used to modify
the USB audio device specific parameters are still valid and expected to be
used.  These parameters are not mirrored to the ASoC subsystem.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 sound/usb/Kconfig                 |  10 +++
 sound/usb/Makefile                |   2 +
 sound/usb/mixer_usb_offload.c     | 102 ++++++++++++++++++++++++++++++
 sound/usb/mixer_usb_offload.h     |  17 +++++
 sound/usb/qcom/Makefile           |   2 +-
 sound/usb/qcom/qc_audio_offload.c |   2 +
 6 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 sound/usb/mixer_usb_offload.c
 create mode 100644 sound/usb/mixer_usb_offload.h

Comments

Takashi Iwai Nov. 20, 2024, 12:12 p.m. UTC | #1
On Wed, 06 Nov 2024 20:34:11 +0100,
Wesley Cheng wrote:
> 
> 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.
> 
> The USB offload kcontrol will be added in addition to the existing
> kcontrols identified by the USB SND mixer.  The kcontrols used to modify
> the USB audio device specific parameters are still valid and expected to be
> used.  These parameters are not mirrored to the ASoC subsystem.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>

IIRC, this representation of kcontrol was one argued issue; Pierre
expressed the concern about the complexity of the kcontrol.
I didn't follow exactly, but did we get consensus?

Apart from that: the Kconfig defition below ...

> +config SND_USB_OFFLOAD_MIXER
> +	tristate "USB Audio Offload mixer control"
> +	help
> +	 Say Y to enable the 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.
> +	 The returning values specify the mapped ASoC card and PCM device
> +	 the USB audio device is associated to.

... and Makefile addition below ...

> --- a/sound/usb/Makefile
> +++ b/sound/usb/Makefile
> @@ -36,3 +36,5 @@ obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
>  
>  obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ qcom/
>  obj-$(CONFIG_SND_USB_LINE6)	+= line6/
> +
> +obj-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o

... indicates that this code will be an individual module, although
it's solely used from snd-usb-audio-qmi driver.  This should be rather
a boolean and moved to sound/usb/qcom/, and linked to
snd-usb-audio-qmi driver itself, e.g.

--- a/sound/usb/qcom/Makefile
+++ b/sound/usb/qcom/Makefile
@@ -1,2 +1,3 @@
 snd-usb-audio-qmi-objs := usb_audio_qmi_v01.o qc_audio_offload.o
+snd-usb-audio-qmi-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
 obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o

Then you can drop EXPORT_SYMBOL_GPL(), too.


thanks,

Takashi
Wesley Cheng Nov. 20, 2024, 7:13 p.m. UTC | #2
Hi Takashi,

On 11/20/2024 4:12 AM, Takashi Iwai wrote:
> On Wed, 06 Nov 2024 20:34:11 +0100,
> Wesley Cheng wrote:
>> 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.
>>
>> The USB offload kcontrol will be added in addition to the existing
>> kcontrols identified by the USB SND mixer.  The kcontrols used to modify
>> the USB audio device specific parameters are still valid and expected to be
>> used.  These parameters are not mirrored to the ASoC subsystem.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> IIRC, this representation of kcontrol was one argued issue; Pierre
> expressed the concern about the complexity of the kcontrol.
> I didn't follow exactly, but did we get consensus?
So the part that Pierre had concerns on was that previously, the implementation was placing offload kcontrols to the ASoC platform card, and had some additional controls that complicated the offload implementation about the offload status for each USB audio device.  This was discussed here:

https://lore.kernel.org/linux-usb/957b3c13-e4ba-45e3-b880-7a313e48c33f@quicinc.com/

To summarize, I made the decision to move the offload status kcontrols from ASoC --> USB SND and limited it to only one kcontrol (mapped offload device).  So now, there exists a kcontrol for every USB SND device (if the offload mixer is enabled), where it tells userspace the mapped ASoC platform card and pcm device that handles USB offloading, else you'll see the "-1, -1" pair, which means offload is not possible for that USB audio device.

> Apart from that: the Kconfig defition below ...
>
>> +config SND_USB_OFFLOAD_MIXER
>> +	tristate "USB Audio Offload mixer control"
>> +	help
>> +	 Say Y to enable the 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.
>> +	 The returning values specify the mapped ASoC card and PCM device
>> +	 the USB audio device is associated to.
> ... and Makefile addition below ...
>
>> --- a/sound/usb/Makefile
>> +++ b/sound/usb/Makefile
>> @@ -36,3 +36,5 @@ obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
>>  
>>  obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ qcom/
>>  obj-$(CONFIG_SND_USB_LINE6)	+= line6/
>> +
>> +obj-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
> ... indicates that this code will be an individual module, although
> it's solely used from snd-usb-audio-qmi driver.  This should be rather
> a boolean and moved to sound/usb/qcom/, and linked to
> snd-usb-audio-qmi driver itself, e.g.
>
> --- a/sound/usb/qcom/Makefile
> +++ b/sound/usb/qcom/Makefile
> @@ -1,2 +1,3 @@
>  snd-usb-audio-qmi-objs := usb_audio_qmi_v01.o qc_audio_offload.o
> +snd-usb-audio-qmi-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
>  obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o
>
> Then you can drop EXPORT_SYMBOL_GPL(), too.

Had a discussion with Pierre on this too below.

https://lore.kernel.org/linux-usb/f507a228-4865-4df5-9215-bc59e330a82f@linux.intel.com/

I remember you commenting to place it in this vendor offload module, which is what I did on v24.

Thanks

Wesley Cheng
Wesley Cheng Nov. 25, 2024, 8:33 p.m. UTC | #3
Hi Takashi,

On 11/21/2024 7:50 AM, Takashi Iwai wrote:
> On Wed, 20 Nov 2024 20:13:34 +0100,
> Wesley Cheng wrote:
>> Hi Takashi,
>>
>> On 11/20/2024 4:12 AM, Takashi Iwai wrote:
>>> On Wed, 06 Nov 2024 20:34:11 +0100,
>>> Wesley Cheng wrote:
>>>> 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.
>>>>
>>>> The USB offload kcontrol will be added in addition to the existing
>>>> kcontrols identified by the USB SND mixer.  The kcontrols used to modify
>>>> the USB audio device specific parameters are still valid and expected to be
>>>> used.  These parameters are not mirrored to the ASoC subsystem.
>>>>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> IIRC, this representation of kcontrol was one argued issue; Pierre
>>> expressed the concern about the complexity of the kcontrol.
>>> I didn't follow exactly, but did we get consensus?
>> So the part that Pierre had concerns on was that previously, the
>>> implementation was placing offload kcontrols to the ASoC platform
>>> card, and had some additional controls that complicated the
>>> offload implementation about the offload status for each USB audio
>>> device.  This was discussed here:
>> https://lore.kernel.org/linux-usb/957b3c13-e4ba-45e3-b880-7a313e48c33f@quicinc.com/
>>
>> To summarize, I made the decision to move the offload status
>> kcontrols from ASoC --> USB SND and limited it to only one kcontrol
>> (mapped offload device).  So now, there exists a kcontrol for every
>> USB SND device (if the offload mixer is enabled), where it tells
>> userspace the mapped ASoC platform card and pcm device that handles
>> USB offloading, else you'll see the "-1, -1" pair, which means
>> offload is not possible for that USB audio device.
> OK, the simplification is good.  But I wonder whether the current
> representation is the best.  Why not just providing two controls per
> PCM, one for card and one for device, instead of two integer array?
> It would look more intuitive to me.
>

I could separate it, but we would have to have a pair of controls for each available USB PCM playback stream supported by the device.  However, before I get into making that change, I think the decision for either two or one FE needs to be decided.  Again, I think the 2 FE approach is much less invasive to the USB SND/ASoC core files, and ensures the legacy USB SND path still works through the non-offloaded data path. 


>>> Apart from that: the Kconfig defition below ...
>>>
>>>> +config SND_USB_OFFLOAD_MIXER
>>>> +	tristate "USB Audio Offload mixer control"
>>>> +	help
>>>> +	 Say Y to enable the 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.
>>>> +	 The returning values specify the mapped ASoC card and PCM device
>>>> +	 the USB audio device is associated to.
>>> ... and Makefile addition below ...
>>>
>>>> --- a/sound/usb/Makefile
>>>> +++ b/sound/usb/Makefile
>>>> @@ -36,3 +36,5 @@ obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
>>>>  
>>>>  obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ qcom/
>>>>  obj-$(CONFIG_SND_USB_LINE6)	+= line6/
>>>> +
>>>> +obj-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
>>> ... indicates that this code will be an individual module, although
>>> it's solely used from snd-usb-audio-qmi driver.  This should be rather
>>> a boolean and moved to sound/usb/qcom/, and linked to
>>> snd-usb-audio-qmi driver itself, e.g.
>>>
>>> --- a/sound/usb/qcom/Makefile
>>> +++ b/sound/usb/qcom/Makefile
>>> @@ -1,2 +1,3 @@
>>>  snd-usb-audio-qmi-objs := usb_audio_qmi_v01.o qc_audio_offload.o
>>> +snd-usb-audio-qmi-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
>>>  obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o
>>>
>>> Then you can drop EXPORT_SYMBOL_GPL(), too.
>> Had a discussion with Pierre on this too below.
>>
>> https://lore.kernel.org/linux-usb/f507a228-4865-4df5-9215-bc59e330a82f@linux.intel.com/
>>
>> I remember you commenting to place it in this vendor offload module,
>> which is what I did on v24.
> I assume that my early comment was based on your old implementations,
> and I guess it was because the mixer part didn't belong to the qcom
> stuff.  Now it belongs solely to qcom, the situation changed; it makes
> no sense to make it an individual module at all.
>
>
I guess Pierre's feedback was that he believed this should be vendor agnostic, because any vendor that could potentially support USB audio offload should have the same kcontrol within the USB SND device.  Hence the reason for keeping it within generic code.  Since QC is the only user of this now.  Do you prefer to make this part of the vendor module for now, until another user comes along and introduces offload support?

Thanks

Wesley Cheng
Wesley Cheng Nov. 26, 2024, 11:19 p.m. UTC | #4
Hi Takashi,

On 11/26/2024 6:14 AM, Takashi Iwai wrote:
> On Mon, 25 Nov 2024 21:33:03 +0100,
> Wesley Cheng wrote:
>> Hi Takashi,
>>
>> On 11/21/2024 7:50 AM, Takashi Iwai wrote:
>>> On Wed, 20 Nov 2024 20:13:34 +0100,
>>> Wesley Cheng wrote:
>>>> Hi Takashi,
>>>>
>>>> On 11/20/2024 4:12 AM, Takashi Iwai wrote:
>>>>> On Wed, 06 Nov 2024 20:34:11 +0100,
>>>>> Wesley Cheng wrote:
>>>>>> 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.
>>>>>>
>>>>>> The USB offload kcontrol will be added in addition to the existing
>>>>>> kcontrols identified by the USB SND mixer.  The kcontrols used to modify
>>>>>> the USB audio device specific parameters are still valid and expected to be
>>>>>> used.  These parameters are not mirrored to the ASoC subsystem.
>>>>>>
>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>> IIRC, this representation of kcontrol was one argued issue; Pierre
>>>>> expressed the concern about the complexity of the kcontrol.
>>>>> I didn't follow exactly, but did we get consensus?
>>>> So the part that Pierre had concerns on was that previously, the
>>>>> implementation was placing offload kcontrols to the ASoC platform
>>>>> card, and had some additional controls that complicated the
>>>>> offload implementation about the offload status for each USB audio
>>>>> device.  This was discussed here:
>>>> https://lore.kernel.org/linux-usb/957b3c13-e4ba-45e3-b880-7a313e48c33f@quicinc.com/
>>>>
>>>> To summarize, I made the decision to move the offload status
>>>> kcontrols from ASoC --> USB SND and limited it to only one kcontrol
>>>> (mapped offload device).  So now, there exists a kcontrol for every
>>>> USB SND device (if the offload mixer is enabled), where it tells
>>>> userspace the mapped ASoC platform card and pcm device that handles
>>>> USB offloading, else you'll see the "-1, -1" pair, which means
>>>> offload is not possible for that USB audio device.
>>> OK, the simplification is good.  But I wonder whether the current
>>> representation is the best.  Why not just providing two controls per
>>> PCM, one for card and one for device, instead of two integer array?
>>> It would look more intuitive to me.
>>>
>> I could separate it, but we would have to have a pair of controls
>> for each available USB PCM playback stream supported by the device.
>> However, before I get into making that change, I think the decision
>> for either two or one FE needs to be decided. Again, I think the 2
>> FE approach is much less invasive to the USB SND/ASoC core files,
>> and ensures the legacy USB SND path still works through the
>> non-offloaded data path.
> Sure, the decision about the 2 FEs is the most significant one, and
> those controls depend on that.


Would like to get closure on that here... (1 vs 2 FEs)  My stance is that the 2 FE approach can be the initial one we can take as it still incorporates the significant blocks that would apply to both situations.  From the userspace perspective, since audio offloading doesn't exist yet, applications should be working off the USB SND device and the SW path.  For those, without changes to look for the offload kcontrol, they can continue to operate off the USB SND PCM devices directly.  If enhancements are made to look for the offload kcontrol, then applications would first refer to the kcontrol of the USB SND device and look for existence of the offload controls.  If present they can then open the ASoC PCM devices.

If we then decided to move to the 1 FE approach in the future, this doesn't affect the userspace entities that added support during the 2 FE stages, because we are always first referring to the USB SND device to query for offload support.  The idea for the 1 FE design is that the userspace doesn't need to check for offload support, it just behaves as if it was opening the USB SND devices. (which should be the base of all applications working with USB SND)


> So my comment assumes that, and if that applied, we need to consider
> which kcontrol representation is better for users.  I don't mind too
> much about that, but generally speaking, simpler representation is
> better in the end, even if it leads to more elements.  e.g. sysfs
> allows basically only one value per file principle, too.
>
>
>>>>> Apart from that: the Kconfig defition below ...
>>>>>
>>>>>> +config SND_USB_OFFLOAD_MIXER
>>>>>> +	tristate "USB Audio Offload mixer control"
>>>>>> +	help
>>>>>> +	 Say Y to enable the 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.
>>>>>> +	 The returning values specify the mapped ASoC card and PCM device
>>>>>> +	 the USB audio device is associated to.
>>>>> ... and Makefile addition below ...
>>>>>
>>>>>> --- a/sound/usb/Makefile
>>>>>> +++ b/sound/usb/Makefile
>>>>>> @@ -36,3 +36,5 @@ obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
>>>>>>  
>>>>>>  obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ qcom/
>>>>>>  obj-$(CONFIG_SND_USB_LINE6)	+= line6/
>>>>>> +
>>>>>> +obj-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
>>>>> ... indicates that this code will be an individual module, although
>>>>> it's solely used from snd-usb-audio-qmi driver.  This should be rather
>>>>> a boolean and moved to sound/usb/qcom/, and linked to
>>>>> snd-usb-audio-qmi driver itself, e.g.
>>>>>
>>>>> --- a/sound/usb/qcom/Makefile
>>>>> +++ b/sound/usb/qcom/Makefile
>>>>> @@ -1,2 +1,3 @@
>>>>>  snd-usb-audio-qmi-objs := usb_audio_qmi_v01.o qc_audio_offload.o
>>>>> +snd-usb-audio-qmi-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
>>>>>  obj-$(CONFIG_SND_USB_AUDIO_QMI) += snd-usb-audio-qmi.o
>>>>>
>>>>> Then you can drop EXPORT_SYMBOL_GPL(), too.
>>>> Had a discussion with Pierre on this too below.
>>>>
>>>> https://lore.kernel.org/linux-usb/f507a228-4865-4df5-9215-bc59e330a82f@linux.intel.com/
>>>>
>>>> I remember you commenting to place it in this vendor offload module,
>>>> which is what I did on v24.
>>> I assume that my early comment was based on your old implementations,
>>> and I guess it was because the mixer part didn't belong to the qcom
>>> stuff.  Now it belongs solely to qcom, the situation changed; it makes
>>> no sense to make it an individual module at all.
>>>
>>>
>> I guess Pierre's feedback was that he believed this should be vendor
>> agnostic, because any vendor that could potentially support USB
>> audio offload should have the same kcontrol within the USB SND
>> device.  Hence the reason for keeping it within generic code.  Since
>> QC is the only user of this now.  Do you prefer to make this part of
>> the vendor module for now, until another user comes along and
>> introduces offload support?
> Yes, less module is preferred for now.  If the stuff is agnostic and
> really used by multiple instances, we can factor out to an individual
> module again.
>

OK, sounds good.  I will make it as part of our package then.

Thanks

Wesley Cheng
Cezary Rojewski Dec. 6, 2024, 9:09 a.m. UTC | #5
On 2024-12-04 12:15 AM, Wesley Cheng wrote:
> 
> On 12/3/2024 8:13 AM, Cezary Rojewski wrote:
>> On 2024-11-06 8:34 PM, Wesley Cheng wrote:
>>> 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.

...

>> R) += mixer_usb_offload.o
>>> diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c
>>> new file mode 100644
>>> index 000000000000..e0689a3b9b86
>>> --- /dev/null
>>> +++ b/sound/usb/mixer_usb_offload.c
>>> @@ -0,0 +1,102 @@
>>> +// 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>
>>
>> ALSA-components should not be dependent on ASoC ones. It should be done the other way around: ALSA <- ASoC.
>>
> 
> At least for this kcontrol, we need to know the status of the ASoC state, so that we can communicate the proper path to userspace.  If the ASoC path is not probed or ready, then this module isn't blocked.  It will just communicate that there isn't a valid offload path.

I'm not asking _why_ you need soc-usb.h header, your reasoning is 
probably perfectly fine. The code hierarchy is not though. If a sound 
module is dependent on soc-xxx.h i.e. ASoC symbols, it shall be part of 
sound/soc/ space.
Wesley Cheng Dec. 6, 2024, 8:43 p.m. UTC | #6
On 12/6/2024 1:09 AM, Cezary Rojewski wrote:
> On 2024-12-04 12:15 AM, Wesley Cheng wrote:
>>
>> On 12/3/2024 8:13 AM, Cezary Rojewski wrote:
>>> On 2024-11-06 8:34 PM, Wesley Cheng wrote:
>>>> 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.
>
> ...
>
>>> R) += mixer_usb_offload.o
>>>> diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c
>>>> new file mode 100644
>>>> index 000000000000..e0689a3b9b86
>>>> --- /dev/null
>>>> +++ b/sound/usb/mixer_usb_offload.c
>>>> @@ -0,0 +1,102 @@
>>>> +// 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>
>>>
>>> ALSA-components should not be dependent on ASoC ones. It should be done the other way around: ALSA <- ASoC.
>>>
>>
>> At least for this kcontrol, we need to know the status of the ASoC state, so that we can communicate the proper path to userspace.  If the ASoC path is not probed or ready, then this module isn't blocked.  It will just communicate that there isn't a valid offload path.
>
> I'm not asking _why_ you need soc-usb.h header, your reasoning is probably perfectly fine. The code hierarchy is not though. If a sound module is dependent on soc-xxx.h i.e. ASoC symbols, it shall be part of sound/soc/ space.


That would basically require a significant change in the current design.  Was that requirement documented somewhere, where ALSA components should not be dependent on ASoC?  What was the reasoning for making it one direction, but not the other?


Thanks

Wesley Cheng
Wesley Cheng Dec. 6, 2024, 11:35 p.m. UTC | #7
On 12/6/2024 1:09 AM, Cezary Rojewski wrote:
> On 2024-12-04 12:15 AM, Wesley Cheng wrote:
>>
>> On 12/3/2024 8:13 AM, Cezary Rojewski wrote:
>>> On 2024-11-06 8:34 PM, Wesley Cheng wrote:
>>>> 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.
>
> ...
>
>>> R) += mixer_usb_offload.o
>>>> diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c
>>>> new file mode 100644
>>>> index 000000000000..e0689a3b9b86
>>>> --- /dev/null
>>>> +++ b/sound/usb/mixer_usb_offload.c
>>>> @@ -0,0 +1,102 @@
>>>> +// 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>
>>>
>>> ALSA-components should not be dependent on ASoC ones. It should be done the other way around: ALSA <- ASoC.
>>>
>>
>> At least for this kcontrol, we need to know the status of the ASoC state, so that we can communicate the proper path to userspace.  If the ASoC path is not probed or ready, then this module isn't blocked.  It will just communicate that there isn't a valid offload path.
>
> I'm not asking _why_ you need soc-usb.h header, your reasoning is probably perfectly fine. The code hierarchy is not though. If a sound module is dependent on soc-xxx.h i.e. ASoC symbols, it shall be part of sound/soc/ space.


I'm still reviewing the HDAudio flow a bit, so please correct me if I'm wrong.  During module initialization, it looks like there will be some overall platform card that will call snd_hdac_ext_bus_init() to create the HDA bus.  I referred to the Intel AVS core.  How do you ensure that the ALSA entities are loaded before this call goes out?  I think once the bus is created dynamic creation/removal of HDA devices is fine, and the hdev_attach/detach is executed. 

Thanks

Wesley Cheng
diff mbox series

Patch

diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig
index d0f65311894f..4994e6261be5 100644
--- a/sound/usb/Kconfig
+++ b/sound/usb/Kconfig
@@ -176,10 +176,20 @@  config SND_BCD2000
 	  To compile this driver as a module, choose M here: the module
 	  will be called snd-bcd2000.
 
+config SND_USB_OFFLOAD_MIXER
+	tristate "USB Audio Offload mixer control"
+	help
+	 Say Y to enable the 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.
+	 The returning values specify the mapped ASoC card and PCM device
+	 the USB audio device is associated to.
+
 config SND_USB_AUDIO_QMI
 	tristate "Qualcomm Audio Offload driver"
 	depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SEC_INTR && SND_SOC_USB
 	select SND_PCM
+	select SND_USB_OFFLOAD_MIXER
 	help
 	  Say Y here to enable the Qualcomm USB audio offloading feature.
 
diff --git a/sound/usb/Makefile b/sound/usb/Makefile
index 54a06a2f73ca..2f19f854944c 100644
--- a/sound/usb/Makefile
+++ b/sound/usb/Makefile
@@ -36,3 +36,5 @@  obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
 
 obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ qcom/
 obj-$(CONFIG_SND_USB_LINE6)	+= line6/
+
+obj-$(CONFIG_SND_USB_OFFLOAD_MIXER) += mixer_usb_offload.o
diff --git a/sound/usb/mixer_usb_offload.c b/sound/usb/mixer_usb_offload.c
new file mode 100644
index 000000000000..e0689a3b9b86
--- /dev/null
+++ b/sound/usb/mixer_usb_offload.c
@@ -0,0 +1,102 @@ 
+// 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 "usbaudio.h"
+#include "card.h"
+#include "helper.h"
+#include "mixer.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 ret;
+
+	ret = snd_soc_usb_update_offload_route(sysdev,
+					       CARD_IDX(kcontrol->private_value),
+					       PCM_IDX(kcontrol->private_value),
+					       SNDRV_PCM_STREAM_PLAYBACK,
+					       ucontrol->value.integer.value);
+	if (ret < 0) {
+		ucontrol->value.integer.value[0] = -1;
+		ucontrol->value.integer.value[1] = -1;
+	}
+
+	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_substream *subs;
+	struct snd_usb_stream *as;
+	char ctl_name[34];
+	int ret;
+
+	list_for_each_entry(as, &chip->pcm_list, list) {
+		subs = &as->substream[SNDRV_PCM_STREAM_PLAYBACK];
+		if (!subs->ep_num || as->pcm_index > 0xff)
+			continue;
+
+		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;
+}
+EXPORT_SYMBOL_GPL(snd_usb_offload_create_ctl);
diff --git a/sound/usb/mixer_usb_offload.h b/sound/usb/mixer_usb_offload.h
new file mode 100644
index 000000000000..3f6e2a8b19c8
--- /dev/null
+++ b/sound/usb/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_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 0;
+}
+#endif
+#endif /* __USB_OFFLOAD_MIXER_H */
diff --git a/sound/usb/qcom/Makefile b/sound/usb/qcom/Makefile
index a81c9b28d484..4005e8391ab9 100644
--- a/sound/usb/qcom/Makefile
+++ b/sound/usb/qcom/Makefile
@@ -1,2 +1,2 @@ 
 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
diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index 9e1c94d28461..f78cd8c504b9 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -36,6 +36,7 @@ 
 #include "../helper.h"
 #include "../pcm.h"
 #include "../power.h"
+#include "../mixer_usb_offload.h"
 
 #include "usb_audio_qmi_v01.h"
 
@@ -1709,6 +1710,7 @@  static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip)
 		sdev->card_idx = chip->card->number;
 		sdev->chip_idx = chip->index;
 
+		snd_usb_offload_create_ctl(chip);
 		snd_soc_usb_connect(usb_get_usb_backend(udev), sdev);
 	}