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@xxxxxxxxxxx > >>> Cc: zonque@xxxxxxxxx > >>> Cc: tiwai@xxxxxxx > >>> Cc: <stable@xxxxxxxxxxxxxxx> #v3.18+ > >>> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > >>> Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx> > >>> --- > >>> > >>> 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html