[RFC v3 00/19] New ioct VIDIOC_G_DEF_EXT_CTRLS

[RFC v3 00/19] New ioct VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

Integer controls provide a way to get their default/initial value, but
any other control (p_u32, p_u8.....) provide no other way to get the
initial value than unloading the module and loading it back.

*What is the actual problem?
I have a custom control with WIDTH integer values. Every value
represents the calibrated FPN (fixed pattern noise) correction value for that
column
-Application A changes the FPN correction value
-Application B wants to restore the calibrated value but it cant :(

*What is the proposed solution?
-Add a new ioctl VIDIOC_G_DEF_EXT_CTRLS, with the same API as
G_EXT_CTRLS, but that returns the initial value of a given control.


I have posted a copy of my working tree to

https://github.com/ribalda/linux/tree/g_def_ext-rfc3

It has been tested with a hacked version of yavta (for normal controls) and a
custom program for the array control.

Changelog v3:
-Comments by Hans Verkuil:
-Remove the control ops from the following drivers
saa7706
ivtv-gpio
wm8739
tvp7002
tvp514x
tvl320aic23b
tda7432
sr030pc30
saa717x
cs5345
adv7393
adv7343

Changelog v2:
-Add documentation
-Split in multiple patches
-Comments by Hans Verkuil:
-Rename ioctl to G_DEF_EXT_CTRL
-Much! better implementation of def_to_user


THANKS!


Ricardo Ribalda Delgado (19):
  media/v4l2-core: Add argument def_value to g_ext_ctrl
  media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS
  videodev2.h: Fix typo in comment
  media/usb/uvc: Implement vivioc_g_def_ext_ctrls
  media/pci/saa7164-encoder: Implement vivioc_g_def_ext_ctrls
  media/pci/saa7164-vbi: Implement vivioc_g_def_ext_ctrls
  media/usb/prusb2: Implement vivioc_g_def_ext_ctrls
  v4l2-subdev: Add g_def_ext_ctrls to core_ops
  media/i2c/bt819: Implement g_def_ext_ctrls core_op
  media/i2c/cs53l32a: Implement g_def_ext_ctrls core_op
  media/i2c/cx25840/cx25840-core: Implement g_def_ext_ctrls core_op
  media/i2c/msp3400-driver: Implement g_def_ext_ctrls core_op
  media/i2c/saa7110: Implement g_def_ext_ctrls core_op
  media/i2c/saa7115: Implement g_def_ext_ctrls core_op
  media/i2c/tlv320aic23b: Implement g_def_ext_ctrls core_op
  media/i2c/vpx3220: Implement g_def_ext_ctrls core_op
  media/i2c/wm8775: Implement g_def_ext_ctrls core_op
  Docbook: media: new ioctl VIDIOC_G_DEF_EXT_CTRLS
  Documentation: media: Fix code sample

 Documentation/DocBook/media/v4l/v4l2.xml           |  8 ++++++
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       | 13 ++++++---
 Documentation/video4linux/v4l2-controls.txt        |  4 ++-
 Documentation/video4linux/v4l2-framework.txt       |  1 +
 Documentation/zh_CN/video4linux/v4l2-framework.txt |  1 +
 drivers/media/i2c/bt819.c                          |  1 +
 drivers/media/i2c/cs53l32a.c                       |  1 +
 drivers/media/i2c/cx25840/cx25840-core.c           |  1 +
 drivers/media/i2c/msp3400-driver.c                 |  1 +
 drivers/media/i2c/saa7110.c                        |  1 +
 drivers/media/i2c/saa7115.c                        |  1 +
 drivers/media/i2c/tlv320aic23b.c                   |  1 +
 drivers/media/i2c/vpx3220.c                        |  1 +
 drivers/media/i2c/wm8775.c                         |  1 +
 drivers/media/pci/saa7164/saa7164-encoder.c        | 28 +++++++++++++++++++
 drivers/media/pci/saa7164/saa7164-vbi.c            | 28 +++++++++++++++++++
 drivers/media/platform/omap3isp/ispvideo.c         |  2 +-
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c           | 28 +++++++++++++++++++
 drivers/media/usb/uvc/uvc_v4l2.c                   | 30 ++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |  4 +++
 drivers/media/v4l2-core/v4l2-ctrls.c               | 32 ++++++++++++++++++----
 drivers/media/v4l2-core/v4l2-ioctl.c               | 25 +++++++++++++++--
 drivers/media/v4l2-core/v4l2-subdev.c              |  5 +++-
 include/media/v4l2-ctrls.h                         |  5 +++-
 include/media/v4l2-ioctl.h                         |  2 ++
 include/media/v4l2-subdev.h                        |  2 ++
 include/uapi/linux/videodev2.h                     |  3 +-
 27 files changed, 214 insertions(+), 16 deletions(-)

-- 
2.1.4

[RFC v3 01/19] media/v4l2-core: Add argument def_value to g_ext_ctrl

[Ricardo Ribalda Delgado]

If def_value is set, the default value for the controls is returned.

Helper function def_to_user is also added with the same interface as
cur_to_user or new_to_user.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/platform/omap3isp/ispvideo.c |  2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c       | 25 ++++++++++++++++++++-----
 drivers/media/v4l2-core/v4l2-ioctl.c       |  4 ++--
 drivers/media/v4l2-core/v4l2-subdev.c      |  2 +-
 include/media/v4l2-ctrls.h                 |  3 ++-
 5 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index d285af18df7f..cdcc51ff6fa7 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -941,7 +941,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 	ctrls.count = 1;
 	ctrls.controls = &ctrl;
 
-	ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &ctrls);
+	ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &ctrls, false);
 	if (ret < 0) {
 		dev_warn(isp->dev, "no pixel rate control in subdev %s\n",
 			 pipe->external->name);
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b6b7dcc1b77d..02ff6f573fd2 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1489,6 +1489,17 @@ static int new_to_user(struct v4l2_ext_control *c,
 	return ptr_to_user(c, ctrl, ctrl->p_new);
 }
 
+/* Helper function: copy the initial control value back to the caller */
+static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
+{
+	int idx;
+
+	for (idx = 0; idx < ctrl->elems; idx++)
+		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
+
+	return ptr_to_user(c, ctrl, ctrl->p_new);
+}
+
 /* Helper function: copy the caller-provider value to the given control value */
 static int user_to_ptr(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl,
@@ -2795,7 +2806,8 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
 
 
 /* Get extended controls. Allocates the helpers array if needed. */
-int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
+int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+		     struct v4l2_ext_controls *cs, bool def_value)
 {
 	struct v4l2_ctrl_helper helper[4];
 	struct v4l2_ctrl_helper *helpers = helper;
@@ -2827,9 +2839,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 
 	for (i = 0; !ret && i < cs->count; i++) {
 		int (*ctrl_to_user)(struct v4l2_ext_control *c,
-				    struct v4l2_ctrl *ctrl) = cur_to_user;
+				    struct v4l2_ctrl *ctrl);
 		struct v4l2_ctrl *master;
 
+		ctrl_to_user = def_value ? def_to_user : cur_to_user;
+
 		if (helpers[i].mref == NULL)
 			continue;
 
@@ -2839,8 +2853,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 		v4l2_ctrl_lock(master);
 
 		/* g_volatile_ctrl will update the new control values */
-		if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
-			(master->has_volatiles && !is_cur_manual(master))) {
+		if (!def_value &&
+		    ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
+		    (master->has_volatiles && !is_cur_manual(master)))) {
 			for (j = 0; j < master->ncontrols; j++)
 				cur_to_new(master->cluster[j]);
 			ret = call_op(master, g_volatile_ctrl);
@@ -2869,7 +2884,7 @@ EXPORT_SYMBOL(v4l2_g_ext_ctrls);
 
 int v4l2_subdev_g_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
 {
-	return v4l2_g_ext_ctrls(sd->ctrl_handler, cs);
+	return v4l2_g_ext_ctrls(sd->ctrl_handler, cs, false);
 }
 EXPORT_SYMBOL(v4l2_subdev_g_ext_ctrls);
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 85de4557f696..a675ccc8f27a 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1982,9 +1982,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
-		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p);
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p, false);
 	if (vfd->ctrl_handler)
-		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p);
+		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p, false);
 	if (ops->vidioc_g_ext_ctrls == NULL)
 		return -ENOTTY;
 	return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 63596063b213..90ed61e6df34 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -203,7 +203,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
 
 	case VIDIOC_G_EXT_CTRLS:
-		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, false);
 
 	case VIDIOC_S_EXT_CTRLS:
 		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 911f3e542834..16f16b67181b 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -812,7 +812,8 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
 int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
 int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 						struct v4l2_control *ctrl);
-int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
+int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c,
+		     bool def_value);
 int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
 int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 						struct v4l2_ext_controls *c);
-- 
2.1.4

[RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

This ioctl returns the default value of one or more extended controls.
It has the same interface as VIDIOC_EXT_CTRLS.

It is needed due to the fact that QUERYCTRL was not enough to
provide the initial value of pointer type controls.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++++
 drivers/media/v4l2-core/v4l2-ioctl.c          | 21 +++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c         |  3 +++
 include/media/v4l2-ioctl.h                    |  2 ++
 include/uapi/linux/videodev2.h                |  1 +
 5 files changed, 31 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index af635430524e..b7ab852b642f 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -817,6 +817,7 @@ static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
 #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
 #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
 #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
+#define VIDIOC_G_DEF_EXT_CTRLS32 _IOWR('V', 104, struct v4l2_ext_controls32)
 
 #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
 #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
@@ -858,6 +859,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_ENUMINPUT32: cmd = VIDIOC_ENUMINPUT; break;
 	case VIDIOC_TRY_FMT32: cmd = VIDIOC_TRY_FMT; break;
 	case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break;
+	case VIDIOC_G_DEF_EXT_CTRLS32: cmd = VIDIOC_G_DEF_EXT_CTRLS; break;
 	case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break;
 	case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break;
 	case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
@@ -935,6 +937,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		break;
 
 	case VIDIOC_G_EXT_CTRLS:
+	case VIDIOC_G_DEF_EXT_CTRLS:
 	case VIDIOC_S_EXT_CTRLS:
 	case VIDIOC_TRY_EXT_CTRLS:
 		err = get_v4l2_ext_controls32(&karg.v2ecs, up);
@@ -962,6 +965,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	   contain information on which control failed. */
 	switch (cmd) {
 	case VIDIOC_G_EXT_CTRLS:
+	case VIDIOC_G_DEF_EXT_CTRLS:
 	case VIDIOC_S_EXT_CTRLS:
 	case VIDIOC_TRY_EXT_CTRLS:
 		if (put_v4l2_ext_controls32(&karg.v2ecs, up))
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a675ccc8f27a..5ed03b8588ec 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1991,6 +1991,25 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 					-EINVAL;
 }
 
+static int v4l_g_def_ext_ctrls(const struct v4l2_ioctl_ops *ops,
+				struct file *file, void *fh, void *arg)
+{
+	struct video_device *vfd = video_devdata(file);
+	struct v4l2_ext_controls *p = arg;
+	struct v4l2_fh *vfh =
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+
+	p->error_idx = p->count;
+	if (vfh && vfh->ctrl_handler)
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p, true);
+	if (vfd->ctrl_handler)
+		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p, true);
+	if (ops->vidioc_g_def_ext_ctrls == NULL)
+		return -ENOTTY;
+	return check_ext_ctrls(p, 0) ?
+		ops->vidioc_g_def_ext_ctrls(file, fh, p) : -EINVAL;
+}
+
 static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -2435,6 +2454,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_FNC(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
 	IOCTL_INFO_FNC(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
 	IOCTL_INFO_FNC(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
+	IOCTL_INFO_FNC(VIDIOC_G_DEF_EXT_CTRLS, v4l_g_def_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
 	IOCTL_INFO_FNC(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
 	IOCTL_INFO_FNC(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
 	IOCTL_INFO_STD(VIDIOC_ENUM_FRAMESIZES, vidioc_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
@@ -2643,6 +2663,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 
 	case VIDIOC_S_EXT_CTRLS:
 	case VIDIOC_G_EXT_CTRLS:
+	case VIDIOC_G_DEF_EXT_CTRLS:
 	case VIDIOC_TRY_EXT_CTRLS: {
 		struct v4l2_ext_controls *ctrls = parg;
 
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 90ed61e6df34..8d75620e4603 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -205,6 +205,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_G_EXT_CTRLS:
 		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, false);
 
+	case VIDIOC_G_DEF_EXT_CTRLS:
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, true);
+
 	case VIDIOC_S_EXT_CTRLS:
 		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
 
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 8fbbd76d78e8..16d7eeec9ff6 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -160,6 +160,8 @@ struct v4l2_ioctl_ops {
 					struct v4l2_control *a);
 	int (*vidioc_g_ext_ctrls)      (struct file *file, void *fh,
 					struct v4l2_ext_controls *a);
+	int (*vidioc_g_def_ext_ctrls)  (struct file *file, void *fh,
+					struct v4l2_ext_controls *a);
 	int (*vidioc_s_ext_ctrls)      (struct file *file, void *fh,
 					struct v4l2_ext_controls *a);
 	int (*vidioc_try_ext_ctrls)    (struct file *file, void *fh,
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 3d5fc72d53a7..b9468a3b833e 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2269,6 +2269,7 @@ struct v4l2_create_buffers {
 #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
 
 #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
+#define VIDIOC_G_DEF_EXT_CTRLS	_IOWR('V', 104, struct v4l2_ext_controls)
 
 /* Reminder: when adding new ioctls please add support for them to
    drivers/media/video/v4l2-compat-ioctl32.c as well! */
-- 
2.1.4

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Sakari Ailus]

Hi Ricardo,

Thanks for the set, and my apologies for the late review!

On Fri, Jun 12, 2015 at 06:46:21PM +0200, Ricardo Ribalda Delgado wrote:
> This ioctl returns the default value of one or more extended controls.
> It has the same interface as VIDIOC_EXT_CTRLS.
> 
> It is needed due to the fact that QUERYCTRL was not enough to
> provide the initial value of pointer type controls.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++++
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 21 +++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-subdev.c         |  3 +++
>  include/media/v4l2-ioctl.h                    |  2 ++
>  include/uapi/linux/videodev2.h                |  1 +
>  5 files changed, 31 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index af635430524e..b7ab852b642f 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -817,6 +817,7 @@ static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
>  #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
>  #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
>  #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
> +#define VIDIOC_G_DEF_EXT_CTRLS32 _IOWR('V', 104, struct v4l2_ext_controls32)
>  
>  #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
>  #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
> @@ -858,6 +859,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	case VIDIOC_ENUMINPUT32: cmd = VIDIOC_ENUMINPUT; break;
>  	case VIDIOC_TRY_FMT32: cmd = VIDIOC_TRY_FMT; break;
>  	case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break;
> +	case VIDIOC_G_DEF_EXT_CTRLS32: cmd = VIDIOC_G_DEF_EXT_CTRLS; break;
>  	case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break;
>  	case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break;
>  	case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
> @@ -935,6 +937,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  		break;
>  
>  	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_DEF_EXT_CTRLS:
>  	case VIDIOC_S_EXT_CTRLS:
>  	case VIDIOC_TRY_EXT_CTRLS:
>  		err = get_v4l2_ext_controls32(&karg.v2ecs, up);
> @@ -962,6 +965,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	   contain information on which control failed. */
>  	switch (cmd) {
>  	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_DEF_EXT_CTRLS:
>  	case VIDIOC_S_EXT_CTRLS:
>  	case VIDIOC_TRY_EXT_CTRLS:
>  		if (put_v4l2_ext_controls32(&karg.v2ecs, up))
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a675ccc8f27a..5ed03b8588ec 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1991,6 +1991,25 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  					-EINVAL;
>  }
>  
> +static int v4l_g_def_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> +				struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +	struct v4l2_ext_controls *p = arg;
> +	struct v4l2_fh *vfh =
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +
> +	p->error_idx = p->count;
> +	if (vfh && vfh->ctrl_handler)
> +		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p, true);
> +	if (vfd->ctrl_handler)
> +		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p, true);
> +	if (ops->vidioc_g_def_ext_ctrls == NULL)
> +		return -ENOTTY;
> +	return check_ext_ctrls(p, 0) ?
> +		ops->vidioc_g_def_ext_ctrls(file, fh, p) : -EINVAL;
> +}
> +
>  static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -2435,6 +2454,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_FNC(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
>  	IOCTL_INFO_FNC(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
>  	IOCTL_INFO_FNC(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
> +	IOCTL_INFO_FNC(VIDIOC_G_DEF_EXT_CTRLS, v4l_g_def_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>  	IOCTL_INFO_FNC(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
>  	IOCTL_INFO_FNC(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>  	IOCTL_INFO_STD(VIDIOC_ENUM_FRAMESIZES, vidioc_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
> @@ -2643,6 +2663,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>  
>  	case VIDIOC_S_EXT_CTRLS:
>  	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_DEF_EXT_CTRLS:
>  	case VIDIOC_TRY_EXT_CTRLS: {
>  		struct v4l2_ext_controls *ctrls = parg;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 90ed61e6df34..8d75620e4603 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -205,6 +205,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  	case VIDIOC_G_EXT_CTRLS:
>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, false);
>  
> +	case VIDIOC_G_DEF_EXT_CTRLS:
> +		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, true);
> +
>  	case VIDIOC_S_EXT_CTRLS:
>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 8fbbd76d78e8..16d7eeec9ff6 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -160,6 +160,8 @@ struct v4l2_ioctl_ops {
>  					struct v4l2_control *a);
>  	int (*vidioc_g_ext_ctrls)      (struct file *file, void *fh,
>  					struct v4l2_ext_controls *a);
> +	int (*vidioc_g_def_ext_ctrls)  (struct file *file, void *fh,
> +					struct v4l2_ext_controls *a);
>  	int (*vidioc_s_ext_ctrls)      (struct file *file, void *fh,
>  					struct v4l2_ext_controls *a);
>  	int (*vidioc_try_ext_ctrls)    (struct file *file, void *fh,
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 3d5fc72d53a7..b9468a3b833e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2269,6 +2269,7 @@ struct v4l2_create_buffers {
>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>  
>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
> +#define VIDIOC_G_DEF_EXT_CTRLS	_IOWR('V', 104, struct v4l2_ext_controls)

I assume that if an application uses pointer controls, then it'd obtain the
default values using VIDIOC_G_DEF_EXT_CTRLS. This suggests all drivers
should support this from the very beginning, and the application would not
work on older kernels that don't have the IOCTL implemented.

Instead of adding a new IOCTL, have you thought about the possibility of
doing this through VIDIOC_QUERY_EXT_CTRL? That's how the default control
value is passed to the user now, and I think it'd look odd to add a new
IOCTL for just that purpose.

One option could be making the default_value field a union such as the one
in struct v4l2_ext_control. If the control type is such that the value is
stored in the memory, one of the pointer fields of the union is used
instead.

As the user cannot be expected to know the size beforehand, the pointer
value may only be used if it's non-zero. This might require a new field
rather than making default_value a union for backward compatibility, as the
documentation does not instruct the user to zero the default_value field.

What do you think?

The result would be no added redundancy, and less driver modifications, as
the drivers also don't need to support multiple interfaces for passing
control default values.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Hans Verkuil]

On 07/17/2015 09:13 AM, Sakari Ailus wrote:
> Hi Ricardo,
> 
> Thanks for the set, and my apologies for the late review!
> 
> On Fri, Jun 12, 2015 at 06:46:21PM +0200, Ricardo Ribalda Delgado wrote:
>> This ioctl returns the default value of one or more extended controls.
>> It has the same interface as VIDIOC_EXT_CTRLS.
>>
>> It is needed due to the fact that QUERYCTRL was not enough to
>> provide the initial value of pointer type controls.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++++
>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 21 +++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-subdev.c         |  3 +++
>>  include/media/v4l2-ioctl.h                    |  2 ++
>>  include/uapi/linux/videodev2.h                |  1 +
>>  5 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index af635430524e..b7ab852b642f 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -817,6 +817,7 @@ static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
>>  #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
>>  #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
>>  #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
>> +#define VIDIOC_G_DEF_EXT_CTRLS32 _IOWR('V', 104, struct v4l2_ext_controls32)
>>  
>>  #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
>>  #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
>> @@ -858,6 +859,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	case VIDIOC_ENUMINPUT32: cmd = VIDIOC_ENUMINPUT; break;
>>  	case VIDIOC_TRY_FMT32: cmd = VIDIOC_TRY_FMT; break;
>>  	case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break;
>> +	case VIDIOC_G_DEF_EXT_CTRLS32: cmd = VIDIOC_G_DEF_EXT_CTRLS; break;
>>  	case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break;
>>  	case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break;
>>  	case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
>> @@ -935,6 +937,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  		break;
>>  
>>  	case VIDIOC_G_EXT_CTRLS:
>> +	case VIDIOC_G_DEF_EXT_CTRLS:
>>  	case VIDIOC_S_EXT_CTRLS:
>>  	case VIDIOC_TRY_EXT_CTRLS:
>>  		err = get_v4l2_ext_controls32(&karg.v2ecs, up);
>> @@ -962,6 +965,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	   contain information on which control failed. */
>>  	switch (cmd) {
>>  	case VIDIOC_G_EXT_CTRLS:
>> +	case VIDIOC_G_DEF_EXT_CTRLS:
>>  	case VIDIOC_S_EXT_CTRLS:
>>  	case VIDIOC_TRY_EXT_CTRLS:
>>  		if (put_v4l2_ext_controls32(&karg.v2ecs, up))
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index a675ccc8f27a..5ed03b8588ec 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1991,6 +1991,25 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>>  					-EINVAL;
>>  }
>>  
>> +static int v4l_g_def_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>> +				struct file *file, void *fh, void *arg)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +	struct v4l2_ext_controls *p = arg;
>> +	struct v4l2_fh *vfh =
>> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>> +
>> +	p->error_idx = p->count;
>> +	if (vfh && vfh->ctrl_handler)
>> +		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p, true);
>> +	if (vfd->ctrl_handler)
>> +		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p, true);
>> +	if (ops->vidioc_g_def_ext_ctrls == NULL)
>> +		return -ENOTTY;
>> +	return check_ext_ctrls(p, 0) ?
>> +		ops->vidioc_g_def_ext_ctrls(file, fh, p) : -EINVAL;
>> +}
>> +
>>  static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> @@ -2435,6 +2454,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>  	IOCTL_INFO_FNC(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
>>  	IOCTL_INFO_FNC(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
>>  	IOCTL_INFO_FNC(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>> +	IOCTL_INFO_FNC(VIDIOC_G_DEF_EXT_CTRLS, v4l_g_def_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>>  	IOCTL_INFO_FNC(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
>>  	IOCTL_INFO_FNC(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>>  	IOCTL_INFO_STD(VIDIOC_ENUM_FRAMESIZES, vidioc_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
>> @@ -2643,6 +2663,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>>  
>>  	case VIDIOC_S_EXT_CTRLS:
>>  	case VIDIOC_G_EXT_CTRLS:
>> +	case VIDIOC_G_DEF_EXT_CTRLS:
>>  	case VIDIOC_TRY_EXT_CTRLS: {
>>  		struct v4l2_ext_controls *ctrls = parg;
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 90ed61e6df34..8d75620e4603 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -205,6 +205,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>  	case VIDIOC_G_EXT_CTRLS:
>>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, false);
>>  
>> +	case VIDIOC_G_DEF_EXT_CTRLS:
>> +		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, true);
>> +
>>  	case VIDIOC_S_EXT_CTRLS:
>>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>>  
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index 8fbbd76d78e8..16d7eeec9ff6 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -160,6 +160,8 @@ struct v4l2_ioctl_ops {
>>  					struct v4l2_control *a);
>>  	int (*vidioc_g_ext_ctrls)      (struct file *file, void *fh,
>>  					struct v4l2_ext_controls *a);
>> +	int (*vidioc_g_def_ext_ctrls)  (struct file *file, void *fh,
>> +					struct v4l2_ext_controls *a);
>>  	int (*vidioc_s_ext_ctrls)      (struct file *file, void *fh,
>>  					struct v4l2_ext_controls *a);
>>  	int (*vidioc_try_ext_ctrls)    (struct file *file, void *fh,
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 3d5fc72d53a7..b9468a3b833e 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2269,6 +2269,7 @@ struct v4l2_create_buffers {
>>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>  
>>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define VIDIOC_G_DEF_EXT_CTRLS	_IOWR('V', 104, struct v4l2_ext_controls)
> 
> I assume that if an application uses pointer controls, then it'd obtain the
> default values using VIDIOC_G_DEF_EXT_CTRLS. This suggests all drivers
> should support this from the very beginning, and the application would not
> work on older kernels that don't have the IOCTL implemented.

But this is true as well for VIDIOC_QUERY_EXT_CTRL, which is a very recent
ioctl as well.

> Instead of adding a new IOCTL, have you thought about the possibility of
> doing this through VIDIOC_QUERY_EXT_CTRL? That's how the default control
> value is passed to the user now, and I think it'd look odd to add a new
> IOCTL for just that purpose.
> 
> One option could be making the default_value field a union such as the one
> in struct v4l2_ext_control. If the control type is such that the value is
> stored in the memory, one of the pointer fields of the union is used
> instead.
> 
> As the user cannot be expected to know the size beforehand, the pointer
> value may only be used if it's non-zero. This might require a new field
> rather than making default_value a union for backward compatibility, as the
> documentation does not instruct the user to zero the default_value field.

You would have to take from the reserved[] array to do this. The default_value
field can't be used for this.

> 
> What do you think?
> 
> The result would be no added redundancy, and less driver modifications, as
> the drivers also don't need to support multiple interfaces for passing
> control default values.

I don't see why this would cause fewer driver modifications.

BTW, I'm not sure if we should bother adding this ioctl to non-control framework
drivers (other than uvc). I'd be OK with it if they just don't support this
ioctl. If you want it, then just convert the driver to the control framework.
I have to think about this. I don't like patching drivers that use old crap.
I'd rather convert them to use the control framework.

Anyway, the reason I very much like to use the VIDIOC_G_DEF_EXT_CTRLS ioctl
for this is that this makes it very easy to do:

	ioctl(fd, VIDIOC_G_DEF_EXT_CTRLS, &ctrls);
	ioctl(fd, VIDIOC_S_EXT_CTRLS, &ctrls);

This will set the whole list of controls to their default values.

Very, very nice and efficient.

Personally I think this is a strong argument in favor of adding a new ioctl.

In addition, adding support for this to QUERY_EXT_CTRLS would actually take
quite a bit more code since I would have to add support for the userspace pointer
in the struct, which is always a lot of work. For G_DEF_EXT_CTRLS that code
already exists. But frankly, it's the fact that this makes it easy to set the
default values that is the real argument for creating this ioctl.

Regards,

	Hans

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

Hi Sakari

Thanks for your review!

On Fri, Jul 17, 2015 at 9:13 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:

>>  #define VIDIOC_QUERY_EXT_CTRL        _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define VIDIOC_G_DEF_EXT_CTRLS       _IOWR('V', 104, struct v4l2_ext_controls)
>
> I assume that if an application uses pointer controls, then it'd obtain the
> default values using VIDIOC_G_DEF_EXT_CTRLS. This suggests all drivers
> should support this from the very beginning, and the application would not
> work on older kernels that don't have the IOCTL implemented.

This patchset add supports for the Ioctl for all the in-tree drivers.
out of tree drivers that use v4l2-ctrl will also work, so we are only
leaving behind out out tree drivers that dont use v4l2-ctrl.
Applications will neither not in old kernels if we do an
implementation with VIDIOC_QUERY_EXT_CTRL.


>
> Instead of adding a new IOCTL, have you thought about the possibility of
> doing this through VIDIOC_QUERY_EXT_CTRL? That's how the default control
> value is passed to the user now, and I think it'd look odd to add a new
> IOCTL for just that purpose.
>
> One option could be making the default_value field a union such as the one
> in struct v4l2_ext_control. If the control type is such that the value is
> stored in the memory, one of the pointer fields of the union is used
> instead.
>
> As the user cannot be expected to know the size beforehand, the pointer
> value may only be used if it's non-zero. This might require a new field
> rather than making default_value a union for backward compatibility, as the
> documentation does not instruct the user to zero the default_value field.
>
> What do you think?
>
> The result would be no added redundancy, and less driver modifications, as
> the drivers also don't need to support multiple interfaces for passing
> control default values.

Although this also a valid option, the implementation by userland can
be a bit tricky, I dont like the idea of passing a pointer to the
kernel without telling it how much memory it has available for
writing.

There is also the problem of legacy applications that do not memset to
zero the reserved fields.... Those application may crash quite badly
if we change
VIDIOC_QUERY_EXT_CTRL the way you suggests

Finally, it is difficult for the user to know if the driver supports
this extra functionality on the ioctl before hand. On my
implementataion -ENOTTY is a pretty good indication of what is the
problem.


Best regards!


-- 
Ricardo Ribalda

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Hans Verkuil]

On 06/12/2015 06:46 PM, Ricardo Ribalda Delgado wrote:
> This ioctl returns the default value of one or more extended controls.
> It has the same interface as VIDIOC_EXT_CTRLS.
> 
> It is needed due to the fact that QUERYCTRL was not enough to
> provide the initial value of pointer type controls.

This was discussed on irc, and both Sakari and Laurent didn't like having to add
a new ioctl.

After some discussion I came up with an alternative and I'd like to have some
feedback on that.

The key change is in struct v4l2_ext_controls where the __u32 ctrl_class field
is changed to:

	union {
		__u32 ctrl_class;
		__u32 which;
	};

And two new defines are added:

#define V4L2_CTRL_WHICH_CUR_VAL        0
#define V4L2_CTRL_WHICH_DEF_VAL        0x0f000000

The 'which' field tells you which controls are get/set/tried.

V4L2_CTRL_WHICH_CUR_VAL: the current value of the controls
V4L2_CTRL_WHICH_DEF_VAL: the default value of the controls
V4L2_CTRL_CLASS_*: the current value of the controls belonging to the specified class.
	Note: this is deprecated usage and is only there for backwards compatibility.
	Which is also why I don't think there is a need to add V4L2_CTRL_WHICH_
	aliases for these defines.

And in the future, if the 'request' patch series for per-frame configuration is
merged, we can specify the request ID here to get/set/try the controls values
of the specified request ID.

This can also be extended to things like 'WHICH_MIN_VAL/MAX_VAL' if we want to.

An attempt to set/try controls for WHICH_DEF_VAL will return -EACCES since
the default value is a read-only value that you cannot change.

Renaming 'ctrl_class' to 'which' makes this something that can be documented
without looking like a hack and it avoids having to make a new ioctl.

Comments are welcome!

Regards,

	Hans

> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++++
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 21 +++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-subdev.c         |  3 +++
>  include/media/v4l2-ioctl.h                    |  2 ++
>  include/uapi/linux/videodev2.h                |  1 +
>  5 files changed, 31 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index af635430524e..b7ab852b642f 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -817,6 +817,7 @@ static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
>  #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
>  #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
>  #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
> +#define VIDIOC_G_DEF_EXT_CTRLS32 _IOWR('V', 104, struct v4l2_ext_controls32)
>  
>  #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
>  #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
> @@ -858,6 +859,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	case VIDIOC_ENUMINPUT32: cmd = VIDIOC_ENUMINPUT; break;
>  	case VIDIOC_TRY_FMT32: cmd = VIDIOC_TRY_FMT; break;
>  	case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break;
> +	case VIDIOC_G_DEF_EXT_CTRLS32: cmd = VIDIOC_G_DEF_EXT_CTRLS; break;
>  	case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break;
>  	case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break;
>  	case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
> @@ -935,6 +937,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  		break;
>  
>  	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_DEF_EXT_CTRLS:
>  	case VIDIOC_S_EXT_CTRLS:
>  	case VIDIOC_TRY_EXT_CTRLS:
>  		err = get_v4l2_ext_controls32(&karg.v2ecs, up);
> @@ -962,6 +965,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	   contain information on which control failed. */
>  	switch (cmd) {
>  	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_DEF_EXT_CTRLS:
>  	case VIDIOC_S_EXT_CTRLS:
>  	case VIDIOC_TRY_EXT_CTRLS:
>  		if (put_v4l2_ext_controls32(&karg.v2ecs, up))
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a675ccc8f27a..5ed03b8588ec 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1991,6 +1991,25 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  					-EINVAL;
>  }
>  
> +static int v4l_g_def_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> +				struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +	struct v4l2_ext_controls *p = arg;
> +	struct v4l2_fh *vfh =
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +
> +	p->error_idx = p->count;
> +	if (vfh && vfh->ctrl_handler)
> +		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p, true);
> +	if (vfd->ctrl_handler)
> +		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p, true);
> +	if (ops->vidioc_g_def_ext_ctrls == NULL)
> +		return -ENOTTY;
> +	return check_ext_ctrls(p, 0) ?
> +		ops->vidioc_g_def_ext_ctrls(file, fh, p) : -EINVAL;
> +}
> +
>  static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -2435,6 +2454,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_FNC(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
>  	IOCTL_INFO_FNC(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
>  	IOCTL_INFO_FNC(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
> +	IOCTL_INFO_FNC(VIDIOC_G_DEF_EXT_CTRLS, v4l_g_def_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>  	IOCTL_INFO_FNC(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
>  	IOCTL_INFO_FNC(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>  	IOCTL_INFO_STD(VIDIOC_ENUM_FRAMESIZES, vidioc_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
> @@ -2643,6 +2663,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>  
>  	case VIDIOC_S_EXT_CTRLS:
>  	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_DEF_EXT_CTRLS:
>  	case VIDIOC_TRY_EXT_CTRLS: {
>  		struct v4l2_ext_controls *ctrls = parg;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 90ed61e6df34..8d75620e4603 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -205,6 +205,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  	case VIDIOC_G_EXT_CTRLS:
>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, false);
>  
> +	case VIDIOC_G_DEF_EXT_CTRLS:
> +		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg, true);
> +
>  	case VIDIOC_S_EXT_CTRLS:
>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 8fbbd76d78e8..16d7eeec9ff6 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -160,6 +160,8 @@ struct v4l2_ioctl_ops {
>  					struct v4l2_control *a);
>  	int (*vidioc_g_ext_ctrls)      (struct file *file, void *fh,
>  					struct v4l2_ext_controls *a);
> +	int (*vidioc_g_def_ext_ctrls)  (struct file *file, void *fh,
> +					struct v4l2_ext_controls *a);
>  	int (*vidioc_s_ext_ctrls)      (struct file *file, void *fh,
>  					struct v4l2_ext_controls *a);
>  	int (*vidioc_try_ext_ctrls)    (struct file *file, void *fh,
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 3d5fc72d53a7..b9468a3b833e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2269,6 +2269,7 @@ struct v4l2_create_buffers {
>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>  
>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
> +#define VIDIOC_G_DEF_EXT_CTRLS	_IOWR('V', 104, struct v4l2_ext_controls)
>  
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> 

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

Hello

I have no preference over the two implementations, but I see an issue
with this suggestion.


What happens to out out tree drivers, or drivers that don't support
this functionality?

With the ioctl, the user receives a -ENOTTY. So he knows there is
something wrong with the driver.

With this class, the driver might interpret this a simple G_VAL and
return he current value with no way for the user to know what is going
on.


Regarding the new implementation.... I can make some code next week,
this week I am 120% busy :)


What do you think?

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Hans Verkuil]

On 07/20/2015 03:52 PM, Ricardo Ribalda Delgado wrote:
> Hello
> 
> I have no preference over the two implementations, but I see an issue
> with this suggestion.
> 
> 
> What happens to out out tree drivers, or drivers that don't support
> this functionality?
> 
> With the ioctl, the user receives a -ENOTTY. So he knows there is
> something wrong with the driver.
> 
> With this class, the driver might interpret this a simple G_VAL and
> return he current value with no way for the user to know what is going
> on.

Drivers that implement the current API correctly will return an error
if V4L2_CTRL_WHICH_DEF_VAL was specified. Such drivers will interpret
the value as a control class, and no control classes in that range exist.
See also class_check() in v4l2-ctrls.c.

The exception here is uvc which doesn't have this class check and it will
just return the current value :-(

I don't see a way around this, unfortunately.

Out-of-tree drivers that use the control framework are fine, and I don't
really care about drivers (out-of-tree or otherwise) that do not use the
control framework.

> Regarding the new implementation.... I can make some code next week,
> this week I am 120% busy :)

Wait until there is a decision first :-)

It's not a lot of work, I think.

Regards,

	Hans

> What do you think?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

Hello Hans, Sakari and Laurent:

I am back from holidays :). Shall I prepare a new patchset with the
suggestion from Hans?

Thanks!

On Mon, Jul 20, 2015 at 4:31 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 07/20/2015 03:52 PM, Ricardo Ribalda Delgado wrote:
>> Hello
>>
>> I have no preference over the two implementations, but I see an issue
>> with this suggestion.
>>
>>
>> What happens to out out tree drivers, or drivers that don't support
>> this functionality?
>>
>> With the ioctl, the user receives a -ENOTTY. So he knows there is
>> something wrong with the driver.
>>
>> With this class, the driver might interpret this a simple G_VAL and
>> return he current value with no way for the user to know what is going
>> on.
>
> Drivers that implement the current API correctly will return an error
> if V4L2_CTRL_WHICH_DEF_VAL was specified. Such drivers will interpret
> the value as a control class, and no control classes in that range exist.
> See also class_check() in v4l2-ctrls.c.
>
> The exception here is uvc which doesn't have this class check and it will
> just return the current value :-(
>
> I don't see a way around this, unfortunately.
>
> Out-of-tree drivers that use the control framework are fine, and I don't
> really care about drivers (out-of-tree or otherwise) that do not use the
> control framework.
>
>> Regarding the new implementation.... I can make some code next week,
>> this week I am 120% busy :)
>
> Wait until there is a decision first :-)
>
> It's not a lot of work, I think.
>
> Regards,
>
>         Hans
>
>> What do you think?
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>



-- 
Ricardo Ribalda

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

ping?

On Mon, Aug 10, 2015 at 2:04 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Hello Hans, Sakari and Laurent:
>
> I am back from holidays :). Shall I prepare a new patchset with the
> suggestion from Hans?
>
> Thanks!
>
> On Mon, Jul 20, 2015 at 4:31 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 07/20/2015 03:52 PM, Ricardo Ribalda Delgado wrote:
>>> Hello
>>>
>>> I have no preference over the two implementations, but I see an issue
>>> with this suggestion.
>>>
>>>
>>> What happens to out out tree drivers, or drivers that don't support
>>> this functionality?
>>>
>>> With the ioctl, the user receives a -ENOTTY. So he knows there is
>>> something wrong with the driver.
>>>
>>> With this class, the driver might interpret this a simple G_VAL and
>>> return he current value with no way for the user to know what is going
>>> on.
>>
>> Drivers that implement the current API correctly will return an error
>> if V4L2_CTRL_WHICH_DEF_VAL was specified. Such drivers will interpret
>> the value as a control class, and no control classes in that range exist.
>> See also class_check() in v4l2-ctrls.c.
>>
>> The exception here is uvc which doesn't have this class check and it will
>> just return the current value :-(
>>
>> I don't see a way around this, unfortunately.
>>
>> Out-of-tree drivers that use the control framework are fine, and I don't
>> really care about drivers (out-of-tree or otherwise) that do not use the
>> control framework.
>>
>>> Regarding the new implementation.... I can make some code next week,
>>> this week I am 120% busy :)
>>
>> Wait until there is a decision first :-)
>>
>> It's not a lot of work, I think.
>>
>> Regards,
>>
>>         Hans
>>
>>> What do you think?
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Laurent Pinchart]

Hi Hans,

I like your proposal, and especially how it would integrate with the requests 
API. Should we give the requests API a try to make sure your proposal works 
fine with it ?

As a side note, I'll need to requests API for Renesas. The current schedule is 
to have a first RFC implementation by the end of October.

On Monday 20 July 2015 16:31:22 Hans Verkuil wrote:
> On 07/20/2015 03:52 PM, Ricardo Ribalda Delgado wrote:
> > Hello
> > 
> > I have no preference over the two implementations, but I see an issue
> > with this suggestion.
> > 
> > What happens to out out tree drivers, or drivers that don't support
> > this functionality?
> > 
> > With the ioctl, the user receives a -ENOTTY. So he knows there is
> > something wrong with the driver.
> > 
> > With this class, the driver might interpret this a simple G_VAL and
> > return he current value with no way for the user to know what is going
> > on.
> 
> Drivers that implement the current API correctly will return an error
> if V4L2_CTRL_WHICH_DEF_VAL was specified. Such drivers will interpret
> the value as a control class, and no control classes in that range exist.
> See also class_check() in v4l2-ctrls.c.
> 
> The exception here is uvc which doesn't have this class check and it will
> just return the current value :-(
> 
> I don't see a way around this, unfortunately.

The rationale for implementing VIDIOC_G_DEF_EXT_CTRLS was to get the default 
value of controls that don't fit in 32 bits. uvcvideo doesn't have any such 
control, so I don't think we really need to care. Of course newer versions of 
the uvcvideo driver should implement the new API.

> Out-of-tree drivers that use the control framework are fine, and I don't
> really care about drivers (out-of-tree or otherwise) that do not use the
> control framework.
> 
> > Regarding the new implementation.... I can make some code next week,
> > this week I am 120% busy :)
> 
> Wait until there is a decision first :-)
> 
> It's not a lot of work, I think.

I think I like your proposal better than VIDIOC_G_DEF_EXT_CTRLS, so seeing an 
implementation would be nice.

-- 
Regards,

Laurent Pinchart

Re: [RFC v3 02/19] media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Hans Verkuil]

On 08/20/15 00:19, Laurent Pinchart wrote:
> Hi Hans,
> 
> I like your proposal, and especially how it would integrate with the requests 
> API. Should we give the requests API a try to make sure your proposal works 
> fine with it ?

To be honest, I don't see what there is to test. Request IDs have their own range
and there is no conflict there. Easiest for me is to rebase my request patch series
on top of Ricardo's patch series that implements this. That should be pretty quick
to do.

Ricardo, just go ahead with this and I'll take care of rebasing the request patch
series on top of it once you're done. I'll probably do that as part of my review
of your new patch series once it is posted.

> As a side note, I'll need to requests API for Renesas. The current schedule is 
> to have a first RFC implementation by the end of October.

Cool. I'd like to get this in! Please involve me if there are any questions you
have or work that you want me to do on the request API to improve it.

Regards,

	Hans

> 
> On Monday 20 July 2015 16:31:22 Hans Verkuil wrote:
>> On 07/20/2015 03:52 PM, Ricardo Ribalda Delgado wrote:
>>> Hello
>>>
>>> I have no preference over the two implementations, but I see an issue
>>> with this suggestion.
>>>
>>> What happens to out out tree drivers, or drivers that don't support
>>> this functionality?
>>>
>>> With the ioctl, the user receives a -ENOTTY. So he knows there is
>>> something wrong with the driver.
>>>
>>> With this class, the driver might interpret this a simple G_VAL and
>>> return he current value with no way for the user to know what is going
>>> on.
>>
>> Drivers that implement the current API correctly will return an error
>> if V4L2_CTRL_WHICH_DEF_VAL was specified. Such drivers will interpret
>> the value as a control class, and no control classes in that range exist.
>> See also class_check() in v4l2-ctrls.c.
>>
>> The exception here is uvc which doesn't have this class check and it will
>> just return the current value :-(
>>
>> I don't see a way around this, unfortunately.
> 
> The rationale for implementing VIDIOC_G_DEF_EXT_CTRLS was to get the default 
> value of controls that don't fit in 32 bits. uvcvideo doesn't have any such 
> control, so I don't think we really need to care. Of course newer versions of 
> the uvcvideo driver should implement the new API.
> 
>> Out-of-tree drivers that use the control framework are fine, and I don't
>> really care about drivers (out-of-tree or otherwise) that do not use the
>> control framework.
>>
>>> Regarding the new implementation.... I can make some code next week,
>>> this week I am 120% busy :)
>>
>> Wait until there is a decision first :-)
>>
>> It's not a lot of work, I think.
> 
> I think I like your proposal better than VIDIOC_G_DEF_EXT_CTRLS, so seeing an 
> implementation would be nice.
> 

[RFC v3 03/19] videodev2.h: Fix typo in comment

[Ricardo Ribalda Delgado]

Referenced file has moved

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 include/uapi/linux/videodev2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index b9468a3b833e..b059237f0214 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2272,7 +2272,7 @@ struct v4l2_create_buffers {
 #define VIDIOC_G_DEF_EXT_CTRLS	_IOWR('V', 104, struct v4l2_ext_controls)
 
 /* Reminder: when adding new ioctls please add support for them to
-   drivers/media/video/v4l2-compat-ioctl32.c as well! */
+   drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
 
 #define BASE_VIDIOC_PRIVATE	192		/* 192-255 are private */
 
-- 
2.1.4

[RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Ricardo Ribalda Delgado]

Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
use the controller framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 2764f43607c1..e2698a77138a 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 	return uvc_ctrl_rollback(handle);
 }
 
+static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
+				     struct v4l2_ext_controls *ctrls)
+{
+	struct uvc_fh *handle = fh;
+	struct uvc_video_chain *chain = handle->chain;
+	struct v4l2_ext_control *ctrl = ctrls->controls;
+	unsigned int i;
+	int ret;
+	struct v4l2_queryctrl qc;
+
+	ret = uvc_ctrl_begin(chain);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
+		qc.id = ctrl->id;
+		ret = uvc_query_v4l2_ctrl(chain, &qc);
+		if (ret < 0) {
+			ctrls->error_idx = i;
+			return ret;
+		}
+		ctrl->value = qc.default_value;
+	}
+
+	ctrls->error_idx = 0;
+
+	return 0;
+}
+
 static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 				     struct v4l2_ext_controls *ctrls,
 				     bool commit)
@@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
 	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
 	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
+	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
 	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
 	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
 	.vidioc_querymenu = uvc_ioctl_querymenu,
-- 
2.1.4

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Hans Verkuil]

Laurent,

Can you review/ack this since it touches on uvc?

Thanks!

	Hans

On 06/12/2015 06:46 PM, Ricardo Ribalda Delgado wrote:
> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> use the controller framework.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 2764f43607c1..e2698a77138a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  	return uvc_ctrl_rollback(handle);
>  }
>  
> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> +				     struct v4l2_ext_controls *ctrls)
> +{
> +	struct uvc_fh *handle = fh;
> +	struct uvc_video_chain *chain = handle->chain;
> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> +	unsigned int i;
> +	int ret;
> +	struct v4l2_queryctrl qc;
> +
> +	ret = uvc_ctrl_begin(chain);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> +		qc.id = ctrl->id;
> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> +		if (ret < 0) {
> +			ctrls->error_idx = i;
> +			return ret;
> +		}
> +		ctrl->value = qc.default_value;
> +	}
> +
> +	ctrls->error_idx = 0;
> +
> +	return 0;
> +}
> +
>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  				     struct v4l2_ext_controls *ctrls,
>  				     bool commit)
> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>  	.vidioc_querymenu = uvc_ioctl_querymenu,
> 

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> use the controller framework.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> void *fh, return uvc_ctrl_rollback(handle);
>  }
> 
> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> +				     struct v4l2_ext_controls *ctrls)
> +{
> +	struct uvc_fh *handle = fh;
> +	struct uvc_video_chain *chain = handle->chain;
> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> +	unsigned int i;
> +	int ret;
> +	struct v4l2_queryctrl qc;
> +
> +	ret = uvc_ctrl_begin(chain);

There's no need to call uvc_ctrl_begin() here (and if there was, you'd have to 
call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).

> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> +		qc.id = ctrl->id;
> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> +		if (ret < 0) {
> +			ctrls->error_idx = i;
> +			return ret;
> +		}
> +		ctrl->value = qc.default_value;
> +	}
> +
> +	ctrls->error_idx = 0;
> +
> +	return 0;
> +}

Instead of open-coding this in multiple drivers, how about adding a helper 
function to the core ? Something like (totally untested)

int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
                               struct v4l2_ext_controls *ctrls)
{
	struct video_device *vdev = video_devdata(file);
	unsigned int i;
	int ret;

	for (i = 0; i < ctrls->count; ++i) {
		struct v4l2_queryctrl qc;

		qc.id = ctrl->id;
		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
		if (ret < 0) {
			ctrls->error_idx = i;
			return ret;
		}

		ctrls->controls[i].value = qc.default_value;
	}

	ctrls->error_idx = 0;

	return 0;
}

The function could be called by v4l_g_def_ext_ctrls() when ops-
>vidioc_g_def_ext_ctrls is NULL.

>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  				     struct v4l2_ext_controls *ctrls,
>  				     bool commit)
> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>  	.vidioc_querymenu = uvc_ioctl_querymenu,

-- 
Regards,

Laurent Pinchart

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Hans Verkuil]

On 07/15/15 23:05, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
>> use the controller framework.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
>> void *fh, return uvc_ctrl_rollback(handle);
>>  }
>>
>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>> +				     struct v4l2_ext_controls *ctrls)
>> +{
>> +	struct uvc_fh *handle = fh;
>> +	struct uvc_video_chain *chain = handle->chain;
>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
>> +	unsigned int i;
>> +	int ret;
>> +	struct v4l2_queryctrl qc;
>> +
>> +	ret = uvc_ctrl_begin(chain);
> 
> There's no need to call uvc_ctrl_begin() here (and if there was, you'd have to 
> call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>> +		qc.id = ctrl->id;
>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
>> +		if (ret < 0) {
>> +			ctrls->error_idx = i;
>> +			return ret;
>> +		}
>> +		ctrl->value = qc.default_value;
>> +	}
>> +
>> +	ctrls->error_idx = 0;
>> +
>> +	return 0;
>> +}
> 
> Instead of open-coding this in multiple drivers, how about adding a helper 
> function to the core ? Something like (totally untested)

