Hello Hans
On Fri, Mar 19, 2021 at 9:35 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> On 18/03/2021 21:29, Ricardo Ribalda wrote:
> > Take a v4l2_ext_controls instead of an array of controls, this way we
> > can access the error_idx in future changes.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 5 ++---
> > drivers/media/usb/uvc/uvc_v4l2.c | 8 ++++++--
> > drivers/media/usb/uvc/uvcvideo.h | 10 ++++------
> > 3 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 1ec8333811bc..fb8155ca0c0d 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1664,8 +1664,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> > }
> >
> > int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > - const struct v4l2_ext_control *xctrls,
> > - unsigned int xctrls_count)
> > + struct v4l2_ext_controls *ctrls)
> > {
> > struct uvc_video_chain *chain = handle->chain;
> > struct uvc_entity *entity;
> > @@ -1679,7 +1678,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > }
> >
> > if (!rollback)
> > - uvc_ctrl_send_events(handle, xctrls, xctrls_count);
> > + uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> > done:
> > mutex_unlock(&chain->ctrl_mutex);
> > return ret;
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index ddebdeb5a81b..ea2c41b04664 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1025,6 +1025,10 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> > struct uvc_fh *handle = fh;
> > struct uvc_video_chain *chain = handle->chain;
> > struct v4l2_ext_control xctrl;
> > + struct v4l2_ext_controls ctrls = {
> > + .count = 1,
> > + .controls = &xctrl,
> > + };
>
> Rather than creating this extra ctrls struct, I think this can be simplified by just
> removing uvc_ioctl_s_ctrl and uvc_ioctl_g_ctrl altogether. The v4l2-ioctl.c source
> will call vidioc_g/s_ext_ctrls if the driver doesn't provide vidioc_g/s_ctrl ops.
>
> Let's just simplify this by adding another patch before this one that just removes
> uvc_ioctl_s_ctrl and uvc_ioctl_g_ctrl.
>
> Otherwise this patch looks good.
I think I have found a 13 year old bug.....
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v8&id=7034e9ed5a8203c73cef9ab972ece48ade70b22f
>
> Regards,
>
> Hans
>
> > int ret;
> >
> > ret = uvc_ctrl_is_accessible(chain, ctrl->id, VIDIOC_S_CTRL);
> > @@ -1045,7 +1049,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> > return ret;
> > }
> >
> > - ret = uvc_ctrl_commit(handle, &xctrl, 1);
> > + ret = uvc_ctrl_commit(handle, &ctrls);
> > if (ret < 0)
> > return ret;
> >
> > @@ -1149,7 +1153,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> > ctrls->error_idx = 0;
> >
> > if (ioctl == VIDIOC_S_EXT_CTRLS)
> > - return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
> > + return uvc_ctrl_commit(handle, ctrls);
> > else
> > return uvc_ctrl_rollback(handle);
> > }
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index a93aeedb5499..4e7b6da3c6d2 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -887,17 +887,15 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> >
> > int uvc_ctrl_begin(struct uvc_video_chain *chain);
> > int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > - const struct v4l2_ext_control *xctrls,
> > - unsigned int xctrls_count);
> > + struct v4l2_ext_controls *ctrls);
> > static inline int uvc_ctrl_commit(struct uvc_fh *handle,
> > - const struct v4l2_ext_control *xctrls,
> > - unsigned int xctrls_count)
> > + struct v4l2_ext_controls *ctrls)
> > {
> > - return __uvc_ctrl_commit(handle, 0, xctrls, xctrls_count);
> > + return __uvc_ctrl_commit(handle, 0, ctrls);
> > }
> > static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> > {
> > - return __uvc_ctrl_commit(handle, 1, NULL, 0);
> > + return __uvc_ctrl_commit(handle, 1, NULL);
> > }
> >
> > int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
> >
>
--
Ricardo Ribalda
