All of lore.kernel.org
 help / color / mirror / Atom feed
* uac2/hid gadget issues on Win10 hosts
@ 2021-08-23 11:45 N. Chen
  2021-08-23 13:17 ` Pavel Hofman
  0 siblings, 1 reply; 5+ messages in thread
From: N. Chen @ 2021-08-23 11:45 UTC (permalink / raw)
  To: linux-usb

I'm attempting to create a composite gadget device that uses uac2 and
hid functions (speakerphone), using the latest kernel branch
(rpi-5.14.y) on a rpi4 device.
Windows10 host does not recognize the device as an audio usb device.
In order to get it recognized, I had to use c_sync in adaptive mode,
as well as make the following adjustments:

diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index ae29ff2b2b68..74a221939ca0 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -177,7 +177,7 @@ static struct uac2_input_terminal_descriptor
io_in_it_desc = {

        .bDescriptorSubtype = UAC_INPUT_TERMINAL,
        /* .bTerminalID = DYNAMIC */
-       .wTerminalType = cpu_to_le16(UAC_INPUT_TERMINAL_UNDEFINED),
+       .wTerminalType = cpu_to_le16(UAC_INPUT_TERMINAL_MICROPHONE),
        .bAssocTerminal = 0,
        /* .bCSourceID = DYNAMIC */
        .iChannelNames = 0,
@@ -205,7 +205,7 @@ static struct uac2_output_terminal_descriptor
io_out_ot_desc = {

        .bDescriptorSubtype = UAC_OUTPUT_TERMINAL,
        /* .bTerminalID = DYNAMIC */
-       .wTerminalType = cpu_to_le16(UAC_OUTPUT_TERMINAL_UNDEFINED),
+       .wTerminalType = cpu_to_le16(UAC_OUTPUT_TERMINAL_SPEAKER),
        .bAssocTerminal = 0,
        /* .bSourceID = DYNAMIC */
        /* .bCSourceID = DYNAMIC */
@@ -216,7 +216,7 @@ static struct uac2_ac_header_descriptor ac_hdr_desc = {
        .bLength = sizeof ac_hdr_desc,
        .bDescriptorType = USB_DT_CS_INTERFACE,

-       .bDescriptorSubtype = UAC_MS_HEADER,
+       .bDescriptorSubtype = UAC_HEADER,
        .bcdADC = cpu_to_le16(0x200),
        .bCategory = UAC2_FUNCTION_IO_BOX,
        /* .wTotalLength = DYNAMIC */
@@ -400,7 +400,7 @@ static struct usb_endpoint_descriptor fs_epin_desc = {
        .bDescriptorType = USB_DT_ENDPOINT,

        .bEndpointAddress = USB_DIR_IN,
-       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
        /* .wMaxPacketSize = DYNAMIC */
        .bInterval = 1,
 };
@@ -409,7 +409,7 @@ static struct usb_endpoint_descriptor hs_epin_desc = {
        .bLength = USB_DT_ENDPOINT_SIZE,
        .bDescriptorType = USB_DT_ENDPOINT,

-       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
        /* .wMaxPacketSize = DYNAMIC */
        .bInterval = 4,
 };
@@ -419,7 +419,7 @@ static struct usb_endpoint_descriptor ss_epin_desc = {
        .bDescriptorType = USB_DT_ENDPOINT,

        .bEndpointAddress = USB_DIR_IN,
-       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
        /* .wMaxPacketSize = DYNAMIC */
        .bInterval = 4,
 };
--

For the hid function report descriptor, I used the telephony led page
with off-hook, mute and ring. I was not able to get any hid packets
when tested using Teams. I made the following change, and it allowed
me to get the ring hid alone, but not the led and the off-hook, so I
guess this hack is incorrect:

diff --git a/drivers/usb/gadget/function/f_hid.c
b/drivers/usb/gadget/function/f_hid.c
index bb476e121eae..10882e18cb88 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -549,7 +549,9 @@ static int hidg_setup(struct usb_function *f,
        case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
                  | HID_REQ_SET_REPORT):
                VDBG(cdev, "set_report | wLength=%d\n", ctrl->wLength);
-               goto stall;
+               req->context  = hidg;
+               req->complete = hidg_set_report_complete;
+               goto respond;
                break;

        case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
--

I'm aware that the aforementioned changes to uac2 function might be
incorrect as well, but it allowed me to get through the first hurdle.
I have no objections to making it more correct.
I especially value an opinion on this matter from someone that has
worked with Windows10 hosts before, as it is a specific issue with
this OS (it works well for Ubuntu hosts, yet to be tested on Macs).

Thanks,
Tak

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: uac2/hid gadget issues on Win10 hosts
  2021-08-23 11:45 uac2/hid gadget issues on Win10 hosts N. Chen
@ 2021-08-23 13:17 ` Pavel Hofman
  2021-09-04  7:40   ` Jack Pham
  0 siblings, 1 reply; 5+ messages in thread
From: Pavel Hofman @ 2021-08-23 13:17 UTC (permalink / raw)
  To: N. Chen, linux-usb; +Cc: Ruslan Bilovol, Jerome Brunet

[-- Attachment #1: Type: text/plain, Size: 3949 bytes --]

Hi Tak,

Dne 23. 08. 21 v 13:45 N. Chen napsal(a):
> I'm attempting to create a composite gadget device that uses uac2 and
> hid functions (speakerphone), using the latest kernel branch
> (rpi-5.14.y) on a rpi4 device.
> Windows10 host does not recognize the device as an audio usb device.
> In order to get it recognized, I had to use c_sync in adaptive mode,
> as well as make the following adjustments:
> 
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index ae29ff2b2b68..74a221939ca0 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -177,7 +177,7 @@ static struct uac2_input_terminal_descriptor
> io_in_it_desc = {
> 
>          .bDescriptorSubtype = UAC_INPUT_TERMINAL,
>          /* .bTerminalID = DYNAMIC */
> -       .wTerminalType = cpu_to_le16(UAC_INPUT_TERMINAL_UNDEFINED),
> +       .wTerminalType = cpu_to_le16(UAC_INPUT_TERMINAL_MICROPHONE),
>          .bAssocTerminal = 0,
>          /* .bCSourceID = DYNAMIC */
>          .iChannelNames = 0,
> @@ -205,7 +205,7 @@ static struct uac2_output_terminal_descriptor
> io_out_ot_desc = {
> 
>          .bDescriptorSubtype = UAC_OUTPUT_TERMINAL,
>          /* .bTerminalID = DYNAMIC */
> -       .wTerminalType = cpu_to_le16(UAC_OUTPUT_TERMINAL_UNDEFINED),
> +       .wTerminalType = cpu_to_le16(UAC_OUTPUT_TERMINAL_SPEAKER),
>          .bAssocTerminal = 0,
>          /* .bSourceID = DYNAMIC */
>          /* .bCSourceID = DYNAMIC */
> @@ -216,7 +216,7 @@ static struct uac2_ac_header_descriptor ac_hdr_desc = {
>          .bLength = sizeof ac_hdr_desc,
>          .bDescriptorType = USB_DT_CS_INTERFACE,
> 
> -       .bDescriptorSubtype = UAC_MS_HEADER,
> +       .bDescriptorSubtype = UAC_HEADER,
>          .bcdADC = cpu_to_le16(0x200),
>          .bCategory = UAC2_FUNCTION_IO_BOX,
>          /* .wTotalLength = DYNAMIC */
> @@ -400,7 +400,7 @@ static struct usb_endpoint_descriptor fs_epin_desc = {
>          .bDescriptorType = USB_DT_ENDPOINT,
> 
>          .bEndpointAddress = USB_DIR_IN,
> -       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
> +       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
>          /* .wMaxPacketSize = DYNAMIC */
>          .bInterval = 1,
>   };
> @@ -409,7 +409,7 @@ static struct usb_endpoint_descriptor hs_epin_desc = {
>          .bLength = USB_DT_ENDPOINT_SIZE,
>          .bDescriptorType = USB_DT_ENDPOINT,
> 
> -       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
> +       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
>          /* .wMaxPacketSize = DYNAMIC */
>          .bInterval = 4,
>   };
> @@ -419,7 +419,7 @@ static struct usb_endpoint_descriptor ss_epin_desc = {
>          .bDescriptorType = USB_DT_ENDPOINT,
> 
>          .bEndpointAddress = USB_DIR_IN,
> -       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
> +       .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
>          /* .wMaxPacketSize = DYNAMIC */
>          .bInterval = 4,
>   };
> --

There is a problem with max packet size calculation for EP-IN. It has 
been discussed here recently
https://www.spinics.net/lists/linux-usb/msg214615.html

The simple change in the post above fixed Win10 enumeration for me and 
another tester.

Also, there is a problem with feedback value calculation which Win10 
ignores and keeps sending the same amount of samples. The fix is to send 
number of samples per the actual packet, not per microframe for USB2. I 
have not posted the attached patch as the whole patchset will most 
likely be reverted for 5.15 
https://www.spinics.net/lists/linux-usb/msg216042.html and I wanted to 
wait till the situation works out to avoid confusion. In the attached 
patch just change the ->c_srate_active to ->c_srate (the patch is on top 
of more changes for switching between multiple samplerates).

Best regards,

Pavel.

[-- Attachment #2: 0009-usb-gadget-u_audio-EP-OUT-bInterval-in-fback-frequen.patch --]
[-- Type: text/x-patch, Size: 2666 bytes --]

From 91dda032dd0958006a4e0a6f1f51526cefd95a1e Mon Sep 17 00:00:00 2001
From: Pavel Hofman <pavel.hofman@ivitera.com>
Date: Thu, 12 Aug 2021 18:03:36 +0200
Subject: [PATCH 9/9] usb: gadget: u_audio: EP-OUT bInterval in fback frequency

Tests have revealed that Win10 and OSX UAC2 drivers require
the feedback frequency to be based on the actual packet
interval instead of on the USB2 microframe. Otherwise they
ignore the feedback value. Linux snd-usb-audio driver
detects the applied bitshift automatically.

The patch increases the bitshift in feedback frequency
calculation with EP-OUT bInterval value.

Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
Tested-by: Henrik Enquist <henrik.enquist@gmail.com>
---
 drivers/usb/gadget/function/u_audio.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index a7896fbbab36..c70bdb565153 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -98,11 +98,13 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
 };
 
 static void u_audio_set_fback_frequency(enum usb_device_speed speed,
+          struct usb_ep *out_ep,
 					unsigned long long freq,
 					unsigned int pitch,
 					void *buf)
 {
 	u32 ff = 0;
+	const struct usb_endpoint_descriptor *ep_desc;
 
 	/*
 	 * Because the pitch base is 1000000, the final divider here
@@ -130,8 +132,13 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed,
 		 * byte fromat (that is Q16.16)
 		 *
 		 * ff = (freq << 16) / 8000
+		 *
+		 * Win10 and OSX UAC2 drivers require number of samples per packet
+		 * in order to honor the feedback value.
+		 * Linux snd-usb-audio detects the used bit shift automatically.
 		 */
-		freq <<= 4;
+		ep_desc = out_ep->desc;
+		freq <<= 4 + (ep_desc->bInterval - 1);
 	}
 
 	ff = DIV_ROUND_CLOSEST_ULL((freq * pitch), 1953125);
@@ -263,7 +270,7 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
 		pr_debug("%s: iso_complete status(%d) %d/%d\n",
 			__func__, status, req->actual, req->length);
 
-	u_audio_set_fback_frequency(audio_dev->gadget->speed,
+	u_audio_set_fback_frequency(audio_dev->gadget->speed, audio_dev->out_ep,
 				    params->c_srate_active, prm->pitch,
 				    req->buf);
 
@@ -578,7 +585,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 	 * be meauserd at start of playback
 	 */
 	prm->pitch = 1000000;
-	u_audio_set_fback_frequency(audio_dev->gadget->speed,
+	u_audio_set_fback_frequency(audio_dev->gadget->speed, ep,
 				    params->c_srate_active, prm->pitch,
 				    req_fback->buf);
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: uac2/hid gadget issues on Win10 hosts
  2021-08-23 13:17 ` Pavel Hofman
