[PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[Hans Verkuil]

This is a follow-up series from Vandana's "[v3] v4l2-core: Add metadata type to
vfl_devnode_type" patch: https://patchwork.linuxtv.org/patch/58755/

While testing that v3 patch with a patched version of vivid that has metadata
capture support, I realized that metadata should be treated the same way as
vbi in determine_valid_ioctls(). That makes sense since vbi *is* effectively
metadata. So I changed Vandana's patch (hence my Co-Developed-by tag) to
correctly validate the ioctls for metadata.

I also added two follow-up patches to simplify the SDR code in that function,
and to fix the code for v4l-touch devices (too many ioctls were enabled for
such devices). I think the final code is easier to read as well.

Regards,

	Hans

Hans Verkuil (2):
  v4l2-dev: simplify the SDR checks
  v4l2-dev: fix is_tch checks

Vandana BN (1):
  v4l2-core: Add metadata type to vfl_devnode_type

 drivers/media/v4l2-core/v4l2-dev.c   | 97 ++++++++++++++++------------
 drivers/media/v4l2-core/v4l2-ioctl.c |  5 +-
 include/media/v4l2-dev.h             |  2 +
 3 files changed, 61 insertions(+), 43 deletions(-)

-- 
2.20.1

[PATCHv4 1/3] v4l2-core: Add metadata type to vfl_devnode_type

[Hans Verkuil]

From: Vandana BN <bnvandana@gmail.com>

Add VFL_TYPE_METADATA, to detect devices of type metadata and
to disable unneeded ioctls.

Without this change, format ioctls for both video and metadata devices
could be called on both device nodes. This is true for other ioctls as
well.

Metadata devices act similar to VBI devices w.r.t. which ioctls should
be enabled. This makes sense since VBI *is* metadata.

Signed-off-by: Vandana BN <bnvandana@gmail.com>
Co-Developed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-dev.c   | 58 ++++++++++++++++------------
 drivers/media/v4l2-core/v4l2-ioctl.c |  5 ++-
 include/media/v4l2-dev.h             |  2 +
 3 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 4037689a945a..27b93280d342 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -112,6 +112,7 @@ static inline unsigned long *devnode_bits(enum vfl_devnode_type vfl_type)
 	   one single bitmap for the purposes of finding a free node number
 	   since all those unassigned types use the same minor range. */
 	int idx = (vfl_type > VFL_TYPE_RADIO) ? VFL_TYPE_MAX - 1 : vfl_type;
+	idx = (vfl_type == VFL_TYPE_METADATA) ? VFL_TYPE_GRABBER : vfl_type;
 
 	return devnode_nums[idx];
 }
@@ -119,7 +120,9 @@ static inline unsigned long *devnode_bits(enum vfl_devnode_type vfl_type)
 /* Return the bitmap corresponding to vfl_type. */
 static inline unsigned long *devnode_bits(enum vfl_devnode_type vfl_type)
 {
-	return devnode_nums[vfl_type];
+	int idx = (vfl_type == VFL_TYPE_METADATA) ? VFL_TYPE_GRABBER : vfl_type;
+
+	return devnode_nums[idx];
 }
 #endif
 
@@ -540,6 +543,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	bool is_radio = vdev->vfl_type == VFL_TYPE_RADIO;
 	bool is_sdr = vdev->vfl_type == VFL_TYPE_SDR;
 	bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
+	bool is_meta = vdev->vfl_type == VFL_TYPE_METADATA;
 	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
 
@@ -587,39 +591,31 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
 
 	if (is_vid || is_tch) {
-		/* video and metadata specific ioctls */
+		/* video and touch specific ioctls */
 		if ((is_rx && (ops->vidioc_enum_fmt_vid_cap ||
-			       ops->vidioc_enum_fmt_vid_overlay ||
-			       ops->vidioc_enum_fmt_meta_cap)) ||
-		    (is_tx && (ops->vidioc_enum_fmt_vid_out ||
-			       ops->vidioc_enum_fmt_meta_out)))
+			       ops->vidioc_enum_fmt_vid_overlay)) ||
+		    (is_tx && ops->vidioc_enum_fmt_vid_out))
 			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
 		if ((is_rx && (ops->vidioc_g_fmt_vid_cap ||
 			       ops->vidioc_g_fmt_vid_cap_mplane ||
-			       ops->vidioc_g_fmt_vid_overlay ||
-			       ops->vidioc_g_fmt_meta_cap)) ||
+			       ops->vidioc_g_fmt_vid_overlay)) ||
 		    (is_tx && (ops->vidioc_g_fmt_vid_out ||
 			       ops->vidioc_g_fmt_vid_out_mplane ||
-			       ops->vidioc_g_fmt_vid_out_overlay ||
-			       ops->vidioc_g_fmt_meta_out)))
+			       ops->vidioc_g_fmt_vid_out_overlay)))
 			 set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
 		if ((is_rx && (ops->vidioc_s_fmt_vid_cap ||
 			       ops->vidioc_s_fmt_vid_cap_mplane ||
-			       ops->vidioc_s_fmt_vid_overlay ||
-			       ops->vidioc_s_fmt_meta_cap)) ||
+			       ops->vidioc_s_fmt_vid_overlay)) ||
 		    (is_tx && (ops->vidioc_s_fmt_vid_out ||
 			       ops->vidioc_s_fmt_vid_out_mplane ||
-			       ops->vidioc_s_fmt_vid_out_overlay ||
-			       ops->vidioc_s_fmt_meta_out)))
+			       ops->vidioc_s_fmt_vid_out_overlay)))
 			 set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
 		if ((is_rx && (ops->vidioc_try_fmt_vid_cap ||
 			       ops->vidioc_try_fmt_vid_cap_mplane ||
-			       ops->vidioc_try_fmt_vid_overlay ||
-			       ops->vidioc_try_fmt_meta_cap)) ||
+			       ops->vidioc_try_fmt_vid_overlay)) ||
 		    (is_tx && (ops->vidioc_try_fmt_vid_out ||
 			       ops->vidioc_try_fmt_vid_out_mplane ||
-			       ops->vidioc_try_fmt_vid_out_overlay ||
-			       ops->vidioc_try_fmt_meta_out)))
+			       ops->vidioc_try_fmt_vid_out_overlay)))
 			 set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
 		SET_VALID_IOCTL(ops, VIDIOC_OVERLAY, vidioc_overlay);
 		SET_VALID_IOCTL(ops, VIDIOC_G_FBUF, vidioc_g_fbuf);
@@ -679,10 +675,22 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
 		if (ops->vidioc_try_fmt_sdr_out)
 			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
+	} else if (is_meta && is_rx) {
+		/* metadata capture specific ioctls */
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_meta_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_meta_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_meta_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_meta_cap);
+	} else if (is_meta && is_tx) {
+		/* metadata output specific ioctls */
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_meta_out);
+		SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_meta_out);
+		SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_meta_out);
+		SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_meta_out);
 	}
 
