RFC: BKL, locking and ioctls

RFC: BKL, locking and ioctls

[Hans Verkuil]

We need to work on getting rid of the BKL, but to do that safely we need a
simple way to convert the many drivers that do not use unlocked_ioctl.

Typically you want to serialize using a mutex. This is trivial to do in the
driver itself for the normal open/read/write/poll/mmap and release fops.

But for unlocked_ioctl it is a bit harder since we like drivers to use
video_ioctl2 directly. And you don't want drivers to put mutex_lock/unlock
calls in every v4l2_ioctl_ops function.

One solution is to add a mutex pointer to struct video_device that
v4l2_unlocked_ioctl can use to do locking:

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 30461cf..44c37e5 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -236,12 +236,18 @@ static long v4l2_unlocked_ioctl(struct file *filp,
                unsigned int cmd, unsigned long arg)
 {
        struct video_device *vdev = video_devdata(filp);
+       int ret;
 
        if (!vdev->fops->unlocked_ioctl)
                return -ENOTTY;
+       if (vdev->ioctl_lock)
+               mutex_lock(vdev->ioctl_lock);
        /* Allow ioctl to continue even if the device was unregistered.
           Things like dequeueing buffers might still be useful. */
-       return vdev->fops->unlocked_ioctl(filp, cmd, arg);
+       ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
+       if (vdev->ioctl_lock)
+               mutex_unlock(vdev->ioctl_lock);
+       return ret;
 }
 
 #ifdef CONFIG_MMU
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 1efcacb..e1ad38a 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -97,6 +97,8 @@ struct video_device
 
        /* ioctl callbacks */
        const struct v4l2_ioctl_ops *ioctl_ops;
+
+       struct mutex *ioctl_lock;
 };
 
 /* dev to video-device */


One area where this may run into problems is with videobuf. The videobuf
subsystem has its own vb_lock, so that will give multiple levels of locking.
More importantly, videobuf can sleep and you don't want to have the global
lock preventing access to the device node.

One option is to let videobuf use the same mutex. However, I don't believe
that is feasible with the current videobuf. Although I hope that this can
be implemented for vb2.

That leaves one other option: the driver has to unlock the global lock before
calling videobuf functions and take the lock again afterwards. I think this is
actually only limited to qbuf and dqbuf so the impact will be small.

Another place where a wait occurs is in v4l2_event_dequeue. But that's part
of the core, so we can unlock ioctl_lock there and lock it afterwards. No
driver changes required.

One other thing that I do not like is this:

        /* Allow ioctl to continue even if the device was unregistered.
           Things like dequeueing buffers might still be useful. */
        return vdev->fops->unlocked_ioctl(filp, cmd, arg);

I do not believe drivers can do anything useful once the device is unregistered
except just close the file handle. There are two exceptions to this: poll()
and VIDIOC_DQEVENT.

Right now drivers have no way of detecting that a disconnect happened. It would
be easy to add a disconnect event and let the core issue it automatically. The
only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
raises an exception. Since all the information regarding events is available in
the core framework it is easy to do this transparently.

So effectively, once a driver unregistered a device node it will never get
called again on that device node except for the release call. That is very
useful for a driver.

And since we can do this in the core, it will also be consistent for all
drivers.

Comments?

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Hi Hans,

Em 19-09-2010 07:29, Hans Verkuil escreveu:
> We need to work on getting rid of the BKL, but to do that safely we need a
> simple way to convert the many drivers that do not use unlocked_ioctl.
> 
> Typically you want to serialize using a mutex. This is trivial to do in the
> driver itself for the normal open/read/write/poll/mmap and release fops.
> 
> But for unlocked_ioctl it is a bit harder since we like drivers to use
> video_ioctl2 directly. And you don't want drivers to put mutex_lock/unlock
> calls in every v4l2_ioctl_ops function.
> 
> One solution is to add a mutex pointer to struct video_device that
> v4l2_unlocked_ioctl can use to do locking:
> 
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 30461cf..44c37e5 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -236,12 +236,18 @@ static long v4l2_unlocked_ioctl(struct file *filp,
>                 unsigned int cmd, unsigned long arg)
>  {
>         struct video_device *vdev = video_devdata(filp);
> +       int ret;
>  
>         if (!vdev->fops->unlocked_ioctl)
>                 return -ENOTTY;
> +       if (vdev->ioctl_lock)
> +               mutex_lock(vdev->ioctl_lock);
>         /* Allow ioctl to continue even if the device was unregistered.
>            Things like dequeueing buffers might still be useful. */
> -       return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> +       ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> +       if (vdev->ioctl_lock)
> +               mutex_unlock(vdev->ioctl_lock);
> +       return ret;
>  }
>  
>  #ifdef CONFIG_MMU
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 1efcacb..e1ad38a 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -97,6 +97,8 @@ struct video_device
>  
>         /* ioctl callbacks */
>         const struct v4l2_ioctl_ops *ioctl_ops;
> +
> +       struct mutex *ioctl_lock;
>  };
>  
>  /* dev to video-device */

As I comment with you on IRC, I'm working on it during this weekend. 

A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
more than one stream per interface.

So, I did a different implementation, implementing the mutex pointer per file handler.
On devices that a simple lock is possible, all you need to do is to use the same locking
for all file handles, but if drivers want a finer control, they can use a per-file handler
lock.

I'm adding the patches I did at media-tree.git. I've created a separate branch there (devel/bkl):
	http://git.linuxtv.org/media_tree.git?a=shortlog;h=refs/heads/devel/bkl

I've already applied there the other BKL-lock removal patches I've sent before, plus one new
one, fixing a lock unbalance at bttv poll function (changeset 32d1c90c85).

The v4l2 core patches are at:

http://git.linuxtv.org/media_tree.git?a=commit;h=285267378581fbf852f24f3f99d2e937cd200fd5
http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d

The approach I took serializes open, close, ioctl, mmap, read and poll, e. g. all file operations
done by the V4L devices.

> One area where this may run into problems is with videobuf. The videobuf
> subsystem has its own vb_lock, so that will give multiple levels of locking.
> More importantly, videobuf can sleep and you don't want to have the global
> lock preventing access to the device node.
> 
> One option is to let videobuf use the same mutex. However, I don't believe
> that is feasible with the current videobuf. Although I hope that this can
> be implemented for vb2.
> 
> That leaves one other option: the driver has to unlock the global lock before
> calling videobuf functions and take the lock again afterwards. I think this is
> actually only limited to qbuf and dqbuf so the impact will be small.
>
> Another place where a wait occurs is in v4l2_event_dequeue. But that's part
> of the core, so we can unlock ioctl_lock there and lock it afterwards. No
> driver changes required.

I did a similar patch to videobuf, allowing an optional lock at videobuf:

http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
http://git.linuxtv.org/media_tree.git?a=commit;h=d14bb839803b662604de627451fe19daa697d1dc

As all mutex-dependent videobuf operations happen during the call of one of the fops, there's
no need of an explicit call inside videobuf.

In order to test it, I've ported two drivers: vivi and em28xx:

http://git.linuxtv.org/media_tree.git?a=commit;h=7ddc1b6ef803014f6ed297c391e774d044d72f9d
http://git.linuxtv.org/media_tree.git?a=commit;h=b59117ed27706bf6059eeabf2698d1d33e2e67d0

On both cases, the lock seems to be enough. I even removed em28xx while streaming, with 
mplayer reproducing the stream and with qv4l2 running. I didn't notice a single issue.

We could need to do some changes there to cover the case where videobuf sleeps, maybe using
mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
to fill the buffers (tests are needed).

> One other thing that I do not like is this:
> 
>         /* Allow ioctl to continue even if the device was unregistered.
>            Things like dequeueing buffers might still be useful. */
>         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> 
> I do not believe drivers can do anything useful once the device is unregistered
> except just close the file handle. There are two exceptions to this: poll()
> and VIDIOC_DQEVENT.
> 
> Right now drivers have no way of detecting that a disconnect happened. It would
> be easy to add a disconnect event and let the core issue it automatically. The
> only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
> raises an exception. Since all the information regarding events is available in
> the core framework it is easy to do this transparently.
> 
> So effectively, once a driver unregistered a device node it will never get
> called again on that device node except for the release call. That is very
> useful for a driver.
> 
> And since we can do this in the core, it will also be consistent for all
> drivers.

I think we should implement a way to detect disconnections. This will allow simplifying the
code at the drivers. Yet, I don't think that the solution is (only) to create an
event. Instead, we need to see how this information could be retrieved from the bus.
As the normal case for disconnections is for USB devices, we basically need to implement
a callback when a diconnection happens. The USB core knows about that, but I don't know
if it provides a callback for it. If it provides, drivers may just implement the callback,
calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
can then properly handle any new fops to that device, passing to the device just the
close() events, returning -ENODEV and POLLERR for userspace.

Cheers,
Mauro

PS.: I'll review your BKL removal patches and apply there, for us to have a common place for the
BKL patches, before being ready to merge all of them at the staging branch.

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em 19-09-2010 07:29, Hans Verkuil escreveu:
> > We need to work on getting rid of the BKL, but to do that safely we need a
> > simple way to convert the many drivers that do not use unlocked_ioctl.
> > 
> > Typically you want to serialize using a mutex. This is trivial to do in the
> > driver itself for the normal open/read/write/poll/mmap and release fops.
> > 
> > But for unlocked_ioctl it is a bit harder since we like drivers to use
> > video_ioctl2 directly. And you don't want drivers to put mutex_lock/unlock
> > calls in every v4l2_ioctl_ops function.
> > 
> > One solution is to add a mutex pointer to struct video_device that
> > v4l2_unlocked_ioctl can use to do locking:
> > 
> > diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> > index 30461cf..44c37e5 100644
> > --- a/drivers/media/video/v4l2-dev.c
> > +++ b/drivers/media/video/v4l2-dev.c
> > @@ -236,12 +236,18 @@ static long v4l2_unlocked_ioctl(struct file *filp,
> >                 unsigned int cmd, unsigned long arg)
> >  {
> >         struct video_device *vdev = video_devdata(filp);
> > +       int ret;
> >  
> >         if (!vdev->fops->unlocked_ioctl)
> >                 return -ENOTTY;
> > +       if (vdev->ioctl_lock)
> > +               mutex_lock(vdev->ioctl_lock);
> >         /* Allow ioctl to continue even if the device was unregistered.
> >            Things like dequeueing buffers might still be useful. */
> > -       return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > +       ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > +       if (vdev->ioctl_lock)
> > +               mutex_unlock(vdev->ioctl_lock);
> > +       return ret;
> >  }
> >  
> >  #ifdef CONFIG_MMU
> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > index 1efcacb..e1ad38a 100644
> > --- a/include/media/v4l2-dev.h
> > +++ b/include/media/v4l2-dev.h
> > @@ -97,6 +97,8 @@ struct video_device
> >  
> >         /* ioctl callbacks */
> >         const struct v4l2_ioctl_ops *ioctl_ops;
> > +
> > +       struct mutex *ioctl_lock;
> >  };
> >  
> >  /* dev to video-device */
> 
> As I comment with you on IRC, I'm working on it during this weekend. 
> 
> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
> more than one stream per interface.

My proposal is actually a lock per device node, not per device (although that's
what many simple drivers probably will use).
 
> So, I did a different implementation, implementing the mutex pointer per file handler.
> On devices that a simple lock is possible, all you need to do is to use the same locking
> for all file handles, but if drivers want a finer control, they can use a per-file handler
> lock.

I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
you need to serialize for a single filehandle (which would only be needed for multithreaded
applications where the threads use the same filehandle), then you definitely need to serialize
between multiple file handles that are open on the same device node.

The device node is the right place for this IMHO.

Regarding creating v4l2_fh structs in the core: many simple drivers do not need a v4l2_fh
at all, and the more complex drivers often need to embed it in a larger struct.

The lookup get_v4l2_fh function also is unnecessary if we do not create these structs and
so is the *really* ugly reinit_v4l2_fh.

> I'm adding the patches I did at media-tree.git. I've created a separate branch there (devel/bkl):
> 	http://git.linuxtv.org/media_tree.git?a=shortlog;h=refs/heads/devel/bkl
> 
> I've already applied there the other BKL-lock removal patches I've sent before, plus one new
> one, fixing a lock unbalance at bttv poll function (changeset 32d1c90c85).
> 
> The v4l2 core patches are at:
> 
> http://git.linuxtv.org/media_tree.git?a=commit;h=285267378581fbf852f24f3f99d2e937cd200fd5
> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
> 
> The approach I took serializes open, close, ioctl, mmap, read and poll, e. g. all file operations
> done by the V4L devices.
> 
> > One area where this may run into problems is with videobuf. The videobuf
> > subsystem has its own vb_lock, so that will give multiple levels of locking.
> > More importantly, videobuf can sleep and you don't want to have the global
> > lock preventing access to the device node.
> > 
> > One option is to let videobuf use the same mutex. However, I don't believe
> > that is feasible with the current videobuf. Although I hope that this can
> > be implemented for vb2.
> > 
> > That leaves one other option: the driver has to unlock the global lock before
> > calling videobuf functions and take the lock again afterwards. I think this is
> > actually only limited to qbuf and dqbuf so the impact will be small.
> >
> > Another place where a wait occurs is in v4l2_event_dequeue. But that's part
> > of the core, so we can unlock ioctl_lock there and lock it afterwards. No
> > driver changes required.
> 
> I did a similar patch to videobuf, allowing an optional lock at videobuf:
> 
> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
> http://git.linuxtv.org/media_tree.git?a=commit;h=d14bb839803b662604de627451fe19daa697d1dc
> 
> As all mutex-dependent videobuf operations happen during the call of one of the fops, there's
> no need of an explicit call inside videobuf.
> 
> In order to test it, I've ported two drivers: vivi and em28xx:
> 
> http://git.linuxtv.org/media_tree.git?a=commit;h=7ddc1b6ef803014f6ed297c391e774d044d72f9d
> http://git.linuxtv.org/media_tree.git?a=commit;h=b59117ed27706bf6059eeabf2698d1d33e2e67d0
> 
> On both cases, the lock seems to be enough. I even removed em28xx while streaming, with 
> mplayer reproducing the stream and with qv4l2 running. I didn't notice a single issue.
> 
> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
> to fill the buffers (tests are needed).

Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
release the lock in waiton. Right now no other access can be done while it is waiting. That
is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
is easy to solve there.

For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
we can do better in vb2.

My suggestion would be to use your videobuf patches, but use my idea for the mutex pointer
in struct video_device. Best of both worlds :-)

> > One other thing that I do not like is this:
> > 
> >         /* Allow ioctl to continue even if the device was unregistered.
> >            Things like dequeueing buffers might still be useful. */
> >         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > 
> > I do not believe drivers can do anything useful once the device is unregistered
> > except just close the file handle. There are two exceptions to this: poll()
> > and VIDIOC_DQEVENT.
> > 
> > Right now drivers have no way of detecting that a disconnect happened. It would
> > be easy to add a disconnect event and let the core issue it automatically. The
> > only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
> > raises an exception. Since all the information regarding events is available in
> > the core framework it is easy to do this transparently.
> > 
> > So effectively, once a driver unregistered a device node it will never get
> > called again on that device node except for the release call. That is very
> > useful for a driver.
> > 
> > And since we can do this in the core, it will also be consistent for all
> > drivers.
> 
> I think we should implement a way to detect disconnections. This will allow simplifying the
> code at the drivers. Yet, I don't think that the solution is (only) to create an
> event. Instead, we need to see how this information could be retrieved from the bus.
> As the normal case for disconnections is for USB devices, we basically need to implement
> a callback when a diconnection happens. The USB core knows about that, but I don't know
> if it provides a callback for it.

Well, USB drivers have a disconnect callback. All V4L2 USB drivers hook into that.
What they are supposed to do is to unregister all video nodes. That sets the 'unregistered'
in the core preventing any further access.

> If it provides, drivers may just implement the callback,
> calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
> can then properly handle any new fops to that device, passing to the device just the
> close() events, returning -ENODEV and POLLERR for userspace.

That basically is what happens right now. Except for passing on the ioctls which is a bad
idea IMHO.

BTW: one other thing I've worked on today is a global release callback in v4l2_device.
Right now we have proper refcounting for struct video_device, but if a driver has multiple
device nodes, then it can be hard to tell when all of them are properly released and it
is safe to release the full device instance.

I made a fairly straightforward implementation available here:

http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core

Without a global release it is almost impossible to cleanup a driver like usbvision
correctly.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Andy Walls]

The device node isn't even the right place for drivers that provide multiple device nodes that can possibly access the same underlying data or register sets.

Any core/infrastructure approach is likely doomed in the general case.  It's trying to protect data and registers in a driver it knows nothing about, by protecting the *code paths* that take essentially unknown actions on that data and registers. :{

Videobuf is the right place to protect videobuf data.
Otherwise the driver really needs to handle protecting things, as it's the only code with full knowledge of the data structures and register blocks in use.

"Protect the data, not the code" -Alan Cox on LKML

R,
Andy


Hans Verkuil <hverkuil@xs4all.nl> wrote:

>On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
>> Hi Hans,
>> 
>> Em 19-09-2010 07:29, Hans Verkuil escreveu:
>> > We need to work on getting rid of the BKL, but to do that safely we need a
>> > simple way to convert the many drivers that do not use unlocked_ioctl.
>> > 
>> > Typically you want to serialize using a mutex. This is trivial to do in the
>> > driver itself for the normal open/read/write/poll/mmap and release fops.
>> > 
>> > But for unlocked_ioctl it is a bit harder since we like drivers to use
>> > video_ioctl2 directly. And you don't want drivers to put mutex_lock/unlock
>> > calls in every v4l2_ioctl_ops function.
>> > 
>> > One solution is to add a mutex pointer to struct video_device that
>> > v4l2_unlocked_ioctl can use to do locking:
>> > 
>> > diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
>> > index 30461cf..44c37e5 100644
>> > --- a/drivers/media/video/v4l2-dev.c
>> > +++ b/drivers/media/video/v4l2-dev.c
>> > @@ -236,12 +236,18 @@ static long v4l2_unlocked_ioctl(struct file *filp,
>> >                 unsigned int cmd, unsigned long arg)
>> >  {
>> >         struct video_device *vdev = video_devdata(filp);
>> > +       int ret;
>> >  
>> >         if (!vdev->fops->unlocked_ioctl)
>> >                 return -ENOTTY;
>> > +       if (vdev->ioctl_lock)
>> > +               mutex_lock(vdev->ioctl_lock);
>> >         /* Allow ioctl to continue even if the device was unregistered.
>> >            Things like dequeueing buffers might still be useful. */
>> > -       return vdev->fops->unlocked_ioctl(filp, cmd, arg);
>> > +       ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
>> > +       if (vdev->ioctl_lock)
>> > +               mutex_unlock(vdev->ioctl_lock);
>> > +       return ret;
>> >  }
>> >  
>> >  #ifdef CONFIG_MMU
>> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> > index 1efcacb..e1ad38a 100644
>> > --- a/include/media/v4l2-dev.h
>> > +++ b/include/media/v4l2-dev.h
>> > @@ -97,6 +97,8 @@ struct video_device
>> >  
>> >         /* ioctl callbacks */
>> >         const struct v4l2_ioctl_ops *ioctl_ops;
>> > +
>> > +       struct mutex *ioctl_lock;
>> >  };
>> >  
>> >  /* dev to video-device */
>> 
>> As I comment with you on IRC, I'm working on it during this weekend. 
>> 
>> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
>> more than one stream per interface.
>
>My proposal is actually a lock per device node, not per device (although that's
>what many simple drivers probably will use).
> 
>> So, I did a different implementation, implementing the mutex pointer per file handler.
>> On devices that a simple lock is possible, all you need to do is to use the same locking
>> for all file handles, but if drivers want a finer control, they can use a per-file handler
>> lock.
>
>I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
>you need to serialize for a single filehandle (which would only be needed for multithreaded
>applications where the threads use the same filehandle), then you definitely need to serialize
>between multiple file handles that are open on the same device node.
>
>The device node is the right place for this IMHO.
>
>Regarding creating v4l2_fh structs in the core: many simple drivers do not need a v4l2_fh
>at all, and the more complex drivers often need to embed it in a larger struct.
>
>The lookup get_v4l2_fh function also is unnecessary if we do not create these structs and
>so is the *really* ugly reinit_v4l2_fh.
>
>> I'm adding the patches I did at media-tree.git. I've created a separate branch there (devel/bkl):
>> 	http://git.linuxtv.org/media_tree.git?a=shortlog;h=refs/heads/devel/bkl
>> 
>> I've already applied there the other BKL-lock removal patches I've sent before, plus one new
>> one, fixing a lock unbalance at bttv poll function (changeset 32d1c90c85).
>> 
>> The v4l2 core patches are at:
>> 
>> http://git.linuxtv.org/media_tree.git?a=commit;h=285267378581fbf852f24f3f99d2e937cd200fd5
>> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
>> 
>> The approach I took serializes open, close, ioctl, mmap, read and poll, e. g. all file operations
>> done by the V4L devices.
>> 
>> > One area where this may run into problems is with videobuf. The videobuf
>> > subsystem has its own vb_lock, so that will give multiple levels of locking.
>> > More importantly, videobuf can sleep and you don't want to have the global
>> > lock preventing access to the device node.
>> > 
>> > One option is to let videobuf use the same mutex. However, I don't believe
>> > that is feasible with the current videobuf. Although I hope that this can
>> > be implemented for vb2.
>> > 
>> > That leaves one other option: the driver has to unlock the global lock before
>> > calling videobuf functions and take the lock again afterwards. I think this is
>> > actually only limited to qbuf and dqbuf so the impact will be small.
>> >
>> > Another place where a wait occurs is in v4l2_event_dequeue. But that's part
>> > of the core, so we can unlock ioctl_lock there and lock it afterwards. No
>> > driver changes required.
>> 
>> I did a similar patch to videobuf, allowing an optional lock at videobuf:
>> 
>> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
>> http://git.linuxtv.org/media_tree.git?a=commit;h=d14bb839803b662604de627451fe19daa697d1dc
>> 
>> As all mutex-dependent videobuf operations happen during the call of one of the fops, there's
>> no need of an explicit call inside videobuf.
>> 
>> In order to test it, I've ported two drivers: vivi and em28xx:
>> 
>> http://git.linuxtv.org/media_tree.git?a=commit;h=7ddc1b6ef803014f6ed297c391e774d044d72f9d
>> http://git.linuxtv.org/media_tree.git?a=commit;h=b59117ed27706bf6059eeabf2698d1d33e2e67d0
>> 
>> On both cases, the lock seems to be enough. I even removed em28xx while streaming, with 
>> mplayer reproducing the stream and with qv4l2 running. I didn't notice a single issue.
>> 
>> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
>> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
>> to fill the buffers (tests are needed).
>
>Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
>release the lock in waiton. Right now no other access can be done while it is waiting. That
>is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
>is easy to solve there.
>
>For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
>Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
>we can do better in vb2.
>
>My suggestion would be to use your videobuf patches, but use my idea for the mutex pointer
>in struct video_device. Best of both worlds :-)
>
>> > One other thing that I do not like is this:
>> > 
>> >         /* Allow ioctl to continue even if the device was unregistered.
>> >            Things like dequeueing buffers might still be useful. */
>> >         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
>> > 
>> > I do not believe drivers can do anything useful once the device is unregistered
>> > except just close the file handle. There are two exceptions to this: poll()
>> > and VIDIOC_DQEVENT.
>> > 
>> > Right now drivers have no way of detecting that a disconnect happened. It would
>> > be easy to add a disconnect event and let the core issue it automatically. The
>> > only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
>> > raises an exception. Since all the information regarding events is available in
>> > the core framework it is easy to do this transparently.
>> > 
>> > So effectively, once a driver unregistered a device node it will never get
>> > called again on that device node except for the release call. That is very
>> > useful for a driver.
>> > 
>> > And since we can do this in the core, it will also be consistent for all
>> > drivers.
>> 
>> I think we should implement a way to detect disconnections. This will allow simplifying the
>> code at the drivers. Yet, I don't think that the solution is (only) to create an
>> event. Instead, we need to see how this information could be retrieved from the bus.
>> As the normal case for disconnections is for USB devices, we basically need to implement
>> a callback when a diconnection happens. The USB core knows about that, but I don't know
>> if it provides a callback for it.
>
>Well, USB drivers have a disconnect callback. All V4L2 USB drivers hook into that.
>What they are supposed to do is to unregister all video nodes. That sets the 'unregistered'
>in the core preventing any further access.
>
>> If it provides, drivers may just implement the callback,
>> calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
>> can then properly handle any new fops to that device, passing to the device just the
>> close() events, returning -ENODEV and POLLERR for userspace.
>
>That basically is what happens right now. Except for passing on the ioctls which is a bad
>idea IMHO.
>
>BTW: one other thing I've worked on today is a global release callback in v4l2_device.
>Right now we have proper refcounting for struct video_device, but if a driver has multiple
>device nodes, then it can be hard to tell when all of them are properly released and it
>is safe to release the full device instance.
>
>I made a fairly straightforward implementation available here:
>
>http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core
>
>Without a global release it is almost impossible to cleanup a driver like usbvision
>correctly.
>
>Regards,
>
>	Hans
>
>-- 
>Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
> The device node isn't even the right place for drivers that provide multiple device nodes that can possibly access the same underlying data or register sets.
> 
> Any core/infrastructure approach is likely doomed in the general case.  It's trying to protect data and registers in a driver it knows nothing about, by protecting the *code paths* that take essentially unknown actions on that data and registers. :{

Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
itself can be either at the top-level device or associated with the actual
video device, depending on the requirements of the driver.

> Videobuf is the right place to protect videobuf data.

vb_lock is AFAIK there to protect the streaming of data. And that's definitely
per device node since only one filehandle per device node can do streaming.

Also remember that we are trying to get rid of the BKL, so staying bug-compatible
is enough for a first version :-)

Regards,

	Hans

