Re: [RFC/PATCH] media: Add stk1160 new driver

Re: [RFC/PATCH] media: Add stk1160 new driver

[Hans Verkuil]

(Ezequiel, your original CC list was mangled, so I'm reposting this)

Thanks!

Here is a quick run through of the code.

Always test your driver using the v4l2-compliance test tool that it part of
v4l-utils! If it passes that, then your code will be in really good shape!

On Sat May 26 2012 18:41:00 Ezequiel Garcia wrote:
> This driver adds support for stk1160 usb bridge as used in some
> video/audio usb capture devices.
> It is a complete rewrite of staging/media/easycap driver and
> it's expected as a future replacement.
> 
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
> As of today testing has been performed using both vlc and mplayer
> on a gentoo machine, including hot unplug and on-the-fly standard
> change using a device with 1-cvs and 1-audio output.
> However more testing is underway with another device and/or another
> distribution.
> 
> Alsa sound support is a missing feature (work in progress).
> 
> As this is my first complete driver, the patch is (obviously) intended as RFC only.
> Any comments/reviews of *any* kind will be greatly appreciated.
> ---
>  drivers/media/video/Kconfig                 |    2 +
>  drivers/media/video/Makefile                |    1 +
>  drivers/media/video/stk1160/Kconfig         |   11 +
>  drivers/media/video/stk1160/Makefile        |    6 +
>  drivers/media/video/stk1160/stk1160-core.c  |  399 +++++++++++++
>  drivers/media/video/stk1160/stk1160-i2c.c   |  304 ++++++++++
>  drivers/media/video/stk1160/stk1160-reg.h   |   78 +++
>  drivers/media/video/stk1160/stk1160-v4l.c   |  846 +++++++++++++++++++++++++++
>  drivers/media/video/stk1160/stk1160-video.c |  506 ++++++++++++++++
>  drivers/media/video/stk1160/stk1160.h       |  183 ++++++
>  10 files changed, 2336 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/stk1160/Kconfig
>  create mode 100644 drivers/media/video/stk1160/Makefile
>  create mode 100644 drivers/media/video/stk1160/stk1160-core.c
>  create mode 100644 drivers/media/video/stk1160/stk1160-i2c.c
>  create mode 100644 drivers/media/video/stk1160/stk1160-reg.h
>  create mode 100644 drivers/media/video/stk1160/stk1160-v4l.c
>  create mode 100644 drivers/media/video/stk1160/stk1160-video.c
>  create mode 100644 drivers/media/video/stk1160/stk1160.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 99937c9..8d94d56 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -661,6 +661,8 @@ source "drivers/media/video/hdpvr/Kconfig"
>  
>  source "drivers/media/video/em28xx/Kconfig"
>  
> +source "drivers/media/video/stk1160/Kconfig"
> +
>  source "drivers/media/video/tlg2300/Kconfig"
>  
>  source "drivers/media/video/cx231xx/Kconfig"
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index d209de0..7698b25 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -125,6 +125,7 @@ obj-$(CONFIG_VIDEO_HEXIUM_ORION) += hexium_orion.o
>  obj-$(CONFIG_VIDEO_HEXIUM_GEMINI) += hexium_gemini.o
>  obj-$(CONFIG_STA2X11_VIP) += sta2x11_vip.o
>  obj-$(CONFIG_VIDEO_TIMBERDALE)	+= timblogiw.o
> +obj-$(CONFIG_VIDEO_STK1160) += stk1160/
>  
>  obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
>  obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
> diff --git a/drivers/media/video/stk1160/Kconfig b/drivers/media/video/stk1160/Kconfig
> new file mode 100644
> index 0000000..d481b8e
> --- /dev/null
> +++ b/drivers/media/video/stk1160/Kconfig
> @@ -0,0 +1,11 @@
> +config VIDEO_STK1160
> +	tristate "STK1160 USB video capture support"
> +	depends on VIDEO_DEV && I2C && EASYCAP!=m && EASYCAP!=y
> +	select VIDEOBUF2_VMALLOC
> +	select VIDEO_SAA711X if VIDEO_HELPER_CHIPS_AUTO
> +
> +	---help---
> +	  This is a video4linux driver for STK1160 based video capture devices
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called stk1160
> diff --git a/drivers/media/video/stk1160/Makefile b/drivers/media/video/stk1160/Makefile
> new file mode 100644
> index 0000000..30ca209
> --- /dev/null
> +++ b/drivers/media/video/stk1160/Makefile
> @@ -0,0 +1,6 @@
> +stk1160-y := stk1160-core.o stk1160-v4l.o stk1160-video.o stk1160-i2c.o
> +
> +obj-$(CONFIG_VIDEO_STK1160) += stk1160.o
> +
> +ccflags-y += -Wall
> +ccflags-y += -Idrivers/media/video
> diff --git a/drivers/media/video/stk1160/stk1160-core.c b/drivers/media/video/stk1160/stk1160-core.c
> new file mode 100644
> index 0000000..7fdee6b
> --- /dev/null

...

> +++ b/drivers/media/video/stk1160/stk1160-core.c
> +static int stk1160_probe(struct usb_interface *interface,
> +		const struct usb_device_id *id)
> +{
> +	int ifnum;
> +	int rc = 0;
> +
> +	unsigned int *alt_max_pkt_size;	/* array of wMaxPacketSize */
> +	struct usb_device *udev;
> +	struct stk1160 *dev = NULL;
> +
> +	ifnum = interface->altsetting[0].desc.bInterfaceNumber;
> +	udev = interface_to_usbdev(interface);
> +
> +	/* Don't register audio interfaces */
> +	if (interface->altsetting[0].desc.bInterfaceClass == USB_CLASS_AUDIO) {
> +		/* Why spit an error message? */
> +		stk1160_err("audio device (%04x:%04x): "
> +			"interface %i, class %i\n",
> +			le16_to_cpu(udev->descriptor.idVendor),
> +			le16_to_cpu(udev->descriptor.idProduct),
> +			ifnum,
> +			interface->altsetting[0].desc.bInterfaceClass);
> +		return -ENODEV;
> +	}
> +
> +	/* Alloc an array for all possible max_pkt_size */
> +	alt_max_pkt_size = kmalloc(sizeof(alt_max_pkt_size[0]) *
> +			interface->num_altsetting, GFP_KERNEL);
> +	if (alt_max_pkt_size == NULL)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Scan usb posibilities and populate alt_max_pkt_size array.
> +	 * Also, check if device speed is fast enough.
> +	 */
> +	rc = stk1160_scan_usb(interface, udev, alt_max_pkt_size);
> +	if (rc < 0) {
> +		kfree(alt_max_pkt_size);
> +		return rc;
> +	}
> +
> +	dev = kzalloc(sizeof(struct stk1160), GFP_KERNEL);
> +	if (dev == NULL) {
> +		kfree(alt_max_pkt_size);
> +		return -ENOMEM;
> +	}
> +
> +	dev->alt_max_pkt_size = alt_max_pkt_size;
> +	dev->udev = udev;
> +	dev->num_alt = interface->num_altsetting;
> +
> +	/* We save struct device for debug purposes only */
> +	dev->dev = &interface->dev;
> +
> +	usb_set_intfdata(interface, dev);
> +
> +	/* initialize videobuf2 stuff */
> +	rc = stk1160_vb2_setup(dev);
> +	if (rc < 0)
> +		goto free_err;
> +
> +	spin_lock_init(&dev->buf_lock);
> +
> +	/*
> +	 * We take the lock just before device registration,
> +	 * to prevent someone (probably udev) from opening us
> +	 * before we finish initialization
> +	 */
> +	mutex_init(&dev->mutex);
> +	mutex_lock(&dev->mutex);
> +
> +	rc = stk1160_video_register(dev);

It's usually better to register the video node as the very last thing
in probe(). That way when the node appears it's ready for udev to use.
No need to lock the mutex in that case.

> +	if (rc < 0)
> +		goto unreg_video;
> +
> +	rc = stk1160_i2c_register(dev);
> +	if (rc < 0)
> +		goto unlock_free;
> +
> +	/*
> +	 * To the best of my knowledge stk1160 boards only have
> +	 * saa7113, but it doesn't hurt to support them all.
> +	 */
> +	dev->sd_saa7115 = v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap,
> +		"saa7115_auto", 0, saa7113_addrs);
> +
> +	stk1160_info("driver ver %s successfully loaded\n",
> +		STK1160_VERSION);
> +
> +	/* i2c reset saa711x */
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_std, dev->norm);
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
> +			SAA7115_COMPOSITE0, 0, 0);
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
> +
> +	stk1160_reg_reset(dev);
> +
> +	mutex_unlock(&dev->mutex);
> +
> +	return 0;
> +
> +	/*
> +	 * Isn't this racy?
> +	 * If i2c_register fails, I unlock mutex, udev opens()
> +	 * but here I free device struct. So what happens
> +	 * with udev?
> +	 */
> +unreg_video:
> +	video_unregister_device(dev->vdev);
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +unlock_free:
> +	mutex_unlock(&dev->mutex);
> +free_err:
> +	kfree(alt_max_pkt_size);
> +	kfree(dev);
> +	return rc;
> +}
> +
> +static void stk1160_disconnect(struct usb_interface *interface)
> +{
> +	struct stk1160 *dev;
> +
> +	dev = usb_get_intfdata(interface);
> +	usb_set_intfdata(interface, NULL);
> +
> +	/*
> +	 * Wait until all current v4l2 operation are finished
> +	 * then deallocate resources
> +	 */
> +	mutex_lock(&dev->mutex);
> +
> +	/* Since saa7115 is no longer present, it's better to unregister it */
> +	v4l2_device_unregister_subdev(dev->sd_saa7115);
> +
> +	stk1160_stop_streaming(dev);
> +
> +	v4l2_device_disconnect(&dev->v4l2_dev);
> +
> +	/* This way current users can detect device is gone */
> +	dev->udev = NULL;
> +
> +	mutex_unlock(&dev->mutex);
> +
> +	stk1160_i2c_unregister(dev);
> +	stk1160_video_unregister(dev);

I recommend that you use the same approach as I did in media/radio/dsbr100.c:
use the v4l2_dev->release callback to handle the final cleanup. That is a safe
method that does all the refcounting for you.

...

> diff --git a/drivers/media/video/stk1160/stk1160-v4l.c b/drivers/media/video/stk1160/stk1160-v4l.c
> new file mode 100644
> index 0000000..a7a012b
> --- /dev/null
> +++ b/drivers/media/video/stk1160/stk1160-v4l.c

...

> +static int stk1160_open(struct file *filp)
> +{
> +	struct stk1160 *dev = video_drvdata(filp);
> +
> +	dev->users++;

Why the users field? You shouldn't need it.

> +
> +	stk1160_info("opened: users=%d\n", dev->users);
> +
> +	return v4l2_fh_open(filp);
> +}
> +
> +static int stk1160_close(struct file *file)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +
> +	dev->users--;
> +
> +	stk1160_info("closed: users=%d\n", dev->users);
> +
> +	/*
> +	 * If this is the last fh remaining open, then we
> +	 * stop streaming and free/dequeue all buffers.
> +	 * This prevents device from streaming without
> +	 * any opened filehandles.
> +	 */
> +	if (v4l2_fh_is_singular_file(file))
> +		vb2_queue_release(&dev->vb_vidq);

No. You should keep track of which filehandle started streaming (actually
the filehandle that called REQBUFS is the owner of the queue) and release
the queue when that particular filehandle is closed (or it calls REQBUFS
with a count of 0).

> +
> +	return v4l2_fh_release(file);
> +}
> +
> +static int stk1160_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	int rc;
> +
> +	stk1160_dbg("vma=0x%08lx\n", (unsigned long)vma);
> +
> +	rc = vb2_mmap(&dev->vb_vidq, vma);
> +
> +	stk1160_dbg("vma start=0x%08lx, size=%ld (%d)\n",
> +		(unsigned long)vma->vm_start,
> +		(unsigned long)vma->vm_end - (unsigned long)vma->vm_start,
> +		rc);
> +	return rc;
> +}
> +
> +static struct v4l2_file_operations stk1160_fops = {
> +	.owner = THIS_MODULE,
> +	.open = stk1160_open,
> +	.release = stk1160_close,
> +	.read = stk1160_read,
> +	.poll = stk1160_poll,
> +	.unlocked_ioctl = video_ioctl2,
> +	.mmap = stk1160_mmap,
> +};
> +
> +/*
> + * IOCTL vidioc handling
> + */
> +static int vidioc_reqbufs(struct file *file, void *priv,
> +			  struct v4l2_requestbuffers *p)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	return vb2_reqbufs(&dev->vb_vidq, p);
> +}
> +
> +static int vidioc_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	return vb2_querybuf(&dev->vb_vidq, p);
> +}
> +
> +static int vidioc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	return vb2_qbuf(&dev->vb_vidq, p);
> +}
> +
> +static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	return vb2_dqbuf(&dev->vb_vidq, p, file->f_flags & O_NONBLOCK);
> +}
> +
> +static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	return vb2_streamon(&dev->vb_vidq, i);
> +}
> +
> +static int vidioc_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	return vb2_streamoff(&dev->vb_vidq, i);
> +}
> +
> +static int vidioc_querycap(struct file *file,
> +		void *priv, struct v4l2_capability *dev)
> +{
> +	strcpy(dev->driver, "stk1160");
> +	strcpy(dev->card, "stk1160");
> +	dev->version = STK1160_VERSION_NUM;
> +	dev->capabilities =
> +		V4L2_CAP_VIDEO_CAPTURE |
> +		V4L2_CAP_STREAMING |
> +		V4L2_CAP_READWRITE;
> +	return 0;
> +}
> +
> +static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
> +		struct v4l2_fmtdesc *f)
> +{
> +	if (f->index != 0)
> +		return -EINVAL;
> +
> +	strlcpy(f->description, format[f->index].name, sizeof(f->description));
> +	f->pixelformat = format[f->index].fourcc;
> +	return 0;
> +}
> +
> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> +					struct v4l2_format *f)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +
> +	f->fmt.pix.width = dev->width;
> +	f->fmt.pix.height = dev->height;
> +	f->fmt.pix.field = V4L2_FIELD_INTERLACED;
> +	f->fmt.pix.pixelformat = dev->fmt->fourcc;
> +	f->fmt.pix.bytesperline = dev->width << 1;
> +	f->fmt.pix.sizeimage = dev->height * f->fmt.pix.bytesperline;
> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> +
> +	return 0;
> +}
> +
> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> +			struct v4l2_format *f)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +
> +	if (f->fmt.pix.pixelformat != format[0].fourcc) {
> +		stk1160_err("fourcc format 0x%08x invalid\n",
> +			f->fmt.pix.pixelformat);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * User can't choose size at his own will,
> +	 * so we just return him the current size chosen
> +	 * at standard selection.
> +	 * TODO: Implement frame scaling?
> +	 */
> +
> +	f->fmt.pix.width = dev->width;
> +	f->fmt.pix.height = dev->height;
> +	f->fmt.pix.field = V4L2_FIELD_INTERLACED;
> +	f->fmt.pix.bytesperline = dev->width << 1;
> +	f->fmt.pix.sizeimage = dev->height * f->fmt.pix.bytesperline;
> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> +
> +	return 0;
> +}
> +
> +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> +					struct v4l2_format *f)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	struct vb2_queue *q = &dev->vb_vidq;
> +
> +	int ret = vidioc_try_fmt_vid_cap(file, priv, f);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (vb2_is_streaming(q)) {
> +		stk1160_err("device busy\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, video, querystd, norm);
> +	return 0;
> +}
> +
> +static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id *norm)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	struct vb2_queue *q = &dev->vb_vidq;
> +
> +	if (vb2_is_streaming(q)) {
> +		stk1160_err("device busy\n");
> +		return -EBUSY;
> +	}
> +
> +	/* Check device presence */
> +	if (!dev->udev)
> +		return -ENODEV;
> +
> +	/* This is taken from saa7115 video decoder */
> +	if (dev->norm & V4L2_STD_525_60) {
> +		dev->width = 720;
> +		dev->height = 480;
> +	} else if (dev->norm & V4L2_STD_625_50) {
> +		dev->width = 720;
> +		dev->height = 576;
> +	} else {
> +		stk1160_err("invalid standard\n");
> +		return -EINVAL;
> +	}
> +
> +	/* We need to set this now, before we call stk1160_set_std() */
> +	dev->norm = *norm;
> +
> +	stk1160_set_std(dev);
> +
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_std, dev->norm);
> +
> +	return 0;
> +}
> +
> +/* FIXME: Extend support for other inputs */
> +static int vidioc_enum_input(struct file *file, void *priv,
> +				struct v4l2_input *i)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +
> +	if (i->index != 0)
> +		return -EINVAL;
> +
> +	strcpy(i->name, "Composite1");
> +	i->type = V4L2_INPUT_TYPE_CAMERA;
> +	i->std = dev->vdev->tvnorms;
> +	return 0;
> +}
> +
> +static int vidioc_g_input(struct file *file, void *priv, unsigned int *i)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	*i = dev->ctl_input;
> +	return 0;
> +}
> +
> +/* FIXME: Extend support for other inputs */
> +static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	if (i != 0)
> +		return -EINVAL;
> +	dev->ctl_input = i;
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
> +			SAA7115_COMPOSITE0, 0, 0);
> +	return 0;
> +}
> +
> +static int vidioc_queryctrl(struct file *file, void *priv,
> +				struct v4l2_queryctrl *qc)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	int id = qc->id;
> +
> +	memset(qc, 0, sizeof(*qc));
> +	qc->id = id;
> +
> +	/* enumerate V4L2 device controls */
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core, queryctrl, qc);
> +	if (qc->type)
> +		return 0;
> +	else
> +		return -EINVAL;
> +}

Use the control framework. I won't accept any new drivers that do not use it.

In your case it is very simple: create a v4l2_ctrl_handler, initialize it and
let v4l2_dev->ctrl_handler point to it. When the subdev is added it will add its
controls to your handler.

> +
> +static int vidioc_g_ctrl(struct file *file, void *priv,
> +				struct v4l2_control *ctrl)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core, g_ctrl, ctrl);
> +	return 0;
> +}
> +
> +static int vidioc_s_ctrl(struct file *file, void *priv,
> +				struct v4l2_control *ctrl)
> +{
> +	struct stk1160 *dev = video_drvdata(file);
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_ctrl, ctrl);
> +	return 0;
> +}
> +
> +static int vidioc_enum_framesizes(struct file *file, void *fh,
> +				 struct v4l2_frmsizeenum *fsize)
> +{
> +	/* TODO: Is this needed? */
> +	return -EINVAL;
> +}
> +
> +static int vidioc_enum_frameintervals(struct file *file, void *fh,
> +				  struct v4l2_frmivalenum *fival)
> +{
> +	/* TODO: Is this needed? */
> +	return -EINVAL;
> +}
> +
> +static const struct v4l2_ioctl_ops stk1160_ioctl_ops = {
> +	.vidioc_querycap      = vidioc_querycap,
> +	.vidioc_enum_fmt_vid_cap  = vidioc_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap     = vidioc_g_fmt_vid_cap,
> +	.vidioc_try_fmt_vid_cap   = vidioc_try_fmt_vid_cap,
> +	.vidioc_s_fmt_vid_cap     = vidioc_s_fmt_vid_cap,
> +	.vidioc_reqbufs       = vidioc_reqbufs,
> +	.vidioc_querybuf      = vidioc_querybuf,
> +	.vidioc_qbuf          = vidioc_qbuf,
> +	.vidioc_dqbuf         = vidioc_dqbuf,
> +	.vidioc_querystd      = vidioc_querystd,
> +	.vidioc_g_std         = NULL, /* don't worry v4l handles this */
> +	.vidioc_s_std         = vidioc_s_std,
> +	.vidioc_enum_input    = vidioc_enum_input,
> +	.vidioc_g_input       = vidioc_g_input,
> +	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_queryctrl     = vidioc_queryctrl,
> +	.vidioc_g_ctrl        = vidioc_g_ctrl,
> +	.vidioc_s_ctrl        = vidioc_s_ctrl,
> +	.vidioc_enum_framesizes = vidioc_enum_framesizes,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_streamon      = vidioc_streamon,
> +	.vidioc_streamoff     = vidioc_streamoff,
> +};
> +
> +/********************************************************************/
> +
> +/*
> + * Videobuf2 operations
> + */
> +static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *v4l_fmt,
> +				unsigned int *nbuffers, unsigned int *nplanes,
> +				unsigned int sizes[], void *alloc_ctxs[])
> +{
> +	struct stk1160 *dev = vb2_get_drv_priv(vq);
> +	unsigned long size;
> +
> +	size = dev->width * dev->height * 2;
> +
> +	/*
> +	 * Here we can change the number of buffers being requested.
> +	 * For instance, we could set a minimum and a maximum,
> +	 * like this:
> +	 */
> +	if (*nbuffers < STK1160_MIN_VIDEO_BUFFERS)
> +		*nbuffers = STK1160_MIN_VIDEO_BUFFERS;
> +	else if (*nbuffers > STK1160_MAX_VIDEO_BUFFERS)
> +		*nbuffers = STK1160_MAX_VIDEO_BUFFERS;
> +
> +	/* This means a packed colorformat */
> +	*nplanes = 1;
> +
> +	sizes[0] = size;
> +
> +	/*
> +	 * videobuf2-vmalloc allocator is context-less so no need to set
> +	 * alloc_ctxs array.
> +	 */
> +
> +	if (v4l_fmt) {
> +		stk1160_info("selected format %d (%d)\n",
> +			v4l_fmt->fmt.pix.pixelformat,
> +			dev->fmt->fourcc);
> +	}
> +
> +	stk1160_info("%s: buffer count %d, each %ld bytes\n",
> +			__func__, *nbuffers, size);
> +
> +	return 0;
> +}
> +
> +static int buffer_init(struct vb2_buffer *vb)
> +{
> +	return 0;
> +}
> +
> +static int buffer_prepare(struct vb2_buffer *vb)
> +{
> +	struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
> +	struct stk1160_buffer *buf =
> +			container_of(vb, struct stk1160_buffer, vb);
> +
> +	/* If the device is disconnected, reject the buffer */
> +	if (!dev->udev)
> +		return -ENODEV;
> +
> +	buf->mem = vb2_plane_vaddr(vb, 0);
> +	buf->length = vb2_plane_size(vb, 0);
> +	buf->bytesused = 0;
> +	buf->pos = 0;
> +
> +	return 0;
> +}
> +
> +static int buffer_finish(struct vb2_buffer *vb)
> +{
> +	return 0;
> +}
> +
> +static void buffer_cleanup(struct vb2_buffer *vb)
> +{
> +}
> +
> +static void buffer_queue(struct vb2_buffer *vb)
> +{
> +	unsigned long flags = 0;
> +	struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
> +	struct stk1160_buffer *buf =
> +		container_of(vb, struct stk1160_buffer, vb);
> +
> +	spin_lock_irqsave(&dev->buf_lock, flags);
> +	if (!dev->udev) {
> +		/*
> +		 * If the device is disconnected return the buffer to userspace
> +		 * directly. The next QBUF call will fail with -ENODEV.
> +		 */
> +		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> +	} else {
> +		list_add_tail(&buf->list, &dev->avail_bufs);
> +	}
> +	spin_unlock_irqrestore(&dev->buf_lock, flags);
> +}
> +
> +static int start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> +	struct stk1160 *dev = vb2_get_drv_priv(vq);
> +	return stk1160_start_streaming(dev);
> +}
> +
> +/* abort streaming and wait for last buffer */
> +static int stop_streaming(struct vb2_queue *vq)
> +{
> +	struct stk1160 *dev = vb2_get_drv_priv(vq);
> +	return stk1160_stop_streaming(dev);
> +}
> +
> +static void stk1160_lock(struct vb2_queue *vq)
> +{
> +	struct stk1160 *dev = vb2_get_drv_priv(vq);
> +	mutex_lock(&dev->mutex);
> +}
> +
> +static void stk1160_unlock(struct vb2_queue *vq)
> +{
> +	struct stk1160 *dev = vb2_get_drv_priv(vq);
> +	mutex_unlock(&dev->mutex);
> +}
> +
> +static void stk1160_release(struct video_device *vd)
> +{
> +	struct stk1160 *dev = video_get_drvdata(vd);
> +
> +	stk1160_info("releasing all resources\n");
> +
> +	video_set_drvdata(vd, NULL);
> +	video_device_release(vd);
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +
> +	kfree(dev->alt_max_pkt_size);
> +	kfree(dev);
> +}
> +
> +static struct vb2_ops stk1160_video_qops = {
> +	.queue_setup		= queue_setup,
> +	.buf_init		= buffer_init,
> +	.buf_prepare		= buffer_prepare,
> +	.buf_finish		= buffer_finish,
> +	.buf_cleanup		= buffer_cleanup,
> +	.buf_queue		= buffer_queue,
> +	.start_streaming	= start_streaming,
> +	.stop_streaming		= stop_streaming,
> +	.wait_prepare		= stk1160_unlock,
> +	.wait_finish		= stk1160_lock,
> +};
> +
> +static struct video_device v4l_template = {
> +	.name = "stk1160",
> +	.tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50,
> +	.current_norm = V4L2_STD_NTSC,
> +	.fops = &stk1160_fops,
> +	.ioctl_ops = &stk1160_ioctl_ops,
> +	.release = stk1160_release,
> +};
> +
> +/********************************************************************/
> +
> +int stk1160_vb2_setup(struct stk1160 *dev)
> +{
> +	int rc;
> +	struct vb2_queue *q;
> +
> +	q = &dev->vb_vidq;
> +	memset(q, 0, sizeof(dev->vb_vidq));
> +	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR;
> +	q->drv_priv = dev;
> +	q->buf_struct_size = sizeof(struct stk1160_buffer);
> +	q->ops = &stk1160_video_qops;
> +	q->mem_ops = &vb2_vmalloc_memops;
> +
> +	rc = vb2_queue_init(q);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* initialize video dma queue */
> +	INIT_LIST_HEAD(&dev->avail_bufs);
> +
> +	return 0;
> +}
> +
> +int stk1160_video_register(struct stk1160 *dev)
> +{
> +	int rc = -ENOMEM;
> +
> +	/* There is no need to set the name if we give a device struct */
> +	rc = v4l2_device_register(dev->dev, &dev->v4l2_dev);
> +	if (rc) {
> +		stk1160_err("v4l2_device_register failed (%d)\n", rc);
> +		return -ENOMEM;
> +	}
> +
> +	dev->vdev = video_device_alloc();

I recommend that vdev is not a pointer but the struct video_device itself
embedded in the struct stk1160. The release callback can be video_device_release_empty
in that case. stk1160_release should be set as the release callback of v4l2_dev.

> +	if (!dev->vdev) {
> +		stk1160_err("video_device_alloc failed (%d)\n", rc);
> +		goto unreg;
> +	}
> +
> +	/* Initialize video_device with a template structure */
> +	*dev->vdev = v4l_template;
> +	dev->vdev->debug = vidioc_debug;
> +
> +	/*
> +	 * Provide a mutex to v4l2 core.
> +	 * It will be used to protect all fops and v4l2 ioctls.
> +	 */
> +	dev->vdev->lock = &dev->mutex;

Please note: we made a change for 3.5 where this lock is only used for
ioctls, not for other file operations. You will have to review your code
whether or not to take the lock explicitly for those other file operations.

> +
> +	/* This will be used to set video_device parent */
> +	dev->vdev->v4l2_dev = &dev->v4l2_dev;
> +
> +	/* It is safer to set this before registering with v4l2 */
> +	video_set_drvdata(dev->vdev, dev);
> +
> +	rc = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);

Do this as the last thing in this function.

> +	if (rc < 0) {
> +		stk1160_err("video_register_device failed (%d)\n", rc);
> +		goto release;
> +	}
> +
> +	v4l2_info(&dev->v4l2_dev, "V4L2 device registered as %s\n",
> +		  video_device_node_name(dev->vdev));
> +
> +	/* I say PAL_Nc is default, could be anyother */
> +	dev->width = 720;
> +	dev->height = 480;
> +
> +	/* set default norm and input */
> +	dev->norm = V4L2_STD_NTSC;
> +	dev->ctl_input = 0;
> +
> +	/* set default format */
> +	dev->fmt = &format[0];
> +	stk1160_set_std(dev);
> +
> +	return 0;
> +
> +release:
> +	video_device_release(dev->vdev);
> +unreg:
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +
> +	return rc;
> +}
> +
> +void stk1160_video_unregister(struct stk1160 *dev)
> +{
> +	v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
> +		video_device_node_name(dev->vdev));
> +
> +	/*
> +	 * This will put video_device, thus calling stk1160_release
> +	 * if it's the last reference. Otherwise,
> +	 * release is posponed until there are no users left.
> +	 */
> +	video_unregister_device(dev->vdev);
> +}

Regards,

	Hans

Re: [RFC/PATCH] media: Add stk1160 new driver

[Ezequiel Garcia]

Hi Hans,

Thanks for your review (I'm a bit amazed at how fast you went through
the code :).
I'll address your excellent comments soon.

I'm still unsure about a numbre of things. Two of them:

1. It seems to mee tracing is not too nice and
I wasn't really sure how to handle it: dev_xxx, pr_xxx, v4l2_xxx.
What's the current trend?

2. The original driver allowed to set frame size, but it seemed to me that
could be done at userspace.

Hence, my implementation says:

V4L2_STD_625_50 is 720x756 and V4L2_STD_525_60 is 720x480.

(This is related to the way the video decoder saa711x also assuming that sizes.)
So userspace is supposed to get frame size, right after changing video standard
and handle buffer of appropriate size.

What do you think?


On Sat, May 26, 2012 at 2:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> (Ezequiel, your original CC list was mangled, so I'm reposting this)
>
Sorry about this :(
I'll check my git-send-mail config.

Thanks again,
Ezequiel.

Re: [RFC/PATCH] media: Add stk1160 new driver

[Hans Verkuil]

On Sat May 26 2012 20:38:00 Ezequiel Garcia wrote:
> Hi Hans,
> 
> Thanks for your review (I'm a bit amazed at how fast you went through
> the code :).
> I'll address your excellent comments soon.
> 
> I'm still unsure about a numbre of things. Two of them:
> 
> 1. It seems to mee tracing is not too nice and
> I wasn't really sure how to handle it: dev_xxx, pr_xxx, v4l2_xxx.
> What's the current trend?

dev_xxx with v4l2_xxx as a second option. If you don't need a driver/device
prefix, then pr_xxx is best.

It's messy and it's on my TODO list to simplify this. Unfortunately it's quite
low on that list.

> 2. The original driver allowed to set frame size, but it seemed to me that
> could be done at userspace.

You mean that the original driver allowed hardware resizing? It would be nice
to keep support for that. For a first version you don't have to, though.

> Hence, my implementation says:
> 
> V4L2_STD_625_50 is 720x756 and V4L2_STD_525_60 is 720x480.
> 
> (This is related to the way the video decoder saa711x also assuming that sizes.)
> So userspace is supposed to get frame size, right after changing video standard
> and handle buffer of appropriate size.
> 
> What do you think?

After the video standard is changed that framesize should always be set to the
default size for that standard. An application can then change it to another
size if there is a hardware scaler.

Regards,

	Hans

> 
> 
> On Sat, May 26, 2012 at 2:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > (Ezequiel, your original CC list was mangled, so I'm reposting this)
> >
> Sorry about this :(
> I'll check my git-send-mail config.
> 
> Thanks again,
> Ezequiel.
> 

Re: [RFC/PATCH] media: Add stk1160 new driver

[Sylwester Nawrocki]

Hi Ezequiel,

On 05/26/2012 07:50 PM, Hans Verkuil wrote:
>> +/*
>> + * IOCTL vidioc handling
>> + */
>> +static int vidioc_reqbufs(struct file *file, void *priv,
>> +			  struct v4l2_requestbuffers *p)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +	return vb2_reqbufs(&dev->vb_vidq, p);
>> +}
>> +
>> +static int vidioc_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +	return vb2_querybuf(&dev->vb_vidq, p);
>> +}
>> +
>> +static int vidioc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +	return vb2_qbuf(&dev->vb_vidq, p);
>> +}
>> +
>> +static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +	return vb2_dqbuf(&dev->vb_vidq, p, file->f_flags&  O_NONBLOCK);
>> +}
>> +
>> +static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +	return vb2_streamon(&dev->vb_vidq, i);
>> +}
>> +
>> +static int vidioc_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +	return vb2_streamoff(&dev->vb_vidq, i);
>> +}
>> +
>> +static int vidioc_querycap(struct file *file,
>> +		void *priv, struct v4l2_capability *dev)
>> +{
>> +	strcpy(dev->driver, "stk1160");
>> +	strcpy(dev->card, "stk1160");
>> +	dev->version = STK1160_VERSION_NUM;

You can drop this line, it's overwritten with KERNEL_VERSION in v4l2-ioctl.c.
Also I could imagine there might be better names, than "dev", for capabilities.

>> +	dev->capabilities =
>> +		V4L2_CAP_VIDEO_CAPTURE |
>> +		V4L2_CAP_STREAMING |
>> +		V4L2_CAP_READWRITE;
>> +	return 0;
>> +}
>> +
>> +static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
>> +		struct v4l2_fmtdesc *f)
>> +{
>> +	if (f->index != 0)
>> +		return -EINVAL;
>> +
>> +	strlcpy(f->description, format[f->index].name, sizeof(f->description));
>> +	f->pixelformat = format[f->index].fourcc;
>> +	return 0;
>> +}
>> +
>> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>> +					struct v4l2_format *f)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +
>> +	f->fmt.pix.width = dev->width;
>> +	f->fmt.pix.height = dev->height;
>> +	f->fmt.pix.field = V4L2_FIELD_INTERLACED;
>> +	f->fmt.pix.pixelformat = dev->fmt->fourcc;
>> +	f->fmt.pix.bytesperline = dev->width<<  1;
                                  ^^^^^^^^^^^^^^^^
Probably better to just write: dev->width * 2.

>> +	f->fmt.pix.sizeimage = dev->height * f->fmt.pix.bytesperline;
>> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +
>> +	return 0;
>> +}
>> +
>> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
>> +			struct v4l2_format *f)
>> +{
>> +	struct stk1160 *dev = video_drvdata(file);
>> +
>> +	if (f->fmt.pix.pixelformat != format[0].fourcc) {

I would rename format[] to stk1160_formats[], but it might just be me.

>> +		stk1160_err("fourcc format 0x%08x invalid\n",
>> +			f->fmt.pix.pixelformat);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * User can't choose size at his own will,
>> +	 * so we just return him the current size chosen
>> +	 * at standard selection.
>> +	 * TODO: Implement frame scaling?
>> +	 */
>> +
>> +	f->fmt.pix.width = dev->width;
>> +	f->fmt.pix.height = dev->height;
>> +	f->fmt.pix.field = V4L2_FIELD_INTERLACED;
>> +	f->fmt.pix.bytesperline = dev->width<<  1;

dev->width * 2;

>> +	f->fmt.pix.sizeimage = dev->height * f->fmt.pix.bytesperline;
>> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +
>> +	return 0;
>> +}
...
>> +/*
>> + * Videobuf2 operations
>> + */
>> +static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *v4l_fmt,
>> +				unsigned int *nbuffers, unsigned int *nplanes,
>> +				unsigned int sizes[], void *alloc_ctxs[])
>> +{
>> +	struct stk1160 *dev = vb2_get_drv_priv(vq);
>> +	unsigned long size;
>> +
>> +	size = dev->width * dev->height * 2;
>> +
>> +	/*
>> +	 * Here we can change the number of buffers being requested.
>> +	 * For instance, we could set a minimum and a maximum,
>> +	 * like this:
>> +	 */
>> +	if (*nbuffers<  STK1160_MIN_VIDEO_BUFFERS)
>> +		*nbuffers = STK1160_MIN_VIDEO_BUFFERS;
>> +	else if (*nbuffers>  STK1160_MAX_VIDEO_BUFFERS)
>> +		*nbuffers = STK1160_MAX_VIDEO_BUFFERS;

Could be also done as:

	*buffers = clamp(*buffers, STK1160_MIN_VIDEO_BUFFERS,
			 STK1160_MAX_VIDEO_BUFFERS);
>> +
>> +	/* This means a packed colorformat */
>> +	*nplanes = 1;
>> +
>> +	sizes[0] = size;
>> +
>> +	/*
>> +	 * videobuf2-vmalloc allocator is context-less so no need to set
>> +	 * alloc_ctxs array.
>> +	 */
>> +
>> +	if (v4l_fmt) {
>> +		stk1160_info("selected format %d (%d)\n",
>> +			v4l_fmt->fmt.pix.pixelformat,
>> +			dev->fmt->fourcc);
>> +	}

This log is not exactly right. You just ignore v4l_fmt, so it is not selected
anywhere. The *v4l_fmt argument is here for ioctls like VIDIOC_CREATE_BUFS,
and in case you wanted to support this ioctl you would need to do something 
like:
	pix = &v4l_fmt->fmt.pix;
	sizes[0] = pix->width * pix->height * 2;

Of course with any required sanity checks.

But this driver does not implement vidioc_create_bufs/vidioc_prepare_buf ioctl
callbacks are not, so the code is generally OK.

>> +
>> +	stk1160_info("%s: buffer count %d, each %ld bytes\n",
>> +			__func__, *nbuffers, size);
>> +
>> +	return 0;
>> +}
>> +
>> +static int buffer_init(struct vb2_buffer *vb)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int buffer_prepare(struct vb2_buffer *vb)
>> +{
>> +	struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct stk1160_buffer *buf =
>> +			container_of(vb, struct stk1160_buffer, vb);
>> +
>> +	/* If the device is disconnected, reject the buffer */
>> +	if (!dev->udev)
>> +		return -ENODEV;
>> +
>> +	buf->mem = vb2_plane_vaddr(vb, 0);
>> +	buf->length = vb2_plane_size(vb, 0);

Where do you check if the buffer you get from vb2 has correct parameters 
for your hardware (with current settings) to start writing data to it ?

It seems that this driver supports just one pixel format and resolution,
but still would be good to do such checks in buf_prepare().

And initialization of buf->mem, buf->length is better done from
buffer_queue() op. This way there would be no need to take dev->buf_lock 
spinlock also in buf_prepare() to protect the driver's buffer (queue),
which isn't done but it should in buffer_prepare() btw.

>> +	buf->bytesused = 0;
>> +	buf->pos = 0;
>> +
>> +	return 0;
>> +}
>> +
>> +static int buffer_finish(struct vb2_buffer *vb)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void buffer_cleanup(struct vb2_buffer *vb)
>> +{
>> +}

buf_init, buf_finish, buf_cleanup are optional callbacks, so if you 
don't use them they could be removed altogether.

>> +
>> +static void buffer_queue(struct vb2_buffer *vb)
>> +{
>> +	unsigned long flags = 0;
>> +	struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct stk1160_buffer *buf =
>> +		container_of(vb, struct stk1160_buffer, vb);
>> +
>> +	spin_lock_irqsave(&dev->buf_lock, flags);
>> +	if (!dev->udev) {
>> +		/*
>> +		 * If the device is disconnected return the buffer to userspace
>> +		 * directly. The next QBUF call will fail with -ENODEV.
>> +		 */
>> +		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>> +	} else {
>> +		list_add_tail(&buf->list,&dev->avail_bufs);
>> +	}
>> +	spin_unlock_irqrestore(&dev->buf_lock, flags);
>> +}

--
Regards,
Sylwester

Re: [RFC/PATCH] media: Add stk1160 new driver

[Ezequiel Garcia]

Hi Sylwester,

On Sat, May 26, 2012 at 4:31 PM, Sylwester Nawrocki <snjw23@gmail.com> wrote:
>
> You can drop this line, it's overwritten with KERNEL_VERSION in v4l2-ioctl.c.
> Also I could imagine there might be better names, than "dev", for capabilities.
>

Yes, indeed. Starting with "cap".

>>> +    dev->capabilities =
>>> +            V4L2_CAP_VIDEO_CAPTURE |
>>> +            V4L2_CAP_STREAMING |
>>> +            V4L2_CAP_READWRITE;
>>> +    return 0;
>>> +    f->fmt.pix.width = dev->width;
>>> +    f->fmt.pix.height = dev->height;
>>> +    f->fmt.pix.field = V4L2_FIELD_INTERLACED;
>>> +    f->fmt.pix.pixelformat = dev->fmt->fourcc;
>>> +    f->fmt.pix.bytesperline = dev->width<<  1;
>                                  ^^^^^^^^^^^^^^^^
> Probably better to just write: dev->width * 2.

I saw like that in a number of places and I tought it was "faster".
Guess, I was being truly naive.

>
> Could be also done as:
>
>        *buffers = clamp(*buffers, STK1160_MIN_VIDEO_BUFFERS,
>                         STK1160_MAX_VIDEO_BUFFERS);

Done.

>>> +    /*
>>> +     * videobuf2-vmalloc allocator is context-less so no need to set
>>> +     * alloc_ctxs array.
>>> +     */
>>> +
>>> +    if (v4l_fmt) {
>>> +            stk1160_info("selected format %d (%d)\n",
>>> +                    v4l_fmt->fmt.pix.pixelformat,
>>> +                    dev->fmt->fourcc);
>>> +    }
>
> This log is not exactly right. You just ignore v4l_fmt, so it is not selected
> anywhere. The *v4l_fmt argument is here for ioctls like VIDIOC_CREATE_BUFS,
> and in case you wanted to support this ioctl you would need to do something
> like:
>        pix = &v4l_fmt->fmt.pix;
>        sizes[0] = pix->width * pix->height * 2;
>
> Of course with any required sanity checks.
>
> But this driver does not implement vidioc_create_bufs/vidioc_prepare_buf ioctl
> callbacks are not, so the code is generally OK.

You're right, that was just legacy code from some early stages.

>>> +static int buffer_prepare(struct vb2_buffer *vb)
>>> +{
>>> +    struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>>> +    struct stk1160_buffer *buf =
>>> +                    container_of(vb, struct stk1160_buffer, vb);
>>> +
>>> +    /* If the device is disconnected, reject the buffer */
>>> +    if (!dev->udev)
>>> +            return -ENODEV;
>>> +
>>> +    buf->mem = vb2_plane_vaddr(vb, 0);
>>> +    buf->length = vb2_plane_size(vb, 0);
>
> Where do you check if the buffer you get from vb2 has correct parameters
> for your hardware (with current settings) to start writing data to it ?
>
> It seems that this driver supports just one pixel format and resolution,
> but still would be good to do such checks in buf_prepare().

You mean I should check buf->length?

>
> And initialization of buf->mem, buf->length is better done from
> buffer_queue() op. This way there would be no need to take dev->buf_lock
> spinlock also in buf_prepare() to protect the driver's buffer (queue),
> which isn't done but it should in buffer_prepare() btw.

Yes, I missed this spot.

>
>>> +    buf->bytesused = 0;
>>> +    buf->pos = 0;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int buffer_finish(struct vb2_buffer *vb)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +static void buffer_cleanup(struct vb2_buffer *vb)
>>> +{
>>> +}
>
> buf_init, buf_finish, buf_cleanup are optional callbacks, so if you
> don't use them they could be removed altogether.
>

Done.

>>> +
>>> +static void buffer_queue(struct vb2_buffer *vb)
>>> +{
>>> +    unsigned long flags = 0;
>>> +    struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>>> +    struct stk1160_buffer *buf =
>>> +            container_of(vb, struct stk1160_buffer, vb);
>>> +
>>> +    spin_lock_irqsave(&dev->buf_lock, flags);
>>> +    if (!dev->udev) {
>>> +            /*
>>> +             * If the device is disconnected return the buffer to userspace
>>> +             * directly. The next QBUF call will fail with -ENODEV.
>>> +             */
>>> +            vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>>> +    } else {
>>> +            list_add_tail(&buf->list,&dev->avail_bufs);
>>> +    }
>>> +    spin_unlock_irqrestore(&dev->buf_lock, flags);
>>> +}
>
> --
> Regards,
> Sylwester

Thanks,
Ezequiel.

Re: [RFC/PATCH] media: Add stk1160 new driver

[Sylwester Nawrocki]

Hi Ezequiel,

On 05/27/2012 11:32 PM, Ezequiel Garcia wrote:
>>>> +static int buffer_prepare(struct vb2_buffer *vb)
>>>> +{
>>>> +    struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>>>> +    struct stk1160_buffer *buf =
>>>> +                    container_of(vb, struct stk1160_buffer, vb);
>>>> +
>>>> +    /* If the device is disconnected, reject the buffer */
>>>> +    if (!dev->udev)
>>>> +            return -ENODEV;
>>>> +
>>>> +    buf->mem = vb2_plane_vaddr(vb, 0);
>>>> +    buf->length = vb2_plane_size(vb, 0);
>>
>> Where do you check if the buffer you get from vb2 has correct parameters
>> for your hardware (with current settings) to start writing data to it ?
>>
>> It seems that this driver supports just one pixel format and resolution,
>> but still would be good to do such checks in buf_prepare().
> 
> You mean I should check buf->length?

Yeah, you should validate the passed in vb2_buffer. Here is some example:
http://lxr.linux.no/#linux+v3.4/drivers/media/video/mx2_emmaprp.c#L715

IOW, you should compare value returned from vb2_plane_vaddr(vb, 0) during
streaming with value computed from pixel format and resolution -
configured with S_FMT. Rather than accepting any buffer passed to the driver
from user space.


Regards,
Sylwester

Re: [RFC/PATCH] media: Add stk1160 new driver

[Ezequiel Garcia]

Hi Hans,

On Sat, May 26, 2012 at 2:05 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Always test your driver using the v4l2-compliance test tool that it part of
> v4l-utils! If it passes that, then your code will be in really good shape!

You're right. I'll add v4l2-compliance output in the next patch.

>
> On Sat May 26 2012 18:41:00 Ezequiel Garcia wrote:
>> This driver adds support for stk1160 usb bridge as used in some
>> video/audio usb capture devices.
>> It is a complete rewrite of staging/media/easycap driver and
>> it's expected as a future replacement.
>>
>> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
>> ---
>> As of today testing has been performed using both vlc and mplayer
>> on a gentoo machine, including hot unplug and on-the-fly standard
>> change using a device with 1-cvs and 1-audio output.
>> However more testing is underway with another device and/or another
>> distribution.
>>
>> Alsa sound support is a missing feature (work in progress).
>>
>> As this is my first complete driver, the patch is (obviously) intended as RFC only.
>> Any comments/reviews of *any* kind will be greatly appreciated.
>> ---
>>  drivers/media/video/Kconfig                 |    2 +
>>  drivers/media/video/Makefile                |    1 +
>>  drivers/media/video/stk1160/Kconfig         |   11 +
>>  drivers/media/video/stk1160/Makefile        |    6 +
>>  drivers/media/video/stk1160/stk1160-core.c  |  399 +++++++++++++
>>  drivers/media/video/stk1160/stk1160-i2c.c   |  304 ++++++++++
>>  drivers/media/video/stk1160/stk1160-reg.h   |   78 +++
>>  drivers/media/video/stk1160/stk1160-v4l.c   |  846 +++++++++++++++++++++++++++
>>  drivers/media/video/stk1160/stk1160-video.c |  506 ++++++++++++++++
>>  drivers/media/video/stk1160/stk1160.h       |  183 ++++++
>>  10 files changed, 2336 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/media/video/stk1160/Kconfig
>>  create mode 100644 drivers/media/video/stk1160/Makefile
>>  create mode 100644 drivers/media/video/stk1160/stk1160-core.c
>>  create mode 100644 drivers/media/video/stk1160/stk1160-i2c.c
>>  create mode 100644 drivers/media/video/stk1160/stk1160-reg.h
>>  create mode 100644 drivers/media/video/stk1160/stk1160-v4l.c
>>  create mode 100644 drivers/media/video/stk1160/stk1160-video.c
>>  create mode 100644 drivers/media/video/stk1160/stk1160.h
>>
>> +
>> +     /*
>> +      * We take the lock just before device registration,
>> +      * to prevent someone (probably udev) from opening us
>> +      * before we finish initialization
>> +      */
>> +     mutex_init(&dev->mutex);
>> +     mutex_lock(&dev->mutex);
>> +
>> +     rc = stk1160_video_register(dev);
>
> It's usually better to register the video node as the very last thing
> in probe(). That way when the node appears it's ready for udev to use.
> No need to lock the mutex in that case.

Done.

>> +     /*
>> +      * Wait until all current v4l2 operation are finished
>> +      * then deallocate resources
>> +      */
>> +     mutex_lock(&dev->mutex);
>> +
>> +     /* Since saa7115 is no longer present, it's better to unregister it */
>> +     v4l2_device_unregister_subdev(dev->sd_saa7115);
>> +
>> +     stk1160_stop_streaming(dev);
>> +
>> +     v4l2_device_disconnect(&dev->v4l2_dev);
>> +
>> +     /* This way current users can detect device is gone */
>> +     dev->udev = NULL;
>> +
>> +     mutex_unlock(&dev->mutex);
>> +
>> +     stk1160_i2c_unregister(dev);
>> +     stk1160_video_unregister(dev);
>
> I recommend that you use the same approach as I did in media/radio/dsbr100.c:
> use the v4l2_dev->release callback to handle the final cleanup. That is a safe
> method that does all the refcounting for you.

I'm sorry but I don't really see the difference.
Right now I'm having video_device release function to handle the final cleanup.
I don't do the refcounting myself either. See my other comment below.

>
> ...
>
>> diff --git a/drivers/media/video/stk1160/stk1160-v4l.c b/drivers/media/video/stk1160/stk1160-v4l.c
>> new file mode 100644
>> index 0000000..a7a012b
>> --- /dev/null
>> +++ b/drivers/media/video/stk1160/stk1160-v4l.c
>
> ...
>
>> +static int stk1160_open(struct file *filp)
>> +{
>> +     struct stk1160 *dev = video_drvdata(filp);
>> +
>> +     dev->users++;
>
> Why the users field? You shouldn't need it.

Done.

>
>> +
>> +     stk1160_info("opened: users=%d\n", dev->users);
>> +
>> +     return v4l2_fh_open(filp);
>> +}
>> +
>> +static int stk1160_close(struct file *file)
>> +{
>> +     struct stk1160 *dev = video_drvdata(file);
>> +
>> +     dev->users--;
>> +
>> +     stk1160_info("closed: users=%d\n", dev->users);
>> +
>> +     /*
>> +      * If this is the last fh remaining open, then we
>> +      * stop streaming and free/dequeue all buffers.
>> +      * This prevents device from streaming without
>> +      * any opened filehandles.
>> +      */
>> +     if (v4l2_fh_is_singular_file(file))
>> +             vb2_queue_release(&dev->vb_vidq);
>
> No. You should keep track of which filehandle started streaming (actually
> the filehandle that called REQBUFS is the owner of the queue) and release
> the queue when that particular filehandle is closed (or it calls REQBUFS
> with a count of 0).

Ah. I gave much thought to this issue. I liked the way uvc managed
"privileged" handles,
but I wasn't sure if this was better or not
(I'm thinking on "policy vs mechanism" stuff, sounds a bit silly, right?).

So, I'll implement an owner filehandle, which is able to start/stop streaming.
Also set format? Or just start/stop streaming? How about the
non-owners filehandles?
Are they only capable of changing controls and such?
Is this documented in media api?


>> +
>> +static int vidioc_queryctrl(struct file *file, void *priv,
>> +                             struct v4l2_queryctrl *qc)
>> +{
>> +     struct stk1160 *dev = video_drvdata(file);
>> +     int id = qc->id;
>> +
>> +     memset(qc, 0, sizeof(*qc));
>> +     qc->id = id;
>> +
>> +     /* enumerate V4L2 device controls */
>> +     v4l2_device_call_all(&dev->v4l2_dev, 0, core, queryctrl, qc);
>> +     if (qc->type)
>> +             return 0;
>> +     else
>> +             return -EINVAL;
>> +}
>
> Use the control framework. I won't accept any new drivers that do not use it.
>
> In your case it is very simple: create a v4l2_ctrl_handler, initialize it and
> let v4l2_dev->ctrl_handler point to it. When the subdev is added it will add its
> controls to your handler.

Done.

>
>> +
>> +static int vidioc_g_ctrl(struct file *file, void *priv,
>> +                             struct v4l2_control *ctrl)
>> +{
>> +     struct stk1160 *dev = video_drvdata(file);
>> +     v4l2_device_call_all(&dev->v4l2_dev, 0, core, g_ctrl, ctrl);
>> +     return 0;
>> +}
>> +
>> +static int vidioc_s_ctrl(struct file *file, void *priv,
>> +                             struct v4l2_control *ctrl)
>> +{
>> +     struct stk1160 *dev = video_drvdata(file);
>> +     v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_ctrl, ctrl);
>> +     return 0;
>> +}
>> +
>> +static int vidioc_enum_framesizes(struct file *file, void *fh,
>> +                              struct v4l2_frmsizeenum *fsize)
>> +{
>> +     /* TODO: Is this needed? */
>> +     return -EINVAL;
>> +}
>> +
>> +static int vidioc_enum_frameintervals(struct file *file, void *fh,
>> +                               struct v4l2_frmivalenum *fival)
>> +{
>> +     /* TODO: Is this needed? */
>> +     return -EINVAL;
>> +}
>> +
>> +static const struct v4l2_ioctl_ops stk1160_ioctl_ops = {
>> +     .vidioc_querycap      = vidioc_querycap,
>> +     .vidioc_enum_fmt_vid_cap  = vidioc_enum_fmt_vid_cap,
>> +     .vidioc_g_fmt_vid_cap     = vidioc_g_fmt_vid_cap,
>> +     .vidioc_try_fmt_vid_cap   = vidioc_try_fmt_vid_cap,
>> +     .vidioc_s_fmt_vid_cap     = vidioc_s_fmt_vid_cap,
>> +     .vidioc_reqbufs       = vidioc_reqbufs,
>> +     .vidioc_querybuf      = vidioc_querybuf,
>> +     .vidioc_qbuf          = vidioc_qbuf,
>> +     .vidioc_dqbuf         = vidioc_dqbuf,
>> +     .vidioc_querystd      = vidioc_querystd,
>> +     .vidioc_g_std         = NULL, /* don't worry v4l handles this */
>> +     .vidioc_s_std         = vidioc_s_std,
>> +     .vidioc_enum_input    = vidioc_enum_input,
>> +     .vidioc_g_input       = vidioc_g_input,
>> +     .vidioc_s_input       = vidioc_s_input,
>> +     .vidioc_queryctrl     = vidioc_queryctrl,
>> +     .vidioc_g_ctrl        = vidioc_g_ctrl,
>> +     .vidioc_s_ctrl        = vidioc_s_ctrl,
>> +     .vidioc_enum_framesizes = vidioc_enum_framesizes,
>> +     .vidioc_enum_frameintervals = vidioc_enum_frameintervals,
>> +     .vidioc_streamon      = vidioc_streamon,
>> +     .vidioc_streamoff     = vidioc_streamoff,
>> +};
>> +
>> +/********************************************************************/
>> +
>> +/*
>> + * Videobuf2 operations
>> + */
>> +static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *v4l_fmt,
>> +                             unsigned int *nbuffers, unsigned int *nplanes,
>> +                             unsigned int sizes[], void *alloc_ctxs[])
>> +{
>> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
>> +     unsigned long size;
>> +
>> +     size = dev->width * dev->height * 2;
>> +
>> +     /*
>> +      * Here we can change the number of buffers being requested.
>> +      * For instance, we could set a minimum and a maximum,
>> +      * like this:
>> +      */
>> +     if (*nbuffers < STK1160_MIN_VIDEO_BUFFERS)
>> +             *nbuffers = STK1160_MIN_VIDEO_BUFFERS;
>> +     else if (*nbuffers > STK1160_MAX_VIDEO_BUFFERS)
>> +             *nbuffers = STK1160_MAX_VIDEO_BUFFERS;
>> +
>> +     /* This means a packed colorformat */
>> +     *nplanes = 1;
>> +
>> +     sizes[0] = size;
>> +
>> +     /*
>> +      * videobuf2-vmalloc allocator is context-less so no need to set
>> +      * alloc_ctxs array.
>> +      */
>> +
>> +     if (v4l_fmt) {
>> +             stk1160_info("selected format %d (%d)\n",
>> +                     v4l_fmt->fmt.pix.pixelformat,
>> +                     dev->fmt->fourcc);
>> +     }
>> +
>> +     stk1160_info("%s: buffer count %d, each %ld bytes\n",
>> +                     __func__, *nbuffers, size);
>> +
>> +     return 0;
>> +}
>> +
>> +static int buffer_init(struct vb2_buffer *vb)
>> +{
>> +     return 0;
>> +}
>> +
>> +static int buffer_prepare(struct vb2_buffer *vb)
>> +{
>> +     struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>> +     struct stk1160_buffer *buf =
>> +                     container_of(vb, struct stk1160_buffer, vb);
>> +
>> +     /* If the device is disconnected, reject the buffer */
>> +     if (!dev->udev)
>> +             return -ENODEV;
>> +
>> +     buf->mem = vb2_plane_vaddr(vb, 0);
>> +     buf->length = vb2_plane_size(vb, 0);
>> +     buf->bytesused = 0;
>> +     buf->pos = 0;
>> +
>> +     return 0;
>> +}
>> +
>> +static int buffer_finish(struct vb2_buffer *vb)
>> +{
>> +     return 0;
>> +}
>> +
>> +static void buffer_cleanup(struct vb2_buffer *vb)
>> +{
>> +}
>> +
>> +static void buffer_queue(struct vb2_buffer *vb)
>> +{
>> +     unsigned long flags = 0;
>> +     struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>> +     struct stk1160_buffer *buf =
>> +             container_of(vb, struct stk1160_buffer, vb);
>> +
>> +     spin_lock_irqsave(&dev->buf_lock, flags);
>> +     if (!dev->udev) {
>> +             /*
>> +              * If the device is disconnected return the buffer to userspace
>> +              * directly. The next QBUF call will fail with -ENODEV.
>> +              */
>> +             vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>> +     } else {
>> +             list_add_tail(&buf->list, &dev->avail_bufs);
>> +     }
>> +     spin_unlock_irqrestore(&dev->buf_lock, flags);
>> +}
>> +
>> +static int start_streaming(struct vb2_queue *vq, unsigned int count)
>> +{
>> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
>> +     return stk1160_start_streaming(dev);
>> +}
>> +
>> +/* abort streaming and wait for last buffer */
>> +static int stop_streaming(struct vb2_queue *vq)
>> +{
>> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
>> +     return stk1160_stop_streaming(dev);
>> +}
>> +
>> +static void stk1160_lock(struct vb2_queue *vq)
>> +{
>> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
>> +     mutex_lock(&dev->mutex);
>> +}
>> +
>> +static void stk1160_unlock(struct vb2_queue *vq)
>> +{
>> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
>> +     mutex_unlock(&dev->mutex);
>> +}
>> +
>> +static void stk1160_release(struct video_device *vd)
>> +{
>> +     struct stk1160 *dev = video_get_drvdata(vd);
>> +
>> +     stk1160_info("releasing all resources\n");
>> +
>> +     video_set_drvdata(vd, NULL);
>> +     video_device_release(vd);
>> +     v4l2_device_unregister(&dev->v4l2_dev);
>> +
>> +     kfree(dev->alt_max_pkt_size);
>> +     kfree(dev);
>> +}
>> +
>> +static struct vb2_ops stk1160_video_qops = {
>> +     .queue_setup            = queue_setup,
>> +     .buf_init               = buffer_init,
>> +     .buf_prepare            = buffer_prepare,
>> +     .buf_finish             = buffer_finish,
>> +     .buf_cleanup            = buffer_cleanup,
>> +     .buf_queue              = buffer_queue,
>> +     .start_streaming        = start_streaming,
>> +     .stop_streaming         = stop_streaming,
>> +     .wait_prepare           = stk1160_unlock,
>> +     .wait_finish            = stk1160_lock,
>> +};
>> +
>> +static struct video_device v4l_template = {
>> +     .name = "stk1160",
>> +     .tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50,
>> +     .current_norm = V4L2_STD_NTSC,
>> +     .fops = &stk1160_fops,
>> +     .ioctl_ops = &stk1160_ioctl_ops,
>> +     .release = stk1160_release,
>> +};
>> +
>> +/********************************************************************/
>> +
>> +int stk1160_vb2_setup(struct stk1160 *dev)
>> +{
>> +     int rc;
>> +     struct vb2_queue *q;
>> +
>> +     q = &dev->vb_vidq;
>> +     memset(q, 0, sizeof(dev->vb_vidq));
>> +     q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +     q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR;
>> +     q->drv_priv = dev;
>> +     q->buf_struct_size = sizeof(struct stk1160_buffer);
>> +     q->ops = &stk1160_video_qops;
>> +     q->mem_ops = &vb2_vmalloc_memops;
>> +
>> +     rc = vb2_queue_init(q);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     /* initialize video dma queue */
>> +     INIT_LIST_HEAD(&dev->avail_bufs);
>> +
>> +     return 0;
>> +}
>> +
>> +int stk1160_video_register(struct stk1160 *dev)
>> +{
>> +     int rc = -ENOMEM;
>> +
>> +     /* There is no need to set the name if we give a device struct */
>> +     rc = v4l2_device_register(dev->dev, &dev->v4l2_dev);
>> +     if (rc) {
>> +             stk1160_err("v4l2_device_register failed (%d)\n", rc);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->vdev = video_device_alloc();
>
> I recommend that vdev is not a pointer but the struct video_device itself
> embedded in the struct stk1160. The release callback can be video_device_release_empty
> in that case. stk1160_release should be set as the release callback of v4l2_dev.

Mmm, I agree with you about the embedded struct part.
But, as I already said, I don't really see why I can't keep
video_device release as the final cleanup release.
It's not to argue you, I'm just trying to understand your point.

Maybe it's cleaner through v4l2_device release. I'll give a try.
I'm still trying to get a clear picture on the differences of
v4l2_device and video_device.

>
>> +     if (!dev->vdev) {
>> +             stk1160_err("video_device_alloc failed (%d)\n", rc);
>> +             goto unreg;
>> +     }
>> +
>> +     /* Initialize video_device with a template structure */
>> +     *dev->vdev = v4l_template;
>> +     dev->vdev->debug = vidioc_debug;
>> +
>> +     /*
>> +      * Provide a mutex to v4l2 core.
>> +      * It will be used to protect all fops and v4l2 ioctls.
>> +      */
>> +     dev->vdev->lock = &dev->mutex;
>
> Please note: we made a change for 3.5 where this lock is only used for
> ioctls, not for other file operations. You will have to review your code
> whether or not to take the lock explicitly for those other file operations.

Ok, I missed that change.

>
>> +
>> +     /* This will be used to set video_device parent */
>> +     dev->vdev->v4l2_dev = &dev->v4l2_dev;
>> +
>> +     /* It is safer to set this before registering with v4l2 */
>> +     video_set_drvdata(dev->vdev, dev);
>> +
>> +     rc = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);
>
> Do this as the last thing in this function.

Done.

>
>> +     if (rc < 0) {
>> +             stk1160_err("video_register_device failed (%d)\n", rc);
>> +             goto release;
>> +     }
>> +

Thanks!
Ezequiel.

Re: [RFC/PATCH] media: Add stk1160 new driver

[Hans Verkuil]

On Sun May 27 2012 23:20:21 Ezequiel Garcia wrote:
> Hi Hans,
> 
> On Sat, May 26, 2012 at 2:05 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > Always test your driver using the v4l2-compliance test tool that it part of
> > v4l-utils! If it passes that, then your code will be in really good shape!
> 
> You're right. I'll add v4l2-compliance output in the next patch.
> 
> >
> > On Sat May 26 2012 18:41:00 Ezequiel Garcia wrote:
> >> This driver adds support for stk1160 usb bridge as used in some
> >> video/audio usb capture devices.
> >> It is a complete rewrite of staging/media/easycap driver and
> >> it's expected as a future replacement.
> >>
> >> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> >> ---
> >> As of today testing has been performed using both vlc and mplayer
> >> on a gentoo machine, including hot unplug and on-the-fly standard
> >> change using a device with 1-cvs and 1-audio output.
> >> However more testing is underway with another device and/or another
> >> distribution.
> >>
> >> Alsa sound support is a missing feature (work in progress).
> >>
> >> As this is my first complete driver, the patch is (obviously) intended as RFC only.
> >> Any comments/reviews of *any* kind will be greatly appreciated.
> >> ---
> >>  drivers/media/video/Kconfig                 |    2 +
> >>  drivers/media/video/Makefile                |    1 +
> >>  drivers/media/video/stk1160/Kconfig         |   11 +
> >>  drivers/media/video/stk1160/Makefile        |    6 +
> >>  drivers/media/video/stk1160/stk1160-core.c  |  399 +++++++++++++
> >>  drivers/media/video/stk1160/stk1160-i2c.c   |  304 ++++++++++
> >>  drivers/media/video/stk1160/stk1160-reg.h   |   78 +++
> >>  drivers/media/video/stk1160/stk1160-v4l.c   |  846 +++++++++++++++++++++++++++
> >>  drivers/media/video/stk1160/stk1160-video.c |  506 ++++++++++++++++
> >>  drivers/media/video/stk1160/stk1160.h       |  183 ++++++
> >>  10 files changed, 2336 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/media/video/stk1160/Kconfig
> >>  create mode 100644 drivers/media/video/stk1160/Makefile
> >>  create mode 100644 drivers/media/video/stk1160/stk1160-core.c
> >>  create mode 100644 drivers/media/video/stk1160/stk1160-i2c.c
> >>  create mode 100644 drivers/media/video/stk1160/stk1160-reg.h
> >>  create mode 100644 drivers/media/video/stk1160/stk1160-v4l.c
> >>  create mode 100644 drivers/media/video/stk1160/stk1160-video.c
> >>  create mode 100644 drivers/media/video/stk1160/stk1160.h
> >>
> >> +
> >> +     /*
> >> +      * We take the lock just before device registration,
> >> +      * to prevent someone (probably udev) from opening us
> >> +      * before we finish initialization
> >> +      */
> >> +     mutex_init(&dev->mutex);
> >> +     mutex_lock(&dev->mutex);
> >> +
> >> +     rc = stk1160_video_register(dev);
> >
> > It's usually better to register the video node as the very last thing
> > in probe(). That way when the node appears it's ready for udev to use.
> > No need to lock the mutex in that case.
> 
> Done.
> 
> >> +     /*
> >> +      * Wait until all current v4l2 operation are finished
> >> +      * then deallocate resources
> >> +      */
> >> +     mutex_lock(&dev->mutex);
> >> +
> >> +     /* Since saa7115 is no longer present, it's better to unregister it */
> >> +     v4l2_device_unregister_subdev(dev->sd_saa7115);
> >> +
> >> +     stk1160_stop_streaming(dev);
> >> +
> >> +     v4l2_device_disconnect(&dev->v4l2_dev);
> >> +
> >> +     /* This way current users can detect device is gone */
> >> +     dev->udev = NULL;
> >> +
> >> +     mutex_unlock(&dev->mutex);
> >> +
> >> +     stk1160_i2c_unregister(dev);
> >> +     stk1160_video_unregister(dev);
> >
> > I recommend that you use the same approach as I did in media/radio/dsbr100.c:
> > use the v4l2_dev->release callback to handle the final cleanup. That is a safe
> > method that does all the refcounting for you.
> 
> I'm sorry but I don't really see the difference.
> Right now I'm having video_device release function to handle the final cleanup.
> I don't do the refcounting myself either. See my other comment below.

First of all, you can make it work reliably with just the video_device release().
But the advantage of using the release of v4l2_device is that it will also work with
USB devices with multiple video/vbi/radio nodes and it allows you to put all the code
in disconnect() under the lock. The disconnect call can happen at any time, so it
can be hard to get it right. In the code above an application can open the video
node right after the unlock and before i2c_unregister (which I would move to the
release callback anyway). The clean up from i2c_unregister might cause problems
for that new open.

In practice it seems that the easiest approach is not to clean up anything in the
disconnect, just take the lock, do the bare minimum necessary for the disconnect,
unregister the video nodes, unlock and end with v4l2_device_put(v4l2_dev).

It's a suggestion only, but experience has shown that it works well. And as I said,
when you get multiple device nodes, then this is the only workable approach.

Weren't there easycap devices with multiple video inputs? Or is that handled by
cycling through the inputs?

> 
> >
> > ...
> >
> >> diff --git a/drivers/media/video/stk1160/stk1160-v4l.c b/drivers/media/video/stk1160/stk1160-v4l.c
> >> new file mode 100644
> >> index 0000000..a7a012b
> >> --- /dev/null
> >> +++ b/drivers/media/video/stk1160/stk1160-v4l.c
> >
> > ...
> >
> >> +static int stk1160_open(struct file *filp)
> >> +{
> >> +     struct stk1160 *dev = video_drvdata(filp);
> >> +
> >> +     dev->users++;
> >
> > Why the users field? You shouldn't need it.
> 
> Done.
> 
> >
> >> +
> >> +     stk1160_info("opened: users=%d\n", dev->users);
> >> +
> >> +     return v4l2_fh_open(filp);
> >> +}
> >> +
> >> +static int stk1160_close(struct file *file)
> >> +{
> >> +     struct stk1160 *dev = video_drvdata(file);
> >> +
> >> +     dev->users--;
> >> +
> >> +     stk1160_info("closed: users=%d\n", dev->users);
> >> +
> >> +     /*
> >> +      * If this is the last fh remaining open, then we
> >> +      * stop streaming and free/dequeue all buffers.
> >> +      * This prevents device from streaming without
> >> +      * any opened filehandles.
> >> +      */
> >> +     if (v4l2_fh_is_singular_file(file))
> >> +             vb2_queue_release(&dev->vb_vidq);
> >
> > No. You should keep track of which filehandle started streaming (actually
> > the filehandle that called REQBUFS is the owner of the queue) and release
> > the queue when that particular filehandle is closed (or it calls REQBUFS
> > with a count of 0).
> 
> Ah. I gave much thought to this issue. I liked the way uvc managed
> "privileged" handles,
> but I wasn't sure if this was better or not
> (I'm thinking on "policy vs mechanism" stuff, sounds a bit silly, right?).
> 
> So, I'll implement an owner filehandle, which is able to start/stop streaming.
> Also set format? Or just start/stop streaming? How about the
> non-owners filehandles?
> Are they only capable of changing controls and such?
> Is this documented in media api?

OK, the general rule is as follows (many drivers do not follow this correctly, BTW,
but this is what should happen):

- the filehandle that calls REQBUFS owns the buffers and is the only one that can
start/stop streaming and queue/dequeue buffers. This is until REQBUFS with count == 0
is called, or until the filehandle is closed.

- setting formats, controls, etc. can be done by any filehandle unless the hardware
cannot support such a change while streaming is in progress (or prepared for, e.g.
changing the format will change the buffer size, which should be prohibited once a
filehandle called REQBUFS).

- by supporting VIDIOC_G/S_PRIORITY applications can raise the prio of a filehandle,
preventing other filehandles from making changes. If you use struct v4l2_fh, then all
you need to do is to set the V4L2_FL_USE_FH_PRIO flag (see dsbr100.c) and you get
priority handling for free.

> 
> 
> >> +
> >> +static int vidioc_queryctrl(struct file *file, void *priv,
> >> +                             struct v4l2_queryctrl *qc)
> >> +{
> >> +     struct stk1160 *dev = video_drvdata(file);
> >> +     int id = qc->id;
> >> +
> >> +     memset(qc, 0, sizeof(*qc));
> >> +     qc->id = id;
> >> +
> >> +     /* enumerate V4L2 device controls */
> >> +     v4l2_device_call_all(&dev->v4l2_dev, 0, core, queryctrl, qc);
> >> +     if (qc->type)
> >> +             return 0;
> >> +     else
> >> +             return -EINVAL;
> >> +}
> >
> > Use the control framework. I won't accept any new drivers that do not use it.
> >
> > In your case it is very simple: create a v4l2_ctrl_handler, initialize it and
> > let v4l2_dev->ctrl_handler point to it. When the subdev is added it will add its
> > controls to your handler.
> 
> Done.
> 
> >
> >> +
> >> +static int vidioc_g_ctrl(struct file *file, void *priv,
> >> +                             struct v4l2_control *ctrl)
> >> +{
> >> +     struct stk1160 *dev = video_drvdata(file);
> >> +     v4l2_device_call_all(&dev->v4l2_dev, 0, core, g_ctrl, ctrl);
> >> +     return 0;
> >> +}
> >> +
> >> +static int vidioc_s_ctrl(struct file *file, void *priv,
> >> +                             struct v4l2_control *ctrl)
> >> +{
> >> +     struct stk1160 *dev = video_drvdata(file);
> >> +     v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_ctrl, ctrl);
> >> +     return 0;
> >> +}
> >> +
> >> +static int vidioc_enum_framesizes(struct file *file, void *fh,
> >> +                              struct v4l2_frmsizeenum *fsize)
> >> +{
> >> +     /* TODO: Is this needed? */
> >> +     return -EINVAL;
> >> +}
> >> +
> >> +static int vidioc_enum_frameintervals(struct file *file, void *fh,
> >> +                               struct v4l2_frmivalenum *fival)
> >> +{
> >> +     /* TODO: Is this needed? */
> >> +     return -EINVAL;
> >> +}
> >> +
> >> +static const struct v4l2_ioctl_ops stk1160_ioctl_ops = {
> >> +     .vidioc_querycap      = vidioc_querycap,
> >> +     .vidioc_enum_fmt_vid_cap  = vidioc_enum_fmt_vid_cap,
> >> +     .vidioc_g_fmt_vid_cap     = vidioc_g_fmt_vid_cap,
> >> +     .vidioc_try_fmt_vid_cap   = vidioc_try_fmt_vid_cap,
> >> +     .vidioc_s_fmt_vid_cap     = vidioc_s_fmt_vid_cap,
> >> +     .vidioc_reqbufs       = vidioc_reqbufs,
> >> +     .vidioc_querybuf      = vidioc_querybuf,
> >> +     .vidioc_qbuf          = vidioc_qbuf,
> >> +     .vidioc_dqbuf         = vidioc_dqbuf,
> >> +     .vidioc_querystd      = vidioc_querystd,
> >> +     .vidioc_g_std         = NULL, /* don't worry v4l handles this */
> >> +     .vidioc_s_std         = vidioc_s_std,
> >> +     .vidioc_enum_input    = vidioc_enum_input,
> >> +     .vidioc_g_input       = vidioc_g_input,
> >> +     .vidioc_s_input       = vidioc_s_input,
> >> +     .vidioc_queryctrl     = vidioc_queryctrl,
> >> +     .vidioc_g_ctrl        = vidioc_g_ctrl,
> >> +     .vidioc_s_ctrl        = vidioc_s_ctrl,
> >> +     .vidioc_enum_framesizes = vidioc_enum_framesizes,
> >> +     .vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> >> +     .vidioc_streamon      = vidioc_streamon,
> >> +     .vidioc_streamoff     = vidioc_streamoff,
> >> +};
> >> +
> >> +/********************************************************************/
> >> +
> >> +/*
> >> + * Videobuf2 operations
> >> + */
> >> +static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *v4l_fmt,
> >> +                             unsigned int *nbuffers, unsigned int *nplanes,
> >> +                             unsigned int sizes[], void *alloc_ctxs[])
> >> +{
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
> >> +     unsigned long size;
> >> +
> >> +     size = dev->width * dev->height * 2;
> >> +
> >> +     /*
> >> +      * Here we can change the number of buffers being requested.
> >> +      * For instance, we could set a minimum and a maximum,
> >> +      * like this:
> >> +      */
> >> +     if (*nbuffers < STK1160_MIN_VIDEO_BUFFERS)
> >> +             *nbuffers = STK1160_MIN_VIDEO_BUFFERS;
> >> +     else if (*nbuffers > STK1160_MAX_VIDEO_BUFFERS)
> >> +             *nbuffers = STK1160_MAX_VIDEO_BUFFERS;
> >> +
> >> +     /* This means a packed colorformat */
> >> +     *nplanes = 1;
> >> +
> >> +     sizes[0] = size;
> >> +
> >> +     /*
> >> +      * videobuf2-vmalloc allocator is context-less so no need to set
> >> +      * alloc_ctxs array.
> >> +      */
> >> +
> >> +     if (v4l_fmt) {
> >> +             stk1160_info("selected format %d (%d)\n",
> >> +                     v4l_fmt->fmt.pix.pixelformat,
> >> +                     dev->fmt->fourcc);
> >> +     }
> >> +
> >> +     stk1160_info("%s: buffer count %d, each %ld bytes\n",
> >> +                     __func__, *nbuffers, size);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int buffer_init(struct vb2_buffer *vb)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >> +static int buffer_prepare(struct vb2_buffer *vb)
> >> +{
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
> >> +     struct stk1160_buffer *buf =
> >> +                     container_of(vb, struct stk1160_buffer, vb);
> >> +
> >> +     /* If the device is disconnected, reject the buffer */
> >> +     if (!dev->udev)
> >> +             return -ENODEV;
> >> +
> >> +     buf->mem = vb2_plane_vaddr(vb, 0);
> >> +     buf->length = vb2_plane_size(vb, 0);
> >> +     buf->bytesused = 0;
> >> +     buf->pos = 0;
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int buffer_finish(struct vb2_buffer *vb)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >> +static void buffer_cleanup(struct vb2_buffer *vb)
> >> +{
> >> +}
> >> +
> >> +static void buffer_queue(struct vb2_buffer *vb)
> >> +{
> >> +     unsigned long flags = 0;
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
> >> +     struct stk1160_buffer *buf =
> >> +             container_of(vb, struct stk1160_buffer, vb);
> >> +
> >> +     spin_lock_irqsave(&dev->buf_lock, flags);
> >> +     if (!dev->udev) {
> >> +             /*
> >> +              * If the device is disconnected return the buffer to userspace
> >> +              * directly. The next QBUF call will fail with -ENODEV.
> >> +              */
> >> +             vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> >> +     } else {
> >> +             list_add_tail(&buf->list, &dev->avail_bufs);
> >> +     }
> >> +     spin_unlock_irqrestore(&dev->buf_lock, flags);
> >> +}
> >> +
> >> +static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >> +{
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
> >> +     return stk1160_start_streaming(dev);
> >> +}
> >> +
> >> +/* abort streaming and wait for last buffer */
> >> +static int stop_streaming(struct vb2_queue *vq)
> >> +{
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
> >> +     return stk1160_stop_streaming(dev);
> >> +}
> >> +
> >> +static void stk1160_lock(struct vb2_queue *vq)
> >> +{
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
> >> +     mutex_lock(&dev->mutex);
> >> +}
> >> +
> >> +static void stk1160_unlock(struct vb2_queue *vq)
> >> +{
> >> +     struct stk1160 *dev = vb2_get_drv_priv(vq);
> >> +     mutex_unlock(&dev->mutex);
> >> +}
> >> +
> >> +static void stk1160_release(struct video_device *vd)
> >> +{
> >> +     struct stk1160 *dev = video_get_drvdata(vd);
> >> +
> >> +     stk1160_info("releasing all resources\n");
> >> +
> >> +     video_set_drvdata(vd, NULL);
> >> +     video_device_release(vd);
> >> +     v4l2_device_unregister(&dev->v4l2_dev);
> >> +
> >> +     kfree(dev->alt_max_pkt_size);
> >> +     kfree(dev);
> >> +}
> >> +
> >> +static struct vb2_ops stk1160_video_qops = {
> >> +     .queue_setup            = queue_setup,
> >> +     .buf_init               = buffer_init,
> >> +     .buf_prepare            = buffer_prepare,
> >> +     .buf_finish             = buffer_finish,
> >> +     .buf_cleanup            = buffer_cleanup,
> >> +     .buf_queue              = buffer_queue,
> >> +     .start_streaming        = start_streaming,
> >> +     .stop_streaming         = stop_streaming,
> >> +     .wait_prepare           = stk1160_unlock,
> >> +     .wait_finish            = stk1160_lock,
> >> +};
> >> +
> >> +static struct video_device v4l_template = {
> >> +     .name = "stk1160",
> >> +     .tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50,
> >> +     .current_norm = V4L2_STD_NTSC,
> >> +     .fops = &stk1160_fops,
> >> +     .ioctl_ops = &stk1160_ioctl_ops,
> >> +     .release = stk1160_release,
> >> +};
> >> +
> >> +/********************************************************************/
> >> +
> >> +int stk1160_vb2_setup(struct stk1160 *dev)
> >> +{
> >> +     int rc;
> >> +     struct vb2_queue *q;
> >> +
> >> +     q = &dev->vb_vidq;
> >> +     memset(q, 0, sizeof(dev->vb_vidq));
> >> +     q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +     q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR;
> >> +     q->drv_priv = dev;
> >> +     q->buf_struct_size = sizeof(struct stk1160_buffer);
> >> +     q->ops = &stk1160_video_qops;
> >> +     q->mem_ops = &vb2_vmalloc_memops;
> >> +
> >> +     rc = vb2_queue_init(q);
> >> +     if (rc < 0)
> >> +             return rc;
> >> +
> >> +     /* initialize video dma queue */
> >> +     INIT_LIST_HEAD(&dev->avail_bufs);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +int stk1160_video_register(struct stk1160 *dev)
> >> +{
> >> +     int rc = -ENOMEM;
> >> +
> >> +     /* There is no need to set the name if we give a device struct */
> >> +     rc = v4l2_device_register(dev->dev, &dev->v4l2_dev);
> >> +     if (rc) {
> >> +             stk1160_err("v4l2_device_register failed (%d)\n", rc);
> >> +             return -ENOMEM;
> >> +     }
> >> +
> >> +     dev->vdev = video_device_alloc();
> >
> > I recommend that vdev is not a pointer but the struct video_device itself
> > embedded in the struct stk1160. The release callback can be video_device_release_empty
> > in that case. stk1160_release should be set as the release callback of v4l2_dev.
> 
> Mmm, I agree with you about the embedded struct part.
> But, as I already said, I don't really see why I can't keep
> video_device release as the final cleanup release.
> It's not to argue you, I'm just trying to understand your point.
> 
> Maybe it's cleaner through v4l2_device release. I'll give a try.
> I'm still trying to get a clear picture on the differences of
> v4l2_device and video_device.

v4l2_device is a top-level struct, video_device represents a single device node.
For cleanup purposes there isn't much difference between the two if you have
only one device node. When you have more, then those differences are much more
important.

> 
> >
> >> +     if (!dev->vdev) {
> >> +             stk1160_err("video_device_alloc failed (%d)\n", rc);
> >> +             goto unreg;
> >> +     }
> >> +
> >> +     /* Initialize video_device with a template structure */
> >> +     *dev->vdev = v4l_template;
> >> +     dev->vdev->debug = vidioc_debug;
> >> +
> >> +     /*
> >> +      * Provide a mutex to v4l2 core.
> >> +      * It will be used to protect all fops and v4l2 ioctls.
> >> +      */
> >> +     dev->vdev->lock = &dev->mutex;
> >
> > Please note: we made a change for 3.5 where this lock is only used for
> > ioctls, not for other file operations. You will have to review your code
> > whether or not to take the lock explicitly for those other file operations.
> 
> Ok, I missed that change.

Yeah, it just went in.

> 
> >
> >> +
> >> +     /* This will be used to set video_device parent */
> >> +     dev->vdev->v4l2_dev = &dev->v4l2_dev;
> >> +
> >> +     /* It is safer to set this before registering with v4l2 */
> >> +     video_set_drvdata(dev->vdev, dev);
> >> +
> >> +     rc = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);
> >
> > Do this as the last thing in this function.
> 
> Done.
> 
> >
> >> +     if (rc < 0) {
> >> +             stk1160_err("video_register_device failed (%d)\n", rc);
> >> +             goto release;
> >> +     }
> >> +
> 
> Thanks!
> Ezequiel.
> 

Regards,

	Hans

Re: [RFC/PATCH] media: Add stk1160 new driver

[Ezequiel Garcia]

On Mon, May 28, 2012 at 7:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> In practice it seems that the easiest approach is not to clean up anything in the
> disconnect, just take the lock, do the bare minimum necessary for the disconnect,
> unregister the video nodes, unlock and end with v4l2_device_put(v4l2_dev).
>
> It's a suggestion only, but experience has shown that it works well. And as I said,
> when you get multiple device nodes, then this is the only workable approach.

I'm convinced: it's both cleaner and more logical to use
v4l2_release instead of video_device release to the final cleanup.

>
> OK, the general rule is as follows (many drivers do not follow this correctly, BTW,
> but this is what should happen):
>
> - the filehandle that calls REQBUFS owns the buffers and is the only one that can
> start/stop streaming and queue/dequeue buffers.

and read, poll, etc right?

> This is until REQBUFS with count == 0
> is called, or until the filehandle is closed.

Okey. But currently videobuf2 doesn't notify the driver
when reqbufs with zero count has been called.

So, I have to "assume" it (aka trouble ahead) or "capture" the zero
count case before/after calling vb2_reqbufs (aka ugly).

I humbly think that, if we wan't to enforce this behavior
(as part of v4l2 driver semantics)
then we should have videobuf2 tell the driver when reqbufs has been
called with zero count.

You can take a look at pwc which only drops owner on filehandle close,
or uvc which captures this from vb2_reqbufs.

After looking at uvc, now I wonder is it really ugly? or perhaps
it's just ok.


> v4l2_device is a top-level struct, video_device represents a single device node.
> For cleanup purposes there isn't much difference between the two if you have
> only one device node. When you have more, then those differences are much more
> important.

Yes, it's cleaner now.

Thanks!
Ezequiel.

Re: [RFC/PATCH] media: Add stk1160 new driver

[Hans Verkuil]

On Tue 29 May 2012 14:05:08 Ezequiel Garcia wrote:
> On Mon, May 28, 2012 at 7:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > In practice it seems that the easiest approach is not to clean up anything in the
> > disconnect, just take the lock, do the bare minimum necessary for the disconnect,
> > unregister the video nodes, unlock and end with v4l2_device_put(v4l2_dev).
> >
> > It's a suggestion only, but experience has shown that it works well. And as I said,
> > when you get multiple device nodes, then this is the only workable approach.
> 
> I'm convinced: it's both cleaner and more logical to use
> v4l2_release instead of video_device release to the final cleanup.
> 
> >
> > OK, the general rule is as follows (many drivers do not follow this correctly, BTW,
> > but this is what should happen):
> >
> > - the filehandle that calls REQBUFS owns the buffers and is the only one that can
> > start/stop streaming and queue/dequeue buffers.
> 
> and read, poll, etc right?

Read yes, but anyone can poll.

> 
> > This is until REQBUFS with count == 0
> > is called, or until the filehandle is closed.
> 
> Okey. But currently videobuf2 doesn't notify the driver
> when reqbufs with zero count has been called.
> 
> So, I have to "assume" it (aka trouble ahead) or "capture" the zero
> count case before/after calling vb2_reqbufs (aka ugly).

You just check the count after calling vb2_reqbufs. Nothing ugly about it.

> I humbly think that, if we wan't to enforce this behavior
> (as part of v4l2 driver semantics)
> then we should have videobuf2 tell the driver when reqbufs has been
> called with zero count.
> 
> You can take a look at pwc which only drops owner on filehandle close,

That's a bug. REQBUFS(0) must drop the owner: you've just released all
resources related to streaming, so there is no reason to prevent others
from using them. But I suspect 90% of all drivers do this wrong. Not to
mention that the old videobuf doesn't handle a count of 0 at all, which is
a complete violation of the spec. So an application never knows whether
a count of 0 will work. Lovely... The qv4l2 test tool has some really ugly
workaround for this :-)

At some point I'm going to add a test for this to v4l2-compliance...

> or uvc which captures this from vb2_reqbufs.
> 
> After looking at uvc, now I wonder is it really ugly? or perhaps
> it's just ok.

It would be nice if this was somehow integrated into videobuf2, but not everyone
liked the idea of vb2 using filehandle information from what I remember. But I
didn't push for it very hard.

> 
> 
> > v4l2_device is a top-level struct, video_device represents a single device node.
> > For cleanup purposes there isn't much difference between the two if you have
> > only one device node. When you have more, then those differences are much more
> > important.
> 
> Yes, it's cleaner now.
> 
> Thanks!
> Ezequiel.
> 

Your welcome!

	Hans