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

[Peter Chen <peter.chen@freescale.com>]

On Thu, Jul 23, 2015 at 08:11:11AM +0200, Daniel Mack wrote:
> 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).
> 

Assume the interval for each packet is 2ms, the rate is 192 kbytes
for both FS & HS:
uac2->p_interval = 2000;
uac2->p_pktsize = 192 kbytes / 2000 = 96 bytes

And the uac2->p_pktsize is real usb packet length, it means hardware
would expect there are 96 bytes per 2ms, but the real frame rate is 192k,
and it needs to 192 * 2 bytes per 2ms in the bus, am I missing
something?

Another think I am not understand is why playback uses IN endpoint?
Don't this playback mean the data is from host, and play at device side?

-- 

Best Regards,
Peter Chen