Message-ID: <20221021093956.GA25599@qjv001-XeonWs>
Date:   Fri, 21 Oct 2022 04:39:58 -0500
From:   Jeff Vanhoof <jdv1029@...il.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     Jeffrey Vanhoof <jvanhoof@...orola.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "corbet@....net" <corbet@....net>,
        "dan.scally@...asonboard.com" <dan.scally@...asonboard.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "laurent.pinchart@...asonboard.com" 
        <laurent.pinchart@...asonboard.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "m.grzeschik@...gutronix.de" <m.grzeschik@...gutronix.de>,
        "paul.elder@...asonboard.com" <paul.elder@...asonboard.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Dan Vacura <W36195@...orola.com>
Subject: Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of
 release after missed isoc

Hi Thinh,

On Fri, Oct 21, 2022 at 12:55:51AM +0000, Thinh Nguyen wrote:
> On Thu, Oct 20, 2022, Thinh Nguyen wrote:
> > On Thu, Oct 20, 2022, Jeff Vanhoof wrote:
> > > Hi Thinh,
> > > 
> > > On Wed, Oct 19, 2022 at 11:06:08PM +0000, Thinh Nguyen wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Oct 19, 2022, Jeff Vanhoof wrote:
> > > > > Hi Thinh,
> > > > > On Wed, Oct 19, 2022 at 07:08:27PM +0000, Thinh Nguyen wrote:
> > > > > > On Wed, Oct 19, 2022, Jeff Vanhoof wrote:
> > > > 
> > > > <snip>
> > > > 
> > > > > > > 
> > > > > > > From what I can gather from the log, with the current changes it seems that
> > > > > > > after a missed isoc event few requests are staying longer than expected in the
> > > > > > > started_list (not getting reclaimed) and this is preventing the transmission
> > > > > > > from stopping/starting again, and opening the door for continuous stream of
> > > > > > > missed isoc events that cause what appears to the user as a frozen video.
> > > > > > > 
> > > > > > > So one thought, if IOC bit is not set every frame, but IMI bit is, when a
> > > > > > > missed isoc related interrupt occurs it seems likely that more than one trb
> > > > > > > request will need to be reclaimed, but the current set of changes is not
> > > > > > > handling this.
> > > > > > > 
> > > > > > > In the good transfer case this issue seems to be taken care of since the IOC
> > > > > > > bit is not set every frame and the reclaimation will loop through every item in
> > > > > > > the started_list and only stop if there are no additional trbs or if one has
> > > > > > 
> > > > > > It should stop at the request that associated with the interrupt event,
> > > > > > whether it's because of IMI or IOC.
> > > > > 
> > > > > In this case I was concerned that if multipled queued reqs did not have IOC bit
> > > > > set, but there was a missed isoc on one of the last reqs, whether or not we would
> > > > > reclaim all of the requests up to the missed isoc related req. I'm not sure if
> > > > > my concern is valid or not.
> > > > > 
> > > > 
> > > > There should be no problem. If there's an interrupt event indicating a
> > > > TRB completion, the driver will give back all the requests up to the
> > > > request associated with the interrupt event, and the controller will
> > > > continue processing the remaining TRBs. On the next TRB completion
> > > > event, the driver will again give back all the requests up to the
> > > > request associated with that event.
> > > >
> > > 
> > > I was testing with the following patch you suggested:
> > > 
> > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > > index 61fba2b7389b..8352f4b5dd9f 100644
> > > > --- a/drivers/usb/dwc3/gadget.c
> > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > @@ -3657,6 +3657,10 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> > > >  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
> > > >  		return 1;
> > > >  
> > > > +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> > > > +	    (event->status & DEPEVT_STATUS_MISSED_ISOC) && !chain)
> > > > +		return 1;
> > > > +
> > > >  	if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
> > > >  	    (trb->ctrl & DWC3_TRB_CTRL_LST))
> > > >  		return 1;
> > > >
> > > 
> > > At this time the IMI bit was set for every frame. With these changes it
> > > appeared in case of missed isoc that sometimes not all requests would be
> > > reclaimed (enqueued != dequeued even 100ms after the last interrupt was
> > > handled). If the 1st req in the started_list was fine (IMI set, but not IOC),
> > > and a later req was the one actually missed, because of this status check the
> > > reclaimation could stop early and not clean up to the appropriate req. As
> > 
> > Oops. You're right.
> > 
> > > suggested yesterday, I also tried only setting the IMI bit when no_interrupt is
> > > not set, however I was still seeing the complete freezes. After analyzing this
> > > issue a bit, I have updated the diff to look more like this:
> > > 
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index dfaf9ac24c4f..bb800a81815b 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -1230,8 +1230,9 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
> > >  			trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
> > >  		}
> > >  
> > > -		/* always enable Interrupt on Missed ISOC */
> > > -		trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> > > +		/* enable Interrupt on Missed ISOC */
> > > +		if ((!no_interrupt && !chain) || must_interrupt)
> > > +		    trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> > >  		break;
> > 
> > Either all or none of the TRBs of a request is set with IMI, and not
> > some.
> > 
> > >  
> > >  	case USB_ENDPOINT_XFER_BULK:
> > > @@ -3195,6 +3196,11 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> > >  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
> > >  		return 1;
> > >  
> > > +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> > > +		(event->status & DEPEVT_STATUS_MISSED_ISOC) && !chain
> > > +		&& (trb->ctrl & DWC3_TRB_CTRL_ISP_IMI))
> > > +		return 1;
> > > +
> > >  	if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
> > >  	    (trb->ctrl & DWC3_TRB_CTRL_LST))
> > >  		return 1;
> > > 
> > > Where the trb must have the IMI set before returning early. This seemed to make
> > > the freezes recoverable.
> > 
> > Can you try this revised change:
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 61fba2b7389b..a69d8c28d86b 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -3654,7 +3654,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> >  	if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
> >  		return 1;
> >  
> > -	if (event->status & DEPEVT_STATUS_SHORT && !chain)
> 
> I accidentally deleted a couple of lines here.
> 
> > +	if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
> >  		return 1;
> >  
> >  	if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
> 
> I meant to do this:
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 61fba2b7389b..cb65371572ee 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3657,6 +3657,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>  		return 1;
>  
> +	if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
> +		return 1;
> +
>  	if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>  	    (trb->ctrl & DWC3_TRB_CTRL_LST))
>  		return 1;
> @@ -3673,6 +3676,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  	struct scatterlist *s;
>  	unsigned int num_queued = req->num_queued_sgs;
>  	unsigned int i;
> +	bool missed_isoc = false;
>  	int ret = 0;
>  
>  	for_each_sg(sg, s, num_queued, i) {
> @@ -3681,12 +3685,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  		req->sg = sg_next(s);
>  		req->num_queued_sgs--;
>  
> +		if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
> +			missed_isoc = true;
> +
>  		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>  				trb, event, status, true);
>  		if (ret)
>  			break;
>  	}
>  
> +	if (missed_isoc)
> +		ret = 1;
> +
>  	return ret;
>  }
>  
> 
> BR,
> Thinh

