Re: usb:gadget:f_uac2: RFC: allowing multiple altsetttings for channel/samplesize combinations

[Chris Wulff <Chris.Wulff@biamp.com>]

On 4.29.2024 11:02 Pavel Hofman  wrote:
>> Example from that thread with c_alt_name changed to c_name as it lives
>> in an "alt.x" directory and removal of the num_alt_modes in favor of just
>> allowing "mkdir alt.x" to create new alt mode settings:
>>
>> (all existing properties + the following new properties)
>> c_it_name
>> c_it_ch_name
>> c_fu_name
>> c_ot_name
>> p_it_name
>> p_it_ch_name
>> p_fu_name
>> p_ot_name
>>
>> alt.0
>>   c_name
>>   p_name
>> alt.1 (for alt.1, alt.2, etc.)
>>   c_name
>>   p_name
>>   c_ssize
>>   p_ssize
>>   (Additional properties here for other things that are settable for each alt mode,
>>    but the only one I've implemented in my prototype so far is sample size.)
>
>Please let me a question regarding some of the proposed string configs.
>Currently we have one feature unit which implements volume and mute
>feature controls. IIUC more feature units can be added, with specific
>controls set, as specified by the UAC. IIUC the linux USB audio driver
>works with volume+mute specifically and other controls by creating
>corresponding alsa ctls, Windows UAC2 driver works with AGC too
>https://learn.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#class-requests-and-interrupt-data-messages
>. That means there is a potential for adding more feature units to the
>gadget.
>
>Would it make sense to name the p/c_fu_name properties specifically
>p/c_fu_volume_name, to leave name room for additional feature units?

Yeah, I think that makes sense. I will change it to be p/c_fu_volume_name.

>Please what does p/c_it_ch_name stand for?

This is the iChannelNames string from the Input Terminal descriptor
"the name of the first logical channel" from the UAC1 spec.

>>
>>
>> Here is a more detailed breakdown of the (current) proposal:
>>
>> * Allow the user to create "alt.x" directories inside uac1 and uac2
>>   function configfs
>>   * Prior to creation of any of these alt.x subdirectories, the
>>     function behaves the same as it does today. No "alt.x" directories would
>>     exist on creation of the function.
>>   * Creation of "alt.0" contains only "c_name" and "p_name" to set the
>>     USB string name for this alt mode, with the defaults as "Capture Inactive"
>>     and "Playback Inactive"
>>   * Creation of "alt.x" where x is an integer, contains the same name
>>     strings but with defaults of "Capture Active" and "Playback Active" and
>>     additional files for any per-alt-mode configurable settings (such as
>>     c_ssize, p_ssize, etc.) At the time of creation, values for those are copied
>>     from the corresponding settings in the function main configfs directory.
>>   * Creation of "alt.1" in particular changes the meaning of the files
>>     in the main function configfs dir so that they are only _default_ values used
>>     when creating "alt.x" directories and settings from "alt.1" will now be used
>>     for alt mode 1. (Alt mode 1 always exists, even when "alt.1" has not been
>>     created.)
>>
>> * ALSA card interface behavior
>>   * Configuration of the ALSA card interface remains the same. It is configured
>>     when binding the function to match the settings in the main function configfs.
>>     "alt.x" settings have no effect on the created ALSA card setup
>>   * Sample size will be converted between ALSA and USB data by dropping
>>     extra bits or zero padding samples (eg 16->24 will zero pad a byte, 24->16
>>     will drop a byte)
>
>Is not this work for userspace, e.g. for the plug plugin? IMO the
>hw_params should reflect the requested format as is now, doing no
>conversions in the gadget driver. Currently the driver just copies the
>buffer from the packet area to the alsa area which is the correct way, IMO.
>
>It also allows for future addition of FLOAT_LE format which is part of
>UAC specs and which (at least) the windows and linux host drivers
>support (for which I would already have a use case). Actually the linux
>and windows USB audio driver supports the TYPE III digital formats.
>
>IIUC the gadget itself does not (and should not, IMO) care much about
>the actual format (apart of converting the USB format ID to the alsa
>format code for hw_params).
>
>
>>   * Channel count differences will ignore extra incoming channels and zero
>>     outgoing extra channels (eg 8->2 will just copy the first two and ignore
>>     the rest. 2->8 will copy the first two and zero the rest.)
>
>I think it's dtto. Either alsa automatic plug plugin, or detailed with
>the route plugin.
>
>>   * Per-alt-mode configurable settings will have a read-only ALSA control (like
>>     sample rate does currently) that can be used to inform the program attached
>>     to the ALSA card what the current state of those settings are when the USB
>>     host selects an alt mode.
>
>The fact is that samplerate is already reported in a separate ctl. But
>the main motivation for this control was not to report the rate, but to
>give some indication that USB host started streaming/requesting data.
>The rate ctl was actually handy to report both this change and the
>actual rate.
>
>Actually there have been submitted patches (IMO not yet included) which
>report this change using uevents
>https://patchwork.kernel.org/project/linux-usb/list/?series=745790&state=*2A&archive=bothPkQoo0QVJgWMFZtW46HQJf-mRkm_KJCQ$
>. IMO a perfectly valid approach too.
>
>Also the alsa loopback device is very similar from this POV. It reports
>(via ctl notifications) that the other/master side has been opened, and
>it's up to the userspace to read current hw_params to determine what
>format/channels/rate to use to successfully open the device.
>
>Maybe we could leave it to the userspace here too. In fact there are
>already open-source clients which try to handle this master/slave
>principle of the loopback and gadget basically in the same way
>https://github.com/HEnquist/camilladsp/pull/341*issue-2267218348
>https://github.com/HEnquist/camilladsp/issues/342
>

I will take a look at the references you provided. I may have solved this problem
in a different way and maybe there is a better way to handle this.

The primary use case for the USB gadget interface of the products that I am
working on is to interface with UC clients (like Microsoft Teams or Google Meet).

My basic problem is that I'm using alsaloop to connect the UAC gadget to a
second ALSA interface. I ended up making modifications to alsaloop that
keeps it running correctly when the host starts/stops audio on the USB interface.
Without doing that I was having trouble with dropping the start of speaking
or high latency when the USB host decides to start streaming audio because of
the time required to get alsaloop running again after getting notified that a
different alt mode was selected. I do still have the plug plugin in the pipeline
as well, so this does result in a double conversion with how I have the UAC
gadget driver currently doing sample size conversion.

If we allow userspace to handle all the rate/channel conversion (which does
seem like a cleaner approach and is what I initially was trying to do), I think that
would mean advertising multiple bit depths/channel counts in the hw_params.
That part is pretty easy.

Then the userspace program can pick which to use, but what should we do
with the sample data to/from USB if the userspace program picks a different
combination than the alt mode currently selected by the host?

Perhaps just discarding audio when the ALSA and USB formats differ would
be the right thing to do here. I might be able to solve my latency/data chopping
issues instead by reconfiguring the ALSA pipeline in response to the change
of the ssize ALSA control (or uevent). I think a fair bit of my time delay was
re-launching alsaloop. I will experiment a bit with this and see what I can get.

>
>>
>> An simple example of how this could be used to create a second alt mode:
>>
>> mkdir uac1.0
>> echo 24 > uac1.0/p_ssize
>> echo 24 > uac1.0/c_ssize
>> mkdir uac1.0/alt.2
>> echo 16 > uac1.0/alt.2/c_ssize
>
>Currently the p/c_ssize values represent number of bytes (i.e. 1-4). IMO
>it would make sense to keep this meaning here. In the future e.g.
>negative numbers could be used to select some non-integer format types.

My mistake. You are correct and it is in bytes. I was not intending to change
it here. My example should have had 3 and 2 instead of 24 and 16.

>
>
>> NOTE: Alt modes are separately selectable by the host for playback and capture
>> so the host can pick and choose as desired. There is no need to make an alt mode
>> for every combination of playback and capture settings. In this example the host
>> can pick either 24 or 16 bit samples for capture, but is only allowed 24 bit samples
>> for playback as both alt.1 (via uac1.0/p_ssize) and alt.2 (via default copied to
>> uac1.0/alt.2/p_ssize) have been configured as 24-bit.
>
>Are capture and playback alt modes dependent? I thought they were
>separate configurations but I may be wrong.
>
>If they are separate, perhaps p_alt.X and c_alt.X dirs would make sense
>instead, with using non-prefixed properties within them (ssize, ch_mask,
>etc.). I.e. p/c_xxx on the main level (setting the defaults and default
>alt1 for each direction) and non-prefixed properties in the actual
>p/c_alt.X subdirs.

They are indeed separate. I like this idea. I will separate these into c_alt.x and p_alt.x
which can be created separately.

>
>Thanks a lot for your great effort,
>
>Pavel.

And thank you for your feedback. I'm happy to work towards a solution that works
for more than just my particular use case.

  -- Chris Wulff