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, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
