diff mbox series

[v2,4/7] media: uvcvideo: Reorganize format descriptor parsing

Message ID 20230506071427.28108-5-laurent.pinchart@ideasonboard.com
State Accepted
Commit c9d597b9b7ef9ffcb54051f04798b608edc6850c
Headers show
Series media: uvcvideo: Misc cleanups | expand

Commit Message

Laurent Pinchart May 6, 2023, 7:14 a.m. UTC
Format descriptor parsing has grown over time and now mixes parsing of
frame intervals with various quirk handling. Reorganize it to make the
code easier to follow, by parsing frame intervals first, and then
applying fixes and quirks. No functional change is intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 40 +++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 15 deletions(-)

Comments

Ricardo Ribalda May 15, 2023, 12:16 p.m. UTC | #1
On Sat, 6 May 2023 at 09:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Format descriptor parsing has grown over time and now mixes parsing of
> frame intervals with various quirk handling. Reorganize it to make the
> code easier to follow, by parsing frame intervals first, and then
> applying fixes and quirks. No functional change is intended.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 40 +++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9e597bbbfe07..446bd8ff128c 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -405,8 +405,27 @@ static int uvc_parse_format(struct uvc_device *dev,
>                                 get_unaligned_le32(&buffer[17]);
>                         frame->bFrameIntervalType = buffer[21];
>                 }
> +
> +               /*
> +                * Copy the frame intervals.
> +                *
> +                * Some bogus devices report dwMinFrameInterval equal to
> +                * dwMaxFrameInterval and have dwFrameIntervalStep set to
> +                * zero. Setting all null intervals to 1 fixes the problem and
> +                * some other divisions by zero that could happen.
> +                */
>                 frame->dwFrameInterval = *intervals;
>
> +               for (i = 0; i < n; ++i) {
> +                       interval = get_unaligned_le32(&buffer[26+4*i]);
> +                       *(*intervals)++ = interval ? interval : 1;
> +               }
> +
> +               /*
> +                * Apply more fixes, quirks and workarounds to handle incorrect
> +                * or broken descriptors.
> +                */
> +
>                 /*
>                  * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
>                  * completely. Observed behaviours range from setting the
> @@ -420,27 +439,18 @@ static int uvc_parse_format(struct uvc_device *dev,
>                         frame->dwMaxVideoFrameBufferSize = format->bpp
>                                 * frame->wWidth * frame->wHeight / 8;
>
> -               /*
> -                * Some bogus devices report dwMinFrameInterval equal to
> -                * dwMaxFrameInterval and have dwFrameIntervalStep set to
> -                * zero. Setting all null intervals to 1 fixes the problem and
> -                * some other divisions by zero that could happen.
> -                */
> -               for (i = 0; i < n; ++i) {
> -                       interval = get_unaligned_le32(&buffer[26+4*i]);
> -                       *(*intervals)++ = interval ? interval : 1;
> -               }
> -
> -               /*
> -                * Make sure that the default frame interval stays between
> -                * the boundaries.
> -                */
> +               /* Clamp the default frame interval to the boundaries. */
>                 n -= frame->bFrameIntervalType ? 1 : 2;
>                 frame->dwDefaultFrameInterval =
>                         clamp(frame->dwDefaultFrameInterval,
>                               frame->dwFrameInterval[0],
>                               frame->dwFrameInterval[n]);
>
> +               /*
> +                * Some devices report frame intervals that are not functional.
> +                * If the corresponding quirk is set, restrict operation to the
> +                * first interval only.
> +                */
>                 if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
>                         frame->bFrameIntervalType = 1;
>                         frame->dwFrameInterval[0] =
> --
> Regards,
>
> Laurent Pinchart
>
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9e597bbbfe07..446bd8ff128c 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -405,8 +405,27 @@  static int uvc_parse_format(struct uvc_device *dev,
 				get_unaligned_le32(&buffer[17]);
 			frame->bFrameIntervalType = buffer[21];
 		}
+
+		/*
+		 * Copy the frame intervals.
+		 *
+		 * Some bogus devices report dwMinFrameInterval equal to
+		 * dwMaxFrameInterval and have dwFrameIntervalStep set to
+		 * zero. Setting all null intervals to 1 fixes the problem and
+		 * some other divisions by zero that could happen.
+		 */
 		frame->dwFrameInterval = *intervals;
 
+		for (i = 0; i < n; ++i) {
+			interval = get_unaligned_le32(&buffer[26+4*i]);
+			*(*intervals)++ = interval ? interval : 1;
+		}
+
+		/*
+		 * Apply more fixes, quirks and workarounds to handle incorrect
+		 * or broken descriptors.
+		 */
+
 		/*
 		 * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
 		 * completely. Observed behaviours range from setting the
@@ -420,27 +439,18 @@  static int uvc_parse_format(struct uvc_device *dev,
 			frame->dwMaxVideoFrameBufferSize = format->bpp
 				* frame->wWidth * frame->wHeight / 8;
 
-		/*
-		 * Some bogus devices report dwMinFrameInterval equal to
-		 * dwMaxFrameInterval and have dwFrameIntervalStep set to
-		 * zero. Setting all null intervals to 1 fixes the problem and
-		 * some other divisions by zero that could happen.
-		 */
-		for (i = 0; i < n; ++i) {
-			interval = get_unaligned_le32(&buffer[26+4*i]);
-			*(*intervals)++ = interval ? interval : 1;
-		}
-
-		/*
-		 * Make sure that the default frame interval stays between
-		 * the boundaries.
-		 */
+		/* Clamp the default frame interval to the boundaries. */
 		n -= frame->bFrameIntervalType ? 1 : 2;
 		frame->dwDefaultFrameInterval =
 			clamp(frame->dwDefaultFrameInterval,
 			      frame->dwFrameInterval[0],
 			      frame->dwFrameInterval[n]);
 
+		/*
+		 * Some devices report frame intervals that are not functional.
+		 * If the corresponding quirk is set, restrict operation to the
+		 * first interval only.
+		 */
 		if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
 			frame->bFrameIntervalType = 1;
 			frame->dwFrameInterval[0] =