[v10,02/11] media: uvcvideo: add uvc_ctrl_get_boundary for getting default value
Commit Message
Introduce uvc_ctrl_get_boundary(), making it easier to extend to
support compound controls and V4L2_CTRL_WHICH_MIN/MAX_VAL in the
following patches.
For now, it simply returns EINVAL for compound controls and calls
__query_v4l2_ctrl() for non-compound controls.
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v9:
- Make __uvc_ctrl_get_boundary_std() static.
Changelog since v8:
- No Change.
Changelog since v7:
- Rename uvc_ctrl_get_fixed() to uvc_ctrl_get_boundary().
- Move refactor (introduce __uvc_ctrl_get_boundary_std()) in this patch
instead of in the patch where we implement compound control.
- Fix locking. uvc_ctrl_get_boundary() now acquires ctrl_mutex.
__uvc_ctrl_get_boundary_std() calls __uvc_query_v4l2_ctrl() while
holding the mutex.
- Define a uvc_ctrl_mapping_is_compound() for readability.
drivers/media/usb/uvc/uvc_ctrl.c | 49 ++++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvc_v4l2.c | 6 +---
drivers/media/usb/uvc/uvcvideo.h | 2 ++
3 files changed, 52 insertions(+), 5 deletions(-)
Comments
Hi Yunke
On 09/11/2022 06:06, Yunke Cao wrote:
> Introduce uvc_ctrl_get_boundary(), making it easier to extend to
> support compound controls and V4L2_CTRL_WHICH_MIN/MAX_VAL in the
> following patches.
>
> For now, it simply returns EINVAL for compound controls and calls
> __query_v4l2_ctrl() for non-compound controls.
>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v9:
> - Make __uvc_ctrl_get_boundary_std() static.
> Changelog since v8:
> - No Change.
> Changelog since v7:
> - Rename uvc_ctrl_get_fixed() to uvc_ctrl_get_boundary().
> - Move refactor (introduce __uvc_ctrl_get_boundary_std()) in this patch
> instead of in the patch where we implement compound control.
> - Fix locking. uvc_ctrl_get_boundary() now acquires ctrl_mutex.
> __uvc_ctrl_get_boundary_std() calls __uvc_query_v4l2_ctrl() while
> holding the mutex.
> - Define a uvc_ctrl_mapping_is_compound() for readability.
>
> drivers/media/usb/uvc/uvc_ctrl.c | 49 ++++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvc_v4l2.c | 6 +---
> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> 3 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index c95a2229f4fa..dfb9d1daece6 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -837,6 +837,12 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> }
> }
>
> +static bool
> +uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
> +{
> + return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES;
> +}
> +
> /* ------------------------------------------------------------------------
> * Terminal and unit management
> */
> @@ -1743,6 +1749,49 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
> return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> }
>
> +static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl,
> + struct uvc_control_mapping *mapping,
> + u32 v4l2_which,
> + struct v4l2_ext_control *xctrl)
Here you define a parameter for v4l2_which, but further down...
> +{
> + struct v4l2_queryctrl qc = { .id = xctrl->id };
> +
> + int ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &qc);
> +
> + if (ret < 0)
> + return ret;
> +
> + xctrl->value = qc.default_value;
> + return 0;
> +}
> +
> +int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> + struct v4l2_ext_control *xctrl)
> +{
> + struct uvc_control *ctrl;
> + struct uvc_control_mapping *mapping;
> + int ret;
> +
> + if (mutex_lock_interruptible(&chain->ctrl_mutex))
> + return -ERESTARTSYS;
> +
> + ctrl = uvc_find_control(chain, xctrl->id, &mapping);
> + if (!ctrl) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + if (uvc_ctrl_mapping_is_compound(mapping))
> + ret = -EINVAL;
> + else
> + ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
...here, you're not passing it, which causes a compilation error.
Otherwise I think the patch looks ok.
> +
> +done:
> + mutex_unlock(&chain->ctrl_mutex);
> + return ret;
> +}
> +
> int uvc_ctrl_set(struct uvc_fh *handle,
> struct v4l2_ext_control *xctrl)
> {
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4d4c33b6dfb..e807e348aa41 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1046,15 +1046,11 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>
> if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
> for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> - struct v4l2_queryctrl qc = { .id = ctrl->id };
> -
> - ret = uvc_query_v4l2_ctrl(chain, &qc);
> + ret = uvc_ctrl_get_boundary(chain, ctrl);
> if (ret < 0) {
> ctrls->error_idx = i;
> return ret;
> }
> -
> - ctrl->value = qc.default_value;
> }
>
> return 0;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..b2ee3d59a4c8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -759,6 +759,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> }
>
> int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
> +int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> + struct v4l2_ext_control *xctrl);
> int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
> int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> bool read);
Hi Dan,
Thanks for the review.
On Fri, Dec 16, 2022 at 7:37 PM Dan Scally <dan.scally@ideasonboard.com> wrote:
>
> Hi Yunke
>
> On 09/11/2022 06:06, Yunke Cao wrote:
> > Introduce uvc_ctrl_get_boundary(), making it easier to extend to
> > support compound controls and V4L2_CTRL_WHICH_MIN/MAX_VAL in the
> > following patches.
> >
> > For now, it simply returns EINVAL for compound controls and calls
> > __query_v4l2_ctrl() for non-compound controls.
> >
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: Yunke Cao <yunkec@google.com>
> > ---
> > Changelog since v9:
> > - Make __uvc_ctrl_get_boundary_std() static.
> > Changelog since v8:
> > - No Change.
> > Changelog since v7:
> > - Rename uvc_ctrl_get_fixed() to uvc_ctrl_get_boundary().
> > - Move refactor (introduce __uvc_ctrl_get_boundary_std()) in this patch
> > instead of in the patch where we implement compound control.
> > - Fix locking. uvc_ctrl_get_boundary() now acquires ctrl_mutex.
> > __uvc_ctrl_get_boundary_std() calls __uvc_query_v4l2_ctrl() while
> > holding the mutex.
> > - Define a uvc_ctrl_mapping_is_compound() for readability.
> >
> > drivers/media/usb/uvc/uvc_ctrl.c | 49 ++++++++++++++++++++++++++++++++
> > drivers/media/usb/uvc/uvc_v4l2.c | 6 +---
> > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > 3 files changed, 52 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index c95a2229f4fa..dfb9d1daece6 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -837,6 +837,12 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> > }
> > }
> >
> > +static bool
> > +uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
> > +{
> > + return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES;
> > +}
> > +
> > /* ------------------------------------------------------------------------
> > * Terminal and unit management
> > */
> > @@ -1743,6 +1749,49 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
> > return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> > }
> >
> > +static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> > + struct uvc_control *ctrl,
> > + struct uvc_control_mapping *mapping,
> > + u32 v4l2_which,
> > + struct v4l2_ext_control *xctrl)
>
>
> Here you define a parameter for v4l2_which, but further down...
>
> > +{
> > + struct v4l2_queryctrl qc = { .id = xctrl->id };
> > +
> > + int ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &qc);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + xctrl->value = qc.default_value;
> > + return 0;
> > +}
> > +
> > +int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> > + struct v4l2_ext_control *xctrl)
> > +{
> > + struct uvc_control *ctrl;
> > + struct uvc_control_mapping *mapping;
> > + int ret;
> > +
> > + if (mutex_lock_interruptible(&chain->ctrl_mutex))
> > + return -ERESTARTSYS;
> > +
> > + ctrl = uvc_find_control(chain, xctrl->id, &mapping);
> > + if (!ctrl) {
> > + ret = -EINVAL;
> > + goto done;
> > + }
> > +
> > + if (uvc_ctrl_mapping_is_compound(mapping))
> > + ret = -EINVAL;
> > + else
> > + ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
>
>
> ...here, you're not passing it, which causes a compilation error.
> Otherwise I think the patch looks ok.
>
Sorry, I messed this up during rebasing :P. The v4l2_which parameter
should be introduced as part of patch 09/10. Will fix it in the next
version.
Best,
Yunke
> > +
> > +done:
> > + mutex_unlock(&chain->ctrl_mutex);
> > + return ret;
> > +}
> > +
> > int uvc_ctrl_set(struct uvc_fh *handle,
> > struct v4l2_ext_control *xctrl)
> > {
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index f4d4c33b6dfb..e807e348aa41 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1046,15 +1046,11 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
> >
> > if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
> > for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> > - struct v4l2_queryctrl qc = { .id = ctrl->id };
> > -
> > - ret = uvc_query_v4l2_ctrl(chain, &qc);
> > + ret = uvc_ctrl_get_boundary(chain, ctrl);
> > if (ret < 0) {
> > ctrls->error_idx = i;
> > return ret;
> > }
> > -
> > - ctrl->value = qc.default_value;
> > }
> >
> > return 0;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index df93db259312..b2ee3d59a4c8 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -759,6 +759,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> > }
> >
> > int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
> > +int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> > + struct v4l2_ext_control *xctrl);
> > int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
> > int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> > bool read);
@@ -837,6 +837,12 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
}
}
+static bool
+uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
+{
+ return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES;
+}
+
/* ------------------------------------------------------------------------
* Terminal and unit management
*/
@@ -1743,6 +1749,49 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
}
+static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
+ struct uvc_control *ctrl,
+ struct uvc_control_mapping *mapping,
+ u32 v4l2_which,
+ struct v4l2_ext_control *xctrl)
+{
+ struct v4l2_queryctrl qc = { .id = xctrl->id };
+
+ int ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &qc);
+
+ if (ret < 0)
+ return ret;
+
+ xctrl->value = qc.default_value;
+ return 0;
+}
+
+int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
+ struct v4l2_ext_control *xctrl)
+{
+ struct uvc_control *ctrl;
+ struct uvc_control_mapping *mapping;
+ int ret;
+
+ if (mutex_lock_interruptible(&chain->ctrl_mutex))
+ return -ERESTARTSYS;
+
+ ctrl = uvc_find_control(chain, xctrl->id, &mapping);
+ if (!ctrl) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ if (uvc_ctrl_mapping_is_compound(mapping))
+ ret = -EINVAL;
+ else
+ ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
+
+done:
+ mutex_unlock(&chain->ctrl_mutex);
+ return ret;
+}
+
int uvc_ctrl_set(struct uvc_fh *handle,
struct v4l2_ext_control *xctrl)
{
@@ -1046,15 +1046,11 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
- struct v4l2_queryctrl qc = { .id = ctrl->id };
-
- ret = uvc_query_v4l2_ctrl(chain, &qc);
+ ret = uvc_ctrl_get_boundary(chain, ctrl);
if (ret < 0) {
ctrls->error_idx = i;
return ret;
}
-
- ctrl->value = qc.default_value;
}
return 0;
@@ -759,6 +759,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
}
int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
+int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
+ struct v4l2_ext_control *xctrl);
int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
bool read);