| Message ID | 20210318202928.166955-16-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | uvcvideo: Fix v4l2-compliance errors | expand |
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@chromium.org> > --- > 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. 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); >
Hello Hans On Fri, Mar 19, 2021 at 9:35 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@chromium.org> > > --- > > 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
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, + }; 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);
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@chromium.org> --- 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(-)