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