> Otherwise the driver really needs to handle protecting things, as it's the only code with full knowledge of the data structures and register blocks in use.
> 
> "Protect the data, not the code" -Alan Cox on LKML
> 
> R,
> Andy
> 
> 
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> >On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
> >> Hi Hans,
> >> 
> >> Em 19-09-2010 07:29, Hans Verkuil escreveu:
> >> > We need to work on getting rid of the BKL, but to do that safely we need a
> >> > simple way to convert the many drivers that do not use unlocked_ioctl.
> >> > 
> >> > Typically you want to serialize using a mutex. This is trivial to do in the
> >> > driver itself for the normal open/read/write/poll/mmap and release fops.
> >> > 
> >> > But for unlocked_ioctl it is a bit harder since we like drivers to use
> >> > video_ioctl2 directly. And you don't want drivers to put mutex_lock/unlock
> >> > calls in every v4l2_ioctl_ops function.
> >> > 
> >> > One solution is to add a mutex pointer to struct video_device that
> >> > v4l2_unlocked_ioctl can use to do locking:
> >> > 
> >> > diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> >> > index 30461cf..44c37e5 100644
> >> > --- a/drivers/media/video/v4l2-dev.c
> >> > +++ b/drivers/media/video/v4l2-dev.c
> >> > @@ -236,12 +236,18 @@ static long v4l2_unlocked_ioctl(struct file *filp,
> >> >                 unsigned int cmd, unsigned long arg)
> >> >  {
> >> >         struct video_device *vdev = video_devdata(filp);
> >> > +       int ret;
> >> >  
> >> >         if (!vdev->fops->unlocked_ioctl)
> >> >                 return -ENOTTY;
> >> > +       if (vdev->ioctl_lock)
> >> > +               mutex_lock(vdev->ioctl_lock);
> >> >         /* Allow ioctl to continue even if the device was unregistered.
> >> >            Things like dequeueing buffers might still be useful. */
> >> > -       return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> >> > +       ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> >> > +       if (vdev->ioctl_lock)
> >> > +               mutex_unlock(vdev->ioctl_lock);
> >> > +       return ret;
> >> >  }
> >> >  
> >> >  #ifdef CONFIG_MMU
> >> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> >> > index 1efcacb..e1ad38a 100644
> >> > --- a/include/media/v4l2-dev.h
> >> > +++ b/include/media/v4l2-dev.h
> >> > @@ -97,6 +97,8 @@ struct video_device
> >> >  
> >> >         /* ioctl callbacks */
> >> >         const struct v4l2_ioctl_ops *ioctl_ops;
> >> > +
> >> > +       struct mutex *ioctl_lock;
> >> >  };
> >> >  
> >> >  /* dev to video-device */
> >> 
> >> As I comment with you on IRC, I'm working on it during this weekend. 
> >> 
> >> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
> >> more than one stream per interface.
> >
> >My proposal is actually a lock per device node, not per device (although that's
> >what many simple drivers probably will use).
> > 
> >> So, I did a different implementation, implementing the mutex pointer per file handler.
> >> On devices that a simple lock is possible, all you need to do is to use the same locking
> >> for all file handles, but if drivers want a finer control, they can use a per-file handler
> >> lock.
> >
> >I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
> >you need to serialize for a single filehandle (which would only be needed for multithreaded
> >applications where the threads use the same filehandle), then you definitely need to serialize
> >between multiple file handles that are open on the same device node.
> >
> >The device node is the right place for this IMHO.
> >
> >Regarding creating v4l2_fh structs in the core: many simple drivers do not need a v4l2_fh
> >at all, and the more complex drivers often need to embed it in a larger struct.
> >
> >The lookup get_v4l2_fh function also is unnecessary if we do not create these structs and
> >so is the *really* ugly reinit_v4l2_fh.
> >
> >> I'm adding the patches I did at media-tree.git. I've created a separate branch there (devel/bkl):
> >> 	http://git.linuxtv.org/media_tree.git?a=shortlog;h=refs/heads/devel/bkl
> >> 
> >> I've already applied there the other BKL-lock removal patches I've sent before, plus one new
> >> one, fixing a lock unbalance at bttv poll function (changeset 32d1c90c85).
> >> 
> >> The v4l2 core patches are at:
> >> 
> >> http://git.linuxtv.org/media_tree.git?a=commit;h=285267378581fbf852f24f3f99d2e937cd200fd5
> >> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
> >> 
> >> The approach I took serializes open, close, ioctl, mmap, read and poll, e. g. all file operations
> >> done by the V4L devices.
> >> 
> >> > One area where this may run into problems is with videobuf. The videobuf
> >> > subsystem has its own vb_lock, so that will give multiple levels of locking.
> >> > More importantly, videobuf can sleep and you don't want to have the global
> >> > lock preventing access to the device node.
> >> > 
> >> > One option is to let videobuf use the same mutex. However, I don't believe
> >> > that is feasible with the current videobuf. Although I hope that this can
> >> > be implemented for vb2.
> >> > 
> >> > That leaves one other option: the driver has to unlock the global lock before
> >> > calling videobuf functions and take the lock again afterwards. I think this is
> >> > actually only limited to qbuf and dqbuf so the impact will be small.
> >> >
> >> > Another place where a wait occurs is in v4l2_event_dequeue. But that's part
> >> > of the core, so we can unlock ioctl_lock there and lock it afterwards. No
> >> > driver changes required.
> >> 
> >> I did a similar patch to videobuf, allowing an optional lock at videobuf:
> >> 
> >> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
> >> http://git.linuxtv.org/media_tree.git?a=commit;h=d14bb839803b662604de627451fe19daa697d1dc
> >> 
> >> As all mutex-dependent videobuf operations happen during the call of one of the fops, there's
> >> no need of an explicit call inside videobuf.
> >> 
> >> In order to test it, I've ported two drivers: vivi and em28xx:
> >> 
> >> http://git.linuxtv.org/media_tree.git?a=commit;h=7ddc1b6ef803014f6ed297c391e774d044d72f9d
> >> http://git.linuxtv.org/media_tree.git?a=commit;h=b59117ed27706bf6059eeabf2698d1d33e2e67d0
> >> 
> >> On both cases, the lock seems to be enough. I even removed em28xx while streaming, with 
> >> mplayer reproducing the stream and with qv4l2 running. I didn't notice a single issue.
> >> 
> >> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
> >> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
> >> to fill the buffers (tests are needed).
> >
> >Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
> >release the lock in waiton. Right now no other access can be done while it is waiting. That
> >is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
> >is easy to solve there.
> >
> >For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
> >Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
> >we can do better in vb2.
> >
> >My suggestion would be to use your videobuf patches, but use my idea for the mutex pointer
> >in struct video_device. Best of both worlds :-)
> >
> >> > One other thing that I do not like is this:
> >> > 
> >> >         /* Allow ioctl to continue even if the device was unregistered.
> >> >            Things like dequeueing buffers might still be useful. */
> >> >         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> >> > 
> >> > I do not believe drivers can do anything useful once the device is unregistered
> >> > except just close the file handle. There are two exceptions to this: poll()
> >> > and VIDIOC_DQEVENT.
> >> > 
> >> > Right now drivers have no way of detecting that a disconnect happened. It would
> >> > be easy to add a disconnect event and let the core issue it automatically. The
> >> > only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
> >> > raises an exception. Since all the information regarding events is available in
> >> > the core framework it is easy to do this transparently.
> >> > 
> >> > So effectively, once a driver unregistered a device node it will never get
> >> > called again on that device node except for the release call. That is very
> >> > useful for a driver.
> >> > 
> >> > And since we can do this in the core, it will also be consistent for all
> >> > drivers.
> >> 
> >> I think we should implement a way to detect disconnections. This will allow simplifying the
> >> code at the drivers. Yet, I don't think that the solution is (only) to create an
> >> event. Instead, we need to see how this information could be retrieved from the bus.
> >> As the normal case for disconnections is for USB devices, we basically need to implement
> >> a callback when a diconnection happens. The USB core knows about that, but I don't know
> >> if it provides a callback for it.
> >
> >Well, USB drivers have a disconnect callback. All V4L2 USB drivers hook into that.
> >What they are supposed to do is to unregister all video nodes. That sets the 'unregistered'
> >in the core preventing any further access.
> >
> >> If it provides, drivers may just implement the callback,
> >> calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
> >> can then properly handle any new fops to that device, passing to the device just the
> >> close() events, returning -ENODEV and POLLERR for userspace.
> >
> >That basically is what happens right now. Except for passing on the ioctls which is a bad
> >idea IMHO.
> >
> >BTW: one other thing I've worked on today is a global release callback in v4l2_device.
> >Right now we have proper refcounting for struct video_device, but if a driver has multiple
> >device nodes, then it can be hard to tell when all of them are properly released and it
> >is safe to release the full device instance.
> >
> >I made a fairly straightforward implementation available here:
> >
> >http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core
> >
> >Without a global release it is almost impossible to cleanup a driver like usbvision
> >correctly.
> >
> >Regards,
> >
> >	Hans
> >
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 13:10, Hans Verkuil escreveu:
> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
>> The device node isn't even the right place for drivers that provide multiple device nodes that can possibly access the same underlying data or register sets.
>>
>> Any core/infrastructure approach is likely doomed in the general case.  It's trying to protect data and registers in a driver it knows nothing about, by protecting the *code paths* that take essentially unknown actions on that data and registers. :{
> 
> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
> itself can be either at the top-level device or associated with the actual
> video device, depending on the requirements of the driver.
> 
>> Videobuf is the right place to protect videobuf data.
> 
> vb_lock is AFAIK there to protect the streaming of data.

True, but part of this data is outside videobuf struct. So, a mutex inside videobuf is not enough.
See for example the tricks that bttv-driver need to do in order to protect data. I suspect that
there are even some race issues there, since it needs to unlock struct bttv in order to get videobuf
lock on some places.

> And that's definitely
> per device node since only one filehandle per device node can do streaming.

That's a wrong assumption. There are some drivers, like bttv, cx88 and saa7134 that allows streaming
on two separate file handlers, using the same device. This is valid according with V4L2 spec, and
some applications, like xawtv and xdtv assumes this behavior, when you try to record a video.

Cheers,
Mauro

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 11:58, Hans Verkuil escreveu:
> On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:

>> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
>> more than one stream per interface.
> 
> My proposal is actually a lock per device node, not per device (although that's
> what many simple drivers probably will use).

Yes, that's what I meant. However, V4L2 API allows multiple opens and multiple streams per
device node (and this is actually in use by several drivers).

>> So, I did a different implementation, implementing the mutex pointer per file handler.
>> On devices that a simple lock is possible, all you need to do is to use the same locking
>> for all file handles, but if drivers want a finer control, they can use a per-file handler
>> lock.
> 
> I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
> you need to serialize for a single filehandle (which would only be needed for multithreaded
> applications where the threads use the same filehandle), then you definitely need to serialize
> between multiple file handles that are open on the same device node.

On multithread apps, they'll share the same file handle, so, there's no issue. Some applications
like xawtv and xdtv allows recording a video by starting another proccess that will use the read() 
interface for one stream, while the other stream is using mmap() (or overlay) will have two different
file handlers, one for each app. That's said, a driver using per-fh locks will likely need to
have an additional lock for global resources. I didn't start porting cx88 driver, but I suspect
that it will need to use it.

> The device node is the right place for this IMHO.
> 
> Regarding creating v4l2_fh structs in the core: many simple drivers do not need a v4l2_fh
> at all, and the more complex drivers often need to embed it in a larger struct.
> 
> The lookup get_v4l2_fh function also is unnecessary if we do not create these structs and
> so is the *really* ugly reinit_v4l2_fh.

The reinit_v4l2_fh() is a temporary workaround, to avoid the need of rewriting the v4l2_fh
implementation at ivtv, while keeping it work properly.

There are other ways to allow drivers to embed a per-fh data for the more complex devices, like
adding a void *priv pointer, and letting the drivers to do whatever they want.

The problem with the current implementation of v4l2_fh() is that there's no way for the core
to get per-fh info.

>> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
>> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
>> to fill the buffers (tests are needed).
> 
> Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
> release the lock in waiton. 

Well, mutex is allowed to be locked while scheduling. That's why it uses a mutex, instead of a
spinlock. So, it seems to be safe. Yet, I agree that it should be releasing it at waiton, 
in order to avoid troubles with some kernel threads that may be needing to lock to access
the same data. So, the code will likely need to be changed in order to work with some drivers.

> Right now no other access can be done while it is waiting. That
> is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
> is easy to solve there.
> 
> For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
> Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
> we can do better in vb2.

Having two mutexes passing as parameter for vb would be a confusing interface. Maybe the better is
to postpone such change to happen after having all drivers ported to the new locking schema.

> My suggestion would be to use your videobuf patches, but use my idea for the mutex pointer
> in struct video_device. Best of both worlds :-)

Maybe.

>>> One other thing that I do not like is this:
>>>
>>>         /* Allow ioctl to continue even if the device was unregistered.
>>>            Things like dequeueing buffers might still be useful. */
>>>         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
>>>
>>> I do not believe drivers can do anything useful once the device is unregistered
>>> except just close the file handle. There are two exceptions to this: poll()
>>> and VIDIOC_DQEVENT.
>>>
>>> Right now drivers have no way of detecting that a disconnect happened. It would
>>> be easy to add a disconnect event and let the core issue it automatically. The
>>> only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
>>> raises an exception. Since all the information regarding events is available in
>>> the core framework it is easy to do this transparently.
>>>
>>> So effectively, once a driver unregistered a device node it will never get
>>> called again on that device node except for the release call. That is very
>>> useful for a driver.
>>>
>>> And since we can do this in the core, it will also be consistent for all
>>> drivers.
>>
>> I think we should implement a way to detect disconnections. This will allow simplifying the
>> code at the drivers. Yet, I don't think that the solution is (only) to create an
>> event. Instead, we need to see how this information could be retrieved from the bus.
>> As the normal case for disconnections is for USB devices, we basically need to implement
>> a callback when a diconnection happens. The USB core knows about that, but I don't know
>> if it provides a callback for it.
> 
> Well, USB drivers have a disconnect callback. All V4L2 USB drivers hook into that.
> What they are supposed to do is to unregister all video nodes. That sets the 'unregistered'
> in the core preventing any further access.

I don't think all drivers implement it. I need to double check.

>> If it provides, drivers may just implement the callback,
>> calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
>> can then properly handle any new fops to that device, passing to the device just the
>> close() events, returning -ENODEV and POLLERR for userspace.
> 
> That basically is what happens right now. Except for passing on the ioctls which is a bad
> idea IMHO.

Yeah. If core already knows that the device got disconnected, it can just use a default handler
for disconnected devices, instead of passing ioctls to the drivers.

> BTW: one other thing I've worked on today is a global release callback in v4l2_device.
> Right now we have proper refcounting for struct video_device, but if a driver has multiple
> device nodes, then it can be hard to tell when all of them are properly released and it
> is safe to release the full device instance.
> 
> I made a fairly straightforward implementation available here:
> 
> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core
> 
> Without a global release it is almost impossible to cleanup a driver like usbvision
> correctly.

It seems interesting. Not sure how we can use it on drivers like cx88, but it is probably ok
for most of the drivers. In the case of cx88, the refcount should be done at cx88 core struct,
that it is used by 4 different drivers (3 of them opening V4L devices: cx88, cx88-mpeg, cx88-blackbird,
plust cx88-alsa).

Cheers,
Mauro

Re: RFC: BKL, locking and ioctls

[Andy Walls]

On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
> > The device node isn't even the right place for drivers that provide
> multiple device nodes that can possibly access the same underlying
> data or register sets.
> > 
> > Any core/infrastructure approach is likely doomed in the general
> case.  It's trying to protect data and registers in a driver it knows
> nothing about, by protecting the *code paths* that take essentially
> unknown actions on that data and registers. :{
> 
> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
> itself can be either at the top-level device or associated with the actual
> video device, depending on the requirements of the driver.

OK.  Or the mutex can be NULL, where the driver does everything for
itself.

Locking at the device node level for ioctl()'s is better than the
v4l2_fh proposal, which serializes too much in some contexts and not
enough for others.

<obvious>
Any driver that creates ALSA, dvb, or fb device nodes, or another video
device node, with access to the same underlying data structures or
registers, will still need to perform proper locking.  The lock for the
ioctl() code paths will have to apply at a higher level than the device
node in these cases.
</obvious>

We're still preserving one of the main headaches of the BKL: "What
exactly is this lock protecting in this driver?".  We're just adding a
smaller scoped version to our own infrastructure.  At least maybe for
ioctl()'s in v4l2 the answer to the question is simpler: we're generally
protecting against concurrent access to the many and varied
v4l2_subdev's.
(Although that doesn't apply to VIDIOC_QUERYCAP and similar ioctl()'s.)


> > Videobuf is the right place to protect videobuf data.
> 
> vb_lock is AFAIK there to protect the streaming of data. And that's definitely
> per device node since only one filehandle per device node can do streaming.
> 
> Also remember that we are trying to get rid of the BKL, so staying bug-compatible
> is enough for a first version :-)

