linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: uvcvideo: Add bandwidth_cap module param
@ 2020-08-21 22:00 Andrew Murray
  2020-08-23 22:33 ` Laurent Pinchart
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Murray @ 2020-08-21 22:00 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media

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.").

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.

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.

Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
---
 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 { \
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] media: uvcvideo: Add bandwidth_cap module param
  2020-08-21 22:00 [PATCH] media: uvcvideo: Add bandwidth_cap module param Andrew Murray
@ 2020-08-23 22:33 ` Laurent Pinchart
  2020-08-24  9:13   ` Andrew Murray
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2020-08-23 22:33 UTC (permalink / raw)
  To: Andrew Murray; +Cc: linux-media

Hi Andrew,

Thank you for the patch.

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.").
> 
> 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.
> 
> 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. 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 ?

> Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
> ---
>  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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] media: uvcvideo: Add bandwidth_cap module param
  2020-08-23 22:33 ` Laurent Pinchart
@ 2020-08-24  9:13   ` Andrew Murray
  2020-09-21  9:36     ` Andrew Murray
  2020-10-16  8:37     ` Kieran Bingham
  0 siblings, 2 replies; 7+ messages in thread
From: Andrew Murray @ 2020-08-24  9:13 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media

Hi Laurent,

On Sun, 23 Aug 2020 at 23:34, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Andrew,
>
> Thank you for the patch.
>
> 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.").
> >
> > 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.
> >
> > 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
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?

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.

Thanks,

Andrew Murray

>
> > Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] media: uvcvideo: Add bandwidth_cap module param
  2020-08-24  9:13   ` Andrew Murray
@ 2020-09-21  9:36     ` Andrew Murray
  2020-11-26  0:07       ` Andrew Murray
  2020-10-16  8:37     ` Kieran Bingham
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Murray @ 2020-09-21  9:36 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media

On Mon, 24 Aug 2020 at 10:13, Andrew Murray
<amurray@thegoodpenguin.co.uk> wrote:
>
> Hi Laurent,
>
> On Sun, 23 Aug 2020 at 23:34, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Andrew,
> >
> > Thank you for the patch.
> >
> > 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.").
> > >
> > > 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.
> > >
> > > 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
> 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?
>
> 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.

Hello,

Is there any feedback on this?

Thanks,

Andrew Murray

>
> Thanks,
>
> Andrew Murray
>
> >
> > > Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
> > > ---
> > >  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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] media: uvcvideo: Add bandwidth_cap module param
  2020-08-24  9:13   ` Andrew Murray
  2020-09-21  9:36     ` Andrew Murray
@ 2020-10-16  8:37     ` Kieran Bingham
  2020-10-16 10:40       ` Andrew Murray
  1 sibling, 1 reply; 7+ messages in thread
From: Kieran Bingham @ 2020-10-16  8:37 UTC (permalink / raw)
  To: Andrew Murray, Laurent Pinchart; +Cc: linux-media

Hi Andrew,

On 24/08/2020 10:13, Andrew Murray wrote:
> Hi Laurent,
> 
> On Sun, 23 Aug 2020 at 23:34, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Andrew,
>>
>> Thank you for the patch.
>>
>> 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?

>>> 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?


>>> 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?)


> 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)...

Ayeee....

Kieran


> 
> Thanks,
> 
> Andrew Murray
> 
>>
>>> Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
>>> ---
>>>  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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] media: uvcvideo: Add bandwidth_cap module param
  2020-10-16  8:37     ` Kieran Bingham
@ 2020-10-16 10:40       ` Andrew Murray
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Murray @ 2020-10-16 10:40 UTC (permalink / raw)
  To: kieran.bingham+renesas; +Cc: Laurent Pinchart, linux-media

Hi Kieran,

Thanks for the feedback...

On Fri, 16 Oct 2020 at 09:37, Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> 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@thegoodpenguin.co.uk>
> >>> ---
> >>>  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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] media: uvcvideo: Add bandwidth_cap module param
  2020-09-21  9:36     ` Andrew Murray
@ 2020-11-26  0:07       ` Andrew Murray
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Murray @ 2020-11-26  0:07 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media

Hi Laurent,

I've not had any further feedback from you on this - should I rebase
it or just drop it?

Thanks,

Andrew Murray

On Mon, 21 Sept 2020 at 10:36, Andrew Murray
<amurray@thegoodpenguin.co.uk> wrote:
>
> On Mon, 24 Aug 2020 at 10:13, Andrew Murray
> <amurray@thegoodpenguin.co.uk> wrote:
> >
> > Hi Laurent,
> >
> > On Sun, 23 Aug 2020 at 23:34, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Andrew,
> > >
> > > Thank you for the patch.
> > >
> > > 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.").
> > > >
> > > > 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.
> > > >
> > > > 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
> > 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?
> >
> > 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.
>
> Hello,
>
> Is there any feedback on this?
>
> Thanks,
>
> Andrew Murray
>
> >
> > Thanks,
> >
> > Andrew Murray
> >
> > >
> > > > Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
> > > > ---
> > > >  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



-- 
Andrew Murray, Director
https://www.thegoodpenguin.co.uk

The Good Penguin Ltd is a company registered in England and Wales with
company number 12374667 and VAT number 341687879. Registered office:
The Good Penguin Ltd, Westcott, Glasllwch Lane, Newport, NP20 3PS.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-26  0:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 22:00 [PATCH] media: uvcvideo: Add bandwidth_cap module param Andrew Murray
2020-08-23 22:33 ` Laurent Pinchart
2020-08-24  9:13   ` Andrew Murray
2020-09-21  9:36     ` Andrew Murray
2020-11-26  0:07       ` Andrew Murray
2020-10-16  8:37     ` Kieran Bingham
2020-10-16 10:40       ` Andrew Murray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).