On 18/03/2021 08:39, Hans Verkuil wrote:
> Hi Ricardo,
>
> On 17/03/2021 17:45, Ricardo Ribalda 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.
Note that this needs to be documented in vidioc-g-ext-ctrls.rst and
vidioc-g-ctrl.rst as well. Currently setting inactive controls shouldn't
return EACCES, but this has now changed.
Regards,
Hans
>>
>> Signed-off-by: Ricardo Ribalda <riba...@chromium.org>
>> Suggested-by: Hans Verkuil <hverk...@xs4all.nl>
>
> In several of the patches in this series you added 'Suggested-by' or
> 'Credit-to'.
> Please use <hverkuil-ci...@xs4all.nl> so Cisco gets the credit as well :-)
>
>> ---
>> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++++++-------
>> drivers/media/usb/uvc/uvc_v4l2.c | 11 ++++-
>> drivers/media/usb/uvc/uvcvideo.h | 3 +-
>> 3 files changed, 69 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
>> b/drivers/media/usb/uvc/uvc_ctrl.c
>> index af1d4d9b8afb..26d3494b401b 100644
>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>> @@ -1046,10 +1046,62 @@ static int uvc_query_v4l2_class(struct
>> uvc_video_chain *chain, u32 req_id,
>> return 0;
>> }
>>
>> -int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id, bool
>> read)
>> +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;
>> +}
>> +
>> +int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id,
>
> Same typo: accesible -> accessible
>
>> + unsigned long ioctl)
>> {
>> struct uvc_control_mapping *mapping;
>> struct uvc_control *ctrl;
>> + bool read, try;
>> +
>> + switch (ioctl) {
>> + case VIDIOC_G_EXT_CTRLS:
>> + read = true;
>> + try = false;
>> + break;
>> + case VIDIOC_S_EXT_CTRLS:
>> + read = false;
>> + try = false;
>> + break;
>> + case VIDIOC_TRY_EXT_CTRLS:
>> + read = false;
>> + try = true;
>> + break;
>> + case VIDIOC_G_CTRL:
>> + read = true;
>> + try = false;
>> + break;
>> + case VIDIOC_S_CTRL:
>> + read = false;
>> + try = false;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>
> That's ugly. Just remove the bools and switch and just check the ioctl
> below...
>
>>
>> if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
>> return -EACCES;
>> @@ -1064,6 +1116,9 @@ int uvc_ctrl_is_accesible(struct uvc_video_chain
>> *chain, u32 v4l2_id, bool read)
>> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
>> return -EACCES;
>>
>> + if (!read && !try && uvc_ctrl_is_inactive(chain, ctrl, mapping))
>
> and this becomes:
>
> if ((ioctl == VIDIOC_S_EXT_CTRLS || ioctl == VIDIOC_S_CTRL) &&
> uvc_ctrl_is_inactive(chain, ctrl, mapping))
>
> Much, much simpler!
>
>> + return -EACCES;
>> +
>> return 0;
>> }
>>
>> @@ -1086,8 +1141,6 @@ static int __uvc_query_v4l2_ctrl(struct
>> uvc_video_chain *chain,
>> 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;
>>
>> @@ -1103,18 +1156,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);
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c
>> index ce55b4bff687..8e9051a245c7 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -999,6 +999,10 @@ static int uvc_ioctl_g_ctrl(struct file *file, void *fh,
>> struct v4l2_ext_control xctrl;
>> int ret;
>>
>> + ret = uvc_ctrl_is_accesible(chain, ctrl->id, VIDIOC_G_CTRL);
>> + if (ret)
>> + return ret;
>> +
>> memset(&xctrl, 0, sizeof(xctrl));
>> xctrl.id = ctrl->id;
>>
>> @@ -1023,6 +1027,10 @@ static int uvc_ioctl_s_ctrl(struct file *file, void
>> *fh,
>> struct v4l2_ext_control xctrl;
>> int ret;
>>
>> + ret = uvc_ctrl_is_accesible(chain, ctrl->id, VIDIOC_S_CTRL);
>> + if (ret)
>> + return ret;
>> +
>> memset(&xctrl, 0, sizeof(xctrl));
>> xctrl.id = ctrl->id;
>> xctrl.value = ctrl->value;
>> @@ -1054,8 +1062,7 @@ static int uvc_ctrl_check_access(struct
>> uvc_video_chain *chain,
>> int ret = 0;
>>
>> for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>> - ret = uvc_ctrl_is_accesible(chain, ctrl->id,
>> - ioctl == VIDIOC_G_EXT_CTRLS);
>> + ret = uvc_ctrl_is_accesible(chain, ctrl->id, ioctl);
>> if (ret)
>> break;
>> }
>> diff --git a/drivers/media/usb/uvc/uvcvideo.h
>> b/drivers/media/usb/uvc/uvcvideo.h
>> index 3288b118264e..4e86d0983d58 100644
>> --- a/drivers/media/usb/uvc/uvcvideo.h
>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>> @@ -902,7 +902,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_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
>> -int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id, bool
>> read);
>> +int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id,
>> + unsigned long ioctl);
>>
>> int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>> struct uvc_xu_control_query *xqry);
>>
>
> Regards,
>
> Hans
>