-	if (is_vid || is_vbi || is_sdr || is_tch) {
-		/* ioctls valid for video, metadata, vbi or sdr */
+	if (is_vid || is_vbi || is_sdr || is_tch || is_meta) {
+		/* ioctls valid for video, vbi, sdr, touch and metadata */
 		SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
 		SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
 		SET_VALID_IOCTL(ops, VIDIOC_QBUF, vidioc_qbuf);
@@ -694,8 +702,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
 	}
 
-	if (is_vid || is_vbi || is_tch) {
-		/* ioctls valid for video or vbi */
+	if (is_vid || is_vbi || is_tch || is_meta) {
+		/* ioctls valid for video, vbi, touch and metadata */
 		if (ops->vidioc_s_std)
 			set_bit(_IOC_NR(VIDIOC_ENUMSTD), valid_ioctls);
 		SET_VALID_IOCTL(ops, VIDIOC_S_STD, vidioc_s_std);
@@ -719,8 +727,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
 			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
 		}
-		if (ops->vidioc_g_parm || (vdev->vfl_type == VFL_TYPE_GRABBER &&
-					ops->vidioc_g_std))
+		if (ops->vidioc_g_parm || ops->vidioc_g_std)
 			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
 		SET_VALID_IOCTL(ops, VIDIOC_S_PARM, vidioc_s_parm);
 		SET_VALID_IOCTL(ops, VIDIOC_S_DV_TIMINGS, vidioc_s_dv_timings);
@@ -762,6 +769,7 @@ static int video_register_media_controller(struct video_device *vdev)
 
 	switch (vdev->vfl_type) {
 	case VFL_TYPE_GRABBER:
+	case VFL_TYPE_METADATA:
 		intf_type = MEDIA_INTF_T_V4L_VIDEO;
 		vdev->entity.function = MEDIA_ENT_F_IO_V4L;
 		break;
@@ -870,6 +878,7 @@ int __video_register_device(struct video_device *vdev,
 	/* Part 1: check device type */
 	switch (type) {
 	case VFL_TYPE_GRABBER:
+	case VFL_TYPE_METADATA:
 		name_base = "video";
 		break;
 	case VFL_TYPE_VBI:
@@ -914,6 +923,7 @@ int __video_register_device(struct video_device *vdev,
 	 * (new style). */
 	switch (type) {
 	case VFL_TYPE_GRABBER:
+	case VFL_TYPE_METADATA:
 		minor_offset = 0;
 		minor_cnt = 64;
 		break;
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 51b912743f0f..0d71c06c82cf 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -938,6 +938,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 	bool is_vbi = vfd->vfl_type == VFL_TYPE_VBI;
 	bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
 	bool is_tch = vfd->vfl_type == VFL_TYPE_TOUCH;
+	bool is_meta = vfd->vfl_type == VFL_TYPE_METADATA;
 	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
 
@@ -996,11 +997,11 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 			return 0;
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
-		if (is_vid && is_rx && ops->vidioc_g_fmt_meta_cap)
+		if (is_meta && is_rx && ops->vidioc_g_fmt_meta_cap)
 			return 0;
 		break;
 	case V4L2_BUF_TYPE_META_OUTPUT:
-		if (is_vid && is_tx && ops->vidioc_g_fmt_meta_out)
+		if (is_meta && is_tx && ops->vidioc_g_fmt_meta_out)
 			return 0;
 		break;
 	default:
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 48531e57cc5a..2da91d454c10 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -30,6 +30,7 @@
  * @VFL_TYPE_SUBDEV:	for V4L2 subdevices
  * @VFL_TYPE_SDR:	for Software Defined Radio tuners
  * @VFL_TYPE_TOUCH:	for touch sensors
+ * @VFL_TYPE_METADATA:	for Metadata device
  * @VFL_TYPE_MAX:	number of VFL types, must always be last in the enum
  */
 enum vfl_devnode_type {
@@ -39,6 +40,7 @@ enum vfl_devnode_type {
 	VFL_TYPE_SUBDEV,
 	VFL_TYPE_SDR,
 	VFL_TYPE_TOUCH,
+	VFL_TYPE_METADATA,
 	VFL_TYPE_MAX /* Shall be the last one */
 };
 
-- 
2.20.1

[PATCHv4 2/3] v4l2-dev: simplify the SDR checks

[Hans Verkuil]

In determine_valid_ioctls() we can use SET_VALID_IOCTL to enable
ioctls for SDR, simplifying the code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-dev.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 27b93280d342..c7132eb72071 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -657,24 +657,16 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_G_SLICED_VBI_CAP, vidioc_g_sliced_vbi_cap);
 	} else if (is_sdr && is_rx) {
 		/* SDR receiver specific ioctls */
-		if (ops->vidioc_enum_fmt_sdr_cap)
-			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
-		if (ops->vidioc_g_fmt_sdr_cap)
-			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
-		if (ops->vidioc_s_fmt_sdr_cap)
-			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
-		if (ops->vidioc_try_fmt_sdr_cap)
-			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_sdr_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_sdr_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_sdr_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_sdr_cap);
 	} else if (is_sdr && is_tx) {
 		/* SDR transmitter specific ioctls */
-		if (ops->vidioc_enum_fmt_sdr_out)
-			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
-		if (ops->vidioc_g_fmt_sdr_out)
-			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
-		if (ops->vidioc_s_fmt_sdr_out)
-			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
-		if (ops->vidioc_try_fmt_sdr_out)
-			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_sdr_out);
+		SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_sdr_out);
+		SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_sdr_out);
+		SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_sdr_out);
 	} else if (is_meta && is_rx) {
 		/* metadata capture specific ioctls */
 		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_meta_cap);
-- 
2.20.1

[PATCHv4 3/3] v4l2-dev: fix is_tch checks

[Hans Verkuil]

Touch devices mark too many ioctls as valid. Restrict the list of
valid ioctls for touch devices.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-dev.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c7132eb72071..fd6792cb0d57 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -590,8 +590,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator)
 		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
 
