[PATCH 00/14] media: omap3isp: v4l2-compliance fixes

[PATCH 00/14] media: omap3isp: v4l2-compliance fixes

[Hans Verkuil]

When I worked on the patch series to remove the vb2 wait_prepare/finish
callbacks, I had to test that series on my Beagle xM board with a
Leopard Imaging li5m03 camera.

Since I wanted to use v4l2-compliance to test the vb2 change, I first had
to fix a bunch of other compliance problems in this driver so it would
actually be able to run the streaming tests.

This series contains the fixes I made to get to that point.

It depends on one sensor driver fix (posted separately):

https://patchwork.linuxtv.org/project/linux-media/patch/554fb9d7-374b-4868-b91b-959b8fd69b4d@kernel.org/

The last two patches in this series are for reference only and not
meant to be merged.

The first patch ("configure entity functions") definitely needs to be
reviewed: I'm not certain what all blocks did, so in a few places I had
to guess what the entity function is.

This series doesn't fix all compliance problems, but at least it is
a lot better now.

Regards,

	Hans

Hans Verkuil (14):
  media: omap3isp: configure entity functions
  media: omap3isp: add V4L2_CAP_IO_MC and don't set bus_info
  media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes
  media: omap3isp: implement enum_fmt_vid_cap/out
  media: omap3isp: use V4L2_COLORSPACE_SRGB instead of _JPEG
  media: omap3isp: set initial format
  media: omap3isp: rework isp_video_try/set_format
  media: omap3isp: implement create/prepare_bufs
  media: omap3isp: better VIDIOC_G/S_PARM handling
  media: omap3isp: support ctrl events for isppreview
  media: omap3isp: ispccp2: always clamp in ccp2_try_format()
  media: omap3isp: isppreview: always clamp in preview_try_format()
  DO NOT MERGE: media: omap3isp: change default resolution to 864x648
  DO NOT MERGE: omap3-beagle-xm.dts: add Leopard Imaging li5m03 support

 .../dts/ti/omap/omap3-beagle-xm-li5m03.dtsi   |  64 +++++++
 arch/arm/boot/dts/ti/omap/omap3-beagle-xm.dts |   2 +
 drivers/media/platform/ti/omap3isp/ispccdc.c  |   1 +
 drivers/media/platform/ti/omap3isp/ispccp2.c  |   3 +-
 drivers/media/platform/ti/omap3isp/ispcsi2.c  |   1 +
 .../media/platform/ti/omap3isp/isppreview.c   |  26 ++-
 .../media/platform/ti/omap3isp/ispresizer.c   |   3 +-
 drivers/media/platform/ti/omap3isp/ispstat.c  |   1 +
 drivers/media/platform/ti/omap3isp/ispvideo.c | 174 +++++++++++++-----
 9 files changed, 216 insertions(+), 59 deletions(-)
 create mode 100644 arch/arm/boot/dts/ti/omap/omap3-beagle-xm-li5m03.dtsi

-- 
2.51.0

[PATCH 01/14] media: omap3isp: configure entity functions

[Hans Verkuil]

For the various subdevices, set the function field.

This fixes v4l2-compliance errors:

$ v4l2-compliance -M0
v4l2-compliance 1.33.0-5410, 32 bits, 64-bit time_t
v4l2-compliance SHA: c12c89c5bd70 2025-10-05 09:58:42

Compliance test for omap3isp device /dev/media0:

Media Driver Info:
Driver name      : omap3isp
Model            : TI OMAP3 ISP
Serial           :
Bus info         : platform:480bc000.isp
Media version    : 6.17.0
Hardware revision: 0x000000f0 (240)
Driver version   : 6.17.0

Required ioctls:
test MEDIA_IOC_DEVICE_INFO: OK
test invalid ioctls: OK

Allow for multiple opens:
test second /dev/media0 open: OK
test MEDIA_IOC_DEVICE_INFO: OK
test for unlimited opens: OK

Media Controller ioctls:
fail: v4l2-test-media.cpp(108): function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
fail: v4l2-test-media.cpp(196): checkFunction(ent.function, true)
test MEDIA_IOC_G_TOPOLOGY: FAIL
fail: v4l2-test-media.cpp(398): num_data_links != num_links
test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
test MEDIA_IOC_SETUP_LINK: OK

Total for omap3isp device /dev/media0: 8, Succeeded: 6, Failed: 2, Warnings: 0

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispccdc.c    | 1 +
 drivers/media/platform/ti/omap3isp/ispccp2.c    | 1 +
 drivers/media/platform/ti/omap3isp/ispcsi2.c    | 1 +
 drivers/media/platform/ti/omap3isp/isppreview.c | 1 +
 drivers/media/platform/ti/omap3isp/ispresizer.c | 1 +
 drivers/media/platform/ti/omap3isp/ispstat.c    | 1 +
 6 files changed, 6 insertions(+)

diff --git a/drivers/media/platform/ti/omap3isp/ispccdc.c b/drivers/media/platform/ti/omap3isp/ispccdc.c
index 55ee14e8b449..9dbf06ac058d 100644
--- a/drivers/media/platform/ti/omap3isp/ispccdc.c
+++ b/drivers/media/platform/ti/omap3isp/ispccdc.c
@@ -2675,6 +2675,7 @@ static int ccdc_init_entities(struct isp_ccdc_device *ccdc)
 	pads[CCDC_PAD_SOURCE_OF].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &ccdc_media_ops;
+	me->function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV;
 	ret = media_entity_pads_init(me, CCDC_PADS_NUM, pads);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/media/platform/ti/omap3isp/ispccp2.c b/drivers/media/platform/ti/omap3isp/ispccp2.c
