Re: usb:gadget:u_audio: Regression in [v3,3/3] usb: gadget: u_audio: add real feedback implementation - wMaxPacketSize calculation

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 13 Jul 2021 at 12:22, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Hi,
>
> I am testing the three Ruslan's async feedback patches as modified by
> Jerome and I hit a regression in windows 10 enumeration.
>
> Ruslan posted an already accepted patch
> https://github.com/torvalds/linux/commit/789ea77310f0200c84002884ffd628e2baf3ad8a#diff-876615ece7fb56ce8d45fc70623bef9caa2548e810426f217fb785ffa10b33d5 
> which allowed win10 enumeration.
>
> Ruslan's async feedback patchset kept the change:
> https://patchwork.kernel.org/project/linux-usb/patch/1614603943-11668-5-git-send-email-ruslan.bilovol@gmail.com/
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index 72b42f8..91b22fb 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -506,6 +506,10 @@  static int set_ep_max_packet_size(const struct
> f_uac2_opts *uac2_opts,
>
>  	max_size_bw = num_channels(chmask) * ssize *
>  		((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1);
> +
> +	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
> +		max_size_bw = max_size_bw * FBACK_FREQ_MAX / 100;
> +
>  	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>  						    max_size_ep));
>
>
> Jerome's rebase patch which was accepted recently changed the functionality
> to the original code:
> https://patchwork.kernel.org/project/linux-usb/patch/20210603220104.1216001-4-jbrunet@baylibre.com/
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index 321e6c05ba93..ae29ff2b2b68 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -584,8 +584,11 @@  static int set_ep_max_packet_size(const struct
> f_uac2_opts *uac2_opts,
>  		ssize = uac2_opts->c_ssize;
>  	}
>
> +	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
> +		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
> +
>  	max_size_bw = num_channels(chmask) * ssize *
> -		((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1);
> +		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>  	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>  						    max_size_ep));
>
> With this version my Win10 does not enumerate the USB AUDIO device, even if
> it has only EP-IN capability (i.e. is_playback = true). For my setup the
> EP-IN wMaxPacketSize is 192bytes vs. 196bytes in Ruslan's version, causing
> win10 reporting "The specified range could not be found in the range list."
>

Maybe I am lacking USB expertize, but is there any reason why a 192bytes
maximum packet size should be considered invalid ? Just from your
comment, I can't figure it out.

It would help if you could detail the parameters you started your gadget
with (rate, format, channel number)

IIRC, Ruslan initial patchset reserved a fixed additional bandwidth
(around 20% I think) to deal with the explicit feedback.

The idea is that, 99.9% of the time, all you need is the ability to fit
one more sample in each packet. This is should be what the default
setting provides (up to 192kHz). If it does not do that, I made mistake.

The setting configurable because I was trying to avoid wasting USB
bandwith but still support poor quality platforms where 1 extra sample
is not enough (I think Ruslan mentioned having seen such system)

> A simple change makes Win10 enumerate both playback-only as well as duplex
> playback/capture async device:
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index ae29ff2b2b68..5ba778814796 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -588,7 +588,7 @@ static int set_ep_max_packet_size(const struct
> f_uac2_opts *uac2_opts,
>                 srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>
>         max_size_bw = num_channels(chmask) * ssize *
> -               DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval -
>                 1)));
> +               (DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval -
> 1))) + 1);

I don't really understand why you should add 1 here and how it correlate to
your remark above ?

>         ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>                                                     max_size_ep));
>
>
> I do not know if this is the most correct solution but IMO a similar patch
> should be applied. I can send a proper patch mail if this solution is
> acceptable.
>
> Thanks a lot,
>
> Pavel.