Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 13 Nov 2019 08:53:08 +0100
From:   Michael Olbrich <m.olbrich@pengutronix.de>
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc:     "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc
 request is queued too late
Message-ID: <20191113075308.qwwgpg7kpfcyjxau@pengutronix.de>
Mail-Followup-To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>
References: <20191111152645.13792-1-m.olbrich@pengutronix.de>
 <20191111152645.13792-3-m.olbrich@pengutronix.de>
 <45ce4107-7c52-48d2-90d1-dfe835024063@synopsys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <45ce4107-7c52-48d2-90d1-dfe835024063@synopsys.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote:
> Michael Olbrich wrote:
> > Currently, most gadget drivers handle isoc transfers on a best effort
> > bases: If the request queue runs empty, then there will simply be gaps in
> > the isoc data stream.
> >
> > The UVC gadget depends on this behaviour. It simply provides new requests
> > when video frames are available and assumes that they are sent as soon as
> > possible.
> >
> > The dwc3 gadget currently works differently: It assumes that there is a
> > contiguous stream of requests without any gaps. If a request is too late,
> > then it is dropped by the hardware.
> > For the UVC gadget this means that a live stream stops after the first
> > frame because all following requests are late.
> 
> Can you explain little more how UVC gadget fails?
> dwc3 controller expects a steady stream of data otherwise it will result 
> in missed_isoc status, and it should be fine as long as new requests are 
> queued. The controller doesn't just drop the request unless there's some 
> other failure.

UVC (with a live stream) does not fill the complete bandwidth of an
isochronous endpoint. Let's assume for the example that one video frame
fills 3 requests. Because it is a live stream, there will be a gap between
video frames. This is unavoidable, especially for compressed video. So the
UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14
15 and so on.
The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9
10 11 12. So except for the fist few requests, all are late and result in a
missed_isoc. I tried to just ignore the missed_isoc but that did not work
for me. I only received the first frame at the other end.
Maybe I missing something here, i don't have access to the hardware
documentation, so I can only guess from the existing driver.

Regards,
Michael

> > This patch changes the behaviour of the dwc3 gadget driver to match the
> > expectations. If a request arrives too late, then the transfer will be
> > restart to create the needed gap.
> >
> > A request is late if:
> > 1. There are currently no requests queued in the hardware
> > 2. The current frame number provided by DSTS does not match the frame
> >     number returned by the last transfer.
> >
> > If this happens transfers are stopped. The following XferNotReady provides
> > the new correct frame number and restarts the transfer.
> >
> > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> > ---
> >   drivers/usb/dwc3/gadget.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index ac4673d91939..eb7f09929f28 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1473,6 +1473,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> >   				return __dwc3_gadget_start_isoc(dep);
> >   			}
> >   		}
> > +		if ((dep->flags & DWC3_EP_TRANSFER_STARTED) &&
> > +		    list_empty(&dep->started_list) &&
> > +		    ((dep->frame_number & 0x3fff) !=
> > +		    __dwc3_gadget_get_frame(dwc))) {
> > +			dwc3_stop_active_transfer(dep, true, true);
> > +			dep->flags = DWC3_EP_ENABLED;
> > +			return 0;
> > +		}
> >   	}
> >   
> >   	return __dwc3_gadget_kick_transfer(dep, false);
> 
> BR,
> Thinh
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |