Re: [PATCH 4/6] uvcvideo: Set device_caps in VIDIOC_QUERYCAP

Re: [PATCH 4/6] uvcvideo: Set device_caps in VIDIOC_QUERYCAP

[Hans Verkuil]

On Thu September 27 2012 17:16:18 Laurent Pinchart wrote:
> Set the capabilities field to global capabilities, and the device_caps
> field to the video node capabilities.
> 
> This issue was found by the v4l2-compliance tool.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c |    5 +++++
>  drivers/media/usb/uvc/uvc_v4l2.c   |   10 ++++++----
>  drivers/media/usb/uvc/uvcvideo.h   |    2 ++
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 5967081..ae24f7d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1741,6 +1741,11 @@ static int uvc_register_video(struct uvc_device *dev,
>  		return ret;
>  	}
>  
> +	if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> +	else
> +		stream->chain->caps |= V4L2_CAP_VIDEO_OUTPUT;
> +
>  	atomic_inc(&dev->nstreams);
>  	return 0;
>  }
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 3bd9373..b1aa55f 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -565,12 +565,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		usb_make_path(stream->dev->udev,
>  			      cap->bus_info, sizeof(cap->bus_info));
>  		cap->version = LINUX_VERSION_CODE;
> +		cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> +				  | chain->caps;
>  		if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> -			cap->capabilities = V4L2_CAP_VIDEO_CAPTURE
> -					  | V4L2_CAP_STREAMING;
> +			cap->device_caps = V4L2_CAP_VIDEO_CAPTURE
> +					 | V4L2_CAP_STREAMING;
>  		else
> -			cap->capabilities = V4L2_CAP_VIDEO_OUTPUT
> -					  | V4L2_CAP_STREAMING;
> +			cap->device_caps = V4L2_CAP_VIDEO_OUTPUT
> +					 | V4L2_CAP_STREAMING;

This seems weird. Wouldn't it be easier to do:

		cap->device_caps = chain->caps | V4L2_CAP_STREAMING;

You don't need the if/else here.

>  		break;
>  	}
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 7244455..28ff015 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -371,6 +371,8 @@ struct uvc_video_chain {
>  	struct uvc_entity *selector;		/* Selector unit */
>  
>  	struct mutex ctrl_mutex;		/* Protects ctrl.info */
> +
> +	u32 caps;				/* V4L2 chain-wide caps */
>  };
>  
>  struct uvc_stats_frame {
> 

Regards,

	Hans

Re: [PATCH 6/6] uvcvideo: Add VIDIOC_[GS]_PRIORITY support

[Hans Verkuil]

On Thu September 27 2012 17:16:20 Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c |    3 ++
>  drivers/media/usb/uvc/uvc_v4l2.c   |   45 ++++++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |    1 +
>  3 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index ae24f7d..22f14d2 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1562,6 +1562,7 @@ static int uvc_scan_device(struct uvc_device *dev)
>  		INIT_LIST_HEAD(&chain->entities);
>  		mutex_init(&chain->ctrl_mutex);
>  		chain->dev = dev;
> +		v4l2_prio_init(&chain->prio);
>  
>  		if (uvc_scan_chain(chain, term) < 0) {
>  			kfree(chain);
> @@ -1722,6 +1723,8 @@ static int uvc_register_video(struct uvc_device *dev,
>  	vdev->v4l2_dev = &dev->vdev;
>  	vdev->fops = &uvc_fops;
>  	vdev->release = uvc_release;
> +	vdev->prio = &stream->chain->prio;
> +	set_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags);

This set_bit() doesn't do anything as long as you are not using video_ioctl2().
And why aren't you using video_ioctl2()? This is the last driver to do it all
manually. If you'd switch to video_ioctl2(), then setting this bit would be
all you had to do.

>  	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>  		vdev->vfl_dir = VFL_DIR_TX;
>  	strlcpy(vdev->name, dev->name, sizeof vdev->name);
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index bf9d073..d6aa402 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -576,6 +576,19 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		break;
>  	}
>  
> +	/* Priority */
> +	case VIDIOC_G_PRIORITY:
> +		*(u32 *)arg = v4l2_prio_max(vdev->prio);
> +		break;
> +
> +	case VIDIOC_S_PRIORITY:
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
> +		return v4l2_prio_change(vdev->prio, &handle->vfh.prio,
> +					*(u32 *)arg);
> +
>  	/* Get, Set & Query control */
>  	case VIDIOC_QUERYCTRL:
>  		return uvc_query_v4l2_ctrl(chain, arg);
> @@ -606,6 +619,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		struct v4l2_control *ctrl = arg;
>  		struct v4l2_ext_control xctrl;
>  
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		memset(&xctrl, 0, sizeof xctrl);
>  		xctrl.id = ctrl->id;
>  		xctrl.value = ctrl->value;
> @@ -653,6 +670,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  	}
>  
>  	case VIDIOC_S_EXT_CTRLS:
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  	case VIDIOC_TRY_EXT_CTRLS:
>  	{
>  		struct v4l2_ext_controls *ctrls = arg;
> @@ -747,6 +768,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  	{
>  		u32 input = *(u32 *)arg + 1;
>  
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		if ((ret = uvc_acquire_privileges(handle)) < 0)
>  			return ret;
>  
> @@ -800,6 +825,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  	}
>  
>  	case VIDIOC_S_FMT:
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		if ((ret = uvc_acquire_privileges(handle)) < 0)
>  			return ret;
>  
> @@ -902,6 +931,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		return uvc_v4l2_get_streamparm(stream, arg);
>  
>  	case VIDIOC_S_PARM:
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		if ((ret = uvc_acquire_privileges(handle)) < 0)
>  			return ret;
>  
> @@ -936,6 +969,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  
>  	/* Buffers & streaming */
>  	case VIDIOC_REQBUFS:
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		if ((ret = uvc_acquire_privileges(handle)) < 0)
>  			return ret;
>  
> @@ -981,6 +1018,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		if (*type != stream->type)
>  			return -EINVAL;
>  
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		if (!uvc_has_privileges(handle))
>  			return -EBUSY;
>  
> @@ -999,6 +1040,10 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		if (*type != stream->type)
>  			return -EINVAL;
>  
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;
> +
>  		if (!uvc_has_privileges(handle))
>  			return -EBUSY;

This patch is hard to read since I can't see for which ioctls you check the prio.
Can you regenerate the patch with more context lines? The patch as it is will
probably not apply reliably due to the same reason.

In particular, make sure you also check for the UVC-specific ioctls (UVCIOC_CTRL_MAP
might need this, but I'm not sure about that).

Regards,

	Hans

>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 28ff015..acf6bf2 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -372,6 +372,7 @@ struct uvc_video_chain {
>  
>  	struct mutex ctrl_mutex;		/* Protects ctrl.info */
>  
> +	struct v4l2_prio_state prio;		/* V4L2 priority state */
>  	u32 caps;				/* V4L2 chain-wide caps */
>  };
>  
> 

Re: [PATCH 0/6] uvcvideo: V4L2 compliance fixes

[Hans Verkuil]

Hi Laurent,

My apologies for the long delay before I got around to reviewing these.

On Thu September 27 2012 17:16:14 Laurent Pinchart wrote:
> Hi everybody,
> 
> Here are 6 patches that fix V4L2 compliance issues in the uvcvideo driver found
> by the v4l2-compliance tool.
> 
> I'm working on the last remaining issue (control classes not implemented).
> 
> Laurent Pinchart (6):
>   uvcvideo: Set error_idx properly for extended controls API failures
>   uvcvideo: Return -EACCES when trying to access a read/write-only
>     control
>   uvcvideo: Don't fail when an unsupported format is requested
>   uvcvideo: Set device_caps in VIDIOC_QUERYCAP
>   uvcvideo: Return -ENOTTY for unsupported ioctls
>   uvcvideo: Add VIDIOC_[GS]_PRIORITY support
> 
>  drivers/media/usb/uvc/uvc_ctrl.c   |   21 +++++----
>  drivers/media/usb/uvc/uvc_driver.c |    8 +++
>  drivers/media/usb/uvc/uvc_v4l2.c   |   89 ++++++++++++++++++++++++++++-------
>  drivers/media/usb/uvc/uvc_video.c  |    1 +
>  drivers/media/usb/uvc/uvcvideo.h   |    4 ++
>  5 files changed, 96 insertions(+), 27 deletions(-)
> 
> 

For patches 1, 2, 3 and 5:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

I've got some comments for patches 4 and 6. See my reply to those.

Regards,

	Hans

Re: [PATCH 4/6] uvcvideo: Set device_caps in VIDIOC_QUERYCAP

[Laurent Pinchart]

Hi Hans,

Thanks for the review.

On Friday 16 November 2012 15:00:29 Hans Verkuil wrote:
> On Thu September 27 2012 17:16:18 Laurent Pinchart wrote:
> > Set the capabilities field to global capabilities, and the device_caps
> > field to the video node capabilities.
> > 
> > This issue was found by the v4l2-compliance tool.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/usb/uvc/uvc_driver.c |    5 +++++
> >  drivers/media/usb/uvc/uvc_v4l2.c   |   10 ++++++----
> >  drivers/media/usb/uvc/uvcvideo.h   |    2 ++
> >  3 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index 5967081..ae24f7d 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1741,6 +1741,11 @@ static int uvc_register_video(struct uvc_device
> > *dev,
> >  		return ret;
> >  	}
> > 
> > +	if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> > +	else
> > +		stream->chain->caps |= V4L2_CAP_VIDEO_OUTPUT;
> > +
> >  	atomic_inc(&dev->nstreams);
> >  	return 0;
> >  }
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c index 3bd9373..b1aa55f 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -565,12 +565,14 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > unsigned int cmd, void *arg)> 
> >  		usb_make_path(stream->dev->udev,
> >  			      cap->bus_info, sizeof(cap->bus_info));
> >  		cap->version = LINUX_VERSION_CODE;
> > +		cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> > +				  | chain->caps;
> >  		if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > -			cap->capabilities = V4L2_CAP_VIDEO_CAPTURE
> > -					  | V4L2_CAP_STREAMING;
> > +			cap->device_caps = V4L2_CAP_VIDEO_CAPTURE
> > +					 | V4L2_CAP_STREAMING;
> >  		else
> > -			cap->capabilities = V4L2_CAP_VIDEO_OUTPUT
> > -					  | V4L2_CAP_STREAMING;
> > +			cap->device_caps = V4L2_CAP_VIDEO_OUTPUT
> > +					 | V4L2_CAP_STREAMING;
> 
> This seems weird. Wouldn't it be easier to do:
> 
> 		cap->device_caps = chain->caps | V4L2_CAP_STREAMING;
> 
> You don't need the if/else here.

No, because chain->caps can be V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT 
as a chain can contain several video nodes. We want to caps of this particular 
video node only here.

> >  		break;
> >  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] uvcvideo: Add VIDIOC_[GS]_PRIORITY support

[Laurent Pinchart]

Hi Hans,

Thank you for the review.

On Friday 16 November 2012 15:07:42 Hans Verkuil wrote:
> On Thu September 27 2012 17:16:20 Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/usb/uvc/uvc_driver.c |    3 ++
> >  drivers/media/usb/uvc/uvc_v4l2.c   |   45 +++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |    1 +
> >  3 files changed, 49 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index ae24f7d..22f14d2 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c

[snip]

> > @@ -1722,6 +1723,8 @@ static int uvc_register_video(struct uvc_device
> > *dev,
> >  	vdev->v4l2_dev = &dev->vdev;
> >  	vdev->fops = &uvc_fops;
> >  	vdev->release = uvc_release;
> > +	vdev->prio = &stream->chain->prio;
> > +	set_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags);
> 
> This set_bit() doesn't do anything as long as you are not using
> video_ioctl2().

The bit also makes v4l2_fh_(add|del)() call v4l2_prio_(open|close)().

> And why aren't you using video_ioctl2()? This is the last driver to do it
> all manually. If you'd switch to video_ioctl2(), then setting this bit would
> be all you had to do.

I have a patch for that, I need to resurect it.

> >  	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> >  		vdev->vfl_dir = VFL_DIR_TX;
> >  	
> >  	strlcpy(vdev->name, dev->name, sizeof vdev->name);
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c index bf9d073..d6aa402 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c

[snip]

> This patch is hard to read since I can't see for which ioctls you check the
> prio. Can you regenerate the patch with more context lines? The patch as it
> is will probably not apply reliably due to the same reason.

My bad. I'll resend it.

