[PATCH v2 0/7] media: uvcvideo: Misc cleanups

[PATCH v2 0/7] media: uvcvideo: Misc cleanups

[Laurent Pinchart]

Hello,

This small patch series contains miscellaneous drive-by cleanups for the
uvcvideo driver. There isn't much to tell here, please see individual
patches for details.

Laurent Pinchart (7):
  media: uvcvideo: Rename uvc_streaming 'format' field to 'formats'
  media: uvcvideo: Rename uvc_format 'frame' field to 'frames'
  media: uvcvideo: Use clamp() to replace manual implementation
  media: uvcvideo: Reorganize format descriptor parsing
  media: uvcvideo: Increment intervals pointer at end of parsing
  media: uvcvideo: Constify formats, frames and intervals
  media: uvcvideo: Constify descriptor buffers

 drivers/media/usb/uvc/uvc_driver.c | 76 ++++++++++++++++------------
 drivers/media/usb/uvc/uvc_v4l2.c   | 79 +++++++++++++++---------------
 drivers/media/usb/uvc/uvc_video.c  | 20 ++++----
 drivers/media/usb/uvc/uvcvideo.h   | 12 ++---
 4 files changed, 101 insertions(+), 86 deletions(-)


base-commit: 423f331364bfbcd1212b78ac9052894ff5213ac9
prerequisite-patch-id: 742f7d139e5ba686a5487b0133ba6587b5b421dc
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/7] media: uvcvideo: Rename uvc_streaming 'format' field to 'formats'

[Laurent Pinchart]

The uvc_streaming 'format' field points to an array of formats. Rename
it to 'formats' to make this clearer.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c |  4 ++--
 drivers/media/usb/uvc/uvc_v4l2.c   | 16 ++++++++--------
 drivers/media/usb/uvc/uvc_video.c  |  6 +++---
 drivers/media/usb/uvc/uvcvideo.h   |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index d631ce4f9f7b..6a4b3423b3b7 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -184,7 +184,7 @@ static void uvc_stream_delete(struct uvc_streaming *stream)
 
 	usb_put_intf(stream->intf);
 
-	kfree(stream->format);
+	kfree(stream->formats);
 	kfree(stream->header.bmaControls);
 	kfree(stream);
 }
@@ -677,7 +677,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 	frame = (struct uvc_frame *)&format[nformats];
 	interval = (u32 *)&frame[nframes];
 
-	streaming->format = format;
+	streaming->formats = format;
 	streaming->nformats = 0;
 
 	/* Parse the format descriptors. */
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 35453f81c1d9..6960d7ebd904 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -235,7 +235,7 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	 * format otherwise.
 	 */
 	for (i = 0; i < stream->nformats; ++i) {
-		format = &stream->format[i];
+		format = &stream->formats[i];
 		if (format->fcc == fmt->fmt.pix.pixelformat)
 			break;
 	}
