[PATCH v2] usb: gadget: f_uac2: fixed EP-IN wMaxPacketSize

[PATCH v2] usb: gadget: f_uac2: fixed EP-IN wMaxPacketSize

[Pavel Hofman]

Async feedback patches broke enumeration on Windows 10 previously fixed
by commit 789ea77310f0 ("usb: gadget: f_uac2: always increase endpoint
max_packet_size by one audio slot").

While the existing calculation for EP OUT capture for async mode yields
size+1 frame due to uac2_opts->fb_max > 0, playback side lost the +1
feature.  Therefore the +1 frame addition must be re-introduced for
playback. Win10 enumerates the device only when both EP IN and EP OUT
max packet sizes are (at least) +1 frame.

Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback
implementation")
Cc: stable@vger.kernel.org
Tested-by: Henrik Enquist <henrik.enquist@gmail.com>
Tested-by: Jack Pham <jackp@codeaurora.org>
---
 drivers/usb/gadget/function/f_uac2.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 17a7ab2c799c..5c7eddf511e5 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -665,11 +665,17 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 		ssize = uac2_opts->c_ssize;
 	}
 
-	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
+	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC)) {
+	  // Win10 requires max packet size + 1 frame
 		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
-
-	max_size_bw = num_channels(chmask) * ssize *
-		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
+		// updated srate is always bigger, therefore DIV_ROUND_UP always yields +1
+		max_size_bw = num_channels(chmask) * ssize *
+			(DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1))));
+	} else {
+		// adding 1 frame provision for Win10
+		max_size_bw = num_channels(chmask) * ssize *
+			(DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1))) + 1);
+	}
 	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
 						    max_size_ep));
 
-- 
2.25.1

Re: [PATCH v2] usb: gadget: f_uac2: fixed EP-IN wMaxPacketSize

[Jerome Brunet]

On Tue 05 Oct 2021 at 10:44, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Async feedback patches broke enumeration on Windows 10 previously fixed
> by commit 789ea77310f0 ("usb: gadget: f_uac2: always increase endpoint
> max_packet_size by one audio slot").
>
> While the existing calculation for EP OUT capture for async mode yields
> size+1 frame due to uac2_opts->fb_max > 0, playback side lost the +1
> feature.  Therefore the +1 frame addition must be re-introduced for
> playback. Win10 enumerates the device only when both EP IN and EP OUT
> max packet sizes are (at least) +1 frame.
>
> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback
> implementation")
> Cc: stable@vger.kernel.org
> Tested-by: Henrik Enquist <henrik.enquist@gmail.com>
> Tested-by: Jack Pham <jackp@codeaurora.org>
> ---
>  drivers/usb/gadget/function/f_uac2.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index 17a7ab2c799c..5c7eddf511e5 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -665,11 +665,17 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>  		ssize = uac2_opts->c_ssize;
>  	}
>  
> -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
> +	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC)) {
> +	  // Win10 requires max packet size + 1 frame

Actually it is not really about windows 10, but more about the USB Audio
Format specification:
see https://www.usb.org/sites/default/files/USB%20Audio%20v3.0_0.zip 
section 2.3.1.2.1

Windows 10 is just picky about what the SIP size should be.

>  		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
> -
> -	max_size_bw = num_channels(chmask) * ssize *
> -		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
> +		// updated srate is always bigger, therefore DIV_ROUND_UP always yields +1
> +		max_size_bw = num_channels(chmask) * ssize *
> +			(DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1))));

Doing DIV_ROUND_UP is actually right for 
if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE))

otherwise, it should be rounded down + 1.
I'll reply with my version of the patch to make things more clear

> +	} else {
> +		// adding 1 frame provision for Win10
> +		max_size_bw = num_channels(chmask) * ssize *
> +			(DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1))) + 1);
> +	}
>  	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>  						    max_size_ep));

[RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Jerome Brunet]

This fixes the wMaxPacketSize of the audio gadget so it is in line with
USB Audio Format specification - section 2.3.1.2.1