> In particular, make sure you also check for the UVC-specific ioctls
> (UVCIOC_CTRL_MAP might need this, but I'm not sure about that).

The UVC-specific ioctls are only control operations, they don't require 
priority handling.

-- 
Regards,

Laurent Pinchart

[PATCH v2 6/6] uvcvideo: Add VIDIOC_[GS]_PRIORITY support

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c |    3 ++
 drivers/media/usb/uvc/uvc_v4l2.c   |   45 ++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |    1 +
 3 files changed, 49 insertions(+), 0 deletions(-)

Resent with larger contexts to make review easier.

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index ae24f7d..22f14d2 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1560,10 +1560,11 @@ static int uvc_scan_device(struct uvc_device *dev)
 			return -ENOMEM;
 
 		INIT_LIST_HEAD(&chain->entities);
 		mutex_init(&chain->ctrl_mutex);
 		chain->dev = dev;
+		v4l2_prio_init(&chain->prio);
 
 		if (uvc_scan_chain(chain, term) < 0) {
 			kfree(chain);
 			continue;
 		}
@@ -1720,10 +1721,12 @@ static int uvc_register_video(struct uvc_device *dev,
 	 * get another one.
 	 */
 	vdev->v4l2_dev = &dev->vdev;
 	vdev->fops = &uvc_fops;
 	vdev->release = uvc_release;
+	vdev->prio = &stream->chain->prio;
+	set_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags);
 	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		vdev->vfl_dir = VFL_DIR_TX;
 	strlcpy(vdev->name, dev->name, sizeof vdev->name);
 
 	/* Set the driver data before calling video_register_device, otherwise
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index bf9d073..d6aa402 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -574,10 +574,23 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 			cap->device_caps = V4L2_CAP_VIDEO_OUTPUT
 					 | V4L2_CAP_STREAMING;
 		break;
 	}
 
+	/* Priority */
+	case VIDIOC_G_PRIORITY:
+		*(u32 *)arg = v4l2_prio_max(vdev->prio);
+		break;
+
+	case VIDIOC_S_PRIORITY:
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
+		return v4l2_prio_change(vdev->prio, &handle->vfh.prio,
+					*(u32 *)arg);
+
 	/* Get, Set & Query control */
 	case VIDIOC_QUERYCTRL:
 		return uvc_query_v4l2_ctrl(chain, arg);
 
 	case VIDIOC_G_CTRL:
@@ -604,10 +617,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_S_CTRL:
 	{
 		struct v4l2_control *ctrl = arg;
 		struct v4l2_ext_control xctrl;
 
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		memset(&xctrl, 0, sizeof xctrl);
 		xctrl.id = ctrl->id;
 		xctrl.value = ctrl->value;
 
 		ret = uvc_ctrl_begin(chain);
@@ -651,10 +668,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		ret = uvc_ctrl_rollback(handle);
 		break;
 	}
 
 	case VIDIOC_S_EXT_CTRLS:
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 	case VIDIOC_TRY_EXT_CTRLS:
 	{
 		struct v4l2_ext_controls *ctrls = arg;
 		struct v4l2_ext_control *ctrl = ctrls->controls;
 		unsigned int i;
@@ -745,10 +766,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 	case VIDIOC_S_INPUT:
 	{
 		u32 input = *(u32 *)arg + 1;
 
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		if ((ret = uvc_acquire_privileges(handle)) < 0)
 			return ret;
 
 		if (chain->selector == NULL ||
 		    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
@@ -798,10 +823,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 		return uvc_v4l2_try_format(stream, arg, &probe, NULL, NULL);
 	}
 
 	case VIDIOC_S_FMT:
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		if ((ret = uvc_acquire_privileges(handle)) < 0)
 			return ret;
 
 		return uvc_v4l2_set_format(stream, arg);
 
@@ -900,10 +929,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	/* Get & Set streaming parameters */
 	case VIDIOC_G_PARM:
 		return uvc_v4l2_get_streamparm(stream, arg);
 
 	case VIDIOC_S_PARM:
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		if ((ret = uvc_acquire_privileges(handle)) < 0)
 			return ret;
 
 		return uvc_v4l2_set_streamparm(stream, arg);
 
@@ -934,10 +967,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_S_CROP:
 		return -ENOTTY;
 
 	/* Buffers & streaming */
 	case VIDIOC_REQBUFS:
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		if ((ret = uvc_acquire_privileges(handle)) < 0)
 			return ret;
 
 		mutex_lock(&stream->mutex);
 		ret = uvc_alloc_buffers(&stream->queue, arg);
@@ -979,10 +1016,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		int *type = arg;
 
 		if (*type != stream->type)
 			return -EINVAL;
 
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		if (!uvc_has_privileges(handle))
 			return -EBUSY;
 
 		mutex_lock(&stream->mutex);
 		ret = uvc_video_enable(stream, 1);
@@ -997,10 +1038,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		int *type = arg;
 
 		if (*type != stream->type)
 			return -EINVAL;
 
+		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
+		if (ret < 0)
+			return ret;
+
 		if (!uvc_has_privileges(handle))
 			return -EBUSY;
 
 		return uvc_video_enable(stream, 0);
 	}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a6c561f..006ae27 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -370,10 +370,11 @@ struct uvc_video_chain {
 	struct uvc_entity *processing;		/* Processing unit */
 	struct uvc_entity *selector;		/* Selector unit */
 
 	struct mutex ctrl_mutex;		/* Protects ctrl.info */
 
+	struct v4l2_prio_state prio;		/* V4L2 priority state */
 	u32 caps;				/* V4L2 chain-wide caps */
 };
 
 struct uvc_stats_frame {
 	unsigned int size;		/* Number of bytes captured */
-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] uvcvideo: Set device_caps in VIDIOC_QUERYCAP

[Hans Verkuil]

On Fri November 23 2012 13:20:10 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
> On Friday 16 November 2012 15:00:29 Hans Verkuil wrote:
> > On Thu September 27 2012 17:16:18 Laurent Pinchart wrote:
> > > Set the capabilities field to global capabilities, and the device_caps
> > > field to the video node capabilities.
> > > 
> > > This issue was found by the v4l2-compliance tool.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/usb/uvc/uvc_driver.c |    5 +++++
> > >  drivers/media/usb/uvc/uvc_v4l2.c   |   10 ++++++----
> > >  drivers/media/usb/uvc/uvcvideo.h   |    2 ++
> > >  3 files changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > > b/drivers/media/usb/uvc/uvc_driver.c index 5967081..ae24f7d 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1741,6 +1741,11 @@ static int uvc_register_video(struct uvc_device
> > > *dev,
> > >  		return ret;
> > >  	}
> > > 
> > > +	if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > +		stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> > > +	else
> > > +		stream->chain->caps |= V4L2_CAP_VIDEO_OUTPUT;
> > > +
> > >  	atomic_inc(&dev->nstreams);
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > b/drivers/media/usb/uvc/uvc_v4l2.c index 3bd9373..b1aa55f 100644
> > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > @@ -565,12 +565,14 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > unsigned int cmd, void *arg)> 
> > >  		usb_make_path(stream->dev->udev,
> > >  			      cap->bus_info, sizeof(cap->bus_info));
> > >  		cap->version = LINUX_VERSION_CODE;
> > > +		cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> > > +				  | chain->caps;
> > >  		if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > -			cap->capabilities = V4L2_CAP_VIDEO_CAPTURE
> > > -					  | V4L2_CAP_STREAMING;
> > > +			cap->device_caps = V4L2_CAP_VIDEO_CAPTURE
> > > +					 | V4L2_CAP_STREAMING;
> > >  		else
> > > -			cap->capabilities = V4L2_CAP_VIDEO_OUTPUT
> > > -					  | V4L2_CAP_STREAMING;
> > > +			cap->device_caps = V4L2_CAP_VIDEO_OUTPUT
> > > +					 | V4L2_CAP_STREAMING;
> > 
> > This seems weird. Wouldn't it be easier to do:
> > 
> > 		cap->device_caps = chain->caps | V4L2_CAP_STREAMING;
> > 
> > You don't need the if/else here.
> 
> No, because chain->caps can be V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT 
> as a chain can contain several video nodes. We want to caps of this particular 
> video node only here.

That explains it :-)

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

