diff mbox series

g_audio regression on dwc3 udc

Message ID 20180810190545.GA4119@uda0271908 (mailing list archive)
State New, archived
Headers show
Series g_audio regression on dwc3 udc | expand

Commit Message

Bin Liu Aug. 10, 2018, 7:05 p.m. UTC
Hi Felipe,

I noticed a regression on g_audio (uac2) with dwc3 udc on v4.14:

- nosound in playback
- playback can only play once, the following error happens from the 2nd
  playback

  aplay: set_params:1361: Unable to install hw params:

A manual bisect tells the two issues are seperate and the regression
happens in v4.11-rc2 with the following two patches.

40d829fb2ec63 ("usb: dwc3: gadget: Correct ISOC DATA PIDs for short packets")

which cases the first no-sound issue.

ep->mult is already 1 from g_audio, so mult-- leads to incorrect value.
The following change solves it.


With the two changes above, g_audio works fine with v4.14.40, but not with
v4.18-rc7, in which the console is flooded with following message.

u_audio_iso_complete: iso_complete status(-18) 0/256

Regards,
-Bin.

Comments

Thinh Nguyen Aug. 10, 2018, 9:36 p.m. UTC | #1
Hi Bin,

On 8/10/2018 12:05 PM, Bin Liu wrote:
> Hi Felipe,
>
> I noticed a regression on g_audio (uac2) with dwc3 udc on v4.14:
>
> - nosound in playback
> - playback can only play once, the following error happens from the 2nd
>   playback
>
>   aplay: set_params:1361: Unable to install hw params:
>
> A manual bisect tells the two issues are seperate and the regression
> happens in v4.11-rc2 with the following two patches.
>
> 40d829fb2ec63 ("usb: dwc3: gadget: Correct ISOC DATA PIDs for short packets")
>
> which cases the first no-sound issue.
>
> ep->mult is already 1 from g_audio, so mult-- leads to incorrect value.
> The following change solves it.