index 1204ee221c9e..f4147c79639c 100644
--- a/drivers/media/platform/ti/omap3isp/ispccp2.c
+++ b/drivers/media/platform/ti/omap3isp/ispccp2.c
@@ -1086,6 +1086,7 @@ static int ccp2_init_entities(struct isp_ccp2_device *ccp2)
 	pads[CCP2_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &ccp2_media_ops;
+	me->function = MEDIA_ENT_F_VID_IF_BRIDGE;
 	ret = media_entity_pads_init(me, CCP2_PADS_NUM, pads);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/media/platform/ti/omap3isp/ispcsi2.c b/drivers/media/platform/ti/omap3isp/ispcsi2.c
index ae574e1b6528..f227042b61b6 100644
--- a/drivers/media/platform/ti/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/ti/omap3isp/ispcsi2.c
@@ -1251,6 +1251,7 @@ static int csi2_init_entities(struct isp_csi2_device *csi2)
 				    | MEDIA_PAD_FL_MUST_CONNECT;
 
 	me->ops = &csi2_media_ops;
+	me->function = MEDIA_ENT_F_VID_IF_BRIDGE;
 	ret = media_entity_pads_init(me, CSI2_PADS_NUM, pads);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/media/platform/ti/omap3isp/isppreview.c b/drivers/media/platform/ti/omap3isp/isppreview.c
index e383a57654de..26f7167d1f4f 100644
--- a/drivers/media/platform/ti/omap3isp/isppreview.c
+++ b/drivers/media/platform/ti/omap3isp/isppreview.c
@@ -2294,6 +2294,7 @@ static int preview_init_entities(struct isp_prev_device *prev)
 	pads[PREV_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &preview_media_ops;
+	me->function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	ret = media_entity_pads_init(me, PREV_PADS_NUM, pads);
 	if (ret < 0)
 		goto error_handler_free;
diff --git a/drivers/media/platform/ti/omap3isp/ispresizer.c b/drivers/media/platform/ti/omap3isp/ispresizer.c
index 87d821b02e5c..5dff48489394 100644
--- a/drivers/media/platform/ti/omap3isp/ispresizer.c
+++ b/drivers/media/platform/ti/omap3isp/ispresizer.c
@@ -1738,6 +1738,7 @@ static int resizer_init_entities(struct isp_res_device *res)
 	pads[RESZ_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &resizer_media_ops;
+	me->function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
 	ret = media_entity_pads_init(me, RESZ_PADS_NUM, pads);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/media/platform/ti/omap3isp/ispstat.c b/drivers/media/platform/ti/omap3isp/ispstat.c
index 07bd62a93d99..64bc71d830c4 100644
--- a/drivers/media/platform/ti/omap3isp/ispstat.c
+++ b/drivers/media/platform/ti/omap3isp/ispstat.c
@@ -1037,6 +1037,7 @@ static int isp_stat_init_entities(struct ispstat *stat, const char *name,
 
 	stat->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
 	me->ops = NULL;
+	me->function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
 
 	return media_entity_pads_init(me, 1, &stat->pad);
 }
-- 
2.51.0

[PATCH 02/14] media: omap3isp: add V4L2_CAP_IO_MC and don't set bus_info

[Hans Verkuil]

Since this is a media-centric device set the V4L2_CAP_IO_MC
capability. Also don't set bus_info, leave that to the v4l2 core.

This fixes v4l2-compliance errors:

test MC information (see 'Media Driver Info' above): OK
	fail: v4l2-compliance.cpp(661): missing bus_info prefix ('media')

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 0e7f0bf2b346..46609045e2c8 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -645,11 +645,9 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
 
 	strscpy(cap->driver, ISP_VIDEO_DRIVER_NAME, sizeof(cap->driver));
 	strscpy(cap->card, video->video.name, sizeof(cap->card));
-	strscpy(cap->bus_info, "media", sizeof(cap->bus_info));
 
 	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT
-		| V4L2_CAP_STREAMING | V4L2_CAP_DEVICE_CAPS;
-
+		| V4L2_CAP_STREAMING | V4L2_CAP_DEVICE_CAPS | V4L2_CAP_IO_MC;
 
 	return 0;
 }
@@ -1451,10 +1449,10 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
 	video->video.ioctl_ops = &isp_video_ioctl_ops;
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		video->video.device_caps = V4L2_CAP_VIDEO_CAPTURE
-					 | V4L2_CAP_STREAMING;
+					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
 	else
 		video->video.device_caps = V4L2_CAP_VIDEO_OUTPUT
-					 | V4L2_CAP_STREAMING;
+					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
 
 	video->pipe.stream_state = ISP_PIPELINE_STREAM_STOPPED;
 
-- 
2.51.0

[PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Hans Verkuil]

The isp_video_mbus_to_pix/pix_to_mbus functions did not take
the last empty entry { 0, } of the formats array into account.

As a result, isp_video_mbus_to_pix would accept code 0 and
isp_video_pix_to_mbus would select code 0 if no match was found.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 46609045e2c8..864d38140b87 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
 	pix->width = mbus->width;
 	pix->height = mbus->height;
 
-	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
 		if (formats[i].code == mbus->code)
 			break;
 	}
 
-	if (WARN_ON(i == ARRAY_SIZE(formats)))
+	if (WARN_ON(i == ARRAY_SIZE(formats) - 1))
 		return 0;
 
 	min_bpl = pix->width * formats[i].bpp;
@@ -191,7 +191,7 @@ static void isp_video_pix_to_mbus(const struct v4l2_pix_format *pix,
 	/* Skip the last format in the loop so that it will be selected if no
 	 * match is found.
 	 */
-	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
+	for (i = 0; i < ARRAY_SIZE(formats) - 2; ++i) {
 		if (formats[i].pixelformat == pix->pixelformat)
 			break;
 	}
-- 
2.51.0

[PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

[Hans Verkuil]

Add missing ioctls. This makes v4l2-compliance happier:

fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
	test VIDIOC_G_FMT: FAIL
fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
	test VIDIOC_TRY_FMT: FAIL
fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
	test VIDIOC_S_FMT: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 864d38140b87..77beea00d507 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
 	return 0;
 }
 
+static int
+isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
+{
+	struct isp_video *video = video_drvdata(file);
+	unsigned int i, j;
+	unsigned int skip_last_fmts = 1;
+
+	if (f->type != video->type)
+		return -EINVAL;
+
+	/*
+	 * The last two formats have the same pixelformat as the two
+	 * formats before them, but they do have different mediabus
+	 * codes. So to avoid reporting duplicate pixelformats we skip
+	 * those two, provided f->mbus_code is 0.
+	 */
+	if (!f->mbus_code)
+		skip_last_fmts += 2;
+	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
+		if (f->mbus_code && formats[i].code != f->mbus_code)
+			continue;
+
+		if (j == f->index) {
+			f->pixelformat = formats[i].pixelformat;
+			return 0;
+		}
+		j++;
+	}
+
+	return -EINVAL;
+}
+
 static int
 isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
 {
@@ -1258,9 +1290,11 @@ isp_video_s_input(struct file *file, void *fh, unsigned int input)
 
 static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 	.vidioc_querycap		= isp_video_querycap,
+	.vidioc_enum_fmt_vid_cap	= isp_video_enum_format,
 	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
 	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
 	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
+	.vidioc_enum_fmt_vid_out	= isp_video_enum_format,
 	.vidioc_g_fmt_vid_out		= isp_video_get_format,
 	.vidioc_s_fmt_vid_out		= isp_video_set_format,
 	.vidioc_try_fmt_vid_out		= isp_video_try_format,
-- 
2.51.0

[PATCH 05/14] media: omap3isp: use V4L2_COLORSPACE_SRGB instead of _JPEG

[Hans Verkuil]

JPEG colorspace should generally not be used unless it is actually
dealing with JPG data. This fixes v4l2-compliance errors:

	fail: v4l2-test-formats.cpp(416): pixelformat != V4L2_PIX_FMT_JPEG && pixelformat != V4L2_PIX_FMT_MJPEG && colorspace == V4L2_COLORSPACE_JPEG
	fail: v4l2-test-formats.cpp(521): testColorspace(!node->is_io_mc, pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
test VIDIOC_TRY_FMT: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/isppreview.c | 2 +-
 drivers/media/platform/ti/omap3isp/ispresizer.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/isppreview.c b/drivers/media/platform/ti/omap3isp/isppreview.c
index 26f7167d1f4f..9992db782870 100644
--- a/drivers/media/platform/ti/omap3isp/isppreview.c
+++ b/drivers/media/platform/ti/omap3isp/isppreview.c
@@ -1796,7 +1796,7 @@ static void preview_try_format(struct isp_prev_device *prev,
 		fmt->width = crop->width;
 		fmt->height = crop->height;
 
-		fmt->colorspace = V4L2_COLORSPACE_JPEG;
+		fmt->colorspace = V4L2_COLORSPACE_SRGB;
 		break;
 	}
 
diff --git a/drivers/media/platform/ti/omap3isp/ispresizer.c b/drivers/media/platform/ti/omap3isp/ispresizer.c
index 5dff48489394..ad0127f5b5cb 100644
--- a/drivers/media/platform/ti/omap3isp/ispresizer.c
+++ b/drivers/media/platform/ti/omap3isp/ispresizer.c
@@ -1405,7 +1405,7 @@ static void resizer_try_format(struct isp_res_device *res,
 		break;
 	}
 
-	fmt->colorspace = V4L2_COLORSPACE_JPEG;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
 	fmt->field = V4L2_FIELD_NONE;
 }
 
-- 
2.51.0

[PATCH 06/14] media: omap3isp: set initial format

[Hans Verkuil]

Initialize the v4l2_format to a default. Empty formats are
not allowed in V4L2, so this fixes v4l2-compliance issues:

	fail: v4l2-test-formats.cpp(514): !pix.width || !pix.height
test VIDIOC_G_FMT: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 77beea00d507..d3fe28506fb0 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -1320,6 +1320,7 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 static int isp_video_open(struct file *file)
 {
 	struct isp_video *video = video_drvdata(file);
+	struct v4l2_mbus_framefmt fmt;
 	struct isp_video_fh *handle;
 	struct vb2_queue *queue;
 	int ret = 0;
@@ -1362,6 +1363,13 @@ static int isp_video_open(struct file *file)
 
 	memset(&handle->format, 0, sizeof(handle->format));
 	handle->format.type = video->type;
+	handle->format.fmt.pix.width = 720;
+	handle->format.fmt.pix.height = 480;
+	handle->format.fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
+	handle->format.fmt.pix.field = V4L2_FIELD_NONE;
+	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
+	isp_video_pix_to_mbus(&handle->format.fmt.pix, &fmt);
+	isp_video_mbus_to_pix(video, &fmt, &handle->format.fmt.pix);
 	handle->timeperframe.denominator = 1;
 
 	handle->video = video;
-- 
2.51.0

[PATCH 07/14] media: omap3isp: rework isp_video_try/set_format

[Hans Verkuil]

isp_video_set_format now calls isp_video_try_format first, ensuring
consistent behavior and removing duplicate code in both functions.

This fixes an v4l2-compliance error:

	fail: v4l2-test-formats.cpp(519): !pix.sizeimage
test VIDIOC_S_FMT: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 60 +++++++++----------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index d3fe28506fb0..69d17e4dcd36 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -701,11 +701,15 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
 }
 
 static int
-isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
+isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
 {
-	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
 	struct isp_video *video = video_drvdata(file);
-	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_subdev *subdev;
+	u32 pad;
+	int ret;
 
 	if (format->type != video->type)
 		return -EINVAL;
@@ -745,32 +749,11 @@ isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
 		break;
 	}
 
-	/* Fill the bytesperline and sizeimage fields by converting to media bus
-	 * format and back to pixel format.
-	 */
-	isp_video_pix_to_mbus(&format->fmt.pix, &fmt);
-	isp_video_mbus_to_pix(video, &fmt, &format->fmt.pix);
-
-	mutex_lock(&video->mutex);
-	vfh->format = *format;
-	mutex_unlock(&video->mutex);
-
-	return 0;
-}
-
-static int
-isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
-{
-	struct isp_video *video = video_drvdata(file);
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_subdev *subdev;
-	u32 pad;
-	int ret;
-
-	if (format->type != video->type)
-		return -EINVAL;
+	if (video->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+		isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
+		isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
+		return 0;
+	}
 
 	subdev = isp_video_remote_subdev(video, &pad);
 	if (subdev == NULL)
@@ -781,12 +764,29 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
 	fmt.pad = pad;
 	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
 	if (ret)
-		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
+		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
 
 	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
 	return 0;
 }
 
+static int
+isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
+{
+	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
+	struct isp_video *video = video_drvdata(file);
+	int ret = isp_video_try_format(file, fh, format);
+
+	if (ret)
+		return ret;
+
+	mutex_lock(&video->mutex);
+	vfh->format = *format;
+	mutex_unlock(&video->mutex);
+
+	return 0;
+}
+
 static int
 isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
 {
-- 
2.51.0

[PATCH 08/14] media: omap3isp: implement create/prepare_bufs

[Hans Verkuil]

Add missing ioctls. This makes v4l2-compliance happier:

	warn: v4l2-test-buffers.cpp(813): VIDIOC_CREATE_BUFS not supported
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 47 ++++++++++++++++++-
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 69d17e4dcd36..471defa6e7fb 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -325,6 +325,13 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
 	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
 	struct isp_video *video = vfh->video;
 
+	if (*num_planes) {
+		if (*num_planes != 1)
+			return -EINVAL;
+		if (sizes[0] < vfh->format.fmt.pix.sizeimage)
+			return -EINVAL;
+		return 0;
+	}
 	*num_planes = 1;
 
 	sizes[0] = vfh->format.fmt.pix.sizeimage;
@@ -340,6 +347,7 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(buf);
 	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
+	unsigned int size = vfh->format.fmt.pix.sizeimage;
 	struct isp_buffer *buffer = to_isp_buffer(vbuf);
 	struct isp_video *video = vfh->video;
 	dma_addr_t addr;
@@ -360,8 +368,13 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
 		return -EINVAL;
 	}
 
-	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0,
-			      vfh->format.fmt.pix.sizeimage);
+	if (vb2_plane_size(&buffer->vb.vb2_buf, 0) < size) {
+		dev_dbg(video->isp->dev,
+			"data will not fit into plane (%lu < %u)\n",
+			vb2_plane_size(&buffer->vb.vb2_buf, 0), size);
+		return -EINVAL;
+	}
+	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0, size);
 	buffer->dma = addr;
 
 	return 0;
@@ -935,6 +948,20 @@ isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
 	return ret;
 }
 
+static int
+isp_video_create_bufs(struct file *file, void *fh, struct v4l2_create_buffers *p)
+{
+	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
+	struct isp_video *video = video_drvdata(file);
+	int ret;
+
+	mutex_lock(&video->queue_lock);
+	ret = vb2_create_bufs(&vfh->queue, p);
+	mutex_unlock(&video->queue_lock);
+
+	return ret;
+}
+
 static int
 isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
 {
@@ -949,6 +976,20 @@ isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
 	return ret;
 }
 
+static int
+isp_video_prepare_buf(struct file *file, void *fh, struct v4l2_buffer *b)
+{
+	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
+	struct isp_video *video = video_drvdata(file);
+	int ret;
+
+	mutex_lock(&video->queue_lock);
+	ret = vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
+	mutex_unlock(&video->queue_lock);
+
+	return ret;
+}
+
 static int
 isp_video_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 {
@@ -1303,7 +1344,9 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 	.vidioc_g_parm			= isp_video_get_param,
 	.vidioc_s_parm			= isp_video_set_param,
 	.vidioc_reqbufs			= isp_video_reqbufs,
+	.vidioc_create_bufs		= isp_video_create_bufs,
 	.vidioc_querybuf		= isp_video_querybuf,
+	.vidioc_prepare_buf		= isp_video_prepare_buf,
 	.vidioc_qbuf			= isp_video_qbuf,
 	.vidioc_dqbuf			= isp_video_dqbuf,
 	.vidioc_streamon		= isp_video_streamon,
-- 
2.51.0

[PATCH 09/14] media: omap3isp: better VIDIOC_G/S_PARM handling

[Hans Verkuil]

Fix various v4l2-compliance errors relating to timeperframe.

VIDIOC_G/S_PARM is only supported for Video Output, so disable
these ioctls for Capture devices.

Ensure numerator and denominator are never 0.

Set missing V4L2_CAP_TIMEPERFRAME capability for VIDIOC_S_PARM.

v4l2-compliance:

	fail: v4l2-test-formats.cpp(1388): out->timeperframe.numerator == 0 || out->timeperframe.denominator == 0
test VIDIOC_G/S_PARM: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 471defa6e7fb..5603586271f5 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -928,7 +928,10 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
 
 	if (a->parm.output.timeperframe.denominator == 0)
 		a->parm.output.timeperframe.denominator = 1;
+	if (a->parm.output.timeperframe.numerator == 0)
+		a->parm.output.timeperframe.numerator = 1;
 
+	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
 	vfh->timeperframe = a->parm.output.timeperframe;
 
 	return 0;
@@ -1413,6 +1416,7 @@ static int isp_video_open(struct file *file)
 	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
 	isp_video_pix_to_mbus(&handle->format.fmt.pix, &fmt);
 	isp_video_mbus_to_pix(video, &fmt, &handle->format.fmt.pix);
+	handle->timeperframe.numerator = 1;
 	handle->timeperframe.denominator = 1;
 
 	handle->video = video;
@@ -1532,12 +1536,15 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
 	video->video.vfl_type = VFL_TYPE_VIDEO;
 	video->video.release = video_device_release_empty;
 	video->video.ioctl_ops = &isp_video_ioctl_ops;
-	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		video->video.device_caps = V4L2_CAP_VIDEO_CAPTURE
 					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
-	else
+		v4l2_disable_ioctl(&video->video, VIDIOC_S_PARM);
+		v4l2_disable_ioctl(&video->video, VIDIOC_G_PARM);
+	} else {
 		video->video.device_caps = V4L2_CAP_VIDEO_OUTPUT
 					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
+	}
 
 	video->pipe.stream_state = ISP_PIPELINE_STREAM_STOPPED;
 
