On Tuesday, November 16, 2010 17:49:05 Hans Verkuil wrote:
> On Tuesday, November 16, 2010 17:01:36 Arnd Bergmann wrote:
> > On Tuesday 16 November 2010, Hans Verkuil wrote:
> > > > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > > > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > > > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > > > struct video_device (and I really hope I can rename that to v4l2_devnode
> > > > soon since that's a very confusing name).
> > > >
> > > > You typically need to serialize between all the device nodes belonging to
> > > > the same video hardware. A mutex in struct video_device doesn't do that,
> > > > that just serializes access to that single device node. But a mutex in
> > > > v4l2_device is at the right level.
> >
> > Ok, got it now.
> >
> > > A quick follow-up as I saw I didn't fully answer your question: to my
> > > knowledge there are no per-driver data structures that need a BKL for
> > > protection. It's definitely not something I am worried about.
> >
> > Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> > like the right place with your explanation. I would not put in the
> > CONFIG_BKL switch, because I tried that for two other subsystems and got
> > called back, but I'm not going to stop you.
> >
> > As for the fallback to a global mutex, I guess you can set the
> > videodev->lock pointer and use unlocked_ioctl for those drivers
> > that do not use a v4l2_device yet, if there are only a handful of them.
> >
> > Arnd
> >
>
> I will look into it. I'll try to have something today or tomorrow.
OK, here is my patch adding a mutex to v4l2_device.
I did some tests if we merge this patch then there are three classes of
drivers:
1) Those implementing unlocked_ioctl: these work like a charm.
2) Those implementing v4l2_device: capturing works fine, but calling ioctls
at the same time from another process or thread is *exceedingly* slow. But at
least there is no interference from other drivers.
3) Those not implementing v4l2_device: using a core lock makes it simply
impossible to capture from e.g. two devices at the same time. I tried with two
uvc webcams: the capture rate is simply horrible.
Note that this is tested in blocking mode. These problems do not appear if you
capture in non-blocking mode.
I consider class 3 unacceptable for commonly seen devices. I did a quick scan
of the v4l drivers and the only common driver that falls in that class is uvc.
There is one other option, although it is very dirty: don't take the lock if
the ioctl command is VIDIOC_DQBUF. It works and reliably as well for uvc and
videobuf (I did a quick code analysis). But I don't know if it works everywhere.
I would like to get the opinion of others before I implement such a check. But
frankly, I think this may be our best bet.
So the patch below would look like this if I add the check:
- mutex_lock(&v4l2_ioctl_mutex);
+ if (cmd != VIDIOC_DQBUF)
+ mutex_lock(m);
if (video_is_registered(vdev))
ret = vdev->fops->ioctl(filp, cmd, arg);
- mutex_unlock(&v4l2_ioctl_mutex);
+ if (cmd != VIDIOC_DQBUF)
+ mutex_unlock(m);
Comments?
Regards,
Hans
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 03f7f46..026bf38 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -247,11 +247,13 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
} else if (vdev->fops->ioctl) {
/* TODO: convert all drivers to unlocked_ioctl */
static DEFINE_MUTEX(v4l2_ioctl_mutex);
+ struct mutex *m = vdev->v4l2_dev ?
+ &vdev->v4l2_dev->ioctl_lock : &v4l2_ioctl_mutex;
- mutex_lock(&v4l2_ioctl_mutex);
+ mutex_lock(m);
if (video_is_registered(vdev))
ret = vdev->fops->ioctl(filp, cmd, arg);
- mutex_unlock(&v4l2_ioctl_mutex);
+ mutex_unlock(m);
} else
ret = -ENOTTY;
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 0b08f96..7fe6f92 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -35,6 +35,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
INIT_LIST_HEAD(&v4l2_dev->subdevs);
spin_lock_init(&v4l2_dev->lock);
+ mutex_init(&v4l2_dev->ioctl_lock);
v4l2_dev->dev = dev;
if (dev == NULL) {
/* If dev == NULL, then name must be filled in by the caller */
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 6648036..b16f307 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -51,6 +51,8 @@ struct v4l2_device {
unsigned int notification, void *arg);
/* The control handler. May be NULL. */
struct v4l2_ctrl_handler *ctrl_handler;
+ /* BKL replacement mutex. Temporary solution only. */
+ struct mutex ioctl_lock;
};
/* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev.
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
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
