| Message ID | 1393530597-12259-8-git-send-email-balbi@ti.com |
|---|---|
| State | Accepted |
| Commit | f3af36511e60669a2b5644d17378c7ea4e42d8b1 |
| Headers | show |
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 ;-)
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
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
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.
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
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; }
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(-)