[8/8] usb: dwc3: gadget: always enable IOC on bulk/interrupt transfers

Message ID 1393530597-12259-8-git-send-email-balbi@ti.com
State Accepted
Commit f3af36511e60669a2b5644d17378c7ea4e42d8b1
Headers show

Commit Message

Felipe Balbi Feb. 27, 2014, 7:49 p.m.
by setting IOC always, we can recycle TRBs a
lot sooner at the expense of some increased
CPU load.

The extra load seems to be quite minimal on
OMAP5 devices (instead of 1 IRQ for one MSC
transfer, we get
CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS).

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/dwc3/gadget.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Felipe Balbi May 5, 2014, 7:26 p.m. | #1
On Mon, May 05, 2014 at 01:34:10PM +0530, Amit Virdi wrote:
> [Re-sending, as previously it was HTML formatted and rejected by the server]
> 
> Dear Felipe,
> 
> On Fri, Feb 28, 2014 at 1:19 AM, Felipe Balbi <balbi@ti.com> wrote:
> >
> > by setting IOC always, we can recycle TRBs a
> > lot sooner at the expense of some increased
> > CPU load.
> >
> > The extra load seems to be quite minimal on
> > OMAP5 devices (instead of 1 IRQ for one MSC
> > transfer, we get
> > CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS).
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/usb/dwc3/gadget.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 2da0a5a..9e878d9 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -771,9 +771,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> >                         trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS_FIRST;
> >                 else
> >                         trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
> > -
> > -               if (!req->request.no_interrupt && !chain)
> > -                       trb->ctrl |= DWC3_TRB_CTRL_IOC;
> >                 break;
> >
> >         case USB_ENDPOINT_XFER_BULK:
> > @@ -788,6 +785,9 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> >                 BUG();
> >         }
> >
> > +       if (!req->request.no_interrupt && !chain)
> > +               trb->ctrl |= DWC3_TRB_CTRL_IOC;
> > +
> >         if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
> >                 trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> >                 trb->ctrl |= DWC3_TRB_CTRL_CSP;
> 
> As a result of this patch, IOC bit for TRBs corresponding to
> Bulk/Interrupt/Isoc
> transfers is set. However, we are still not enabling XferInProgress event
> for Bulk
> and Interrupt EPs. It means enabling IOC is as good as not enabling it.
> This is
> because as per the DWC3 spec:
> 
> When IOC is set in a TRB, and once the transfer for this buffer
> is completed, the core will issue XferInProgress event with IOC
> bit set in the event's status. This indicates the buffer is
> available for software to reuse or release.
> 
> If we are not enabling the XferInProgress event for interrupt EP but
> setting its
> IOC flag, the overall effect of this is nothing positive. If my
> understanding is
> correct the only intention to enable IOC should be to process the buffer
> asap
> which does not happen until we enable XferInProgress event.
> 
> Here's some testing I did to discover it:
> I queued 8 TRBs for interrupt EP in the request_queue. So, while
> preparing the TRBs, the DWC3 driver sets the ctrl fields as :
>       TRB0 - IOC
>       TRB1 - IOC
>       TRB2 - IOC
>       TRB3 - IOC
>       TRB4 - IOC
>       TRB5 - IOC
>       TRB6 - IOC
>       TRB7 - IOC, LST
> 
> Now, since XferInProgress event is not enabled, so the core issues only one
> XferComplete event after it processes all the eight trbs. Am I missing
> anything here?
> 
> I understand that enabling XferInProgress event for bulk/interrupt
> transfers will completely
> change the design of the dwc3 driver and hence is not the viable solution
> to the issue here.

just send a patch enabling XferInProgress.. I haven't gotten to it yet.
If you beat me to it, more power for you ;-)
Amit Virdi May 6, 2014, 6:22 a.m. | #2
On 5/6/2014 12:56 AM, Felipe Balbi wrote:
>> I understand that enabling XferInProgress event for bulk/interrupt
>> >transfers will completely
>> >change the design of the dwc3 driver and hence is not the viable solution
>> >to the issue here.
> just send a patch enabling XferInProgress.. I haven't gotten to it yet.
> If you beat me to it, more power for you;-)

Enabling XferInProgress event for bulk and interrupt would incur 
significant testing efforts. Till it is done can we revert this patch as 
it isn't correct conceptually?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pratyush Anand May 9, 2014, 5:55 a.m. | #3
Hi Amit,

On Tue, May 06, 2014 at 02:22:12PM +0800, Amit VIRDI wrote:
> On 5/6/2014 12:56 AM, Felipe Balbi wrote:
> >> I understand that enabling XferInProgress event for bulk/interrupt
> >> >transfers will completely
> >> >change the design of the dwc3 driver and hence is not the viable solution
> >> >to the issue here.
> > just send a patch enabling XferInProgress.. I haven't gotten to it yet.
> > If you beat me to it, more power for you;-)
> 
> Enabling XferInProgress event for bulk and interrupt would incur 
> significant testing efforts. Till it is done can we revert this patch as 
> it isn't correct conceptually?

