[RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[Hans Verkuil]

Currently the VIDIOC_SUBDEV_G/S_EDID and struct v4l2_subdev_edid are subdev
APIs. However, that's in reality quite annoying since for simple video
pipelines there is no need to create v4l-subdev device nodes for anything
else except for setting or getting EDIDs.

What happens in practice is that v4l2 bridge drivers add explicit support
for VIDIOC_SUBDEV_G/S_EDID themselves, just to avoid having to create
subdev device nodes just for this.

So this patch series makes the ioctls available as regular ioctls as
well. In that case the pad field should be set to 0 and the bridge driver
will fill in the right pad value internally depending on the current
input or output and pass it along to the actual subdev driver.

Regards,

	Hans

[RFCv1 PATCH 1/4] v4l2: allow v4l2_subdev_edid to be used with video nodes

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Struct v4l2_subdev_edid and the VIDIOC_SUBDEV_G/S_EDID ioctls were
specific for subdevices, but for hardware with a simple video pipeline
you do not need/want to create subdevice nodes to just get/set the EDID.

Move the v4l2_subdev_edid struct to v4l2-common.h and rename as
v4l2_edid. Add the same ioctls to videodev2.h as well, thus allowing
this API to be used with both video nodes and v4l-subdev nodes.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/v4l2-common.h |  8 ++++++++
 include/uapi/linux/v4l2-subdev.h | 14 +++++---------
 include/uapi/linux/videodev2.h   |  2 ++
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 4f0667e..270db89 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -68,4 +68,12 @@
 #define V4L2_SUBDEV_SEL_FLAG_SIZE_LE	V4L2_SEL_FLAG_LE
 #define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG V4L2_SEL_FLAG_KEEP_CONFIG
 
+struct v4l2_edid {
+	__u32 pad;
+	__u32 start_block;
+	__u32 blocks;
+	__u32 reserved[5];
+	__u8 __user *edid;
+};
+
 #endif /* __V4L2_COMMON__ */
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index a33c4da..87e0515 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -148,13 +148,8 @@ struct v4l2_subdev_selection {
 	__u32 reserved[8];
 };
 
-struct v4l2_subdev_edid {
-	__u32 pad;
-	__u32 start_block;
-	__u32 blocks;
-	__u32 reserved[5];
-	__u8 __user *edid;
-};
+/* Backwards compatibility define --- to be removed */
+#define v4l2_subdev_edid v4l2_edid
 
 #define VIDIOC_SUBDEV_G_FMT	_IOWR('V',  4, struct v4l2_subdev_format)
 #define VIDIOC_SUBDEV_S_FMT	_IOWR('V',  5, struct v4l2_subdev_format)
@@ -174,7 +169,8 @@ struct v4l2_subdev_edid {
 	_IOWR('V', 61, struct v4l2_subdev_selection)
 #define VIDIOC_SUBDEV_S_SELECTION \
 	_IOWR('V', 62, struct v4l2_subdev_selection)
-#define VIDIOC_SUBDEV_G_EDID	_IOWR('V', 40, struct v4l2_subdev_edid)
-#define VIDIOC_SUBDEV_S_EDID	_IOWR('V', 41, struct v4l2_subdev_edid)
+/* These two G/S_EDID ioctls are identical to the ioctls in videodev2.h */
+#define VIDIOC_SUBDEV_G_EDID	_IOWR('V', 40, struct v4l2_edid)
+#define VIDIOC_SUBDEV_S_EDID	_IOWR('V', 41, struct v4l2_edid)
 
 #endif
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 6ae7bbe..55d976c 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1885,6 +1885,8 @@ struct v4l2_create_buffers {
 #define VIDIOC_QUERYMENU	_IOWR('V', 37, struct v4l2_querymenu)
 #define VIDIOC_G_INPUT		 _IOR('V', 38, int)
 #define VIDIOC_S_INPUT		_IOWR('V', 39, int)
+#define VIDIOC_G_EDID		_IOWR('V', 40, struct v4l2_edid)
+#define VIDIOC_S_EDID		_IOWR('V', 41, struct v4l2_edid)
 #define VIDIOC_G_OUTPUT		 _IOR('V', 46, int)
 #define VIDIOC_S_OUTPUT		_IOWR('V', 47, int)
 #define VIDIOC_ENUMOUTPUT	_IOWR('V', 48, struct v4l2_output)
-- 
1.8.4.rc3

[RFCv1 PATCH 2/4] v4l2: add VIDIOC_G/S_EDID support to the v4l2 core.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Support this ioctl as part of the v4l2 core. Use the new ioctl
name and struct v4l2_edid type in the existing core code.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 +++++++++++++--------------
 drivers/media/v4l2-core/v4l2-dev.c            |  2 ++
 drivers/media/v4l2-core/v4l2-ioctl.c          | 16 +++++++++++---
 drivers/media/v4l2-core/v4l2-subdev.c         |  4 ++--
 include/media/v4l2-ioctl.h                    |  2 ++
 include/media/v4l2-subdev.h                   |  4 ++--
 6 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 1b18616..6463c87 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -740,7 +740,7 @@ static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32 __user *u
 	return 0;
 }
 
-struct v4l2_subdev_edid32 {
+struct v4l2_edid32 {
 	__u32 pad;
 	__u32 start_block;
 	__u32 blocks;
@@ -748,11 +748,11 @@ struct v4l2_subdev_edid32 {
 	compat_caddr_t edid;
 };
 
-static int get_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subdev_edid32 __user *up)
+static int get_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
 {
 	u32 tmp;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_subdev_edid32)) ||
+	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_edid32)) ||
 		get_user(kp->pad, &up->pad) ||
 		get_user(kp->start_block, &up->start_block) ||
 		get_user(kp->blocks, &up->blocks) ||
@@ -763,11 +763,11 @@ static int get_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
 	return 0;
 }
 
-static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subdev_edid32 __user *up)
+static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
 {
 	u32 tmp = (u32)((unsigned long)kp->edid);
 
-	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_subdev_edid32)) ||
+	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_edid32)) ||
 		put_user(kp->pad, &up->pad) ||
 		put_user(kp->start_block, &up->start_block) ||
 		put_user(kp->blocks, &up->blocks) ||
@@ -787,8 +787,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
 #define VIDIOC_DQBUF32		_IOWR('V', 17, struct v4l2_buffer32)
 #define VIDIOC_ENUMSTD32	_IOWR('V', 25, struct v4l2_standard32)
 #define VIDIOC_ENUMINPUT32	_IOWR('V', 26, struct v4l2_input32)
