| Message ID | 1434127598-11719-3-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- 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
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!
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! */ > -- 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
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? -- 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
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 > -- 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
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 >> >
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
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.
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. > -- 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
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! */
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(+)