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

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

[Peter Chen]

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;
 	hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
-- 
1.9.1

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

[Peter Chen]

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.
> 
> 

You mean I need to consider high bandwidth ISO transfer (3 packets per
SoF)? 

-- 

Best Regards,
Peter Chen

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

[Daniel Mack]

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.


Daniel

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

[Peter Chen]

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?

-- 

Best Regards,
Peter Chen

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

[Daniel Mack]

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,
Daniel

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

[Peter Chen]

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

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

[Daniel Mack]

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).

> Two more questions:
> 
> 1. If the wMaxPacketSize is calculated correctly at afunc_bind, could we use it
> directly at afunc_set_alt?

All code that sets up runtime parameters should live in afunc_set_alt(),
while code that cares for preparation of descriptor parameters remains
in afunc_setup(). At least in theory, the driver could support multiple
alternate settings which operate on different parameters.

> 2. If we use DIV_ROUND_UP to calculate packet size, do we still need 
> p_pktsize_residue?

The packets the audio driver sends can only contain full sample frames,
and the residue cares about accumulated left-overs that are smaller than
such frames. Setting p_pktsize with DIV_ROUND_UP() would cause every
packet to be slightly too large in certain setups, which will make the
audio run slightly too fast. So yes, we do need the residue logic in
order to provide exact timing.

Note that this is different from the wMaxPacketSize calculation, which
is a value that's stored in the descriptors, transfered to the host and
cached there, so it and cannot be changed at runtime. Hence, it has to
prepare for the 'worst' case, while the determination of actual packet
sizes at runtime might come up with smaller values than the maximum.


Thanks,
Daniel

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

[Peter Chen]

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

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

[Daniel Mack]

On 07/23/2015 10:35 AM, Peter Chen wrote:
> 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?

Hmm? The numerator in that division ('rate') includes the actual frame size:

  rate = opts->p_srate * uac2->p_framesize;
  uac2->p_pktsize = rate / uac2->p_interval;

Which means for 192KHz, stereo 16bit, rate is 768000. In this case,
bInterval = 4 doesn't work, as the packets would become >512 bytes, so
the bandwidth wouldn't suffice.

> 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?

That's a little confusing on first sight, but actually quite logical:

The ALSA capture stream is the one that allows applications on the
gadget to record audio, which is data that is sent *from* the host
(playback on their side) to the device, using OUT tokens.

The ALSA playback stream is the one that allows applications on the
gadget to playback audio, which is data that is sent *to* the host
(capture on their side) by the device, using IN tokens.


Thanks,
Daniel

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

[Alan Stern]

On Thu, 23 Jul 2015, Peter Chen wrote:

> 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

It's important to keep track of the units.  Intervals are measures of
time, and they use units like seconds or microseconds.  In this case,
one packet gets sent every 2000 us, so the value is 2000 us/packet.

The data rate is 192 KB/s.  Let's agree to use units of microseconds 
and bytes.  So converting from KB to bytes and seconds to microseconds, 
the data rate is:

	(192 KB/s) * (1000 bytes/KB) * (1/1000000 s/us)

	= (0.192 bytes/us).

Therefore to compute the number of bytes per packet (the packet size),
you have to _multiply_ the values -- not _divide_:

	(0.192 bytes/us) * (2000 us/packet) = (384 bytes/packet).

Therefore the packet size should be 384 bytes, not 96.

Alan Stern

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

[Daniel Mack]

On 07/23/2015 03:00 AM, Peter Chen wrote:
> 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:

Ok, sorry. I just read the code again an figured you're right here, this
needs fixing. It doesn't really matter for the currently configured
values of bInterval though, as p_interval will always be 1000 for both
HS and FS.

> @@ -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));

Your version is correct. b_interval needs to get larger when bInterval
decreases of course.

> +               uac2->p_pktsize = min_t(unsigned int, DIV_ROUND_UP(rate,
> +                                       uac2->p_interval),
>                                         prm->max_psize);

This change, however, is not needed. uac2->p_pktsize needs to be rounded
down, so an extra frame can be added when the residue accumulator
overflows. The reason is simply that we can only send packets that
contain full sample frames, so we have to evenly distribute those
left-over samples that accumulate in one go once we have enough to fill
a complete frame.

Could you put the above change in an extra patch, as it's not directly
related to your wMaxPacketSize change?


Thanks,
Daniel

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

[Peter Chen]

 
> 
> > @@ -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));
> 
> Your version is correct. b_interval needs to get larger when bInterval decreases
> of course.
> 
> > +               uac2->p_pktsize = min_t(unsigned int, DIV_ROUND_UP(rate,
> > +                                       uac2->p_interval),
> >                                         prm->max_psize);
> 
> This change, however, is not needed. uac2->p_pktsize needs to be rounded
> down, so an extra frame can be added when the residue accumulator
> overflows. The reason is simply that we can only send packets that contain full
> sample frames, so we have to evenly distribute those left-over samples that
> accumulate in one go once we have enough to fill a complete frame.
> 
> Could you put the above change in an extra patch, as it's not directly related to
> your wMaxPacketSize change?
> 

Ok, I will.

Peter