-#define VIDIOC_SUBDEV_G_EDID32	_IOWR('V', 63, struct v4l2_subdev_edid32)
-#define VIDIOC_SUBDEV_S_EDID32	_IOWR('V', 64, struct v4l2_subdev_edid32)
+#define VIDIOC_G_EDID32		_IOWR('V', 63, struct v4l2_edid32)
+#define VIDIOC_S_EDID32		_IOWR('V', 64, struct v4l2_edid32)
 #define VIDIOC_TRY_FMT32      	_IOWR('V', 64, struct v4l2_format32)
 #define VIDIOC_G_EXT_CTRLS32    _IOWR('V', 71, struct v4l2_ext_controls32)
 #define VIDIOC_S_EXT_CTRLS32    _IOWR('V', 72, struct v4l2_ext_controls32)
@@ -816,7 +816,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		struct v4l2_ext_controls v2ecs;
 		struct v4l2_event v2ev;
 		struct v4l2_create_buffers v2crt;
-		struct v4l2_subdev_edid v2edid;
+		struct v4l2_edid v2edid;
 		unsigned long vx;
 		int vi;
 	} karg;
@@ -849,8 +849,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_S_OUTPUT32: cmd = VIDIOC_S_OUTPUT; break;
 	case VIDIOC_CREATE_BUFS32: cmd = VIDIOC_CREATE_BUFS; break;
 	case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
-	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
-	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
+	case VIDIOC_G_EDID32: cmd = VIDIOC_G_EDID; break;
+	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
 	}
 
 	switch (cmd) {
@@ -868,9 +868,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_SUBDEV_G_EDID:
-	case VIDIOC_SUBDEV_S_EDID:
-		err = get_v4l2_subdev_edid32(&karg.v2edid, up);
+	case VIDIOC_G_EDID:
+	case VIDIOC_S_EDID:
+		err = get_v4l2_edid32(&karg.v2edid, up);
 		compatible_arg = 0;
 		break;
 
@@ -966,9 +966,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		err = put_v4l2_event32(&karg.v2ev, up);
 		break;
 
-	case VIDIOC_SUBDEV_G_EDID:
-	case VIDIOC_SUBDEV_S_EDID:
-		err = put_v4l2_subdev_edid32(&karg.v2edid, up);
+	case VIDIOC_G_EDID:
+	case VIDIOC_S_EDID:
+		err = put_v4l2_edid32(&karg.v2edid, up);
 		break;
 
 	case VIDIOC_G_FMT:
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 0a30dbf..b61bc68 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -693,6 +693,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
 			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
 			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
+			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
 		}
 		if (ops->vidioc_g_crop || ops->vidioc_g_selection)
 			set_bit(_IOC_NR(VIDIOC_G_CROP), valid_ioctls);
@@ -710,6 +711,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_G_DV_TIMINGS, vidioc_g_dv_timings);
 		SET_VALID_IOCTL(ops, VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings);
 		SET_VALID_IOCTL(ops, VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap);
+		SET_VALID_IOCTL(ops, VIDIOC_G_EDID, vidioc_g_edid);
 	}
 	if (is_tx) {
 		/* transmitter only ioctls */
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 707aef7..26b4c5b 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -834,6 +834,14 @@ static void v4l_print_freq_band(const void *arg, bool write_only)
 			p->rangehigh, p->modulation);
 }
 
+static void v4l_print_edid(const void *arg, bool write_only)
+{
+	const struct v4l2_edid *p = arg;
+
+	pr_cont("pad=%u, start_block=%u, blocks=%u\n",
+			p->pad, p->start_block, p->blocks);
+}
+
 static void v4l_print_u32(const void *arg, bool write_only)
 {
 	pr_cont("value=%u\n", *(const u32 *)arg);
@@ -2009,6 +2017,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
 	IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
 	IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
+	IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid, INFO_FL_CLEAR(v4l2_edid, edid)),
+	IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_edid, edid)),
 	IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
 	IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
@@ -2221,9 +2231,9 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 		break;
 	}
 
-	case VIDIOC_SUBDEV_G_EDID:
-	case VIDIOC_SUBDEV_S_EDID: {
-		struct v4l2_subdev_edid *edid = parg;
+	case VIDIOC_G_EDID:
+	case VIDIOC_S_EDID: {
+		struct v4l2_edid *edid = parg;
 
 		if (edid->blocks) {
 			if (edid->blocks > 256) {
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 60d2550..aea84ac 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -349,10 +349,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 			sd, pad, set_selection, subdev_fh, sel);
 	}
 
-	case VIDIOC_SUBDEV_G_EDID:
+	case VIDIOC_G_EDID:
 		return v4l2_subdev_call(sd, pad, get_edid, arg);
 
-	case VIDIOC_SUBDEV_S_EDID:
+	case VIDIOC_S_EDID:
 		return v4l2_subdev_call(sd, pad, set_edid, arg);
 #endif
 	default:
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index e0b74a4..c362756 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -265,6 +265,8 @@ struct v4l2_ioctl_ops {
 				    struct v4l2_enum_dv_timings *timings);
 	int (*vidioc_dv_timings_cap) (struct file *file, void *fh,
 				    struct v4l2_dv_timings_cap *cap);
+	int (*vidioc_g_edid) (struct file *file, void *fh, struct v4l2_edid *edid);
+	int (*vidioc_s_edid) (struct file *file, void *fh, struct v4l2_edid *edid);
 
 	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
 					const struct v4l2_event_subscription *sub);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1752530..855c928 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -507,8 +507,8 @@ struct v4l2_subdev_pad_ops {
 			     struct v4l2_subdev_selection *sel);
 	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 			     struct v4l2_subdev_selection *sel);
-	int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid);
-	int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid);
+	int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
+	int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
 #ifdef CONFIG_MEDIA_CONTROLLER
 	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
 			     struct v4l2_subdev_format *source_fmt,
-- 
1.8.4.rc3

[RFCv1 PATCH 3/4] adv*: replace the deprecated v4l2_subdev_edid by v4l2_edid.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/ad9389b.c | 2 +-
 drivers/media/i2c/adv7511.c | 2 +-
 drivers/media/i2c/adv7604.c | 4 ++--
 drivers/media/i2c/adv7842.c | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ad9389b.c b/drivers/media/i2c/ad9389b.c
