Re: [PATCH v2 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth

[Daniel Mack <daniel@zonque.org>]

On 07/23/2015 03:00 AM, Peter Chen wrote:
>> That detail is  merely about completeness. The code that calculates the
>> > value of wMaxPacketSize should take into account what is configured in
>> > bInterval of the endpoint, so if users change one thing, they don't have
>> > to tweak the other as well.
>> > 
>> > bInterval denotes how many packets an endpoint can serve per second, and
>> > wMaxPacketSize defines how large each packet can be. So in an
>> > application that knows how many bytes/s are to be transferred,
>> > wMaxPacketSize depends on bInterval.
>> > 
>> > On HS endpoints, we have 8 microframes per USB frame, so the divisor is
>> > 8000, not 1000. However, I just figured the descriptors in f_uac2 set
>> > .bInterval to 4, which means a period of 8 (2^(4-1)), and that
>> > compensates the factor again.
>> > 
>> > So, to conclude - your calculation indeed comes up with the correct
>> > value, but it should still take the configured endpoint details into
>> > account so the code makes clear how the numbers are determined.
>> > Something like the following should work:
>> > 
>> >   /* for FS */
>> >   div = 1000 / (1 << (fs_epout_desc->bInterval - 1));
>> > 
>> >   /* for HS */
>> >   div = 8000 / (1 << (hs_epout_desc->bInterval - 1));
>> > 
>> >   c_max_packet_size = uac2_opts->c_chmask * uac2_opts->c_ssize
>> >                       * DIV_ROUND_UP(uac2_opts->c_srate, div);
>> > 
>> > 
>> > Makes sense?
>> > 
> Thanks, it is correct. But looking the code at afunc_set_alt:
> the method of calculating uac2->p_pktsize seems incorrect, it
> may need to change like below:
> 
> @@ -1176,15 +1188,16 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>                         factor = 1000;
>                 } else {
>                         ep_desc = &hs_epin_desc;
> -                       factor = 125;
> +                       factor = 8000;
>                 }
>  
>                 /* pre-compute some values for iso_complete() */
>                 uac2->p_framesize = opts->p_ssize *
>                                     num_channels(opts->p_chmask);
>                 rate = opts->p_srate * uac2->p_framesize;
> -               uac2->p_interval = (1 << (ep_desc->bInterval - 1)) * factor;
> -               uac2->p_pktsize = min_t(unsigned int, rate / uac2->p_interval,
> +               uac2->p_interval =  factor / (1 << (ep_desc->bInterval - 1));
> +               uac2->p_pktsize = min_t(unsigned int, DIV_ROUND_UP(rate,
> +                                       uac2->p_interval),
>                                         prm->max_psize);

Your p_interval calculation is equivalent in both cases:

   FS:  1 * 1000 == 1000 / 1
   HS:  8 *  125 == 8000 / 8

And no, p_pktsize is intentionally set to the minimum packet size that a
packet will usually have. The actual size might be higher due to the
accumulated residue (see below).

> Two more questions:
> 
> 1. If the wMaxPacketSize is calculated correctly at afunc_bind, could we use it
> directly at afunc_set_alt?

All code that sets up runtime parameters should live in afunc_set_alt(),
while code that cares for preparation of descriptor parameters remains
in afunc_setup(). At least in theory, the driver could support multiple
alternate settings which operate on different parameters.

> 2. If we use DIV_ROUND_UP to calculate packet size, do we still need 
> p_pktsize_residue?

The packets the audio driver sends can only contain full sample frames,
and the residue cares about accumulated left-overs that are smaller than
such frames. Setting p_pktsize with DIV_ROUND_UP() would cause every
packet to be slightly too large in certain setups, which will make the
audio run slightly too fast. So yes, we do need the residue logic in
order to provide exact timing.

Note that this is different from the wMaxPacketSize calculation, which
is a value that's stored in the descriptors, transfered to the host and
cached there, so it and cannot be changed at runtime. Hence, it has to
prepare for the 'worst' case, while the determination of actual packet
sizes at runtime might come up with smaller values than the maximum.


Thanks,
Daniel