Sure.

Regards,
Andy

Re: RFC: BKL, locking and ioctls

[Andy Walls]

On Sun, 2010-09-19 at 08:43 -0300, Mauro Carvalho Chehab wrote: 
> Hi Hans,
> 

> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
> more than one stream per interface.
> 
> So, I did a different implementation, implementing the mutex pointer per file handler.
> On devices that a simple lock is possible, all you need to do is to use the same locking
> for all file handles, but if drivers want a finer control, they can use a per-file handler
> lock.
> 
> I'm adding the patches I did at media-tree.git. I've created a separate branch there (devel/bkl):
> 	http://git.linuxtv.org/media_tree.git?a=shortlog;h=refs/heads/devel/bkl
> 
> I've already applied there the other BKL-lock removal patches I've sent before, plus one new
> one, fixing a lock unbalance at bttv poll function (changeset 32d1c90c85).
> 
> The v4l2 core patches are at:
> 
> http://git.linuxtv.org/media_tree.git?a=commit;h=285267378581fbf852f24f3f99d2e937cd200fd5
> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
> 
> The approach I took serializes open, close, ioctl, mmap, read and poll, e. g. all file operations
> done by the V4L devices.

Hi Mauro,

I took a look at your changes in the first link.  The approach seems
like it serializes too much for one fd, and not enough for multiple fd's
opened on the same device node.

for a single fd, ioctl() probably doesn't need to be serialized against
read() and poll() at all - at least for drivers that only provide the
read I/O method.

read() and poll() on the same device node in most cases can access to
shared data structure in kernel using test_bit() and atomic_read().
poll() usually just needs to check if a count is  > 0 in some incoming
buffer count or incoming byte count somewhere, right?


Multiple opens of the device node (e.g one fd for streaming, one fd for
control) is what MythTV does, so the locking on a per fd basis doesn't
work there.


Regards,
Andy

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 20:29:58 Mauro Carvalho Chehab wrote:
> Em 19-09-2010 11:58, Hans Verkuil escreveu:
> > On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
> 
> >> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
> >> more than one stream per interface.
> > 
> > My proposal is actually a lock per device node, not per device (although that's
> > what many simple drivers probably will use).
> 
> Yes, that's what I meant. However, V4L2 API allows multiple opens and multiple streams per
> device node (and this is actually in use by several drivers).

Just to be clear: multiple opens is a V4L2 requirement. Some older drivers had exclusive
access, but those are gradually fixed.

Multiple stream per device node: if you are talking about allowing e.g. both VBI streaming
and video streaming from one device node, then that is indeed allowed by the current spec.
Few drivers support this though, and it is a really bad idea. During the Helsinki meeting we
agreed to remove this from the spec (point 10a in the mini summit report).

> >> So, I did a different implementation, implementing the mutex pointer per file handler.
> >> On devices that a simple lock is possible, all you need to do is to use the same locking
> >> for all file handles, but if drivers want a finer control, they can use a per-file handler
> >> lock.
> > 
> > I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
> > you need to serialize for a single filehandle (which would only be needed for multithreaded
> > applications where the threads use the same filehandle), then you definitely need to serialize
> > between multiple file handles that are open on the same device node.
> 
> On multithread apps, they'll share the same file handle, so, there's no issue. Some applications
> like xawtv and xdtv allows recording a video by starting another proccess that will use the read() 
> interface for one stream, while the other stream is using mmap() (or overlay) will have two different
> file handlers, one for each app. That's said, a driver using per-fh locks will likely need to
> have an additional lock for global resources. I didn't start porting cx88 driver, but I suspect
> that it will need to use it.

That read/mmap construct was discussed as well in Helsinki (also point 10a). I quote from the report:

"Mixed read() and mmap() streaming. Allow or disallow? bttv allows it, which is against the spec since
it only has one buffer queue so a read() will steal a frame. No conclusion was reached. Everyone thought
it was very ugly but some apps apparently use this. Even though few drivers actually support this functionality."

Applications must be able to work without this 'feature' since so few drivers allow this. And it
is against the spec as well. Perhaps we should try to remove this 'feature' and see if the apps
still work. If they do, then kill it. It's truly horrible. And it is definitely not a reason to
choose a overly complex locking scheme just so that some old apps can do a read and dqbuf at the
same time.

> > The device node is the right place for this IMHO.
> > 
> > Regarding creating v4l2_fh structs in the core: many simple drivers do not need a v4l2_fh
> > at all, and the more complex drivers often need to embed it in a larger struct.
> > 
> > The lookup get_v4l2_fh function also is unnecessary if we do not create these structs and
> > so is the *really* ugly reinit_v4l2_fh.
> 
> The reinit_v4l2_fh() is a temporary workaround, to avoid the need of rewriting the v4l2_fh
> implementation at ivtv, while keeping it work properly.
> 
> There are other ways to allow drivers to embed a per-fh data for the more complex devices, like
> adding a void *priv pointer, and letting the drivers to do whatever they want.

I've become a big fan of embedded data structures. It's the standard way the kernel structs
operate and it is the correct one as well. I don't like those void pointers. You loose all
type checking that way, and it makes managing the memory harder as well. It's the last
resort for me.
 
> The problem with the current implementation of v4l2_fh() is that there's no way for the core
> to get per-fh info.

You mean how to get from a struct file to a v4l2_fh? That should be done through
filp->private_data, but since many driver still put other things there, that is not
really usable at the moment without changing all those drivers first.

> >> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
> >> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
> >> to fill the buffers (tests are needed).
> > 
> > Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
> > release the lock in waiton. 
> 
> Well, mutex is allowed to be locked while scheduling. That's why it uses a mutex, instead of a
> spinlock. So, it seems to be safe. Yet, I agree that it should be releasing it at waiton, 
> in order to avoid troubles with some kernel threads that may be needing to lock to access
> the same data. So, the code will likely need to be changed in order to work with some drivers.

It's safe, sure, but a major blocker in the literal sense. If for some reason no new frames
arrive, then the whole system grinds to a halt because everyone is waiting for that mutex to
be release.
 
> > Right now no other access can be done while it is waiting. That
> > is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
> > is easy to solve there.
> > 
> > For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
> > Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
> > we can do better in vb2.
> 
> Having two mutexes passing as parameter for vb would be a confusing interface. Maybe the better is
> to postpone such change to happen after having all drivers ported to the new locking schema.

I'm OK with that as a temporary measure during developing the patch series, but what enters
the mainline should be working correctly in this respect. I.e. no mutex should be held when
the driver has to wait for a new frame (or a new event for that matter). Other apps must be
able to open/read/ioctl/whatever during that time.

> > My suggestion would be to use your videobuf patches, but use my idea for the mutex pointer
> > in struct video_device. Best of both worlds :-)
> 
> Maybe.

Hmmm. I need to do more work to convince you :-)

> >>> One other thing that I do not like is this:
> >>>
> >>>         /* Allow ioctl to continue even if the device was unregistered.
> >>>            Things like dequeueing buffers might still be useful. */
> >>>         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> >>>
> >>> I do not believe drivers can do anything useful once the device is unregistered
> >>> except just close the file handle. There are two exceptions to this: poll()
> >>> and VIDIOC_DQEVENT.
> >>>
> >>> Right now drivers have no way of detecting that a disconnect happened. It would
> >>> be easy to add a disconnect event and let the core issue it automatically. The
> >>> only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
> >>> raises an exception. Since all the information regarding events is available in
> >>> the core framework it is easy to do this transparently.
> >>>
> >>> So effectively, once a driver unregistered a device node it will never get
> >>> called again on that device node except for the release call. That is very
> >>> useful for a driver.
> >>>
> >>> And since we can do this in the core, it will also be consistent for all
> >>> drivers.
> >>
> >> I think we should implement a way to detect disconnections. This will allow simplifying the
> >> code at the drivers. Yet, I don't think that the solution is (only) to create an
> >> event. Instead, we need to see how this information could be retrieved from the bus.
> >> As the normal case for disconnections is for USB devices, we basically need to implement
> >> a callback when a diconnection happens. The USB core knows about that, but I don't know
> >> if it provides a callback for it.
> > 
> > Well, USB drivers have a disconnect callback. All V4L2 USB drivers hook into that.
> > What they are supposed to do is to unregister all video nodes. That sets the 'unregistered'
> > in the core preventing any further access.
> 
> I don't think all drivers implement it. I need to double check.