index 83225d6..1b7ecfd 100644
--- a/drivers/media/i2c/ad9389b.c
+++ b/drivers/media/i2c/ad9389b.c
@@ -573,7 +573,7 @@ static const struct v4l2_subdev_core_ops ad9389b_core_ops = {
 
 /* ------------------------------ PAD OPS ------------------------------ */
 
-static int ad9389b_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static int ad9389b_get_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 {
 	struct ad9389b_state *state = get_ad9389b_state(sd);
 
diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c
index ee61894..942ca4b 100644
--- a/drivers/media/i2c/adv7511.c
+++ b/drivers/media/i2c/adv7511.c
@@ -597,7 +597,7 @@ static int adv7511_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 	return 0;
 }
 
-static int adv7511_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static int adv7511_get_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 {
 	struct adv7511_state *state = get_adv7511_state(sd);
 
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 71c8570..98cc540 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1658,7 +1658,7 @@ static int adv7604_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 	return 0;
 }
 
-static int adv7604_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static int adv7604_get_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 {
 	struct adv7604_state *state = to_state(sd);
 	u8 *data = NULL;
@@ -1728,7 +1728,7 @@ static int get_edid_spa_location(const u8 *edid)
 	return -1;
 }
 
-static int adv7604_set_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static int adv7604_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 {
 	struct adv7604_state *state = to_state(sd);
 	int spa_loc;
diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index e04fe3f..4d1e07e 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -2014,7 +2014,7 @@ static int adv7842_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 	return 0;
 }
 
-static int adv7842_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static int adv7842_get_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 {
 	struct adv7842_state *state = to_state(sd);
 	u8 *data = NULL;
@@ -2054,7 +2054,7 @@ static int adv7842_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edi
 	return 0;
 }
 
-static int adv7842_set_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *e)
+static int adv7842_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *e)
 {
 	struct adv7842_state *state = to_state(sd);
 	int err = 0;
-- 
1.8.4.rc3

[RFCv1 PATCH 4/4] DocBook v4l2: update the G/S_EDID documentation

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document that it is now possible to call G/S_EDID from video nodes, not
just sub-device nodes.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/v4l2.xml           |  2 +-
 ...{vidioc-subdev-g-edid.xml => vidioc-g-edid.xml} | 30 +++++++++++++---------
 2 files changed, 19 insertions(+), 13 deletions(-)
 rename Documentation/DocBook/media/v4l/{vidioc-subdev-g-edid.xml => vidioc-g-edid.xml} (82%)

diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml
index 6bf3532..9a4aeb6 100644
--- a/Documentation/DocBook/media/v4l/v4l2.xml
+++ b/Documentation/DocBook/media/v4l/v4l2.xml
@@ -596,6 +596,7 @@ and discussions on the V4L mailing list.</revremark>
     &sub-g-crop;
     &sub-g-ctrl;
     &sub-g-dv-timings;
+    &sub-g-edid;
     &sub-g-enc-index;
     &sub-g-ext-ctrls;
     &sub-g-fbuf;
@@ -627,7 +628,6 @@ and discussions on the V4L mailing list.</revremark>
     &sub-subdev-enum-frame-size;
     &sub-subdev-enum-mbus-code;
     &sub-subdev-g-crop;
-    &sub-subdev-g-edid;
     &sub-subdev-g-fmt;
     &sub-subdev-g-frame-interval;
     &sub-subdev-g-selection;
diff --git a/Documentation/DocBook/media/v4l/vidioc-subdev-g-edid.xml b/Documentation/DocBook/media/v4l/vidioc-g-edid.xml
similarity index 82%
rename from Documentation/DocBook/media/v4l/vidioc-subdev-g-edid.xml
rename to Documentation/DocBook/media/v4l/vidioc-g-edid.xml
index bbd18f0..e04f39f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-subdev-g-edid.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-g-edid.xml
@@ -1,12 +1,12 @@
-<refentry id="vidioc-subdev-g-edid">
+<refentry id="vidioc-g-edid">
   <refmeta>
-    <refentrytitle>ioctl VIDIOC_SUBDEV_G_EDID, VIDIOC_SUBDEV_S_EDID</refentrytitle>
+    <refentrytitle>ioctl VIDIOC_G_EDID, VIDIOC_S_EDID</refentrytitle>
     &manvol;
   </refmeta>
 
   <refnamediv>
-    <refname>VIDIOC_SUBDEV_G_EDID</refname>
-    <refname>VIDIOC_SUBDEV_S_EDID</refname>
+    <refname>VIDIOC_G_EDID</refname>
+    <refname>VIDIOC_S_EDID</refname>
     <refpurpose>Get or set the EDID of a video receiver/transmitter</refpurpose>
   </refnamediv>
 
@@ -16,7 +16,7 @@
 	<funcdef>int <function>ioctl</function></funcdef>
 	<paramdef>int <parameter>fd</parameter></paramdef>
 	<paramdef>int <parameter>request</parameter></paramdef>
-	<paramdef>struct v4l2_subdev_edid *<parameter>argp</parameter></paramdef>
+	<paramdef>struct v4l2_edid *<parameter>argp</parameter></paramdef>
       </funcprototype>
     </funcsynopsis>
     <funcsynopsis>
@@ -24,7 +24,7 @@
 	<funcdef>int <function>ioctl</function></funcdef>
 	<paramdef>int <parameter>fd</parameter></paramdef>
 	<paramdef>int <parameter>request</parameter></paramdef>
-	<paramdef>const struct v4l2_subdev_edid *<parameter>argp</parameter></paramdef>
+	<paramdef>const struct v4l2_edid *<parameter>argp</parameter></paramdef>
       </funcprototype>
     </funcsynopsis>
   </refsynopsisdiv>
@@ -42,7 +42,7 @@
       <varlistentry>
 	<term><parameter>request</parameter></term>
 	<listitem>
-	  <para>VIDIOC_SUBDEV_G_EDID, VIDIOC_SUBDEV_S_EDID</para>
+	  <para>VIDIOC_G_EDID, VIDIOC_S_EDID</para>
 	</listitem>
       </varlistentry>
       <varlistentry>
@@ -57,11 +57,16 @@
   <refsect1>
     <title>Description</title>
     <para>These ioctls can be used to get or set an EDID associated with an input pad
-    from a receiver or an output pad of a transmitter subdevice.</para>
+    from a receiver or an output pad of a transmitter subdevice. These ioctls can be
+    used with subdevice nodes or with video nodes. A video node should only support
+    these ioctls if it is unambiguous which receiver or transmitter is referred to.
+    This will typically be based on the current input or output.
+    When used with video nodes the <structfield>pad</structfield> field shall be set
+    to 0, otherwise &EINVAL; will be returned.</para>
 
     <para>To get the EDID data the application has to fill in the <structfield>pad</structfield>,
     <structfield>start_block</structfield>, <structfield>blocks</structfield> and <structfield>edid</structfield>
-    fields and call <constant>VIDIOC_SUBDEV_G_EDID</constant>. The current EDID from block
+    fields and call <constant>VIDIOC_G_EDID</constant>. The current EDID from block
     <structfield>start_block</structfield> and of size <structfield>blocks</structfield>
     will be placed in the memory <structfield>edid</structfield> points to. The <structfield>edid</structfield>
     pointer must point to memory at least <structfield>blocks</structfield>&nbsp;*&nbsp;128 bytes
@@ -91,15 +96,16 @@
     data in some way. In any case, the end result is the same: the EDID is no longer available.
     </para>
 
-    <table pgwide="1" frame="none" id="v4l2-subdev-edid">
-      <title>struct <structname>v4l2_subdev_edid</structname></title>
+    <table pgwide="1" frame="none" id="v4l2-edid">
+      <title>struct <structname>v4l2_edid</structname></title>
       <tgroup cols="3">
         &cs-str;
 	<tbody valign="top">
 	  <row>
 	    <entry>__u32</entry>
 	    <entry><structfield>pad</structfield></entry>
-	    <entry>Pad for which to get/set the EDID blocks.</entry>
+	    <entry>Pad for which to get/set the EDID blocks. Must be 0 when using these ioctls
+	    with a video device node.</entry>
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
-- 
1.8.4.rc3

Re: [RFCv1 PATCH 2/4] v4l2: add VIDIOC_G/S_EDID support to the v4l2 core.

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday 04 March 2014 12:30:57 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Support this ioctl as part of the v4l2 core. Use the new ioctl
> name and struct v4l2_edid type in the existing core code.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 ++++++++++++------------
>  drivers/media/v4l2-core/v4l2-dev.c            |  2 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 16 +++++++++++---
>  drivers/media/v4l2-core/v4l2-subdev.c         |  4 ++--
>  include/media/v4l2-ioctl.h                    |  2 ++
>  include/media/v4l2-subdev.h                   |  4 ++--
>  6 files changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 1b18616..6463c87
> 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -740,7 +740,7 @@ static int put_v4l2_event32(struct v4l2_event *kp,
> struct v4l2_event32 __user *u return 0;
>  }
> 
> -struct v4l2_subdev_edid32 {
> +struct v4l2_edid32 {
>  	__u32 pad;
>  	__u32 start_block;
>  	__u32 blocks;
> @@ -748,11 +748,11 @@ struct v4l2_subdev_edid32 {
>  	compat_caddr_t edid;
>  };
> 
> -static int get_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct
> v4l2_subdev_edid32 __user *up) +static int get_v4l2_edid32(struct v4l2_edid
> *kp, struct v4l2_edid32 __user *up) {
>  	u32 tmp;
> 
> -	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_subdev_edid32)) ||
> +	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_edid32)) ||
>  		get_user(kp->pad, &up->pad) ||
>  		get_user(kp->start_block, &up->start_block) ||
>  		get_user(kp->blocks, &up->blocks) ||
> @@ -763,11 +763,11 @@ static int get_v4l2_subdev_edid32(struct
> v4l2_subdev_edid *kp, struct v4l2_subde return 0;
>  }
> 
> -static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct
> v4l2_subdev_edid32 __user *up) +static int put_v4l2_edid32(struct v4l2_edid
> *kp, struct v4l2_edid32 __user *up) {
>  	u32 tmp = (u32)((unsigned long)kp->edid);
> 
> -	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_subdev_edid32)) ||
> +	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_edid32)) ||
>  		put_user(kp->pad, &up->pad) ||
>  		put_user(kp->start_block, &up->start_block) ||
>  		put_user(kp->blocks, &up->blocks) ||
> @@ -787,8 +787,8 @@ static int put_v4l2_subdev_edid32(struct
> v4l2_subdev_edid *kp, struct v4l2_subde #define VIDIOC_DQBUF32		
_IOWR('V',
> 17, struct v4l2_buffer32)
>  #define VIDIOC_ENUMSTD32	_IOWR('V', 25, struct v4l2_standard32)
>  #define VIDIOC_ENUMINPUT32	_IOWR('V', 26, struct v4l2_input32)
> -#define VIDIOC_SUBDEV_G_EDID32	_IOWR('V', 63, struct v4l2_subdev_edid32)
> -#define VIDIOC_SUBDEV_S_EDID32	_IOWR('V', 64, struct v4l2_subdev_edid32)
> +#define VIDIOC_G_EDID32		_IOWR('V', 63, struct v4l2_edid32)
> +#define VIDIOC_S_EDID32		_IOWR('V', 64, struct v4l2_edid32)

Shouldn't the ioctl numbers be 40 and 41 ?

>  #define VIDIOC_TRY_FMT32      	_IOWR('V', 64, struct v4l2_format32)
>  #define VIDIOC_G_EXT_CTRLS32    _IOWR('V', 71, struct v4l2_ext_controls32)
>  #define VIDIOC_S_EXT_CTRLS32    _IOWR('V', 72, struct v4l2_ext_controls32)
> @@ -816,7 +816,7 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar struct v4l2_ext_controls v2ecs;
>  		struct v4l2_event v2ev;
>  		struct v4l2_create_buffers v2crt;
> -		struct v4l2_subdev_edid v2edid;
> +		struct v4l2_edid v2edid;
>  		unsigned long vx;
>  		int vi;
>  	} karg;
> @@ -849,8 +849,8 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar case VIDIOC_S_OUTPUT32: cmd = VIDIOC_S_OUTPUT;
> break;
>  	case VIDIOC_CREATE_BUFS32: cmd = VIDIOC_CREATE_BUFS; break;
>  	case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
> -	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
> -	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
> +	case VIDIOC_G_EDID32: cmd = VIDIOC_G_EDID; break;
> +	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
>  	}
> 
>  	switch (cmd) {
> @@ -868,9 +868,9 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar compatible_arg = 0;
>  		break;
> 
> -	case VIDIOC_SUBDEV_G_EDID:
> -	case VIDIOC_SUBDEV_S_EDID:
> -		err = get_v4l2_subdev_edid32(&karg.v2edid, up);
> +	case VIDIOC_G_EDID:
> +	case VIDIOC_S_EDID:
> +		err = get_v4l2_edid32(&karg.v2edid, up);
>  		compatible_arg = 0;
>  		break;
> 
> @@ -966,9 +966,9 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar err = put_v4l2_event32(&karg.v2ev, up);
>  		break;
> 
> -	case VIDIOC_SUBDEV_G_EDID:
> -	case VIDIOC_SUBDEV_S_EDID:
> -		err = put_v4l2_subdev_edid32(&karg.v2edid, up);
> +	case VIDIOC_G_EDID:
> +	case VIDIOC_S_EDID:
> +		err = put_v4l2_edid32(&karg.v2edid, up);
>  		break;
> 
>  	case VIDIOC_G_FMT:
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c index 0a30dbf..b61bc68 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -693,6 +693,7 @@ static void determine_valid_ioctls(struct video_device
> *vdev) SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
> +			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);