I tried out the following patch diff you provided and I did not see any iommu
related crashes with these changes:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index dfaf9ac24c4f..50287437d6de 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
        if (event->status & DEPEVT_STATUS_SHORT && !chain)
                return 1;
 
+       if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
+               return 1;
+
        if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
            (trb->ctrl & DWC3_TRB_CTRL_LST))
                return 1;
@@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
        struct scatterlist *s;
        unsigned int num_queued = req->num_queued_sgs;
        unsigned int i;
+       bool missed_isoc = false;
        int ret = 0;
 
        for_each_sg(sg, s, num_queued, i) {
@@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
                req->sg = sg_next(s);
                req->num_queued_sgs--;
 
+               if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
+                       missed_isoc = true;
+
                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
                                trb, event, status, true);
                if (ret)
                        break;
        }
 
+       if (missed_isoc)
+               ret = 1;
+
        return ret;
 }
 

As we discussed earlier, when uvc's complete function is called, if an -EXDEV
is returned in the request's status, the uvc driver will begin to cancel its
queue. With the current skip interrupt implementation in the uvc driver, if
this occurs while the uvc driver is pumping the current frame, then there is no
guarentee that the last request(s) will have had 'no_interrupt=0'. If the last
requests passed to dwc3 had 'no_interrupt=1', these requests would eventually
be placed at the end of the started_list. Since the IOC bit will not be set,
and if no missed isoc event occurs on these requests, then the dwc3 driver will
not be interrupted, leaving those remaining requests sitting in the
started_list, and dwc3 will not perform an 'End Transfer' as expected. Once the
uvc driver begins to pump the requests for the next frame, then it most likely
will result in additional missed isoc events, with the result being an extended
video freeze seen by the user.

I hope that other uvc driver maintainers can chime in here to help determine the
correct path forward. With the skip interrupt implementation, the uvc driver should
guarentee that the last request sent to dwc3 has 'no_interrupt=0', otherwise
if a missed isoc error occurs, it becomes very likely that the next immediate set of
frames could be dropped/cancelled because the dwc3 driver could not perform a timely
'End Transfer'.

