* [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth @ 2015-07-27 6:51 Peter Chen 2015-07-27 6:51 ` [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2->p_interval Peter Chen 2015-07-27 8:13 ` [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth Daniel Mack 0 siblings, 2 replies; 4+ messages in thread From: Peter Chen @ 2015-07-27 6:51 UTC (permalink / raw) To: balbi Cc: linux-usb, Peter Chen, andrzej.p, Daniel Mack, tiwai, stable, Alan Stern 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: Daniel Mack <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 v3: - Consider 'bInterval' to calculate wMaxPacketSize - playback endpoint is IN ep, and capture endpoint is OUT ep Changes for v2: - Using DIV_ROUND_UP to calculate max packet size drivers/usb/gadget/function/f_uac2.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c index 6d3eb8b..51ca32d 100644 --- a/drivers/usb/gadget/function/f_uac2.c +++ b/drivers/usb/gadget/function/f_uac2.c @@ -987,6 +987,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) struct f_uac2_opts *uac2_opts; struct usb_string *us; int ret; + u16 max_packet_size; + struct usb_endpoint_descriptor *ep_desc; uac2_opts = container_of(fn->fi, struct f_uac2_opts, func_inst); @@ -1070,10 +1072,37 @@ 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 */ + ep_desc = &fs_epin_desc; + max_packet_size = num_channels(uac2_opts->p_chmask) * + uac2_opts->p_ssize * DIV_ROUND_UP(uac2_opts->p_srate, + 1000 / (1 << (ep_desc->bInterval - 1))); + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc->wMaxPacketSize); + + ep_desc = &fs_epout_desc; + max_packet_size = num_channels(uac2_opts->c_chmask) * + uac2_opts->c_ssize * DIV_ROUND_UP(uac2_opts->c_srate, + 1000 / (1 << (ep_desc->bInterval - 1))); + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc->wMaxPacketSize); + + ep_desc = &hs_epin_desc; + max_packet_size = num_channels(uac2_opts->p_chmask) * + uac2_opts->p_ssize * DIV_ROUND_UP(uac2_opts->p_srate, + 8000 / (1 << (ep_desc->bInterval - 1))); + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc->wMaxPacketSize); + + ep_desc = &hs_epout_desc; + max_packet_size = num_channels(uac2_opts->c_chmask) * + uac2_opts->c_ssize * DIV_ROUND_UP(uac2_opts->c_srate, + 8000 / (1 << (ep_desc->bInterval - 1))); + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc->wMaxPacketSize); + hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress; - hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize; hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress; - hs_epin_desc.wMaxPacketSize = fs_epin_desc.wMaxPacketSize; ret = usb_assign_descriptors(fn, fs_audio_desc, hs_audio_desc, NULL); if (ret) -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2->p_interval 2015-07-27 6:51 [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth Peter Chen @ 2015-07-27 6:51 ` Peter Chen 2015-07-27 8:13 ` Daniel Mack 2015-07-27 8:13 ` [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth Daniel Mack 1 sibling, 1 reply; 4+ messages in thread From: Peter Chen @ 2015-07-27 6:51 UTC (permalink / raw) To: balbi; +Cc: linux-usb, Peter Chen, Daniel Mack, stable The p_interval should be less if the 'bInterval' at the descriptor is larger, eg, if 'bInterval' is 5 for HS, the p_interval should be 8000 / 16 = 500. It fixes the patch 9bb87f16 "usb: gadget: f_uac2: send reasonably sized packets" Cc: Daniel Mack <zonque@gmail.com> Cc: <stable@vger.kernel.org> #v3.18+ Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/gadget/function/f_uac2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c index 51ca32d..4bd9a8e 100644 --- a/drivers/usb/gadget/function/f_uac2.c +++ b/drivers/usb/gadget/function/f_uac2.c @@ -1191,14 +1191,14 @@ 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_interval = factor / (1 << (ep_desc->bInterval - 1)); uac2->p_pktsize = min_t(unsigned int, rate / uac2->p_interval, prm->max_psize); -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2->p_interval 2015-07-27 6:51 ` [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2->p_interval Peter Chen @ 2015-07-27 8:13 ` Daniel Mack 0 siblings, 0 replies; 4+ messages in thread From: Daniel Mack @ 2015-07-27 8:13 UTC (permalink / raw) To: Peter Chen, balbi; +Cc: linux-usb, stable On 07/27/2015 08:51 AM, Peter Chen wrote: > The p_interval should be less if the 'bInterval' at the descriptor > is larger, eg, if 'bInterval' is 5 for HS, the p_interval should be > 8000 / 16 = 500. > > It fixes the patch 9bb87f16 "usb: gadget: f_uac2: send reasonably > sized packets" > Acked-by: Daniel Mack <zonque@gmail.com> Thanks for spotting this! Daniel > Cc: Daniel Mack <zonque@gmail.com> > Cc: <stable@vger.kernel.org> #v3.18+ > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/gadget/function/f_uac2.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c > index 51ca32d..4bd9a8e 100644 > --- a/drivers/usb/gadget/function/f_uac2.c > +++ b/drivers/usb/gadget/function/f_uac2.c > @@ -1191,14 +1191,14 @@ 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_interval = factor / (1 << (ep_desc->bInterval - 1)); > uac2->p_pktsize = min_t(unsigned int, rate / uac2->p_interval, > prm->max_psize); > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth 2015-07-27 6:51 [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth Peter Chen 2015-07-27 6:51 ` [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2->p_interval Peter Chen @ 2015-07-27 8:13 ` Daniel Mack 1 sibling, 0 replies; 4+ messages in thread From: Daniel Mack @ 2015-07-27 8:13 UTC (permalink / raw) To: Peter Chen, balbi Cc: linux-usb, andrzej.p, Daniel Mack, tiwai, stable, Alan Stern On 07/27/2015 08:51 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: Daniel Mack <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 v3: > - Consider 'bInterval' to calculate wMaxPacketSize > - playback endpoint is IN ep, and capture endpoint is OUT ep > > Changes for v2: > - Using DIV_ROUND_UP to calculate max packet size > > drivers/usb/gadget/function/f_uac2.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c > index 6d3eb8b..51ca32d 100644 > --- a/drivers/usb/gadget/function/f_uac2.c > +++ b/drivers/usb/gadget/function/f_uac2.c > @@ -987,6 +987,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) > struct f_uac2_opts *uac2_opts; > struct usb_string *us; > int ret; > + u16 max_packet_size; > + struct usb_endpoint_descriptor *ep_desc; > > uac2_opts = container_of(fn->fi, struct f_uac2_opts, func_inst); > > @@ -1070,10 +1072,37 @@ 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 */ > + ep_desc = &fs_epin_desc; > + max_packet_size = num_channels(uac2_opts->p_chmask) * > + uac2_opts->p_ssize * DIV_ROUND_UP(uac2_opts->p_srate, > + 1000 / (1 << (ep_desc->bInterval - 1))); > + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), > + ep_desc->wMaxPacketSize); > + > + ep_desc = &fs_epout_desc; > + max_packet_size = num_channels(uac2_opts->c_chmask) * > + uac2_opts->c_ssize * DIV_ROUND_UP(uac2_opts->c_srate, > + 1000 / (1 << (ep_desc->bInterval - 1))); > + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), > + ep_desc->wMaxPacketSize); > + > + ep_desc = &hs_epin_desc; > + max_packet_size = num_channels(uac2_opts->p_chmask) * > + uac2_opts->p_ssize * DIV_ROUND_UP(uac2_opts->p_srate, > + 8000 / (1 << (ep_desc->bInterval - 1))); > + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), > + ep_desc->wMaxPacketSize); > + > + ep_desc = &hs_epout_desc; > + max_packet_size = num_channels(uac2_opts->c_chmask) * > + uac2_opts->c_ssize * DIV_ROUND_UP(uac2_opts->c_srate, > + 8000 / (1 << (ep_desc->bInterval - 1))); > + ep_desc->wMaxPacketSize = min(cpu_to_le16(max_packet_size), > + ep_desc->wMaxPacketSize); Basically, the same operation is repeated 4 times here. What about factoring the code out to a helper function, so that the call site looks like this: set_ep_max_packet_size(&fs_epin_desc, uac2_opts, 1000); set_ep_max_packet_size(&fs_epout_desc, uac2_opts, 1000); set_ep_max_packet_size(&hs_epin_desc, uac2_opts, 8000); set_ep_max_packet_size(&hs_epout_desc, uac2_opts, 8000); Other than that, the patch now looks good to me! Thanks, Daniel > + > hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress; > - hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize; > hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress; > - hs_epin_desc.wMaxPacketSize = fs_epin_desc.wMaxPacketSize; > > ret = usb_assign_descriptors(fn, fs_audio_desc, hs_audio_desc, NULL); > if (ret) > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-27 8:13 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-27 6:51 [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth Peter Chen 2015-07-27 6:51 ` [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2->p_interval Peter Chen 2015-07-27 8:13 ` Daniel Mack 2015-07-27 8:13 ` [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth Daniel Mack
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).