Re: [PATCH v2 6/6] uvcvideo: Add VIDIOC_[GS]_PRIORITY support

[Hans Verkuil]

Hi Laurent,

You were right about your remark about setting USE_FH_PRIO: you do need to do
that here.

Thanks for reposting with more context, now I can see where the prio checks are
added :-)

I have just one small remark:

On Fri November 23 2012 13:32:05 Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c |    3 ++
>  drivers/media/usb/uvc/uvc_v4l2.c   |   45 ++++++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |    1 +
>  3 files changed, 49 insertions(+), 0 deletions(-)
> 
> Resent with larger contexts to make review easier.
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index ae24f7d..22f14d2 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -651,10 +668,14 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  		ret = uvc_ctrl_rollback(handle);
>  		break;
>  	}
>  
>  	case VIDIOC_S_EXT_CTRLS:
> +		ret = v4l2_prio_check(vdev->prio, handle->vfh.prio);
> +		if (ret < 0)
> +			return ret;

Please add a /* fall through */ comment here.

> +
>  	case VIDIOC_TRY_EXT_CTRLS:
>  	{
>  		struct v4l2_ext_controls *ctrls = arg;
>  		struct v4l2_ext_control *ctrl = ctrls->controls;
>  		unsigned int i;

After making that change you can add my Acked-by:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Laurent Pinchart]

Hi Hans,

On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> When one of the requested controls doesn't exist the error_idx field
> must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx
> must be set to the control count. For TRY_EXT_CTRLS, it must be set to
> the index of the unexisting control.
> 
> This issue was found by the v4l2-compliance tool.

I'm revisiting this patch as it has been reverted in v3.8-rc1.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
>  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
>  2 files changed, 22 insertions(+), 14 deletions(-)

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,

[snip]

> @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
>  			if (ret < 0) {
>  				uvc_ctrl_rollback(handle);
> -				ctrls->error_idx = i;
> -				return ret;
> +				ctrls->error_idx = ret == -ENOENT
> +						 ? ctrls->count : i;
> +				return ret == -ENOENT ? -EINVAL : ret;
>  			}
>  		}
>  		ctrls->error_idx = 0;
> @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
>  			if (ret < 0) {
>  				uvc_ctrl_rollback(handle);
> -				ctrls->error_idx = i;
> -				return ret;
> +				ctrls->error_idx = (ret == -ENOENT &&
> +						    cmd == VIDIOC_S_EXT_CTRLS)
> +						 ? ctrls->count : i;
> +				return ret == -ENOENT ? -EINVAL : ret;
>  			}
>  		}

I've reread the V4L2 specification, and the least I can say is that the text 
is pretty ambiguous. Let's clarify it.

Is there a reason to differentiate between invalid control IDs and other 
errors as far as error_idx is concerned ? It would be simpler if error_idx was 
set to the index of the first error for get and try operations, regardless of 
the error type. What do you think ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Hans Verkuil]

On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > When one of the requested controls doesn't exist the error_idx field
> > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx
> > must be set to the control count. For TRY_EXT_CTRLS, it must be set to
> > the index of the unexisting control.
> > 
> > This issue was found by the v4l2-compliance tool.
> 
> I'm revisiting this patch as it has been reverted in v3.8-rc1.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> >  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> 
> [snip]
> 
> > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> >  			if (ret < 0) {
> >  				uvc_ctrl_rollback(handle);
> > -				ctrls->error_idx = i;
> > -				return ret;
> > +				ctrls->error_idx = ret == -ENOENT
> > +						 ? ctrls->count : i;
> > +				return ret == -ENOENT ? -EINVAL : ret;
> >  			}
> >  		}
> >  		ctrls->error_idx = 0;
> > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> >  			if (ret < 0) {
> >  				uvc_ctrl_rollback(handle);
> > -				ctrls->error_idx = i;
> > -				return ret;
> > +				ctrls->error_idx = (ret == -ENOENT &&
> > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > +						 ? ctrls->count : i;
> > +				return ret == -ENOENT ? -EINVAL : ret;
> >  			}
> >  		}
> 
> I've reread the V4L2 specification, and the least I can say is that the text 
> is pretty ambiguous. Let's clarify it.
> 
> Is there a reason to differentiate between invalid control IDs and other 
> errors as far as error_idx is concerned ? It would be simpler if error_idx was 
> set to the index of the first error for get and try operations, regardless of 
> the error type. What do you think ?

There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to be as atomic
as possible, i.e. it should try hard to prevent leaving the hardware in an
inconsistent state because not all controls could be set. It can never be fully
atomic since writing multiple registers over usb or i2c can always return errors
for one of those writes, but it should certainly check for all the obvious
errors first that do not require actually writing to the hardware, such as
whether all the controls in the control list actually exist.

And for such errors error_idx should be set to the number of controls to
indicate that none of the controls were actually set but that there was a
problem with the list of controls itself.

Since TRY_EXT_CTRLS doesn't touch the hardware at all the error_idx can be set
to the index of the control that caused the problem.

The documentation can definitely be improved as the difference between G/S and
TRY with regards to error_idx is not made explicit.

Regards,

	Hans

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Laurent Pinchart]

Hi Hans,

