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

[Takashi Iwai <tiwai@xxxxxxx>]

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@xxxxxxxxxxx>
> >>> 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@xxxxxxxxxxx/
> >>
> >> 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.

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@xxxxxxxxxxxxxxx/
> >>
> >> 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.


thanks,

Takashi