Hi Dan,
Thanks for the review.
On Fri, Dec 16, 2022 at 7:37 PM Dan Scally <dan.scally@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx>
> > ---
> > 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);