It's only open-coded in drivers that do not use the control framework. For
drivers that use the control framework it is completely transparent.

Regards,

	Hans

> 
> int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>                                struct v4l2_ext_controls *ctrls)
> {
> 	struct video_device *vdev = video_devdata(file);
> 	unsigned int i;
> 	int ret;
> 
> 	for (i = 0; i < ctrls->count; ++i) {
> 		struct v4l2_queryctrl qc;
> 
> 		qc.id = ctrl->id;
> 		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
> 		if (ret < 0) {
> 			ctrls->error_idx = i;
> 			return ret;
> 		}
> 
> 		ctrls->controls[i].value = qc.default_value;
> 	}
> 
> 	ctrls->error_idx = 0;
> 
> 	return 0;
> }
> 
> The function could be called by v4l_g_def_ext_ctrls() when ops-
>> vidioc_g_def_ext_ctrls is NULL.
> 
>>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>>  				     struct v4l2_ext_controls *ctrls,
>>  				     bool commit)
>> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
>> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>>  	.vidioc_querymenu = uvc_ioctl_querymenu,
> 

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Laurent Pinchart]

Hi Hans,

On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
> On 07/15/15 23:05, Laurent Pinchart wrote:
> > On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> >> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> >> use the controller framework.
> >> 
> >> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >> ---
> >> 
> >>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
> >>  1 file changed, 30 insertions(+)
> >> 
> >> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> >> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
> >> 100644
> >> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
> >> *file, void *fh,
> >>  	return uvc_ctrl_rollback(handle);
> >>  }
> >> 
> >> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> >> +				     struct v4l2_ext_controls *ctrls)
> >> +{
> >> +	struct uvc_fh *handle = fh;
> >> +	struct uvc_video_chain *chain = handle->chain;
> >> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> >> +	unsigned int i;
> >> +	int ret;
> >> +	struct v4l2_queryctrl qc;
> >> +
> >> +	ret = uvc_ctrl_begin(chain);
> > 
> > There's no need to call uvc_ctrl_begin() here (and if there was, you'd
> > have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
> > 
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> >> +		qc.id = ctrl->id;
> >> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> >> +		if (ret < 0) {
> >> +			ctrls->error_idx = i;
> >> +			return ret;
> >> +		}
> >> +		ctrl->value = qc.default_value;
> >> +	}
> >> +
> >> +	ctrls->error_idx = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > Instead of open-coding this in multiple drivers, how about adding a helper
> > function to the core ? Something like (totally untested)
> 
> It's only open-coded in drivers that do not use the control framework. For
> drivers that use the control framework it is completely transparent.