-	if (is_vid || is_tch) {
-		/* video and touch specific ioctls */
+	if (is_vid) {
+		/* video specific ioctls */
 		if ((is_rx && (ops->vidioc_enum_fmt_vid_cap ||
 			       ops->vidioc_enum_fmt_vid_overlay)) ||
 		    (is_tx && ops->vidioc_enum_fmt_vid_out))
@@ -667,6 +667,19 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_sdr_out);
 		SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_sdr_out);
 		SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_sdr_out);
+	} else if (is_tch) {
+		/* touch specific ioctls */
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_vid_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_vid_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_vid_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_vid_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMESIZES, vidioc_enum_framesizes);
+		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMEINTERVALS, vidioc_enum_frameintervals);
+		SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
+		SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
+		SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+		SET_VALID_IOCTL(ops, VIDIOC_G_PARM, vidioc_g_parm);
+		SET_VALID_IOCTL(ops, VIDIOC_S_PARM, vidioc_s_parm);
 	} else if (is_meta && is_rx) {
 		/* metadata capture specific ioctls */
 		SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_meta_cap);
@@ -694,8 +707,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
 	}
 
-	if (is_vid || is_vbi || is_tch || is_meta) {
-		/* ioctls valid for video, vbi, touch and metadata */
+	if (is_vid || is_vbi || is_meta) {
+		/* ioctls valid for video, vbi and metadata */
 		if (ops->vidioc_s_std)
 			set_bit(_IOC_NR(VIDIOC_ENUMSTD), valid_ioctls);
 		SET_VALID_IOCTL(ops, VIDIOC_S_STD, vidioc_s_std);
-- 
2.20.1

Re: [PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[Laurent Pinchart]

Hi Hans,

On Tue, Sep 17, 2019 at 03:36:44PM +0200, Hans Verkuil wrote:
> This is a follow-up series from Vandana's "[v3] v4l2-core: Add metadata type to
> vfl_devnode_type" patch: https://patchwork.linuxtv.org/patch/58755/
> 
> While testing that v3 patch with a patched version of vivid that has metadata
> capture support, I realized that metadata should be treated the same way as
> vbi in determine_valid_ioctls(). That makes sense since vbi *is* effectively
> metadata. So I changed Vandana's patch (hence my Co-Developed-by tag) to
> correctly validate the ioctls for metadata.
> 
> I also added two follow-up patches to simplify the SDR code in that function,
> and to fix the code for v4l-touch devices (too many ioctls were enabled for
> such devices). I think the final code is easier to read as well.

Quoting my reply to "[RFC] V4L2 & Metadata: switch to /dev/v4l-metaX
instead of /dev/videoX]" as it may have fell through the cracks, and I
don't want this series to be merged without addressing the issue,

One of the reason [we didn't introduce a metadata video node type] is
CSI-2 sensors and CSI-2 receivers. A CSI-2 link often carries both video
and metadata using two different datatypes. From the point of view of
the CSI-2 receiver or the DMA engines, video and metadata are not
distinguishable, the CSI-2 receiver receives one stream with two data
types, demultiplexes them, and passes them to different DMA engines. A
sensor could send two video datatypes, or even conceptually two metadata
data types, and this would work the exact same way, with each of the two
DMA engines capturing data to buffers without caring about the contents.
Given that the datatype selection happens at runtime, a given DMA engine
is thus not dedicated to video or metadata, any of the DMA engines (and
there could also be more than two) could handle any type of data. For
this type of system we thus can't dedicate device nodes to video or
metadata, they need to support either.

> Hans Verkuil (2):
>   v4l2-dev: simplify the SDR checks
>   v4l2-dev: fix is_tch checks
> 
> Vandana BN (1):
>   v4l2-core: Add metadata type to vfl_devnode_type
> 
>  drivers/media/v4l2-core/v4l2-dev.c   | 97 ++++++++++++++++------------
>  drivers/media/v4l2-core/v4l2-ioctl.c |  5 +-
>  include/media/v4l2-dev.h             |  2 +
>  3 files changed, 61 insertions(+), 43 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[Hans Verkuil]

Hi Laurent,

On 9/21/19 1:48 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tue, Sep 17, 2019 at 03:36:44PM +0200, Hans Verkuil wrote:
>> This is a follow-up series from Vandana's "[v3] v4l2-core: Add metadata type to
>> vfl_devnode_type" patch: https://patchwork.linuxtv.org/patch/58755/
>>
>> While testing that v3 patch with a patched version of vivid that has metadata
>> capture support, I realized that metadata should be treated the same way as
>> vbi in determine_valid_ioctls(). That makes sense since vbi *is* effectively
>> metadata. So I changed Vandana's patch (hence my Co-Developed-by tag) to
>> correctly validate the ioctls for metadata.
>>
>> I also added two follow-up patches to simplify the SDR code in that function,
>> and to fix the code for v4l-touch devices (too many ioctls were enabled for
>> such devices). I think the final code is easier to read as well.
> 
> Quoting my reply to "[RFC] V4L2 & Metadata: switch to /dev/v4l-metaX
> instead of /dev/videoX]" as it may have fell through the cracks, and I
> don't want this series to be merged without addressing the issue,
> 
> One of the reason [we didn't introduce a metadata video node type] is
> CSI-2 sensors and CSI-2 receivers. A CSI-2 link often carries both video
> and metadata using two different datatypes. From the point of view of
> the CSI-2 receiver or the DMA engines, video and metadata are not
> distinguishable, the CSI-2 receiver receives one stream with two data
> types, demultiplexes them, and passes them to different DMA engines. A
> sensor could send two video datatypes, or even conceptually two metadata
> data types, and this would work the exact same way, with each of the two
> DMA engines capturing data to buffers without caring about the contents.
> Given that the datatype selection happens at runtime, a given DMA engine

'happens at runtime': what decides this? The user-specified topology?
Something else?

Is this documented somewhere?

To my knowledge there are no drivers that can do this in mainline code,
right? The current drivers all create dedicated metadata devices.

Also, this specific use-case is for capture only. Do you think this
might be needed for metadata output as well?

Regards,

	Hans

> is thus not dedicated to video or metadata, any of the DMA engines (and
> there could also be more than two) could handle any type of data. For
> this type of system we thus can't dedicate device nodes to video or
> metadata, they need to support either.
> 
>> Hans Verkuil (2):
>>   v4l2-dev: simplify the SDR checks
>>   v4l2-dev: fix is_tch checks
>>
>> Vandana BN (1):
>>   v4l2-core: Add metadata type to vfl_devnode_type
>>
>>  drivers/media/v4l2-core/v4l2-dev.c   | 97 ++++++++++++++++------------
>>  drivers/media/v4l2-core/v4l2-ioctl.c |  5 +-
>>  include/media/v4l2-dev.h             |  2 +
>>  3 files changed, 61 insertions(+), 43 deletions(-)
> 

Re: [PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[Sakari Ailus]

Hi Hans, Laurent,

On Mon, Sep 23, 2019 at 10:11:09AM +0200, Hans Verkuil wrote:
> Hi Laurent,
> 
> On 9/21/19 1:48 AM, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Tue, Sep 17, 2019 at 03:36:44PM +0200, Hans Verkuil wrote:
> >> This is a follow-up series from Vandana's "[v3] v4l2-core: Add metadata type to
> >> vfl_devnode_type" patch: https://patchwork.linuxtv.org/patch/58755/
> >>
> >> While testing that v3 patch with a patched version of vivid that has metadata
> >> capture support, I realized that metadata should be treated the same way as
> >> vbi in determine_valid_ioctls(). That makes sense since vbi *is* effectively
> >> metadata. So I changed Vandana's patch (hence my Co-Developed-by tag) to
> >> correctly validate the ioctls for metadata.
> >>
> >> I also added two follow-up patches to simplify the SDR code in that function,
> >> and to fix the code for v4l-touch devices (too many ioctls were enabled for
> >> such devices). I think the final code is easier to read as well.
> > 
> > Quoting my reply to "[RFC] V4L2 & Metadata: switch to /dev/v4l-metaX
> > instead of /dev/videoX]" as it may have fell through the cracks, and I
> > don't want this series to be merged without addressing the issue,
> > 
> > One of the reason [we didn't introduce a metadata video node type] is
> > CSI-2 sensors and CSI-2 receivers. A CSI-2 link often carries both video
> > and metadata using two different datatypes. From the point of view of
> > the CSI-2 receiver or the DMA engines, video and metadata are not
> > distinguishable, the CSI-2 receiver receives one stream with two data
> > types, demultiplexes them, and passes them to different DMA engines. A
> > sensor could send two video datatypes, or even conceptually two metadata
> > data types, and this would work the exact same way, with each of the two
> > DMA engines capturing data to buffers without caring about the contents.
> > Given that the datatype selection happens at runtime, a given DMA engine
> 
> 'happens at runtime': what decides this? The user-specified topology?
> Something else?
> 
> Is this documented somewhere?

Yes. This ultimately depends on the formats configured by the user. Some of
the formats are metadata, and with sub-stream support, these will be
routable by different video nodes.

I we designate video nodes either "metadata" or "pixel data" nodes, then
this would need to be changed dynamically based on the format selected. I
don't think it's really worth it, as the user space also doesn't expect the
node type to change.

> 
> To my knowledge there are no drivers that can do this in mainline code,
> right? The current drivers all create dedicated metadata devices.

Not right now, no. But it's just a question of when, not if.

> 
> Also, this specific use-case is for capture only. Do you think this
> might be needed for metadata output as well?

As Laurent noted, the DMA engines don't know the data they handle, so in
principle it's possible that this could be relevant for output, too.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[Hans Verkuil]

On 9/23/19 10:17 AM, Sakari Ailus wrote:
> Hi Hans, Laurent,
> 
> On Mon, Sep 23, 2019 at 10:11:09AM +0200, Hans Verkuil wrote:
>> Hi Laurent,
>>
>> On 9/21/19 1:48 AM, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> On Tue, Sep 17, 2019 at 03:36:44PM +0200, Hans Verkuil wrote:
>>>> This is a follow-up series from Vandana's "[v3] v4l2-core: Add metadata type to
>>>> vfl_devnode_type" patch: https://patchwork.linuxtv.org/patch/58755/
>>>>
>>>> While testing that v3 patch with a patched version of vivid that has metadata
>>>> capture support, I realized that metadata should be treated the same way as
>>>> vbi in determine_valid_ioctls(). That makes sense since vbi *is* effectively
>>>> metadata. So I changed Vandana's patch (hence my Co-Developed-by tag) to
>>>> correctly validate the ioctls for metadata.
>>>>
>>>> I also added two follow-up patches to simplify the SDR code in that function,
>>>> and to fix the code for v4l-touch devices (too many ioctls were enabled for
>>>> such devices). I think the final code is easier to read as well.
>>>
>>> Quoting my reply to "[RFC] V4L2 & Metadata: switch to /dev/v4l-metaX
>>> instead of /dev/videoX]" as it may have fell through the cracks, and I
>>> don't want this series to be merged without addressing the issue,
>>>
>>> One of the reason [we didn't introduce a metadata video node type] is
>>> CSI-2 sensors and CSI-2 receivers. A CSI-2 link often carries both video
>>> and metadata using two different datatypes. From the point of view of
>>> the CSI-2 receiver or the DMA engines, video and metadata are not
>>> distinguishable, the CSI-2 receiver receives one stream with two data
>>> types, demultiplexes them, and passes them to different DMA engines. A
>>> sensor could send two video datatypes, or even conceptually two metadata
>>> data types, and this would work the exact same way, with each of the two
>>> DMA engines capturing data to buffers without caring about the contents.
>>> Given that the datatype selection happens at runtime, a given DMA engine
>>
>> 'happens at runtime': what decides this? The user-specified topology?
>> Something else?
>>
>> Is this documented somewhere?
> 
> Yes. This ultimately depends on the formats configured by the user. Some of
> the formats are metadata, and with sub-stream support, these will be
> routable by different video nodes.
> 
> I we designate video nodes either "metadata" or "pixel data" nodes, then
> this would need to be changed dynamically based on the format selected. I
> don't think it's really worth it, as the user space also doesn't expect the
> node type to change.

So these video device nodes will need to have both VIDEO_CAPTURE and METADATA_CAPTURE
set in the device_caps field as returned by VIDIOC_QUERYCAP. Both are needed,
otherwise userspace wouldn't know that it can call ENUM_FMT with both buf types.

When the format is changed from video to metadata or vice versa, then the driver
will have to change the type field in the vb2_queue struct to correspond with the
chosen format.

This also means that in determine_valid_ioctls() in v4l2-dev.c I will have to
check vdev->device_caps if this is a video node that can switch between video
and metadata mode dynamically.

Is this correct?

> 
>>
>> To my knowledge there are no drivers that can do this in mainline code,
>> right? The current drivers all create dedicated metadata devices.
> 
> Not right now, no. But it's just a question of when, not if.

This should be emulated by vivid or possibly vimc. That way we can ensure that
the API is correct and that v4l2-compliance can check this properly.

Next time we MUST have proper emulation and tests in place before we add
such features.

Regards,

	Hans

> 
>>
>> Also, this specific use-case is for capture only. Do you think this
>> might be needed for metadata output as well?
> 
> As Laurent noted, the DMA engines don't know the data they handle, so in
> principle it's possible that this could be relevant for output, too.
> 

Re: [PATCHv4 0/3] v4l2-core: Add metadata type to vfl_devnode_type

[Laurent Pinchart]

On Mon, Sep 23, 2019 at 10:47:56AM +0200, Hans Verkuil wrote:
> On 9/23/19 10:17 AM, Sakari Ailus wrote:
> > On Mon, Sep 23, 2019 at 10:11:09AM +0200, Hans Verkuil wrote:
> >> On 9/21/19 1:48 AM, Laurent Pinchart wrote:
> >>> On Tue, Sep 17, 2019 at 03:36:44PM +0200, Hans Verkuil wrote:
> >>>> This is a follow-up series from Vandana's "[v3] v4l2-core: Add metadata type to
> >>>> vfl_devnode_type" patch: https://patchwork.linuxtv.org/patch/58755/
> >>>>
> >>>> While testing that v3 patch with a patched version of vivid that has metadata
> >>>> capture support, I realized that metadata should be treated the same way as
> >>>> vbi in determine_valid_ioctls(). That makes sense since vbi *is* effectively
> >>>> metadata. So I changed Vandana's patch (hence my Co-Developed-by tag) to
> >>>> correctly validate the ioctls for metadata.
> >>>>
> >>>> I also added two follow-up patches to simplify the SDR code in that function,
> >>>> and to fix the code for v4l-touch devices (too many ioctls were enabled for
> >>>> such devices). I think the final code is easier to read as well.
> >>>
> >>> Quoting my reply to "[RFC] V4L2 & Metadata: switch to /dev/v4l-metaX
> >>> instead of /dev/videoX]" as it may have fell through the cracks, and I
> >>> don't want this series to be merged without addressing the issue,
> >>>
> >>> One of the reason [we didn't introduce a metadata video node type] is
> >>> CSI-2 sensors and CSI-2 receivers. A CSI-2 link often carries both video
> >>> and metadata using two different datatypes. From the point of view of
> >>> the CSI-2 receiver or the DMA engines, video and metadata are not
> >>> distinguishable, the CSI-2 receiver receives one stream with two data
> >>> types, demultiplexes them, and passes them to different DMA engines. A
> >>> sensor could send two video datatypes, or even conceptually two metadata
> >>> data types, and this would work the exact same way, with each of the two
> >>> DMA engines capturing data to buffers without caring about the contents.
> >>> Given that the datatype selection happens at runtime, a given DMA engine
> >>
> >> 'happens at runtime': what decides this? The user-specified topology?
> >> Something else?
> >>
> >> Is this documented somewhere?
> > 
> > Yes. This ultimately depends on the formats configured by the user. Some of
> > the formats are metadata, and with sub-stream support, these will be
> > routable by different video nodes.
> > 
> > I we designate video nodes either "metadata" or "pixel data" nodes, then
> > this would need to be changed dynamically based on the format selected. I
> > don't think it's really worth it, as the user space also doesn't expect the
> > node type to change.
> 
> So these video device nodes will need to have both VIDEO_CAPTURE and METADATA_CAPTURE
> set in the device_caps field as returned by VIDIOC_QUERYCAP. Both are needed,
> otherwise userspace wouldn't know that it can call ENUM_FMT with both buf types.
> 
> When the format is changed from video to metadata or vice versa, then the driver
> will have to change the type field in the vb2_queue struct to correspond with the
> chosen format.
> 
> This also means that in determine_valid_ioctls() in v4l2-dev.c I will have to
> check vdev->device_caps if this is a video node that can switch between video
> and metadata mode dynamically.
> 
> Is this correct?

There's a bit of a chicken-and-egg problem though, as the queue type
would need to be changed in response to a VIDIO_S_FMT call...

> >> To my knowledge there are no drivers that can do this in mainline code,
> >> right? The current drivers all create dedicated metadata devices.
> > 
> > Not right now, no. But it's just a question of when, not if.
> 
> This should be emulated by vivid or possibly vimc. That way we can ensure that
> the API is correct and that v4l2-compliance can check this properly.
> 
> Next time we MUST have proper emulation and tests in place before we add
> such features.
> 
> >> Also, this specific use-case is for capture only. Do you think this
> >> might be needed for metadata output as well?
> > 
> > As Laurent noted, the DMA engines don't know the data they handle, so in
> > principle it's possible that this could be relevant for output, too.

-- 
Regards,

Laurent Pinchart