Shouldn't VIDIOC_S_EDID be enabled for rx devices, not tx devices ?

>  		}
>  		if (ops->vidioc_g_crop || ops->vidioc_g_selection)
>  			set_bit(_IOC_NR(VIDIOC_G_CROP), valid_ioctls);
> @@ -710,6 +711,7 @@ static void determine_valid_ioctls(struct video_device
> *vdev) SET_VALID_IOCTL(ops, VIDIOC_G_DV_TIMINGS, vidioc_g_dv_timings);
> SET_VALID_IOCTL(ops, VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings);
> SET_VALID_IOCTL(ops, VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap);
> +		SET_VALID_IOCTL(ops, VIDIOC_G_EDID, vidioc_g_edid);
>  	}
>  	if (is_tx) {
>  		/* transmitter only ioctls */
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index 707aef7..26b4c5b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -834,6 +834,14 @@ static void v4l_print_freq_band(const void *arg, bool
> write_only) p->rangehigh, p->modulation);
>  }
> 
> +static void v4l_print_edid(const void *arg, bool write_only)
> +{
> +	const struct v4l2_edid *p = arg;
> +
> +	pr_cont("pad=%u, start_block=%u, blocks=%u\n",
> +			p->pad, p->start_block, p->blocks);

Extra tab ?

> +}
> +
>  static void v4l_print_u32(const void *arg, bool write_only)
>  {
>  	pr_cont("value=%u\n", *(const u32 *)arg);
> @@ -2009,6 +2017,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu,
> INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
> IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
> IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
> +	IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid,
> INFO_FL_CLEAR(v4l2_edid, edid)),
> +	IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, INFO_FL_PRIO
> | INFO_FL_CLEAR(v4l2_edid, edid)),
> IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
> IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
> IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput,
> INFO_FL_CLEAR(v4l2_output, index)),
> @@ -2221,9 +2231,9 @@ static int
> check_array_args(unsigned int cmd, void *parg, size_t *array_size, break;
>  	}
> 
> -	case VIDIOC_SUBDEV_G_EDID:
> -	case VIDIOC_SUBDEV_S_EDID: {
> -		struct v4l2_subdev_edid *edid = parg;
> +	case VIDIOC_G_EDID:
> +	case VIDIOC_S_EDID: {
> +		struct v4l2_edid *edid = parg;
> 
>  		if (edid->blocks) {
>  			if (edid->blocks > 256) {
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> b/drivers/media/v4l2-core/v4l2-subdev.c index 60d2550..aea84ac 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -349,10 +349,10 @@ static long subdev_do_ioctl(struct file *file,
> unsigned int cmd, void *arg) sd, pad, set_selection, subdev_fh, sel);
>  	}
> 
> -	case VIDIOC_SUBDEV_G_EDID:
> +	case VIDIOC_G_EDID:
>  		return v4l2_subdev_call(sd, pad, get_edid, arg);
> 
> -	case VIDIOC_SUBDEV_S_EDID:
> +	case VIDIOC_S_EDID:
>  		return v4l2_subdev_call(sd, pad, set_edid, arg);
>  #endif
>  	default:
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e0b74a4..c362756 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -265,6 +265,8 @@ struct v4l2_ioctl_ops {
>  				    struct v4l2_enum_dv_timings *timings);
>  	int (*vidioc_dv_timings_cap) (struct file *file, void *fh,
>  				    struct v4l2_dv_timings_cap *cap);
> +	int (*vidioc_g_edid) (struct file *file, void *fh, struct v4l2_edid
> *edid);
> +	int (*vidioc_s_edid) (struct file *file, void *fh, struct v4l2_edid
> *edid);
> 
>  	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
>  					const struct v4l2_event_subscription *sub);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1752530..855c928 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -507,8 +507,8 @@ struct v4l2_subdev_pad_ops {
>  			     struct v4l2_subdev_selection *sel);
>  	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>  			     struct v4l2_subdev_selection *sel);
> -	int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid);
> -	int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid);
> +	int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
> +	int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
>  			     struct v4l2_subdev_format *source_fmt,

