Re: [RFC/PATCH] media: Add stk1160 new driver
Hi Hans,
On Sat, May 26, 2012 at 2:05 PM, Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Linux Input]
[Video for Linux]
[Mplayer Users]
[Linux USB Devel]
[Linux Audio Users]
[Photos]
[Yosemite Photos]
[Linux Kernel]
[Linux SCSI]
[XFree86]
[Devices]
[Yosemite Backpacking]
