Hi Kieran,
Thanks for the feedback...
On Fri, 16 Oct 2020 at 09:37, Kieran Bingham
<kieran.bingham+renesas@xxxxxxxxxxxxxxxx> wrote:
>
> >>
> >> On Fri, Aug 21, 2020 at 11:00:38PM +0100, Andrew Murray wrote:
> >>> Many UVC devices report larger values for dwMaxPayloadTransferSize than
> >>> appear to be required. This results in less bandwidth available for
> >>> other devices.
> >>>
> >>> This problem is commonly observed when attempting to stream from multiple
> >>> UVC cameras with the host controller returning -ENOSPC and sometimes a
> >>> warning (XHCI controllers: "Not enough bandwidth for new device state.").
>
> So perhaps is this something we can detect at runtime?
I don't think so. We can't detect that a camera is requesting more
bandwidth that it really
needs (except perhaps for specific cameras that are known to be bad).
When we get this
error message, it may not be any fault of the most recently plugged in
camera - it may be
an earlier plugged in camera that erroneously used up most, but not
all of the bandwidth.
>
> >>> For uncompressed video, the UVC_QUIRK_FIX_BANDWIDTH works around this issue
> >>> by overriding the device provided dwMaxPayloadTransferSize with a
> >>> calculation of the actual bandwidth requirements from the requested frame
> >>> size and rate. However for compressed video formats it's not practical to
> >>> estimate the bandwidth required as the kernel doesn't have enough
> >>> information.
>
> Is it something that can be retried with lower and lower values until it
> works when the issue is detected? Or is that difficult because it will
> require interacting/negotiating with other devices on the same bus?
As the kernel has no way of knowing if a device is requesting more
bandwidth than it needs
- you don't know which device is causing problems. Thus you only know
you have a problem
when you run out of bandwidth, in which case that may be because you
have used too many
devices - or it may be because one or more of them have requested more
bandwidth than
needed. At this point you wouldn't want to reconfigure existing
devices, and reducing the
bandwidth requirement of the new device may be incorrect - given it's
not the device at fault.
> >>> Let's provide a pragmatic solution by allowing the user to impose an upper
> >>> threshold to the amount of bandwidth each UVC device can reserve. If the
> >>> parameter isn't used then no threshold is imposed.
> >>
> >> Hmmmm... This is a bit annoying as it will apply equally to all formats
> >> and all cameras. It may solve a real issue, but it's quite a bit of a
> >> hack.
> >
> > Yes you're right. There is certainly a real issue here though, if you google
> > 'usb web cam no space left on device' or similar, you'll find plenty
> > of people having
> > issues. Many of those which could be resolved with a patch like this.
> > Part of the
> > motivation for sharing this patch was so that those people may come across this
> > patch rather than hack their own or give up - though I'd prefer to
> > make this less of a
> > hack.
> >
> > I could respin this to only apply for UVC_FMT_FLAG_COMPRESSED formats, as
>
> One worry I would have is that if the kernel can't decide an appropriate
> value, can we expect users to ?
>
> (And as soon as we can expect users to, can we emulate that decision
> process in the kernel?)
There isn't a perfect solution here. So it's trial and error (!!) from
the user, or the
status quo - where the user can't use multiple cameras at once.
Besides are there any other use-cases where capping the bandwidth per device is
beneficial?
>
>
> > if there is a problem with compressed video then a better solution is to use the
> > existing UVC_QUIRK_FIX_BANDWIDTH.
> >
> > I didn't add this as a quirk that is only applied to specific
> > idVendor/idProducts, as I
> > felt the list might get large, and in any case my assumption is that
> > most of the people
> > that suffer from this issue will likely have a specific camera setup
> > and a bandwidth cap
> > wouldn't cause any issues - for example if you have 4 cameras on a
> > EHCI (perhaps with
> > one camera with a bandwidth issue) platform - then you could cap all
> > cameras high at
> > 90Mbps - that would resolve the camera with the bandwidth issue but
> > not likely affect the
> > other cameras. (Many cameras that I've played with seem to request 195 Mbps).
> >
> >> I'm also concerned that users will be confused regarding how to
> >> use this parameter properly, as there's no documentation that explains
> >> its usage, and how to pick a proper value. Is there any way we could do
> >> better ?
> >
> > I'm happy to write some, though I couldn't find any (in-tree) for the
> > existing parameters
> > (uvc_no_drop_param, uvc_trace_param, etc) so I wasn't sure the best
> > place for this.
> > Any suggestions?
>
> It should probably go somewhere that ends up in the linux-media kernel
> documentation.
>
> > Just as per the UVC_QUIRK_FIX_BANDWIDTH quirk, this works by adjusting
> > dwMaxPayloadTransferSize, which results in the kernel selecting a different USB
> > alternate configuration from those made available by the device. It selects a
> > configuration that matches or provides more bandwidth than that
> > requested. I'm not
> > sure what happens if you stream at a high resolution but select an
> > alternate configuration
> > that has a (too) low bandwidth, perhaps it depends on the camera. It
> > also requires
> > knowledge of the camera to determine how much bandwidth it genuinely
> > needs. Without
> > such knowledge - the best approach is to come up with a reasonable
> > estimate of bandwidth
> > based on compression codec, framesize, rate, etc, look at the
> > available alternate configs
> > (e.g. from lsusb), and then set a value of bandwidth_cap larger than
> > that required. And then
> > of course test to see if it meets your needs.
>
> This sounds quite complex to be able to use a webcam (or two)...
What is the alternative?
Andrew Murray
>
> Ayeee....
>
> Kieran
>
>
> >
> > Thanks,
> >
> > Andrew Murray
> >
> >>
> >>> Signed-off-by: Andrew Murray <amurray@xxxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/media/usb/uvc/uvc_driver.c | 3 +++
> >>> drivers/media/usb/uvc/uvc_video.c | 8 ++++++++
> >>> drivers/media/usb/uvc/uvcvideo.h | 1 +
> >>> 3 files changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >>> index 431d86e1c94b..d5ecac7fc264 100644
> >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>> @@ -33,6 +33,7 @@ unsigned int uvc_no_drop_param;
> >>> static unsigned int uvc_quirks_param = -1;
> >>> unsigned int uvc_trace_param;
> >>> unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;
> >>> +unsigned int uvc_bandwidth_cap_param;
> >>>
> >>> /* ------------------------------------------------------------------------
> >>> * Video formats
> >>> @@ -2389,6 +2390,8 @@ module_param_named(trace, uvc_trace_param, uint, S_IRUGO|S_IWUSR);
> >>> MODULE_PARM_DESC(trace, "Trace level bitmask");
> >>> module_param_named(timeout, uvc_timeout_param, uint, S_IRUGO|S_IWUSR);
> >>> MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> >>> +module_param_named(bandwidth_cap, uvc_bandwidth_cap_param, uint, S_IRUGO|S_IWUSR);
> >>> +MODULE_PARM_DESC(bandwidth_cap, "Maximum bandwidth per device");
> >>>
> >>> /* ------------------------------------------------------------------------
> >>> * Driver initialization and cleanup
> >>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> >>> index a65d5353a441..74a0dc0614cf 100644
> >>> --- a/drivers/media/usb/uvc/uvc_video.c
> >>> +++ b/drivers/media/usb/uvc/uvc_video.c
> >>> @@ -196,6 +196,14 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
> >>>
> >>> ctrl->dwMaxPayloadTransferSize = bandwidth;
> >>> }
> >>> +
> >>> + if (uvc_bandwidth_cap_param &&
> >>> + ctrl->dwMaxPayloadTransferSize > uvc_bandwidth_cap_param) {
> >>> + uvc_trace(UVC_TRACE_VIDEO,
> >>> + "Bandwidth capped from %u to %u B/frame.\n",
> >>> + ctrl->dwMaxPayloadTransferSize, uvc_bandwidth_cap_param);
> >>> + ctrl->dwMaxPayloadTransferSize = uvc_bandwidth_cap_param;
> >>> + }
> >>> }
> >>>
> >>> static size_t uvc_video_ctrl_size(struct uvc_streaming *stream)
> >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>> index 6ab972c643e3..c7d9220c9a7a 100644
> >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>> @@ -718,6 +718,7 @@ extern unsigned int uvc_no_drop_param;
> >>> extern unsigned int uvc_trace_param;
> >>> extern unsigned int uvc_timeout_param;
> >>> extern unsigned int uvc_hw_timestamps_param;
> >>> +extern unsigned int uvc_bandwidth_cap_param;
> >>>
> >>> #define uvc_trace(flag, msg...) \
> >>> do { \
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
>