On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > When one of the requested controls doesn't exist the error_idx field
> > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx
> > > must be set to the control count. For TRY_EXT_CTRLS, it must be set to
> > > the index of the unexisting control.
> > > 
> > > This issue was found by the v4l2-compliance tool.
> > 
> > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > 
> > [snip]
> > 
> > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > 
> > [snip]
> > 
> > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > 
> > >  			if (ret < 0) {
> > >  			
> > >  				uvc_ctrl_rollback(handle);
> > > 
> > > -				ctrls->error_idx = i;
> > > -				return ret;
> > > +				ctrls->error_idx = ret == -ENOENT
> > > +						 ? ctrls->count : i;
> > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > 
> > >  			}
> > >  		
> > >  		}
> > >  		ctrls->error_idx = 0;
> > > 
> > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > 
> > >  			if (ret < 0) {
> > >  			
> > >  				uvc_ctrl_rollback(handle);
> > > 
> > > -				ctrls->error_idx = i;
> > > -				return ret;
> > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > +						 ? ctrls->count : i;
> > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > 
> > >  			}
> > >  		
> > >  		}
> > 
> > I've reread the V4L2 specification, and the least I can say is that the
> > text is pretty ambiguous. Let's clarify it.
> > 
> > Is there a reason to differentiate between invalid control IDs and other
> > errors as far as error_idx is concerned ? It would be simpler if error_idx
> > was set to the index of the first error for get and try operations,
> > regardless of the error type. What do you think ?
> 
> There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to be
> as atomic as possible, i.e. it should try hard to prevent leaving the
> hardware in an inconsistent state because not all controls could be set. It
> can never be fully atomic since writing multiple registers over usb or i2c
> can always return errors for one of those writes, but it should certainly
> check for all the obvious errors first that do not require actually writing
> to the hardware, such as whether all the controls in the control list
> actually exist.
> 
> And for such errors error_idx should be set to the number of controls to
> indicate that none of the controls were actually set but that there was a
> problem with the list of controls itself.

For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, so 
it could get all controls up to the erroneous one.

> Since TRY_EXT_CTRLS doesn't touch the hardware at all the error_idx can be
> set to the index of the control that caused the problem.
> 
> The documentation can definitely be improved as the difference between G/S
> and TRY with regards to error_idx is not made explicit.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Hans Verkuil]

On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > When one of the requested controls doesn't exist the error_idx field
> > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx
> > > > must be set to the control count. For TRY_EXT_CTRLS, it must be set to
> > > > the index of the unexisting control.
> > > > 
> > > > This issue was found by the v4l2-compliance tool.
> > > 
> > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > 
> > > [snip]
> > > 
> > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > 
> > > [snip]
> > > 
> > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > 
> > > >  			if (ret < 0) {
> > > >  			
> > > >  				uvc_ctrl_rollback(handle);
> > > > 
> > > > -				ctrls->error_idx = i;
> > > > -				return ret;
> > > > +				ctrls->error_idx = ret == -ENOENT
> > > > +						 ? ctrls->count : i;
> > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > 
> > > >  			}
> > > >  		
> > > >  		}
> > > >  		ctrls->error_idx = 0;
> > > > 
> > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > 
> > > >  			if (ret < 0) {
> > > >  			
> > > >  				uvc_ctrl_rollback(handle);
> > > > 
> > > > -				ctrls->error_idx = i;
> > > > -				return ret;
> > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > +						 ? ctrls->count : i;
> > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > 
> > > >  			}
> > > >  		
> > > >  		}
> > > 
> > > I've reread the V4L2 specification, and the least I can say is that the
> > > text is pretty ambiguous. Let's clarify it.
> > > 
> > > Is there a reason to differentiate between invalid control IDs and other
> > > errors as far as error_idx is concerned ? It would be simpler if error_idx
> > > was set to the index of the first error for get and try operations,
> > > regardless of the error type. What do you think ?
> > 
> > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to be
> > as atomic as possible, i.e. it should try hard to prevent leaving the
> > hardware in an inconsistent state because not all controls could be set. It
> > can never be fully atomic since writing multiple registers over usb or i2c
> > can always return errors for one of those writes, but it should certainly
> > check for all the obvious errors first that do not require actually writing
> > to the hardware, such as whether all the controls in the control list
> > actually exist.
> > 
> > And for such errors error_idx should be set to the number of controls to
> > indicate that none of the controls were actually set but that there was a
> > problem with the list of controls itself.
> 
> For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state, so 
> it could get all controls up to the erroneous one.

I have thought about that but I decided against it. One reason is to have get
and set behave the same since both access the hardware. The other reason is
that even getting a control value might change the hardware state, for example
by resetting some internal hardware counter when a register is read (it's rare
but there is hardware like that). Furthermore, reading hardware registers can
be slow so why not do the sanity check first?

Regards,

	Hans

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Laurent Pinchart]

Hi Hans,

On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > When one of the requested controls doesn't exist the error_idx field
> > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS,
> > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it
> > > > > must be set to the index of the unexisting control.
> > > > > 
> > > > > This issue was found by the v4l2-compliance tool.
> > > > 
> > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > > 
> > > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > 
> > > > [snip]
> > > > 
> > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > *file,
> > > > 
> > > > [snip]
> > > > 
> > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > >  			if (ret < 0) {
> > > > >  				uvc_ctrl_rollback(handle);
> > > > > 
> > > > > -				ctrls->error_idx = i;
> > > > > -				return ret;
> > > > > +				ctrls->error_idx = ret == -ENOENT
> > > > > +						 ? ctrls->count : i;
> > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > >  			}
> > > > >  		}
> > > > >  		ctrls->error_idx = 0;
> > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > *file,
> > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > >  			if (ret < 0) {
> > > > >  				uvc_ctrl_rollback(handle);
> > > > > 
> > > > > -				ctrls->error_idx = i;
> > > > > -				return ret;
> > > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > > +						 ? ctrls->count : i;
> > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > >  			}
> > > > >  		}
> > > > 
> > > > I've reread the V4L2 specification, and the least I can say is that
> > > > the text is pretty ambiguous. Let's clarify it.
> > > > 
> > > > Is there a reason to differentiate between invalid control IDs and
> > > > other errors as far as error_idx is concerned ? It would be simpler if
> > > > error_idx was set to the index of the first error for get and try
> > > > operations, regardless of the error type. What do you think ?
> > > 
> > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to
> > > be as atomic as possible, i.e. it should try hard to prevent leaving the
> > > hardware in an inconsistent state because not all controls could be set.
> > > It can never be fully atomic since writing multiple registers over usb
> > > or i2c can always return errors for one of those writes, but it should
> > > certainly check for all the obvious errors first that do not require
> > > actually writing to the hardware, such as whether all the controls in
> > > the control list actually exist.
> > > 
> > > And for such errors error_idx should be set to the number of controls to
> > > indicate that none of the controls were actually set but that there was
> > > a problem with the list of controls itself.
> > 
> > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state,
> > so it could get all controls up to the erroneous one.
> 
> I have thought about that but I decided against it. One reason is to have
> get and set behave the same since both access the hardware. The other
> reason is that even getting a control value might change the hardware
> state, for example by resetting some internal hardware counter when a
> register is read (it's rare but there is hardware like that). Furthermore,
> reading hardware registers can be slow so why not do the sanity check
> first?

Get can indeed change the device state in rare cases, but the information 
won't be lost, as the value of all controls before error_idx will be returned.

What bothers me with the current G_EXT_CTRLS implementation (beside that it's 
very slightly more complex for the uvcvideo driver than the one I propose) is 
that an application will have no way to know for which control G_EXT_CTRLS 
failed. This is especially annoying during development.

Maybe we could leave this behaviour as driver-specific ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Mauro Carvalho Chehab]