Sure, but still, the same function is implemented several times while a single 
implementation could do. I'd prefer having it in the core until all (or all 
but one) drivers are converted to the control framework.

> > int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> >                                struct v4l2_ext_controls *ctrls)
> > {
> > 	struct video_device *vdev = video_devdata(file);
> > 	unsigned int i;
> > 	int ret;
> > 	
> > 	for (i = 0; i < ctrls->count; ++i) {
> > 		struct v4l2_queryctrl qc;
> > 		
> > 		qc.id = ctrl->id;
> > 		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
> > 		if (ret < 0) {
> > 			ctrls->error_idx = i;
> > 			return ret;
> > 		}
> > 		ctrls->controls[i].value = qc.default_value;
> > 	}
> > 	ctrls->error_idx = 0;
> > 	
> > 	return 0;
> > }
> > 
> > The function could be called by v4l_g_def_ext_ctrls() when ops->
> > vidioc_g_def_ext_ctrls is NULL.

-- 
Regards,

Laurent Pinchart

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Hans Verkuil]

On 07/16/15 10:11, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
>> On 07/15/15 23:05, Laurent Pinchart wrote:
>>> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
>>>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
>>>> use the controller framework.
>>>>
>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>> ---
>>>>
>>>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>>>>  1 file changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>>>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
>>>> 100644
>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
>>>> *file, void *fh,
>>>>  	return uvc_ctrl_rollback(handle);
>>>>  }
>>>>
>>>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>>>> +				     struct v4l2_ext_controls *ctrls)
>>>> +{
>>>> +	struct uvc_fh *handle = fh;
>>>> +	struct uvc_video_chain *chain = handle->chain;
>>>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
>>>> +	unsigned int i;
>>>> +	int ret;
>>>> +	struct v4l2_queryctrl qc;
>>>> +
>>>> +	ret = uvc_ctrl_begin(chain);
>>>
>>> There's no need to call uvc_ctrl_begin() here (and if there was, you'd
>>> have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
>>>
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>>>> +		qc.id = ctrl->id;
>>>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
>>>> +		if (ret < 0) {
>>>> +			ctrls->error_idx = i;
>>>> +			return ret;
>>>> +		}
>>>> +		ctrl->value = qc.default_value;
>>>> +	}
>>>> +
>>>> +	ctrls->error_idx = 0;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Instead of open-coding this in multiple drivers, how about adding a helper
>>> function to the core ? Something like (totally untested)
>>
>> It's only open-coded in drivers that do not use the control framework. For
>> drivers that use the control framework it is completely transparent.
> 
> Sure, but still, the same function is implemented several times while a single 
> implementation could do. I'd prefer having it in the core until all (or all 
> but one) drivers are converted to the control framework.

