mbox series

[v7,00/17] uvcvideo: Fix v4l2-compliance errors

Message ID 20210318202928.166955-1-ribalda@chromium.org
Headers show
Series uvcvideo: Fix v4l2-compliance errors | expand

Message

Ricardo Ribalda March 18, 2021, 8:29 p.m. UTC 
v4l2-compliance -m /dev/media0 -a -f
Total for uvcvideo device /dev/media0: 8, Succeeded: 6, Failed: 2, Warnings: 0
Total for uvcvideo device /dev/video0: 54, Succeeded: 50, Failed: 4, Warnings: 2
Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0
Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 102,
Failed: 6, Warnings: 2

After fixing all of them we go down to:

Total for uvcvideo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0
Total for uvcvideo device /dev/video0: 54, Succeeded: 54, Failed: 0, Warnings: 0
Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0
Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 108,
Failed: 0, Warnings: 0

YES, NO MORE WARNINGS :)

Note that we depend on:
https://patchwork.linuxtv.org/project/linux-media/patch/20210315172531.101694-1-ribalda@chromium.org/

With Hans patch we can also pass v4l2-compliance -s.

Changelog  from v6 (Thanks to Hans)
- Squash all check_ext_ctrls patches
- Add documentation patch
- Return the correct ctrl_idx if the hw fails
- Fix accessible typo
- Set the proper name also for the metadata node

Hans Verkuil (1):
  uvc: use vb2 ioctl and fop helpers

Ricardo Ribalda (16):
  media: v4l2-ioctl: Fix check_ext_ctrls
  media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL
  media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL
  media: uvcvideo: Set capability in s_param
  media: uvcvideo: Return -EIO for control errors
  media: uvcvideo: refactor __uvc_ctrl_add_mapping
  media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS
  media: uvcvideo: Use dev->name for querycap()
  media: uvcvideo: Set unique vdev name based in type
  media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE
  media: uvcvideo: Use control names from framework
  media: uvcvideo: Check controls flags before accessing them
  media: uvcvideo: Return -EACCES to inactive controls
  media: docs: Document the behaviour of uvcdriver
  media: uvcvideo: Refactor __uvc_ctrl_commit
  media: uvcvideo: Set error_idx during ctrl_commit errors

 .../userspace-api/media/v4l/vidioc-g-ctrl.rst |   5 +
 .../media/v4l/vidioc-g-ext-ctrls.rst          |   5 +
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c      |   4 -
 drivers/media/usb/uvc/uvc_ctrl.c              | 312 +++++++++++----
 drivers/media/usb/uvc/uvc_driver.c            |  22 +-
 drivers/media/usb/uvc/uvc_metadata.c          |  10 +-
 drivers/media/usb/uvc/uvc_queue.c             | 143 -------
 drivers/media/usb/uvc/uvc_v4l2.c              | 360 ++++--------------
 drivers/media/usb/uvc/uvc_video.c             |  13 +-
 drivers/media/usb/uvc/uvcvideo.h              |  53 +--
 drivers/media/v4l2-core/v4l2-ioctl.c          |  59 ++-
 11 files changed, 408 insertions(+), 578 deletions(-)

Comments

Hans Verkuil March 19, 2021, 9:02 a.m. UTC | #1 
On 18/03/2021 21:29, Ricardo Ribalda wrote:
> Drivers that do not use the ctrl-framework use this function instead.

> 

> Fix the following issues:

> 

> - Do not check for multiple classes when getting the DEF_VAL.

> - Return -EINVAL for request_api calls

> - Default value cannot be changed, return EINVAL as soon as possible.

> - Return the right error_idx

> [If an error is found when validating the list of controls passed with

> VIDIOC_G_EXT_CTRLS, then error_idx shall be set to ctrls->count to

> indicate to userspace that no actual hardware was touched.

> It would have been much nicer of course if error_idx could point to the

> control index that failed the validation, but sadly that's not how the

> API was designed.]

> 

> Fixes v4l2-compliance:

> Control ioctls (Input 0):

>         warn: v4l2-test-controls.cpp(834): error_idx should be equal to count

>         warn: v4l2-test-controls.cpp(855): error_idx should be equal to count

> 		fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)

> 	test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

> Buffer ioctls (Input 0):

> 		fail: v4l2-test-buffers.cpp(1994): ret != EINVAL && ret != EBADR && ret != ENOTTY

> 	test Requests: FAIL

> 

> Cc: stable@vger.kernel.org

> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")

> 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/v4l2-core/v4l2-ioctl.c | 59 ++++++++++++++++++----------