Em Wed, 26 Dec 2012 12:33:58 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Hans,
> 
> On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > > When one of the requested controls doesn't exist the error_idx field
> > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS,
> > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it
> > > > > > must be set to the index of the unexisting control.
> > > > > > 
> > > > > > This issue was found by the v4l2-compliance tool.
> > > > > 
> > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > > 
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > *file,
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > > >  			if (ret < 0) {
> > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > 
> > > > > > -				ctrls->error_idx = i;
> > > > > > -				return ret;
> > > > > > +				ctrls->error_idx = ret == -ENOENT
> > > > > > +						 ? ctrls->count : i;
> > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > >  			}
> > > > > >  		}
> > > > > >  		ctrls->error_idx = 0;
> > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > *file,
> > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > > >  			if (ret < 0) {
> > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > 
> > > > > > -				ctrls->error_idx = i;
> > > > > > -				return ret;
> > > > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > > > +						 ? ctrls->count : i;
> > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > >  			}
> > > > > >  		}
> > > > > 
> > > > > I've reread the V4L2 specification, and the least I can say is that
> > > > > the text is pretty ambiguous. Let's clarify it.
> > > > > 
> > > > > Is there a reason to differentiate between invalid control IDs and
> > > > > other errors as far as error_idx is concerned ? It would be simpler if
> > > > > error_idx was set to the index of the first error for get and try
> > > > > operations, regardless of the error type. What do you think ?
> > > > 
> > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to
> > > > be as atomic as possible, i.e. it should try hard to prevent leaving the
> > > > hardware in an inconsistent state because not all controls could be set.
> > > > It can never be fully atomic since writing multiple registers over usb
> > > > or i2c can always return errors for one of those writes, but it should
> > > > certainly check for all the obvious errors first that do not require
> > > > actually writing to the hardware, such as whether all the controls in
> > > > the control list actually exist.
> > > > 
> > > > And for such errors error_idx should be set to the number of controls to
> > > > indicate that none of the controls were actually set but that there was
> > > > a problem with the list of controls itself.
> > > 
> > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state,
> > > so it could get all controls up to the erroneous one.
> > 
> > I have thought about that but I decided against it. One reason is to have
> > get and set behave the same since both access the hardware. The other
> > reason is that even getting a control value might change the hardware
> > state, for example by resetting some internal hardware counter when a
> > register is read (it's rare but there is hardware like that). Furthermore,
> > reading hardware registers can be slow so why not do the sanity check
> > first?
> 
> Get can indeed change the device state in rare cases, but the information 
> won't be lost, as the value of all controls before error_idx will be returned.

Huh? reading a control should never alter the device's state. If the hardware
is resetting a register, then such register should be shadowed, and some other
way to explicitly reset its value should be used.

> What bothers me with the current G_EXT_CTRLS implementation (beside that it's 
> very slightly more complex for the uvcvideo driver than the one I propose) is 
> that an application will have no way to know for which control G_EXT_CTRLS 
> failed. This is especially annoying during development.
> 
> Maybe we could leave this behaviour as driver-specific ?

driver-specific behavior for IOCTL's should be avoided, as applications will
fail if they see something it doesn't expect.

-- 

Cheers,
Mauro

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Laurent Pinchart]

Hi Mauro,

On Wednesday 26 December 2012 12:00:35 Mauro Carvalho Chehab wrote:
> Em Wed, 26 Dec 2012 12:33:58 +0100 Laurent Pinchart escreveu:
> > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > > > When one of the requested controls doesn't exist the error_idx
> > > > > > > field must reflect that situation. For G_EXT_CTRLS and
> > > > > > > S_EXT_CTRLS, error_idx must be set to the control count. For
> > > > > > > TRY_EXT_CTRLS, it must be set to the index of the unexisting
> > > > > > > control.
> > > > > > > 
> > > > > > > This issue was found by the v4l2-compliance tool.
> > > > > > 
> > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > > > 
> > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > > <laurent.pinchart@ideasonboard.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > > > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > > > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > > > > 
> > > > > > >  			if (ret < 0) {
> > > > > > >  			
> > > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > > 
> > > > > > > -				ctrls->error_idx = i;
> > > > > > > -				return ret;
> > > > > > > +				ctrls->error_idx = ret == -ENOENT
> > > > > > > +						 ? ctrls->count : i;
> > > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > > > 
> > > > > > >  			}
> > > > > > >  		
> > > > > > >  		}
> > > > > > >  		ctrls->error_idx = 0;
> > > > > > > 
> > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > > > > 
> > > > > > >  			if (ret < 0) {
> > > > > > >  			
> > > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > > 
> > > > > > > -				ctrls->error_idx = i;
> > > > > > > -				return ret;
> > > > > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > > > > +						 ? ctrls->count : i;
> > > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > > > 
> > > > > > >  			}
> > > > > > >  		
> > > > > > >  		}
> > > > > > 
> > > > > > I've reread the V4L2 specification, and the least I can say is
> > > > > > that the text is pretty ambiguous. Let's clarify it.
> > > > > > 
> > > > > > Is there a reason to differentiate between invalid control IDs and
> > > > > > other errors as far as error_idx is concerned ? It would be
> > > > > > simpler if error_idx was set to the index of the first error for
> > > > > > get and try operations, regardless of the error type. What do you
> > > > > > think ?
> > > > > 
> > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have
> > > > > to be as atomic as possible, i.e. it should try hard to prevent
> > > > > leaving the hardware in an inconsistent state because not all
> > > > > controls could be set. It can never be fully atomic since writing
> > > > > multiple registers over usb or i2c can always return errors for one
> > > > > of those writes, but it should certainly check for all the obvious
> > > > > errors first that do not require actually writing to the hardware,
> > > > > such as whether all the controls in the control list actually exist.
> > > > > 
> > > > > And for such errors error_idx should be set to the number of
> > > > > controls to indicate that none of the controls were actually set but
> > > > > that there was a problem with the list of controls itself.
> > > > 
> > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware
> > > > state, so it could get all controls up to the erroneous one.
> > > 
> > > I have thought about that but I decided against it. One reason is to
> > > have get and set behave the same since both access the hardware. The
> > > other reason is that even getting a control value might change the
> > > hardware state, for example by resetting some internal hardware counter
> > > when a register is read (it's rare but there is hardware like that).
> > > Furthermore, reading hardware registers can be slow so why not do the
> > > sanity check first?
> > 
> > Get can indeed change the device state in rare cases, but the information
> > won't be lost, as the value of all controls before error_idx will be
> > returned.
>
> Huh? reading a control should never alter the device's state. If the
> hardware is resetting a register, then such register should be shadowed,
> and some other way to explicitly reset its value should be used.

The hardware can expose a read-only counter in such a way that the counter is 
reset when read. That would be pretty rare for V4L devices though, I'm not 
aware of any such implementation in any of the devices we support. A common 
way to handle those registers is to turn then in software into a counter that 
is never reset, so we have a solution anyway (this is getting a bit out of 
scope).

> > What bothers me with the current G_EXT_CTRLS implementation (beside that
> > it's very slightly more complex for the uvcvideo driver than the one I
> > propose) is that an application will have no way to know for which
> > control G_EXT_CTRLS failed. This is especially annoying during
> > development.
> > 
> > Maybe we could leave this behaviour as driver-specific ?
> 
> driver-specific behavior for IOCTL's should be avoided, as applications will
> fail if they see something it doesn't expect.

I'm not asking for an unspecified behaviour here, but for giving freedom to 
drivers to choose among the specified behaviours. The V4L specification 
explains that the error_idx can be set to the total number of controls, in 
which case no control was read or written, or to the index of the first 
erroneous control, in which case all controls before that index are read or 
written successfully. For get operations I believe that getting all controls 
up to the first error is the best behaviour (even when we can detect errors 
such as invalid control IDs up front), but I'm not opposed to drivers 
returning error_idx set to the total number of controls without getting any 
control.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Hans Verkuil]