-- 
2.51.0

[PATCH 10/14] media: omap3isp: support ctrl events for isppreview

[Hans Verkuil]

The preview subdev device was missing V4L2_SUBDEV_FL_HAS_EVENTS,
and that prevented VIDIOC_SUBSCRIBE_EVENT from working.

Fixes a v4l2-compliance error:

	fail: v4l2-test-controls.cpp(1128): subscribe event for control 'User Controls' failed
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/isppreview.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/omap3isp/isppreview.c b/drivers/media/platform/ti/omap3isp/isppreview.c
index 9992db782870..3e9e213c6d8a 100644
--- a/drivers/media/platform/ti/omap3isp/isppreview.c
+++ b/drivers/media/platform/ti/omap3isp/isppreview.c
@@ -2277,7 +2277,7 @@ static int preview_init_entities(struct isp_prev_device *prev)
 	strscpy(sd->name, "OMAP3 ISP preview", sizeof(sd->name));
 	sd->grp_id = 1 << 16;	/* group ID for isp subdevs */
 	v4l2_set_subdevdata(sd, prev);
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	v4l2_ctrl_handler_init(&prev->ctrls, 2);
 	v4l2_ctrl_new_std(&prev->ctrls, &preview_ctrl_ops, V4L2_CID_BRIGHTNESS,
-- 
2.51.0

[PATCH 11/14] media: omap3isp: ispccp2: always clamp in ccp2_try_format()

[Hans Verkuil]

If ccp2->input == CCP2_INPUT_NONE, then try_format didn't clamp
the width and height. This can happen with v4l2-compliance tests.

Always clamp.

This fixes this v4l2-compliance error:

	fail: v4l2-test-subdevs.cpp(171): fse.max_width == ~0U || fse.max_height == ~0U
	fail: v4l2-test-subdevs.cpp(270): ret && ret != ENOTTY
test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispccp2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispccp2.c b/drivers/media/platform/ti/omap3isp/ispccp2.c
index f4147c79639c..d668111b44f4 100644
--- a/drivers/media/platform/ti/omap3isp/ispccp2.c
+++ b/drivers/media/platform/ti/omap3isp/ispccp2.c
@@ -658,7 +658,7 @@ static void ccp2_try_format(struct isp_ccp2_device *ccp2,
 			fmt->height = clamp_t(u32, fmt->height,
 					      ISPCCP2_DAT_SIZE_MIN,
 					      ISPCCP2_DAT_SIZE_MAX);
-		} else if (ccp2->input == CCP2_INPUT_MEMORY) {
+		} else {
 			fmt->width = clamp_t(u32, fmt->width,
 					     ISPCCP2_LCM_HSIZE_COUNT_MIN,
 					     ISPCCP2_LCM_HSIZE_COUNT_MAX);
-- 
2.51.0

[PATCH 12/14] media: omap3isp: isppreview: always clamp in preview_try_format()

[Hans Verkuil]

If prev->input != PREVIEW_INPUT_MEMORY the width and height weren't
clamped. Just always clamp.

This fixes a v4l2-compliance error:

	fail: v4l2-test-subdevs.cpp(171): fse.max_width == ~0U || fse.max_height == ~0U
	fail: v4l2-test-subdevs.cpp(270): ret && ret != ENOTTY
test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: FAIL

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 .../media/platform/ti/omap3isp/isppreview.c   | 21 +++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/isppreview.c b/drivers/media/platform/ti/omap3isp/isppreview.c
index 3e9e213c6d8a..3f3b5bd9cdc7 100644
--- a/drivers/media/platform/ti/omap3isp/isppreview.c
+++ b/drivers/media/platform/ti/omap3isp/isppreview.c
@@ -1742,22 +1742,17 @@ static void preview_try_format(struct isp_prev_device *prev,
 
 	switch (pad) {
 	case PREV_PAD_SINK:
-		/* When reading data from the CCDC, the input size has already
-		 * been mangled by the CCDC output pad so it can be accepted
-		 * as-is.
-		 *
-		 * When reading data from memory, clamp the requested width and
-		 * height. The TRM doesn't specify a minimum input height, make
+		/*
+		 * Clamp the requested width and height.
+		 * The TRM doesn't specify a minimum input height, make
 		 * sure we got enough lines to enable the noise filter and color
 		 * filter array interpolation.
 		 */
-		if (prev->input == PREVIEW_INPUT_MEMORY) {
-			fmt->width = clamp_t(u32, fmt->width, PREV_MIN_IN_WIDTH,
-					     preview_max_out_width(prev));
-			fmt->height = clamp_t(u32, fmt->height,
-					      PREV_MIN_IN_HEIGHT,
-					      PREV_MAX_IN_HEIGHT);
-		}
+		fmt->width = clamp_t(u32, fmt->width, PREV_MIN_IN_WIDTH,
+				     preview_max_out_width(prev));
+		fmt->height = clamp_t(u32, fmt->height,
+				      PREV_MIN_IN_HEIGHT,
+				      PREV_MAX_IN_HEIGHT);
 
 		fmt->colorspace = V4L2_COLORSPACE_SRGB;
 
-- 
2.51.0

[PATCH 13/14] DO NOT MERGE: media: omap3isp: change default resolution to 864x648

[Hans Verkuil]

Running v4l2-compliance with the Leopard Imaging li5m03 requires
that the default resolution matches that of the camera. Change
it accordingly.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 5603586271f5..6ea79e4f39bc 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -1409,8 +1409,8 @@ static int isp_video_open(struct file *file)
 
 	memset(&handle->format, 0, sizeof(handle->format));
 	handle->format.type = video->type;
-	handle->format.fmt.pix.width = 720;
-	handle->format.fmt.pix.height = 480;
+	handle->format.fmt.pix.width = 864;
+	handle->format.fmt.pix.height = 648;
 	handle->format.fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
 	handle->format.fmt.pix.field = V4L2_FIELD_NONE;
 	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
-- 
2.51.0

[PATCH 14/14] DO NOT MERGE: omap3-beagle-xm.dts: add Leopard Imaging li5m03 support

[Hans Verkuil]

Adds support for the Leopard Imaging li5m03 camera for the Beagle xM board.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 .../dts/ti/omap/omap3-beagle-xm-li5m03.dtsi   | 64 +++++++++++++++++++
 arch/arm/boot/dts/ti/omap/omap3-beagle-xm.dts |  2 +
 2 files changed, 66 insertions(+)
 create mode 100644 arch/arm/boot/dts/ti/omap/omap3-beagle-xm-li5m03.dtsi

diff --git a/arch/arm/boot/dts/ti/omap/omap3-beagle-xm-li5m03.dtsi b/arch/arm/boot/dts/ti/omap/omap3-beagle-xm-li5m03.dtsi
new file mode 100644
index 000000000000..96d637534af0
--- /dev/null
+++ b/arch/arm/boot/dts/ti/omap/omap3-beagle-xm-li5m03.dtsi
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Device Tree Source for the Beagleboard-xM LI-5M03 add-on camera board
+ *
+ * Copyright (C) 2014 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+&i2c2 {
+	clock-frequency = <100000>;
+
+	mt9p031@48 {
+		compatible = "aptina,mt9p031";
+		reg = <0x48>;
+
+		clocks = <&isp 0>;
+		reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
+
+		vaa-supply = <&hsusb2_power>;
+		vdd-supply = <&vaux3>;
+		vdd_io-supply = <&vaux4>;
+
+		port {
+			mt9p031_out: endpoint {
+				input-clock-frequency = <21000000>;
+				pixel-clock-frequency = <48000000>;
+				remote-endpoint = <&ccdc_ep>;
+			};
+		};
+	};
+};
+
+&isp {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	ports {
+		port@0 {
+			reg = <0>;
+			ccdc_ep: endpoint {
+				remote-endpoint = <&mt9p031_out>;
+				bus-width = <12>;
+				hsync-active = <1>;
+				vsync-active = <1>;
+				pclk-sample = <0>;
+			};
+		};
+	};
+};
+
+&vaux3 {
+	regulator-name = "cam_core";
+	regulator-min-microvolt = <1800000>;
+	regulator-max-microvolt = <1800000>;
+};
+
+&vaux4 {
+	regulator-name = "cam_io";
+	regulator-min-microvolt = <1800000>;
+	regulator-max-microvolt = <1800000>;
+};
diff --git a/arch/arm/boot/dts/ti/omap/omap3-beagle-xm.dts b/arch/arm/boot/dts/ti/omap/omap3-beagle-xm.dts
index 08ee0f8ea68f..29c1864e36a0 100644
--- a/arch/arm/boot/dts/ti/omap/omap3-beagle-xm.dts
+++ b/arch/arm/boot/dts/ti/omap/omap3-beagle-xm.dts
@@ -417,3 +417,5 @@ venc_out: endpoint {
 		};
 	};
 };
+
+#include "omap3-beagle-xm-li5m03.dtsi"
-- 
2.51.0

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Sakari Ailus]

Hi Hans,

Thanks for the set.

On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
> the last empty entry { 0, } of the formats array into account.
> 
> As a result, isp_video_mbus_to_pix would accept code 0 and
> isp_video_pix_to_mbus would select code 0 if no match was found.
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 46609045e2c8..864d38140b87 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
>  	pix->width = mbus->width;
>  	pix->height = mbus->height;
>  
> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {

As it seems all users of the formats array depend on the size of the array
and not its contents, could we remove the sentinel entry from the array
instead?

>  		if (formats[i].code == mbus->code)
>  			break;
>  	}
>  
> -	if (WARN_ON(i == ARRAY_SIZE(formats)))
> +	if (WARN_ON(i == ARRAY_SIZE(formats) - 1))
>  		return 0;
>  
>  	min_bpl = pix->width * formats[i].bpp;
> @@ -191,7 +191,7 @@ static void isp_video_pix_to_mbus(const struct v4l2_pix_format *pix,
>  	/* Skip the last format in the loop so that it will be selected if no
>  	 * match is found.
>  	 */
> -	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
> +	for (i = 0; i < ARRAY_SIZE(formats) - 2; ++i) {
>  		if (formats[i].pixelformat == pix->pixelformat)
>  			break;
>  	}

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

[Sakari Ailus]

Hi Hans,

On Fri, Oct 17, 2025 at 03:26:41PM +0200, Hans Verkuil wrote:
> Add missing ioctls. This makes v4l2-compliance happier:
> 
> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
> 	test VIDIOC_G_FMT: FAIL
> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
> 	test VIDIOC_TRY_FMT: FAIL
> fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
> 	test VIDIOC_S_FMT: FAIL
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 864d38140b87..77beea00d507 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>  	return 0;
>  }
>  
> +static int
> +isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> +{
> +	struct isp_video *video = video_drvdata(file);
> +	unsigned int i, j;
> +	unsigned int skip_last_fmts = 1;
> +
> +	if (f->type != video->type)
> +		return -EINVAL;
> +
> +	/*
> +	 * The last two formats have the same pixelformat as the two
> +	 * formats before them, but they do have different mediabus
> +	 * codes. So to avoid reporting duplicate pixelformats we skip
> +	 * those two, provided f->mbus_code is 0.
> +	 */
> +	if (!f->mbus_code)
> +		skip_last_fmts += 2;
> +	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
> +		if (f->mbus_code && formats[i].code != f->mbus_code)
> +			continue;

How about, instead of the skip_last_fmts thingy, using this:

		/* Weed out pixelformats with the same mbus code. */
		if (i && formats[i - 1].code == formats[i].code)
			continue;

> +
> +		if (j == f->index) {
> +			f->pixelformat = formats[i].pixelformat;
> +			return 0;
> +		}
> +		j++;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int
>  isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
>  {
> @@ -1258,9 +1290,11 @@ isp_video_s_input(struct file *file, void *fh, unsigned int input)
>  
>  static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
>  	.vidioc_querycap		= isp_video_querycap,
> +	.vidioc_enum_fmt_vid_cap	= isp_video_enum_format,
>  	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
>  	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
>  	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
> +	.vidioc_enum_fmt_vid_out	= isp_video_enum_format,
>  	.vidioc_g_fmt_vid_out		= isp_video_get_format,
>  	.vidioc_s_fmt_vid_out		= isp_video_set_format,
>  	.vidioc_try_fmt_vid_out		= isp_video_try_format,

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 07/14] media: omap3isp: rework isp_video_try/set_format

[Sakari Ailus]

Hi Hans,

On Fri, Oct 17, 2025 at 03:26:44PM +0200, Hans Verkuil wrote:
> isp_video_set_format now calls isp_video_try_format first, ensuring
> consistent behavior and removing duplicate code in both functions.
> 
> This fixes an v4l2-compliance error:
> 
> 	fail: v4l2-test-formats.cpp(519): !pix.sizeimage
> test VIDIOC_S_FMT: FAIL
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 60 +++++++++----------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index d3fe28506fb0..69d17e4dcd36 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -701,11 +701,15 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
>  }
>  
>  static int
> -isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
> +isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>  {
> -	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>  	struct isp_video *video = video_drvdata(file);
> -	struct v4l2_mbus_framefmt fmt;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	int ret;
>  
>  	if (format->type != video->type)
>  		return -EINVAL;
> @@ -745,32 +749,11 @@ isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
>  		break;
>  	}
>  
> -	/* Fill the bytesperline and sizeimage fields by converting to media bus
> -	 * format and back to pixel format.
> -	 */
> -	isp_video_pix_to_mbus(&format->fmt.pix, &fmt);
> -	isp_video_mbus_to_pix(video, &fmt, &format->fmt.pix);
> -
> -	mutex_lock(&video->mutex);
> -	vfh->format = *format;
> -	mutex_unlock(&video->mutex);
> -
> -	return 0;
> -}
> -
> -static int
> -isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
> -{
> -	struct isp_video *video = video_drvdata(file);
> -	struct v4l2_subdev_format fmt = {
> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -	};
> -	struct v4l2_subdev *subdev;
> -	u32 pad;
> -	int ret;
> -
> -	if (format->type != video->type)
> -		return -EINVAL;
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +		isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
> +		isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
> +		return 0;
> +	}
>  
>  	subdev = isp_video_remote_subdev(video, &pad);
>  	if (subdev == NULL)
> @@ -781,12 +764,29 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>  	fmt.pad = pad;
>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>  	if (ret)
> -		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;