Cc: Jack Pham <jackp@codeaurora.org>
Cc: Pavel Hofman <pavel.hofman@ivitera.com>
Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 There was a mistake in my previous mail, rounding depends on the
 synchronisation, not the stream direction.

 drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index ae29ff2b2b68..c152efa30def 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 	struct usb_endpoint_descriptor *ep_desc,
 	enum usb_device_speed speed, bool is_playback)
 {
-	int chmask, srate, ssize;
+	int chmask, srate, ssize, spf;
 	u16 max_size_bw, max_size_ep;
 	unsigned int factor;
 
@@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 		ssize = uac2_opts->c_ssize;
 	}
 
-	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
-		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
+	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
+		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
+	else
+		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
 
-	max_size_bw = num_channels(chmask) * ssize *
-		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
+	max_size_bw = num_channels(chmask) * ssize * spf;
 	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
 						    max_size_ep));
 
-- 
2.33.0

[RFC PATCH 2/2] usb: gadget: u_audio: remove fb_max

[Jerome Brunet]

Having a parameter to customize the extra packet size we would like to
reserve for the audio gadget makes no sense. The maximum size the packet
should have is actually specified by the USB Audio Format Specification.

See https://www.usb.org/sites/default/files/USB%20Audio%20v3.0_0.zip
USB Audio Format section 2.3.1.2.1

Cc: Pavel Hofman <pavel.hofman@ivitera.com>
Cc: Jack Pham <jackp@codeaurora.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 I know you guys have continued the development on the audio gadget recently
 I'm sorry I have not been able to follow it closely.
 It is possible this change require a rebase
 I'm sending it now because I think fb_max is part of the problem around
 packet size

 drivers/usb/gadget/function/f_uac2.c  |  4 ---
 drivers/usb/gadget/function/u_audio.c | 40 ++++++++++-----------------
 drivers/usb/gadget/function/u_audio.h |  9 ------
 drivers/usb/gadget/function/u_uac2.h  |  1 -
 4 files changed, 14 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index c152efa30def..381abf8adb31 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -961,7 +961,6 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 	agdev->params.c_srate = uac2_opts->c_srate;
 	agdev->params.c_ssize = uac2_opts->c_ssize;
 	agdev->params.req_number = uac2_opts->req_number;
-	agdev->params.fb_max = uac2_opts->fb_max;
 	ret = g_audio_setup(agdev, "UAC2 PCM", "UAC2_Gadget");
 	if (ret)
 		goto err_free_descs;
@@ -1334,7 +1333,6 @@ UAC2_ATTRIBUTE(c_srate);
 UAC2_ATTRIBUTE_SYNC(c_sync);
 UAC2_ATTRIBUTE(c_ssize);
 UAC2_ATTRIBUTE(req_number);
-UAC2_ATTRIBUTE(fb_max);
 
 static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_p_chmask,
@@ -1345,7 +1343,6 @@ static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_c_ssize,
 	&f_uac2_opts_attr_c_sync,
 	&f_uac2_opts_attr_req_number,
-	&f_uac2_opts_attr_fb_max,
 	NULL,
 };
 
@@ -1385,7 +1382,6 @@ static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->c_ssize = UAC2_DEF_CSSIZE;
 	opts->c_sync = UAC2_DEF_CSYNC;
 	opts->req_number = UAC2_DEF_REQ_NUM;
-	opts->fb_max = UAC2_DEF_FB_MAX;
 	return &opts->func_inst;
 }
 
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 9e5c950612d0..63def8467e9c 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -607,19 +607,20 @@ EXPORT_SYMBOL_GPL(u_audio_stop_playback);
 static int u_audio_pitch_info(struct snd_kcontrol *kcontrol,
 				   struct snd_ctl_elem_info *uinfo)
 {
-	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
-	struct snd_uac_chip *uac = prm->uac;
-	struct g_audio *audio_dev = uac->audio_dev;
-	struct uac_params *params = &audio_dev->params;
-	unsigned int pitch_min, pitch_max;
-
-	pitch_min = (1000 - FBACK_SLOW_MAX) * 1000;
-	pitch_max = (1000 + params->fb_max) * 1000;
-
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 	uinfo->count = 1;
-	uinfo->value.integer.min = pitch_min;
-	uinfo->value.integer.max = pitch_max;
+
+	/*
+	 * TODO: +/- 25% is rough.
+	 * The host constrained by the small and large SIP size so it
+	 * will likely cape before that
+	 *
+	 * On the start of a capture, we should be able to calculate
+	 * the minimum and maximum pitch based on the small and large
+	 * SIP size (See USB Audio Format 3.0 - section 2.3.1.2.1)
+	 */
+	uinfo->value.integer.min = 750000;
+	uinfo->value.integer.max = 1250000;
 	uinfo->value.integer.step = 1;
 	return 0;
 }
