Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tuesday 16 November 2010, Hans Verkuil wrote:
> No, it will also affect e.g. two bttv cards that you capture from in
> parallel. Or two webcams, or...
Would it be safe to turn the global mutex into a per-driver or per-device
mutex? That would largely mitigate the impact as far as I can tell.
> We can't just ditch the BKL yet for 2.6.37 IMHO. Perhaps for 2.6.38 if we
> all work really hard to convert everything.
Linus was pretty clear in that he wanted to make the default for the BKL
disabled for 2.6.37. That may of course change if there are significant
problems with this, but as long as there is an easier way, we could do
that instead.
I have not tested the patch below, but maybe that would solve the
immediate problem without reverting v4l2-dev back to use the BKL.
It would not work if we have drivers that need to serialize access
to multiple v4l2 devices in their ioctl functions because they access
global data, which is unlikely but possible.
Signed-off-by: Arnd Bergmann
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 03f7f46..5873d12 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -246,12 +246,11 @@ static long v4l2_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
mutex_unlock(vdev->lock);
} else if (vdev->fops->ioctl) {
/* TODO: convert all drivers to unlocked_ioctl */
- static DEFINE_MUTEX(v4l2_ioctl_mutex);
-
- mutex_lock(&v4l2_ioctl_mutex);
- if (video_is_registered(vdev))
+ if (video_is_registered(vdev)) {
+ mutex_lock(&vdev->ioctl_lock);
ret = vdev->fops->ioctl(filp, cmd, arg);
- mutex_unlock(&v4l2_ioctl_mutex);
+ mutex_unlock(&vdev->ioctl_lock);
+ }
} else
ret = -ENOTTY;
@@ -507,6 +506,7 @@ static int __video_register_device(struct video_device
*vdev, int type, int nr,
#endif
vdev->minor = i + minor_offset;
vdev->num = nr;
+ mutex_init(&vdev->ioctl_lock);
devnode_set(vdev);
/* Should not happen since we thought this minor was free */
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 15802a0..e8a8485 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -97,6 +97,9 @@ struct video_device
/* serialization lock */
struct mutex *lock;
+
+ /* used for the legacy locked ioctl */
+ struct mutex ioctl_lock;
};
/* dev to video-device */
--
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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tue, Nov 16, 2010 at 4:42 PM, Hans Verkuil wrote: > On Tuesday, November 16, 2010 22:32:57 David Ellingsworth wrote: >> Hans, >> >> I've had some patches pending for a while now that affect the dsbr100 >> driver. The patches can be seen here: >> http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the >> dsbr100 branch. The first patch in the series fixes locking issues >> throughout the driver and converts it to use the unlocked ioctl. The >> series is a bit old, so it doesn't make use of the v4l2 core assisted >> locking; but that is trivial to implement after this patch. > > Would it be a problem for you if for 2.6.37 I just replace .ioctl by > .unlocked_ioctl? And do the full conversion for 2.6.38? That way the > 2.6.37 patches remain small. > If you look at the first patch in that series, you'll see that the conversion isn't that simple. There are a lot of places in that driver that should have been protected by a lock that weren't. At the moment, I think the BKL protects these areas from racing so just replacing the .ioctl with the .unlocked_ioctl here isn't a good solution for this driver. Applying the patch I've provided will however remove the BKL and the resolve all the locking issues as well. Regards, David Ellingsworth -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tuesday, November 16, 2010 22:32:57 David Ellingsworth wrote: > Hans, > > I've had some patches pending for a while now that affect the dsbr100 > driver. The patches can be seen here: > http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the > dsbr100 branch. The first patch in the series fixes locking issues > throughout the driver and converts it to use the unlocked ioctl. The > series is a bit old, so it doesn't make use of the v4l2 core assisted > locking; but that is trivial to implement after this patch. Would it be a problem for you if for 2.6.37 I just replace .ioctl by .unlocked_ioctl? And do the full conversion for 2.6.38? That way the 2.6.37 patches remain small. Regards. Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
Hans, I've had some patches pending for a while now that affect the dsbr100 driver. The patches can be seen here: http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the dsbr100 branch. The first patch in the series fixes locking issues throughout the driver and converts it to use the unlocked ioctl. The series is a bit old, so it doesn't make use of the v4l2 core assisted locking; but that is trivial to implement after this patch. Regards, David Ellingsworth -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tuesday, November 16, 2010 21:29:11 Hans Verkuil wrote: > On Tuesday, November 16, 2010 20:59:41 Andy Walls wrote: > > On Tue, 2010-11-16 at 19:38 +0100, Hans Verkuil wrote: > > > 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. > > BTW, with 'exceedingly slow' I mean that e.g. a v4l2-ctl --list-ctrls takes > 10-20 > seconds if the device node is streaming at the same time. Something that > normally > takes less than 0.01s. > > > > 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. > > > > Is this "in addition to" or "instead of" the mutex lock at > > v4l2_device ? > > In addition to. > > > > 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. > > > > Opinions? No problem! ;) > > > > > > > > I think it is probably bad. > > > > > > > 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); > > > > What happens to driver state when VIDIOC_STREAMOFF has the lock held and > > VIDIOC_DQBUF comes through? I think it is legitimate design for an > > application to have a playback control thread separate from a thread > > that reads in the capture data. > > All I can say is that it will work OK for drivers using videobuf since > videobuf will take its own lock. > > UVC? I'm not sure. But I think that it is p
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tuesday, November 16, 2010 20:59:41 Andy Walls wrote: > On Tue, 2010-11-16 at 19:38 +0100, Hans Verkuil wrote: > > 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. BTW, with 'exceedingly slow' I mean that e.g. a v4l2-ctl --list-ctrls takes 10-20 seconds if the device node is streaming at the same time. Something that normally takes less than 0.01s. > > 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. > > Is this "in addition to" or "instead of" the mutex lock at > v4l2_device ? In addition to. > > 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. > > Opinions? No problem! ;) > > > > I think it is probably bad. > > > > 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); > > What happens to driver state when VIDIOC_STREAMOFF has the lock held and > VIDIOC_DQBUF comes through? I think it is legitimate design for an > application to have a playback control thread separate from a thread > that reads in the capture data. All I can say is that it will work OK for drivers using videobuf since videobuf will take its own lock. UVC? I'm not sure. But I think that it is probably best to fix uvc for 2.6.37 regardless given the importance of this driver. It's dirty, but I actually think that it will work fine. > If this quirk of "infrastructure locking" is going in, might I suggest > that you please document in code comments: > > a. The scope of what infrastructure lock is intended to protect. That > is obvious right now, but
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tue, 2010-11-16 at 19:38 +0100, Hans Verkuil wrote: > 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. Is this "in addition to" or "instead of" the mutex lock at v4l2_device ? > 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. Opinions? No problem! ;) I think it is probably bad. > 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); What happens to driver state when VIDIOC_STREAMOFF has the lock held and VIDIOC_DQBUF comes through? I think it is legitimate design for an application to have a playback control thread separate from a thread that reads in the capture data. If this quirk of "infrastructure locking" is going in, might I suggest that you please document in code comments: a. The scope of what infrastructure lock is intended to protect. That is obvious right now, but may not be in the future. b. Why there is an exception to taking the infrastructure lock or what conditions necessitate having the lock ignored/dropped. c. What code maintenance must be done to remove the exception to taking the lock. A specific bullet-list of problem drivers might be nice. We won't do future maintainers any favors by letting the operation, intended behavior, intended scope, and rationale for this odd locking semantic be lost to history. We just introduce a BKL with smaller scope. > 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, u
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tuesday 16 November 2010, Hans Verkuil wrote: > 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. If uvc is the only important one, that should be easy enough to fix by adding a per-device mutex around uvc_v4l2_do_ioctl() or uvc_v4l2_ioctl(). > 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); > I was thinking of this as well, but didn't bring it up because I considered it too hacky. The patch you posted looks good, thanks for bringing up the problem with my patch and the solution! Acked-by: Arnd Bergmann -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
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/v4
Re: [RFC PATCH 0/8] V4L BKL removal: first round
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. Regards, Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
Em 16-11-2010 14:01, Arnd Bergmann escreveu: > 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. Hans, FYI, Linus already pull Arnd patch that removed BKL on -rc2 (commit 0edf2e5e2bd0ae7689ce8a57ae3c87cc1f0c6548). I've pulled back from -rc2 into linux-media.git tree. So, the patch is now visible on our main tree. Cheers, Mauro -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
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 -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
> >> On Tuesday 16 November 2010, Hans Verkuil wrote: >>> A pointer to this struct is available in vdev->v4l2_dev. However, not >>> all >>> drivers implement struct v4l2_device. But on the other hand, most >>> relevant >>> drivers do. So as a fallback we would still need a static mutex. >> >> Wouldn't that suffer the same problem as putting the mutex into videodev >> as I suggested? You said that there are probably drivers that need to >> serialize between multiple devices, so if we have a mutex per >> v4l2_device, >> you can still get races between multiple ioctl calls accessing the same >> per-driver data. To solve this, we'd have to put the lock into a >> per-driver >> structure like v4l2_file_operations or v4l2_ioctl_ops, which would add >> to the ugliness. > > 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. 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. Regards, Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
> On Tuesday 16 November 2010, Hans Verkuil wrote: >> A pointer to this struct is available in vdev->v4l2_dev. However, not >> all >> drivers implement struct v4l2_device. But on the other hand, most >> relevant >> drivers do. So as a fallback we would still need a static mutex. > > Wouldn't that suffer the same problem as putting the mutex into videodev > as I suggested? You said that there are probably drivers that need to > serialize between multiple devices, so if we have a mutex per v4l2_device, > you can still get races between multiple ioctl calls accessing the same > per-driver data. To solve this, we'd have to put the lock into a > per-driver > structure like v4l2_file_operations or v4l2_ioctl_ops, which would add > to the ugliness. 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. Hans > > Arnd > -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Tuesday 16 November 2010, Hans Verkuil wrote: > A pointer to this struct is available in vdev->v4l2_dev. However, not all > drivers implement struct v4l2_device. But on the other hand, most relevant > drivers do. So as a fallback we would still need a static mutex. Wouldn't that suffer the same problem as putting the mutex into videodev as I suggested? You said that there are probably drivers that need to serialize between multiple devices, so if we have a mutex per v4l2_device, you can still get races between multiple ioctl calls accessing the same per-driver data. To solve this, we'd have to put the lock into a per-driver structure like v4l2_file_operations or v4l2_ioctl_ops, which would add to the ugliness. Arnd -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
> On Tuesday 16 November 2010, Hans Verkuil wrote:
>> No, it will also affect e.g. two bttv cards that you capture from in
>> parallel. Or two webcams, or...
>
> Would it be safe to turn the global mutex into a per-driver or per-device
> mutex? That would largely mitigate the impact as far as I can tell.
What would work better is to add a mutex to struct v4l2_device (which is
the top-level struct for v4l devices) and have the core lock that.
A pointer to this struct is available in vdev->v4l2_dev. However, not all
drivers implement struct v4l2_device. But on the other hand, most relevant
drivers do. So as a fallback we would still need a static mutex.
What would be best is to add an #ifdef CONFIG_LOCK_KERNEL and use the BKL
there. In the #else we can use a v4l2_device mutex, or (if not set) a
static mutex.
Hardly elegant, but it'll have to do.
>> We can't just ditch the BKL yet for 2.6.37 IMHO. Perhaps for 2.6.38 if
>> we
>> all work really hard to convert everything.
>
> Linus was pretty clear in that he wanted to make the default for the BKL
> disabled for 2.6.37. That may of course change if there are significant
> problems with this, but as long as there is an easier way, we could do
> that instead.
>
> I have not tested the patch below, but maybe that would solve the
> immediate problem without reverting v4l2-dev back to use the BKL.
>
> It would not work if we have drivers that need to serialize access
> to multiple v4l2 devices in their ioctl functions because they access
> global data, which is unlikely but possible.
It's very likely, I'm afraid :-(
Regards,
Hans
> Signed-off-by: Arnd Bergmann
>
> diff --git a/drivers/media/video/v4l2-dev.c
> b/drivers/media/video/v4l2-dev.c
> index 03f7f46..5873d12 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -246,12 +246,11 @@ static long v4l2_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
> mutex_unlock(vdev->lock);
> } else if (vdev->fops->ioctl) {
> /* TODO: convert all drivers to unlocked_ioctl */
> - static DEFINE_MUTEX(v4l2_ioctl_mutex);
> -
> - mutex_lock(&v4l2_ioctl_mutex);
> - if (video_is_registered(vdev))
> + if (video_is_registered(vdev)) {
> + mutex_lock(&vdev->ioctl_lock);
> ret = vdev->fops->ioctl(filp, cmd, arg);
> - mutex_unlock(&v4l2_ioctl_mutex);
> + mutex_unlock(&vdev->ioctl_lock);
> + }
> } else
> ret = -ENOTTY;
>
> @@ -507,6 +506,7 @@ static int __video_register_device(struct video_device
> *vdev, int type, int nr,
> #endif
> vdev->minor = i + minor_offset;
> vdev->num = nr;
> + mutex_init(&vdev->ioctl_lock);
> devnode_set(vdev);
>
> /* Should not happen since we thought this minor was free */
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 15802a0..e8a8485 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -97,6 +97,9 @@ struct video_device
>
> /* serialization lock */
> struct mutex *lock;
> +
> + /* used for the legacy locked ioctl */
> + struct mutex ioctl_lock;
> };
>
> /* dev to video-device */
>
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
> Em 16-11-2010 10:35, Hans Verkuil escreveu: >> >>> Em 15-11-2010 07:49, Hans Verkuil escreveu: > On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote: >> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: >>> On Sunday 14 November 2010, Hans Verkuil wrote: This patch series converts 24 v4l drivers to unlocked_ioctl. These >> are low hanging fruit but you have to start somewhere :-) The first patch replaces mutex_lock in the V4L2 core by >> mutex_lock_interruptible for most fops. >>> >>> The patches all look good as far as I can tell, but I suppose the >> title is >>> obsolete now that the BKL has been replaced with a v4l-wide mutex, >> which >>> is what you are removing in the series. >> >> I guess I have to rename it, even though strictly speaking the >> branch >> I'm >> working in doesn't have your patch merged yet. >> >> BTW, replacing the BKL with a static mutex is rather scary: the BKL >> gives up >> the lock whenever you sleep, the mutex doesn't. Since sleeping is >> very >> common >> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for >> a >> new frame >> to arrive), this will make it impossible for another process to >> access >> any >> v4l2 device node while the ioctl is sleeping. >> >> I am not sure whether that is what you intended. Or am I missing >> something? > > I was aware that something like this could happen, but I apparently > misjudged how big the impact is. The general pattern for ioctls is > that > those that get called frequently do not sleep, so it can almost > always > be > called with a mutex held. True in general, but most definitely not true for V4L. The all important VIDIOC_DQBUF ioctl will almost always sleep. Mauro, I think this patch will have to be reverted and we just have to do the hard work ourselves. >>> >>> The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L >>> device >>> ready >>> for stream. During the qbuf/dqbuf loop, the only other ioctls that may >>> appear are >>> the control change ioctl's, to adjust things like bright. I doubt that >>> his >>> will >>> cause a really serious trouble. >> >> Yes, it does. Anyone who is using multiple capture/output devices at the >> same time will be affected. > > One correction to your comment: > "Anyone that uses multiple capture/output devices that were not > converted > to unlocked ioctl will be affected." > This means that devices with multiple entries need to be fixed first. No, it will also affect e.g. two bttv cards that you capture from in parallel. Or two webcams, or... We can't just ditch the BKL yet for 2.6.37 IMHO. Perhaps for 2.6.38 if we all work really hard to convert everything. I would prefer to have those drivers that depend on the BKL to have a dependency on CONFIG_LOCK_KERNEL. Drivers should either work or not be available at all, rather than working poorly (if at all). Regards, Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
Em 16-11-2010 10:35, Hans Verkuil escreveu: > >> Em 15-11-2010 07:49, Hans Verkuil escreveu: >>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote: > On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: >> On Sunday 14 November 2010, Hans Verkuil wrote: >>> This patch series converts 24 v4l drivers to unlocked_ioctl. These > are low >>> hanging fruit but you have to start somewhere :-) >>> >>> The first patch replaces mutex_lock in the V4L2 core by > mutex_lock_interruptible >>> for most fops. >> >> The patches all look good as far as I can tell, but I suppose the > title is >> obsolete now that the BKL has been replaced with a v4l-wide mutex, > which >> is what you are removing in the series. > > I guess I have to rename it, even though strictly speaking the branch > I'm > working in doesn't have your patch merged yet. > > BTW, replacing the BKL with a static mutex is rather scary: the BKL > gives up > the lock whenever you sleep, the mutex doesn't. Since sleeping is very > common > in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a > new frame > to arrive), this will make it impossible for another process to access > any > v4l2 device node while the ioctl is sleeping. > > I am not sure whether that is what you intended. Or am I missing > something? I was aware that something like this could happen, but I apparently misjudged how big the impact is. The general pattern for ioctls is that those that get called frequently do not sleep, so it can almost always be called with a mutex held. >>> >>> True in general, but most definitely not true for V4L. The all important >>> VIDIOC_DQBUF ioctl will almost always sleep. >>> >>> Mauro, I think this patch will have to be reverted and we just have to >>> do >>> the hard work ourselves. >> >> The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device >> ready >> for stream. During the qbuf/dqbuf loop, the only other ioctls that may >> appear are >> the control change ioctl's, to adjust things like bright. I doubt that his >> will >> cause a really serious trouble. > > Yes, it does. Anyone who is using multiple capture/output devices at the > same time will be affected. One correction to your comment: "Anyone that uses multiple capture/output devices that were not converted to unlocked ioctl will be affected." This means that devices with multiple entries need to be fixed first. > For example, anyone who uses the davinci > dm6467 driver for both input and output. And yes, that's what we use at > work. And ship to thousands of customers. Or think about surveillance > applications where you are capturing from many streams simultaneously. Cheers, Mauro -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
> Em 15-11-2010 07:49, Hans Verkuil escreveu: >> >>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote: On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: > On Sunday 14 November 2010, Hans Verkuil wrote: >> This patch series converts 24 v4l drivers to unlocked_ioctl. These are low >> hanging fruit but you have to start somewhere :-) >> >> The first patch replaces mutex_lock in the V4L2 core by mutex_lock_interruptible >> for most fops. > > The patches all look good as far as I can tell, but I suppose the title is > obsolete now that the BKL has been replaced with a v4l-wide mutex, which > is what you are removing in the series. I guess I have to rename it, even though strictly speaking the branch I'm working in doesn't have your patch merged yet. BTW, replacing the BKL with a static mutex is rather scary: the BKL gives up the lock whenever you sleep, the mutex doesn't. Since sleeping is very common in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a new frame to arrive), this will make it impossible for another process to access any v4l2 device node while the ioctl is sleeping. I am not sure whether that is what you intended. Or am I missing something? >>> >>> I was aware that something like this could happen, but I apparently >>> misjudged how big the impact is. The general pattern for ioctls is that >>> those that get called frequently do not sleep, so it can almost always >>> be >>> called with a mutex held. >> >> True in general, but most definitely not true for V4L. The all important >> VIDIOC_DQBUF ioctl will almost always sleep. >> >> Mauro, I think this patch will have to be reverted and we just have to >> do >> the hard work ourselves. > > The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device > ready > for stream. During the qbuf/dqbuf loop, the only other ioctls that may > appear are > the control change ioctl's, to adjust things like bright. I doubt that his > will > cause a really serious trouble. Yes, it does. Anyone who is using multiple capture/output devices at the same time will be affected. For example, anyone who uses the davinci dm6467 driver for both input and output. And yes, that's what we use at work. And ship to thousands of customers. Or think about surveillance applications where you are capturing from many streams simultaneously. This change *does* cause serious trouble. > > On the other hand, currently, if BKL is disabled, the entire V4L subsystem > is > disabled. > > So, IMO, the impact of having Arnd's patch applied is less than just > having > almost all drivers disabled if BKL is not compiled. So, I prefer to apply > his patch and then fix it, driver by driver, than to disable the entire > subsystem on .37. Can't we test for CONFIG_LOCK_KERNEL and switch to lock_kernel if set? For now it is just a kernel config option. That seems much more reasonable. Regards, Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
Em 15-11-2010 07:49, Hans Verkuil escreveu: > >> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote: >>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: On Sunday 14 November 2010, Hans Verkuil wrote: > This patch series converts 24 v4l drivers to unlocked_ioctl. These >>> are low > hanging fruit but you have to start somewhere :-) > > The first patch replaces mutex_lock in the V4L2 core by >>> mutex_lock_interruptible > for most fops. The patches all look good as far as I can tell, but I suppose the >>> title is obsolete now that the BKL has been replaced with a v4l-wide mutex, >>> which is what you are removing in the series. >>> >>> I guess I have to rename it, even though strictly speaking the branch >>> I'm >>> working in doesn't have your patch merged yet. >>> >>> BTW, replacing the BKL with a static mutex is rather scary: the BKL >>> gives up >>> the lock whenever you sleep, the mutex doesn't. Since sleeping is very >>> common >>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a >>> new frame >>> to arrive), this will make it impossible for another process to access >>> any >>> v4l2 device node while the ioctl is sleeping. >>> >>> I am not sure whether that is what you intended. Or am I missing >>> something? >> >> I was aware that something like this could happen, but I apparently >> misjudged how big the impact is. The general pattern for ioctls is that >> those that get called frequently do not sleep, so it can almost always be >> called with a mutex held. > > True in general, but most definitely not true for V4L. The all important > VIDIOC_DQBUF ioctl will almost always sleep. > > Mauro, I think this patch will have to be reverted and we just have to do > the hard work ourselves. The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device ready for stream. During the qbuf/dqbuf loop, the only other ioctls that may appear are the control change ioctl's, to adjust things like bright. I doubt that his will cause a really serious trouble. On the other hand, currently, if BKL is disabled, the entire V4L subsystem is disabled. So, IMO, the impact of having Arnd's patch applied is less than just having almost all drivers disabled if BKL is not compiled. So, I prefer to apply his patch and then fix it, driver by driver, than to disable the entire subsystem on .37. Cheers, Mauro -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote: >> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: >> > On Sunday 14 November 2010, Hans Verkuil wrote: >> > > This patch series converts 24 v4l drivers to unlocked_ioctl. These >> are low >> > > hanging fruit but you have to start somewhere :-) >> > > >> > > The first patch replaces mutex_lock in the V4L2 core by >> mutex_lock_interruptible >> > > for most fops. >> > >> > The patches all look good as far as I can tell, but I suppose the >> title is >> > obsolete now that the BKL has been replaced with a v4l-wide mutex, >> which >> > is what you are removing in the series. >> >> I guess I have to rename it, even though strictly speaking the branch >> I'm >> working in doesn't have your patch merged yet. >> >> BTW, replacing the BKL with a static mutex is rather scary: the BKL >> gives up >> the lock whenever you sleep, the mutex doesn't. Since sleeping is very >> common >> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a >> new frame >> to arrive), this will make it impossible for another process to access >> any >> v4l2 device node while the ioctl is sleeping. >> >> I am not sure whether that is what you intended. Or am I missing >> something? > > I was aware that something like this could happen, but I apparently > misjudged how big the impact is. The general pattern for ioctls is that > those that get called frequently do not sleep, so it can almost always be > called with a mutex held. True in general, but most definitely not true for V4L. The all important VIDIOC_DQBUF ioctl will almost always sleep. Mauro, I think this patch will have to be reverted and we just have to do the hard work ourselves. Regards, Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote: > On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: > > On Sunday 14 November 2010, Hans Verkuil wrote: > > > This patch series converts 24 v4l drivers to unlocked_ioctl. These are low > > > hanging fruit but you have to start somewhere :-) > > > > > > The first patch replaces mutex_lock in the V4L2 core by > > > mutex_lock_interruptible > > > for most fops. > > > > The patches all look good as far as I can tell, but I suppose the title is > > obsolete now that the BKL has been replaced with a v4l-wide mutex, which > > is what you are removing in the series. > > I guess I have to rename it, even though strictly speaking the branch I'm > working in doesn't have your patch merged yet. > > BTW, replacing the BKL with a static mutex is rather scary: the BKL gives up > the lock whenever you sleep, the mutex doesn't. Since sleeping is very common > in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a new > frame > to arrive), this will make it impossible for another process to access any > v4l2 device node while the ioctl is sleeping. > > I am not sure whether that is what you intended. Or am I missing something? I was aware that something like this could happen, but I apparently misjudged how big the impact is. The general pattern for ioctls is that those that get called frequently do not sleep, so it can almost always be called with a mutex held. Arnd -- 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
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote: > On Sunday 14 November 2010, Hans Verkuil wrote: > > This patch series converts 24 v4l drivers to unlocked_ioctl. These are low > > hanging fruit but you have to start somewhere :-) > > > > The first patch replaces mutex_lock in the V4L2 core by > > mutex_lock_interruptible > > for most fops. > > The patches all look good as far as I can tell, but I suppose the title is > obsolete now that the BKL has been replaced with a v4l-wide mutex, which > is what you are removing in the series. I guess I have to rename it, even though strictly speaking the branch I'm working in doesn't have your patch merged yet. BTW, replacing the BKL with a static mutex is rather scary: the BKL gives up the lock whenever you sleep, the mutex doesn't. Since sleeping is very common in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a new frame to arrive), this will make it impossible for another process to access *any* v4l2 device node while the ioctl is sleeping. I am not sure whether that is what you intended. Or am I missing something? Regards, Hans -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] V4L BKL removal: first round
On Sunday 14 November 2010, Hans Verkuil wrote: > This patch series converts 24 v4l drivers to unlocked_ioctl. These are low > hanging fruit but you have to start somewhere :-) > > The first patch replaces mutex_lock in the V4L2 core by > mutex_lock_interruptible > for most fops. The patches all look good as far as I can tell, but I suppose the title is obsolete now that the BKL has been replaced with a v4l-wide mutex, which is what you are removing in the series. Arnd -- 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
