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 Thu 15 Jul 2021 at 14:36, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 15. 07. 21 v 11:39 Jerome Brunet napsal(a):
>> On Tue 13 Jul 2021 at 15:16, Pavel Hofman <pavel.hofman@ivitera.com>
>> wrote:
>> 

>> So 48kHz / 2ch / 16bits. Let's assume USB_SPEED_FULL for example (result
>> is the same for the other speeds).
>> In such condition, the nominal packet size is 192B but to accomodate an
>> extra sample, the maximum should indeed be 196B.
>> 	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
>> 		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>> with fb_max being 5 by default, srate should be 48240 after this.
>> 
>> 	max_size_bw = num_channels(chmask) * ssize *
>> 		DIV_ROUND_UP(srate, factor / (1 << (bInterval - 1)));
>> With USB_SPEED_FULL, bInterval is 1 and factor is 1000 so:
>> => DIV_ROUND_UP(48240, 1000 / 1) should give 49;
>> Then
>> => max_size_bw = 2 * 2 * 49 = 196
>> So the end result should be 196 with current code. I tried on an ARM64
>> platform. Here is what I get:
>> [   26.241946] set_ep_max_packet_size: speed is USB_SPEED_FULL
>> [   26.243208] set_ep_max_packet_size: intermediate Playback srate 48000
>> [   26.249758] set_ep_max_packet_size: max_size_bw 192
>> [   26.254559] set_ep_max_packet_size: speed is USB_SPEED_FULL
>> [   26.260130] set_ep_max_packet_size: intermediate Capture srate 48240
>> [   26.266401] set_ep_max_packet_size: max_size_bw 196
>> [   26.271209] set_ep_max_packet_size: speed is USB_SPEED_HIGH
>> [   26.276873] set_ep_max_packet_size: intermediate Playback srate 48000
>> [   26.283165] set_ep_max_packet_size: max_size_bw 192
>> [   26.288015] set_ep_max_packet_size: speed is USB_SPEED_HIGH
>> [   26.293691] set_ep_max_packet_size: intermediate Capture srate 48240
>> [   26.299965] set_ep_max_packet_size: max_size_bw 196
>> [   26.304753] set_ep_max_packet_size: speed is USB_SPEED_SUPER
>> [   26.310426] set_ep_max_packet_size: intermediate Playback srate 48000
>> [   26.316805] set_ep_max_packet_size: max_size_bw 192
>> [   26.321625] set_ep_max_packet_size: speed is USB_SPEED_SUPER
>> [   26.327309] set_ep_max_packet_size: intermediate Capture srate 48240
>> [   26.333613] set_ep_max_packet_size: max_size_bw 196
>> All seems OK and as expected with what's in mainline ATM.
>> So I'm not quite sure why you would get a different result. It would be
>> great if you could check further.
>> 
>
> The problem is max_size_bw=192 for the Playback (i.e. is_playback =
> true). If only capture direction is activated (p_chmask=0), only EP-OUT 
> with max_size_bw=196 is generated and Win10 enumerates the playback-only
> audio device.

Ok, that was not clear before.

> Once the other direction with max_size_bw=192 is activated 
> (either duplex or capture-only), Win10 refuses to enumerate.

Looking further at the format specification [0] (and crawling the web to
decipher it), it seems that

* For isochronous links: packet size must match the nominal rate.
* For async and adaptative: it must match the nominal rate +/- 1
  packet. That is whether we intend on varying the packet size or not.
  
This has several implication
* In async mode, the device is running of its own clock. It has no
  reason to vary the playback packet size but it should still reserve
  bandwidth for an extra packet to satisfy the spec. This seems to be
  your problem and what Win10 insist on.

  When I tested, I had linux on both sides and apparently it is not too
  picky about that.

* If we apply the spec strictly, like Win10 seems to insist on,
  calculating the maximum packet size based on explicit feedback limits
  is wrong too. Whatever happens, it should be +/- 1 around nominal.

Funny thing, is your change puts a +2 capture compared to nominal but
Win10 is not picky on that ...

I'll send a fix to clean this up. Thanks reporting the problem.

[0]: https://www.usb.org/sites/default/files/frmts10.pdf