@@ -638,29 +639,16 @@ static int u_audio_pitch_put(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
-	struct snd_uac_chip *uac = prm->uac;
-	struct g_audio *audio_dev = uac->audio_dev;
-	struct uac_params *params = &audio_dev->params;
 	unsigned int val;
-	unsigned int pitch_min, pitch_max;
-	int change = 0;
-
-	pitch_min = (1000 - FBACK_SLOW_MAX) * 1000;
-	pitch_max = (1000 + params->fb_max) * 1000;
 
 	val = ucontrol->value.integer.value[0];
 
-	if (val < pitch_min)
-		val = pitch_min;
-	if (val > pitch_max)
-		val = pitch_max;
-
 	if (prm->pitch != val) {
 		prm->pitch = val;
-		change = 1;
+		return 1;
 	}
 
-	return change;
+	return 0;
 }
 
 static const struct snd_kcontrol_new u_audio_controls[]  = {
diff --git a/drivers/usb/gadget/function/u_audio.h b/drivers/usb/gadget/function/u_audio.h
index a218cdf771fe..53e6baf55cbf 100644
--- a/drivers/usb/gadget/function/u_audio.h
+++ b/drivers/usb/gadget/function/u_audio.h
@@ -11,14 +11,6 @@
 
 #include <linux/usb/composite.h>
 
-/*
- * Same maximum frequency deviation on the slower side as in
- * sound/usb/endpoint.c. Value is expressed in per-mil deviation.
- * The maximum deviation on the faster side will be provided as
- * parameter, as it impacts the endpoint required bandwidth.
- */
-#define FBACK_SLOW_MAX	250
-
 struct uac_params {
 	/* playback */
 	int p_chmask;	/* channel mask */
@@ -31,7 +23,6 @@ struct uac_params {
 	int c_ssize;	/* sample size */
 
 	int req_number; /* number of preallocated requests */
-	int fb_max;	/* upper frequency drift feedback limit per-mil */
 };
 
 struct g_audio {
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index 179d3ef6a195..9e9c3c5bb1c8 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -35,7 +35,6 @@ struct f_uac2_opts {
 	int				c_ssize;
 	int				c_sync;
 	int				req_number;
-	int				fb_max;
 	bool				bound;
 
 	struct mutex			lock;
-- 
2.33.0

Re: [RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Pavel Hofman]

Dne 05. 10. 21 v 11:37 Jerome Brunet napsal(a):
> This fixes the wMaxPacketSize of the audio gadget so it is in line with
> USB Audio Format specification - section 2.3.1.2.1
> 
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   There was a mistake in my previous mail, rounding depends on the
>   synchronisation, not the stream direction.
> 
>   drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index ae29ff2b2b68..c152efa30def 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>   	struct usb_endpoint_descriptor *ep_desc,
>   	enum usb_device_speed speed, bool is_playback)
>   {
> -	int chmask, srate, ssize;
> +	int chmask, srate, ssize, spf;
>   	u16 max_size_bw, max_size_ep;
>   	unsigned int factor;
>   
> @@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>   		ssize = uac2_opts->c_ssize;
>   	}
>   
> -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
> -		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
> +	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
> +		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
> +	else
> +		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;

Please correct me if I am wrong, but does the change mean that 
uac2_opts->c_sync value has also impact on playback (EP-IN) wMaxPacketSize?

Thanks,

Pavel.
>   
> -	max_size_bw = num_channels(chmask) * ssize *
> -		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
> +	max_size_bw = num_channels(chmask) * ssize * spf;
>   	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>   						    max_size_ep));
>   
> 

Re: [RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Jerome Brunet]

On Tue 05 Oct 2021 at 12:00, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 05. 10. 21 v 11:37 Jerome Brunet napsal(a):
>> This fixes the wMaxPacketSize of the audio gadget so it is in line with
>> USB Audio Format specification - section 2.3.1.2.1
>> Cc: Jack Pham <jackp@codeaurora.org>
>> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
>> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>   There was a mistake in my previous mail, rounding depends on the
>>   synchronisation, not the stream direction.
>>   drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>> diff --git a/drivers/usb/gadget/function/f_uac2.c
>> b/drivers/usb/gadget/function/f_uac2.c
>> index ae29ff2b2b68..c152efa30def 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>   	struct usb_endpoint_descriptor *ep_desc,
>>   	enum usb_device_speed speed, bool is_playback)
>>   {
>> -	int chmask, srate, ssize;
>> +	int chmask, srate, ssize, spf;
>>   	u16 max_size_bw, max_size_ep;
>>   	unsigned int factor;
>>   @@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct
>> f_uac2_opts *uac2_opts,
>>   		ssize = uac2_opts->c_ssize;
>>   	}
>>   -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
>> -		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>> +	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
>> +		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>> +	else
>> +		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>
> Please correct me if I am wrong, but does the change mean that
> uac2_opts->c_sync value has also impact on playback (EP-IN)
> wMaxPacketSize?