Its not just enabling the xferinprogress event for bulk and interrupt
and things will start working. DWC3 driver has been written in such a
way that it handle all isoc transfer using xferinprogress event and
other transfers using xfercomplete event. So you will have to make
changes at couple of more places to get that working and then a through
stress testing.

@Felip: As per my understanding too, this patch must be reverted.
Enabling IOC for bulk and interrupt without enabling xferinprogress
for them does not make sense.

Regards
Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi May 13, 2014, 3:17 p.m. | #4
Hi,

On Fri, May 09, 2014 at 11:25:46AM +0530, Pratyush Anand wrote:
> On Tue, May 06, 2014 at 02:22:12PM +0800, Amit VIRDI wrote:
> > On 5/6/2014 12:56 AM, Felipe Balbi wrote:
> > >> I understand that enabling XferInProgress event for bulk/interrupt
> > >> >transfers will completely
> > >> >change the design of the dwc3 driver and hence is not the viable solution
> > >> >to the issue here.
> > > just send a patch enabling XferInProgress.. I haven't gotten to it yet.
> > > If you beat me to it, more power for you;-)
> > 
> > Enabling XferInProgress event for bulk and interrupt would incur 
> > significant testing efforts. Till it is done can we revert this patch as 
> > it isn't correct conceptually?
> 
> Its not just enabling the xferinprogress event for bulk and interrupt
> and things will start working. DWC3 driver has been written in such a
> way that it handle all isoc transfer using xferinprogress event and
> other transfers using xfercomplete event. So you will have to make
> changes at couple of more places to get that working and then a through
> stress testing.
> 
> @Felip: As per my understanding too, this patch must be reverted.
> Enabling IOC for bulk and interrupt without enabling xferinprogress
> for them does not make sense.

it also causes no extra overhead because the event won't fire anyway.
Pratyush Anand May 14, 2014, 6:31 a.m. | #5
Hi Balbi,

On Tue, May 13, 2014 at 11:17:02PM +0800, Felipe Balbi wrote:
> Hi,
> 
> On Fri, May 09, 2014 at 11:25:46AM +0530, Pratyush Anand wrote:
> > On Tue, May 06, 2014 at 02:22:12PM +0800, Amit VIRDI wrote:
> > > On 5/6/2014 12:56 AM, Felipe Balbi wrote:
> > > >> I understand that enabling XferInProgress event for bulk/interrupt
> > > >> >transfers will completely
> > > >> >change the design of the dwc3 driver and hence is not the viable solution
> > > >> >to the issue here.
> > > > just send a patch enabling XferInProgress.. I haven't gotten to it yet.
> > > > If you beat me to it, more power for you;-)
> > > 
> > > Enabling XferInProgress event for bulk and interrupt would incur 
> > > significant testing efforts. Till it is done can we revert this patch as 
> > > it isn't correct conceptually?
> > 
> > Its not just enabling the xferinprogress event for bulk and interrupt
> > and things will start working. DWC3 driver has been written in such a
> > way that it handle all isoc transfer using xferinprogress event and
> > other transfers using xfercomplete event. So you will have to make
> > changes at couple of more places to get that working and then a through
> > stress testing.
> > 
> > @Felip: As per my understanding too, this patch must be reverted.
> > Enabling IOC for bulk and interrupt without enabling xferinprogress
> > for them does not make sense.
> 
> it also causes no extra overhead because the event won't fire anyway.

No..Still there could be issue. If I analyze the situation Amit had
described, I see a problem after this patch.

You have 8 Bulk/Intr TRBs. 1-7 with only IOC and 8th with IOC + LST.
So, you will receive interrupt when 8th TRB is completed.

dwc3_endpoint_transfer_complete->
dwc3_cleanup_done_reqs->
__dwc3_cleanup_done_trbs

Now here we expect that __dwc3_cleanup_done_trbs returns 1 only when
all the TRBs are done ie for last TRB.

But following code in __dwc3_cleanup_done_trbs will cause to return 1
even after processing 1st TRB, which is not desired here. In this case
TRB2-8 will not be processed

1915         if ((event->status & DEPEVT_STATUS_IOC) &&
1916                         (trb->ctrl & DWC3_TRB_CTRL_IOC))
1917                 return 1;

Regards
Pratyush

> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2da0a5a..9e878d9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -771,9 +771,6 @@  static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS_FIRST;
 		else
 			trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
-
-		if (!req->request.no_interrupt && !chain)
-			trb->ctrl |= DWC3_TRB_CTRL_IOC;
 		break;
 
 	case USB_ENDPOINT_XFER_BULK:
@@ -788,6 +785,9 @@  static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 		BUG();
 	}
 
+	if (!req->request.no_interrupt && !chain)
+		trb->ctrl |= DWC3_TRB_CTRL_IOC;
+
 	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
 		trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
 		trb->ctrl |= DWC3_TRB_CTRL_CSP;
@@ -1855,9 +1855,6 @@  static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
 		return 1;
 	}
 
-	if ((event->status & DEPEVT_STATUS_IOC) &&
-			(trb->ctrl & DWC3_TRB_CTRL_IOC))
-		return 0;
 	return 1;
 }