-- 
Regards,

Laurent Pinchart

Re: [RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[Laurent Pinchart]

Hi Hans,

Thank you for the patches.

On Tuesday 04 March 2014 12:30:55 Hans Verkuil wrote:
> Currently the VIDIOC_SUBDEV_G/S_EDID and struct v4l2_subdev_edid are subdev
> APIs. However, that's in reality quite annoying since for simple video
> pipelines there is no need to create v4l-subdev device nodes for anything
> else except for setting or getting EDIDs.
> 
> What happens in practice is that v4l2 bridge drivers add explicit support
> for VIDIOC_SUBDEV_G/S_EDID themselves, just to avoid having to create
> subdev device nodes just for this.
> 
> So this patch series makes the ioctls available as regular ioctls as
> well. In that case the pad field should be set to 0 and the bridge driver
> will fill in the right pad value internally depending on the current
> input or output and pass it along to the actual subdev driver.

Would it make sense to allow usage of the pad field on video nodes as well ?

Apart from that and minor issues with patch 2/4 this series looks good to me.

-- 
Regards,

Laurent Pinchart

Re: [RFCv1 PATCH 2/4] v4l2: add VIDIOC_G/S_EDID support to the v4l2 core.

[Hans Verkuil]

Hi Laurent,

Thank you for your comments!

On 03/06/2014 02:41 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tuesday 04 March 2014 12:30:57 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Support this ioctl as part of the v4l2 core. Use the new ioctl
>> name and struct v4l2_edid type in the existing core code.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 ++++++++++++------------
>>  drivers/media/v4l2-core/v4l2-dev.c            |  2 ++
>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 16 +++++++++++---
>>  drivers/media/v4l2-core/v4l2-subdev.c         |  4 ++--
>>  include/media/v4l2-ioctl.h                    |  2 ++
>>  include/media/v4l2-subdev.h                   |  4 ++--
>>  6 files changed, 37 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 1b18616..6463c87
>> 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -740,7 +740,7 @@ static int put_v4l2_event32(struct v4l2_event *kp,
>> struct v4l2_event32 __user *u return 0;
>>  }
>>
>> -struct v4l2_subdev_edid32 {
>> +struct v4l2_edid32 {
>>  	__u32 pad;
>>  	__u32 start_block;
>>  	__u32 blocks;
>> @@ -748,11 +748,11 @@ struct v4l2_subdev_edid32 {
>>  	compat_caddr_t edid;
>>  };
>>
>> -static int get_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct
>> v4l2_subdev_edid32 __user *up) +static int get_v4l2_edid32(struct v4l2_edid
>> *kp, struct v4l2_edid32 __user *up) {
>>  	u32 tmp;
>>
>> -	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_subdev_edid32)) ||
>> +	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_edid32)) ||
>>  		get_user(kp->pad, &up->pad) ||
>>  		get_user(kp->start_block, &up->start_block) ||
>>  		get_user(kp->blocks, &up->blocks) ||
>> @@ -763,11 +763,11 @@ static int get_v4l2_subdev_edid32(struct
>> v4l2_subdev_edid *kp, struct v4l2_subde return 0;
>>  }
>>
>> -static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct
>> v4l2_subdev_edid32 __user *up) +static int put_v4l2_edid32(struct v4l2_edid
>> *kp, struct v4l2_edid32 __user *up) {
>>  	u32 tmp = (u32)((unsigned long)kp->edid);
>>
>> -	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_subdev_edid32)) ||
>> +	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_edid32)) ||
>>  		put_user(kp->pad, &up->pad) ||
>>  		put_user(kp->start_block, &up->start_block) ||
>>  		put_user(kp->blocks, &up->blocks) ||
>> @@ -787,8 +787,8 @@ static int put_v4l2_subdev_edid32(struct
>> v4l2_subdev_edid *kp, struct v4l2_subde #define VIDIOC_DQBUF32		
> _IOWR('V',
>> 17, struct v4l2_buffer32)
>>  #define VIDIOC_ENUMSTD32	_IOWR('V', 25, struct v4l2_standard32)
>>  #define VIDIOC_ENUMINPUT32	_IOWR('V', 26, struct v4l2_input32)
>> -#define VIDIOC_SUBDEV_G_EDID32	_IOWR('V', 63, struct v4l2_subdev_edid32)
>> -#define VIDIOC_SUBDEV_S_EDID32	_IOWR('V', 64, struct v4l2_subdev_edid32)
>> +#define VIDIOC_G_EDID32		_IOWR('V', 63, struct v4l2_edid32)
>> +#define VIDIOC_S_EDID32		_IOWR('V', 64, struct v4l2_edid32)
> 
> Shouldn't the ioctl numbers be 40 and 41 ?

Oh, for crying out loud. That's been wrong since these ioctls were first added
in 3.7. I'll split this off as a patch for 3.14 with a CC to stable.

Well spotted!

> 
>>  #define VIDIOC_TRY_FMT32      	_IOWR('V', 64, struct v4l2_format32)
>>  #define VIDIOC_G_EXT_CTRLS32    _IOWR('V', 71, struct v4l2_ext_controls32)
>>  #define VIDIOC_S_EXT_CTRLS32    _IOWR('V', 72, struct v4l2_ext_controls32)
>> @@ -816,7 +816,7 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar struct v4l2_ext_controls v2ecs;
>>  		struct v4l2_event v2ev;
>>  		struct v4l2_create_buffers v2crt;
>> -		struct v4l2_subdev_edid v2edid;
>> +		struct v4l2_edid v2edid;
>>  		unsigned long vx;
>>  		int vi;
>>  	} karg;
>> @@ -849,8 +849,8 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar case VIDIOC_S_OUTPUT32: cmd = VIDIOC_S_OUTPUT;
>> break;
>>  	case VIDIOC_CREATE_BUFS32: cmd = VIDIOC_CREATE_BUFS; break;
>>  	case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
>> -	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
>> -	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
>> +	case VIDIOC_G_EDID32: cmd = VIDIOC_G_EDID; break;
>> +	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
>>  	}
>>
>>  	switch (cmd) {
>> @@ -868,9 +868,9 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar compatible_arg = 0;
>>  		break;
>>
>> -	case VIDIOC_SUBDEV_G_EDID:
>> -	case VIDIOC_SUBDEV_S_EDID:
>> -		err = get_v4l2_subdev_edid32(&karg.v2edid, up);
>> +	case VIDIOC_G_EDID:
>> +	case VIDIOC_S_EDID:
>> +		err = get_v4l2_edid32(&karg.v2edid, up);
>>  		compatible_arg = 0;
>>  		break;
>>
>> @@ -966,9 +966,9 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar err = put_v4l2_event32(&karg.v2ev, up);
>>  		break;
>>
>> -	case VIDIOC_SUBDEV_G_EDID:
>> -	case VIDIOC_SUBDEV_S_EDID:
>> -		err = put_v4l2_subdev_edid32(&karg.v2edid, up);
>> +	case VIDIOC_G_EDID:
>> +	case VIDIOC_S_EDID:
>> +		err = put_v4l2_edid32(&karg.v2edid, up);
>>  		break;
>>
>>  	case VIDIOC_G_FMT:
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
>> b/drivers/media/v4l2-core/v4l2-dev.c index 0a30dbf..b61bc68 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -693,6 +693,7 @@ static void determine_valid_ioctls(struct video_device
>> *vdev) SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
>> +			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> 
> Shouldn't VIDIOC_S_EDID be enabled for rx devices, not tx devices ?

Yes indeed. That's embarrassing...

> 
>>  		}
>>  		if (ops->vidioc_g_crop || ops->vidioc_g_selection)
>>  			set_bit(_IOC_NR(VIDIOC_G_CROP), valid_ioctls);
>> @@ -710,6 +711,7 @@ static void determine_valid_ioctls(struct video_device
>> *vdev) SET_VALID_IOCTL(ops, VIDIOC_G_DV_TIMINGS, vidioc_g_dv_timings);
>> SET_VALID_IOCTL(ops, VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings);
>> SET_VALID_IOCTL(ops, VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap);
>> +		SET_VALID_IOCTL(ops, VIDIOC_G_EDID, vidioc_g_edid);
>>  	}
>>  	if (is_tx) {
>>  		/* transmitter only ioctls */
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 707aef7..26b4c5b 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -834,6 +834,14 @@ static void v4l_print_freq_band(const void *arg, bool
>> write_only) p->rangehigh, p->modulation);
>>  }
>>
>> +static void v4l_print_edid(const void *arg, bool write_only)
>> +{
>> +	const struct v4l2_edid *p = arg;
>> +
>> +	pr_cont("pad=%u, start_block=%u, blocks=%u\n",
>> +			p->pad, p->start_block, p->blocks);
> 
> Extra tab ?

I'll remove it.

> 
>> +}
>> +
>>  static void v4l_print_u32(const void *arg, bool write_only)
>>  {
>>  	pr_cont("value=%u\n", *(const u32 *)arg);
>> @@ -2009,6 +2017,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>  	IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu,
>> INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
>> IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
>> IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
>> +	IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid,
>> INFO_FL_CLEAR(v4l2_edid, edid)),
>> +	IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, INFO_FL_PRIO
>> | INFO_FL_CLEAR(v4l2_edid, edid)),
>> IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
>> IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
>> IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput,
>> INFO_FL_CLEAR(v4l2_output, index)),
>> @@ -2221,9 +2231,9 @@ static int
>> check_array_args(unsigned int cmd, void *parg, size_t *array_size, break;
>>  	}
>>
>> -	case VIDIOC_SUBDEV_G_EDID:
>> -	case VIDIOC_SUBDEV_S_EDID: {
>> -		struct v4l2_subdev_edid *edid = parg;
>> +	case VIDIOC_G_EDID:
>> +	case VIDIOC_S_EDID: {
>> +		struct v4l2_edid *edid = parg;
>>
>>  		if (edid->blocks) {
>>  			if (edid->blocks > 256) {
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>> b/drivers/media/v4l2-core/v4l2-subdev.c index 60d2550..aea84ac 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -349,10 +349,10 @@ static long subdev_do_ioctl(struct file *file,
>> unsigned int cmd, void *arg) sd, pad, set_selection, subdev_fh, sel);
>>  	}
>>
>> -	case VIDIOC_SUBDEV_G_EDID:
>> +	case VIDIOC_G_EDID:
>>  		return v4l2_subdev_call(sd, pad, get_edid, arg);
>>
>> -	case VIDIOC_SUBDEV_S_EDID:
>> +	case VIDIOC_S_EDID:
>>  		return v4l2_subdev_call(sd, pad, set_edid, arg);
>>  #endif
>>  	default:
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index e0b74a4..c362756 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -265,6 +265,8 @@ struct v4l2_ioctl_ops {
>>  				    struct v4l2_enum_dv_timings *timings);
>>  	int (*vidioc_dv_timings_cap) (struct file *file, void *fh,
>>  				    struct v4l2_dv_timings_cap *cap);
>> +	int (*vidioc_g_edid) (struct file *file, void *fh, struct v4l2_edid
>> *edid);
>> +	int (*vidioc_s_edid) (struct file *file, void *fh, struct v4l2_edid
>> *edid);
>>
>>  	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
>>  					const struct v4l2_event_subscription *sub);
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 1752530..855c928 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -507,8 +507,8 @@ struct v4l2_subdev_pad_ops {
>>  			     struct v4l2_subdev_selection *sel);
>>  	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>>  			     struct v4l2_subdev_selection *sel);
>> -	int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid);
>> -	int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid);
>> +	int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
>> +	int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>  	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
>>  			     struct v4l2_subdev_format *source_fmt,
> 

