| Message ID | 20210618122923.385938-22-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v10,01/21] media: v4l2-ioctl: Fix check_ext_ctrls | expand |
Hi Hans Did you have some hardware that did not work fine without this patch? Am I remembering correctly? Thanks! On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: > > If a control is inactive return -EACCES to let the userspace know that > the value will not be applied automatically when the control is active > again. > > Also make sure that query_v4l2_ctrl doesn't return an error. > > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- > 1 file changed, 49 insertions(+), 24 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index da44d5c0b9ad..4f80c06d3c43 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) > return "Unknown Control"; > } > > +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping) > +{ > + struct uvc_control_mapping *master_map = NULL; > + struct uvc_control *master_ctrl = NULL; > + s32 val; > + int ret; > + > + if (!mapping->master_id) > + return false; > + > + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > + &master_ctrl, 0); > + > + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > + return false; > + > + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > + if (ret < 0 || val == mapping->master_manual) > + return false; > + > + return true; > +} > + > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > struct uvc_control *ctrl, > struct uvc_control_mapping *mapping, > struct v4l2_queryctrl *v4l2_ctrl) > { > - struct uvc_control_mapping *master_map = NULL; > - struct uvc_control *master_ctrl = NULL; > const struct uvc_menu_info *menu; > unsigned int i; > > @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > - if (mapping->master_id) > - __uvc_find_control(ctrl->entity, mapping->master_id, > - &master_map, &master_ctrl, 0); > - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > - s32 val; > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > - if (ret < 0) > - return ret; > - > - if (val != mapping->master_manual) > - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > - } > + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > > if (!ctrl->cached) { > int ret = uvc_ctrl_populate_cache(chain, ctrl); > @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > return 0; > } > > -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > - struct v4l2_ext_controls *ctrls, > - struct uvc_control *uvc_control) > +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, > + struct uvc_entity *entity, > + struct v4l2_ext_controls *ctrls, > + struct uvc_control *err_control, > + int ret) > { > struct uvc_control_mapping *mapping; > struct uvc_control *ctrl_found; > unsigned int i; > > - if (!entity) > - return ctrls->count; > + if (!entity) { > + ctrls->error_idx = ctrls->count; > + return ret; > + } > > for (i = 0; i < ctrls->count; i++) { > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > &ctrl_found, 0); > - if (uvc_control == ctrl_found) > - return i; > + if (err_control == ctrl_found) > + break; > } > + ctrls->error_idx = i; > + > + /* We could not find the control that failed. */ > + if (i == ctrls->count) > + return ret; > + > + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) > + return -EACCES; > > - return ctrls->count; > + return ret; > } > > int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); > done: > if (ret < 0 && ctrls) > - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, > - err_ctrl); > + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, > + ret); > mutex_unlock(&chain->ctrl_mutex); > return ret; > } > -- > 2.32.0.288.g62a8d224e6-goog > -- Ricardo Ribalda
On 25/06/2021 12:29, Ricardo Ribalda wrote: > Hi Hans > > Did you have some hardware that did not work fine without this patch? > Am I remembering correctly? Yes, that's correct. It's one of my webcams, but I can't remember which one it is. You probably want me to test this v10? Regards, Hans > > Thanks! > > On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: >> >> If a control is inactive return -EACCES to let the userspace know that >> the value will not be applied automatically when the control is active >> again. >> >> Also make sure that query_v4l2_ctrl doesn't return an error. >> >> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >> --- >> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- >> 1 file changed, 49 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >> index da44d5c0b9ad..4f80c06d3c43 100644 >> --- a/drivers/media/usb/uvc/uvc_ctrl.c >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) >> return "Unknown Control"; >> } >> >> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, >> + struct uvc_control *ctrl, >> + struct uvc_control_mapping *mapping) >> +{ >> + struct uvc_control_mapping *master_map = NULL; >> + struct uvc_control *master_ctrl = NULL; >> + s32 val; >> + int ret; >> + >> + if (!mapping->master_id) >> + return false; >> + >> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, >> + &master_ctrl, 0); >> + >> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) >> + return false; >> + >> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >> + if (ret < 0 || val == mapping->master_manual) >> + return false; >> + >> + return true; >> +} >> + >> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >> struct uvc_control *ctrl, >> struct uvc_control_mapping *mapping, >> struct v4l2_queryctrl *v4l2_ctrl) >> { >> - struct uvc_control_mapping *master_map = NULL; >> - struct uvc_control *master_ctrl = NULL; >> const struct uvc_menu_info *menu; >> unsigned int i; >> >> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) >> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >> >> - if (mapping->master_id) >> - __uvc_find_control(ctrl->entity, mapping->master_id, >> - &master_map, &master_ctrl, 0); >> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { >> - s32 val; >> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >> - if (ret < 0) >> - return ret; >> - >> - if (val != mapping->master_manual) >> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >> - } >> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) >> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >> >> if (!ctrl->cached) { >> int ret = uvc_ctrl_populate_cache(chain, ctrl); >> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, >> return 0; >> } >> >> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, >> - struct v4l2_ext_controls *ctrls, >> - struct uvc_control *uvc_control) >> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, >> + struct uvc_entity *entity, >> + struct v4l2_ext_controls *ctrls, >> + struct uvc_control *err_control, >> + int ret) >> { >> struct uvc_control_mapping *mapping; >> struct uvc_control *ctrl_found; >> unsigned int i; >> >> - if (!entity) >> - return ctrls->count; >> + if (!entity) { >> + ctrls->error_idx = ctrls->count; >> + return ret; >> + } >> >> for (i = 0; i < ctrls->count; i++) { >> __uvc_find_control(entity, ctrls->controls[i].id, &mapping, >> &ctrl_found, 0); >> - if (uvc_control == ctrl_found) >> - return i; >> + if (err_control == ctrl_found) >> + break; >> } >> + ctrls->error_idx = i; >> + >> + /* We could not find the control that failed. */ >> + if (i == ctrls->count) >> + return ret; >> + >> + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) >> + return -EACCES; >> >> - return ctrls->count; >> + return ret; >> } >> >> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, >> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, >> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); >> done: >> if (ret < 0 && ctrls) >> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, >> - err_ctrl); >> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, >> + ret); >> mutex_unlock(&chain->ctrl_mutex); >> return ret; >> } >> -- >> 2.32.0.288.g62a8d224e6-goog >> > >
Hi Hans On Fri, 25 Jun 2021 at 13:07, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 25/06/2021 12:29, Ricardo Ribalda wrote: > > Hi Hans > > > > Did you have some hardware that did not work fine without this patch? > > Am I remembering correctly? > > Yes, that's correct. It's one of my webcams, but I can't remember which one > it is. You probably want me to test this v10? > > Regards, That would be awesome. Thanks! > > Hans > > > > > Thanks! > > > > On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: > >> > >> If a control is inactive return -EACCES to let the userspace know that > >> the value will not be applied automatically when the control is active > >> again. > >> > >> Also make sure that query_v4l2_ctrl doesn't return an error. > >> > >> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >> --- > >> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- > >> 1 file changed, 49 insertions(+), 24 deletions(-) > >> > >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > >> index da44d5c0b9ad..4f80c06d3c43 100644 > >> --- a/drivers/media/usb/uvc/uvc_ctrl.c > >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c > >> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) > >> return "Unknown Control"; > >> } > >> > >> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, > >> + struct uvc_control *ctrl, > >> + struct uvc_control_mapping *mapping) > >> +{ > >> + struct uvc_control_mapping *master_map = NULL; > >> + struct uvc_control *master_ctrl = NULL; > >> + s32 val; > >> + int ret; > >> + > >> + if (!mapping->master_id) > >> + return false; > >> + > >> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > >> + &master_ctrl, 0); > >> + > >> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > >> + return false; > >> + > >> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > >> + if (ret < 0 || val == mapping->master_manual) > >> + return false; > >> + > >> + return true; > >> +} > >> + > >> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >> struct uvc_control *ctrl, > >> struct uvc_control_mapping *mapping, > >> struct v4l2_queryctrl *v4l2_ctrl) > >> { > >> - struct uvc_control_mapping *master_map = NULL; > >> - struct uvc_control *master_ctrl = NULL; > >> const struct uvc_menu_info *menu; > >> unsigned int i; > >> > >> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) > >> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >> > >> - if (mapping->master_id) > >> - __uvc_find_control(ctrl->entity, mapping->master_id, > >> - &master_map, &master_ctrl, 0); > >> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > >> - s32 val; > >> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > >> - if (ret < 0) > >> - return ret; > >> - > >> - if (val != mapping->master_manual) > >> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > >> - } > >> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) > >> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > >> > >> if (!ctrl->cached) { > >> int ret = uvc_ctrl_populate_cache(chain, ctrl); > >> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > >> return 0; > >> } > >> > >> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > >> - struct v4l2_ext_controls *ctrls, > >> - struct uvc_control *uvc_control) > >> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, > >> + struct uvc_entity *entity, > >> + struct v4l2_ext_controls *ctrls, > >> + struct uvc_control *err_control, > >> + int ret) > >> { > >> struct uvc_control_mapping *mapping; > >> struct uvc_control *ctrl_found; > >> unsigned int i; > >> > >> - if (!entity) > >> - return ctrls->count; > >> + if (!entity) { > >> + ctrls->error_idx = ctrls->count; > >> + return ret; > >> + } > >> > >> for (i = 0; i < ctrls->count; i++) { > >> __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > >> &ctrl_found, 0); > >> - if (uvc_control == ctrl_found) > >> - return i; > >> + if (err_control == ctrl_found) > >> + break; > >> } > >> + ctrls->error_idx = i; > >> + > >> + /* We could not find the control that failed. */ > >> + if (i == ctrls->count) > >> + return ret; > >> + > >> + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) > >> + return -EACCES; > >> > >> - return ctrls->count; > >> + return ret; > >> } > >> > >> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > >> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > >> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); > >> done: > >> if (ret < 0 && ctrls) > >> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, > >> - err_ctrl); > >> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, > >> + ret); > >> mutex_unlock(&chain->ctrl_mutex); > >> return ret; > >> } > >> -- > >> 2.32.0.288.g62a8d224e6-goog > >> > > > > > -- Ricardo Ribalda
Hi Ricardo, On 25/06/2021 15:55, Ricardo Ribalda wrote: > Hi Hans > > On Fri, 25 Jun 2021 at 13:07, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> On 25/06/2021 12:29, Ricardo Ribalda wrote: >>> Hi Hans >>> >>> Did you have some hardware that did not work fine without this patch? >>> Am I remembering correctly? >> >> Yes, that's correct. It's one of my webcams, but I can't remember which one >> it is. You probably want me to test this v10? >> >> Regards, > > That would be awesome. Thanks! You can add my: Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> to this series. I do get these warnings (depends on the webcam model, though, some do, some don't): Streaming ioctls: test read/write: OK (Not Supported) test blocking wait: OK warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 test MMAP (no poll): OK warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 test MMAP (select): OK warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 test MMAP (epoll): OK warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 test USERPTR (no poll): OK warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 test USERPTR (select): OK test DMABUF: Cannot test, specify --expbuf-device It's something to do with the Field ID, but I'm not sure if this is really correctly reporting a dropped frame, or if it is a false report and the sequence counter was wrongly incremented. This is a separate issue, though, and doesn't block this series. Regards, Hans > >> >> Hans >> >>> >>> Thanks! >>> >>> On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: >>>> >>>> If a control is inactive return -EACCES to let the userspace know that >>>> the value will not be applied automatically when the control is active >>>> again. >>>> >>>> Also make sure that query_v4l2_ctrl doesn't return an error. >>>> >>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>>> --- >>>> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- >>>> 1 file changed, 49 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >>>> index da44d5c0b9ad..4f80c06d3c43 100644 >>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c >>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >>>> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) >>>> return "Unknown Control"; >>>> } >>>> >>>> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, >>>> + struct uvc_control *ctrl, >>>> + struct uvc_control_mapping *mapping) >>>> +{ >>>> + struct uvc_control_mapping *master_map = NULL; >>>> + struct uvc_control *master_ctrl = NULL; >>>> + s32 val; >>>> + int ret; >>>> + >>>> + if (!mapping->master_id) >>>> + return false; >>>> + >>>> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, >>>> + &master_ctrl, 0); >>>> + >>>> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) >>>> + return false; >>>> + >>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >>>> + if (ret < 0 || val == mapping->master_manual) >>>> + return false; >>>> + >>>> + return true; >>>> +} >>>> + >>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>> struct uvc_control *ctrl, >>>> struct uvc_control_mapping *mapping, >>>> struct v4l2_queryctrl *v4l2_ctrl) >>>> { >>>> - struct uvc_control_mapping *master_map = NULL; >>>> - struct uvc_control *master_ctrl = NULL; >>>> const struct uvc_menu_info *menu; >>>> unsigned int i; >>>> >>>> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) >>>> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> >>>> - if (mapping->master_id) >>>> - __uvc_find_control(ctrl->entity, mapping->master_id, >>>> - &master_map, &master_ctrl, 0); >>>> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { >>>> - s32 val; >>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >>>> - if (ret < 0) >>>> - return ret; >>>> - >>>> - if (val != mapping->master_manual) >>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >>>> - } >>>> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) >>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >>>> >>>> if (!ctrl->cached) { >>>> int ret = uvc_ctrl_populate_cache(chain, ctrl); >>>> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, >>>> return 0; >>>> } >>>> >>>> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, >>>> - struct v4l2_ext_controls *ctrls, >>>> - struct uvc_control *uvc_control) >>>> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, >>>> + struct uvc_entity *entity, >>>> + struct v4l2_ext_controls *ctrls, >>>> + struct uvc_control *err_control, >>>> + int ret) >>>> { >>>> struct uvc_control_mapping *mapping; >>>> struct uvc_control *ctrl_found; >>>> unsigned int i; >>>> >>>> - if (!entity) >>>> - return ctrls->count; >>>> + if (!entity) { >>>> + ctrls->error_idx = ctrls->count; >>>> + return ret; >>>> + } >>>> >>>> for (i = 0; i < ctrls->count; i++) { >>>> __uvc_find_control(entity, ctrls->controls[i].id, &mapping, >>>> &ctrl_found, 0); >>>> - if (uvc_control == ctrl_found) >>>> - return i; >>>> + if (err_control == ctrl_found) >>>> + break; >>>> } >>>> + ctrls->error_idx = i; >>>> + >>>> + /* We could not find the control that failed. */ >>>> + if (i == ctrls->count) >>>> + return ret; >>>> + >>>> + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) >>>> + return -EACCES; >>>> >>>> - return ctrls->count; >>>> + return ret; >>>> } >>>> >>>> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, >>>> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, >>>> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); >>>> done: >>>> if (ret < 0 && ctrls) >>>> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, >>>> - err_ctrl); >>>> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, >>>> + ret); >>>> mutex_unlock(&chain->ctrl_mutex); >>>> return ret; >>>> } >>>> -- >>>> 2.32.0.288.g62a8d224e6-goog >>>> >>> >>> >> > >
Hi Hans On Wed, 30 Jun 2021 at 11:03, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Ricardo, > > On 25/06/2021 15:55, Ricardo Ribalda wrote: > > Hi Hans > > > > On Fri, 25 Jun 2021 at 13:07, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> On 25/06/2021 12:29, Ricardo Ribalda wrote: > >>> Hi Hans > >>> > >>> Did you have some hardware that did not work fine without this patch? > >>> Am I remembering correctly? > >> > >> Yes, that's correct. It's one of my webcams, but I can't remember which one > >> it is. You probably want me to test this v10? > >> > >> Regards, > > > > That would be awesome. Thanks! > > You can add my: > > Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks a lot for testing it! Sorry to bother you, but could I ask you to try again, but with this patch reverted? This is v10 1-20, without 21/21 Thanks! > > to this series. > > I do get these warnings (depends on the webcam model, though, some do, some don't): > > Streaming ioctls: > test read/write: OK (Not Supported) > test blocking wait: OK > warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > test MMAP (no poll): OK > warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > test MMAP (select): OK > warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > test MMAP (epoll): OK > warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > test USERPTR (no poll): OK > warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > test USERPTR (select): OK > test DMABUF: Cannot test, specify --expbuf-device > > It's something to do with the Field ID, but I'm not sure if this is really correctly > reporting a dropped frame, or if it is a false report and the sequence counter was > wrongly incremented. > > This is a separate issue, though, and doesn't block this series. > > Regards, > > Hans > > > > >> > >> Hans > >> > >>> > >>> Thanks! > >>> > >>> On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: > >>>> > >>>> If a control is inactive return -EACCES to let the userspace know that > >>>> the value will not be applied automatically when the control is active > >>>> again. > >>>> > >>>> Also make sure that query_v4l2_ctrl doesn't return an error. > >>>> > >>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>>> --- > >>>> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- > >>>> 1 file changed, 49 insertions(+), 24 deletions(-) > >>>> > >>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > >>>> index da44d5c0b9ad..4f80c06d3c43 100644 > >>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c > >>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c > >>>> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) > >>>> return "Unknown Control"; > >>>> } > >>>> > >>>> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, > >>>> + struct uvc_control *ctrl, > >>>> + struct uvc_control_mapping *mapping) > >>>> +{ > >>>> + struct uvc_control_mapping *master_map = NULL; > >>>> + struct uvc_control *master_ctrl = NULL; > >>>> + s32 val; > >>>> + int ret; > >>>> + > >>>> + if (!mapping->master_id) > >>>> + return false; > >>>> + > >>>> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > >>>> + &master_ctrl, 0); > >>>> + > >>>> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > >>>> + return false; > >>>> + > >>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > >>>> + if (ret < 0 || val == mapping->master_manual) > >>>> + return false; > >>>> + > >>>> + return true; > >>>> +} > >>>> + > >>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>> struct uvc_control *ctrl, > >>>> struct uvc_control_mapping *mapping, > >>>> struct v4l2_queryctrl *v4l2_ctrl) > >>>> { > >>>> - struct uvc_control_mapping *master_map = NULL; > >>>> - struct uvc_control *master_ctrl = NULL; > >>>> const struct uvc_menu_info *menu; > >>>> unsigned int i; > >>>> > >>>> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) > >>>> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>>> > >>>> - if (mapping->master_id) > >>>> - __uvc_find_control(ctrl->entity, mapping->master_id, > >>>> - &master_map, &master_ctrl, 0); > >>>> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > >>>> - s32 val; > >>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > >>>> - if (ret < 0) > >>>> - return ret; > >>>> - > >>>> - if (val != mapping->master_manual) > >>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > >>>> - } > >>>> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) > >>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > >>>> > >>>> if (!ctrl->cached) { > >>>> int ret = uvc_ctrl_populate_cache(chain, ctrl); > >>>> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > >>>> return 0; > >>>> } > >>>> > >>>> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > >>>> - struct v4l2_ext_controls *ctrls, > >>>> - struct uvc_control *uvc_control) > >>>> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, > >>>> + struct uvc_entity *entity, > >>>> + struct v4l2_ext_controls *ctrls, > >>>> + struct uvc_control *err_control, > >>>> + int ret) > >>>> { > >>>> struct uvc_control_mapping *mapping; > >>>> struct uvc_control *ctrl_found; > >>>> unsigned int i; > >>>> > >>>> - if (!entity) > >>>> - return ctrls->count; > >>>> + if (!entity) { > >>>> + ctrls->error_idx = ctrls->count; > >>>> + return ret; > >>>> + } > >>>> > >>>> for (i = 0; i < ctrls->count; i++) { > >>>> __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > >>>> &ctrl_found, 0); > >>>> - if (uvc_control == ctrl_found) > >>>> - return i; > >>>> + if (err_control == ctrl_found) > >>>> + break; > >>>> } > >>>> + ctrls->error_idx = i; > >>>> + > >>>> + /* We could not find the control that failed. */ > >>>> + if (i == ctrls->count) > >>>> + return ret; > >>>> + > >>>> + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) > >>>> + return -EACCES; > >>>> > >>>> - return ctrls->count; > >>>> + return ret; > >>>> } > >>>> > >>>> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > >>>> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > >>>> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); > >>>> done: > >>>> if (ret < 0 && ctrls) > >>>> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, > >>>> - err_ctrl); > >>>> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, > >>>> + ret); > >>>> mutex_unlock(&chain->ctrl_mutex); > >>>> return ret; > >>>> } > >>>> -- > >>>> 2.32.0.288.g62a8d224e6-goog > >>>> > >>> > >>> > >> > > > > > -- Ricardo Ribalda
On 30/06/2021 14:51, Ricardo Ribalda wrote: > Hi Hans > > > On Wed, 30 Jun 2021 at 11:03, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> Hi Ricardo, >> >> On 25/06/2021 15:55, Ricardo Ribalda wrote: >>> Hi Hans >>> >>> On Fri, 25 Jun 2021 at 13:07, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >>>> >>>> On 25/06/2021 12:29, Ricardo Ribalda wrote: >>>>> Hi Hans >>>>> >>>>> Did you have some hardware that did not work fine without this patch? >>>>> Am I remembering correctly? >>>> >>>> Yes, that's correct. It's one of my webcams, but I can't remember which one >>>> it is. You probably want me to test this v10? >>>> >>>> Regards, >>> >>> That would be awesome. Thanks! >> >> You can add my: >> >> Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Thanks a lot for testing it! > > Sorry to bother you, but could I ask you to try again, but with this > patch reverted? This is v10 1-20, without 21/21 I tried with and without this patch and I didn't see any difference. Unfortunately, I don't remember the exact details of what I tested last time and with which hardware. I would drop this patch, it can always be added later. Regards, Hans > > > Thanks! >> >> to this series. >> >> I do get these warnings (depends on the webcam model, though, some do, some don't): >> >> Streaming ioctls: >> test read/write: OK (Not Supported) >> test blocking wait: OK >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 >> test MMAP (no poll): OK >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 >> test MMAP (select): OK >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 >> test MMAP (epoll): OK >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 >> test USERPTR (no poll): OK >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 >> test USERPTR (select): OK >> test DMABUF: Cannot test, specify --expbuf-device >> >> It's something to do with the Field ID, but I'm not sure if this is really correctly >> reporting a dropped frame, or if it is a false report and the sequence counter was >> wrongly incremented. >> >> This is a separate issue, though, and doesn't block this series. >> >> Regards, >> >> Hans >> >>> >>>> >>>> Hans >>>> >>>>> >>>>> Thanks! >>>>> >>>>> On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: >>>>>> >>>>>> If a control is inactive return -EACCES to let the userspace know that >>>>>> the value will not be applied automatically when the control is active >>>>>> again. >>>>>> >>>>>> Also make sure that query_v4l2_ctrl doesn't return an error. >>>>>> >>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>>>>> --- >>>>>> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- >>>>>> 1 file changed, 49 insertions(+), 24 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >>>>>> index da44d5c0b9ad..4f80c06d3c43 100644 >>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c >>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >>>>>> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) >>>>>> return "Unknown Control"; >>>>>> } >>>>>> >>>>>> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, >>>>>> + struct uvc_control *ctrl, >>>>>> + struct uvc_control_mapping *mapping) >>>>>> +{ >>>>>> + struct uvc_control_mapping *master_map = NULL; >>>>>> + struct uvc_control *master_ctrl = NULL; >>>>>> + s32 val; >>>>>> + int ret; >>>>>> + >>>>>> + if (!mapping->master_id) >>>>>> + return false; >>>>>> + >>>>>> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, >>>>>> + &master_ctrl, 0); >>>>>> + >>>>>> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) >>>>>> + return false; >>>>>> + >>>>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >>>>>> + if (ret < 0 || val == mapping->master_manual) >>>>>> + return false; >>>>>> + >>>>>> + return true; >>>>>> +} >>>>>> + >>>>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>>>> struct uvc_control *ctrl, >>>>>> struct uvc_control_mapping *mapping, >>>>>> struct v4l2_queryctrl *v4l2_ctrl) >>>>>> { >>>>>> - struct uvc_control_mapping *master_map = NULL; >>>>>> - struct uvc_control *master_ctrl = NULL; >>>>>> const struct uvc_menu_info *menu; >>>>>> unsigned int i; >>>>>> >>>>>> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>>>> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) >>>>>> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>>> >>>>>> - if (mapping->master_id) >>>>>> - __uvc_find_control(ctrl->entity, mapping->master_id, >>>>>> - &master_map, &master_ctrl, 0); >>>>>> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { >>>>>> - s32 val; >>>>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >>>>>> - if (ret < 0) >>>>>> - return ret; >>>>>> - >>>>>> - if (val != mapping->master_manual) >>>>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >>>>>> - } >>>>>> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) >>>>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >>>>>> >>>>>> if (!ctrl->cached) { >>>>>> int ret = uvc_ctrl_populate_cache(chain, ctrl); >>>>>> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, >>>>>> - struct v4l2_ext_controls *ctrls, >>>>>> - struct uvc_control *uvc_control) >>>>>> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, >>>>>> + struct uvc_entity *entity, >>>>>> + struct v4l2_ext_controls *ctrls, >>>>>> + struct uvc_control *err_control, >>>>>> + int ret) >>>>>> { >>>>>> struct uvc_control_mapping *mapping; >>>>>> struct uvc_control *ctrl_found; >>>>>> unsigned int i; >>>>>> >>>>>> - if (!entity) >>>>>> - return ctrls->count; >>>>>> + if (!entity) { >>>>>> + ctrls->error_idx = ctrls->count; >>>>>> + return ret; >>>>>> + } >>>>>> >>>>>> for (i = 0; i < ctrls->count; i++) { >>>>>> __uvc_find_control(entity, ctrls->controls[i].id, &mapping, >>>>>> &ctrl_found, 0); >>>>>> - if (uvc_control == ctrl_found) >>>>>> - return i; >>>>>> + if (err_control == ctrl_found) >>>>>> + break; >>>>>> } >>>>>> + ctrls->error_idx = i; >>>>>> + >>>>>> + /* We could not find the control that failed. */ >>>>>> + if (i == ctrls->count) >>>>>> + return ret; >>>>>> + >>>>>> + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) >>>>>> + return -EACCES; >>>>>> >>>>>> - return ctrls->count; >>>>>> + return ret; >>>>>> } >>>>>> >>>>>> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, >>>>>> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, >>>>>> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); >>>>>> done: >>>>>> if (ret < 0 && ctrls) >>>>>> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, >>>>>> - err_ctrl); >>>>>> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, >>>>>> + ret); >>>>>> mutex_unlock(&chain->ctrl_mutex); >>>>>> return ret; >>>>>> } >>>>>> -- >>>>>> 2.32.0.288.g62a8d224e6-goog >>>>>> >>>>> >>>>> >>>> >>> >>> >> > >
Hi Hans On Tue, 6 Jul 2021 at 16:19, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 30/06/2021 14:51, Ricardo Ribalda wrote: > > Hi Hans > > > > > > On Wed, 30 Jun 2021 at 11:03, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> Hi Ricardo, > >> > >> On 25/06/2021 15:55, Ricardo Ribalda wrote: > >>> Hi Hans > >>> > >>> On Fri, 25 Jun 2021 at 13:07, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >>>> > >>>> On 25/06/2021 12:29, Ricardo Ribalda wrote: > >>>>> Hi Hans > >>>>> > >>>>> Did you have some hardware that did not work fine without this patch? > >>>>> Am I remembering correctly? > >>>> > >>>> Yes, that's correct. It's one of my webcams, but I can't remember which one > >>>> it is. You probably want me to test this v10? > >>>> > >>>> Regards, > >>> > >>> That would be awesome. Thanks! > >> > >> You can add my: > >> > >> Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > > Thanks a lot for testing it! > > > > Sorry to bother you, but could I ask you to try again, but with this > > patch reverted? This is v10 1-20, without 21/21 > > I tried with and without this patch and I didn't see any difference. Unfortunately, > I don't remember the exact details of what I tested last time and with which hardware. > > I would drop this patch, it can always be added later. Thank you very much for testing. I think there were no more comments in this set. Laurent: Shall I resend the series without this last patch, or can you take it as is ignoring the last one? Thanks! > > Regards, > > Hans > > > > > > > Thanks! > >> > >> to this series. > >> > >> I do get these warnings (depends on the webcam model, though, some do, some don't): > >> > >> Streaming ioctls: > >> test read/write: OK (Not Supported) > >> test blocking wait: OK > >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > >> test MMAP (no poll): OK > >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > >> test MMAP (select): OK > >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > >> test MMAP (epoll): OK > >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > >> test USERPTR (no poll): OK > >> warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0 > >> test USERPTR (select): OK > >> test DMABUF: Cannot test, specify --expbuf-device > >> > >> It's something to do with the Field ID, but I'm not sure if this is really correctly > >> reporting a dropped frame, or if it is a false report and the sequence counter was > >> wrongly incremented. > >> > >> This is a separate issue, though, and doesn't block this series. > >> > >> Regards, > >> > >> Hans > >> > >>> > >>>> > >>>> Hans > >>>> > >>>>> > >>>>> Thanks! > >>>>> > >>>>> On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@chromium.org> wrote: > >>>>>> > >>>>>> If a control is inactive return -EACCES to let the userspace know that > >>>>>> the value will not be applied automatically when the control is active > >>>>>> again. > >>>>>> > >>>>>> Also make sure that query_v4l2_ctrl doesn't return an error. > >>>>>> > >>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>>>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>>>>> --- > >>>>>> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++----------- > >>>>>> 1 file changed, 49 insertions(+), 24 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > >>>>>> index da44d5c0b9ad..4f80c06d3c43 100644 > >>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c > >>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c > >>>>>> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) > >>>>>> return "Unknown Control"; > >>>>>> } > >>>>>> > >>>>>> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, > >>>>>> + struct uvc_control *ctrl, > >>>>>> + struct uvc_control_mapping *mapping) > >>>>>> +{ > >>>>>> + struct uvc_control_mapping *master_map = NULL; > >>>>>> + struct uvc_control *master_ctrl = NULL; > >>>>>> + s32 val; > >>>>>> + int ret; > >>>>>> + > >>>>>> + if (!mapping->master_id) > >>>>>> + return false; > >>>>>> + > >>>>>> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > >>>>>> + &master_ctrl, 0); > >>>>>> + > >>>>>> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > >>>>>> + return false; > >>>>>> + > >>>>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > >>>>>> + if (ret < 0 || val == mapping->master_manual) > >>>>>> + return false; > >>>>>> + > >>>>>> + return true; > >>>>>> +} > >>>>>> + > >>>>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>>>> struct uvc_control *ctrl, > >>>>>> struct uvc_control_mapping *mapping, > >>>>>> struct v4l2_queryctrl *v4l2_ctrl) > >>>>>> { > >>>>>> - struct uvc_control_mapping *master_map = NULL; > >>>>>> - struct uvc_control *master_ctrl = NULL; > >>>>>> const struct uvc_menu_info *menu; > >>>>>> unsigned int i; > >>>>>> > >>>>>> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>>>> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) > >>>>>> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>>>>> > >>>>>> - if (mapping->master_id) > >>>>>> - __uvc_find_control(ctrl->entity, mapping->master_id, > >>>>>> - &master_map, &master_ctrl, 0); > >>>>>> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > >>>>>> - s32 val; > >>>>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > >>>>>> - if (ret < 0) > >>>>>> - return ret; > >>>>>> - > >>>>>> - if (val != mapping->master_manual) > >>>>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > >>>>>> - } > >>>>>> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) > >>>>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > >>>>>> > >>>>>> if (!ctrl->cached) { > >>>>>> int ret = uvc_ctrl_populate_cache(chain, ctrl); > >>>>>> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > >>>>>> - struct v4l2_ext_controls *ctrls, > >>>>>> - struct uvc_control *uvc_control) > >>>>>> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, > >>>>>> + struct uvc_entity *entity, > >>>>>> + struct v4l2_ext_controls *ctrls, > >>>>>> + struct uvc_control *err_control, > >>>>>> + int ret) > >>>>>> { > >>>>>> struct uvc_control_mapping *mapping; > >>>>>> struct uvc_control *ctrl_found; > >>>>>> unsigned int i; > >>>>>> > >>>>>> - if (!entity) > >>>>>> - return ctrls->count; > >>>>>> + if (!entity) { > >>>>>> + ctrls->error_idx = ctrls->count; > >>>>>> + return ret; > >>>>>> + } > >>>>>> > >>>>>> for (i = 0; i < ctrls->count; i++) { > >>>>>> __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > >>>>>> &ctrl_found, 0); > >>>>>> - if (uvc_control == ctrl_found) > >>>>>> - return i; > >>>>>> + if (err_control == ctrl_found) > >>>>>> + break; > >>>>>> } > >>>>>> + ctrls->error_idx = i; > >>>>>> + > >>>>>> + /* We could not find the control that failed. */ > >>>>>> + if (i == ctrls->count) > >>>>>> + return ret; > >>>>>> + > >>>>>> + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) > >>>>>> + return -EACCES; > >>>>>> > >>>>>> - return ctrls->count; > >>>>>> + return ret; > >>>>>> } > >>>>>> > >>>>>> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > >>>>>> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > >>>>>> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); > >>>>>> done: > >>>>>> if (ret < 0 && ctrls) > >>>>>> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, > >>>>>> - err_ctrl); > >>>>>> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, > >>>>>> + ret); > >>>>>> mutex_unlock(&chain->ctrl_mutex); > >>>>>> return ret; > >>>>>> } > >>>>>> -- > >>>>>> 2.32.0.288.g62a8d224e6-goog > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > >> > > > > > -- Ricardo Ribalda
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index da44d5c0b9ad..4f80c06d3c43 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) return "Unknown Control"; } +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, + struct uvc_control *ctrl, + struct uvc_control_mapping *mapping) +{ + struct uvc_control_mapping *master_map = NULL; + struct uvc_control *master_ctrl = NULL; + s32 val; + int ret; + + if (!mapping->master_id) + return false; + + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, + &master_ctrl, 0); + + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) + return false; + + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); + if (ret < 0 || val == mapping->master_manual) + return false; + + return true; +} + static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, struct uvc_control *ctrl, struct uvc_control_mapping *mapping, struct v4l2_queryctrl *v4l2_ctrl) { - struct uvc_control_mapping *master_map = NULL; - struct uvc_control *master_ctrl = NULL; const struct uvc_menu_info *menu; unsigned int i; @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; - if (mapping->master_id) - __uvc_find_control(ctrl->entity, mapping->master_id, - &master_map, &master_ctrl, 0); - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { - s32 val; - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); - if (ret < 0) - return ret; - - if (val != mapping->master_manual) - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; - } + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; if (!ctrl->cached) { int ret = uvc_ctrl_populate_cache(chain, ctrl); @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, return 0; } -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, - struct v4l2_ext_controls *ctrls, - struct uvc_control *uvc_control) +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, + struct uvc_entity *entity, + struct v4l2_ext_controls *ctrls, + struct uvc_control *err_control, + int ret) { struct uvc_control_mapping *mapping; struct uvc_control *ctrl_found; unsigned int i; - if (!entity) - return ctrls->count; + if (!entity) { + ctrls->error_idx = ctrls->count; + return ret; + } for (i = 0; i < ctrls->count; i++) { __uvc_find_control(entity, ctrls->controls[i].id, &mapping, &ctrl_found, 0); - if (uvc_control == ctrl_found) - return i; + if (err_control == ctrl_found) + break; } + ctrls->error_idx = i; + + /* We could not find the control that failed. */ + if (i == ctrls->count) + return ret; + + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) + return -EACCES; - return ctrls->count; + return ret; } int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); done: if (ret < 0 && ctrls) - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, - err_ctrl); + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, + ret); mutex_unlock(&chain->ctrl_mutex); return ret; }