On Wed December 26 2012 12:33:58 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > > When one of the requested controls doesn't exist the error_idx field
> > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS,
> > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it
> > > > > > must be set to the index of the unexisting control.
> > > > > > 
> > > > > > This issue was found by the v4l2-compliance tool.
> > > > > 
> > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > > 
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > *file,
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > > >  			if (ret < 0) {
> > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > 
> > > > > > -				ctrls->error_idx = i;
> > > > > > -				return ret;
> > > > > > +				ctrls->error_idx = ret == -ENOENT
> > > > > > +						 ? ctrls->count : i;
> > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > >  			}
> > > > > >  		}
> > > > > >  		ctrls->error_idx = 0;
> > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > *file,
> > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > > >  			if (ret < 0) {
> > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > 
> > > > > > -				ctrls->error_idx = i;
> > > > > > -				return ret;
> > > > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > > > +						 ? ctrls->count : i;
> > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > >  			}
> > > > > >  		}
> > > > > 
> > > > > I've reread the V4L2 specification, and the least I can say is that
> > > > > the text is pretty ambiguous. Let's clarify it.
> > > > > 
> > > > > Is there a reason to differentiate between invalid control IDs and
> > > > > other errors as far as error_idx is concerned ? It would be simpler if
> > > > > error_idx was set to the index of the first error for get and try
> > > > > operations, regardless of the error type. What do you think ?
> > > > 
> > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to
> > > > be as atomic as possible, i.e. it should try hard to prevent leaving the
> > > > hardware in an inconsistent state because not all controls could be set.
> > > > It can never be fully atomic since writing multiple registers over usb
> > > > or i2c can always return errors for one of those writes, but it should
> > > > certainly check for all the obvious errors first that do not require
> > > > actually writing to the hardware, such as whether all the controls in
> > > > the control list actually exist.
> > > > 
> > > > And for such errors error_idx should be set to the number of controls to
> > > > indicate that none of the controls were actually set but that there was
> > > > a problem with the list of controls itself.
> > > 
> > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state,
> > > so it could get all controls up to the erroneous one.
> > 
> > I have thought about that but I decided against it. One reason is to have
> > get and set behave the same since both access the hardware. The other
> > reason is that even getting a control value might change the hardware
> > state, for example by resetting some internal hardware counter when a
> > register is read (it's rare but there is hardware like that). Furthermore,
> > reading hardware registers can be slow so why not do the sanity check
> > first?
> 
> Get can indeed change the device state in rare cases, but the information 
> won't be lost, as the value of all controls before error_idx will be returned.
> 
> What bothers me with the current G_EXT_CTRLS implementation (beside that it's 
> very slightly more complex for the uvcvideo driver than the one I propose) is 
> that an application will have no way to know for which control G_EXT_CTRLS 
> failed. This is especially annoying during development.

For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control failed,
but you don't have that option for G_EXT_CTRLS. That's actually something I
hadn't considered.

> Maybe we could leave this behaviour as driver-specific ?

I need to think about this some more. Is this urgent or can it wait until
January 7th? I'm back at work by then. I am actually attempting to touch my
computer as little as possible this vacation :-)

Regards,

	Hans

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Laurent Pinchart]

Hi Hans,

