usb: gadget: f_uac2: fix packet size calculation

Commit Message

John Keeping Jan. 10, 2020, 11:28 a.m. UTC

On Fri, 10 Jan 2020 08:29:06 +0100
Pavel Hofman <[e-mail]> wrote:

> Together with dwc2 maintainer Minas Harutyunyan we have been
> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
> the g_audio in capture (EP OUT, playback from USB host) requests req->
> length larger than maxpacket*mc.
> 
> As a workaround we removed the check in dwc2/gadget.c, however that is
> not a proper solution. Minas with his team decided to add a patch where 
> dwc2 will rejecting this type of wrong requests in dwc2_hsotg_ep_queue()
> function, to not process these request at all. The f_uac2 + g_audio
> gadget should restrain from sending such requests.
> 
> Steps to reproduce:
> 
> * Changing fs_epout_desc.bInterval in f_uac2.c from 4 (1ms) to 1 (125us) 
> - the goal is to maximize available throughput of the audio gadget
> 
> * Loading the g_audio module with c_srate=48000, c_ssize=2, c_chmask=2 - 
> i.e. standard 48kHz/16bit/2ch USB playback -> alsa capture
> 
> This combination produces mps=24 and mc=1 for EP OUT. Yet the audio 
> function driver sometimes queues request with req->length 192.

This sounded familiar to me, and I realised that I've been running with
a patch for this case that I never submitted upstream.

Does the patch below fix the issue you're seeing?

-- >8 --
The packet size for USB audio must always be a multiple of the frame
size, otherwise we are transmitting a partial frame which omits some
channels (and these end up at the wrong offset in the next packet).
Furthermore, it breaks the residue handling such that we end up trying
to send a packet exceeding the maximum packet size for the endpoint.

Signed-off-by: John Keeping <[e-mail]>
---
 drivers/usb/gadget/function/u_audio.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Pavel Hofman Jan. 11, 2020, 9:12 a.m. UTC | #1

Hi,

Dne 10. 01. 20 v 12:28 John Keeping napsal(a):
> On Fri, 10 Jan 2020 08:29:06 +0100
> Pavel Hofman <[e-mail]> wrote:
> 
>> Together with dwc2 maintainer Minas Harutyunyan we have been
>> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
>> the g_audio in capture (EP OUT, playback from USB host) requests req->
>> length larger than maxpacket*mc.
>>
>> As a workaround we removed the check in dwc2/gadget.c, however that is
>> not a proper solution. Minas with his team decided to add a patch where
>> dwc2 will rejecting this type of wrong requests in dwc2_hsotg_ep_queue()
>> function, to not process these request at all. The f_uac2 + g_audio
>> gadget should restrain from sending such requests.
>>
>> Steps to reproduce:
>>
>> * Changing fs_epout_desc.bInterval in f_uac2.c from 4 (1ms) to 1 (125us)
>> - the goal is to maximize available throughput of the audio gadget
>>
>> * Loading the g_audio module with c_srate=48000, c_ssize=2, c_chmask=2 -
>> i.e. standard 48kHz/16bit/2ch USB playback -> alsa capture
>>
>> This combination produces mps=24 and mc=1 for EP OUT. Yet the audio
>> function driver sometimes queues request with req->length 192.
> 
> This sounded familiar to me, and I realised that I've been running with
> a patch for this case that I never submitted upstream.
> 
> Does the patch below fix the issue you're seeing?
> @@ -407,7 +407,7 @@ int u_audio_start_playback(struct g_audio *audio_dev)

Thanks for your hint. Your patch handles u_audio playback which is the 
other direction than I am trying to fix (usb host playback -> u_audio 
capture).

With regards,

Pavel.

Patch

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index cf4f2358889b..6d956f190f5a 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -407,7 +407,7 @@  int u_audio_start_playback(struct g_audio *audio_dev)
 	struct usb_ep *ep;
 	struct uac_rtd_params *prm;
 	struct uac_params *params = &audio_dev->params;
-	unsigned int factor, rate;
+	unsigned int factor;
 	const struct usb_endpoint_descriptor *ep_desc;
 	int req_len, i;
 
@@ -426,13 +426,15 @@  int u_audio_start_playback(struct g_audio *audio_dev)
 	/* pre-compute some values for iso_complete() */
 	uac->p_framesize = params->p_ssize *
 			    num_channels(params->p_chmask);
-	rate = params->p_srate * uac->p_framesize;
 	uac->p_interval = factor / (1 << (ep_desc->bInterval - 1));
-	uac->p_pktsize = min_t(unsigned int, rate / uac->p_interval,
+	uac->p_pktsize = min_t(unsigned int,
+				uac->p_framesize *
+					(params->p_srate / uac->p_interval),
 				prm->max_psize);
 
 	if (uac->p_pktsize < prm->max_psize)
-		uac->p_pktsize_residue = rate % uac->p_interval;
+		uac->p_pktsize_residue = uac->p_framesize *
+			(params->p_srate % uac->p_interval);
 	else
 		uac->p_pktsize_residue = 0;