On Sun May 27 2012 23:20:21 Ezequiel Garcia wrote:
> Hi Hans,
>
> On Sat, May 26, 2012 at 2:05 PM, Hans Verkuil <hverk...@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 <elezegar...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