>  1 file changed, 38 insertions(+), 21 deletions(-)

> 

> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c

> index 31d1342e61e8..ccd21b4d9c72 100644

> --- a/drivers/media/v4l2-core/v4l2-ioctl.c

> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c

> @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only)

>  	pr_cont("driver-specific ioctl\n");

>  }

>  

> -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)

> +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl)

>  {

>  	__u32 i;

>  

> @@ -917,23 +917,40 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)

>  	for (i = 0; i < c->count; i++)

>  		c->controls[i].reserved2[0] = 0;

>  

> -	/* V4L2_CID_PRIVATE_BASE cannot be used as control class

> -	   when using extended controls.

> -	   Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL

> -	   is it allowed for backwards compatibility.

> -	 */

> -	if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE)

> -		return 0;

> -	if (!c->which)

> -		return 1;

> +	switch (c->which) {

> +	case V4L2_CID_PRIVATE_BASE:

> +		/*

> +		 * V4L2_CID_PRIVATE_BASE cannot be used as control class

> +		 * when using extended controls.

> +		 * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL

> +		 * is it allowed for backwards compatibility.

> +		 */

> +		if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP)


S_CROP -> S_CTRL :-)

Regards,

	Hans

> +			return false;

> +		break;

> +	case V4L2_CTRL_WHICH_DEF_VAL:

> +		/* Default value cannot be changed */

> +		if (ioctl == VIDIOC_S_EXT_CTRLS ||

> +		    ioctl == VIDIOC_TRY_EXT_CTRLS) {

> +			c->error_idx = c->count;

> +			return false;

> +		}

> +		return true;

> +	case V4L2_CTRL_WHICH_CUR_VAL:

> +		return true;

> +	case V4L2_CTRL_WHICH_REQUEST_VAL:

> +		c->error_idx = c->count;

> +		return false;

> +	}

> +

>  	/* Check that all controls are from the same control class. */

>  	for (i = 0; i < c->count; i++) {

>  		if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {

> -			c->error_idx = i;

> -			return 0;

> +			c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i : c->count;

> +			return false;

>  		}

>  	}

> -	return 1;

> +	return true;

>  }

>  

>  static int check_fmt(struct file *file, enum v4l2_buf_type type)

> @@ -2229,7 +2246,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,

>  	ctrls.controls = &ctrl;

>  	ctrl.id = p->id;

>  	ctrl.value = p->value;

> -	if (check_ext_ctrls(&ctrls, 1)) {

> +	if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) {

>  		int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls);

>  

>  		if (ret == 0)

> @@ -2263,7 +2280,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,

>  	ctrls.controls = &ctrl;

>  	ctrl.id = p->id;

>  	ctrl.value = p->value;

> -	if (check_ext_ctrls(&ctrls, 1))

> +	if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))

>  		return ops->vidioc_s_ext_ctrls(file, fh, &ctrls);

>  	return -EINVAL;

>  }

> @@ -2285,8 +2302,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,

>  					vfd, vfd->v4l2_dev->mdev, p);

>  	if (ops->vidioc_g_ext_ctrls == NULL)

>  		return -ENOTTY;

> -	return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :

> -					-EINVAL;

> +	return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ?

> +				ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL;

>  }

>  

>  static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,

> @@ -2306,8 +2323,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,

>  					vfd, vfd->v4l2_dev->mdev, p);

>  	if (ops->vidioc_s_ext_ctrls == NULL)

>  		return -ENOTTY;

> -	return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :

> -					-EINVAL;

> +	return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ?

> +				ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL;

>  }

>  

>  static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,

> @@ -2327,8 +2344,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,

>  					  vfd, vfd->v4l2_dev->mdev, p);

>  	if (ops->vidioc_try_ext_ctrls == NULL)

>  		return -ENOTTY;

> -	return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :

> -					-EINVAL;

> +	return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ?

> +			ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL;

>  }

>  

>  /*

>
Hans Verkuil March 19, 2021, 9:10 a.m. UTC | #2 
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@xs4all.nl>

> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

> ---

>  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.

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);

>
Ricardo Ribalda March 19, 2021, 9:49 a.m. UTC | #3 
Hi Hans

Thanks for testing this.




On Fri, Mar 19, 2021 at 10:10 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@xs4all.nl>

> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

> > ---

> >  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 ?

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);

> >

>



-- 
Ricardo Ribalda
Hans Verkuil March 19, 2021, 1:40 p.m. UTC | #4 
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@xs4all.nl> 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@xs4all.nl>

>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

>>> ---

>>>  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);

>>>

>>

> 

>