For testing I implemented the following changes to see what I could do for this
issue. Note that I am on an older implementation and it's missing a lot of the
sg related implementation. The idea here is that if the queue is empty, and that
req_int_count is non-zero then the last request likely had 'no_interrupt=1' set.
And if this is the case then we will want to send some dummy request to dwc3 with
'no_interrupt=0' set to make sure that no requests get stuck in its started_list.

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index a00786bc6e60..cb82af6bf453 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -253,6 +253,70 @@ uvc_video_alloc_requests(struct uvc_video *video)
  * Video streaming
  */
 
+/*
+ * uvcg_video_check_pump_fake - Pumps fake/empty video data into a USB request
+ *
+ * This function fills an available USB requests (listed in req_free) with
+ * fake/empty video data to ensure that the last request had no_interrupt == 0
+ */
+static void uvcg_video_check_pump_fake(struct work_struct *work)
+{
+       struct uvc_video *video = container_of(work, struct uvc_video, pump);
+       struct uvc_video_queue *queue = &video->queue;
+       struct usb_request *req = NULL;
+       unsigned long flags;
+       uint8_t *data = NULL;
+       int ret;
+
+       if (video->ep->enabled && list_empty(&queue->irqqueue) &&
+                       video->req_int_count != 0) {
+               /* Retrieve the first available USB request, protected by the
+                * request lock.
+                */
+               spin_lock_irqsave(&video->req_lock, flags);
+               if (list_empty(&video->req_free)) {
+                       spin_unlock_irqrestore(&video->req_lock, flags);
+                       return;
+               }
+               req = list_first_entry(&video->req_free, struct usb_request,
+                                       list);
+               list_del(&req->list);
+               spin_unlock_irqrestore(&video->req_lock, flags);
+
+               /* Set up the fake/empty uvc request */
+               data = req->buf;
+               data[0] = 2;
+               data[1] = UVC_STREAM_EOH | video->fid | UVC_STREAM_EOF;
+               req->length = 2;
+               req->no_interrupt = 0;
+
+               /* Queue the USB request */
+               spin_lock_irqsave(&queue->irqlock, flags);
+               ret = uvcg_video_ep_queue(video, req);
+               spin_unlock_irqrestore(&queue->irqlock, flags);
+
+               if (ret < 0) {
+                       uvcg_queue_cancel(queue, 0);
+               } else {
+                       /* Endpoint now owns the request */
+                       req = NULL;
+
+                       /* Reset req_int_count to force an interrupt on the next request
+                        * and to avoid going through uvcg_video_check_pump_fake again.
+                        */
+                       video->req_int_count = 0;
+               }
+       }
+
+       if (!req)
+               return;
+
+       spin_lock_irqsave(&video->req_lock, flags);
+       list_add_tail(&req->list, &video->req_free);
+       spin_unlock_irqrestore(&video->req_lock, flags);
+       return;
+}
+
 /*
  * uvcg_video_pump - Pump video data into the USB requests
  *
@@ -268,6 +332,8 @@ static void uvcg_video_pump(struct work_struct *work)
        unsigned long flags;
        int ret;
 
+       uvcg_video_check_pump_fake(work);
+
        while (video->ep->enabled) {
                /* Retrieve the first available USB request, protected by the
                 * request lock.
@@ -318,7 +384,8 @@ static void uvcg_video_pump(struct work_struct *work)
                        /* Endpoint now owns the request */
                        req = NULL;
                }
-               video->req_int_count++;
+               if (buf->state != UVC_BUF_STATE_DONE)
+                       video->req_int_count++;
        }
 
        if (!req)


Alternatively we may just not want to cancel the queue upon receiving -EXDEV
and this could solve the problem too, but I don't think that it's such a great
idea, especially if things start falling behind.

I hope that someone more fluent in this area of code can take a crack at
improving/fixing this issue. 

The changes above do seem to help dwc3 timely end its transfers, but mainly for
cases where some requests are missed but the next immediate ones are not (i'm
talking within a couple of hundred microseconds). Most of the time if missed
isocs occurs for a frame that the remaining reqs in the started_list will
likely also error out and the list will be emptied and dwc3 will still timely
send 'End Transfer'. In reality this is to cover a corner case that can
adversely affect the quality of the video being watched. Just wanted to be
upfront with these details.

Thinh, any pointers on how we should proceed from here? It looks like your
changes are working well.

Thanks,
Jeff

Powered by blists - more mailing lists