Duh :( - Thanks for catching this ! we only support async for playback

I guess you get the idea, I meant the rounding depends on the sync mode:
 ADAPTIVE: spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
 ASYNC: spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;

The important thing that you should round down for async (not up, as in
the patch you have sent)

Here is quick example with USB full speed
 - ADAPTIVE:
  * 48kHz: 48 samples/SIP (Service Interval Packet)
  * 44.1kHz: max 45 samples/SIP

 - ASYNC
  * 48kHz: small SIP=47samples - big SIP=49samples
  * 44.1kHz small SIP=44samples - big SIP=45samples

Your initial patch would not be correct for ASYNC@44.1kHz.
I think it would give a maximum size (big SIP) of 46 samples instead of
45. 

>
> Thanks,
>
> Pavel.
>>   -	max_size_bw = num_channels(chmask) * ssize *
>> -		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>> +	max_size_bw = num_channels(chmask) * ssize * spf;
>>   	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>>   						    max_size_ep));
>>   

Re: [RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Pavel Hofman]

Dne 05. 10. 21 v 12:13 Jerome Brunet napsal(a):
> 
> On Tue 05 Oct 2021 at 12:00, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>> Dne 05. 10. 21 v 11:37 Jerome Brunet napsal(a):
>>> This fixes the wMaxPacketSize of the audio gadget so it is in line with
>>> USB Audio Format specification - section 2.3.1.2.1
>>> Cc: Jack Pham <jackp@codeaurora.org>
>>> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
>>> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>    There was a mistake in my previous mail, rounding depends on the
>>>    synchronisation, not the stream direction.
>>>    drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
>>>    1 file changed, 6 insertions(+), 5 deletions(-)
>>> diff --git a/drivers/usb/gadget/function/f_uac2.c
>>> b/drivers/usb/gadget/function/f_uac2.c
>>> index ae29ff2b2b68..c152efa30def 100644
>>> --- a/drivers/usb/gadget/function/f_uac2.c
>>> +++ b/drivers/usb/gadget/function/f_uac2.c
>>> @@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>>    	struct usb_endpoint_descriptor *ep_desc,
>>>    	enum usb_device_speed speed, bool is_playback)
>>>    {
>>> -	int chmask, srate, ssize;
>>> +	int chmask, srate, ssize, spf;
>>>    	u16 max_size_bw, max_size_ep;
>>>    	unsigned int factor;
>>>    @@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct
>>> f_uac2_opts *uac2_opts,
>>>    		ssize = uac2_opts->c_ssize;
>>>    	}
>>>    -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
>>> -		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>>> +	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
>>> +		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>>> +	else
>>> +		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>>
>> Please correct me if I am wrong, but does the change mean that
>> uac2_opts->c_sync value has also impact on playback (EP-IN)
>> wMaxPacketSize?
> 
> Duh :( - Thanks for catching this ! we only support async for playback
> 
> I guess you get the idea, I meant the rounding depends on the sync mode:
>   ADAPTIVE: spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>   ASYNC: spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
> 
> The important thing that you should round down for async (not up, as in
> the patch you have sent)
> 
> Here is quick example with USB full speed
>   - ADAPTIVE:
>    * 48kHz: 48 samples/SIP (Service Interval Packet)
>    * 44.1kHz: max 45 samples/SIP
> 
>   - ASYNC
>    * 48kHz: small SIP=47samples - big SIP=49samples
>    * 44.1kHz small SIP=44samples - big SIP=45samples
> 
> Your initial patch would not be correct for ASYNC@44.1kHz.
> I think it would give a maximum size (big SIP) of 46 samples instead of
> 45.

I am sorry I do not understand. You mention chapter 2.3.1.2.1 (I assume 
it is SERVICE INTERVAL PACKET SIZE CALCULATION in Fmts30-Errata.pdf). 
IIUC that chapter does not deal with async mode because exact samplerate 
values converted to sample numbers are used there. How does your new 
calculation take into account the upper range of the async rate, now 
increased to +25% by your second patch? The max packet size calculation 
is done prior to "tweaking" the rate via async feedback, IMO it should 
logically take into account the maximum possible increase (which the 
previous algorithm did via the fb_max (always > 0) adjustment).

Maybe there is a difference in UAC3 which the Fmts30 specs seem to 
describe, I do not know. I just do not see how the possible increase of 
packet size in async mode fits into this calculation.


Thanks a lot,

Pavel.

Re: [RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Jerome Brunet]

On Tue 05 Oct 2021 at 13:12, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 05. 10. 21 v 12:13 Jerome Brunet napsal(a):
>> On Tue 05 Oct 2021 at 12:00, Pavel Hofman <pavel.hofman@ivitera.com>
>> wrote:
>> 
>>> Dne 05. 10. 21 v 11:37 Jerome Brunet napsal(a):
>>>> This fixes the wMaxPacketSize of the audio gadget so it is in line with
>>>> USB Audio Format specification - section 2.3.1.2.1
>>>> Cc: Jack Pham <jackp@codeaurora.org>
>>>> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
>>>> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>> ---
>>>>    There was a mistake in my previous mail, rounding depends on the
>>>>    synchronisation, not the stream direction.
>>>>    drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
>>>>    1 file changed, 6 insertions(+), 5 deletions(-)
>>>> diff --git a/drivers/usb/gadget/function/f_uac2.c
>>>> b/drivers/usb/gadget/function/f_uac2.c
>>>> index ae29ff2b2b68..c152efa30def 100644
>>>> --- a/drivers/usb/gadget/function/f_uac2.c
>>>> +++ b/drivers/usb/gadget/function/f_uac2.c
>>>> @@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>>>    	struct usb_endpoint_descriptor *ep_desc,
>>>>    	enum usb_device_speed speed, bool is_playback)
>>>>    {
>>>> -	int chmask, srate, ssize;
>>>> +	int chmask, srate, ssize, spf;
>>>>    	u16 max_size_bw, max_size_ep;
>>>>    	unsigned int factor;
>>>>    @@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct
>>>> f_uac2_opts *uac2_opts,
>>>>    		ssize = uac2_opts->c_ssize;
>>>>    	}
>>>>    -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
>>>> -		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>>>> +	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
>>>> +		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>>>> +	else
>>>> +		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>>>
>>> Please correct me if I am wrong, but does the change mean that
>>> uac2_opts->c_sync value has also impact on playback (EP-IN)
>>> wMaxPacketSize?
>> Duh :( - Thanks for catching this ! we only support async for playback
>> I guess you get the idea, I meant the rounding depends on the sync mode:
>>   ADAPTIVE: spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>>   ASYNC: spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>> The important thing that you should round down for async (not up, as in
>> the patch you have sent)
>> Here is quick example with USB full speed
>>   - ADAPTIVE:
>>    * 48kHz: 48 samples/SIP (Service Interval Packet)
>>    * 44.1kHz: max 45 samples/SIP
>>   - ASYNC
>>    * 48kHz: small SIP=47samples - big SIP=49samples
>>    * 44.1kHz small SIP=44samples - big SIP=45samples
>> Your initial patch would not be correct for ASYNC@44.1kHz.
>> I think it would give a maximum size (big SIP) of 46 samples instead of
>> 45.
>
> I am sorry I do not understand. You mention chapter 2.3.1.2.1 (I assume it
> is SERVICE INTERVAL PACKET SIZE CALCULATION in Fmts30-Errata.pdf).

Yes, UNIVERSAL SERIAL BUS DEVICE CLASS DEFINITION FOR AUDIO DATA FORMATS

Wording has changed in v3 but you can check v2 here (around 2.3.1):
https://www.usb.org/sites/default/files/Audio2.0_final.zip

It is the same thing, SIP becomes VFP.

> IIUC
> that chapter does not deal with async mode because exact samplerate values
> converted to sample numbers are used there.

In the section mentionned above, I don't see any reference to the sync mode.

> How does your new calculation
> take into account the upper range of the async rate, now increased to +25%
> by your second patch?

25% is only the upper limit of the *request* that can be made. What the
host can actually do is different, and the bandwidth is different.
There is comment in 2nd patch which is fairly important.

> The max packet size calculation is done prior to
> "tweaking" the rate via async feedback, IMO it should logically take into
> account the maximum possible increase (which the previous algorithm did via
> the fb_max (always > 0) adjustment).

Well ... maybe. Honestly, reading the spec, I am under the impression
that the extra bandwidth allocated is not *customisable*. AFAIU, one
should use the large VFP/SIP size. It this case, the 'fb_max' makes no sense.

... but maybe I mis-understand the spec, it's not easiest document to
read :/ .

>
> Maybe there is a difference in UAC3 which the Fmts30 specs seem to
> describe, I do not know. I just do not see how the possible increase of 
> packet size in async mode fits into this calculation.
>
>
> Thanks a lot,
>
> Pavel.

Re: [RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Pavel Hofman]

Dne 05. 10. 21 v 18:24 Jerome Brunet napsal(a):
> 
> On Tue 05 Oct 2021 at 13:12, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>> Dne 05. 10. 21 v 12:13 Jerome Brunet napsal(a):
>>> On Tue 05 Oct 2021 at 12:00, Pavel Hofman <pavel.hofman@ivitera.com>
>>> wrote:
>>>
>>>> Dne 05. 10. 21 v 11:37 Jerome Brunet napsal(a):
>>>>> This fixes the wMaxPacketSize of the audio gadget so it is in line with
>>>>> USB Audio Format specification - section 2.3.1.2.1
>>>>> Cc: Jack Pham <jackp@codeaurora.org>
>>>>> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
>>>>> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
>>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>>> ---
>>>>>     There was a mistake in my previous mail, rounding depends on the
>>>>>     synchronisation, not the stream direction.
>>>>>     drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
>>>>>     1 file changed, 6 insertions(+), 5 deletions(-)
>>>>> diff --git a/drivers/usb/gadget/function/f_uac2.c
>>>>> b/drivers/usb/gadget/function/f_uac2.c
>>>>> index ae29ff2b2b68..c152efa30def 100644
>>>>> --- a/drivers/usb/gadget/function/f_uac2.c
>>>>> +++ b/drivers/usb/gadget/function/f_uac2.c
>>>>> @@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>>>>     	struct usb_endpoint_descriptor *ep_desc,
>>>>>     	enum usb_device_speed speed, bool is_playback)
>>>>>     {
>>>>> -	int chmask, srate, ssize;
>>>>> +	int chmask, srate, ssize, spf;
>>>>>     	u16 max_size_bw, max_size_ep;
>>>>>     	unsigned int factor;
>>>>>     @@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct
>>>>> f_uac2_opts *uac2_opts,
>>>>>     		ssize = uac2_opts->c_ssize;
>>>>>     	}
>>>>>     -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
>>>>> -		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>>>>> +	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
>>>>> +		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>>>>> +	else
>>>>> +		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>>>>
>>>> Please correct me if I am wrong, but does the change mean that
>>>> uac2_opts->c_sync value has also impact on playback (EP-IN)
>>>> wMaxPacketSize?
>>> Duh :( - Thanks for catching this ! we only support async for playback
>>> I guess you get the idea, I meant the rounding depends on the sync mode:
>>>    ADAPTIVE: spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>>>    ASYNC: spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>>> The important thing that you should round down for async (not up, as in
>>> the patch you have sent)
>>> Here is quick example with USB full speed
>>>    - ADAPTIVE:
>>>     * 48kHz: 48 samples/SIP (Service Interval Packet)
>>>     * 44.1kHz: max 45 samples/SIP
>>>    - ASYNC
>>>     * 48kHz: small SIP=47samples - big SIP=49samples
>>>     * 44.1kHz small SIP=44samples - big SIP=45samples
>>> Your initial patch would not be correct for ASYNC@44.1kHz.
>>> I think it would give a maximum size (big SIP) of 46 samples instead of
>>> 45.
>>
>> I am sorry I do not understand. You mention chapter 2.3.1.2.1 (I assume it
>> is SERVICE INTERVAL PACKET SIZE CALCULATION in Fmts30-Errata.pdf).
> 
> Yes, UNIVERSAL SERIAL BUS DEVICE CLASS DEFINITION FOR AUDIO DATA FORMATS
> 
> Wording has changed in v3 but you can check v2 here (around 2.3.1):
> https://www.usb.org/sites/default/files/Audio2.0_final.zip
> 
> It is the same thing, SIP becomes VFP.

Jerome, thanks for your insight. IMO those sections describe calculation 
of numbers of audio frames in each packet (SIP/VFP) for a particular 
samplerate (be it 48kHz or 48.005kHz), not specifically about max packet 
size calculation for the base samplerate of 48kHz. Adaptive transfer 
which recovers audio clock via PLL requires the audioframes count 
variation as small as possible (therefore only the +/-1 packet allowed 
from the "average"). Please note the next subsection "Pitch Control" 
specifically says:

"Pitch control is restricted to adaptive endpoints only"

The way I (poorly) understand it is this: let's have requested 48kHz, 
i.e. 6 audioframes for every SIP with bInterval=1. The maxPacketSize 
must be AT LEAST 1 sample larger otherwise Win10 refuses enumeration 
(correctly, according to the USB specs) - therefore that + 1 in our 
patch. But the maxPacketSize for async endpoint must be actually larger 
to account for the max possible increase in samplerate. IIUC the host 
has no information on how much the device will actually increase the 
rate when running, but the MINIMUM allowable maxPacketSize is still 7 
audioframes. If the device specifies more (because its upper rate limit 
+ 1 exceeds 7), it's problem of the device that it reserves more 
bandwidth and that user plugged in such a "hungry" device. But the 
criterium is adhered to, at least 7 audioframes, both for async and 
adaptive. You may remember how you wondered why Win10 accepted a larger 
maxPacketSize value than was actually necessary. IMO this fits the picture.

The actual maxPacketSize calculated and reported by the device must take 
into account the possible increase in packet size it will possibly 
request from the host or it will possibly send to the host when the 
samplerate is increased on the device side. Otherwise EP-IN data from 
the device will be lost or not enough data will be sent by the host to 
EP-OUT.

Win10 accepts some range of the feedback message value which affects the 
possible range of the samplerate variation allowed by the device but IMO 
we cannot do much with it. The range is quite large 
https://groups.google.com/g/audio-widget/c/COAfYP2BCzw/m/5f0q2iqeAQAJ 
(see the unwrapped post in the list), IMO enough for any practical 
usage. If the feedback value is outside of this range, Win10 simply 
stops adjusting the rate. Tests of this resulted in the patch 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/usb/+/f5dfd98a80ff8d50cf4ae2820857d7f5a46cbab9 
. Feedback values, originally calculated for SIP 125us, fell below the 
range the Win10 driver expected for bInterval=4 (being actually 8x 
smaller) and Win10 kept sending 48samples every 1ms at 48kHz samplerate, 
no matter what the feedback value was when adjusted by the "Capture 
Pitch 1000000" ctl (still way below the lower allowed limit of the 
feedback value range for 48kHz and 1ms SIP).

Your patch #2 hard-coding the fb_max to +25% corresponds to the USB 
specs, but still IMO fb_max=1.25 must enter the maxPacketSize 
calculation to reserve the bandwidth when gadget side is allowed to 
request such range. It will result in useless over-reservation of the 
isochronous bandwidth as no practical usage will need +25% rate adjust. 
I find the currently chosen fb_max +0.5% OK, enough for any 
crystal-based clock, it can be changed/patched later if needed.

That's just how I view it, I may be completely wrong. Thanks a lot for 
your great help and involvement.

Best regards,

Pavel.