I believe -ENOTTY was right. Yet this should not happen (albeit I haven't
checked) as all omap3isp sub-device drivers with connected video devices
presumably implement get_fmt.

>  
>  	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
>  	return 0;
>  }
>  
> +static int
> +isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
> +{
> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> +	struct isp_video *video = video_drvdata(file);
> +	int ret = isp_video_try_format(file, fh, format);

I'd do this assignment separately: initialising variables in declaration
that requires error handling isn't very nice.

> +
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&video->mutex);
> +	vfh->format = *format;
> +	mutex_unlock(&video->mutex);
> +
> +	return 0;
> +}
> +
>  static int
>  isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
>  {

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 08/14] media: omap3isp: implement create/prepare_bufs

[Sakari Ailus]

Hi Hans,

On Fri, Oct 17, 2025 at 03:26:45PM +0200, Hans Verkuil wrote:
> Add missing ioctls. This makes v4l2-compliance happier:
> 
> 	warn: v4l2-test-buffers.cpp(813): VIDIOC_CREATE_BUFS not supported
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 47 ++++++++++++++++++-
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 69d17e4dcd36..471defa6e7fb 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -325,6 +325,13 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
>  	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
>  	struct isp_video *video = vfh->video;
>  
> +	if (*num_planes) {
> +		if (*num_planes != 1)
> +			return -EINVAL;
> +		if (sizes[0] < vfh->format.fmt.pix.sizeimage)
> +			return -EINVAL;
> +		return 0;
> +	}
>  	*num_planes = 1;
>  
>  	sizes[0] = vfh->format.fmt.pix.sizeimage;
> @@ -340,6 +347,7 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(buf);
>  	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
> +	unsigned int size = vfh->format.fmt.pix.sizeimage;
>  	struct isp_buffer *buffer = to_isp_buffer(vbuf);
>  	struct isp_video *video = vfh->video;
>  	dma_addr_t addr;
> @@ -360,8 +368,13 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
>  		return -EINVAL;
>  	}
>  
> -	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0,
> -			      vfh->format.fmt.pix.sizeimage);
> +	if (vb2_plane_size(&buffer->vb.vb2_buf, 0) < size) {
> +		dev_dbg(video->isp->dev,
> +			"data will not fit into plane (%lu < %u)\n",
> +			vb2_plane_size(&buffer->vb.vb2_buf, 0), size);
> +		return -EINVAL;
> +	}
> +	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0, size);
>  	buffer->dma = addr;
>  
>  	return 0;
> @@ -935,6 +948,20 @@ isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
>  	return ret;
>  }
>  
> +static int
> +isp_video_create_bufs(struct file *file, void *fh, struct v4l2_create_buffers *p)
> +{
> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> +	struct isp_video *video = video_drvdata(file);
> +	int ret;
> +
> +	mutex_lock(&video->queue_lock);
> +	ret = vb2_create_bufs(&vfh->queue, p);
> +	mutex_unlock(&video->queue_lock);
> +
> +	return ret;
> +}
> +
>  static int
>  isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
>  {
> @@ -949,6 +976,20 @@ isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
>  	return ret;
>  }
>  
> +static int
> +isp_video_prepare_buf(struct file *file, void *fh, struct v4l2_buffer *b)
> +{
> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> +	struct isp_video *video = video_drvdata(file);
> +	int ret;
> +
> +	mutex_lock(&video->queue_lock);
> +	ret = vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
> +	mutex_unlock(&video->queue_lock);
> +
> +	return ret;

Also:

#include <linux/cleanup.h>

...

	guard(mutex)(&video->queue_lock);

	return vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);

Same for create_bufs implementation.


> +}
> +
>  static int
>  isp_video_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
>  {
> @@ -1303,7 +1344,9 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
>  	.vidioc_g_parm			= isp_video_get_param,
>  	.vidioc_s_parm			= isp_video_set_param,
>  	.vidioc_reqbufs			= isp_video_reqbufs,
> +	.vidioc_create_bufs		= isp_video_create_bufs,
>  	.vidioc_querybuf		= isp_video_querybuf,
> +	.vidioc_prepare_buf		= isp_video_prepare_buf,
>  	.vidioc_qbuf			= isp_video_qbuf,
>  	.vidioc_dqbuf			= isp_video_dqbuf,
>  	.vidioc_streamon		= isp_video_streamon,

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 09/14] media: omap3isp: better VIDIOC_G/S_PARM handling

[Sakari Ailus]

Hi Hans,

On Fri, Oct 17, 2025 at 03:26:46PM +0200, Hans Verkuil wrote:
> Fix various v4l2-compliance errors relating to timeperframe.
> 
> VIDIOC_G/S_PARM is only supported for Video Output, so disable
> these ioctls for Capture devices.
> 
> Ensure numerator and denominator are never 0.
> 
> Set missing V4L2_CAP_TIMEPERFRAME capability for VIDIOC_S_PARM.
> 
> v4l2-compliance:
> 
> 	fail: v4l2-test-formats.cpp(1388): out->timeperframe.numerator == 0 || out->timeperframe.denominator == 0
> test VIDIOC_G/S_PARM: FAIL
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 471defa6e7fb..5603586271f5 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -928,7 +928,10 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
>  
>  	if (a->parm.output.timeperframe.denominator == 0)
>  		a->parm.output.timeperframe.denominator = 1;
> +	if (a->parm.output.timeperframe.numerator == 0)
> +		a->parm.output.timeperframe.numerator = 1;

I believe S_PARM support has probably been added for v4l2-compliance in the
past. Should there be either a dummy implementation for more or less all
Media device centric drivers or could this be simply omitted?

>  
> +	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>  	vfh->timeperframe = a->parm.output.timeperframe;
>  
>  	return 0;
> @@ -1413,6 +1416,7 @@ static int isp_video_open(struct file *file)
>  	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>  	isp_video_pix_to_mbus(&handle->format.fmt.pix, &fmt);
>  	isp_video_mbus_to_pix(video, &fmt, &handle->format.fmt.pix);
> +	handle->timeperframe.numerator = 1;
>  	handle->timeperframe.denominator = 1;
>  
>  	handle->video = video;
> @@ -1532,12 +1536,15 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
>  	video->video.vfl_type = VFL_TYPE_VIDEO;
>  	video->video.release = video_device_release_empty;
>  	video->video.ioctl_ops = &isp_video_ioctl_ops;
> -	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>  		video->video.device_caps = V4L2_CAP_VIDEO_CAPTURE
>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> -	else
> +		v4l2_disable_ioctl(&video->video, VIDIOC_S_PARM);
> +		v4l2_disable_ioctl(&video->video, VIDIOC_G_PARM);
> +	} else {
>  		video->video.device_caps = V4L2_CAP_VIDEO_OUTPUT
>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> +	}
>  
>  	video->pipe.stream_state = ISP_PIPELINE_STREAM_STOPPED;
>  

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Hans Verkuil]

Hi Sakari,

On 22/10/2025 09:48, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the set.
> 
> On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
>> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
>> the last empty entry { 0, } of the formats array into account.
>>
>> As a result, isp_video_mbus_to_pix would accept code 0 and
>> isp_video_pix_to_mbus would select code 0 if no match was found.
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 46609045e2c8..864d38140b87 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
>>  	pix->width = mbus->width;
>>  	pix->height = mbus->height;
>>  
>> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
> 
> As it seems all users of the formats array depend on the size of the array
> and not its contents, could we remove the sentinel entry from the array
> instead?