There are only three drivers that need to implement this manually: uvc, saa7164
and pvrusb2. That's not enough to warrant moving this into the core.

One of these days I should sit down and convert saa7164 to the control framework.
That shouldn't be too difficult.

Regards,

	Hans

> 
>>> int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>>>                                struct v4l2_ext_controls *ctrls)
>>> {
>>> 	struct video_device *vdev = video_devdata(file);
>>> 	unsigned int i;
>>> 	int ret;
>>> 	
>>> 	for (i = 0; i < ctrls->count; ++i) {
>>> 		struct v4l2_queryctrl qc;
>>> 		
>>> 		qc.id = ctrl->id;
>>> 		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
>>> 		if (ret < 0) {
>>> 			ctrls->error_idx = i;
>>> 			return ret;
>>> 		}
>>> 		ctrls->controls[i].value = qc.default_value;
>>> 	}
>>> 	ctrls->error_idx = 0;
>>> 	
>>> 	return 0;
>>> }
>>>
>>> The function could be called by v4l_g_def_ext_ctrls() when ops->
>>> vidioc_g_def_ext_ctrls is NULL.
> 

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Laurent Pinchart]

On Thursday 16 July 2015 10:23:03 Hans Verkuil wrote:
> On 07/16/15 10:11, Laurent Pinchart wrote:
> > On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
> >> On 07/15/15 23:05, Laurent Pinchart wrote:
> >>> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> >>>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> >>>> use the controller framework.
> >>>> 
> >>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >>>> ---
> >>>> 
> >>>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
> >>>>  1 file changed, 30 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> >>>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
> >>>> 100644
> >>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
> >>>> *file, void *fh,
> >>>>  	return uvc_ctrl_rollback(handle);
> >>>>  }
> >>>> 
> >>>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> >>>> +				     struct v4l2_ext_controls *ctrls)
> >>>> +{
> >>>> +	struct uvc_fh *handle = fh;
> >>>> +	struct uvc_video_chain *chain = handle->chain;
> >>>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> >>>> +	unsigned int i;
> >>>> +	int ret;
> >>>> +	struct v4l2_queryctrl qc;
> >>>> +
> >>>> +	ret = uvc_ctrl_begin(chain);
> >>> 
> >>> There's no need to call uvc_ctrl_begin() here (and if there was, you'd
> >>> have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
> >>> 
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>> +
> >>>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> >>>> +		qc.id = ctrl->id;
> >>>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> >>>> +		if (ret < 0) {
> >>>> +			ctrls->error_idx = i;
> >>>> +			return ret;
> >>>> +		}
> >>>> +		ctrl->value = qc.default_value;
> >>>> +	}
> >>>> +
> >>>> +	ctrls->error_idx = 0;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>> 
> >>> Instead of open-coding this in multiple drivers, how about adding a
> >>> helper function to the core ? Something like (totally untested)
> >> 
> >> It's only open-coded in drivers that do not use the control framework.
> >> For drivers that use the control framework it is completely transparent.
> > 
> > Sure, but still, the same function is implemented several times while a
> > single implementation could do. I'd prefer having it in the core until
> > all (or all but one) drivers are converted to the control framework.
> 
> There are only three drivers that need to implement this manually: uvc,
> saa7164 and pvrusb2. That's not enough to warrant moving this into the
> core.

I'd argue that even just two drivers would be enough :-) Especially given that 
the proposed implementation for uvcvideo is wrong.

> One of these days I should sit down and convert saa7164 to the control
> framework. That shouldn't be too difficult.

How about pvrusb2, is there a good reason not to use the control framework 
there ?

-- 
Regards,

Laurent Pinchart

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Hans Verkuil]

On 07/16/15 10:27, Laurent Pinchart wrote:
> On Thursday 16 July 2015 10:23:03 Hans Verkuil wrote:
>> On 07/16/15 10:11, Laurent Pinchart wrote:
>>> On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
>>>> On 07/15/15 23:05, Laurent Pinchart wrote:
>>>>> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
>>>>>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
>>>>>> use the controller framework.
>>>>>>
>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 30 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
>>>>>> 100644
>>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
>>>>>> *file, void *fh,
>>>>>>  	return uvc_ctrl_rollback(handle);
>>>>>>  }
>>>>>>
>>>>>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>>>>>> +				     struct v4l2_ext_controls *ctrls)
>>>>>> +{
>>>>>> +	struct uvc_fh *handle = fh;
>>>>>> +	struct uvc_video_chain *chain = handle->chain;
>>>>>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
>>>>>> +	unsigned int i;
>>>>>> +	int ret;
>>>>>> +	struct v4l2_queryctrl qc;
>>>>>> +
>>>>>> +	ret = uvc_ctrl_begin(chain);
>>>>>
>>>>> There's no need to call uvc_ctrl_begin() here (and if there was, you'd
>>>>> have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
>>>>>
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>>>>>> +		qc.id = ctrl->id;
>>>>>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
>>>>>> +		if (ret < 0) {
>>>>>> +			ctrls->error_idx = i;
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +		ctrl->value = qc.default_value;
>>>>>> +	}
>>>>>> +
>>>>>> +	ctrls->error_idx = 0;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>
>>>>> Instead of open-coding this in multiple drivers, how about adding a
>>>>> helper function to the core ? Something like (totally untested)
>>>>
>>>> It's only open-coded in drivers that do not use the control framework.
>>>> For drivers that use the control framework it is completely transparent.
>>>
>>> Sure, but still, the same function is implemented several times while a
>>> single implementation could do. I'd prefer having it in the core until
>>> all (or all but one) drivers are converted to the control framework.
>>
>> There are only three drivers that need to implement this manually: uvc,
>> saa7164 and pvrusb2. That's not enough to warrant moving this into the
>> core.
> 
> I'd argue that even just two drivers would be enough :-) Especially given that 
> the proposed implementation for uvcvideo is wrong.

I don't want to add code to the core to cater to exceptions.

>> One of these days I should sit down and convert saa7164 to the control
>> framework. That shouldn't be too difficult.
> 
> How about pvrusb2, is there a good reason not to use the control framework 
> there ?

Brrr. pvrusb2 exports controls (and a bunch of other things) to /sys allowing
scripts to get/set controls using cat and echo. I have no idea how to switch pvrusb2
to the control framework while still keeping this non-standard API.

I guess one of these days I will need to look at it, but I've been postponing it
for years now :-)

There are still a few other drivers that do not use the control framework:

saa7164, zoran, fsl-viu, omap_vout (although I hope that one can be removed),
usbvision and bcm2048 (that's a staging driver, so I can probably ignore that one).

pvrusb2 is actually fairly important: v4l2-subdev still has legacy ops for handling
controls and as long as pvrusb2 is not converted I cannot remove those ops.

Regards,

	Hans

Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

[Ricardo Ribalda Delgado]

Hello Laurent

On Thu, Jul 16, 2015 at 10:27 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I'd argue that even just two drivers would be enough :-) Especially given that
> the proposed implementation for uvcvideo is wrong.

This is why we have the review process :P. I do my best, but you are
the expert on your driver.

A core implementation cannot be completely correct, as it will not
support array controls.

I will resend this patch ASAP.


Thanks for your comments!


-- 
Ricardo Ribalda

[RFC v3 05/19] media/pci/saa7164-encoder: Implement vivioc_g_def_ext_ctrls

[Ricardo Ribalda Delgado]

Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
use the controller framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/pci/saa7164/saa7164-encoder.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/pci/saa7164/saa7164-encoder.c b/drivers/media/pci/saa7164/saa7164-encoder.c
index 4434e0f28c26..63840b0000c0 100644
--- a/drivers/media/pci/saa7164/saa7164-encoder.c
+++ b/drivers/media/pci/saa7164/saa7164-encoder.c
@@ -884,6 +884,33 @@ static int vidioc_queryctrl(struct file *file, void *priv,
 	return -EINVAL;
 }
 
+static int vidioc_g_def_ext_ctrls(struct file *file, void *priv,
+	struct v4l2_ext_controls *ctrls)
+{
+	struct saa7164_encoder_fh *fh = file->private_data;
+	struct saa7164_port *port = fh->port;
+	int i, err = 0;
+	struct v4l2_queryctrl q;
+
+	if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+		for (i = 0; i < ctrls->count; i++) {
+			struct v4l2_ext_control *ctrl = ctrls->controls + i;
+
+			q.id = ctrl->id;
+			err = fill_queryctrl(&port->encoder_params, &q);
+			if (err) {
+				ctrls->error_idx = i;
+				break;
+			}
+			ctrl->value = q.default_value;
+		}
+		return err;
+
+	}
+
+	return -EINVAL;
+}
+
 static int saa7164_encoder_stop_port(struct saa7164_port *port)
 {
 	struct saa7164_dev *dev = port->dev;
@@ -1317,6 +1344,7 @@ static const struct v4l2_ioctl_ops mpeg_ioctl_ops = {
 	.vidioc_try_fmt_vid_cap	 = vidioc_try_fmt_vid_cap,
 	.vidioc_s_fmt_vid_cap	 = vidioc_s_fmt_vid_cap,
 	.vidioc_g_ext_ctrls	 = vidioc_g_ext_ctrls,
+	.vidioc_g_def_ext_ctrls	 = vidioc_g_def_ext_ctrls,
 	.vidioc_s_ext_ctrls	 = vidioc_s_ext_ctrls,
 	.vidioc_try_ext_ctrls	 = vidioc_try_ext_ctrls,
 	.vidioc_queryctrl	 = vidioc_queryctrl,
-- 
2.1.4

[RFC v3 06/19] media/pci/saa7164-vbi: Implement vivioc_g_def_ext_ctrls

[Ricardo Ribalda Delgado]

Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
use the controller framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/pci/saa7164/saa7164-vbi.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/pci/saa7164/saa7164-vbi.c b/drivers/media/pci/saa7164/saa7164-vbi.c
index 859fd03d82f9..e3f6cf8e83ee 100644
--- a/drivers/media/pci/saa7164/saa7164-vbi.c
+++ b/drivers/media/pci/saa7164/saa7164-vbi.c
@@ -810,6 +810,33 @@ static int vidioc_queryctrl(struct file *file, void *priv,
 	return -EINVAL;
 }
 
+static int vidioc_g_def_ext_ctrls(struct file *file, void *priv,
+	struct v4l2_ext_controls *ctrls)
+{
+	struct saa7164_vbi_fh *fh = file->private_data;
+	struct saa7164_port *port = fh->port;
+	int i, err = 0;
+	struct v4l2_queryctrl q;
+
+	if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+		for (i = 0; i < ctrls->count; i++) {
+			struct v4l2_ext_control *ctrl = ctrls->controls + i;
+
+			q.id = ctrl->id;
+			err = fill_queryctrl(&port->vbi_params, &q);
+			if (err) {
+				ctrls->error_idx = i;
+				break;
+			}
+			ctrl->value = q.default_value;
+		}
+		return err;
+
+	}
+
+	return -EINVAL;
+}
+
 static int saa7164_vbi_stop_port(struct saa7164_port *port)
 {
 	struct saa7164_dev *dev = port->dev;
@@ -1263,6 +1290,7 @@ static const struct v4l2_ioctl_ops vbi_ioctl_ops = {
 	.vidioc_try_fmt_vid_cap	 = vidioc_try_fmt_vid_cap,
 	.vidioc_s_fmt_vid_cap	 = vidioc_s_fmt_vid_cap,
 	.vidioc_g_ext_ctrls	 = vidioc_g_ext_ctrls,
+	.vidioc_g_def_ext_ctrls	 = vidioc_g_def_ext_ctrls,
 	.vidioc_s_ext_ctrls	 = vidioc_s_ext_ctrls,
 	.vidioc_try_ext_ctrls	 = vidioc_try_ext_ctrls,
 	.vidioc_queryctrl	 = vidioc_queryctrl,
-- 
2.1.4

[RFC v3 07/19] media/usb/prusb2: Implement vivioc_g_def_ext_ctrls

[Ricardo Ribalda Delgado]

Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
use the controller framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 1c5f85bf7ed4..16198c53ffa3 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -649,6 +649,33 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
 	return 0;
 }
 
+static int pvr2_g_def_ext_ctrls(struct file *file, void *priv,
+					struct v4l2_ext_controls *ctls)
+{
+	struct pvr2_v4l2_fh *fh = file->private_data;
+	struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
+	struct v4l2_ext_control *ctrl;
+	unsigned int idx;
+	int ret;
+	struct pvr2_ctrl *cptr;
+
+	ret = 0;
+	for (idx = 0; idx < ctls->count; idx++) {
+		ctrl = ctls->controls + idx;
+		cptr = pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id);
+		if (!ctrl){
+			ctls->error_idx = idx;
+			return -EINVAL;
+		}
+
+		/* Ensure that if read as a 64 bit value, the user
+		   will still get a hopefully sane value */
+		ctrl->value64 = 0;
+		pvr2_ctrl_get_def(cptr, &ctrl->value);
+	}
+	return 0;
+}
+
 static int pvr2_s_ext_ctrls(struct file *file, void *priv,
 		struct v4l2_ext_controls *ctls)
 {
@@ -809,6 +836,7 @@ static const struct v4l2_ioctl_ops pvr2_ioctl_ops = {
 	.vidioc_g_ctrl			    = pvr2_g_ctrl,
 	.vidioc_s_ctrl			    = pvr2_s_ctrl,
 	.vidioc_g_ext_ctrls		    = pvr2_g_ext_ctrls,
+	.vidioc_g_def_ext_ctrls		    = pvr2_g_def_ext_ctrls,
 	.vidioc_s_ext_ctrls		    = pvr2_s_ext_ctrls,
 	.vidioc_try_ext_ctrls		    = pvr2_try_ext_ctrls,
 };
-- 
2.1.4

[RFC v3 08/19] v4l2-subdev: Add g_def_ext_ctrls to core_ops

[Ricardo Ribalda Delgado]

This function returns the default value of an extended control. Provides
sub-devices with the same functionality as ioctl VIDIOC_G_DEF_EXT_CTRLS.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
 include/media/v4l2-ctrls.h           | 2 ++
 include/media/v4l2-subdev.h          | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 02ff6f573fd2..2a42b826f857 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2888,6 +2888,13 @@ int v4l2_subdev_g_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 }
 EXPORT_SYMBOL(v4l2_subdev_g_ext_ctrls);
 
+int v4l2_subdev_g_def_ext_ctrls(struct v4l2_subdev *sd,
+				struct v4l2_ext_controls *cs)
+{
+	return v4l2_g_ext_ctrls(sd->ctrl_handler, cs, true);
+}
+EXPORT_SYMBOL(v4l2_subdev_g_def_ext_ctrls);
+
 /* Helper function to get a single control */
 static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
 {
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 16f16b67181b..61e6cd67fc10 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -823,6 +823,8 @@ int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 int v4l2_subdev_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc);
 int v4l2_subdev_querymenu(struct v4l2_subdev *sd, struct v4l2_querymenu *qm);
 int v4l2_subdev_g_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs);