Thanks!

	Hans

Re: [RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[Hans Verkuil]

On 03/06/2014 02:45 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patches.
> 
> On Tuesday 04 March 2014 12:30:55 Hans Verkuil wrote:
>> Currently the VIDIOC_SUBDEV_G/S_EDID and struct v4l2_subdev_edid are subdev
>> APIs. However, that's in reality quite annoying since for simple video
>> pipelines there is no need to create v4l-subdev device nodes for anything
>> else except for setting or getting EDIDs.
>>
>> What happens in practice is that v4l2 bridge drivers add explicit support
>> for VIDIOC_SUBDEV_G/S_EDID themselves, just to avoid having to create
>> subdev device nodes just for this.
>>
>> So this patch series makes the ioctls available as regular ioctls as
>> well. In that case the pad field should be set to 0 and the bridge driver
>> will fill in the right pad value internally depending on the current
>> input or output and pass it along to the actual subdev driver.
> 
> Would it make sense to allow usage of the pad field on video nodes as well ?

No, really not. The video node driver has full control over which inputs/outputs
map to which pads. None of that is (or should be) visible from userspace.

What should probably change is that rather than requiring userspace to set pad
to 0, I just say that it is ignored. The bridge driver will fill in the pad
before handing it over to the relevant subdev. Requiring apps to set it to 0
(which is a valid pad number anyway, so that doesn't really help with possible
future use of the field) will require the driver to set it to 0 as well after
having called the subdev. Which is annoying and will be forgotten anyway.

I also need to mention in the docbook that if it is called for a pad, an input
or an output that does not support EDIDs these ioctls will return -EINVAL.

I'll post a REVIEWv1 series soon.

Regards,

	Hans

> 
> Apart from that and minor issues with patch 2/4 this series looks good to me.
> 

Re: [RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[Laurent Pinchart]

Hi Hans,

On Thursday 06 March 2014 08:35:07 Hans Verkuil wrote:
> On 03/06/2014 02:45 AM, Laurent Pinchart wrote:
> > On Tuesday 04 March 2014 12:30:55 Hans Verkuil wrote:
> >> Currently the VIDIOC_SUBDEV_G/S_EDID and struct v4l2_subdev_edid are
> >> subdev APIs. However, that's in reality quite annoying since for simple
> >> video pipelines there is no need to create v4l-subdev device nodes for
> >> anything else except for setting or getting EDIDs.
> >> 
> >> What happens in practice is that v4l2 bridge drivers add explicit support
> >> for VIDIOC_SUBDEV_G/S_EDID themselves, just to avoid having to create
> >> subdev device nodes just for this.
> >> 
> >> So this patch series makes the ioctls available as regular ioctls as
> >> well. In that case the pad field should be set to 0 and the bridge driver
> >> will fill in the right pad value internally depending on the current
> >> input or output and pass it along to the actual subdev driver.
> > 
> > Would it make sense to allow usage of the pad field on video nodes as well
> > ?
> No, really not. The video node driver has full control over which
> inputs/outputs map to which pads. None of that is (or should be) visible
> from userspace.

What about using the pad field as an input number in that case ? That would 
allow getting and setting EDID for the different inputs without requiring to 
change the active input, just like we can do with the subdev API.

> What should probably change is that rather than requiring userspace to set
> pad to 0, I just say that it is ignored. The bridge driver will fill in the
> pad before handing it over to the relevant subdev. Requiring apps to set it
> to 0 (which is a valid pad number anyway, so that doesn't really help with
> possible future use of the field) will require the driver to set it to 0 as
> well after having called the subdev. Which is annoying and will be
> forgotten anyway.
> 
> I also need to mention in the docbook that if it is called for a pad, an
> input or an output that does not support EDIDs these ioctls will return
> -EINVAL.
> 
> I'll post a REVIEWv1 series soon.
> 
> Regards,
> 
> 	Hans
> 
> > Apart from that and minor issues with patch 2/4 this series looks good to
> > me.

-- 
Regards,

Laurent Pinchart

Re: [RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[Hans Verkuil]

On 03/06/14 11:37, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 06 March 2014 08:35:07 Hans Verkuil wrote:
>> On 03/06/2014 02:45 AM, Laurent Pinchart wrote:
>>> On Tuesday 04 March 2014 12:30:55 Hans Verkuil wrote:
>>>> Currently the VIDIOC_SUBDEV_G/S_EDID and struct v4l2_subdev_edid are
>>>> subdev APIs. However, that's in reality quite annoying since for simple
>>>> video pipelines there is no need to create v4l-subdev device nodes for
>>>> anything else except for setting or getting EDIDs.
>>>>
>>>> What happens in practice is that v4l2 bridge drivers add explicit support
>>>> for VIDIOC_SUBDEV_G/S_EDID themselves, just to avoid having to create
>>>> subdev device nodes just for this.
>>>>
>>>> So this patch series makes the ioctls available as regular ioctls as
>>>> well. In that case the pad field should be set to 0 and the bridge driver
>>>> will fill in the right pad value internally depending on the current
>>>> input or output and pass it along to the actual subdev driver.
>>>
>>> Would it make sense to allow usage of the pad field on video nodes as well
>>> ?
>> No, really not. The video node driver has full control over which
>> inputs/outputs map to which pads. None of that is (or should be) visible
>> from userspace.
> 
> What about using the pad field as an input number in that case ? That would 
> allow getting and setting EDID for the different inputs without requiring to 
> change the active input, just like we can do with the subdev API.

That's a good idea. How should I do that with the v4l2_edid struct: just
expect userspace to fill in the pad but interpret it differently, or change
it to a union:

	union {
		__u32 pad;
		__u32 input;
		__u32 output;
	};

Regards,

	Hans

> 
>> What should probably change is that rather than requiring userspace to set
>> pad to 0, I just say that it is ignored. The bridge driver will fill in the
>> pad before handing it over to the relevant subdev. Requiring apps to set it
>> to 0 (which is a valid pad number anyway, so that doesn't really help with
>> possible future use of the field) will require the driver to set it to 0 as
>> well after having called the subdev. Which is annoying and will be
>> forgotten anyway.
>>
>> I also need to mention in the docbook that if it is called for a pad, an
>> input or an output that does not support EDIDs these ioctls will return
>> -EINVAL.
>>
>> I'll post a REVIEWv1 series soon.
>>
>> Regards,
>>
>> 	Hans
>>
>>> Apart from that and minor issues with patch 2/4 this series looks good to
>>> me.
> 

Re: [RFCv1 PATCH 0/4] add G/S_EDID support for video nodes

[Laurent Pinchart]

Hi Hans,

On Thursday 06 March 2014 13:58:52 Hans Verkuil wrote:
> On 03/06/14 11:37, Laurent Pinchart wrote:
> > On Thursday 06 March 2014 08:35:07 Hans Verkuil wrote:
> >> On 03/06/2014 02:45 AM, Laurent Pinchart wrote:
> >>> On Tuesday 04 March 2014 12:30:55 Hans Verkuil wrote:
> >>>> Currently the VIDIOC_SUBDEV_G/S_EDID and struct v4l2_subdev_edid are
> >>>> subdev APIs. However, that's in reality quite annoying since for simple
> >>>> video pipelines there is no need to create v4l-subdev device nodes for
> >>>> anything else except for setting or getting EDIDs.
> >>>> 
> >>>> What happens in practice is that v4l2 bridge drivers add explicit
> >>>> support for VIDIOC_SUBDEV_G/S_EDID themselves, just to avoid having to
> >>>> create subdev device nodes just for this.
> >>>> 
> >>>> So this patch series makes the ioctls available as regular ioctls as
> >>>> well. In that case the pad field should be set to 0 and the bridge
> >>>> driver will fill in the right pad value internally depending on the
> >>>> current input or output and pass it along to the actual subdev driver.
> >>> 
> >>> Would it make sense to allow usage of the pad field on video nodes as
> >>> well ?
> >> 
> >> No, really not. The video node driver has full control over which
> >> inputs/outputs map to which pads. None of that is (or should be) visible
> >> from userspace.
> > 
> > What about using the pad field as an input number in that case ? That
> > would allow getting and setting EDID for the different inputs without
> > requiring to change the active input, just like we can do with the subdev
> > API.
> 
> That's a good idea. How should I do that with the v4l2_edid struct: just
> expect userspace to fill in the pad but interpret it differently, or change
> it to a union:
> 
> 	union {
> 		__u32 pad;
> 		__u32 input;
> 		__u32 output;
> 	};

I think I woul djust document the pad field as being used as an input or 
output number in that case, to keep the code simpler, but I'm fine with a 
union if you find it cleaner.

-- 
Regards,

Laurent Pinchart