On Thursday 27 December 2012 12:59:15 Hans Verkuil wrote:
> On Wed December 26 2012 12:33:58 Laurent Pinchart wrote:
> > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > > > When one of the requested controls doesn't exist the error_idx
> > > > > > > field must reflect that situation. For G_EXT_CTRLS and
> > > > > > > S_EXT_CTRLS, error_idx must be set to the control count. For
> > > > > > > TRY_EXT_CTRLS, it must be set to the index of the unexisting
> > > > > > > control.
> > > > > > > 
> > > > > > > This issue was found by the v4l2-compliance tool.
> > > > > > 
> > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > > > 
> > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > > <laurent.pinchart@ideasonboard.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > > > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > > > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > > > > 
> > > > > > >  			if (ret < 0) {
> > > > > > >  			
> > > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > > 
> > > > > > > -				ctrls->error_idx = i;
> > > > > > > -				return ret;
> > > > > > > +				ctrls->error_idx = ret == -ENOENT
> > > > > > > +						 ? ctrls->count : i;
> > > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > > > 
> > > > > > >  			}
> > > > > > >  		
> > > > > > >  		}
> > > > > > >  		ctrls->error_idx = 0;
> > > > > > > 
> > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > > > > 
> > > > > > >  			if (ret < 0) {
> > > > > > >  			
> > > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > > 
> > > > > > > -				ctrls->error_idx = i;
> > > > > > > -				return ret;
> > > > > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > > > > +						 ? ctrls->count : i;
> > > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > > > 
> > > > > > >  			}
> > > > > > >  		
> > > > > > >  		}
> > > > > > 
> > > > > > I've reread the V4L2 specification, and the least I can say is
> > > > > > that
> > > > > > the text is pretty ambiguous. Let's clarify it.
> > > > > > 
> > > > > > Is there a reason to differentiate between invalid control IDs and
> > > > > > other errors as far as error_idx is concerned ? It would be
> > > > > > simpler if error_idx was set to the index of the first error for
> > > > > > get and try operations, regardless of the error type. What do you
> > > > > > think ?
> > > > > 
> > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have
> > > > > to be as atomic as possible, i.e. it should try hard to prevent
> > > > > leaving the hardware in an inconsistent state because not all
> > > > > controls could be set. It can never be fully atomic since writing
> > > > > multiple registers over usb or i2c can always return errors for one
> > > > > of those writes, but it should certainly check for all the obvious
> > > > > errors first that do not require actually writing to the hardware,
> > > > > such as whether all the controls in the control list actually exist.
> > > > > 
> > > > > And for such errors error_idx should be set to the number of
> > > > > controls to indicate that none of the controls were actually set but
> > > > > that there was a problem with the list of controls itself.
> > > > 
> > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware
> > > > state, so it could get all controls up to the erroneous one.
> > > 
> > > I have thought about that but I decided against it. One reason is to
> > > have get and set behave the same since both access the hardware. The
> > > other reason is that even getting a control value might change the
> > > hardware state, for example by resetting some internal hardware counter
> > > when a register is read (it's rare but there is hardware like that).
> > > Furthermore, reading hardware registers can be slow so why not do the
> > > sanity check first?
> > 
> > Get can indeed change the device state in rare cases, but the information
> > won't be lost, as the value of all controls before error_idx will be
> > returned.
> > 
> > What bothers me with the current G_EXT_CTRLS implementation (beside that
> > it's very slightly more complex for the uvcvideo driver than the one I
> > propose) is that an application will have no way to know for which
> > control G_EXT_CTRLS failed. This is especially annoying during
> > development.
> 
> For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control
> failed, but you don't have that option for G_EXT_CTRLS. That's actually
> something I hadn't considered.
> 
> > Maybe we could leave this behaviour as driver-specific ?
> 
> I need to think about this some more. Is this urgent or can it wait until
> January 7th? I'm back at work by then. I am actually attempting to touch my
> computer as little as possible this vacation :-)

There's a v3.8 related regression in uvcvideo that I need to fix, but that can 
certainly wait until January the 7th.

Enjoy your holidays and get away from the keyboard now :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Hans Verkuil]

On Thu December 27 2012 12:59:15 Hans Verkuil wrote:
> On Wed December 26 2012 12:33:58 Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > > > When one of the requested controls doesn't exist the error_idx field
> > > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS,
> > > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it
> > > > > > > must be set to the index of the unexisting control.
> > > > > > > 
> > > > > > > This issue was found by the v4l2-compliance tool.
> > > > > > 
> > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > > > 
> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
> > > > > > >  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
> > > > > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > > > >  			if (ret < 0) {
> > > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > > 
> > > > > > > -				ctrls->error_idx = i;
> > > > > > > -				return ret;
> > > > > > > +				ctrls->error_idx = ret == -ENOENT
> > > > > > > +						 ? ctrls->count : i;
> > > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > > >  			}
> > > > > > >  		}
> > > > > > >  		ctrls->error_idx = 0;
> > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > > > >  			if (ret < 0) {
> > > > > > >  				uvc_ctrl_rollback(handle);
> > > > > > > 
> > > > > > > -				ctrls->error_idx = i;
> > > > > > > -				return ret;
> > > > > > > +				ctrls->error_idx = (ret == -ENOENT &&
> > > > > > > +						    cmd == VIDIOC_S_EXT_CTRLS)
> > > > > > > +						 ? ctrls->count : i;
> > > > > > > +				return ret == -ENOENT ? -EINVAL : ret;
> > > > > > >  			}
> > > > > > >  		}
> > > > > > 
> > > > > > I've reread the V4L2 specification, and the least I can say is that
> > > > > > the text is pretty ambiguous. Let's clarify it.
> > > > > > 
> > > > > > Is there a reason to differentiate between invalid control IDs and
> > > > > > other errors as far as error_idx is concerned ? It would be simpler if
> > > > > > error_idx was set to the index of the first error for get and try
> > > > > > operations, regardless of the error type. What do you think ?
> > > > > 
> > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to
> > > > > be as atomic as possible, i.e. it should try hard to prevent leaving the
> > > > > hardware in an inconsistent state because not all controls could be set.
> > > > > It can never be fully atomic since writing multiple registers over usb
> > > > > or i2c can always return errors for one of those writes, but it should
> > > > > certainly check for all the obvious errors first that do not require
> > > > > actually writing to the hardware, such as whether all the controls in
> > > > > the control list actually exist.
> > > > > 
> > > > > And for such errors error_idx should be set to the number of controls to
> > > > > indicate that none of the controls were actually set but that there was
> > > > > a problem with the list of controls itself.
> > > > 
> > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state,
> > > > so it could get all controls up to the erroneous one.
> > > 
> > > I have thought about that but I decided against it. One reason is to have
> > > get and set behave the same since both access the hardware. The other
> > > reason is that even getting a control value might change the hardware
> > > state, for example by resetting some internal hardware counter when a
> > > register is read (it's rare but there is hardware like that). Furthermore,
> > > reading hardware registers can be slow so why not do the sanity check
> > > first?
> > 
> > Get can indeed change the device state in rare cases, but the information 
> > won't be lost, as the value of all controls before error_idx will be returned.
> > 
> > What bothers me with the current G_EXT_CTRLS implementation (beside that it's 
> > very slightly more complex for the uvcvideo driver than the one I propose) is 
> > that an application will have no way to know for which control G_EXT_CTRLS 
> > failed. This is especially annoying during development.
> 
> For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control failed,
> but you don't have that option for G_EXT_CTRLS. That's actually something I
> hadn't considered.
> 
> > Maybe we could leave this behaviour as driver-specific ?

I don't like the idea of leaving this driver-specific. That always bites you
in the end.

I thought seriously about changing the spec so G_EXT_CTRLS behaves like
TRY_EXT_CTRLS when it comes to setting error_idx, but I decided against that.

The main reason is that if G_EXT_CTRLS returns an error_idx < count, then you no
longer know whether the values of the controls up to error_idx-1 were actually
retrieved or not. v4l2-ctrls.c does sanity checks up front, so if it returns
an error for control index 2, does that mean that the sanity checks failed
in which case no controls were retrieved yet, or that getting control index
2 failed due to some hardware-related problem, in which case controls 0 and 1
*were* successfully retrieved.

In addition, changing the behavior means changing the API, albeit in a very
minor way, and I don't think it is worth doing that in this case.

I will prepare a patch that clarifies the API, though. It can certainly be
improved.

Also note that I agree that the situation is not ideal and if I would write
the API from scratch I'd probably handle this a bit differently, most likely
by adding some flags field that can be used to give more precise information.

Regards,

	Hans