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