Probably, but see this comment just before the sentinel in the array:

        /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
         * module and avoid NULL pointer dereferences.
         */
        { 0, }

Now, I wonder if this comment is out of date, since I don't see code 0 being used
by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.

Regards,

	Hans

> 
>>  		if (formats[i].code == mbus->code)
>>  			break;
>>  	}
>>  
>> -	if (WARN_ON(i == ARRAY_SIZE(formats)))
>> +	if (WARN_ON(i == ARRAY_SIZE(formats) - 1))
>>  		return 0;
>>  
>>  	min_bpl = pix->width * formats[i].bpp;
>> @@ -191,7 +191,7 @@ static void isp_video_pix_to_mbus(const struct v4l2_pix_format *pix,
>>  	/* Skip the last format in the loop so that it will be selected if no
>>  	 * match is found.
>>  	 */
>> -	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
>> +	for (i = 0; i < ARRAY_SIZE(formats) - 2; ++i) {
>>  		if (formats[i].pixelformat == pix->pixelformat)
>>  			break;
>>  	}
> 

Re: [PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

[Hans Verkuil]

On 22/10/2025 09:56, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Oct 17, 2025 at 03:26:41PM +0200, Hans Verkuil wrote:
>> Add missing ioctls. This makes v4l2-compliance happier:
>>
>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
>> 	test VIDIOC_G_FMT: FAIL
>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
>> 	test VIDIOC_TRY_FMT: FAIL
>> fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
>> 	test VIDIOC_S_FMT: FAIL
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 864d38140b87..77beea00d507 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>>  	return 0;
>>  }
>>  
>> +static int
>> +isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>> +{
>> +	struct isp_video *video = video_drvdata(file);
>> +	unsigned int i, j;
>> +	unsigned int skip_last_fmts = 1;
>> +
>> +	if (f->type != video->type)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * The last two formats have the same pixelformat as the two
>> +	 * formats before them, but they do have different mediabus
>> +	 * codes. So to avoid reporting duplicate pixelformats we skip
>> +	 * those two, provided f->mbus_code is 0.
>> +	 */
>> +	if (!f->mbus_code)
>> +		skip_last_fmts += 2;
>> +	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
>> +		if (f->mbus_code && formats[i].code != f->mbus_code)
>> +			continue;
> 
> How about, instead of the skip_last_fmts thingy, using this:
> 
> 		/* Weed out pixelformats with the same mbus code. */
> 		if (i && formats[i - 1].code == formats[i].code)
> 			continue;

Good idea, but it should be this:

               /* Weed out duplicate pixelformats with different mbus codes */
               if (!f->mbus_code && i &&
                   formats[i - 1].pixelformat == formats[i].pixelformat)
                       continue;

And the duplicate pixelformats in the formats array must be together in the
array for this to work. Easy enough to change.

Regards,

	Hans

> 
>> +
>> +		if (j == f->index) {
>> +			f->pixelformat = formats[i].pixelformat;
>> +			return 0;
>> +		}
>> +		j++;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static int
>>  isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
>>  {
>> @@ -1258,9 +1290,11 @@ isp_video_s_input(struct file *file, void *fh, unsigned int input)
>>  
>>  static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
>>  	.vidioc_querycap		= isp_video_querycap,
>> +	.vidioc_enum_fmt_vid_cap	= isp_video_enum_format,
>>  	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
>>  	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
>>  	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
>> +	.vidioc_enum_fmt_vid_out	= isp_video_enum_format,
>>  	.vidioc_g_fmt_vid_out		= isp_video_get_format,
>>  	.vidioc_s_fmt_vid_out		= isp_video_set_format,
>>  	.vidioc_try_fmt_vid_out		= isp_video_try_format,
> 

Re: [PATCH 07/14] media: omap3isp: rework isp_video_try/set_format

[Hans Verkuil]

On 22/10/2025 10:04, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Oct 17, 2025 at 03:26:44PM +0200, Hans Verkuil wrote:
>> isp_video_set_format now calls isp_video_try_format first, ensuring
>> consistent behavior and removing duplicate code in both functions.
>>
>> This fixes an v4l2-compliance error:
>>
>> 	fail: v4l2-test-formats.cpp(519): !pix.sizeimage
>> test VIDIOC_S_FMT: FAIL
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 60 +++++++++----------
>>  1 file changed, 30 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index d3fe28506fb0..69d17e4dcd36 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -701,11 +701,15 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
>>  }
>>  
>>  static int
>> -isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
>> +isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>>  {
>> -	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>>  	struct isp_video *video = video_drvdata(file);
>> -	struct v4l2_mbus_framefmt fmt;
>> +	struct v4l2_subdev_format fmt = {
>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> +	};
>> +	struct v4l2_subdev *subdev;
>> +	u32 pad;
>> +	int ret;
>>  
>>  	if (format->type != video->type)
>>  		return -EINVAL;
>> @@ -745,32 +749,11 @@ isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
>>  		break;
>>  	}
>>  
>> -	/* Fill the bytesperline and sizeimage fields by converting to media bus
>> -	 * format and back to pixel format.
>> -	 */
>> -	isp_video_pix_to_mbus(&format->fmt.pix, &fmt);
>> -	isp_video_mbus_to_pix(video, &fmt, &format->fmt.pix);
>> -
>> -	mutex_lock(&video->mutex);
>> -	vfh->format = *format;
>> -	mutex_unlock(&video->mutex);
>> -
>> -	return 0;
>> -}
>> -
>> -static int
>> -isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>> -{
>> -	struct isp_video *video = video_drvdata(file);
>> -	struct v4l2_subdev_format fmt = {
>> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> -	};
>> -	struct v4l2_subdev *subdev;
>> -	u32 pad;
>> -	int ret;
>> -
>> -	if (format->type != video->type)
>> -		return -EINVAL;
>> +	if (video->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>> +		isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
>> +		isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
>> +		return 0;
>> +	}
>>  
>>  	subdev = isp_video_remote_subdev(video, &pad);
>>  	if (subdev == NULL)
>> @@ -781,12 +764,29 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>>  	fmt.pad = pad;
>>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>  	if (ret)
>> -		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
>> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> 
> I believe -ENOTTY was right. Yet this should not happen (albeit I haven't
> checked) as all omap3isp sub-device drivers with connected video devices
> presumably implement get_fmt.

If this returns -ENOTTY, then that indicates that the TRY_FMT ioctl is not supported.
But that's not the case, it is, but something else is wrong.

Given the fact that this function also returns -EINVAL if subdev == NULL,
I don't think returning -ENOTTY is the right thing to do.

An alternative to -EINVAL might be to return -ENODEV if subdev == NULL or the
get_fmt op is not present.

> 
>>  
>>  	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
>>  	return 0;
>>  }
>>  
>> +static int
>> +isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
>> +{
>> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>> +	struct isp_video *video = video_drvdata(file);
>> +	int ret = isp_video_try_format(file, fh, format);
> 
> I'd do this assignment separately: initialising variables in declaration
> that requires error handling isn't very nice.

OK.

Regards,

	Hans

> 
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_lock(&video->mutex);
>> +	vfh->format = *format;
>> +	mutex_unlock(&video->mutex);
>> +
>> +	return 0;
>> +}
>> +
>>  static int
>>  isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
>>  {
> 

Re: [PATCH 08/14] media: omap3isp: implement create/prepare_bufs

[Hans Verkuil]

On 22/10/2025 10:06, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Oct 17, 2025 at 03:26:45PM +0200, Hans Verkuil wrote:
>> Add missing ioctls. This makes v4l2-compliance happier:
>>
>> 	warn: v4l2-test-buffers.cpp(813): VIDIOC_CREATE_BUFS not supported
>> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 47 ++++++++++++++++++-
>>  1 file changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 69d17e4dcd36..471defa6e7fb 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -325,6 +325,13 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
>>  	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
>>  	struct isp_video *video = vfh->video;
>>  
>> +	if (*num_planes) {
>> +		if (*num_planes != 1)
>> +			return -EINVAL;
>> +		if (sizes[0] < vfh->format.fmt.pix.sizeimage)
>> +			return -EINVAL;
>> +		return 0;
>> +	}
>>  	*num_planes = 1;
>>  
>>  	sizes[0] = vfh->format.fmt.pix.sizeimage;
>> @@ -340,6 +347,7 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
>>  {
>>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(buf);
>>  	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
>> +	unsigned int size = vfh->format.fmt.pix.sizeimage;
>>  	struct isp_buffer *buffer = to_isp_buffer(vbuf);
>>  	struct isp_video *video = vfh->video;
>>  	dma_addr_t addr;
>> @@ -360,8 +368,13 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
>>  		return -EINVAL;
>>  	}
>>  
>> -	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0,
>> -			      vfh->format.fmt.pix.sizeimage);
>> +	if (vb2_plane_size(&buffer->vb.vb2_buf, 0) < size) {
>> +		dev_dbg(video->isp->dev,
>> +			"data will not fit into plane (%lu < %u)\n",
>> +			vb2_plane_size(&buffer->vb.vb2_buf, 0), size);
>> +		return -EINVAL;
>> +	}
>> +	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0, size);
>>  	buffer->dma = addr;
>>  
>>  	return 0;
>> @@ -935,6 +948,20 @@ isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
>>  	return ret;
>>  }
>>  
>> +static int
>> +isp_video_create_bufs(struct file *file, void *fh, struct v4l2_create_buffers *p)
>> +{
>> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>> +	struct isp_video *video = video_drvdata(file);
>> +	int ret;
>> +
>> +	mutex_lock(&video->queue_lock);
>> +	ret = vb2_create_bufs(&vfh->queue, p);
>> +	mutex_unlock(&video->queue_lock);
>> +
>> +	return ret;
>> +}
>> +
>>  static int
>>  isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
>>  {
>> @@ -949,6 +976,20 @@ isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
>>  	return ret;
>>  }
>>  
>> +static int
>> +isp_video_prepare_buf(struct file *file, void *fh, struct v4l2_buffer *b)
>> +{
>> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>> +	struct isp_video *video = video_drvdata(file);
>> +	int ret;
>> +
>> +	mutex_lock(&video->queue_lock);
>> +	ret = vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
>> +	mutex_unlock(&video->queue_lock);
>> +
>> +	return ret;
> 
> Also:
> 
> #include <linux/cleanup.h>
> 
> ...
> 
> 	guard(mutex)(&video->queue_lock);
> 
> 	return vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
> 
> Same for create_bufs implementation.

Hmm, I'm hesitant to do that for two reasons:

1) Consistency with existing vb2 callbacks
2) It's harder to backport this to older kernels.

Since this is an old driver, the chances someone might want to backport this
to an old kernel is higher than usual.

If you really want this, then I prefer to make a new patch that introduces
the guards for all vb2 callbacks in this driver.

Regards,

	Hans