If a USB driver doesn't, then that's a driver bug. It will almost certainly crash if
you disconnect unexpectedly. Not that the presence of that callback gives you any
guarantees: usbvision has the callback but crashes spectacularly when you disconnect
while capturing :-(
 
> >> If it provides, drivers may just implement the callback,
> >> calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
> >> can then properly handle any new fops to that device, passing to the device just the
> >> close() events, returning -ENODEV and POLLERR for userspace.
> > 
> > That basically is what happens right now. Except for passing on the ioctls which is a bad
> > idea IMHO.
> 
> Yeah. If core already knows that the device got disconnected, it can just use a default handler
> for disconnected devices, instead of passing ioctls to the drivers.
> 
> > BTW: one other thing I've worked on today is a global release callback in v4l2_device.
> > Right now we have proper refcounting for struct video_device, but if a driver has multiple
> > device nodes, then it can be hard to tell when all of them are properly released and it
> > is safe to release the full device instance.
> > 
> > I made a fairly straightforward implementation available here:
> > 
> > http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core
> > 
> > Without a global release it is almost impossible to cleanup a driver like usbvision
> > correctly.
> 
> It seems interesting. Not sure how we can use it on drivers like cx88, but it is probably ok
> for most of the drivers. In the case of cx88, the refcount should be done at cx88 core struct,
> that it is used by 4 different drivers (3 of them opening V4L devices: cx88, cx88-mpeg, cx88-blackbird,
> plust cx88-alsa).

This actually will work correctly. When a device node is registered in cx88, it is already
hooked into the v4l2_device of the core struct. This was needed to handle the i2c subdevs
in the right place: the cx88 core struct. So refcounting will also be done in the core struct.

Regards,

	Hans

> 
> Cheers,
> Mauro
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 15:38, Andy Walls escreveu:
> On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
>> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
>>> The device node isn't even the right place for drivers that provide
>> multiple device nodes that can possibly access the same underlying
>> data or register sets.
>>>
>>> Any core/infrastructure approach is likely doomed in the general
>> case.  It's trying to protect data and registers in a driver it knows
>> nothing about, by protecting the *code paths* that take essentially
>> unknown actions on that data and registers. :{
>>
>> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
>> itself can be either at the top-level device or associated with the actual
>> video device, depending on the requirements of the driver.
> 
> OK.  Or the mutex can be NULL, where the driver does everything for
> itself.

Yes. If you don't like it, or have a better idea, you can just pass NULL
and do whatever you want on your driver.
> 
> Locking at the device node level for ioctl()'s is better than the
> v4l2_fh proposal, which serializes too much in some contexts and not
> enough for others.

The per-fh allows fine graining when needed. As it is a pointer, you can opt
to have per-device or per-fh locks (or even per-driver lock).
Open/close/mmap/read/poll need to be serialized anyway, as they generally
touch at the same data that you need to protect on ioctl.

By having a global locking schema like that, it is better to over-protect than
to leave some race conditions.

> <obvious>
> Any driver that creates ALSA, dvb, or fb device nodes, or another video
> device node, with access to the same underlying data structures or
> registers, will still need to perform proper locking.  The lock for the
> ioctl() code paths will have to apply at a higher level than the device
> node in these cases.
> </obvious>

Yes. Drivers will need to take care of it. If you look at the em28xx driver I ported,
it preserves the lock at dvb and alsa.

> We're still preserving one of the main headaches of the BKL: "What
> exactly is this lock protecting in this driver?".  We're just adding a
> smaller scoped version to our own infrastructure.  At least maybe for
> ioctl()'s in v4l2 the answer to the question is simpler: we're generally
> protecting against concurrent access to the many and varied
> v4l2_subdev's.
> (Although that doesn't apply to VIDIOC_QUERYCAP and similar ioctl()'s.)

Even querycap might need to be protected on a few drivers, where some info are detected
at runtime.

On the other hand, there's no practical impact on serializing querycap, as the userspace
applications already serialize the access to those enumeration ioctls.

The performance impact for serializing will happen at QBUF/DQBUF and read()/poll() for
both vbi and video, and at alsa and dvb streaming logic.

I agree with Hans that the additional penalty to serialize what's outside the streaming 
syscalls are not relevant and that it is better to sacrifice it in favor to a simpler 
locking schema.

>>> Videobuf is the right place to protect videobuf data.
>>
>> vb_lock is AFAIK there to protect the streaming of data. And that's definitely
>> per device node since only one filehandle per device node can do streaming.
>>
>> Also remember that we are trying to get rid of the BKL, so staying bug-compatible
>> is enough for a first version :-)
> 
> Sure.
> 
> Regards,
> Andy
> 

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 16:05, Andy Walls escreveu:
> On Sun, 2010-09-19 at 08:43 -0300, Mauro Carvalho Chehab wrote: 
>> Hi Hans,
>>
> 
>> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
>> more than one stream per interface.
>>
>> So, I did a different implementation, implementing the mutex pointer per file handler.
>> On devices that a simple lock is possible, all you need to do is to use the same locking
>> for all file handles, but if drivers want a finer control, they can use a per-file handler
>> lock.
>>
>> I'm adding the patches I did at media-tree.git. I've created a separate branch there (devel/bkl):
>> 	http://git.linuxtv.org/media_tree.git?a=shortlog;h=refs/heads/devel/bkl
>>
>> I've already applied there the other BKL-lock removal patches I've sent before, plus one new
>> one, fixing a lock unbalance at bttv poll function (changeset 32d1c90c85).
>>
>> The v4l2 core patches are at:
>>
>> http://git.linuxtv.org/media_tree.git?a=commit;h=285267378581fbf852f24f3f99d2e937cd200fd5
>> http://git.linuxtv.org/media_tree.git?a=commit;h=5f7b2159c87b08d4f0961c233a2d1d1b87c8b38d
>>
>> The approach I took serializes open, close, ioctl, mmap, read and poll, e. g. all file operations
>> done by the V4L devices.
> 
> Hi Mauro,
> 
> I took a look at your changes in the first link.  The approach seems
> like it serializes too much for one fd, and not enough for multiple fd's
> opened on the same device node.

Had you look at em28xx and vivi implementation? On both cases, we're using a per-device locking.
That's completely valid. For vivi, a per-fh would also work fine, but it won't make any difference,
as the support for multiple streams for vivi got removed. In the care of em28xx, a per-device-node
lock wouldn't work, since the data that needs to be protected is global for the entire device.

A real per-fd implementation will likely need an additional lock on the driver level, for some
ioctls. The core shouldn't prevent it.

> for a single fd, ioctl() probably doesn't need to be serialized against
> read() and poll() at all - at least for drivers that only provide the
> read I/O method.
> 
> read() and poll() on the same device node in most cases can access to
> shared data structure in kernel using test_bit() and atomic_read().
> poll() usually just needs to check if a count is  > 0 in some incoming
> buffer count or incoming byte count somewhere, right?

There are very few drivers that only implements read() method - I think only cx18 and ivtv.
In the mmap() case, you still need to protect read() x mmap(), read() x open(), and other
possible race issues. Some of them will even happen on single fd cases, especially if you
have multiple threads accessing the same fd. Of course, the mutex is more important on multiple-fd
cases, like the em28xx example.

> Multiple opens of the device node (e.g one fd for streaming, one fd for
> control) is what MythTV does, so the locking on a per fd basis doesn't
> work there.

Cheers,
Mauro

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 21:10:06 Mauro Carvalho Chehab wrote:
> Em 19-09-2010 15:38, Andy Walls escreveu:
> > On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
> >> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
> >>> The device node isn't even the right place for drivers that provide
> >> multiple device nodes that can possibly access the same underlying
> >> data or register sets.
> >>>
> >>> Any core/infrastructure approach is likely doomed in the general
> >> case.  It's trying to protect data and registers in a driver it knows
> >> nothing about, by protecting the *code paths* that take essentially
> >> unknown actions on that data and registers. :{
> >>
> >> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
> >> itself can be either at the top-level device or associated with the actual
> >> video device, depending on the requirements of the driver.
> > 
> > OK.  Or the mutex can be NULL, where the driver does everything for
> > itself.
> 
> Yes. If you don't like it, or have a better idea, you can just pass NULL
> and do whatever you want on your driver.
> > 
> > Locking at the device node level for ioctl()'s is better than the
> > v4l2_fh proposal, which serializes too much in some contexts and not
> > enough for others.
> 
> The per-fh allows fine graining when needed. As it is a pointer, you can opt
> to have per-device or per-fh locks (or even per-driver lock).
> Open/close/mmap/read/poll need to be serialized anyway, as they generally
> touch at the same data that you need to protect on ioctl.
> 
> By having a global locking schema like that, it is better to over-protect than
> to leave some race conditions.

That makes no sense. It is overly fine-grained locking schemes that lead to
race conditions. Overly course-grained schemes can lead to performance issues.
The problem is finding the right balance. Which to me is at the device node
level. AFAIK there is not a single driver that does locking at the file handle
level. It's either at the top level or at the device node level (usually through
videobuf's vb_lock).

And as I said before the v4l2_fh changes are fairly major and going in a
direction that I really don't like. Whereas putting the mutex pointer in struct
video_device is a very minor change that's easy to review and easy to understand.
A pointer to the video_device is also available almost everywhere. Whereas v4l2_fh
is not needed at all by many drivers.

If a driver really wants to do locking at the file handle level, then that
driver can always override the core locking and handle everything manually.

If you have unusual requirements, then that requires unusual amounts of work.
No need to let all the other drivers suffer.

> > <obvious>
> > Any driver that creates ALSA, dvb, or fb device nodes, or another video
> > device node, with access to the same underlying data structures or
> > registers, will still need to perform proper locking.  The lock for the
> > ioctl() code paths will have to apply at a higher level than the device
> > node in these cases.
> > </obvious>
> 
> Yes. Drivers will need to take care of it. If you look at the em28xx driver I ported,
> it preserves the lock at dvb and alsa.
> 
> > We're still preserving one of the main headaches of the BKL: "What
> > exactly is this lock protecting in this driver?".  We're just adding a
> > smaller scoped version to our own infrastructure.  At least maybe for
> > ioctl()'s in v4l2 the answer to the question is simpler: we're generally
> > protecting against concurrent access to the many and varied
> > v4l2_subdev's.
> > (Although that doesn't apply to VIDIOC_QUERYCAP and similar ioctl()'s.)
> 
> Even querycap might need to be protected on a few drivers, where some info are detected
> at runtime.
> 
> On the other hand, there's no practical impact on serializing querycap, as the userspace
> applications already serialize the access to those enumeration ioctls.
> 
> The performance impact for serializing will happen at QBUF/DQBUF and read()/poll() for
> both vbi and video, and at alsa and dvb streaming logic.
> 
> I agree with Hans that the additional penalty to serialize what's outside the streaming 
> syscalls are not relevant and that it is better to sacrifice it in favor to a simpler 
> locking schema.

And may I add to that that I think that attempting to optimize performance for
apps doing really weird things (like read and streaming i/o from the same source at
the same time) also falls under the header 'not relevant'?

Requirements I can think of:

1) The basic capture and output streaming (either read/write or streaming I/O) must
perform well. There is no need to go to extreme measures here, since the typical
control flow is to prepare a buffer, setup the DMA and then wait for the DMA to
finish. So this is not terribly time sensitive and it is perfectly OK to have to
wait (within reason) for another ioctl from another thread to finish first.

2) While capturing/displaying other threads must be able to control the device at the
same time and gather status information. This also means that such a thread should
not be blocked from controlling the device because a dqbuf ioctl happens to be waiting
for the DMA to finish in the main thread.

3) We must also make sure that the mem-to-mem drivers keep working. That might be a
special case that will become more important in the future. These are the only device
nodes that can do capture and output streaming at the same time.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 21:38:08 Hans Verkuil wrote:
> On Sunday, September 19, 2010 21:10:06 Mauro Carvalho Chehab wrote:
> > Em 19-09-2010 15:38, Andy Walls escreveu:
> > > On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
> > >> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
> > >>> The device node isn't even the right place for drivers that provide
> > >> multiple device nodes that can possibly access the same underlying
> > >> data or register sets.
> > >>>
> > >>> Any core/infrastructure approach is likely doomed in the general
> > >> case.  It's trying to protect data and registers in a driver it knows
> > >> nothing about, by protecting the *code paths* that take essentially
> > >> unknown actions on that data and registers. :{
> > >>
> > >> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
> > >> itself can be either at the top-level device or associated with the actual
> > >> video device, depending on the requirements of the driver.
> > > 
> > > OK.  Or the mutex can be NULL, where the driver does everything for
> > > itself.
> > 
> > Yes. If you don't like it, or have a better idea, you can just pass NULL
> > and do whatever you want on your driver.
> > > 
> > > Locking at the device node level for ioctl()'s is better than the
> > > v4l2_fh proposal, which serializes too much in some contexts and not
> > > enough for others.
> > 
> > The per-fh allows fine graining when needed. As it is a pointer, you can opt
> > to have per-device or per-fh locks (or even per-driver lock).
> > Open/close/mmap/read/poll need to be serialized anyway, as they generally
> > touch at the same data that you need to protect on ioctl.
> > 
> > By having a global locking schema like that, it is better to over-protect than
> > to leave some race conditions.
> 
> That makes no sense. It is overly fine-grained locking schemes that lead to
> race conditions. Overly course-grained schemes can lead to performance issues.
> The problem is finding the right balance. Which to me is at the device node
> level. AFAIK there is not a single driver that does locking at the file handle
> level. It's either at the top level or at the device node level (usually through
> videobuf's vb_lock).

BTW, something I just thought of: every V4L driver has by definition a video_device
struct. But there are still drivers that don't have the top-level v4l2_device struct
and many that do not have the v4l2_fh struct. So struct video_device is also the
place of the least impact on drivers.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Andy Walls]

On Sun, 2010-09-19 at 21:38 +0200, Hans Verkuil wrote:
> On Sunday, September 19, 2010 21:10:06 Mauro Carvalho Chehab wrote:
> > Em 19-09-2010 15:38, Andy Walls escreveu:
> > > On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
> > >> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:

> Requirements I can think of:
> 
> 1) The basic capture and output streaming (either read/write or streaming I/O) must
> perform well. There is no need to go to extreme measures here, since the typical
> control flow is to prepare a buffer, setup the DMA and then wait for the DMA to
> finish. So this is not terribly time sensitive and it is perfectly OK to have to
> wait (within reason) for another ioctl from another thread to finish first.

I'll add a data point to this one.  A sleep in read() can noticeably
affect application performance.  Last time I had cx18 use a mutex in
read(), live playback performance was really bad.  The read() call would
sleep for around 10 ms - not good at normal frame rates.  It was a
highly(?) contended mutex for the buffer queue between DMA and the
read() call - bad idea.

According to conversations on the ksummit2010 mailing list, the 10 ms
sleep may have been due to the default 100 HZ tick and other reasons.
Patches from the RT tree may be integrated soon that make mutexes
(mutices?) much better performers.

Regards,
Andy

> 2) While capturing/displaying other threads must be able to control the device at the
> same time and gather status information. This also means that such a thread should
> not be blocked from controlling the device because a dqbuf ioctl happens to be waiting
> for the DMA to finish in the main thread.
> 
> 3) We must also make sure that the mem-to-mem drivers keep working. That might be a
> special case that will become more important in the future. These are the only device
> nodes that can do capture and output streaming at the same time.
> 
> Regards,
> 
> 	Hans

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 16:06, Hans Verkuil escreveu:
> On Sunday, September 19, 2010 20:29:58 Mauro Carvalho Chehab wrote:
>> Em 19-09-2010 11:58, Hans Verkuil escreveu:
>>> On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
>>
>>>> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
>>>> more than one stream per interface.
>>>
>>> My proposal is actually a lock per device node, not per device (although that's
>>> what many simple drivers probably will use).
>>
>> Yes, that's what I meant. However, V4L2 API allows multiple opens and multiple streams per
>> device node (and this is actually in use by several drivers).
> 
> Just to be clear: multiple opens is a V4L2 requirement. Some older drivers had exclusive
> access, but those are gradually fixed.
> 
> Multiple stream per device node: if you are talking about allowing e.g. both VBI streaming
> and video streaming from one device node, then that is indeed allowed by the current spec.
> Few drivers support this though, and it is a really bad idea. During the Helsinki meeting we
> agreed to remove this from the spec (point 10a in the mini summit report).

I'm talking about read(), overlay and mmap(). The spec says, at "Multiple Opens" [1]:
	"When a device supports multiple functions like capturing and overlay simultaneously,
	 multiple opens allow concurrent use of the device by forked processes or specialized applications."

[1] http://linuxtv.org/downloads/v4l-dvb-apis/ch01.html#id2717880

So, it is allowed by the spec. What is forbidden is to have some copy logic in kernel to duplicate data.

> 
>>>> So, I did a different implementation, implementing the mutex pointer per file handler.
>>>> On devices that a simple lock is possible, all you need to do is to use the same locking
>>>> for all file handles, but if drivers want a finer control, they can use a per-file handler
>>>> lock.
>>>
>>> I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
>>> you need to serialize for a single filehandle (which would only be needed for multithreaded
>>> applications where the threads use the same filehandle), then you definitely need to serialize
>>> between multiple file handles that are open on the same device node.
>>
>> On multithread apps, they'll share the same file handle, so, there's no issue. Some applications
>> like xawtv and xdtv allows recording a video by starting another proccess that will use the read() 
>> interface for one stream, while the other stream is using mmap() (or overlay) will have two different
>> file handlers, one for each app. That's said, a driver using per-fh locks will likely need to
>> have an additional lock for global resources. I didn't start porting cx88 driver, but I suspect
>> that it will need to use it.
> 
> That read/mmap construct was discussed as well in Helsinki (also point 10a). I quote from the report:
> 
> "Mixed read() and mmap() streaming. Allow or disallow? bttv allows it, which is against the spec since
> it only has one buffer queue so a read() will steal a frame. No conclusion was reached. Everyone thought
> it was very ugly but some apps apparently use this. Even though few drivers actually support this functionality."
> 
> Applications must be able to work without this 'feature' since so few drivers allow this. And it
> is against the spec as well. Perhaps we should try to remove this 'feature' and see if the apps
> still work. If they do, then kill it. It's truly horrible. And it is definitely not a reason to
> choose a overly complex locking scheme just so that some old apps can do a read and dqbuf at the
> same time.

xawtv will stop working in record mode. It is one of the applications we added on our list that
we should use as reference.

I'm not against patching it to implement a different logic for record. Patches are welcome.

Considering that, currently, very few applications allow recording (I think only xawtv/xdtv, both using
ffmpeg or mencoder for record) and mythtv are the only ones, I don't think we should remove it, without
having it implemented on more places.

Besides that, not all device drivers will work with all applications or provide the complete set of
functionalities. For example, there are only three drivers (ivtv, cx18 and pvrusb2), as far as I remember, 
that only implements read() method. By using your logic that "only a few drivers allow feature X", maybe
we should deprecate read() as well ;)