@@ -319,8 +319,8 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	 * accepted the requested format as-is.
 	 */
 	for (i = 0; i < stream->nformats; ++i) {
-		if (probe->bFormatIndex == stream->format[i].index) {
-			format = &stream->format[i];
+		if (probe->bFormatIndex == stream->formats[i].index) {
+			format = &stream->formats[i];
 			break;
 		}
 	}
@@ -708,7 +708,7 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
 	fmt->index = index;
 	fmt->type = type;
 
-	format = &stream->format[fmt->index];
+	format = &stream->formats[fmt->index];
 	fmt->flags = 0;
 	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
 		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
@@ -1256,8 +1256,8 @@ static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
 
 	/* Look for the given pixel format */
 	for (i = 0; i < stream->nformats; i++) {
-		if (stream->format[i].fcc == fsize->pixel_format) {
-			format = &stream->format[i];
+		if (stream->formats[i].fcc == fsize->pixel_format) {
+			format = &stream->formats[i];
 			break;
 		}
 	}
@@ -1297,8 +1297,8 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
 
 	/* Look for the given pixel format and frame size */
 	for (i = 0; i < stream->nformats; i++) {
-		if (stream->format[i].fcc == fival->pixel_format) {
-			format = &stream->format[i];
+		if (stream->formats[i].fcc == fival->pixel_format) {
+			format = &stream->formats[i];
 			break;
 		}
 	}
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index d4b023d4de7c..af540f435099 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -166,8 +166,8 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	}
 
 	for (i = 0; i < stream->nformats; ++i) {
-		if (stream->format[i].index == ctrl->bFormatIndex) {
-			format = &stream->format[i];
+		if (stream->formats[i].index == ctrl->bFormatIndex) {
+			format = &stream->formats[i];
 			break;
 		}
 	}
@@ -2161,7 +2161,7 @@ int uvc_video_init(struct uvc_streaming *stream)
 	 * available format otherwise.
 	 */
 	for (i = stream->nformats; i > 0; --i) {
-		format = &stream->format[i-1];
+		format = &stream->formats[i-1];
 		if (format->index == probe->bFormatIndex)
 			break;
 	}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9a596c8d894a..05fec637417d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -438,7 +438,7 @@ struct uvc_streaming {
 	enum v4l2_buf_type type;
 
 	unsigned int nformats;
-	struct uvc_format *format;
+	struct uvc_format *formats;
 
 	struct uvc_streaming_control ctrl;
 	struct uvc_format *def_format;
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/7] media: uvcvideo: Rename uvc_format 'frame' field to 'frames'

[Laurent Pinchart]

The uvc_format 'frame' field points to an array of frames. Rename it to
'frames' to make this clearer.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c |  8 ++++----
 drivers/media/usb/uvc/uvc_v4l2.c   | 32 +++++++++++++++---------------
 drivers/media/usb/uvc/uvc_video.c  |  6 +++---
 drivers/media/usb/uvc/uvcvideo.h   |  2 +-
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 6a4b3423b3b7..1539eaf8f7d8 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -339,8 +339,8 @@ static int uvc_parse_format(struct uvc_device *dev,
 		ftype = 0;
 
 		/* Create a dummy frame descriptor. */
-		frame = &format->frame[0];
-		memset(&format->frame[0], 0, sizeof(format->frame[0]));
+		frame = &format->frames[0];
+		memset(&format->frames[0], 0, sizeof(format->frames[0]));
 		frame->bFrameIntervalType = 1;
 		frame->dwDefaultFrameInterval = 1;
 		frame->dwFrameInterval = *intervals;
@@ -370,7 +370,7 @@ static int uvc_parse_format(struct uvc_device *dev,
 	 */
 	while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
 	       buffer[2] == ftype) {
-		frame = &format->frame[format->nframes];
+		frame = &format->frames[format->nframes];
 		if (ftype != UVC_VS_FRAME_FRAME_BASED)
 			n = buflen > 25 ? buffer[25] : 0;
 		else
@@ -687,7 +687,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 		case UVC_VS_FORMAT_MJPEG:
 		case UVC_VS_FORMAT_DV:
 		case UVC_VS_FORMAT_FRAME_BASED:
-			format->frame = frame;
+			format->frames = frame;
 			ret = uvc_parse_format(dev, streaming, format,
 				&interval, buffer, buflen);
 			if (ret < 0)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 6960d7ebd904..e7261b2543cb 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -255,14 +255,14 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	maxd = (unsigned int)-1;
 
 	for (i = 0; i < format->nframes; ++i) {
-		u16 w = format->frame[i].wWidth;
-		u16 h = format->frame[i].wHeight;
+		u16 w = format->frames[i].wWidth;
+		u16 h = format->frames[i].wHeight;
 
 		d = min(w, rw) * min(h, rh);
 		d = w*h + rw*rh - 2*d;
 		if (d < maxd) {
 			maxd = d;
-			frame = &format->frame[i];
+			frame = &format->frames[i];
 		}
 
 		if (maxd == 0)
@@ -331,8 +331,8 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 			probe->bFormatIndex);
 
 	for (i = 0; i < format->nframes; ++i) {
-		if (probe->bFrameIndex == format->frame[i].bFrameIndex) {
-			frame = &format->frame[i];
+		if (probe->bFrameIndex == format->frames[i].bFrameIndex) {
+			frame = &format->frames[i];
 			break;
 		}
 	}
@@ -501,19 +501,19 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 	for (i = 0; i < format->nframes && maxd != 0; i++) {
 		u32 d, ival;
 
-		if (&format->frame[i] == stream->cur_frame)
+		if (&format->frames[i] == stream->cur_frame)
 			continue;
 
-		if (format->frame[i].wWidth != stream->cur_frame->wWidth ||
-		    format->frame[i].wHeight != stream->cur_frame->wHeight)
+		if (format->frames[i].wWidth != stream->cur_frame->wWidth ||
+		    format->frames[i].wHeight != stream->cur_frame->wHeight)
 			continue;
 
-		ival = uvc_try_frame_interval(&format->frame[i], interval);
+		ival = uvc_try_frame_interval(&format->frames[i], interval);
 		d = abs((s32)ival - interval);
 		if (d >= maxd)
 			continue;
 
-		frame = &format->frame[i];
+		frame = &format->frames[i];
 		probe.bFrameIndex = frame->bFrameIndex;
 		probe.dwFrameInterval = ival;
 		maxd = d;
@@ -1266,10 +1266,10 @@ static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
 
 	/* Skip duplicate frame sizes */
 	for (i = 0, index = 0; i < format->nframes; i++) {
-		if (frame && frame->wWidth == format->frame[i].wWidth &&
-		    frame->wHeight == format->frame[i].wHeight)
+		if (frame && frame->wWidth == format->frames[i].wWidth &&
+		    frame->wHeight == format->frames[i].wHeight)
 			continue;
-		frame = &format->frame[i];
+		frame = &format->frames[i];
 		if (index == fsize->index)
 			break;
 		index++;
@@ -1307,9 +1307,9 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
 
 	index = fival->index;
 	for (i = 0; i < format->nframes; i++) {
-		if (format->frame[i].wWidth == fival->width &&
-		    format->frame[i].wHeight == fival->height) {
-			frame = &format->frame[i];
+		if (format->frames[i].wWidth == fival->width &&
+		    format->frames[i].wHeight == fival->height) {
+			frame = &format->frames[i];
 			nintervals = frame->bFrameIntervalType ?: 1;
 			if (index < nintervals)
 				break;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index af540f435099..34c781b7dee2 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -176,8 +176,8 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 		return;
 
 	for (i = 0; i < format->nframes; ++i) {
-		if (format->frame[i].bFrameIndex == ctrl->bFrameIndex) {
-			frame = &format->frame[i];
+		if (format->frames[i].bFrameIndex == ctrl->bFrameIndex) {
+			frame = &format->frames[i];
 			break;
 		}
 	}
@@ -2179,7 +2179,7 @@ int uvc_video_init(struct uvc_streaming *stream)
 	 * descriptor is not found, use the first available frame.
 	 */
 	for (i = format->nframes; i > 0; --i) {
-		frame = &format->frame[i-1];
+		frame = &format->frames[i-1];
 		if (frame->bFrameIndex == probe->bFrameIndex)
 			break;
 	}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 05fec637417d..bb0773637171 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -265,7 +265,7 @@ struct uvc_format {
 	u32 flags;
 
 	unsigned int nframes;
-	struct uvc_frame *frame;
+	struct uvc_frame *frames;
 };
 
 struct uvc_streaming_header {
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/7] media: uvcvideo: Use clamp() to replace manual implementation

[Laurent Pinchart]

The kernel has a nice clamp() macro, use it to replace a manual
implementation based on min() and max(). No functional change is
intended.

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 1539eaf8f7d8..9e597bbbfe07 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -437,9 +437,9 @@ static int uvc_parse_format(struct uvc_device *dev,
 		 */
 		n -= frame->bFrameIntervalType ? 1 : 2;
 		frame->dwDefaultFrameInterval =
-			min(frame->dwFrameInterval[n],
-			    max(frame->dwFrameInterval[0],
-				frame->dwDefaultFrameInterval));
+			clamp(frame->dwDefaultFrameInterval,
+			      frame->dwFrameInterval[0],
+			      frame->dwFrameInterval[n]);
 
 		if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
 			frame->bFrameIntervalType = 1;
-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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

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

[PATCH v2 5/7] media: uvcvideo: Increment intervals pointer at end of parsing

[Laurent Pinchart]

The intervals pointer is incremented for each interval when parsing the
format descriptor. This doesn't cause any issue as such, but gets in the
way of constifying some pointers. Modify the parsing code to index the
intervals pointer as an array and increment it in one go at end of
parsing.

Careful readers will notice that the maxIntervalIndex variable is set to
1 instead of n - 2 when bFrameIntervalType has a zero value. This is
functionally equivalent, as n is equal to 3 in that case.

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 446bd8ff128c..11e4fa007f3f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev,
 	 */
 	while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
 	       buffer[2] == ftype) {
+		unsigned int maxIntervalIndex;
+
 		frame = &format->frames[format->nframes];
 		if (ftype != UVC_VS_FRAME_FRAME_BASED)
 			n = buflen > 25 ? buffer[25] : 0;
@@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev,
 
 		for (i = 0; i < n; ++i) {
 			interval = get_unaligned_le32(&buffer[26+4*i]);
-			*(*intervals)++ = interval ? interval : 1;
+			(*intervals)[i] = interval ? interval : 1;
 		}
 
 		/*
@@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev,
 				* frame->wWidth * frame->wHeight / 8;
 
 		/* Clamp the default frame interval to the boundaries. */
-		n -= frame->bFrameIntervalType ? 1 : 2;
+		maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
 		frame->dwDefaultFrameInterval =
 			clamp(frame->dwDefaultFrameInterval,
 			      frame->dwFrameInterval[0],
-			      frame->dwFrameInterval[n]);
+			      frame->dwFrameInterval[maxIntervalIndex]);
 
 		/*
 		 * Some devices report frame intervals that are not functional.
@@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev,
 			(100000000 / frame->dwDefaultFrameInterval) % 10);
 
 		format->nframes++;
+		*intervals += n;
+
 		buflen -= buffer[0];
 		buffer += buffer[0];
 	}
-- 
Regards,

Laurent Pinchart

[PATCH v2 6/7] media: uvcvideo: Constify formats, frames and intervals

[Laurent Pinchart]

The formats, frames and intervals stored in the uvc_streaming structure
are not meant to change after being parsed at probe time. Make them
const to prevent unintended modifications, and adapt the probe code
accordingly to use non-const pointers during parsing.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 16 +++++++--------
 drivers/media/usb/uvc/uvc_v4l2.c   | 31 +++++++++++++++---------------
 drivers/media/usb/uvc/uvc_video.c  |  8 ++++----
 drivers/media/usb/uvc/uvcvideo.h   | 12 ++++++------
 4 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 11e4fa007f3f..b68fa7d17e41 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -221,7 +221,8 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
 
 static int uvc_parse_format(struct uvc_device *dev,
 	struct uvc_streaming *streaming, struct uvc_format *format,
-	u32 **intervals, unsigned char *buffer, int buflen)
+	struct uvc_frame *frames, u32 **intervals, unsigned char *buffer,
+	int buflen)
 {
 	struct usb_interface *intf = streaming->intf;
 	struct usb_host_interface *alts = intf->cur_altsetting;
@@ -235,6 +236,7 @@ static int uvc_parse_format(struct uvc_device *dev,
 
 	format->type = buffer[2];
 	format->index = buffer[3];
+	format->frames = frames;
 
 	switch (buffer[2]) {
 	case UVC_VS_FORMAT_UNCOMPRESSED:
@@ -339,8 +341,8 @@ static int uvc_parse_format(struct uvc_device *dev,
 		ftype = 0;
 
 		/* Create a dummy frame descriptor. */
-		frame = &format->frames[0];
-		memset(&format->frames[0], 0, sizeof(format->frames[0]));
+		frame = &frames[0];
+		memset(frame, 0, sizeof(*frame));
 		frame->bFrameIntervalType = 1;
 		frame->dwDefaultFrameInterval = 1;
 		frame->dwFrameInterval = *intervals;
@@ -372,7 +374,7 @@ static int uvc_parse_format(struct uvc_device *dev,
 	       buffer[2] == ftype) {
 		unsigned int maxIntervalIndex;
 
-		frame = &format->frames[format->nframes];
+		frame = &frames[format->nframes];
 		if (ftype != UVC_VS_FRAME_FRAME_BASED)
 			n = buflen > 25 ? buffer[25] : 0;
 		else
@@ -455,8 +457,7 @@ static int uvc_parse_format(struct uvc_device *dev,
 		 */
 		if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
 			frame->bFrameIntervalType = 1;
-			frame->dwFrameInterval[0] =
-				frame->dwDefaultFrameInterval;
+			(*intervals)[0] = frame->dwDefaultFrameInterval;
 		}
 
 		uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
@@ -701,8 +702,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 		case UVC_VS_FORMAT_MJPEG:
 		case UVC_VS_FORMAT_DV:
 		case UVC_VS_FORMAT_FRAME_BASED:
-			format->frames = frame;
-			ret = uvc_parse_format(dev, streaming, format,
+			ret = uvc_parse_format(dev, streaming, format, frame,
 				&interval, buffer, buflen);
 			if (ret < 0)
 				goto error;
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index e7261b2543cb..5ac2a424b13d 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -161,7 +161,7 @@ static int uvc_ioctl_xu_ctrl_map(struct uvc_video_chain *chain,
  * the Video Probe and Commit negotiation, but some hardware don't implement
  * that feature.
  */
-static u32 uvc_try_frame_interval(struct uvc_frame *frame, u32 interval)
+static u32 uvc_try_frame_interval(const struct uvc_frame *frame, u32 interval)
 {
 	unsigned int i;
 
@@ -210,10 +210,11 @@ static u32 uvc_v4l2_get_bytesperline(const struct uvc_format *format,
 
 static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	struct v4l2_format *fmt, struct uvc_streaming_control *probe,
-	struct uvc_format **uvc_format, struct uvc_frame **uvc_frame)
+	const struct uvc_format **uvc_format,
+	const struct uvc_frame **uvc_frame)
 {
-	struct uvc_format *format = NULL;
-	struct uvc_frame *frame = NULL;
+	const struct uvc_format *format = NULL;
+	const struct uvc_frame *frame = NULL;
 	u16 rw, rh;
 	unsigned int d, maxd;
 	unsigned int i;
@@ -363,8 +364,8 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 static int uvc_v4l2_get_format(struct uvc_streaming *stream,
 	struct v4l2_format *fmt)
 {
-	struct uvc_format *format;
-	struct uvc_frame *frame;
+	const struct uvc_format *format;
+	const struct uvc_frame *frame;
 	int ret = 0;
 
 	if (fmt->type != stream->type)
@@ -398,8 +399,8 @@ static int uvc_v4l2_set_format(struct uvc_streaming *stream,
 	struct v4l2_format *fmt)
 {
 	struct uvc_streaming_control probe;
-	struct uvc_format *format;
-	struct uvc_frame *frame;
+	const struct uvc_format *format;
+	const struct uvc_frame *frame;
 	int ret;
 
 	if (fmt->type != stream->type)
@@ -465,8 +466,8 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 {
 	struct uvc_streaming_control probe;
 	struct v4l2_fract timeperframe;
-	struct uvc_format *format;
-	struct uvc_frame *frame;
+	const struct uvc_format *format;
+	const struct uvc_frame *frame;
 	u32 interval, maxd;
 	unsigned int i;
 	int ret;
@@ -697,7 +698,7 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
 static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
 			      struct v4l2_fmtdesc *fmt)
 {
-	struct uvc_format *format;
+	const struct uvc_format *format;
 	enum v4l2_buf_type type = fmt->type;
 	u32 index = fmt->index;
 
@@ -1249,8 +1250,8 @@ static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
-	struct uvc_format *format = NULL;
-	struct uvc_frame *frame = NULL;
+	const struct uvc_format *format = NULL;
+	const struct uvc_frame *frame = NULL;
 	unsigned int index;
 	unsigned int i;
 
@@ -1289,8 +1290,8 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
-	struct uvc_format *format = NULL;
-	struct uvc_frame *frame = NULL;
+	const struct uvc_format *format = NULL;
+	const struct uvc_frame *frame = NULL;
 	unsigned int nintervals;
 	unsigned int index;
 	unsigned int i;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 34c781b7dee2..28dde08ec6c5 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -137,8 +137,8 @@ static const struct usb_device_id elgato_cam_link_4k = {
 static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_streaming_control *ctrl)
 {
-	struct uvc_format *format = NULL;
-	struct uvc_frame *frame = NULL;
+	const struct uvc_format *format = NULL;
+	const struct uvc_frame *frame = NULL;
 	unsigned int i;
 
 	/*
@@ -2100,8 +2100,8 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
 int uvc_video_init(struct uvc_streaming *stream)
 {
 	struct uvc_streaming_control *probe = &stream->ctrl;
-	struct uvc_format *format = NULL;
-	struct uvc_frame *frame = NULL;
+	const struct uvc_format *format = NULL;
+	const struct uvc_frame *frame = NULL;
 	struct uvc_urb *uvc_urb;
 	unsigned int i;
 	int ret;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index bb0773637171..6fb0a78b1b00 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -251,7 +251,7 @@ struct uvc_frame {
 	u32 dwMaxVideoFrameBufferSize;
 	u8  bFrameIntervalType;
 	u32 dwDefaultFrameInterval;
-	u32 *dwFrameInterval;
+	const u32 *dwFrameInterval;
 };
 
 struct uvc_format {
@@ -265,7 +265,7 @@ struct uvc_format {
 	u32 flags;
 
 	unsigned int nframes;
-	struct uvc_frame *frames;
+	const struct uvc_frame *frames;
 };
 
 struct uvc_streaming_header {
@@ -438,12 +438,12 @@ struct uvc_streaming {
 	enum v4l2_buf_type type;
 
 	unsigned int nformats;
-	struct uvc_format *formats;
+	const struct uvc_format *formats;
 
 	struct uvc_streaming_control ctrl;
-	struct uvc_format *def_format;
-	struct uvc_format *cur_format;
-	struct uvc_frame *cur_frame;
+	const struct uvc_format *def_format;
+	const struct uvc_format *cur_format;
+	const struct uvc_frame *cur_frame;
 
 	/*
 	 * Protect access to ctrl, cur_format, cur_frame and hardware video
-- 
Regards,

Laurent Pinchart

[PATCH v2 7/7] media: uvcvideo: Constify descriptor buffers

[Laurent Pinchart]

There is no need to modify the content of UVC descriptor buffers during
parsing. Make all the corresponding pointers const to avoid unintended
modifications.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index b68fa7d17e41..c8071c0c9bde 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -221,7 +221,7 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
 
 static int uvc_parse_format(struct uvc_device *dev,
 	struct uvc_streaming *streaming, struct uvc_format *format,
-	struct uvc_frame *frames, u32 **intervals, unsigned char *buffer,
+	struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer,
 	int buflen)
 {
 	struct usb_interface *intf = streaming->intf;
@@ -508,7 +508,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 	struct uvc_format *format;
 	struct uvc_frame *frame;
 	struct usb_host_interface *alts = &intf->altsetting[0];
-	unsigned char *_buffer, *buffer = alts->extra;
+	const unsigned char *_buffer, *buffer = alts->extra;
 	int _buflen, buflen = alts->extralen;
 	unsigned int nformats = 0, nframes = 0, nintervals = 0;
 	unsigned int size, i, n, p;
@@ -1161,7 +1161,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
 static int uvc_parse_control(struct uvc_device *dev)
 {
 	struct usb_host_interface *alts = dev->intf->cur_altsetting;
-	unsigned char *buffer = alts->extra;
+	const unsigned char *buffer = alts->extra;
 	int buflen = alts->extralen;
 	int ret;
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/7] media: uvcvideo: Use clamp() to replace manual implementation

[Ricardo Ribalda]

On Sat, 6 May 2023 at 09:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The kernel has a nice clamp() macro, use it to replace a manual
> implementation based on min() and max(). 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 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 1539eaf8f7d8..9e597bbbfe07 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -437,9 +437,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>                  */
>                 n -= frame->bFrameIntervalType ? 1 : 2;
>                 frame->dwDefaultFrameInterval =
> -                       min(frame->dwFrameInterval[n],
> -                           max(frame->dwFrameInterval[0],
> -                               frame->dwDefaultFrameInterval));
> +                       clamp(frame->dwDefaultFrameInterval,
> +                             frame->dwFrameInterval[0],
> +                             frame->dwFrameInterval[n]);
>
>                 if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
>                         frame->bFrameIntervalType = 1;
> --
> Regards,
>
> Laurent Pinchart
>


-- 
Ricardo Ribalda

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

[Ricardo Ribalda]

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
>


-- 
Ricardo Ribalda

Re: [PATCH v2 5/7] media: uvcvideo: Increment intervals pointer at end of parsing

[Ricardo Ribalda]

Hi Laurent

On Sat, 6 May 2023 at 09:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The intervals pointer is incremented for each interval when parsing the
> format descriptor. This doesn't cause any issue as such, but gets in the
> way of constifying some pointers. Modify the parsing code to index the
> intervals pointer as an array and increment it in one go at end of
> parsing.
>
> Careful readers will notice that the maxIntervalIndex variable is set to
> 1 instead of n - 2 when bFrameIntervalType has a zero value. This is
> functionally equivalent, as n is equal to 3 in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 446bd8ff128c..11e4fa007f3f 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev,
>          */
>         while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
>                buffer[2] == ftype) {
> +               unsigned int maxIntervalIndex;
> +
>                 frame = &format->frames[format->nframes];
>                 if (ftype != UVC_VS_FRAME_FRAME_BASED)
>                         n = buflen > 25 ? buffer[25] : 0;
> @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev,
>
>                 for (i = 0; i < n; ++i) {
>                         interval = get_unaligned_le32(&buffer[26+4*i]);
> -                       *(*intervals)++ = interval ? interval : 1;
> +                       (*intervals)[i] = interval ? interval : 1;
>                 }
>
>                 /*
> @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev,
>                                 * frame->wWidth * frame->wHeight / 8;
>
>                 /* Clamp the default frame interval to the boundaries. */
> -               n -= frame->bFrameIntervalType ? 1 : 2;
> +               maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;

Maybe it is worth mentioning that bFrameIntervalType == 0 is a
continuous interval. idex 0 is min and 1 is max.

>                 frame->dwDefaultFrameInterval =
>                         clamp(frame->dwDefaultFrameInterval,
>                               frame->dwFrameInterval[0],
> -                             frame->dwFrameInterval[n]);
> +                             frame->dwFrameInterval[maxIntervalIndex]);
>
>                 /*
>                  * Some devices report frame intervals that are not functional.
> @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev,
>                         (100000000 / frame->dwDefaultFrameInterval) % 10);
>
>                 format->nframes++;
> +               *intervals += n;
> +
>                 buflen -= buffer[0];
>                 buffer += buffer[0];
>         }
> --
> Regards,
>
> Laurent Pinchart
>


-- 
Ricardo Ribalda

Re: [PATCH v2 6/7] media: uvcvideo: Constify formats, frames and intervals

[Ricardo Ribalda]

On Sat, 6 May 2023 at 09:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The formats, frames and intervals stored in the uvc_streaming structure
> are not meant to change after being parsed at probe time. Make them
> const to prevent unintended modifications, and adapt the probe code
> accordingly to use non-const pointers during parsing.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 16 +++++++--------
>  drivers/media/usb/uvc/uvc_v4l2.c   | 31 +++++++++++++++---------------
>  drivers/media/usb/uvc/uvc_video.c  |  8 ++++----
>  drivers/media/usb/uvc/uvcvideo.h   | 12 ++++++------
>  4 files changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 11e4fa007f3f..b68fa7d17e41 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -221,7 +221,8 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
>
>  static int uvc_parse_format(struct uvc_device *dev,
>         struct uvc_streaming *streaming, struct uvc_format *format,
> -       u32 **intervals, unsigned char *buffer, int buflen)
> +       struct uvc_frame *frames, u32 **intervals, unsigned char *buffer,
> +       int buflen)
>  {
>         struct usb_interface *intf = streaming->intf;
>         struct usb_host_interface *alts = intf->cur_altsetting;
> @@ -235,6 +236,7 @@ static int uvc_parse_format(struct uvc_device *dev,
>
>         format->type = buffer[2];
>         format->index = buffer[3];
> +       format->frames = frames;
>
>         switch (buffer[2]) {
>         case UVC_VS_FORMAT_UNCOMPRESSED:
> @@ -339,8 +341,8 @@ static int uvc_parse_format(struct uvc_device *dev,
>                 ftype = 0;
>
>                 /* Create a dummy frame descriptor. */
> -               frame = &format->frames[0];
> -               memset(&format->frames[0], 0, sizeof(format->frames[0]));
> +               frame = &frames[0];
> +               memset(frame, 0, sizeof(*frame));
>                 frame->bFrameIntervalType = 1;
>                 frame->dwDefaultFrameInterval = 1;
>                 frame->dwFrameInterval = *intervals;
> @@ -372,7 +374,7 @@ static int uvc_parse_format(struct uvc_device *dev,
>                buffer[2] == ftype) {
>                 unsigned int maxIntervalIndex;
>
> -               frame = &format->frames[format->nframes];
> +               frame = &frames[format->nframes];
>                 if (ftype != UVC_VS_FRAME_FRAME_BASED)
>                         n = buflen > 25 ? buffer[25] : 0;
>                 else
> @@ -455,8 +457,7 @@ static int uvc_parse_format(struct uvc_device *dev,
>                  */
>                 if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
>                         frame->bFrameIntervalType = 1;
> -                       frame->dwFrameInterval[0] =
> -                               frame->dwDefaultFrameInterval;
> +                       (*intervals)[0] = frame->dwDefaultFrameInterval;
>                 }
>
>                 uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> @@ -701,8 +702,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
>                 case UVC_VS_FORMAT_MJPEG:
>                 case UVC_VS_FORMAT_DV:
>                 case UVC_VS_FORMAT_FRAME_BASED:
> -                       format->frames = frame;
> -                       ret = uvc_parse_format(dev, streaming, format,
> +                       ret = uvc_parse_format(dev, streaming, format, frame,
>                                 &interval, buffer, buflen);
>                         if (ret < 0)
>                                 goto error;
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index e7261b2543cb..5ac2a424b13d 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -161,7 +161,7 @@ static int uvc_ioctl_xu_ctrl_map(struct uvc_video_chain *chain,
>   * the Video Probe and Commit negotiation, but some hardware don't implement
>   * that feature.
>   */
> -static u32 uvc_try_frame_interval(struct uvc_frame *frame, u32 interval)
> +static u32 uvc_try_frame_interval(const struct uvc_frame *frame, u32 interval)
>  {
>         unsigned int i;
>
> @@ -210,10 +210,11 @@ static u32 uvc_v4l2_get_bytesperline(const struct uvc_format *format,
>
>  static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>         struct v4l2_format *fmt, struct uvc_streaming_control *probe,
> -       struct uvc_format **uvc_format, struct uvc_frame **uvc_frame)
> +       const struct uvc_format **uvc_format,
> +       const struct uvc_frame **uvc_frame)
>  {
> -       struct uvc_format *format = NULL;
> -       struct uvc_frame *frame = NULL;
> +       const struct uvc_format *format = NULL;
> +       const struct uvc_frame *frame = NULL;
>         u16 rw, rh;
>         unsigned int d, maxd;
>         unsigned int i;
> @@ -363,8 +364,8 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>  static int uvc_v4l2_get_format(struct uvc_streaming *stream,
>         struct v4l2_format *fmt)
>  {
> -       struct uvc_format *format;
> -       struct uvc_frame *frame;
> +       const struct uvc_format *format;
> +       const struct uvc_frame *frame;
>         int ret = 0;
>
>         if (fmt->type != stream->type)
> @@ -398,8 +399,8 @@ static int uvc_v4l2_set_format(struct uvc_streaming *stream,
>         struct v4l2_format *fmt)
>  {
>         struct uvc_streaming_control probe;
> -       struct uvc_format *format;
> -       struct uvc_frame *frame;
> +       const struct uvc_format *format;
> +       const struct uvc_frame *frame;
>         int ret;
>
>         if (fmt->type != stream->type)
> @@ -465,8 +466,8 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
>  {
>         struct uvc_streaming_control probe;
>         struct v4l2_fract timeperframe;
> -       struct uvc_format *format;
> -       struct uvc_frame *frame;
> +       const struct uvc_format *format;
> +       const struct uvc_frame *frame;
>         u32 interval, maxd;
>         unsigned int i;
>         int ret;
> @@ -697,7 +698,7 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
>  static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>                               struct v4l2_fmtdesc *fmt)
>  {
> -       struct uvc_format *format;
> +       const struct uvc_format *format;
>         enum v4l2_buf_type type = fmt->type;
>         u32 index = fmt->index;
>
> @@ -1249,8 +1250,8 @@ static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
>  {
>         struct uvc_fh *handle = fh;
>         struct uvc_streaming *stream = handle->stream;
> -       struct uvc_format *format = NULL;
> -       struct uvc_frame *frame = NULL;
> +       const struct uvc_format *format = NULL;
> +       const struct uvc_frame *frame = NULL;
>         unsigned int index;
>         unsigned int i;
>
> @@ -1289,8 +1290,8 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
>  {
>         struct uvc_fh *handle = fh;
>         struct uvc_streaming *stream = handle->stream;
> -       struct uvc_format *format = NULL;
> -       struct uvc_frame *frame = NULL;
> +       const struct uvc_format *format = NULL;
> +       const struct uvc_frame *frame = NULL;
>         unsigned int nintervals;
>         unsigned int index;
>         unsigned int i;
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 34c781b7dee2..28dde08ec6c5 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -137,8 +137,8 @@ static const struct usb_device_id elgato_cam_link_4k = {
>  static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
>         struct uvc_streaming_control *ctrl)
>  {
> -       struct uvc_format *format = NULL;
> -       struct uvc_frame *frame = NULL;
> +       const struct uvc_format *format = NULL;
> +       const struct uvc_frame *frame = NULL;
>         unsigned int i;
>
>         /*
> @@ -2100,8 +2100,8 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
>  int uvc_video_init(struct uvc_streaming *stream)
>  {
>         struct uvc_streaming_control *probe = &stream->ctrl;
> -       struct uvc_format *format = NULL;
> -       struct uvc_frame *frame = NULL;
> +       const struct uvc_format *format = NULL;
> +       const struct uvc_frame *frame = NULL;
>         struct uvc_urb *uvc_urb;
>         unsigned int i;
>         int ret;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index bb0773637171..6fb0a78b1b00 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -251,7 +251,7 @@ struct uvc_frame {
>         u32 dwMaxVideoFrameBufferSize;
>         u8  bFrameIntervalType;
>         u32 dwDefaultFrameInterval;
> -       u32 *dwFrameInterval;
> +       const u32 *dwFrameInterval;
>  };
>
>  struct uvc_format {
> @@ -265,7 +265,7 @@ struct uvc_format {
>         u32 flags;
>
>         unsigned int nframes;
> -       struct uvc_frame *frames;
> +       const struct uvc_frame *frames;
>  };
>
>  struct uvc_streaming_header {
> @@ -438,12 +438,12 @@ struct uvc_streaming {
>         enum v4l2_buf_type type;
>
>         unsigned int nformats;
> -       struct uvc_format *formats;
> +       const struct uvc_format *formats;
>
>         struct uvc_streaming_control ctrl;
> -       struct uvc_format *def_format;
> -       struct uvc_format *cur_format;
> -       struct uvc_frame *cur_frame;
> +       const struct uvc_format *def_format;
> +       const struct uvc_format *cur_format;
> +       const struct uvc_frame *cur_frame;
>
>         /*
>          * Protect access to ctrl, cur_format, cur_frame and hardware video
> --
> Regards,
>
> Laurent Pinchart
>


-- 
Ricardo Ribalda

Re: [PATCH v2 5/7] media: uvcvideo: Increment intervals pointer at end of parsing

[Laurent Pinchart]

Hi Ricardo,

On Mon, May 15, 2023 at 03:22:13PM +0200, Ricardo Ribalda wrote:
> On Sat, 6 May 2023 at 09:14, Laurent Pinchart wrote:
> >
> > The intervals pointer is incremented for each interval when parsing the
> > format descriptor. This doesn't cause any issue as such, but gets in the
> > way of constifying some pointers. Modify the parsing code to index the
> > intervals pointer as an array and increment it in one go at end of
> > parsing.
> >
> > Careful readers will notice that the maxIntervalIndex variable is set to
> > 1 instead of n - 2 when bFrameIntervalType has a zero value. This is
> > functionally equivalent, as n is equal to 3 in that case.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 446bd8ff128c..11e4fa007f3f 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >          */
> >         while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> >                buffer[2] == ftype) {
> > +               unsigned int maxIntervalIndex;
> > +
> >                 frame = &format->frames[format->nframes];
> >                 if (ftype != UVC_VS_FRAME_FRAME_BASED)
> >                         n = buflen > 25 ? buffer[25] : 0;
> > @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev,
> >
> >                 for (i = 0; i < n; ++i) {
> >                         interval = get_unaligned_le32(&buffer[26+4*i]);
> > -                       *(*intervals)++ = interval ? interval : 1;
> > +                       (*intervals)[i] = interval ? interval : 1;
> >                 }
> >
> >                 /*
> > @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                                 * frame->wWidth * frame->wHeight / 8;
> >
> >                 /* Clamp the default frame interval to the boundaries. */
> > -               n -= frame->bFrameIntervalType ? 1 : 2;
> > +               maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> 
> Maybe it is worth mentioning that bFrameIntervalType == 0 is a
> continuous interval. idex 0 is min and 1 is max.

I'll update the comment to

                /*
                 * Clamp the default frame interval to the boundaries. A zero
                 * bFrameIntervalType value indicates a continuous frame
                 * interval range, with dwFrameInterval[0] storing the minimum
                 * value and dwFrameInterval[1] storing the maximum value.
                 */

> >                 frame->dwDefaultFrameInterval =
> >                         clamp(frame->dwDefaultFrameInterval,
> >                               frame->dwFrameInterval[0],
> > -                             frame->dwFrameInterval[n]);
> > +                             frame->dwFrameInterval[maxIntervalIndex]);
> >
> >                 /*
> >                  * Some devices report frame intervals that are not functional.
> > @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                         (100000000 / frame->dwDefaultFrameInterval) % 10);
> >
> >                 format->nframes++;
> > +               *intervals += n;
> > +
> >                 buflen -= buffer[0];
> >                 buffer += buffer[0];
> >         }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/7] media: uvcvideo: Increment intervals pointer at end of parsing

[Ricardo Ribalda]

Hi Laurent

On Mon, 15 May 2023 at 16:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Mon, May 15, 2023 at 03:22:13PM +0200, Ricardo Ribalda wrote:
> > On Sat, 6 May 2023 at 09:14, Laurent Pinchart wrote:
> > >
> > > The intervals pointer is incremented for each interval when parsing the
> > > format descriptor. This doesn't cause any issue as such, but gets in the
> > > way of constifying some pointers. Modify the parsing code to index the
> > > intervals pointer as an array and increment it in one go at end of
> > > parsing.
> > >
> > > Careful readers will notice that the maxIntervalIndex variable is set to
> > > 1 instead of n - 2 when bFrameIntervalType has a zero value. This is
> > > functionally equivalent, as n is equal to 3 in that case.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> >
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 446bd8ff128c..11e4fa007f3f 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> > >          */
> > >         while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> > >                buffer[2] == ftype) {
> > > +               unsigned int maxIntervalIndex;
> > > +
> > >                 frame = &format->frames[format->nframes];
> > >                 if (ftype != UVC_VS_FRAME_FRAME_BASED)
> > >                         n = buflen > 25 ? buffer[25] : 0;
> > > @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev,
> > >
> > >                 for (i = 0; i < n; ++i) {
> > >                         interval = get_unaligned_le32(&buffer[26+4*i]);
> > > -                       *(*intervals)++ = interval ? interval : 1;
> > > +                       (*intervals)[i] = interval ? interval : 1;
> > >                 }
> > >
> > >                 /*
> > > @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev,
> > >                                 * frame->wWidth * frame->wHeight / 8;
> > >
> > >                 /* Clamp the default frame interval to the boundaries. */
> > > -               n -= frame->bFrameIntervalType ? 1 : 2;
> > > +               maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> >
> > Maybe it is worth mentioning that bFrameIntervalType == 0 is a
> > continuous interval. idex 0 is min and 1 is max.
>
> I'll update the comment to
>
>                 /*
>                  * Clamp the default frame interval to the boundaries. A zero
>                  * bFrameIntervalType value indicates a continuous frame
>                  * interval range, with dwFrameInterval[0] storing the minimum
>                  * value and dwFrameInterval[1] storing the maximum value.
>                  */
>
Thanks! It looks much better :)


> > >                 frame->dwDefaultFrameInterval =
> > >                         clamp(frame->dwDefaultFrameInterval,
> > >                               frame->dwFrameInterval[0],
> > > -                             frame->dwFrameInterval[n]);
> > > +                             frame->dwFrameInterval[maxIntervalIndex]);
> > >
> > >                 /*
> > >                  * Some devices report frame intervals that are not functional.
> > > @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> > >                         (100000000 / frame->dwDefaultFrameInterval) % 10);
> > >
> > >                 format->nframes++;
> > > +               *intervals += n;
> > > +
> > >                 buflen -= buffer[0];
> > >                 buffer += buffer[0];
> > >         }
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda