[PATCH 0/2] uvcvideo: Refactor code to ease metadata implementation

[PATCH 0/2] uvcvideo: Refactor code to ease metadata implementation

[Laurent Pinchart]

Hi Guennadi,

This small patch series refactors the uvc_video_register() function to extract
the code that you need into a new uvc_video_register_device() function. Please
let me know if it can help.

Laurent Pinchart (2):
  uvcvideo: Factor out video device registration to a function
  uvcvideo: Report V4L2 device caps through the video_device structure

 drivers/media/usb/uvc/uvc_driver.c | 77 +++++++++++++++++++++++++-------------
 drivers/media/usb/uvc/uvc_v4l2.c   |  4 --
 drivers/media/usb/uvc/uvcvideo.h   |  8 ++++
 3 files changed, 60 insertions(+), 29 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] uvcvideo: Factor out video device registration to a function

[Laurent Pinchart]

The function will then be used to register the video device for metadata
capture.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 66 +++++++++++++++++++++++---------------
 drivers/media/usb/uvc/uvcvideo.h   |  8 +++++
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index f77e31fcfc57..b832929d3382 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -24,6 +24,7 @@
 #include <asm/unaligned.h>
 
 #include <media/v4l2-common.h>
+#include <media/v4l2-ioctl.h>
 
 #include "uvcvideo.h"
 
@@ -1895,52 +1896,63 @@ static void uvc_unregister_video(struct uvc_device *dev)
 	kref_put(&dev->ref, uvc_delete);
 }
 
-static int uvc_register_video(struct uvc_device *dev,
-		struct uvc_streaming *stream)
+int uvc_register_video_device(struct uvc_device *dev,
+			      struct uvc_streaming *stream,
+			      struct video_device *vdev,
+			      struct uvc_video_queue *queue,
+			      enum v4l2_buf_type type,
+			      const struct v4l2_file_operations *fops,
+			      const struct v4l2_ioctl_ops *ioctl_ops)
 {
-	struct video_device *vdev = &stream->vdev;
 	int ret;
 
 	/* Initialize the video buffers queue. */
-	ret = uvc_queue_init(&stream->queue, stream->type, !uvc_no_drop_param);
+	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
 	if (ret)
 		return ret;
 
-	/* Initialize the streaming interface with default streaming
-	 * parameters.
-	 */
-	ret = uvc_video_init(stream);
-	if (ret < 0) {
-		uvc_printk(KERN_ERR, "Failed to initialize the device "
-			"(%d).\n", ret);
-		return ret;
-	}
-
-	uvc_debugfs_init_stream(stream);
-
 	/* Register the device with V4L. */
 
-	/* We already hold a reference to dev->udev. The video device will be
+	/*
+	 * We already hold a reference to dev->udev. The video device will be
 	 * unregistered before the reference is released, so we don't need to
 	 * get another one.
 	 */
 	vdev->v4l2_dev = &dev->vdev;
-	vdev->fops = &uvc_fops;
-	vdev->ioctl_ops = &uvc_ioctl_ops;
+	vdev->fops = fops;
+	vdev->ioctl_ops = ioctl_ops;
 	vdev->release = uvc_release;
 	vdev->prio = &stream->chain->prio;
-	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+	if (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
-	 * uvc_v4l2_open might race us.
+	/*
+	 * Set the driver data before calling video_register_device, otherwise
+	 * the file open() handler might race us.
 	 */
 	video_set_drvdata(vdev, stream);
 
 	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
 	if (ret < 0) {
-		uvc_printk(KERN_ERR, "Failed to register video device (%d).\n",
+		uvc_printk(KERN_ERR, "Failed to register %s device (%d).\n",
+			   v4l2_type_names[type], ret);
+		return ret;
+	}
+
+	kref_get(&dev->ref);
+	return 0;
+}
+
+static int uvc_register_video(struct uvc_device *dev,
+		struct uvc_streaming *stream)
+{
+	int ret;
+
+	/* Initialize the streaming interface with default parameters. */
+	ret = uvc_video_init(stream);
+	if (ret < 0) {
+		uvc_printk(KERN_ERR, "Failed to initialize the device (%d).\n",
 			   ret);
 		return ret;
 	}
@@ -1950,8 +1962,12 @@ static int uvc_register_video(struct uvc_device *dev,
 	else
 		stream->chain->caps |= V4L2_CAP_VIDEO_OUTPUT;
 
-	kref_get(&dev->ref);
-	return 0;
+	uvc_debugfs_init_stream(stream);
+
+	/* Register the device with V4L. */
+	return uvc_register_video_device(dev, stream, &stream->vdev,
+					 &stream->queue, stream->type,
+					 &uvc_fops, &uvc_ioctl_ops);
 }
 
 /*
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index e2169caefe9e..f5e8a9b17296 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -716,6 +716,14 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
 			    struct vb2_v4l2_buffer *vbuf,
 			    struct uvc_buffer *buf);
 
+int uvc_register_video_device(struct uvc_device *dev,
+			      struct uvc_streaming *stream,
+			      struct video_device *vdev,
+			      struct uvc_video_queue *queue,
+			      enum v4l2_buf_type type,
+			      const struct v4l2_file_operations *fops,
+			      const struct v4l2_ioctl_ops *ioctl_ops);
+
 /* Status */
 extern int uvc_status_init(struct uvc_device *dev);
 extern void uvc_status_unregister(struct uvc_device *dev);
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] uvcvideo: Factor out video device registration to a function

[Guennadi Liakhovetski]

Hi Laurent,

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> The function will then be used to register the video device for metadata
> capture.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 66 +++++++++++++++++++++++---------------
>  drivers/media/usb/uvc/uvcvideo.h   |  8 +++++
>  2 files changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index f77e31fcfc57..b832929d3382 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -24,6 +24,7 @@
>  #include <asm/unaligned.h>
>  
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-ioctl.h>
>  
>  #include "uvcvideo.h"
>  
> @@ -1895,52 +1896,63 @@ static void uvc_unregister_video(struct uvc_device *dev)

[snip]

>  	vdev->release = uvc_release;
>  	vdev->prio = &stream->chain->prio;
> -	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>  		vdev->vfl_dir = VFL_DIR_TX;

Why isn't .vfl_dir set for other stream types? Are you jusut relying on 
VFL_DIR_RX == 0? I'd use a switch (type) here which then would be extended 
in your next patch with .device_caps fields.

Thanks
Guennadi

Re: [PATCH 1/2] uvcvideo: Factor out video device registration to a function

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 5 December 2017 11:14:18 EET Guennadi Liakhovetski wrote:
> On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > The function will then be used to register the video device for metadata
> > capture.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/usb/uvc/uvc_driver.c | 66 ++++++++++++++++++++-------------
> >  drivers/media/usb/uvc/uvcvideo.h   |  8 +++++
> >  2 files changed, 49 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index f77e31fcfc57..b832929d3382
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -24,6 +24,7 @@
> >  #include <asm/unaligned.h>
> >  
> >  #include <media/v4l2-common.h>
> > +#include <media/v4l2-ioctl.h>
> > 
> >  #include "uvcvideo.h"
> > @@ -1895,52 +1896,63 @@ static void uvc_unregister_video(struct uvc_device
> > *dev)
> 
> [snip]
> 
> >  	vdev->release = uvc_release;
> >  	vdev->prio = &stream->chain->prio;
> > 
> > -	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > +	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> >  		vdev->vfl_dir = VFL_DIR_TX;
> 
> Why isn't .vfl_dir set for other stream types? Are you jusut relying on
> VFL_DIR_RX == 0? I'd use a switch (type) here which then would be extended
> in your next patch with .device_caps fields.

Yes, and I agree it's not right. How about

	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		vdev->vfl_dir = VFL_DIR_TX;
	else
 		vdev->vfl_dir = VFL_DIR_RX;

Then it won't need to be touched when adding metadata support.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] uvcvideo: Factor out video device registration to a function

[Guennadi Liakhovetski]

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday, 5 December 2017 11:14:18 EET Guennadi Liakhovetski wrote:
> > On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > > The function will then be used to register the video device for metadata
> > > capture.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/usb/uvc/uvc_driver.c | 66 ++++++++++++++++++++-------------
> > >  drivers/media/usb/uvc/uvcvideo.h   |  8 +++++
> > >  2 files changed, 49 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > > b/drivers/media/usb/uvc/uvc_driver.c index f77e31fcfc57..b832929d3382
> > > 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -24,6 +24,7 @@
> > >  #include <asm/unaligned.h>
> > >  
> > >  #include <media/v4l2-common.h>
> > > +#include <media/v4l2-ioctl.h>
> > > 
> > >  #include "uvcvideo.h"
> > > @@ -1895,52 +1896,63 @@ static void uvc_unregister_video(struct uvc_device
> > > *dev)
> > 
> > [snip]
> > 
> > >  	vdev->release = uvc_release;
> > >  	vdev->prio = &stream->chain->prio;
> > > 
> > > -	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > > +	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > >  		vdev->vfl_dir = VFL_DIR_TX;
> > 
> > Why isn't .vfl_dir set for other stream types? Are you jusut relying on
> > VFL_DIR_RX == 0? I'd use a switch (type) here which then would be extended
> > in your next patch with .device_caps fields.
> 
> Yes, and I agree it's not right. How about
> 
> 	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>  		vdev->vfl_dir = VFL_DIR_TX;
> 	else
>  		vdev->vfl_dir = VFL_DIR_RX;
> 
> Then it won't need to be touched when adding metadata support.

Well, I personally find it a bit less than elegant to have

	if (x = a)
		...
	else
		...

	switch (x) {
	case a:
		...
	...
	}

Also, if I did end up having these two separate, I'd also rather use the 
?: operator for the first one, but in the end it's up to you, I won't 
fight for that :-)

Thanks
Guennadi

[PATCH 2/2] uvcvideo: Report V4L2 device caps through the video_device structure

[Laurent Pinchart]

The V4L2 core populates the struct v4l2_capability device_caps field
from the same field in video_device. There's no need to handle that
manually in the driver.

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index b832929d3382..0ebdcc8b4ff6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1925,6 +1925,17 @@ int uvc_register_video_device(struct uvc_device *dev,
 	vdev->prio = &stream->chain->prio;
 	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		vdev->vfl_dir = VFL_DIR_TX;
+
+	switch (type) {
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
+	default:
+		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+		break;
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
+		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
+		break;
+	}
+
 	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 3e7e283a44a8..5e0323982577 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -568,10 +568,6 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
 	usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
 	cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
 			  | chain->caps;
-	if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
-	else
-		cap->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
 
 	return 0;
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/2] uvcvideo: Refactor code to ease metadata implementation

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for the patches. Please feel free to add either or both of

Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
Tested-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>

to both of the patches. Whereas in fact strictly speaking your current 
tree has updated improved versions of the patches, at least of the first 
of them - it now correctly handles the struct video_device::vfl_dir field, 
even thoough I'd still find merging that "if" with the following "switch" 
prettier ;-) So, strictly speaking you'd have to post those updated 
versions, in any case my approval tags refer to versions in your tree with 
commit IDs

53464c9f76da054ac3c291d27f348170d2a346c6
and
b6c5f10563c4ee8437cd9131bc3d389514456519

Thanks
Guennadi

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> This small patch series refactors the uvc_video_register() function to extract
> the code that you need into a new uvc_video_register_device() function. Please
> let me know if it can help.
> 
> Laurent Pinchart (2):
>   uvcvideo: Factor out video device registration to a function
>   uvcvideo: Report V4L2 device caps through the video_device structure
> 
>  drivers/media/usb/uvc/uvc_driver.c | 77 +++++++++++++++++++++++++-------------
>  drivers/media/usb/uvc/uvc_v4l2.c   |  4 --
>  drivers/media/usb/uvc/uvcvideo.h   |  8 ++++
>  3 files changed, 60 insertions(+), 29 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 0/2] uvcvideo: Refactor code to ease metadata implementation

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 12 December 2017 09:45:11 EET Guennadi Liakhovetski wrote:
> Hi Laurent,
> 
> Thanks for the patches. Please feel free to add either or both of
> 
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> Tested-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> 
> to both of the patches. Whereas in fact strictly speaking your current
> tree has updated improved versions of the patches, at least of the first
> of them - it now correctly handles the struct video_device::vfl_dir field,
> even thoough I'd still find merging that "if" with the following "switch"
> prettier ;-) So, strictly speaking you'd have to post those updated
> versions, in any case my approval tags refer to versions in your tree with
> commit IDs
> 
> 53464c9f76da054ac3c291d27f348170d2a346c6
> and
> b6c5f10563c4ee8437cd9131bc3d389514456519

Thank you. You're absolutely right, I've reposted the patches in a v2 with 
your tags included.

-- 
Regards,

Laurent Pinchart