> 
>>> The device node is the right place for this IMHO.
>>>
>>> Regarding creating v4l2_fh structs in the core: many simple drivers do not need a v4l2_fh
>>> at all, and the more complex drivers often need to embed it in a larger struct.
>>>
>>> The lookup get_v4l2_fh function also is unnecessary if we do not create these structs and
>>> so is the *really* ugly reinit_v4l2_fh.
>>
>> The reinit_v4l2_fh() is a temporary workaround, to avoid the need of rewriting the v4l2_fh
>> implementation at ivtv, while keeping it work properly.
>>
>> There are other ways to allow drivers to embed a per-fh data for the more complex devices, like
>> adding a void *priv pointer, and letting the drivers to do whatever they want.
> 
> I've become a big fan of embedded data structures. It's the standard way the kernel structs
> operate and it is the correct one as well. I don't like those void pointers. You loose all
> type checking that way, and it makes managing the memory harder as well. It's the last
> resort for me.

void *priv is also the standard way to support embeed data. So, it is also a valid approach.

>> The problem with the current implementation of v4l2_fh() is that there's no way for the core
>> to get per-fh info.
> 
> You mean how to get from a struct file to a v4l2_fh? That should be done through
> filp->private_data, but since many driver still put other things there, that is not
> really usable at the moment without changing all those drivers first.

It should be drivers decision to put whatever they want on a "priv_data". If you want to have
core data, then you need to use embeed for the core, but keeping priv_data for private driver's
internal usage. That's the standard way used on Linux. You're doing just the reverse ;)

>>>> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
>>>> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
>>>> to fill the buffers (tests are needed).
>>>
>>> Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
>>> release the lock in waiton. 
>>
>> Well, mutex is allowed to be locked while scheduling. That's why it uses a mutex, instead of a
>> spinlock. So, it seems to be safe. Yet, I agree that it should be releasing it at waiton, 
>> in order to avoid troubles with some kernel threads that may be needing to lock to access
>> the same data. So, the code will likely need to be changed in order to work with some drivers.
> 
> It's safe, sure, but a major blocker in the literal sense. If for some reason no new frames
> arrive, then the whole system grinds to a halt because everyone is waiting for that mutex to
> be release.

This need to be fixed, that's for sure, but only the userspace app and the driver will be blocked,
while the buffer timeout doesn't happen. AFAIK, all drivers have a timeout to return error if
the stream stops.

>>> Right now no other access can be done while it is waiting. That
>>> is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
>>> is easy to solve there.
>>>
>>> For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
>>> Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
>>> we can do better in vb2.
>>
>> Having two mutexes passing as parameter for vb would be a confusing interface. Maybe the better is
>> to postpone such change to happen after having all drivers ported to the new locking schema.
> 
> I'm OK with that as a temporary measure during developing the patch series, but what enters
> the mainline should be working correctly in this respect. I.e. no mutex should be held when
> the driver has to wait for a new frame (or a new event for that matter). Other apps must be
> able to open/read/ioctl/whatever during that time.

If we'll be fast enough to write the entire series, that would be the better. I'm afraid that
there are too many drivers using it nowadays. Perhaps we could create a new videobuf init call
for the new mutex.

>>> My suggestion would be to use your videobuf patches, but use my idea for the mutex pointer
>>> in struct video_device. Best of both worlds :-)
>>
>> Maybe.
> 
> Hmmm. I need to do more work to convince you :-)

:)

>>>>> One other thing that I do not like is this:
>>>>>
>>>>>         /* Allow ioctl to continue even if the device was unregistered.
>>>>>            Things like dequeueing buffers might still be useful. */
>>>>>         return vdev->fops->unlocked_ioctl(filp, cmd, arg);
>>>>>
>>>>> I do not believe drivers can do anything useful once the device is unregistered
>>>>> except just close the file handle. There are two exceptions to this: poll()
>>>>> and VIDIOC_DQEVENT.
>>>>>
>>>>> Right now drivers have no way of detecting that a disconnect happened. It would
>>>>> be easy to add a disconnect event and let the core issue it automatically. The
>>>>> only thing needed is that VIDIOC_DQEVENT ioctls are passed on and that poll
>>>>> raises an exception. Since all the information regarding events is available in
>>>>> the core framework it is easy to do this transparently.
>>>>>
>>>>> So effectively, once a driver unregistered a device node it will never get
>>>>> called again on that device node except for the release call. That is very
>>>>> useful for a driver.
>>>>>
>>>>> And since we can do this in the core, it will also be consistent for all
>>>>> drivers.
>>>>
>>>> I think we should implement a way to detect disconnections. This will allow simplifying the
>>>> code at the drivers. Yet, I don't think that the solution is (only) to create an
>>>> event. Instead, we need to see how this information could be retrieved from the bus.
>>>> As the normal case for disconnections is for USB devices, we basically need to implement
>>>> a callback when a diconnection happens. The USB core knows about that, but I don't know
>>>> if it provides a callback for it.
>>>
>>> Well, USB drivers have a disconnect callback. All V4L2 USB drivers hook into that.
>>> What they are supposed to do is to unregister all video nodes. That sets the 'unregistered'
>>> in the core preventing any further access.
>>
>> I don't think all drivers implement it. I need to double check.
> 
> If a USB driver doesn't, then that's a driver bug. It will almost certainly crash if
> you disconnect unexpectedly. Not that the presence of that callback gives you any
> guarantees: usbvision has the callback but crashes spectacularly when you disconnect
> while capturing :-(
>  
>>>> If it provides, drivers may just implement the callback,
>>>> calling buffer_release, and saying to V4L2 core that the device is disconnected. V4L2 core
>>>> can then properly handle any new fops to that device, passing to the device just the
>>>> close() events, returning -ENODEV and POLLERR for userspace.
>>>
>>> That basically is what happens right now. Except for passing on the ioctls which is a bad
>>> idea IMHO.
>>
>> Yeah. If core already knows that the device got disconnected, it can just use a default handler
>> for disconnected devices, instead of passing ioctls to the drivers.
>>
>>> BTW: one other thing I've worked on today is a global release callback in v4l2_device.
>>> Right now we have proper refcounting for struct video_device, but if a driver has multiple
>>> device nodes, then it can be hard to tell when all of them are properly released and it
>>> is safe to release the full device instance.
>>>
>>> I made a fairly straightforward implementation available here:
>>>
>>> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core
>>>
>>> Without a global release it is almost impossible to cleanup a driver like usbvision
>>> correctly.
>>
>> It seems interesting. Not sure how we can use it on drivers like cx88, but it is probably ok
>> for most of the drivers. In the case of cx88, the refcount should be done at cx88 core struct,
>> that it is used by 4 different drivers (3 of them opening V4L devices: cx88, cx88-mpeg, cx88-blackbird,
>> plust cx88-alsa).
> 
> This actually will work correctly. When a device node is registered in cx88, it is already
> hooked into the v4l2_device of the core struct. This was needed to handle the i2c subdevs
> in the right place: the cx88 core struct. So refcounting will also be done in the core struct.

No. Look at the actual code. For example, this is what cx88-mpeg does:

struct cx8802_dev *dev = pci_get_drvdata(pci_dev);

cx88 core is at dev->core.

The same happens with cx88-video, using struct cx8800:

struct cx8800_dev *dev = pci_get_drvdata(pci_dev);

cx88 core is also at dev->core.

This device is implemented using multiple PCI devices, one for each function. Function 0 (video) and Function 2
(used for TS devices, like mpeg encoders) can be used independently, but there are some data that are concurrent.
So, drivers will likely need to use two locks, one for the core and one for the function.

Cheers,
Mauro.

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 16:57, Andy Walls escreveu:
> On Sun, 2010-09-19 at 21:38 +0200, Hans Verkuil wrote:
>> On Sunday, September 19, 2010 21:10:06 Mauro Carvalho Chehab wrote:
>>> Em 19-09-2010 15:38, Andy Walls escreveu:
>>>> On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
>>>>> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
> 
>> Requirements I can think of:
>>
>> 1) The basic capture and output streaming (either read/write or streaming I/O) must
>> perform well. There is no need to go to extreme measures here, since the typical
>> control flow is to prepare a buffer, setup the DMA and then wait for the DMA to
>> finish. So this is not terribly time sensitive and it is perfectly OK to have to
>> wait (within reason) for another ioctl from another thread to finish first.
> 
> I'll add a data point to this one.  A sleep in read() can noticeably
> affect application performance.  Last time I had cx18 use a mutex in
> read(), live playback performance was really bad.  The read() call would
> sleep for around 10 ms - not good at normal frame rates.  It was a
> highly(?) contended mutex for the buffer queue between DMA and the
> read() call - bad idea.
> 
> According to conversations on the ksummit2010 mailing list, the 10 ms
> sleep may have been due to the default 100 HZ tick and other reasons.
> Patches from the RT tree may be integrated soon that make mutexes
> (mutices?) much better performers.

If read() and poll() are not protected, almost all drivers will break, except, perhaps,
for cx18, ivtv and pvrusb2. Streaming data need to be protected on both, when
using mmap().

Yet, having a mutex there doesn't mean that the driver will sleep. The issue you felt
is likely due to some other trouble, and not for the mutex itself.

Cheers,
Mauro

Re: RFC: BKL, locking and ioctls

[Andy Walls]

On Sun, 2010-09-19 at 17:20 -0300, Mauro Carvalho Chehab wrote:
> Em 19-09-2010 16:06, Hans Verkuil escreveu:
> > On Sunday, September 19, 2010 20:29:58 Mauro Carvalho Chehab wrote:
> >> Em 19-09-2010 11:58, Hans Verkuil escreveu:
> >>> On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
> >>
> >>>> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
> >>>> more than one stream per interface.
> >>>
> >>> My proposal is actually a lock per device node, not per device (although that's
> >>> what many simple drivers probably will use).
> >>
> >> Yes, that's what I meant. However, V4L2 API allows multiple opens and multiple streams per
> >> device node (and this is actually in use by several drivers).
> > 
> > Just to be clear: multiple opens is a V4L2 requirement. Some older drivers had exclusive
> > access, but those are gradually fixed.
> > 
> > Multiple stream per device node: if you are talking about allowing e.g. both VBI streaming
> > and video streaming from one device node, then that is indeed allowed by the current spec.
> > Few drivers support this though, and it is a really bad idea. During the Helsinki meeting we
> > agreed to remove this from the spec (point 10a in the mini summit report).
> 
> I'm talking about read(), overlay and mmap(). The spec says, at "Multiple Opens" [1]:
> 	"When a device supports multiple functions like capturing and overlay simultaneously,
> 	 multiple opens allow concurrent use of the device by forked processes or specialized applications."
> 
> [1] http://linuxtv.org/downloads/v4l-dvb-apis/ch01.html#id2717880
> 
> So, it is allowed by the spec. What is forbidden is to have some copy logic in kernel to duplicate data.
> 
> > 
> >>>> So, I did a different implementation, implementing the mutex pointer per file handler.
> >>>> On devices that a simple lock is possible, all you need to do is to use the same locking
> >>>> for all file handles, but if drivers want a finer control, they can use a per-file handler
> >>>> lock.
> >>>
> >>> I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
> >>> you need to serialize for a single filehandle (which would only be needed for multithreaded
> >>> applications where the threads use the same filehandle), then you definitely need to serialize
> >>> between multiple file handles that are open on the same device node.
> >>
> >> On multithread apps, they'll share the same file handle, so, there's no issue. Some applications
> >> like xawtv and xdtv allows recording a video by starting another proccess that will use the read() 
> >> interface for one stream, while the other stream is using mmap() (or overlay) will have two different
> >> file handlers, one for each app. That's said, a driver using per-fh locks will likely need to
> >> have an additional lock for global resources. I didn't start porting cx88 driver, but I suspect
> >> that it will need to use it.
> > 
> > That read/mmap construct was discussed as well in Helsinki (also point 10a). I quote from the report:
> > 
> > "Mixed read() and mmap() streaming. Allow or disallow? bttv allows it, which is against the spec since
> > it only has one buffer queue so a read() will steal a frame. No conclusion was reached. Everyone thought
> > it was very ugly but some apps apparently use this. Even though few drivers actually support this functionality."
> > 
> > Applications must be able to work without this 'feature' since so few drivers allow this. And it
> > is against the spec as well. Perhaps we should try to remove this 'feature' and see if the apps
> > still work. If they do, then kill it. It's truly horrible. And it is definitely not a reason to
> > choose a overly complex locking scheme just so that some old apps can do a read and dqbuf at the
> > same time.
> 
> xawtv will stop working in record mode. It is one of the applications we added on our list that
> we should use as reference.
> 
> I'm not against patching it to implement a different logic for record. Patches are welcome.
> 
> Considering that, currently, very few applications allow recording (I think only xawtv/xdtv, both using
> ffmpeg or mencoder for record) and mythtv are the only ones, I don't think we should remove it, without
> having it implemented on more places.

For non-MPEG v4l2 devices
mythtv-0.21/libs/libmythtv/NuppelVideoRecorder.cpp::DoV4L2() looks like
it only uses VIDIOC_QBUF and VIDIOC_DQBUF for video frames - no read()s.
It appears to use read() for VBI data on a different file descriptor.
(em28xx VBI appears to be implemented via videobuf in the em28xx
driver.)

For the MPEG class of devices (ivtv/cx18),
mythtv-0.21/libs/libmythtv/mpegrecoder.cpp only uses read().



> Besides that, not all device drivers will work with all applications or provide the complete set of
> functionalities. For example, there are only three drivers (ivtv, cx18 and pvrusb2), as far as I remember, 
> that only implements read() method. By using your logic that "only a few drivers allow feature X", maybe
> we should deprecate read() as well ;)

Hans,

On an somewhat related note, but off-topic: what is the proper way to
implement VIDIOC_QUERYCAP for a driver that implements read()
on /dev/video0 (MPEG) and mmap() streaming on /dev/video32 (YUV)?

I'm assuming the right way is for VIDIOC_QUERYCAP to return different
caps based on which device node was queried.