> 
> 
>> +}
>> +
>>  static int
>>  isp_video_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
>>  {
>> @@ -1303,7 +1344,9 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
>>  	.vidioc_g_parm			= isp_video_get_param,
>>  	.vidioc_s_parm			= isp_video_set_param,
>>  	.vidioc_reqbufs			= isp_video_reqbufs,
>> +	.vidioc_create_bufs		= isp_video_create_bufs,
>>  	.vidioc_querybuf		= isp_video_querybuf,
>> +	.vidioc_prepare_buf		= isp_video_prepare_buf,
>>  	.vidioc_qbuf			= isp_video_qbuf,
>>  	.vidioc_dqbuf			= isp_video_dqbuf,
>>  	.vidioc_streamon		= isp_video_streamon,
> 

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 09:27:55AM +0100, Hans Verkuil wrote:
> Hi Sakari,
> 
> On 22/10/2025 09:48, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks for the set.
> > 
> > On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
> >> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
> >> the last empty entry { 0, } of the formats array into account.
> >>
> >> As a result, isp_video_mbus_to_pix would accept code 0 and
> >> isp_video_pix_to_mbus would select code 0 if no match was found.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >> ---
> >>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> index 46609045e2c8..864d38140b87 100644
> >> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
> >>  	pix->width = mbus->width;
> >>  	pix->height = mbus->height;
> >>  
> >> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> >> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
> > 
> > As it seems all users of the formats array depend on the size of the array
> > and not its contents, could we remove the sentinel entry from the array
> > instead?
> 
> Probably, but see this comment just before the sentinel in the array:
> 
>         /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
>          * module and avoid NULL pointer dereferences.
>          */
>         { 0, }
> 
> Now, I wonder if this comment is out of date, since I don't see code 0 being used
> by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.

Yes, please!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 09/14] media: omap3isp: better VIDIOC_G/S_PARM handling

[Hans Verkuil]

On 22/10/2025 10:09, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Oct 17, 2025 at 03:26:46PM +0200, Hans Verkuil wrote:
>> Fix various v4l2-compliance errors relating to timeperframe.
>>
>> VIDIOC_G/S_PARM is only supported for Video Output, so disable
>> these ioctls for Capture devices.
>>
>> Ensure numerator and denominator are never 0.
>>
>> Set missing V4L2_CAP_TIMEPERFRAME capability for VIDIOC_S_PARM.
>>
>> v4l2-compliance:
>>
>> 	fail: v4l2-test-formats.cpp(1388): out->timeperframe.numerator == 0 || out->timeperframe.denominator == 0
>> test VIDIOC_G/S_PARM: FAIL
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 471defa6e7fb..5603586271f5 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -928,7 +928,10 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
>>  
>>  	if (a->parm.output.timeperframe.denominator == 0)
>>  		a->parm.output.timeperframe.denominator = 1;
>> +	if (a->parm.output.timeperframe.numerator == 0)
>> +		a->parm.output.timeperframe.numerator = 1;
> 
> I believe S_PARM support has probably been added for v4l2-compliance in the
> past. Should there be either a dummy implementation for more or less all
> Media device centric drivers or could this be simply omitted?

v4l2-compliance has seen quite a few changes w.r.t. the G/S_PARM tests,
and I think it is fine to just drop support for these ioctls.

I'll test this a bit more, and if I don't find any issues, then I'll
just remove support for these ioctls in omap3isp.

Regards,

	Hans

> 
>>  
>> +	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>>  	vfh->timeperframe = a->parm.output.timeperframe;
>>  
>>  	return 0;
>> @@ -1413,6 +1416,7 @@ static int isp_video_open(struct file *file)
>>  	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>>  	isp_video_pix_to_mbus(&handle->format.fmt.pix, &fmt);
>>  	isp_video_mbus_to_pix(video, &fmt, &handle->format.fmt.pix);
>> +	handle->timeperframe.numerator = 1;
>>  	handle->timeperframe.denominator = 1;
>>  
>>  	handle->video = video;
>> @@ -1532,12 +1536,15 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
>>  	video->video.vfl_type = VFL_TYPE_VIDEO;
>>  	video->video.release = video_device_release_empty;
>>  	video->video.ioctl_ops = &isp_video_ioctl_ops;
>> -	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>>  		video->video.device_caps = V4L2_CAP_VIDEO_CAPTURE
>>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>> -	else
>> +		v4l2_disable_ioctl(&video->video, VIDIOC_S_PARM);
>> +		v4l2_disable_ioctl(&video->video, VIDIOC_G_PARM);
>> +	} else {
>>  		video->video.device_caps = V4L2_CAP_VIDEO_OUTPUT
>>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>> +	}
>>  
>>  	video->pipe.stream_state = ISP_PIPELINE_STREAM_STOPPED;
>>  
> 

Re: [PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 09:40:58AM +0100, Hans Verkuil wrote:
> On 22/10/2025 09:56, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Oct 17, 2025 at 03:26:41PM +0200, Hans Verkuil wrote:
> >> Add missing ioctls. This makes v4l2-compliance happier:
> >>
> >> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
> >> 	test VIDIOC_G_FMT: FAIL
> >> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
> >> 	test VIDIOC_TRY_FMT: FAIL
> >> fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
> >> 	test VIDIOC_S_FMT: FAIL
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >> ---
> >>  drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> index 864d38140b87..77beea00d507 100644
> >> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> @@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
> >>  	return 0;
> >>  }
> >>  
> >> +static int
> >> +isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> >> +{
> >> +	struct isp_video *video = video_drvdata(file);
> >> +	unsigned int i, j;
> >> +	unsigned int skip_last_fmts = 1;
> >> +
> >> +	if (f->type != video->type)
> >> +		return -EINVAL;
> >> +
> >> +	/*
> >> +	 * The last two formats have the same pixelformat as the two
> >> +	 * formats before them, but they do have different mediabus
> >> +	 * codes. So to avoid reporting duplicate pixelformats we skip
> >> +	 * those two, provided f->mbus_code is 0.
> >> +	 */
> >> +	if (!f->mbus_code)
> >> +		skip_last_fmts += 2;
> >> +	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
> >> +		if (f->mbus_code && formats[i].code != f->mbus_code)
> >> +			continue;
> > 
> > How about, instead of the skip_last_fmts thingy, using this:
> > 
> > 		/* Weed out pixelformats with the same mbus code. */
> > 		if (i && formats[i - 1].code == formats[i].code)
> > 			continue;
> 
> Good idea, but it should be this:
> 
>                /* Weed out duplicate pixelformats with different mbus codes */
>                if (!f->mbus_code && i &&
>                    formats[i - 1].pixelformat == formats[i].pixelformat)
>                        continue;

I think you shouldn't add !f->mbus_code check here, there's already a check
for that right after the for ... line.

> 
> And the duplicate pixelformats in the formats array must be together in the
> array for this to work. Easy enough to change.

The pixelformats in formats array would need to be sorted for this -- right
now they're not (16- vs. 8-bit YUV formats at the end of the table). So I
think this should be:

		if (f->mbus_code && formats[i].code != f->mbus_code)
			continue;

		/* Weed up duplicate pixelformats. */
		if (i && formats[i - 1].pixelformat == formats[i].pixelformat)
			continue;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 07/14] media: omap3isp: rework isp_video_try/set_format

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 09:48:42AM +0100, Hans Verkuil wrote:
> On 22/10/2025 10:04, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Oct 17, 2025 at 03:26:44PM +0200, Hans Verkuil wrote:
> >> isp_video_set_format now calls isp_video_try_format first, ensuring
> >> consistent behavior and removing duplicate code in both functions.
> >>
> >> This fixes an v4l2-compliance error:
> >>
> >> 	fail: v4l2-test-formats.cpp(519): !pix.sizeimage
> >> test VIDIOC_S_FMT: FAIL
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >> ---
> >>  drivers/media/platform/ti/omap3isp/ispvideo.c | 60 +++++++++----------
> >>  1 file changed, 30 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> index d3fe28506fb0..69d17e4dcd36 100644
> >> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> @@ -701,11 +701,15 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
> >>  }
> >>  
> >>  static int
> >> -isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
> >> +isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
> >>  {
> >> -	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> >>  	struct isp_video *video = video_drvdata(file);
> >> -	struct v4l2_mbus_framefmt fmt;
> >> +	struct v4l2_subdev_format fmt = {
> >> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >> +	};
> >> +	struct v4l2_subdev *subdev;
> >> +	u32 pad;
> >> +	int ret;
> >>  
> >>  	if (format->type != video->type)
> >>  		return -EINVAL;
> >> @@ -745,32 +749,11 @@ isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
> >>  		break;
> >>  	}
> >>  
> >> -	/* Fill the bytesperline and sizeimage fields by converting to media bus
> >> -	 * format and back to pixel format.
> >> -	 */
> >> -	isp_video_pix_to_mbus(&format->fmt.pix, &fmt);
> >> -	isp_video_mbus_to_pix(video, &fmt, &format->fmt.pix);
> >> -
> >> -	mutex_lock(&video->mutex);
> >> -	vfh->format = *format;
> >> -	mutex_unlock(&video->mutex);
> >> -
> >> -	return 0;
> >> -}
> >> -
> >> -static int
> >> -isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
> >> -{
> >> -	struct isp_video *video = video_drvdata(file);
> >> -	struct v4l2_subdev_format fmt = {
> >> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >> -	};
> >> -	struct v4l2_subdev *subdev;
> >> -	u32 pad;
> >> -	int ret;
> >> -
> >> -	if (format->type != video->type)
> >> -		return -EINVAL;
> >> +	if (video->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> >> +		isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
> >> +		isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
> >> +		return 0;
> >> +	}
> >>  
> >>  	subdev = isp_video_remote_subdev(video, &pad);
> >>  	if (subdev == NULL)
> >> @@ -781,12 +764,29 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
> >>  	fmt.pad = pad;
> >>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> >>  	if (ret)
> >> -		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
> >> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> > 
> > I believe -ENOTTY was right. Yet this should not happen (albeit I haven't
> > checked) as all omap3isp sub-device drivers with connected video devices
> > presumably implement get_fmt.
> 
> If this returns -ENOTTY, then that indicates that the TRY_FMT ioctl is not supported.
> But that's not the case, it is, but something else is wrong.

The try_format sub-device pad op is used as a backend here. If that's not
implemented, I'd use -ENOTTY to signal that. But as I said I don't think
this happens. So why change it to something that's more opaque?

> 
> Given the fact that this function also returns -EINVAL if subdev == NULL,
> I don't think returning -ENOTTY is the right thing to do.

I don't think that happens either in practice: it'd require a driver bug
for that to happen.

> 
> An alternative to -EINVAL might be to return -ENODEV if subdev == NULL or the
> get_fmt op is not present.

Sounds good.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 08/14] media: omap3isp: implement create/prepare_bufs

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 09:54:50AM +0100, Hans Verkuil wrote:
> On 22/10/2025 10:06, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Oct 17, 2025 at 03:26:45PM +0200, Hans Verkuil wrote:
> >> Add missing ioctls. This makes v4l2-compliance happier:
> >>
> >> 	warn: v4l2-test-buffers.cpp(813): VIDIOC_CREATE_BUFS not supported
> >> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >> ---
> >>  drivers/media/platform/ti/omap3isp/ispvideo.c | 47 ++++++++++++++++++-
> >>  1 file changed, 45 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> index 69d17e4dcd36..471defa6e7fb 100644
> >> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >> @@ -325,6 +325,13 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
> >>  	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
> >>  	struct isp_video *video = vfh->video;
> >>  
> >> +	if (*num_planes) {
> >> +		if (*num_planes != 1)
> >> +			return -EINVAL;
> >> +		if (sizes[0] < vfh->format.fmt.pix.sizeimage)
> >> +			return -EINVAL;
> >> +		return 0;
> >> +	}
> >>  	*num_planes = 1;
> >>  
> >>  	sizes[0] = vfh->format.fmt.pix.sizeimage;
> >> @@ -340,6 +347,7 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
> >>  {
> >>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(buf);
> >>  	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
> >> +	unsigned int size = vfh->format.fmt.pix.sizeimage;
> >>  	struct isp_buffer *buffer = to_isp_buffer(vbuf);
> >>  	struct isp_video *video = vfh->video;
> >>  	dma_addr_t addr;
> >> @@ -360,8 +368,13 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0,
> >> -			      vfh->format.fmt.pix.sizeimage);
> >> +	if (vb2_plane_size(&buffer->vb.vb2_buf, 0) < size) {
> >> +		dev_dbg(video->isp->dev,
> >> +			"data will not fit into plane (%lu < %u)\n",
> >> +			vb2_plane_size(&buffer->vb.vb2_buf, 0), size);
> >> +		return -EINVAL;
> >> +	}
> >> +	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0, size);
> >>  	buffer->dma = addr;
> >>  
> >>  	return 0;
> >> @@ -935,6 +948,20 @@ isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
> >>  	return ret;
> >>  }
> >>  
> >> +static int
> >> +isp_video_create_bufs(struct file *file, void *fh, struct v4l2_create_buffers *p)
> >> +{
> >> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> >> +	struct isp_video *video = video_drvdata(file);
> >> +	int ret;
> >> +
> >> +	mutex_lock(&video->queue_lock);
> >> +	ret = vb2_create_bufs(&vfh->queue, p);
> >> +	mutex_unlock(&video->queue_lock);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static int
> >>  isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
> >>  {
> >> @@ -949,6 +976,20 @@ isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
> >>  	return ret;
> >>  }
> >>  
> >> +static int
> >> +isp_video_prepare_buf(struct file *file, void *fh, struct v4l2_buffer *b)
> >> +{
> >> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> >> +	struct isp_video *video = video_drvdata(file);
> >> +	int ret;
> >> +
> >> +	mutex_lock(&video->queue_lock);
> >> +	ret = vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
> >> +	mutex_unlock(&video->queue_lock);
> >> +
> >> +	return ret;
> > 
> > Also:
> > 
> > #include <linux/cleanup.h>
> > 
> > ...
> > 
> > 	guard(mutex)(&video->queue_lock);
> > 
> > 	return vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
> > 
> > Same for create_bufs implementation.
> 
> Hmm, I'm hesitant to do that for two reasons:
> 
> 1) Consistency with existing vb2 callbacks
> 2) It's harder to backport this to older kernels.
> 
> Since this is an old driver, the chances someone might want to backport this
> to an old kernel is higher than usual.
> 
> If you really want this, then I prefer to make a new patch that introduces
> the guards for all vb2 callbacks in this driver.

