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

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

On Wed, Jul 22, 2015 at 12:11:55PM +0200, Daniel Mack wrote:
> On 07/22/2015 10:23 AM, Peter Chen wrote:
> > On Wed, Jul 22, 2015 at 10:04:30AM +0200, Daniel Mack wrote:
> >> On 07/22/2015 08:45 AM, Peter Chen wrote:
> >>> According to USB Audio Device 2.0 Spec, Ch4.10.1.1:
> >>> wMaxPacketSize is defined as follows:
> >>> Maximum packet size this endpoint is capable of sending or receiving
> >>> when this configuration is selected.
> >>> This is determined by the audio bandwidth constraints of the endpoint.
> >>>
> >>> In current code, the wMaxPacketSize is defined as the maximum packet size
> >>> for ISO endpoint, and it will let the host reserve much more space than
> >>> it really needs, so that we can't let more endpoints work together at
> >>> one frame.
> >>>
> >>> We find this issue when we try to let 4 f_uac2 gadgets work together [1]
> >>> at FS connection.
> >>>
> >>> [1]http://www.spinics.net/lists/linux-usb/msg123478.html
> >>>
> >>> Cc: andrzej.p@samsung.com
> >>> Cc: zonque@gmail.com
> >>> Cc: tiwai@suse.de
> >>> Cc: <stable@vger.kernel.org> #v3.18+
> >>> Cc: Alan Stern <stern@rowland.harvard.edu>
> >>> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> >>> ---
> >>>
> >>> Changes for v2:
> >>> - Using DIV_ROUND_UP to calculate max packet size
> >>>
> >>>  drivers/usb/gadget/function/f_uac2.c | 14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> >>> index 6d3eb8b..6eaa4c4 100644
> >>> --- a/drivers/usb/gadget/function/f_uac2.c
> >>> +++ b/drivers/usb/gadget/function/f_uac2.c
> >>> @@ -987,6 +987,7 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
> >>>  	struct f_uac2_opts *uac2_opts;
> >>>  	struct usb_string *us;
> >>>  	int ret;
> >>> +	u16 c_max_packet_size, p_max_packet_size;
> >>>  
> >>>  	uac2_opts = container_of(fn->fi, struct f_uac2_opts, func_inst);
> >>>  
> >>> @@ -1070,6 +1071,19 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
> >>>  	uac2->p_prm.uac2 = uac2;
> >>>  	uac2->c_prm.uac2 = uac2;
> >>>  
> >>> +	/* Calculate wMaxPacketSize according to audio bandwidth */
> >>> +	c_max_packet_size = uac2_opts->c_chmask * uac2_opts->c_ssize
> >>> +		* DIV_ROUND_UP(uac2_opts->c_srate, 1000);
> >>> +	p_max_packet_size = uac2_opts->p_chmask * uac2_opts->p_ssize
> >>> +		* DIV_ROUND_UP(uac2_opts->p_srate, 1000);
> >>> +	if ((c_max_packet_size > fs_epout_desc.wMaxPacketSize) ||
> >>> +		(p_max_packet_size > fs_epin_desc.wMaxPacketSize)) {
> >>> +		dev_err(dev, "parameters are incorrect\n");
> >>> +		goto err;
> >>> +	}
> >>> +	fs_epin_desc.wMaxPacketSize = cpu_to_le16(c_max_packet_size);
> >>> +	fs_epout_desc.wMaxPacketSize = cpu_to_le16(p_max_packet_size);
> >>> +
> >>>  	hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
> >>>  	hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
> >>
> >> Your calculation still doesn't take into account the endpoint's
> >> 'bInterval', and for HS, the value is still wrong.
> >>
> > 
> > I still not understand why I need to consider 'bInterval' for packet
> > size, per my understanding, 'bInterval' is the interval time for sending
> > each packet. At current code, it defines wMaxPacketSize as max value
> > (1023/1024) for one packet, it may cause problem for audio driver,
> > so you have the patch (9bb87f168931 usb: gadget: f_uac2: send reasonably
> > sized packets) for reducing packet size according to its 'bInterval', but
> > with my change, the wMaxPacketSize will be smaller than its max value,
> > do we still need to reduce packet size for each transfer?
> 
> 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);

Two more questions:

1. If the wMaxPacketSize is calculated correctly at afunc_bind, could we use it
directly at afunc_set_alt?
2. If we use DIV_ROUND_UP to calculate packet size, do we still need 
p_pktsize_residue?

-- 

Best Regards,
Peter Chen