@ 2021-09-04  7:40   ` Jack Pham
  2021-09-06 12:43     ` Pavel Hofman
  0 siblings, 1 reply; 5+ messages in thread
From: Jack Pham @ 2021-09-04  7:40 UTC (permalink / raw)
  To: Pavel Hofman; +Cc: N. Chen, linux-usb, Ruslan Bilovol, Jerome Brunet

Hi Pavel,

On Mon, Aug 23, 2021 at 03:17:11PM +0200, Pavel Hofman wrote:
> There is a problem with max packet size calculation for EP-IN. It has been
> discussed here recently
> https://www.spinics.net/lists/linux-usb/msg214615.html
> 
> The simple change in the post above fixed Win10 enumeration for me and
> another tester.

I faced the same error on Win10 and also tried the above patch and it
worked for me as well.  Are you planning to send a formal patch for it?
If so, you can add my

Tested-by: Jack Pham <jackp@codeaurora.org>

> Also, there is a problem with feedback value calculation which Win10 ignores
> and keeps sending the same amount of samples. The fix is to send number of
> samples per the actual packet, not per microframe for USB2. I have not
> posted the attached patch as the whole patchset will most likely be reverted
> for 5.15 https://www.spinics.net/lists/linux-usb/msg216042.html and I wanted
> to wait till the situation works out to avoid confusion. In the attached
> patch just change the ->c_srate_active to ->c_srate (the patch is on top of
> more changes for switching between multiple samplerates).