Up to you.

-- 
Regards,

Sakari Ailus

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Sakari Ailus]

On Mon, Dec 01, 2025 at 12:22:12PM +0200, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Dec 01, 2025 at 09:27:55AM +0100, Hans Verkuil wrote:
> > Hi Sakari,
> > 
> > On 22/10/2025 09:48, Sakari Ailus wrote:
> > > Hi Hans,
> > > 
> > > Thanks for the set.
> > > 
> > > On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
> > >> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
> > >> the last empty entry { 0, } of the formats array into account.
> > >>
> > >> As a result, isp_video_mbus_to_pix would accept code 0 and
> > >> isp_video_pix_to_mbus would select code 0 if no match was found.
> > >>
> > >> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> > >> ---
> > >>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
> > >>  1 file changed, 3 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> > >> index 46609045e2c8..864d38140b87 100644
> > >> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> > >> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> > >> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
> > >>  	pix->width = mbus->width;
> > >>  	pix->height = mbus->height;
> > >>  
> > >> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> > >> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
> > > 
> > > As it seems all users of the formats array depend on the size of the array
> > > and not its contents, could we remove the sentinel entry from the array
> > > instead?
> > 
> > Probably, but see this comment just before the sentinel in the array:
> > 
> >         /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
> >          * module and avoid NULL pointer dereferences.
> >          */
> >         { 0, }
> > 
> > Now, I wonder if this comment is out of date, since I don't see code 0 being used
> > by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.
> 
> Yes, please!

Actually it's omap3isp_video_format_info() I understand ispccdc.c relies
not to return NULL. I might add a separate variable for that, to get rid of
this obscure arrangement.

You could also add:

Fixes: c51364cafa26 ("[media] omap3isp: ccdc: Add YUV input formats support")

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 09/14] media: omap3isp: better VIDIOC_G/S_PARM handling

[Hans Verkuil]

On 01/12/2025 11:28, Hans Verkuil wrote:
> On 22/10/2025 10:09, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Fri, Oct 17, 2025 at 03:26:46PM +0200, Hans Verkuil wrote:
>>> Fix various v4l2-compliance errors relating to timeperframe.
>>>
>>> VIDIOC_G/S_PARM is only supported for Video Output, so disable
>>> these ioctls for Capture devices.
>>>
>>> Ensure numerator and denominator are never 0.
>>>
>>> Set missing V4L2_CAP_TIMEPERFRAME capability for VIDIOC_S_PARM.
>>>
>>> v4l2-compliance:
>>>
>>> 	fail: v4l2-test-formats.cpp(1388): out->timeperframe.numerator == 0 || out->timeperframe.denominator == 0
>>> test VIDIOC_G/S_PARM: FAIL
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>>> ---
>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>> index 471defa6e7fb..5603586271f5 100644
>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>> @@ -928,7 +928,10 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
>>>  
>>>  	if (a->parm.output.timeperframe.denominator == 0)
>>>  		a->parm.output.timeperframe.denominator = 1;
>>> +	if (a->parm.output.timeperframe.numerator == 0)
>>> +		a->parm.output.timeperframe.numerator = 1;
>>
>> I believe S_PARM support has probably been added for v4l2-compliance in the
>> past. Should there be either a dummy implementation for more or less all
>> Media device centric drivers or could this be simply omitted?
> 
> v4l2-compliance has seen quite a few changes w.r.t. the G/S_PARM tests,
> and I think it is fine to just drop support for these ioctls.
> 
> I'll test this a bit more, and if I don't find any issues, then I'll
> just remove support for these ioctls in omap3isp.

Actually, S_PARM is used to set the max framerate for the output of the omap3isp.

So it is in use.

I'm keeping this patch.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>>>  
>>> +	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>>>  	vfh->timeperframe = a->parm.output.timeperframe;
>>>  
>>>  	return 0;
>>> @@ -1413,6 +1416,7 @@ static int isp_video_open(struct file *file)
>>>  	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>>>  	isp_video_pix_to_mbus(&handle->format.fmt.pix, &fmt);
>>>  	isp_video_mbus_to_pix(video, &fmt, &handle->format.fmt.pix);
>>> +	handle->timeperframe.numerator = 1;
>>>  	handle->timeperframe.denominator = 1;
>>>  
>>>  	handle->video = video;
>>> @@ -1532,12 +1536,15 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
>>>  	video->video.vfl_type = VFL_TYPE_VIDEO;
>>>  	video->video.release = video_device_release_empty;
>>>  	video->video.ioctl_ops = &isp_video_ioctl_ops;
>>> -	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>>>  		video->video.device_caps = V4L2_CAP_VIDEO_CAPTURE
>>>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>>> -	else
>>> +		v4l2_disable_ioctl(&video->video, VIDIOC_S_PARM);
>>> +		v4l2_disable_ioctl(&video->video, VIDIOC_G_PARM);
>>> +	} else {
>>>  		video->video.device_caps = V4L2_CAP_VIDEO_OUTPUT
>>>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>>> +	}
>>>  
>>>  	video->pipe.stream_state = ISP_PIPELINE_STREAM_STOPPED;
>>>  
>>
> 
> 