+int v4l2_subdev_g_def_ext_ctrls(struct v4l2_subdev *sd,
+				struct v4l2_ext_controls *cs);
 int v4l2_subdev_try_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs);
 int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs);
 int v4l2_subdev_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index dc20102ff600..01b3354942cf 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -158,6 +158,8 @@ struct v4l2_subdev_core_ops {
 	int (*g_ctrl)(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
 	int (*s_ctrl)(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
 	int (*g_ext_ctrls)(struct v4l2_subdev *sd, struct v4l2_ext_controls *ctrls);
+	int (*g_def_ext_ctrls)(struct v4l2_subdev *sd,
+			       struct v4l2_ext_controls *ctrls);
 	int (*s_ext_ctrls)(struct v4l2_subdev *sd, struct v4l2_ext_controls *ctrls);
 	int (*try_ext_ctrls)(struct v4l2_subdev *sd, struct v4l2_ext_controls *ctrls);
 	int (*querymenu)(struct v4l2_subdev *sd, struct v4l2_querymenu *qm);
-- 
2.1.4

[RFC v3 09/19] media/i2c/bt819: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/bt819.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/bt819.c b/drivers/media/i2c/bt819.c
index 76b334a6a56d..b3e9ac3616b5 100644
--- a/drivers/media/i2c/bt819.c
+++ b/drivers/media/i2c/bt819.c
@@ -381,6 +381,7 @@ static const struct v4l2_ctrl_ops bt819_ctrl_ops = {
 
 static const struct v4l2_subdev_core_ops bt819_core_ops = {
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 10/19] media/i2c/cs53l32a: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/cs53l32a.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/cs53l32a.c b/drivers/media/i2c/cs53l32a.c
index 27400c16ef9a..9358350278cc 100644
--- a/drivers/media/i2c/cs53l32a.c
+++ b/drivers/media/i2c/cs53l32a.c
@@ -122,6 +122,7 @@ static const struct v4l2_ctrl_ops cs53l32a_ctrl_ops = {
 static const struct v4l2_subdev_core_ops cs53l32a_core_ops = {
 	.log_status = cs53l32a_log_status,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 11/19] media/i2c/cx25840/cx25840-core: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/cx25840/cx25840-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/cx25840/cx25840-core.c b/drivers/media/i2c/cx25840/cx25840-core.c
index e15a789ad596..5d8d25e6bbf0 100644
--- a/drivers/media/i2c/cx25840/cx25840-core.c
+++ b/drivers/media/i2c/cx25840/cx25840-core.c
@@ -5044,6 +5044,7 @@ static const struct v4l2_subdev_core_ops cx25840_core_ops = {
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.queryctrl = v4l2_subdev_queryctrl,
 	.querymenu = v4l2_subdev_querymenu,
 	.reset = cx25840_reset,
-- 
2.1.4

[RFC v3 12/19] media/i2c/msp3400-driver: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/msp3400-driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/msp3400-driver.c b/drivers/media/i2c/msp3400-driver.c
index dcc68ec71732..10837564af8e 100644
--- a/drivers/media/i2c/msp3400-driver.c
+++ b/drivers/media/i2c/msp3400-driver.c
@@ -643,6 +643,7 @@ static const struct v4l2_ctrl_ops msp_ctrl_ops = {
 static const struct v4l2_subdev_core_ops msp_core_ops = {
 	.log_status = msp_log_status,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 13/19] media/i2c/saa7110: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/saa7110.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/saa7110.c b/drivers/media/i2c/saa7110.c
index 99689ee57d7e..964cc2cf1508 100644
--- a/drivers/media/i2c/saa7110.c
+++ b/drivers/media/i2c/saa7110.c
@@ -359,6 +359,7 @@ static const struct v4l2_ctrl_ops saa7110_ctrl_ops = {
 
 static const struct v4l2_subdev_core_ops saa7110_core_ops = {
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 14/19] media/i2c/saa7115: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/saa7115.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index 0eae5f4471e2..c227dc11b136 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -1582,6 +1582,7 @@ static const struct v4l2_ctrl_ops saa711x_ctrl_ops = {
 static const struct v4l2_subdev_core_ops saa711x_core_ops = {
 	.log_status = saa711x_log_status,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 15/19] media/i2c/tlv320aic23b: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/tlv320aic23b.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/tlv320aic23b.c b/drivers/media/i2c/tlv320aic23b.c
index ef87f7b09ea2..4e279de8e194 100644
--- a/drivers/media/i2c/tlv320aic23b.c
+++ b/drivers/media/i2c/tlv320aic23b.c
@@ -123,6 +123,7 @@ static const struct v4l2_ctrl_ops tlv320aic23b_ctrl_ops = {
 static const struct v4l2_subdev_core_ops tlv320aic23b_core_ops = {
 	.log_status = tlv320aic23b_log_status,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 16/19] media/i2c/vpx3220: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/vpx3220.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/vpx3220.c b/drivers/media/i2c/vpx3220.c
index 016e766e72ba..60d635250b19 100644
--- a/drivers/media/i2c/vpx3220.c
+++ b/drivers/media/i2c/vpx3220.c
@@ -451,6 +451,7 @@ static const struct v4l2_ctrl_ops vpx3220_ctrl_ops = {
 static const struct v4l2_subdev_core_ops vpx3220_core_ops = {
 	.init = vpx3220_init,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 17/19] media/i2c/wm8775: Implement g_def_ext_ctrls core_op

[Ricardo Ribalda Delgado]

Via control framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/wm8775.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/wm8775.c b/drivers/media/i2c/wm8775.c
index bee7946faa7c..45d4873aca74 100644
--- a/drivers/media/i2c/wm8775.c
+++ b/drivers/media/i2c/wm8775.c
@@ -179,6 +179,7 @@ static const struct v4l2_ctrl_ops wm8775_ctrl_ops = {
 static const struct v4l2_subdev_core_ops wm8775_core_ops = {
 	.log_status = wm8775_log_status,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 	.g_ctrl = v4l2_subdev_g_ctrl,
-- 
2.1.4

[RFC v3 18/19] Docbook: media: new ioctl VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

Add documentation for new ioctl.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 Documentation/DocBook/media/v4l/v4l2.xml               |  8 ++++++++
 Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 13 +++++++++----
 Documentation/video4linux/v4l2-controls.txt            |  3 ++-
 Documentation/video4linux/v4l2-framework.txt           |  1 +
 Documentation/zh_CN/video4linux/v4l2-framework.txt     |  1 +
 5 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml
index e98caa1c39bd..027cf8408382 100644
--- a/Documentation/DocBook/media/v4l/v4l2.xml
+++ b/Documentation/DocBook/media/v4l/v4l2.xml
@@ -153,6 +153,14 @@ structs, ioctls) must be noted in more detail in the history chapter
 applications. -->
 
       <revision>
+	<revnumber>4.2</revnumber>
+	<date>2015-06-12</date>
+	<authorinitials>rr</authorinitials>
+	<revremark>Extend &vidioc-g-ext-ctrls;. Add ioctl <constant>VIDIOC_G_DEF_EXT_CTRLS</constant>
+to get the default value of multiple controls.
+	</revremark>
+      </revision>
+      <revision>
 	<revnumber>3.21</revnumber>
 	<date>2015-02-13</date>
 	<authorinitials>mcc</authorinitials>
diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
index c5bdbfcc42b3..5f8283a7e288 100644
--- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
@@ -1,12 +1,13 @@
 <refentry id="vidioc-g-ext-ctrls">
   <refmeta>
     <refentrytitle>ioctl VIDIOC_G_EXT_CTRLS, VIDIOC_S_EXT_CTRLS,
-VIDIOC_TRY_EXT_CTRLS</refentrytitle>
+VIDIOC_TRY_EXT_CTRLS, VIDIOC_G_DEF_EXT_CTRLS</refentrytitle>
     &manvol;
   </refmeta>
 
   <refnamediv>
     <refname>VIDIOC_G_EXT_CTRLS</refname>
+    <refname>VIDIOC_G_DEF_EXT_CTRLS</refname>
     <refname>VIDIOC_S_EXT_CTRLS</refname>
     <refname>VIDIOC_TRY_EXT_CTRLS</refname>
     <refpurpose>Get or set the value of several controls, try control
@@ -39,7 +40,7 @@ values</refpurpose>
 	<term><parameter>request</parameter></term>
 	<listitem>
 	  <para>VIDIOC_G_EXT_CTRLS, VIDIOC_S_EXT_CTRLS,
-VIDIOC_TRY_EXT_CTRLS</para>
+VIDIOC_TRY_EXT_CTRLS, VIDIOC_G_DEF_EXT_CTRLS</para>
 	</listitem>
       </varlistentry>
       <varlistentry>
@@ -74,7 +75,10 @@ of each &v4l2-ext-control; and call the
 <constant>VIDIOC_G_EXT_CTRLS</constant> ioctl. String controls controls
 must also set the <structfield>string</structfield> field. Controls
 of compound types (<constant>V4L2_CTRL_FLAG_HAS_PAYLOAD</constant> is set)
-must set the <structfield>ptr</structfield> field.</para>
+must set the <structfield>ptr</structfield> field. To get the default value
+instead of the current value, call the
+<constant>VIDIOC_G_DEF_EXT_CTRLS</constant> ioctl with the same arguments.
+</para>
 
     <para>If the <structfield>size</structfield> is too small to
 receive the control result (only relevant for pointer-type controls
@@ -141,7 +145,8 @@ application.</entry>
 	    <entry>The total size in bytes of the payload of this
 control. This is normally 0, but for pointer controls this should be
 set to the size of the memory containing the payload, or that will
-receive the payload. If <constant>VIDIOC_G_EXT_CTRLS</constant> finds
+receive the payload. If <constant>VIDIOC_G_EXT_CTRLS</constant>
+or <constant>VIDIOC_G_DEF_EXT_CTRLS</constant> finds
 that this value is less than is required to store
 the payload result, then it is set to a value large enough to store the
 payload result and ENOSPC is returned. Note that for string controls
diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 5517db602f37..7e3dfcacdbee 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -79,7 +79,8 @@ Basic usage for V4L2 and sub-device drivers
 
   Finally, remove all control functions from your v4l2_ioctl_ops (if any):
   vidioc_queryctrl, vidioc_query_ext_ctrl, vidioc_querymenu, vidioc_g_ctrl,
-  vidioc_s_ctrl, vidioc_g_ext_ctrls, vidioc_try_ext_ctrls and vidioc_s_ext_ctrls.
+  vidioc_s_ctrl, vidioc_g_ext_ctrls, vidioc_try_ext_ctrls,
+  vidioc_g_def_ext_ctrls, and vidioc_s_ext_ctrls.
   Those are now no longer needed.
 
 1.3.2) For sub-device drivers do this:
diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 75d5c18d689a..4672396f48b1 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -462,6 +462,7 @@ VIDIOC_QUERYMENU
 VIDIOC_G_CTRL
 VIDIOC_S_CTRL
 VIDIOC_G_EXT_CTRLS
+VIDIOC_G_DEF_EXT_CTRLS
 VIDIOC_S_EXT_CTRLS
 VIDIOC_TRY_EXT_CTRLS
 
diff --git a/Documentation/zh_CN/video4linux/v4l2-framework.txt b/Documentation/zh_CN/video4linux/v4l2-framework.txt
index 2b828e631e31..b8c0d6fb6595 100644
--- a/Documentation/zh_CN/video4linux/v4l2-framework.txt
+++ b/Documentation/zh_CN/video4linux/v4l2-framework.txt
@@ -401,6 +401,7 @@ VIDIOC_QUERYMENU
 VIDIOC_G_CTRL
 VIDIOC_S_CTRL
 VIDIOC_G_EXT_CTRLS
+VIDIOC_G_DEF_EXT_CTRLS
 VIDIOC_S_EXT_CTRLS
 VIDIOC_TRY_EXT_CTRLS
 
-- 
2.1.4

[RFC v3 19/19] Documentation: media: Fix code sample

[Ricardo Ribalda Delgado]

Add newly created core op to the example.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 Documentation/video4linux/v4l2-controls.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 7e3dfcacdbee..1d25de0199c4 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -105,6 +105,7 @@ Basic usage for V4L2 and sub-device drivers
 	.g_ctrl = v4l2_subdev_g_ctrl,
 	.s_ctrl = v4l2_subdev_s_ctrl,
 	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.g_def_ext_ctrls = v4l2_subdev_g_def_ext_ctrls,
 	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
 	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
 
-- 
2.1.4

Re: [RFC v3 00/19] New ioct VIDIOC_G_DEF_EXT_CTRLS

[Hans Verkuil]

Hi Ricardo,

This patch series looks good to me. I'll wait a bit to see if anyone else has
comments. If not, then I'll merge for 4.3.

Regards,

	Hans

On 06/12/2015 06:46 PM, Ricardo Ribalda Delgado wrote:
> Integer controls provide a way to get their default/initial value, but
> any other control (p_u32, p_u8.....) provide no other way to get the
> initial value than unloading the module and loading it back.
> 
> *What is the actual problem?
> I have a custom control with WIDTH integer values. Every value
> represents the calibrated FPN (fixed pattern noise) correction value for that
> column
> -Application A changes the FPN correction value
> -Application B wants to restore the calibrated value but it cant :(
> 
> *What is the proposed solution?
> -Add a new ioctl VIDIOC_G_DEF_EXT_CTRLS, with the same API as
> G_EXT_CTRLS, but that returns the initial value of a given control.
> 
> 
> I have posted a copy of my working tree to
> 
> https://github.com/ribalda/linux/tree/g_def_ext-rfc3
> 
> It has been tested with a hacked version of yavta (for normal controls) and a
> custom program for the array control.
> 
> Changelog v3:
> -Comments by Hans Verkuil:
> -Remove the control ops from the following drivers
> saa7706
> ivtv-gpio
> wm8739
> tvp7002
> tvp514x
> tvl320aic23b
> tda7432
> sr030pc30
> saa717x
> cs5345
> adv7393
> adv7343
> 
> Changelog v2:
> -Add documentation
> -Split in multiple patches
> -Comments by Hans Verkuil:
> -Rename ioctl to G_DEF_EXT_CTRL
> -Much! better implementation of def_to_user
> 
> 
> THANKS!
> 
> 
> Ricardo Ribalda Delgado (19):
>   media/v4l2-core: Add argument def_value to g_ext_ctrl
>   media/v4l2-core: add new ioctl VIDIOC_G_DEF_EXT_CTRLS
>   videodev2.h: Fix typo in comment
>   media/usb/uvc: Implement vivioc_g_def_ext_ctrls
>   media/pci/saa7164-encoder: Implement vivioc_g_def_ext_ctrls
>   media/pci/saa7164-vbi: Implement vivioc_g_def_ext_ctrls
>   media/usb/prusb2: Implement vivioc_g_def_ext_ctrls
>   v4l2-subdev: Add g_def_ext_ctrls to core_ops
>   media/i2c/bt819: Implement g_def_ext_ctrls core_op
>   media/i2c/cs53l32a: Implement g_def_ext_ctrls core_op
>   media/i2c/cx25840/cx25840-core: Implement g_def_ext_ctrls core_op
>   media/i2c/msp3400-driver: Implement g_def_ext_ctrls core_op
>   media/i2c/saa7110: Implement g_def_ext_ctrls core_op
>   media/i2c/saa7115: Implement g_def_ext_ctrls core_op
>   media/i2c/tlv320aic23b: Implement g_def_ext_ctrls core_op
>   media/i2c/vpx3220: Implement g_def_ext_ctrls core_op
>   media/i2c/wm8775: Implement g_def_ext_ctrls core_op
>   Docbook: media: new ioctl VIDIOC_G_DEF_EXT_CTRLS
>   Documentation: media: Fix code sample
> 
>  Documentation/DocBook/media/v4l/v4l2.xml           |  8 ++++++
>  .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       | 13 ++++++---
>  Documentation/video4linux/v4l2-controls.txt        |  4 ++-
>  Documentation/video4linux/v4l2-framework.txt       |  1 +
>  Documentation/zh_CN/video4linux/v4l2-framework.txt |  1 +
>  drivers/media/i2c/bt819.c                          |  1 +
>  drivers/media/i2c/cs53l32a.c                       |  1 +
>  drivers/media/i2c/cx25840/cx25840-core.c           |  1 +
>  drivers/media/i2c/msp3400-driver.c                 |  1 +
>  drivers/media/i2c/saa7110.c                        |  1 +
>  drivers/media/i2c/saa7115.c                        |  1 +
>  drivers/media/i2c/tlv320aic23b.c                   |  1 +
>  drivers/media/i2c/vpx3220.c                        |  1 +
>  drivers/media/i2c/wm8775.c                         |  1 +
>  drivers/media/pci/saa7164/saa7164-encoder.c        | 28 +++++++++++++++++++
>  drivers/media/pci/saa7164/saa7164-vbi.c            | 28 +++++++++++++++++++
>  drivers/media/platform/omap3isp/ispvideo.c         |  2 +-
>  drivers/media/usb/pvrusb2/pvrusb2-v4l2.c           | 28 +++++++++++++++++++
>  drivers/media/usb/uvc/uvc_v4l2.c                   | 30 ++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |  4 +++
>  drivers/media/v4l2-core/v4l2-ctrls.c               | 32 ++++++++++++++++++----
>  drivers/media/v4l2-core/v4l2-ioctl.c               | 25 +++++++++++++++--
>  drivers/media/v4l2-core/v4l2-subdev.c              |  5 +++-
>  include/media/v4l2-ctrls.h                         |  5 +++-
>  include/media/v4l2-ioctl.h                         |  2 ++
>  include/media/v4l2-subdev.h                        |  2 ++
>  include/uapi/linux/videodev2.h                     |  3 +-
>  27 files changed, 214 insertions(+), 16 deletions(-)
> 

Re: [RFC v3 00/19] New ioct VIDIOC_G_DEF_EXT_CTRLS

[Ricardo Ribalda Delgado]

Hello Hans

On Tue, Jun 16, 2015 at 8:09 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Ricardo,
>
> This patch series looks good to me. I'll wait a bit to see if anyone else has
> comments. If not, then I'll merge for 4.3.

If this is merged in 4.3 I need to fix the documentation.

Please use the last patch for that.

Thanks for your time!