On 19/03/2021 10:49, Ricardo Ribalda wrote:
> Hi Hans
>
> Thanks for testing this.
>
>
>
>
> On Fri, Mar 19, 2021 at 10:10 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
>> On 18/03/2021 21:29, 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.
>>>
>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>>> ---
>>> drivers/media/usb/uvc/uvc_ctrl.c | 68 ++++++++++++++++++++++----------
>>> drivers/media/usb/uvc/uvc_v4l2.c | 11 +++++-
>>> drivers/media/usb/uvc/uvcvideo.h | 2 +-
>>> 3 files changed, 58 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 24fd5afc4e4f..1ec8333811bc 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1046,8 +1046,33 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>>> return 0;
>>> }
>>>
>>> +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;
>>> +}
>>
>> This doesn't work. The problem is that you need to test the new value for the
>> master control against master_manual, and you are testing against the old value.
>>
>> So for my uvc camera I have this situation after boot:
>>
>> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
>> 1: Manual Mode
>> 3: Aperture Priority Mode
>> exposure_time_absolute 0x009a0902 (int) : min=3 max=2047 step=1 default=250 value=250 flags=inactive
>>
>> But trying to change both auto_exposure to manual AND set the new exposure_time_absolute
>> will fail:
>>
>> $ v4l2-ctl -c auto_exposure=1,exposure_time_absolute=251
>> VIDIOC_S_EXT_CTRLS: failed: Permission denied
>> Error setting controls: Permission denied
>>
>> This works though with the uvc driver as is currently in the kernel.
>>
>> Unfortunately, this is not something that is explicitly tested in v4l2-compliance.
>>
>
> Can you try dropping this patch and relaying on media: uvcvideo: Set
> error_idx during ctrl_commit errors ?
That doesn't work all that well. The uvc_query_ctrl() function is problematic:
1) It can return -EILSEQ where -EACCES is expected. Not a big deal, EACCES makes
more sense here anyway.
2) It can return error code 6 (Invalid control), and then it returns -EIO. I'm not
entirely clear why I get code 6, I haven't dug deep enough for that. If I
change that to EACCES, then v4l2-compliance passes, but...
3) This function keeps spamming the "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n"
error in the kernel log. In all fairness, the current kernel driver does the same, but
with this patch it no longer does.
I think the uvc driver really has to check this. It doesn't have to be during the
validation step in uvc_ctrl_check_access(), it is fine if this check happens during
the commit phase.
I checked the UVC 1.5 spec and in 4.2.2.1.4 (Exposure Time (Absolute) Control) it says:
When the Auto-Exposure Mode control is in Auto mode or Aperture Priority mode attempts
to programmatically set this control shall result in a protocol STALL and an error code
of bRequestErrorCode = “Wrong state”.
So the uvc driver should just detect this and avoid writing this control in such a case.
Regards,
Hans
>
> thanks!
>
>> Regards,
>>
>> Hans
>>
>>> +
>>> int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>>> - bool read)
>>> + unsigned long ioctl)
>>> {
>>> struct uvc_control_mapping *mapping;
>>> struct uvc_control *ctrl;
>>> @@ -1059,11 +1084,26 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>>> if (!ctrl)
>>> return -EINVAL;
>>>
>>> - if (!(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) && read)
>>> - return -EACCES;
>>> -
>>> - if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
>>> - return -EACCES;
>>> + switch (ioctl) {
>>> + case VIDIOC_G_CTRL:
>>> + case VIDIOC_G_EXT_CTRLS:
>>> + if (!(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
>>> + return -EACCES;
>>> + break;
>>> + case VIDIOC_S_EXT_CTRLS:
>>> + case VIDIOC_S_CTRL:
>>> + if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
>>> + return -EACCES;
>>> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
>>> + return -EACCES;
>>> + break;
>>> + case VIDIOC_TRY_EXT_CTRLS:
>>> + if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
>>> + return -EACCES;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>>
>>> return 0;
>>> }
>>> @@ -1087,8 +1127,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;
>>>
>>> @@ -1104,18 +1142,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 fbb99f3c2fb4..ddebdeb5a81b 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_accessible(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_accessible(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_accessible(chain, ctrl->id,
>>> - ioctl == VIDIOC_G_EXT_CTRLS);
>>> + ret = uvc_ctrl_is_accessible(chain, ctrl->id, ioctl);
>>> if (ret)
>>> break;
>>> }
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 9471c342a310..a93aeedb5499 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -903,7 +903,7 @@ 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_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>>> - bool read);
>>> + unsigned long ioctl);
>>>
>>> int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>>> struct uvc_xu_control_query *xqry);
>>>
>>
>
>
