diff mbox series

media: uvcvideo: extend the bandwdith quirk to USB 3.x

Message ID 20240114213518.03e22698@foxbook (mailing list archive)
State New
Delegated to: Laurent Pinchart
Headers
Series media: uvcvideo: extend the bandwdith quirk to USB 3.x |

Commit Message

Michal Pecio Jan. 14, 2024, 8:35 p.m. UTC 
  The bandwidth fixup quirk which is needed to run certain buggy cameras
doesn't know that SuperSpeed exists and has the same 8 service intervals
per millisecond as High Speed; hence its calculations are badly wrong.

Assume that all speeds from HS up use 8 intervals per millisecond.

No further changes are required. Updated code has been confirmed to work
properly with a SuperSpeed camera, as well as some High Speed ones.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/media/usb/uvc/uvc_video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Ricardo Ribalda Jan. 14, 2024, 10:58 p.m. UTC | #1 
Hi Michal

Thanks for your patch.

Out of curiosity, what camera are you using? Could you also share the
patch with the quirk?

Thanks!

On Sun, 14 Jan 2024 at 21:35, Michal Pecio <michal.pecio@gmail.com> wrote:
>
> The bandwidth fixup quirk which is needed to run certain buggy cameras
> doesn't know that SuperSpeed exists and has the same 8 service intervals
> per millisecond as High Speed; hence its calculations are badly wrong.
>
> Assume that all speeds from HS up use 8 intervals per millisecond.
>
> No further changes are required. Updated code has been confirmed to work
> properly with a SuperSpeed camera, as well as some High Speed ones.
>
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 28dde08ec6c5..4b86bef06a52 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -214,13 +214,13 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
>                  * Compute a bandwidth estimation by multiplying the frame
>                  * size by the number of video frames per second, divide the
>                  * result by the number of USB frames (or micro-frames for
> -                * high-speed devices) per second and add the UVC header size
> -                * (assumed to be 12 bytes long).
> +                * high- and super-speed devices) per second and add the UVC
> +                * header size (assumed to be 12 bytes long).
>                  */
>                 bandwidth = frame->wWidth * frame->wHeight / 8 * format->bpp;
>                 bandwidth *= 10000000 / interval + 1;
>                 bandwidth /= 1000;
> -               if (stream->dev->udev->speed == USB_SPEED_HIGH)
> +               if (stream->dev->udev->speed >= USB_SPEED_HIGH)
>                         bandwidth /= 8;
>                 bandwidth += 12;
>
> --
> 2.43.0
>
>
Michal Pecio Jan. 14, 2024, 11:57 p.m. UTC | #2 
Hi,

> Out of curiosity, what camera are you using? Could you also share the
> patch with the quirk?

I have no idea what camera I am using ;)

It's some dodgy no-name thing which doesn't even have "made in China"
written on it and reports IDs belonging to a completely different kind
of camera.

But it sort of works, so what the heck. And I use the BW quirk with it
because it otherwise asks for way too much.

Squatting on others' IDs appears to be a fancy new cost reduction trick
(not the first time I see it happen). I'm not really convinced that it
would be a good idea to push quirks on such IDs upstream, but I figured
the BW calculation fix could be useful.

Regards,
Michal
Michal Pecio Jan. 18, 2024, 11:30 a.m. UTC | #3 
Hi again,

> > Out of curiosity, what camera are you using? Could you also share
> > the patch with the quirk?  
> 
> I have no idea what camera I am using ;)
> 
> It's some dodgy no-name thing which doesn't even have "made in China"
> written on it and reports IDs belonging to a completely different kind
> of camera.

In a plot twist, I discovered that the original camera has already had
this quirk applied 15 years ago. So my ID squatter gets the quirk too.
(I think it's a squatter, the old chip was USB 2.0 and this is USB 3.0).

Not sure why I believed otherwise and used to set it with modprobe.
Maybe I noticed the excessive bandwdith requests made by buggy quirk,
then force-enabled the quirk hoping it will solve them, then fixed the
quirk to actually get it right.


Michal
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 28dde08ec6c5..4b86bef06a52 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -214,13 +214,13 @@  static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 		 * Compute a bandwidth estimation by multiplying the frame
 		 * size by the number of video frames per second, divide the
 		 * result by the number of USB frames (or micro-frames for
-		 * high-speed devices) per second and add the UVC header size
-		 * (assumed to be 12 bytes long).
+		 * high- and super-speed devices) per second and add the UVC
+		 * header size (assumed to be 12 bytes long).
 		 */
 		bandwidth = frame->wWidth * frame->wHeight / 8 * format->bpp;
 		bandwidth *= 10000000 / interval + 1;
 		bandwidth /= 1000;
-		if (stream->dev->udev->speed == USB_SPEED_HIGH)
+		if (stream->dev->udev->speed >= USB_SPEED_HIGH)
 			bandwidth /= 8;
 		bandwidth += 12;