Regards,
Andy

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 22:20:18 Mauro Carvalho Chehab wrote:
> Em 19-09-2010 16:06, Hans Verkuil escreveu:
> > On Sunday, September 19, 2010 20:29:58 Mauro Carvalho Chehab wrote:
> >> Em 19-09-2010 11:58, Hans Verkuil escreveu:
> >>> On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
> >>
> >>>> A per-dev lock may not be good on devices where you have lots of interfaces, and that allows
> >>>> more than one stream per interface.
> >>>
> >>> My proposal is actually a lock per device node, not per device (although that's
> >>> what many simple drivers probably will use).
> >>
> >> Yes, that's what I meant. However, V4L2 API allows multiple opens and multiple streams per
> >> device node (and this is actually in use by several drivers).
> > 
> > Just to be clear: multiple opens is a V4L2 requirement. Some older drivers had exclusive
> > access, but those are gradually fixed.
> > 
> > Multiple stream per device node: if you are talking about allowing e.g. both VBI streaming
> > and video streaming from one device node, then that is indeed allowed by the current spec.
> > Few drivers support this though, and it is a really bad idea. During the Helsinki meeting we
> > agreed to remove this from the spec (point 10a in the mini summit report).
> 
> I'm talking about read(), overlay and mmap(). The spec says, at "Multiple Opens" [1]:
> 	"When a device supports multiple functions like capturing and overlay simultaneously,
> 	 multiple opens allow concurrent use of the device by forked processes or specialized applications."
> 
> [1] http://linuxtv.org/downloads/v4l-dvb-apis/ch01.html#id2717880
> 
> So, it is allowed by the spec. What is forbidden is to have some copy logic in kernel to duplicate data.

There is no streaming involved with overlays, is there? It is all handled in the driver and
userspace just tells the driver where the window is. I may be wrong, I'm much more familiar
with output overlays (OSD). Are overlays actually still working anywhere these days?

> 
> > 
> >>>> So, I did a different implementation, implementing the mutex pointer per file handler.
> >>>> On devices that a simple lock is possible, all you need to do is to use the same locking
> >>>> for all file handles, but if drivers want a finer control, they can use a per-file handler
> >>>> lock.
> >>>
> >>> I am rather unhappy about this. First of all, per-filehandle locks are pretty pointless. If
> >>> you need to serialize for a single filehandle (which would only be needed for multithreaded
> >>> applications where the threads use the same filehandle), then you definitely need to serialize
> >>> between multiple file handles that are open on the same device node.
> >>
> >> On multithread apps, they'll share the same file handle, so, there's no issue. Some applications
> >> like xawtv and xdtv allows recording a video by starting another proccess that will use the read() 
> >> interface for one stream, while the other stream is using mmap() (or overlay) will have two different
> >> file handlers, one for each app. That's said, a driver using per-fh locks will likely need to
> >> have an additional lock for global resources. I didn't start porting cx88 driver, but I suspect
> >> that it will need to use it.
> > 
> > That read/mmap construct was discussed as well in Helsinki (also point 10a). I quote from the report:
> > 
> > "Mixed read() and mmap() streaming. Allow or disallow? bttv allows it, which is against the spec since
> > it only has one buffer queue so a read() will steal a frame. No conclusion was reached. Everyone thought
> > it was very ugly but some apps apparently use this. Even though few drivers actually support this functionality."
> > 
> > Applications must be able to work without this 'feature' since so few drivers allow this. And it
> > is against the spec as well. Perhaps we should try to remove this 'feature' and see if the apps
> > still work. If they do, then kill it. It's truly horrible. And it is definitely not a reason to
> > choose a overly complex locking scheme just so that some old apps can do a read and dqbuf at the
> > same time.
> 
> xawtv will stop working in record mode. It is one of the applications we added on our list that
> we should use as reference.
> 
> I'm not against patching it to implement a different logic for record. Patches are welcome.
> 
> Considering that, currently, very few applications allow recording (I think only xawtv/xdtv, both using
> ffmpeg or mencoder for record) and mythtv are the only ones, I don't think we should remove it, without
> having it implemented on more places.

Well, it doesn't need to be removed (although we should look into it), but I do not consider this
a reason to lock at filehandle level.
 
> Besides that, not all device drivers will work with all applications or provide the complete set of
> functionalities. For example, there are only three drivers (ivtv, cx18 and pvrusb2), as far as I remember, 
> that only implements read() method. By using your logic that "only a few drivers allow feature X", maybe
> we should deprecate read() as well ;)

There's nothing wrong with read. But reading while streaming at the same time from the same source,
that's another matter. And I'm hoping that vb2 will make it possible to implement streaming in
ivtv and cx18.

<snip>

> >> The problem with the current implementation of v4l2_fh() is that there's no way for the core
> >> to get per-fh info.
> > 
> > You mean how to get from a struct file to a v4l2_fh? That should be done through
> > filp->private_data, but since many driver still put other things there, that is not
> > really usable at the moment without changing all those drivers first.
> 
> It should be drivers decision to put whatever they want on a "priv_data". If you want to have
> core data, then you need to use embeed for the core, but keeping priv_data for private driver's
> internal usage. That's the standard way used on Linux. You're doing just the reverse ;)

I don't follow your reasoning here.
 
> >>>> We could need to do some changes there to cover the case where videobuf sleeps, maybe using
> >>>> mutex_lock_interruptible at core, in order to allow abort userspace, if the driver fails
> >>>> to fill the buffers (tests are needed).
> >>>
> >>> Regarding the (very courageous!) videobuf patches: I'm impressed. But videobuf must really
> >>> release the lock in waiton. 
> >>
> >> Well, mutex is allowed to be locked while scheduling. That's why it uses a mutex, instead of a
> >> spinlock. So, it seems to be safe. Yet, I agree that it should be releasing it at waiton, 
> >> in order to avoid troubles with some kernel threads that may be needing to lock to access
> >> the same data. So, the code will likely need to be changed in order to work with some drivers.
> > 
> > It's safe, sure, but a major blocker in the literal sense. If for some reason no new frames
> > arrive, then the whole system grinds to a halt because everyone is waiting for that mutex to
> > be release.
> 
> This need to be fixed, that's for sure, but only the userspace app and the driver will be blocked,
> while the buffer timeout doesn't happen. AFAIK, all drivers have a timeout to return error if
> the stream stops.

There can be many reasons why no new frames arrive. It is not acceptable that other apps
that want to use the same device node would block because of that.

> 
> >>> Right now no other access can be done while it is waiting. That
> >>> is not acceptable. The same issue appears in the VIDIOC_DQEVENT core handler, although it
> >>> is easy to solve there.
> >>>
> >>> For videobuf it might be better to pass a pointer to the serializing mutex as an argument.
> >>> Then videobuf can use that to unlock/relock when it has to wait. Not elegant, but hopefully
> >>> we can do better in vb2.
> >>
> >> Having two mutexes passing as parameter for vb would be a confusing interface. Maybe the better is
> >> to postpone such change to happen after having all drivers ported to the new locking schema.
> > 
> > I'm OK with that as a temporary measure during developing the patch series, but what enters
> > the mainline should be working correctly in this respect. I.e. no mutex should be held when
> > the driver has to wait for a new frame (or a new event for that matter). Other apps must be
> > able to open/read/ioctl/whatever during that time.
> 
> If we'll be fast enough to write the entire series, that would be the better. I'm afraid that
> there are too many drivers using it nowadays. Perhaps we could create a new videobuf init call
> for the new mutex.

I've no problem with that.

<snip>

> >>> BTW: one other thing I've worked on today is a global release callback in v4l2_device.
> >>> Right now we have proper refcounting for struct video_device, but if a driver has multiple
> >>> device nodes, then it can be hard to tell when all of them are properly released and it
> >>> is safe to release the full device instance.
> >>>
> >>> I made a fairly straightforward implementation available here:
> >>>
> >>> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/v4l2core
> >>>
> >>> Without a global release it is almost impossible to cleanup a driver like usbvision
> >>> correctly.
> >>
> >> It seems interesting. Not sure how we can use it on drivers like cx88, but it is probably ok
> >> for most of the drivers. In the case of cx88, the refcount should be done at cx88 core struct,
> >> that it is used by 4 different drivers (3 of them opening V4L devices: cx88, cx88-mpeg, cx88-blackbird,
> >> plust cx88-alsa).
> > 
> > This actually will work correctly. When a device node is registered in cx88, it is already
> > hooked into the v4l2_device of the core struct. This was needed to handle the i2c subdevs
> > in the right place: the cx88 core struct. So refcounting will also be done in the core struct.
> 
> No. Look at the actual code. For example, this is what cx88-mpeg does:
> 
> struct cx8802_dev *dev = pci_get_drvdata(pci_dev);
> 
> cx88 core is at dev->core.
> 
> The same happens with cx88-video, using struct cx8800:
> 
> struct cx8800_dev *dev = pci_get_drvdata(pci_dev);
> 
> cx88 core is also at dev->core.
> 
> This device is implemented using multiple PCI devices, one for each function. Function 0 (video) and Function 2
> (used for TS devices, like mpeg encoders) can be used independently, but there are some data that are concurrent.
> So, drivers will likely need to use two locks, one for the core and one for the function.

I was talking about refcounting in cx88 using my patch, not locking. Locking in
cx88 will almost certainly need two locks.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Sunday, September 19, 2010 23:02:31 Andy Walls wrote:
> Hans,
> 
> On an somewhat related note, but off-topic: what is the proper way to
> implement VIDIOC_QUERYCAP for a driver that implements read()
> on /dev/video0 (MPEG) and mmap() streaming on /dev/video32 (YUV)?
> 
> I'm assuming the right way is for VIDIOC_QUERYCAP to return different
> caps based on which device node was queried.

The spec is not really clear about this. It would be the right thing to do
IMHO, but the spec would need a change.

The caps that are allowed to change between device nodes would have to be
clearly documented. Basically only the last three in the list, and the phrase
'The device supports the...' should be replaced with 'The device node supports
the...'.

It would need some analysis and an RFC as well.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: RFC: BKL, locking and ioctls

[Devin Heitmueller]

On Sun, Sep 19, 2010 at 5:17 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Sunday, September 19, 2010 23:02:31 Andy Walls wrote:
>> Hans,
>>
>> On an somewhat related note, but off-topic: what is the proper way to
>> implement VIDIOC_QUERYCAP for a driver that implements read()
>> on /dev/video0 (MPEG) and mmap() streaming on /dev/video32 (YUV)?
>>
>> I'm assuming the right way is for VIDIOC_QUERYCAP to return different
>> caps based on which device node was queried.
>
> The spec is not really clear about this. It would be the right thing to do
> IMHO, but the spec would need a change.
>
> The caps that are allowed to change between device nodes would have to be
> clearly documented. Basically only the last three in the list, and the phrase
> 'The device supports the...' should be replaced with 'The device node supports
> the...'.

This would be great to straighten out.  One of the common problems new
users have setting up MythTV is trying to figure out what type of
device they should be choosing (e.g. "V4L2 capture device" versus
"IVTV MPEG capture device").  The problem is that the application
cannot limit the list of /dev/videoX entries for a given type because
some devices report both for all device nodes (even though, for
example, the cx18 can only do MPEG on /dev/video1 and raw video on
/dev/video0).

This results in all sorts of confusion when people wonder why they
cannot watch TV because they picked "IVTV MPEG capture device", and
then picked /dev/video0 as the device node.

And of course the real fun comes around when they cannot figure out
why they cannot capture video on /dev/video24 and /dev/video32 because
those aren't actually video capture devices *at all*.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 16:38, Hans Verkuil escreveu:
> On Sunday, September 19, 2010 21:10:06 Mauro Carvalho Chehab wrote:
>> Em 19-09-2010 15:38, Andy Walls escreveu:
>>> On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
>>>> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
>>>>> The device node isn't even the right place for drivers that provide
>>>> multiple device nodes that can possibly access the same underlying
>>>> data or register sets.
>>>>>
>>>>> Any core/infrastructure approach is likely doomed in the general
>>>> case.  It's trying to protect data and registers in a driver it knows
>>>> nothing about, by protecting the *code paths* that take essentially
>>>> unknown actions on that data and registers. :{
>>>>
>>>> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
>>>> itself can be either at the top-level device or associated with the actual
>>>> video device, depending on the requirements of the driver.
>>>
>>> OK.  Or the mutex can be NULL, where the driver does everything for
>>> itself.
>>
>> Yes. If you don't like it, or have a better idea, you can just pass NULL
>> and do whatever you want on your driver.
>>>
>>> Locking at the device node level for ioctl()'s is better than the
>>> v4l2_fh proposal, which serializes too much in some contexts and not
>>> enough for others.
>>
>> The per-fh allows fine graining when needed. As it is a pointer, you can opt
>> to have per-device or per-fh locks (or even per-driver lock).
>> Open/close/mmap/read/poll need to be serialized anyway, as they generally
>> touch at the same data that you need to protect on ioctl.
>>
>> By having a global locking schema like that, it is better to over-protect than
>> to leave some race conditions.
> 
> That makes no sense. It is overly fine-grained locking schemes that lead to
> race conditions. Overly course-grained schemes can lead to performance issues.
> The problem is finding the right balance. Which to me is at the device node
> level. AFAIK there is not a single driver that does locking at the file handle
> level. It's either at the top level or at the device node level (usually through
> videobuf's vb_lock).

The discussion here is not related of being at per-device or per-fh. Andy is arguing
that the V4L2 core should not be locking poll() and read(), but both operations need
to be locked in order for it to warrant no race conditions for mmap(). The only 
exception is that this is not needed is for the three drivers that (still)
don't implement mmap().

> And as I said before the v4l2_fh changes are fairly major and going in a
> direction that I really don't like. Whereas putting the mutex pointer in struct
> video_device is a very minor change that's easy to review and easy to understand.
> A pointer to the video_device is also available almost everywhere. Whereas v4l2_fh
> is not needed at all by many drivers.
> 
> If a driver really wants to do locking at the file handle level, then that
> driver can always override the core locking and handle everything manually.
> 
> If you have unusual requirements, then that requires unusual amounts of work.
> No need to let all the other drivers suffer.

Ok, this is a good point. Could you please write a patch against the devel branch?

Yet, we need to keep read() and poll() locked, otherwise most drivers will break.

>>> <obvious>
>>> Any driver that creates ALSA, dvb, or fb device nodes, or another video
>>> device node, with access to the same underlying data structures or
>>> registers, will still need to perform proper locking.  The lock for the
>>> ioctl() code paths will have to apply at a higher level than the device
>>> node in these cases.
>>> </obvious>
>>
>> Yes. Drivers will need to take care of it. If you look at the em28xx driver I ported,
>> it preserves the lock at dvb and alsa.
>>
>>> We're still preserving one of the main headaches of the BKL: "What
>>> exactly is this lock protecting in this driver?".  We're just adding a
>>> smaller scoped version to our own infrastructure.  At least maybe for
>>> ioctl()'s in v4l2 the answer to the question is simpler: we're generally
>>> protecting against concurrent access to the many and varied
>>> v4l2_subdev's.
>>> (Although that doesn't apply to VIDIOC_QUERYCAP and similar ioctl()'s.)
>>
>> Even querycap might need to be protected on a few drivers, where some info are detected
>> at runtime.
>>
>> On the other hand, there's no practical impact on serializing querycap, as the userspace
>> applications already serialize the access to those enumeration ioctls.
>>
>> The performance impact for serializing will happen at QBUF/DQBUF and read()/poll() for
>> both vbi and video, and at alsa and dvb streaming logic.
>>
>> I agree with Hans that the additional penalty to serialize what's outside the streaming 
>> syscalls are not relevant and that it is better to sacrifice it in favor to a simpler 
>> locking schema.
> 
> And may I add to that that I think that attempting to optimize performance for
> apps doing really weird things (like read and streaming i/o from the same source at
> the same time) also falls under the header 'not relevant'?

This is relevant, since reverting this behavior is clearly a regression, as userspace apps
will break.

> Requirements I can think of:
> 
> 1) The basic capture and output streaming (either read/write or streaming I/O) must
> perform well. There is no need to go to extreme measures here, since the typical
> control flow is to prepare a buffer, setup the DMA and then wait for the DMA to
> finish. So this is not terribly time sensitive and it is perfectly OK to have to
> wait (within reason) for another ioctl from another thread to finish first.