I believe v4.14.40 is missing this patch regarding that issue:
ec5bb87e4e2a ("usb: dwc3: gadget: Fix PCM1 for ISOC EP with ep->mult
less than 3")

>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f92e2f2c7fbe..758cc258895c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -930,10 +930,10 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
>                                 unsigned int mult = ep->mult - 1;
>                                 unsigned int maxp = usb_endpoint_maxp(ep->desc);
>  
> -                               if (length <= (2 * maxp))
> +                               if (mult && length <= (2 * maxp))
>                                         mult--;
>  
> -                               if (length <= maxp)
> +                               if (mult && length <= maxp)
>                                         mult--;
>  
>                                 trb->size |= DWC3_TRB_SIZE_PCM1(mult);
>
> cf3113d893d4 ("usb: dwc3: gadget: properly increment dequeue pointer on ep_dequeue")
>
> which causes the multiple playback failure. The following hack covers it.
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f92e2f2c7fbe..758cc258895c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1411,7 +1411,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>                 if (r == req) {
>                         /* wait until it is processed */
>                         dwc3_stop_active_transfer(dwc, dep->number, true);
> -
> +#if 0
>                         /*
>                          * If request was already started, this means we had to
>                          * stop the transfer. With that we also need to ignore
> @@ -1473,6 +1473,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>                                         dwc3_ep_inc_deq(dep);
>                                 }
>                         }
> +#endif
>                         goto out1;
>                 }
>                 dev_err(dwc->dev, "request %pK was not queued to %s\n",
>
> With the two changes above, g_audio works fine with v4.14.40, but not with
> v4.18-rc7, in which the console is flooded with following message.
>
> u_audio_iso_complete: iso_complete status(-18) 0/256
>

Pierre Le Magourou attempted to fix the second issue with this patch
(though the patch may need rework):
https://patchwork.kernel.org/patch/10509639/

You can try it out. It works fine when I tested it on HAPS platform
(using Felipe's next branch).

BR,
Thinh
Bin Liu Aug. 13, 2018, 3:21 p.m. UTC | #2
Hi Thinh,

On Fri, Aug 10, 2018 at 09:36:30PM +0000, Thinh Nguyen wrote:
> Hi Bin,
> 
> On 8/10/2018 12:05 PM, Bin Liu wrote:
> > Hi Felipe,
> >
> > I noticed a regression on g_audio (uac2) with dwc3 udc on v4.14:
> >
> > - nosound in playback
> > - playback can only play once, the following error happens from the 2nd
> >   playback
> >
> >   aplay: set_params:1361: Unable to install hw params:
> >
> > A manual bisect tells the two issues are seperate and the regression
> > happens in v4.11-rc2 with the following two patches.
> >
> > 40d829fb2ec63 ("usb: dwc3: gadget: Correct ISOC DATA PIDs for short packets")
> >
> > which cases the first no-sound issue.
> >
> > ep->mult is already 1 from g_audio, so mult-- leads to incorrect value.
> > The following change solves it.
> 
> I believe v4.14.40 is missing this patch regarding that issue:
> ec5bb87e4e2a ("usb: dwc3: gadget: Fix PCM1 for ISOC EP with ep->mult
> less than 3")
> 
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index f92e2f2c7fbe..758cc258895c 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -930,10 +930,10 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
> >                                 unsigned int mult = ep->mult - 1;
> >                                 unsigned int maxp = usb_endpoint_maxp(ep->desc);
> >  
> > -                               if (length <= (2 * maxp))
> > +                               if (mult && length <= (2 * maxp))
> >                                         mult--;
> >  
> > -                               if (length <= maxp)
> > +                               if (mult && length <= maxp)
> >                                         mult--;
> >  
> >                                 trb->size |= DWC3_TRB_SIZE_PCM1(mult);
> >
> > cf3113d893d4 ("usb: dwc3: gadget: properly increment dequeue pointer on ep_dequeue")
> >
> > which causes the multiple playback failure. The following hack covers it.
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index f92e2f2c7fbe..758cc258895c 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1411,7 +1411,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> >                 if (r == req) {
> >                         /* wait until it is processed */
> >                         dwc3_stop_active_transfer(dwc, dep->number, true);
> > -
> > +#if 0
> >                         /*
> >                          * If request was already started, this means we had to
> >                          * stop the transfer. With that we also need to ignore
> > @@ -1473,6 +1473,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> >                                         dwc3_ep_inc_deq(dep);
> >                                 }
> >                         }
> > +#endif
> >                         goto out1;
> >                 }
> >                 dev_err(dwc->dev, "request %pK was not queued to %s\n",
> >
> > With the two changes above, g_audio works fine with v4.14.40, but not with
> > v4.18-rc7, in which the console is flooded with following message.
> >
> > u_audio_iso_complete: iso_complete status(-18) 0/256
> >
> 
> Pierre Le Magourou attempted to fix the second issue with this patch
> (though the patch may need rework):
> https://patchwork.kernel.org/patch/10509639/
> 
> You can try it out. It works fine when I tested it on HAPS platform
> (using Felipe's next branch).

Thanks for the hints. With the two patches you pointed, g_audio works
fine on v4.14.40.

Regards,
-Bin.
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f92e2f2c7fbe..758cc258895c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -930,10 +930,10 @@  static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
                                unsigned int mult = ep->mult - 1;
                                unsigned int maxp = usb_endpoint_maxp(ep->desc);
 
-                               if (length <= (2 * maxp))
+                               if (mult && length <= (2 * maxp))
                                        mult--;
 
-                               if (length <= maxp)
+                               if (mult && length <= maxp)
                                        mult--;
 
                                trb->size |= DWC3_TRB_SIZE_PCM1(mult);

cf3113d893d4 ("usb: dwc3: gadget: properly increment dequeue pointer on ep_dequeue")

which causes the multiple playback failure. The following hack covers it.

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f92e2f2c7fbe..758cc258895c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1411,7 +1411,7 @@  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
                if (r == req) {
                        /* wait until it is processed */
                        dwc3_stop_active_transfer(dwc, dep->number, true);
-
+#if 0
                        /*
                         * If request was already started, this means we had to
                         * stop the transfer. With that we also need to ignore
@@ -1473,6 +1473,7 @@  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
                                        dwc3_ep_inc_deq(dep);
                                }
                        }
+#endif
                        goto out1;
                }
                dev_err(dwc->dev, "request %pK was not queued to %s\n",