Re: [PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

[Hans Verkuil]

On 01/12/2025 12:35, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Dec 01, 2025 at 09:40:58AM +0100, Hans Verkuil wrote:
>> On 22/10/2025 09:56, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Fri, Oct 17, 2025 at 03:26:41PM +0200, Hans Verkuil wrote:
>>>> Add missing ioctls. This makes v4l2-compliance happier:
>>>>
>>>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
>>>> 	test VIDIOC_G_FMT: FAIL
>>>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
>>>> 	test VIDIOC_TRY_FMT: FAIL
>>>> fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
>>>> 	test VIDIOC_S_FMT: FAIL
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>>>> ---
>>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>> index 864d38140b87..77beea00d507 100644
>>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>> @@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int
>>>> +isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>>>> +{
>>>> +	struct isp_video *video = video_drvdata(file);
>>>> +	unsigned int i, j;
>>>> +	unsigned int skip_last_fmts = 1;
>>>> +
>>>> +	if (f->type != video->type)
>>>> +		return -EINVAL;
>>>> +
>>>> +	/*
>>>> +	 * The last two formats have the same pixelformat as the two
>>>> +	 * formats before them, but they do have different mediabus
>>>> +	 * codes. So to avoid reporting duplicate pixelformats we skip
>>>> +	 * those two, provided f->mbus_code is 0.
>>>> +	 */
>>>> +	if (!f->mbus_code)
>>>> +		skip_last_fmts += 2;
>>>> +	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
>>>> +		if (f->mbus_code && formats[i].code != f->mbus_code)
>>>> +			continue;
>>>
>>> How about, instead of the skip_last_fmts thingy, using this:
>>>
>>> 		/* Weed out pixelformats with the same mbus code. */
>>> 		if (i && formats[i - 1].code == formats[i].code)
>>> 			continue;
>>
>> Good idea, but it should be this:
>>
>>                /* Weed out duplicate pixelformats with different mbus codes */
>>                if (!f->mbus_code && i &&
>>                    formats[i - 1].pixelformat == formats[i].pixelformat)
>>                        continue;
> 
> I think you shouldn't add !f->mbus_code check here, there's already a check
> for that right after the for ... line.

Ah, no. If you search with non-zero f->mbus_code, then you want to
see all matching formats for that mbus_code.

Without the '!f->mbus_code' condition you would incorrectly skip entries in
the formats array.

E.g. if f->mbus_code is set to MEDIA_BUS_FMT_UYVY8_2X8, then it would fail to
find V4L2_PIX_FMT_UYVY as a match.

Regards,

	Hans

> 
>>
>> And the duplicate pixelformats in the formats array must be together in the
>> array for this to work. Easy enough to change.
> 
> The pixelformats in formats array would need to be sorted for this -- right
> now they're not (16- vs. 8-bit YUV formats at the end of the table). So I
> think this should be:
> 
> 		if (f->mbus_code && formats[i].code != f->mbus_code)
> 			continue;
> 
> 		/* Weed up duplicate pixelformats. */
> 		if (i && formats[i - 1].pixelformat == formats[i].pixelformat)
> 			continue;
> 

Re: [PATCH 09/14] media: omap3isp: better VIDIOC_G/S_PARM handling

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 02:40:38PM +0100, Hans Verkuil wrote:
> On 01/12/2025 11:28, Hans Verkuil wrote:
> > On 22/10/2025 10:09, Sakari Ailus wrote:
> >> Hi Hans,
> >>
> >> On Fri, Oct 17, 2025 at 03:26:46PM +0200, Hans Verkuil wrote:
> >>> Fix various v4l2-compliance errors relating to timeperframe.
> >>>
> >>> VIDIOC_G/S_PARM is only supported for Video Output, so disable
> >>> these ioctls for Capture devices.
> >>>
> >>> Ensure numerator and denominator are never 0.
> >>>
> >>> Set missing V4L2_CAP_TIMEPERFRAME capability for VIDIOC_S_PARM.
> >>>
> >>> v4l2-compliance:
> >>>
> >>> 	fail: v4l2-test-formats.cpp(1388): out->timeperframe.numerator == 0 || out->timeperframe.denominator == 0
> >>> test VIDIOC_G/S_PARM: FAIL
> >>>
> >>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >>> ---
> >>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 11 +++++++++--
> >>>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>> index 471defa6e7fb..5603586271f5 100644
> >>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>> @@ -928,7 +928,10 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
> >>>  
> >>>  	if (a->parm.output.timeperframe.denominator == 0)
> >>>  		a->parm.output.timeperframe.denominator = 1;
> >>> +	if (a->parm.output.timeperframe.numerator == 0)
> >>> +		a->parm.output.timeperframe.numerator = 1;
> >>
> >> I believe S_PARM support has probably been added for v4l2-compliance in the
> >> past. Should there be either a dummy implementation for more or less all
> >> Media device centric drivers or could this be simply omitted?
> > 
> > v4l2-compliance has seen quite a few changes w.r.t. the G/S_PARM tests,
> > and I think it is fine to just drop support for these ioctls.
> > 
> > I'll test this a bit more, and if I don't find any issues, then I'll
> > just remove support for these ioctls in omap3isp.
> 
> Actually, S_PARM is used to set the max framerate for the output of the omap3isp.
> 
> So it is in use.
> 
> I'm keeping this patch.

You're right; it's used on OUTPUT buffers indeed.

-- 
Sakari Ailus

Re: [PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 02:44:44PM +0100, Hans Verkuil wrote:
> On 01/12/2025 12:35, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Dec 01, 2025 at 09:40:58AM +0100, Hans Verkuil wrote:
> >> On 22/10/2025 09:56, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Fri, Oct 17, 2025 at 03:26:41PM +0200, Hans Verkuil wrote:
> >>>> Add missing ioctls. This makes v4l2-compliance happier:
> >>>>
> >>>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
> >>>> 	test VIDIOC_G_FMT: FAIL
> >>>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
> >>>> 	test VIDIOC_TRY_FMT: FAIL
> >>>> fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
> >>>> 	test VIDIOC_S_FMT: FAIL
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >>>> ---
> >>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
> >>>>  1 file changed, 34 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>>> index 864d38140b87..77beea00d507 100644
> >>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>>> @@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> +static int
> >>>> +isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> >>>> +{
> >>>> +	struct isp_video *video = video_drvdata(file);
> >>>> +	unsigned int i, j;
> >>>> +	unsigned int skip_last_fmts = 1;
> >>>> +
> >>>> +	if (f->type != video->type)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	/*
> >>>> +	 * The last two formats have the same pixelformat as the two
> >>>> +	 * formats before them, but they do have different mediabus
> >>>> +	 * codes. So to avoid reporting duplicate pixelformats we skip
> >>>> +	 * those two, provided f->mbus_code is 0.
> >>>> +	 */
> >>>> +	if (!f->mbus_code)
> >>>> +		skip_last_fmts += 2;
> >>>> +	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
> >>>> +		if (f->mbus_code && formats[i].code != f->mbus_code)
> >>>> +			continue;
> >>>
> >>> How about, instead of the skip_last_fmts thingy, using this:
> >>>
> >>> 		/* Weed out pixelformats with the same mbus code. */
> >>> 		if (i && formats[i - 1].code == formats[i].code)
> >>> 			continue;
> >>
> >> Good idea, but it should be this:
> >>
> >>                /* Weed out duplicate pixelformats with different mbus codes */
> >>                if (!f->mbus_code && i &&
> >>                    formats[i - 1].pixelformat == formats[i].pixelformat)
> >>                        continue;
> > 
> > I think you shouldn't add !f->mbus_code check here, there's already a check
> > for that right after the for ... line.
> 
> Ah, no. If you search with non-zero f->mbus_code, then you want to
> see all matching formats for that mbus_code.
> 
> Without the '!f->mbus_code' condition you would incorrectly skip entries in
> the formats array.
> 
> E.g. if f->mbus_code is set to MEDIA_BUS_FMT_UYVY8_2X8, then it would fail to
> find V4L2_PIX_FMT_UYVY as a match.

Ack, sounds good.

-- 
Sakari Ailus

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Hans Verkuil]

On 01/12/2025 13:56, Sakari Ailus wrote:
> On Mon, Dec 01, 2025 at 12:22:12PM +0200, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Mon, Dec 01, 2025 at 09:27:55AM +0100, Hans Verkuil wrote:
>>> Hi Sakari,
>>>
>>> On 22/10/2025 09:48, Sakari Ailus wrote:
>>>> Hi Hans,
>>>>
>>>> Thanks for the set.
>>>>
>>>> On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
>>>>> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
>>>>> the last empty entry { 0, } of the formats array into account.
>>>>>
>>>>> As a result, isp_video_mbus_to_pix would accept code 0 and
>>>>> isp_video_pix_to_mbus would select code 0 if no match was found.
>>>>>
>>>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>>>>> ---
>>>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>> index 46609045e2c8..864d38140b87 100644
>>>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
>>>>>  	pix->width = mbus->width;
>>>>>  	pix->height = mbus->height;
>>>>>  
>>>>> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>>>>> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
>>>>
>>>> As it seems all users of the formats array depend on the size of the array
>>>> and not its contents, could we remove the sentinel entry from the array
>>>> instead?
>>>
>>> Probably, but see this comment just before the sentinel in the array:
>>>
>>>         /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
>>>          * module and avoid NULL pointer dereferences.
>>>          */
>>>         { 0, }
>>>
>>> Now, I wonder if this comment is out of date, since I don't see code 0 being used
>>> by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.
>>
>> Yes, please!
> 
> Actually it's omap3isp_video_format_info() I understand ispccdc.c relies
> not to return NULL. I might add a separate variable for that, to get rid of
> this obscure arrangement.

So ispccdc.c can call omap3isp_video_format_info with a code value of 0?
Can you give an example where that happens? If true, then this feels very
fragile.

Regards,

	Hans

> 
> You could also add:
> 
> Fixes: c51364cafa26 ("[media] omap3isp: ccdc: Add YUV input formats support")
> 

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Sakari Ailus]

Hi Hans,

On Mon, Dec 01, 2025 at 04:35:23PM +0100, Hans Verkuil wrote:
> On 01/12/2025 13:56, Sakari Ailus wrote:
> > On Mon, Dec 01, 2025 at 12:22:12PM +0200, Sakari Ailus wrote:
> >> Hi Hans,
> >>
> >> On Mon, Dec 01, 2025 at 09:27:55AM +0100, Hans Verkuil wrote:
> >>> Hi Sakari,
> >>>
> >>> On 22/10/2025 09:48, Sakari Ailus wrote:
> >>>> Hi Hans,
> >>>>
> >>>> Thanks for the set.
> >>>>
> >>>> On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
> >>>>> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
> >>>>> the last empty entry { 0, } of the formats array into account.
> >>>>>
> >>>>> As a result, isp_video_mbus_to_pix would accept code 0 and
> >>>>> isp_video_pix_to_mbus would select code 0 if no match was found.
> >>>>>
> >>>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> >>>>> ---
> >>>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
> >>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>>>> index 46609045e2c8..864d38140b87 100644
> >>>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> >>>>> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
> >>>>>  	pix->width = mbus->width;
> >>>>>  	pix->height = mbus->height;
> >>>>>  
> >>>>> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> >>>>> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
> >>>>
> >>>> As it seems all users of the formats array depend on the size of the array
> >>>> and not its contents, could we remove the sentinel entry from the array
> >>>> instead?
> >>>
> >>> Probably, but see this comment just before the sentinel in the array:
> >>>
> >>>         /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
> >>>          * module and avoid NULL pointer dereferences.
> >>>          */
> >>>         { 0, }
> >>>
> >>> Now, I wonder if this comment is out of date, since I don't see code 0 being used
> >>> by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.
> >>
> >> Yes, please!
> > 
> > Actually it's omap3isp_video_format_info() I understand ispccdc.c relies
> > not to return NULL. I might add a separate variable for that, to get rid of
> > this obscure arrangement.
> 
> So ispccdc.c can call omap3isp_video_format_info with a code value of 0?
> Can you give an example where that happens? If true, then this feels very
> fragile.

It is fragile, yes. I can't point to a place where this happens but the
driver relies on every mbus code ever used to be on that table, and this
not holding will result in NULL dereference. In other words, it's not easy
at all to figure out it won't happen.

-- 
Sakari Ailus

Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Hans Verkuil]

On 01/12/2025 17:43, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Dec 01, 2025 at 04:35:23PM +0100, Hans Verkuil wrote:
>> On 01/12/2025 13:56, Sakari Ailus wrote:
>>> On Mon, Dec 01, 2025 at 12:22:12PM +0200, Sakari Ailus wrote:
>>>> Hi Hans,
>>>>
>>>> On Mon, Dec 01, 2025 at 09:27:55AM +0100, Hans Verkuil wrote:
>>>>> Hi Sakari,
>>>>>
>>>>> On 22/10/2025 09:48, Sakari Ailus wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> Thanks for the set.
>>>>>>
>>>>>> On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
>>>>>>> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
>>>>>>> the last empty entry { 0, } of the formats array into account.
>>>>>>>
>>>>>>> As a result, isp_video_mbus_to_pix would accept code 0 and
>>>>>>> isp_video_pix_to_mbus would select code 0 if no match was found.
>>>>>>>
>>>>>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>>>>>>> ---
>>>>>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
>>>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>>>> index 46609045e2c8..864d38140b87 100644
>>>>>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>>>> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
>>>>>>>  	pix->width = mbus->width;
>>>>>>>  	pix->height = mbus->height;
>>>>>>>  
>>>>>>> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>>>>>>> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
>>>>>>
>>>>>> As it seems all users of the formats array depend on the size of the array
>>>>>> and not its contents, could we remove the sentinel entry from the array
>>>>>> instead?
>>>>>
>>>>> Probably, but see this comment just before the sentinel in the array:
>>>>>
>>>>>         /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
>>>>>          * module and avoid NULL pointer dereferences.
>>>>>          */
>>>>>         { 0, }
>>>>>
>>>>> Now, I wonder if this comment is out of date, since I don't see code 0 being used
>>>>> by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.
>>>>
>>>> Yes, please!
>>>
>>> Actually it's omap3isp_video_format_info() I understand ispccdc.c relies
>>> not to return NULL. I might add a separate variable for that, to get rid of
>>> this obscure arrangement.
>>
>> So ispccdc.c can call omap3isp_video_format_info with a code value of 0?
>> Can you give an example where that happens? If true, then this feels very
>> fragile.
> 
> It is fragile, yes. I can't point to a place where this happens but the
> driver relies on every mbus code ever used to be on that table, and this
> not holding will result in NULL dereference. In other words, it's not easy
> at all to figure out it won't happen.
> 

I think I prefer to stick to my original patch, keeping the sentinel.

I don't want to spend too much time on this, it was just a bunch of patches
that I accumulated while working on the vb2 wait_prepare/finish callback
removal. There are a lot more issues that v4l2-compliance found that I
never spend time on.

I'll put the sentinel back and post a new v2 series later this week.

Regards,

	Hans