On all USB devices, you also need to do double-buffering, to remove the USB headers from
the video and audio information. They generally spend a considerable amount of time doing
it on some drivers, in order to get the beginning of the frames.

> 2) While capturing/displaying other threads must be able to control the device at the
> same time and gather status information. This also means that such a thread should
> not be blocked from controlling the device because a dqbuf ioctl happens to be waiting
> for the DMA to finish in the main thread.

True.

> 3) We must also make sure that the mem-to-mem drivers keep working. That might be a
> special case that will become more important in the future. These are the only device
> nodes that can do capture and output streaming at the same time.

Agreed.

Mauro.

Re: RFC: BKL, locking and ioctls

[Mauro Carvalho Chehab]

Em 19-09-2010 18:10, Hans Verkuil escreveu:
> On Sunday, September 19, 2010 22:20:18 Mauro Carvalho Chehab wrote:
>> Em 19-09-2010 16:06, Hans Verkuil escreveu:
>>> On Sunday, September 19, 2010 20:29:58 Mauro Carvalho Chehab wrote:
>>>> Em 19-09-2010 11:58, Hans Verkuil escreveu:
>>>>> On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
>>>>
>>> Multiple stream per device node: if you are talking about allowing e.g. both VBI streaming
>>> and video streaming from one device node, then that is indeed allowed by the current spec.
>>> Few drivers support this though, and it is a really bad idea. During the Helsinki meeting we
>>> agreed to remove this from the spec (point 10a in the mini summit report).
>>
>> I'm talking about read(), overlay and mmap(). The spec says, at "Multiple Opens" [1]:
>> 	"When a device supports multiple functions like capturing and overlay simultaneously,
>> 	 multiple opens allow concurrent use of the device by forked processes or specialized applications."
>>
>> [1] http://linuxtv.org/downloads/v4l-dvb-apis/ch01.html#id2717880
>>
>> So, it is allowed by the spec. What is forbidden is to have some copy logic in kernel to duplicate data.
> 
> There is no streaming involved with overlays, is there? It is all handled in the driver and
> userspace just tells the driver where the window is. I may be wrong, I'm much more familiar
> with output overlays (OSD). Are overlays actually still working anywhere these days?

It depends on what you call streaming. On overlay mode, there's a PCI2PCI transfer stream, from video 
capture memory into the video adapter memory. It is still a stream, even though it is not "visible"
after started.

>> Besides that, not all device drivers will work with all applications or provide the complete set of
>> functionalities. For example, there are only three drivers (ivtv, cx18 and pvrusb2), as far as I remember, 
>> that only implements read() method. By using your logic that "only a few drivers allow feature X", maybe
>> we should deprecate read() as well ;)
> 
> There's nothing wrong with read. But reading while streaming at the same time from the same source,
> that's another matter. 

You may not like its implementation, but it is currently in use, and there's nothing at spec
forbidding it.

> And I'm hoping that vb2 will make it possible to implement streaming in
> ivtv and cx18.

Ok. That's another reason why we should lock poll/read.

> <snip>
> 
>>>> The problem with the current implementation of v4l2_fh() is that there's no way for the core
>>>> to get per-fh info.
>>>
>>> You mean how to get from a struct file to a v4l2_fh? That should be done through
>>> filp->private_data, but since many driver still put other things there, that is not
>>> really usable at the moment without changing all those drivers first.
>>
>> It should be drivers decision to put whatever they want on a "priv_data". If you want to have
>> core data, then you need to use embeed for the core, but keeping priv_data for private driver's
>> internal usage. That's the standard way used on Linux. You're doing just the reverse ;)
> 
> I don't follow your reasoning here.

What kernel does, in general, is to use a "class inheritance" by embeding one struct into another.
This is used mainly on the core structs. For drivers, it provides void *priv data for internal driver-only
usage.

The way you're wanting to do with v4l2_fh is just the reverse: use priv_data for core usage, and embed 
struct for the drivers.

>>> This actually will work correctly. When a device node is registered in cx88, it is already
>>> hooked into the v4l2_device of the core struct. This was needed to handle the i2c subdevs
>>> in the right place: the cx88 core struct. So refcounting will also be done in the core struct.
>>
>> No. Look at the actual code. For example, this is what cx88-mpeg does:
>>
>> struct cx8802_dev *dev = pci_get_drvdata(pci_dev);
>>
>> cx88 core is at dev->core.
>>
>> The same happens with cx88-video, using struct cx8800:
>>
>> struct cx8800_dev *dev = pci_get_drvdata(pci_dev);
>>
>> cx88 core is also at dev->core.
>>
>> This device is implemented using multiple PCI devices, one for each function. Function 0 (video) and Function 2
>> (used for TS devices, like mpeg encoders) can be used independently, but there are some data that are concurrent.
>> So, drivers will likely need to use two locks, one for the core and one for the function.
> 
> I was talking about refcounting in cx88 using my patch, not locking. Locking in
> cx88 will almost certainly need two locks.

Depending on the way the cx88 core lock will be implemented, you may need to unlock it before release.

I just arguing that it needs to take more care/review at cx88, in order to avoid having a dead lock there.

Cheers,
Mauro

Re: RFC: BKL, locking and ioctls

[Hans Verkuil]

On Monday, September 20, 2010 03:52:30 Mauro Carvalho Chehab wrote:
> Em 19-09-2010 16:38, Hans Verkuil escreveu:
> > On Sunday, September 19, 2010 21:10:06 Mauro Carvalho Chehab wrote:
> >> Em 19-09-2010 15:38, Andy Walls escreveu:
> >>> On Sun, 2010-09-19 at 18:10 +0200, Hans Verkuil wrote:
> >>>> On Sunday, September 19, 2010 17:38:18 Andy Walls wrote:
> >>>>> The device node isn't even the right place for drivers that provide
> >>>> multiple device nodes that can possibly access the same underlying
> >>>> data or register sets.
> >>>>>
> >>>>> Any core/infrastructure approach is likely doomed in the general
> >>>> case.  It's trying to protect data and registers in a driver it knows
> >>>> nothing about, by protecting the *code paths* that take essentially
> >>>> unknown actions on that data and registers. :{
> >>>>
> >>>> Just to clarify: struct video_device gets a *pointer* to a mutex. The mutex
> >>>> itself can be either at the top-level device or associated with the actual
> >>>> video device, depending on the requirements of the driver.
> >>>
> >>> OK.  Or the mutex can be NULL, where the driver does everything for
> >>> itself.
> >>
> >> Yes. If you don't like it, or have a better idea, you can just pass NULL
> >> and do whatever you want on your driver.
> >>>
> >>> Locking at the device node level for ioctl()'s is better than the
> >>> v4l2_fh proposal, which serializes too much in some contexts and not
> >>> enough for others.
> >>
> >> The per-fh allows fine graining when needed. As it is a pointer, you can opt
> >> to have per-device or per-fh locks (or even per-driver lock).
> >> Open/close/mmap/read/poll need to be serialized anyway, as they generally
> >> touch at the same data that you need to protect on ioctl.
> >>
> >> By having a global locking schema like that, it is better to over-protect than
> >> to leave some race conditions.
> > 
> > That makes no sense. It is overly fine-grained locking schemes that lead to
> > race conditions. Overly course-grained schemes can lead to performance issues.
> > The problem is finding the right balance. Which to me is at the device node
> > level. AFAIK there is not a single driver that does locking at the file handle
> > level. It's either at the top level or at the device node level (usually through
> > videobuf's vb_lock).
> 
> The discussion here is not related of being at per-device or per-fh. Andy is arguing
> that the V4L2 core should not be locking poll() and read(), but both operations need
> to be locked in order for it to warrant no race conditions for mmap(). The only 
> exception is that this is not needed is for the three drivers that (still)
> don't implement mmap().

I think we should just start off by locking read/write/poll as well, and if
it causes problems, then we can create flags that drivers can set to prevent
the core from locking selected fops.

Something I've always wondered about: what's the point of the get_unmapped_area
file op? Someone added it but not a single driver uses it. Can we get rid of this
in v4l2_file_operations?
 
> > And as I said before the v4l2_fh changes are fairly major and going in a
> > direction that I really don't like. Whereas putting the mutex pointer in struct
> > video_device is a very minor change that's easy to review and easy to understand.
> > A pointer to the video_device is also available almost everywhere. Whereas v4l2_fh
> > is not needed at all by many drivers.
> > 
> > If a driver really wants to do locking at the file handle level, then that
> > driver can always override the core locking and handle everything manually.
> > 
> > If you have unusual requirements, then that requires unusual amounts of work.
> > No need to let all the other drivers suffer.
> 
> Ok, this is a good point. Could you please write a patch against the devel branch?

OK, I will do that.

Is it possible for you to commit at least the vtx patches first? Those also touch
on the core and I'd prefer not to have conflicts with that patch series. The same
really for the pwc, cpia2, usbvision and driver deprecation patches. They are
uncontroversial and it gives me a good starting point.

For my work on implementing the control framework I would also appreciate if the
i2c patches removing v4l2-i2c-drv.h are merged. These patches are trivial, but they
are a prerequisite for the control framework conversions.
 
> Yet, we need to keep read() and poll() locked, otherwise most drivers will break.

We should see if we can do better with vb2. It would be good to come up with
a document that describes 'best practices' when it comes to locking in V4L2 and
vb2.

Other options for the future: split the big v4l2_ioctl_ops into two pieces:
'unlocked ops' and 'locked ops'. Many of the query and enum ops do not normally
need a lock, so by putting them in a struct of their own it will be easy for
drivers to tell that those ops are called with the lock. It won't work to just
document them, it needs something more explicit.

> 
> >>> <obvious>
> >>> Any driver that creates ALSA, dvb, or fb device nodes, or another video
> >>> device node, with access to the same underlying data structures or
> >>> registers, will still need to perform proper locking.  The lock for the
> >>> ioctl() code paths will have to apply at a higher level than the device
> >>> node in these cases.
> >>> </obvious>
> >>
> >> Yes. Drivers will need to take care of it. If you look at the em28xx driver I ported,
> >> it preserves the lock at dvb and alsa.
> >>
> >>> We're still preserving one of the main headaches of the BKL: "What
> >>> exactly is this lock protecting in this driver?".  We're just adding a
> >>> smaller scoped version to our own infrastructure.  At least maybe for
> >>> ioctl()'s in v4l2 the answer to the question is simpler: we're generally
> >>> protecting against concurrent access to the many and varied
> >>> v4l2_subdev's.
> >>> (Although that doesn't apply to VIDIOC_QUERYCAP and similar ioctl()'s.)
> >>
> >> Even querycap might need to be protected on a few drivers, where some info are detected
> >> at runtime.
> >>
> >> On the other hand, there's no practical impact on serializing querycap, as the userspace
> >> applications already serialize the access to those enumeration ioctls.
> >>
> >> The performance impact for serializing will happen at QBUF/DQBUF and read()/poll() for
> >> both vbi and video, and at alsa and dvb streaming logic.
> >>
> >> I agree with Hans that the additional penalty to serialize what's outside the streaming 
> >> syscalls are not relevant and that it is better to sacrifice it in favor to a simpler 
> >> locking schema.
> > 
> > And may I add to that that I think that attempting to optimize performance for
> > apps doing really weird things (like read and streaming i/o from the same source at
> > the same time) also falls under the header 'not relevant'?
> 
> This is relevant, since reverting this behavior is clearly a regression, as userspace apps
> will break.

I never said that we should break this functionality, that's not possible without changing
apps. But it isn't a high-performance use-case either.
 
> > Requirements I can think of:
> > 
> > 1) The basic capture and output streaming (either read/write or streaming I/O) must
> > perform well. There is no need to go to extreme measures here, since the typical
> > control flow is to prepare a buffer, setup the DMA and then wait for the DMA to
> > finish. So this is not terribly time sensitive and it is perfectly OK to have to
> > wait (within reason) for another ioctl from another thread to finish first.
> 
> On all USB devices, you also need to do double-buffering, to remove the USB headers from
> the video and audio information. They generally spend a considerable amount of time doing
> it on some drivers, in order to get the beginning of the frames.

In an ideal world such drivers should probably release the lock while they are doing that.

> > 2) While capturing/displaying other threads must be able to control the device at the
> > same time and gather status information. This also means that such a thread should
> > not be blocked from controlling the device because a dqbuf ioctl happens to be waiting
> > for the DMA to finish in the main thread.
> 
> True.
> 
> > 3) We must also make sure that the mem-to-mem drivers keep working. That might be a
> > special case that will become more important in the future. These are the only device
> > nodes that can do capture and output streaming at the same time.
> 
> Agreed.
> 
> Mauro.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco