Hi,
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> writes:
> On Monday, 5 November 2018 14:05:59 EET Felipe Balbi wrote:
>> Laurent Pinchart writes:
>> > On Saturday, 3 November 2018 23:07:33 EET Terence Neill wrote:
>> >> Hi folks,
>> >>
>> >> Thanks for the replies, I really appreciate them. I've made some
>> >> progress on this. I'm new to USB so apologies if the following text is
>> >> incorrect or is just bizarre!:
>> >>
>> >> 1. The problem with streaming_maxpacket set to 2048 seemed to be caused
>> >> by not enough requests being queued with the dwc3 driver at one time,
>> >> resulting in request starvation at higher bandwidths. Increasing the
>> >> value of the UVC_NUM_REQUESTS macro from 4 to 16 (in
>> >> drivers/usb/gadget/function/uvc.h) seemed to fix this problem.
>> >
>> > This clearly needs to be fixed. I wonder if we should try to compute a
>> > good value at runtime, or go for a large enough fixed value for all cases.
>>
>> just go for something large. Computing in runtime would involving
>> measuring latency in runtime and adapting your queue accordingly. Too
>> much work for very little benefit.
>
> I've received a request some time ago from a UVC gadget user who wanted to
> bump the UVC_NUM_REQUESTS up to store a full video frame. That's likely too
> much, we'll have to decide on a proper middle ground.
At least dwc3 can take as many requests are you can feed and will add
them to the interval as possible. With g-mass-storage, I've been using
255 requests for the over a year now, but I've ran with with 2048
requests at least on a couple occasions to make sure dwc3 would handle
it properly
>> >> From reading some more about USB, it looks like isochronous transfers may
>> >> have the option sending zero length packets whenever there is no data to
>> >> send? Is this true? If so, would this be a possible option to move
>> >> forward with this issue - eliminating the need to handle missed isocs at
>> >> the f_uvc layer?
>> >
>> > We will always have to handle the missed intervals, as something could go
>> > wrong and result in such a situation, and we don't want to freeze the
>> > stream in that case. Transmitting 0-length packets could be an option,
>> > but I wonder how much CPU overhead it would generate (both on the gadget
>> > and host side). Felipe, do you have any experience with this and/or
>> > recommendation ?
>>
>> I'd expect the UDC to handle that in HW. At least dwc3 does, don't know
>> about MUSB and the others.
>
> But dwc3 still returns missed intervals errors in that case, right ? I think
> Terence proposal was to explicitly queue ZLP requests in order to avoid
> missing any interval.
there's no way you can avoid missing intervals. What we need to do,
first and foremost, is fix up ->get_frame() operation so that gadget
can, more reliably, queue requests for future intervals. I've been
thinking about something like this (it won't compile, of course, but it
illustrates the idea):
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2b53194081ba..69b3aa092e60 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -27,7 +27,7 @@
#include "gadget.h"
#include "io.h"
-#define DWC3_ALIGN_FRAME(d) (((d)->frame_number + (d)->interval) \
+#define DWC3_ALIGN_FRAME(d) (((d)->dwc->frame_number + (d)->interval) \
& ~((d)->interval - 1))
/**
@@ -1224,7 +1224,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
cmd = DWC3_DEPCMD_STARTTRANSFER;
if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
- cmd |= DWC3_DEPCMD_PARAM(dep->frame_number);
+ cmd |= DWC3_DEPCMD_PARAM(dep->dwc->frame_number);
} else {
cmd = DWC3_DEPCMD_UPDATETRANSFER |
DWC3_DEPCMD_PARAM(dep->resource_index);
@@ -1248,10 +1248,17 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
{
- u32 reg;
+ u64 diff;
+ u64 now;
+ u32 ret;
- reg = dwc3_readl(dwc->regs, DWC3_DSTS);
- return DWC3_DSTS_SOFFN(reg);
+ now = ktime_get_ns();
+ diff = now - dwc->frame_timestamp;
+
+ ret = dwc->frame_number;
+ ret += DIV_ROUND_UP(diff, 125000);
+
+ return ret;
}
static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
@@ -1263,7 +1270,7 @@ static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
return;
}
- dep->frame_number = DWC3_ALIGN_FRAME(dep);
+ dep->dwc->frame_number = DWC3_ALIGN_FRAME(dep);
__dwc3_gadget_kick_transfer(dep);
}
@@ -1569,8 +1576,14 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
static int dwc3_gadget_get_frame(struct usb_gadget *g)
{
struct dwc3 *dwc = gadget_to_dwc(g);
+ unsigned long flags;
+ int ret;
- return __dwc3_gadget_get_frame(dwc);
+ spin_lock_irqsave(dwc->lock, flags)
+ ret = __dwc3_gadget_get_frame(dwc);
+ spin_unlock_irqrestore(dwc->lock, flags);
+
+ return ret;
}
static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
@@ -2367,7 +2380,8 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
const struct dwc3_event_depevt *event)
{
- dep->frame_number = event->parameters;
+ dep->dwc->frame_number = event->parameters;
+ dep->dwc->frame_timestamp = ktime_get_ns();
}
static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
With that, gadget driver can have a much better representation of the
current frame number and use that information to queue requests to
future intervals.
Now, assuming this information is reliable, gadget driver can tell me
which interval the next request should start. Granted, I can also
calculate it internally in dwc3 and hide this from the gadget driver; it
really is a matter of choice here. Do we think gadget drivers need to
have control over this information or not?
If nobody uses ->get_frame(), we might as well get rid of the API
altogether.
What do you think??
--
balbi
Attachment:
signature.asc
Description: PGP signature