It doesn't look like any of the feedback EP changes got reverted for
5.14 / 5.15-rc1 so it looks like the dust has settled.  Are you going to
send the below patch formally as well?

Thanks!
Jack

> From 91dda032dd0958006a4e0a6f1f51526cefd95a1e Mon Sep 17 00:00:00 2001
> From: Pavel Hofman <pavel.hofman@ivitera.com>
> Date: Thu, 12 Aug 2021 18:03:36 +0200
> Subject: [PATCH 9/9] usb: gadget: u_audio: EP-OUT bInterval in fback frequency
> 
> Tests have revealed that Win10 and OSX UAC2 drivers require
> the feedback frequency to be based on the actual packet
> interval instead of on the USB2 microframe. Otherwise they
> ignore the feedback value. Linux snd-usb-audio driver
> detects the applied bitshift automatically.
> 
> The patch increases the bitshift in feedback frequency
> calculation with EP-OUT bInterval value.
> 
> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
> Tested-by: Henrik Enquist <henrik.enquist@gmail.com>
> ---
>  drivers/usb/gadget/function/u_audio.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index a7896fbbab36..c70bdb565153 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -98,11 +98,13 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
>  };
>  
>  static void u_audio_set_fback_frequency(enum usb_device_speed speed,
> +          struct usb_ep *out_ep,
>  					unsigned long long freq,
>  					unsigned int pitch,
>  					void *buf)
>  {
>  	u32 ff = 0;
> +	const struct usb_endpoint_descriptor *ep_desc;
>  
>  	/*
>  	 * Because the pitch base is 1000000, the final divider here
> @@ -130,8 +132,13 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed,
>  		 * byte fromat (that is Q16.16)
>  		 *
>  		 * ff = (freq << 16) / 8000
> +		 *
> +		 * Win10 and OSX UAC2 drivers require number of samples per packet
> +		 * in order to honor the feedback value.
> +		 * Linux snd-usb-audio detects the used bit shift automatically.
>  		 */
> -		freq <<= 4;
> +		ep_desc = out_ep->desc;
> +		freq <<= 4 + (ep_desc->bInterval - 1);
>  	}
>  
>  	ff = DIV_ROUND_CLOSEST_ULL((freq * pitch), 1953125);
> @@ -263,7 +270,7 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
>  		pr_debug("%s: iso_complete status(%d) %d/%d\n",
>  			__func__, status, req->actual, req->length);
>  
> -	u_audio_set_fback_frequency(audio_dev->gadget->speed,
> +	u_audio_set_fback_frequency(audio_dev->gadget->speed, audio_dev->out_ep,
>  				    params->c_srate_active, prm->pitch,
>  				    req->buf);
>  
> @@ -578,7 +585,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
>  	 * be meauserd at start of playback
>  	 */
>  	prm->pitch = 1000000;
> -	u_audio_set_fback_frequency(audio_dev->gadget->speed,
> +	u_audio_set_fback_frequency(audio_dev->gadget->speed, ep,
>  				    params->c_srate_active, prm->pitch,
>  				    req_fback->buf);
>  
> -- 
> 2.25.1
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: uac2/hid gadget issues on Win10 hosts
  2021-09-04  7:40   ` Jack Pham
@ 2021-09-06 12:43     ` Pavel Hofman
  2021-09-22 15:52       ` Jack Pham
  0 siblings, 1 reply; 5+ messages in thread
From: Pavel Hofman @ 2021-09-06 12:43 UTC (permalink / raw)
  To: Jack Pham; +Cc: N. Chen, linux-usb, Ruslan Bilovol, Jerome Brunet

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

Hi Jack,

Dne 04. 09. 21 v 9:40 Jack Pham napsal(a):
> Hi Pavel,
> 
> On Mon, Aug 23, 2021 at 03:17:11PM +0200, Pavel Hofman wrote:
>> There is a problem with max packet size calculation for EP-IN. It has been
>> discussed here recently
>> https://www.spinics.net/lists/linux-usb/msg214615.html
>>
>> The simple change in the post above fixed Win10 enumeration for me and
>> another tester.
> 
> I faced the same error on Win10 and also tried the above patch and it
> worked for me as well.  Are you planning to send a formal patch for it?
> If so, you can add my
> 
> Tested-by: Jack Pham <jackp@codeaurora.org>
> 
>> Also, there is a problem with feedback value calculation which Win10 ignores
>> and keeps sending the same amount of samples. The fix is to send number of
>> samples per the actual packet, not per microframe for USB2. I have not
>> posted the attached patch as the whole patchset will most likely be reverted
>> for 5.15 https://www.spinics.net/lists/linux-usb/msg216042.html and I wanted
>> to wait till the situation works out to avoid confusion. In the attached
>> patch just change the ->c_srate_active to ->c_srate (the patch is on top of
>> more changes for switching between multiple samplerates).
> 
> It doesn't look like any of the feedback EP changes got reverted for
> 5.14 / 5.15-rc1 so it looks like the dust has settled.  Are you going to
> send the below patch formally as well?
> 

Thanks for testing the patch. I did not want to intrude into Jerome's 
plan. However, if Jerome is OK with the attached patch, I can submit it 
formally and continue with submitting more patches for Win10 support.

Thanks,

Pavel.

[-- Attachment #2: 0001-usb-gadget-f_uac2-fixed-EP-IN-wMaxPacketSize.patch --]
[-- Type: text/x-patch, Size: 2168 bytes --]

From 26f5a49c2ddac2d5c52c4072bc756e7d15b47bc8 Mon Sep 17 00:00:00 2001
From: Pavel Hofman <pavel.hofman@ivitera.com>
Date: Mon, 6 Sep 2021 14:04:00 +0200
Subject: [PATCH] usb: gadget: f_uac2: fixed EP-IN wMaxPacketSize

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>
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 ae29ff2b2b68..bdc7e6e78455 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -584,11 +584,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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: uac2/hid gadget issues on Win10 hosts
  2021-09-06 12:43     ` Pavel Hofman
@ 2021-09-22 15:52       ` Jack Pham
  0 siblings, 0 replies; 5+ messages in thread
From: Jack Pham @ 2021-09-22 15:52 UTC (permalink / raw)
  To: Pavel Hofman; +Cc: N. Chen, linux-usb, Ruslan Bilovol, Jerome Brunet

Hi Pavel,

On Mon, Sep 06, 2021 at 02:43:25PM +0200, Pavel Hofman wrote:
> Hi Jack,
> 
> Dne 04. 09. 21 v 9:40 Jack Pham napsal(a):
> > Hi Pavel,
> > 
> > On Mon, Aug 23, 2021 at 03:17:11PM +0200, Pavel Hofman wrote:
> > > There is a problem with max packet size calculation for EP-IN. It has been
> > > discussed here recently
> > > https://www.spinics.net/lists/linux-usb/msg214615.html
> > > 
> > > The simple change in the post above fixed Win10 enumeration for me and
> > > another tester.
> > 
> > I faced the same error on Win10 and also tried the above patch and it
> > worked for me as well.  Are you planning to send a formal patch for it?
> > If so, you can add my
> > 
> > Tested-by: Jack Pham <jackp@codeaurora.org>
> > 
> > > Also, there is a problem with feedback value calculation which Win10 ignores
> > > and keeps sending the same amount of samples. The fix is to send number of
> > > samples per the actual packet, not per microframe for USB2. I have not
> > > posted the attached patch as the whole patchset will most likely be reverted
> > > for 5.15 https://www.spinics.net/lists/linux-usb/msg216042.html and I wanted
> > > to wait till the situation works out to avoid confusion. In the attached
> > > patch just change the ->c_srate_active to ->c_srate (the patch is on top of
> > > more changes for switching between multiple samplerates).
> > 
> > It doesn't look like any of the feedback EP changes got reverted for
> > 5.14 / 5.15-rc1 so it looks like the dust has settled.  Are you going to
> > send the below patch formally as well?
> > 
> 
> Thanks for testing the patch. I did not want to intrude into Jerome's plan.
> However, if Jerome is OK with the attached patch, I can submit it formally
> and continue with submitting more patches for Win10 support.
> 
> Thanks,
> 
> Pavel.

> From 26f5a49c2ddac2d5c52c4072bc756e7d15b47bc8 Mon Sep 17 00:00:00 2001
> From: Pavel Hofman <pavel.hofman@ivitera.com>
> Date: Mon, 6 Sep 2021 14:04:00 +0200
> Subject: [PATCH] usb: gadget: f_uac2: fixed EP-IN wMaxPacketSize

Thanks for this patch, it looks good to me.  Can you please send this as
a formal patch in its own mail so Greg & Felipe can properly take it?
Or if you like I can send it (if so I'd need to add my S-o-b as well).

Jack

> 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>
> 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 ae29ff2b2b68..bdc7e6e78455 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -584,11 +584,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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-09-22 15:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23 11:45 uac2/hid gadget issues on Win10 hosts N. Chen
2021-08-23 13:17 ` Pavel Hofman
2021-09-04  7:40   ` Jack Pham
2021-09-06 12:43     ` Pavel Hofman
2021-09-22 15:52       ` Jack Pham

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.