[RFC] Serialization flag example

[RFC] Serialization flag example

[Hans Verkuil]

I made a quick implementation which is available here:

http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize

It's pretty easy to use and it also gives you a very simple way to block
access to the video device nodes until all have been allocated by simply
taking the serialization lock and holding it until we are done with the
initialization.

I converted radio-mr800.c and ivtv.

That said, almost all drivers that register multiple device nodes probably
suffer from a race condition when one of the device node registrations
returns an error and all devices have to be unregistered and the driver
needs to release all resources.

Currently most if not all drivers just release resources and free the memory.
But if an application managed to open one device before the driver removes it
again, then we have almost certainly a crash.

It is possible to do this correctly in the driver, but it really needs core
support where a release callback can be installed in v4l2_device that is
called when the last video_device is closed by the application.

We already can cleanup correctly after the last close of a video_device, but
there is no top-level release yet.


Anyway, I tried to use the serialization flag in bttv as well, but I ran into
problems with videobuf. Basically when you need to wait for some event you
should release the serialization lock and grab it after the event arrives.

Unfortunately videobuf has no access to v4l2_device at the moment. If we would
have that, then videobuf can just release the serialization lock while waiting
for something to happen.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [RFC] Serialization flag example

[Mauro Carvalho Chehab]

Hans,


Hans Verkuil wrote:
> I made a quick implementation which is available here:
> 
> http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize
> 
> It's pretty easy to use and it also gives you a very simple way to block
> access to the video device nodes until all have been allocated by simply
> taking the serialization lock and holding it until we are done with the
> initialization.
> 
> I converted radio-mr800.c and ivtv.
> 
> That said, almost all drivers that register multiple device nodes probably
> suffer from a race condition when one of the device node registrations
> returns an error and all devices have to be unregistered and the driver
> needs to release all resources.
> 
> Currently most if not all drivers just release resources and free the memory.
> But if an application managed to open one device before the driver removes it
> again, then we have almost certainly a crash.
> 
> It is possible to do this correctly in the driver, but it really needs core
> support where a release callback can be installed in v4l2_device that is
> called when the last video_device is closed by the application.
> 
> We already can cleanup correctly after the last close of a video_device, but
> there is no top-level release yet.
> 
> 
> Anyway, I tried to use the serialization flag in bttv as well, but I ran into
> problems with videobuf. Basically when you need to wait for some event you
> should release the serialization lock and grab it after the event arrives.
> 
> Unfortunately videobuf has no access to v4l2_device at the moment. If we would
> have that, then videobuf can just release the serialization lock while waiting
> for something to happen.


While this code works like a charm with the radio devices, it probably
won't work fine with complex devices.

You'll have issues also with -alsa and -dvb parts that are present on the drivers.

Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves
like two separate drivers (each with its own bind code at V4L core), but, as there's
just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock
should cross between the different drivers.

So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers,
nor the lock will work as expected, at least with most of the devices.

Also, although this is not a real problem, your lock is too pedantic: it is 
blocking access to several ioctl's that don't need to be locked. For example, there are
several ioctl's that just returns static: information: capabilities, supported video standards,
etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF
without locking (or with a minimum lock), to allow different threads to handle the buffers.

The big issue I see with this approach is that developers will become lazy on checking
the locks inside the drivers: they'll just apply the flag and trust that all of their
locking troubles were magically solved by the core. 

Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
used internally on the driver. This approach will still be pedantic, as all ioctls will
keep being serialized, but at least the driver will need to explicitly handle the lock,
and the same lock can be used on other parts of the driver.

-- 

Cheers,
Mauro

Re: [RFC] Serialization flag example

[Hans Verkuil]

On Thursday 01 April 2010 20:24:12 Mauro Carvalho Chehab wrote:
> Hans,
> 
> 
> Hans Verkuil wrote:
> > I made a quick implementation which is available here:
> > 
> > http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize
> > 
> > It's pretty easy to use and it also gives you a very simple way to block
> > access to the video device nodes until all have been allocated by simply
> > taking the serialization lock and holding it until we are done with the
> > initialization.
> > 
> > I converted radio-mr800.c and ivtv.
> > 
> > That said, almost all drivers that register multiple device nodes probably
> > suffer from a race condition when one of the device node registrations
> > returns an error and all devices have to be unregistered and the driver
> > needs to release all resources.
> > 
> > Currently most if not all drivers just release resources and free the memory.
> > But if an application managed to open one device before the driver removes it
> > again, then we have almost certainly a crash.
> > 
> > It is possible to do this correctly in the driver, but it really needs core
> > support where a release callback can be installed in v4l2_device that is
> > called when the last video_device is closed by the application.
> > 
> > We already can cleanup correctly after the last close of a video_device, but
> > there is no top-level release yet.
> > 
> > 
> > Anyway, I tried to use the serialization flag in bttv as well, but I ran into
> > problems with videobuf. Basically when you need to wait for some event you
> > should release the serialization lock and grab it after the event arrives.
> > 
> > Unfortunately videobuf has no access to v4l2_device at the moment. If we would
> > have that, then videobuf can just release the serialization lock while waiting
> > for something to happen.
> 
> 
> While this code works like a charm with the radio devices, it probably
> won't work fine with complex devices.

Nor was it meant to. Although ivtv is a pretty complex device and it works fine
there. But yes, when you also have alsa, dvb or other parts then you will have to
think more carefully and possibly abandon the core serialization altogether.

However, of the almost 80 drivers we have it is my conservative estimate that about
75% of those fall in the 'simple' device category, at least when it comes to locking.
I would like to see a simple, but good, locking scheme for those 60 drivers. And the
remaining 20 can take care of their own locking.

> You'll have issues also with -alsa and -dvb parts that are present on the drivers.
> 
> Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves
> like two separate drivers (each with its own bind code at V4L core), but, as there's
> just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock
> should cross between the different drivers.
> 
> So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers,
> nor the lock will work as expected, at least with most of the devices.

As I said, you have to enable this serialization explicitly. And obviously you shouldn't
enable it mindlessly.
 
> Also, although this is not a real problem, your lock is too pedantic: it is 
> blocking access to several ioctl's that don't need to be locked. For example, there are
> several ioctl's that just returns static: information: capabilities, supported video standards,
> etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF
> without locking (or with a minimum lock), to allow different threads to handle the buffers.

Which is why videobuf should be aware of such a global lock so that it can release it
when it has to wait.
 
> The big issue I see with this approach is that developers will become lazy on checking
> the locks inside the drivers: they'll just apply the flag and trust that all of their
> locking troubles were magically solved by the core. 

Well, for simple drivers (i.e. the vast majority) that is indeed the case.
Locking is hard. If this can be moved into the core for most drivers, then that is
good. I like it if developers can be lazy. Less chance of bugs.

And mind that this is exactly the situation we have now with ioctl and the BKL!

> Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
> used internally on the driver. This approach will still be pedantic, as all ioctls will
> keep being serialized, but at least the driver will need to explicitly handle the lock,
> and the same lock can be used on other parts of the driver.

Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device
that drivers can set. But I don't see much of a difference in practice.

Regarding the 'pedantic approach': we can easily move the locking to video_ioctl2:

	struct video_device *vdev = video_devdata(file);
	int serialize = must_serialize_ioctl(vdev, cmd);

	if (serialize)
		mutex_lock(&vdev->v4l2_dev->serialize_lock);
        /* Handles IOCTL */
        err = __video_do_ioctl(file, cmd, parg);
	if (serialize)
		mutex_unlock(&vdev->v4l2_dev->serialize_lock);


And must_serialize_ioctl() looks like this:

static int must_serialize_ioctl(struct video_device *vdev, int cmd)
{
	if (!vdev->v4l2_dev || !vdev->v4l2_dev->fl_serialize)
		return 0;
	switch (cmd) {
	case VIDIOC_QUERYCAP:
	case VIDIOC_ENUM_FMT:
	...
		return 0;
	}
	return 1;
}

Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
serialized. I am not sure whether the streaming ioctls should also be included
here. I can't really grasp the consequences of whatever choice we make. If we
want to be compatible with what happens today with ioctl and the BKL, then we
need to lock and have videobuf unlock whenever it has to wait for an event.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [RFC] Serialization flag example

[Mauro Carvalho Chehab]

Hans Verkuil wrote:
>> Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
>> used internally on the driver. This approach will still be pedantic, as all ioctls will
>> keep being serialized, but at least the driver will need to explicitly handle the lock,
>> and the same lock can be used on other parts of the driver.
> 
> Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device
> that drivers can set. But I don't see much of a difference in practice.

It makes easier to implement more complex approaches, where you may need to use more
locks. Also, It makes no sense to make a DVB code or an alsa code dependent on a V4L
header, just because it needs to see a mutex.

Also, a mutex at the driver need to be initialized inside the driver. It is not just one
flag that someone writing a new driver will clone without really understanding what
it is doing.

> Regarding the 'pedantic approach': we can easily move the locking to video_ioctl2:
> 
> 	struct video_device *vdev = video_devdata(file);
> 	int serialize = must_serialize_ioctl(vdev, cmd);
> 
> 	if (serialize)
> 		mutex_lock(&vdev->v4l2_dev->serialize_lock);
>         /* Handles IOCTL */
>         err = __video_do_ioctl(file, cmd, parg);
> 	if (serialize)
> 		mutex_unlock(&vdev->v4l2_dev->serialize_lock);
> 
> 
> And must_serialize_ioctl() looks like this:
> 
> static int must_serialize_ioctl(struct video_device *vdev, int cmd)
> {
> 	if (!vdev->v4l2_dev || !vdev->v4l2_dev->fl_serialize)
> 		return 0;
> 	switch (cmd) {
> 	case VIDIOC_QUERYCAP:
> 	case VIDIOC_ENUM_FMT:
> 	...
> 		return 0;
> 	}
> 	return 1;
> }
> 
> Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
> serialized. I am not sure whether the streaming ioctls should also be included
> here. I can't really grasp the consequences of whatever choice we make. If we
> want to be compatible with what happens today with ioctl and the BKL, then we
> need to lock and have videobuf unlock whenever it has to wait for an event.

I suspect that the list of "must have" is driver-dependent.

-- 

Cheers,
Mauro

Re: [RFC] Serialization flag example

[Hans Verkuil]

On Thursday 01 April 2010 23:32:33 Mauro Carvalho Chehab wrote:
> Hans Verkuil wrote:
> >> Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
> >> used internally on the driver. This approach will still be pedantic, as all ioctls will
> >> keep being serialized, but at least the driver will need to explicitly handle the lock,
> >> and the same lock can be used on other parts of the driver.
> > 
> > Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device
> > that drivers can set. But I don't see much of a difference in practice.
> 
> It makes easier to implement more complex approaches, where you may need to use more
> locks. Also, It makes no sense to make a DVB code or an alsa code dependent on a V4L
> header, just because it needs to see a mutex.

What's in a name. v4l2_device is meant to be a top-level object in a driver.
There is nothing particularly v4l about it and it would be trivial to rename
it to media_device.

> Also, a mutex at the driver need to be initialized inside the driver. It is not just one
> flag that someone writing a new driver will clone without really understanding what
> it is doing.

Having a driver do mutex_init() really does not improve understanding. But
good documentation will. Creating a simple, easy to understand and well
documented locking scheme will go a long way to making our drivers better.

Now, having said all this, I do think upon reflection that using a pointer
to a mutex might be better. The main reason being that while I do think that
renaming v4l2_device to media_device is a good idea and that more code sharing
between v4l and dvb would benefit both (heck, perhaps there should be more
integration between v4l-dvb and alsa as well), the political reality is
different.

> 
> > Regarding the 'pedantic approach': we can easily move the locking to video_ioctl2:
> > 
> > 	struct video_device *vdev = video_devdata(file);
> > 	int serialize = must_serialize_ioctl(vdev, cmd);
> > 
> > 	if (serialize)
> > 		mutex_lock(&vdev->v4l2_dev->serialize_lock);
> >         /* Handles IOCTL */
> >         err = __video_do_ioctl(file, cmd, parg);
> > 	if (serialize)
> > 		mutex_unlock(&vdev->v4l2_dev->serialize_lock);
> > 
> > 
> > And must_serialize_ioctl() looks like this:
> > 
> > static int must_serialize_ioctl(struct video_device *vdev, int cmd)
> > {
> > 	if (!vdev->v4l2_dev || !vdev->v4l2_dev->fl_serialize)
> > 		return 0;
> > 	switch (cmd) {
> > 	case VIDIOC_QUERYCAP:
> > 	case VIDIOC_ENUM_FMT:
> > 	...
> > 		return 0;
> > 	}
> > 	return 1;
> > }
> > 
> > Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
> > serialized. I am not sure whether the streaming ioctls should also be included
> > here. I can't really grasp the consequences of whatever choice we make. If we
> > want to be compatible with what happens today with ioctl and the BKL, then we
> > need to lock and have videobuf unlock whenever it has to wait for an event.
> 
> I suspect that the list of "must have" is driver-dependent.

If needed one could allow drivers to override this function. But again, start
simple and only make it more complex if we really need to. Overengineering is
one of the worst mistakes one can make. I have seen too many projects fail
because of that.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [RFC] Serialization flag example

[Mauro Carvalho Chehab]

Hans Verkuil wrote:
> On Thursday 01 April 2010 23:32:33 Mauro Carvalho Chehab wrote:
>> Hans Verkuil wrote:
>>>> Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
>>>> used internally on the driver. This approach will still be pedantic, as all ioctls will
>>>> keep being serialized, but at least the driver will need to explicitly handle the lock,
>>>> and the same lock can be used on other parts of the driver.
>>> Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device
>>> that drivers can set. But I don't see much of a difference in practice.
>> It makes easier to implement more complex approaches, where you may need to use more
>> locks. Also, It makes no sense to make a DVB code or an alsa code dependent on a V4L
>> header, just because it needs to see a mutex.
> 
> What's in a name. v4l2_device is meant to be a top-level object in a driver.
> There is nothing particularly v4l about it and it would be trivial to rename
> it to media_device.

This has nothing to do with a name. The point is that such mutex need to be used by
other systems (for example, some drivers have alsa provided by snd-usb-audio).

>> Also, a mutex at the driver need to be initialized inside the driver. It is not just one
>> flag that someone writing a new driver will clone without really understanding what
>> it is doing.
> 
> Having a driver do mutex_init() really does not improve understanding. But
> good documentation will. Creating a simple, easy to understand and well
> documented locking scheme will go a long way to making our drivers better.

Documentation is for sure needed. Having the mutex inside of the driver helps
people to understand, as I doubt that most of the developers take a deep look
inside V4L and Linux core implementations before writing a driver. 

I'm all for porting things that are common to the core, but things that
depends on the driver internal logic, like the mutexes that protect the driver
data structs, should be more visible (or be fully implemented) outside the core.

> Now, having said all this, I do think upon reflection that using a pointer
> to a mutex might be better. The main reason being that while I do think that
> renaming v4l2_device to media_device is a good idea and that more code sharing
> between v4l and dvb would benefit both (heck, perhaps there should be more
> integration between v4l-dvb and alsa as well), the political reality is
> different.

With respect to v4l2_device:

The reason should be technical, not political. A proper module/subsystem decoupling is
interesting, to easy maintainership. As I said, back on LPC/2007, the core functions
provided by v4l2_device makes sense and should also be used on DVB. That's my 2 cents.

The reality is that migrating existing drivers to it will be very time consuming,
and maybe not valuable enough, at least for those dvb drivers that are almost
unmaintained nowadays, but I think that using it for new drivers and for the drivers
where we have a constant pattern of maintainability would be a good thing.

I think we should evolute to have a more integrated core for both V4L and DVB,
that will provide the proper locking between the two subsystems. 

>>> Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
>>> serialized. I am not sure whether the streaming ioctls should also be included
>>> here. I can't really grasp the consequences of whatever choice we make. If we
>>> want to be compatible with what happens today with ioctl and the BKL, then we
>>> need to lock and have videobuf unlock whenever it has to wait for an event.
>> I suspect that the list of "must have" is driver-dependent.
> 
> If needed one could allow drivers to override this function. But again, start
> simple and only make it more complex if we really need to. Overengineering is
> one of the worst mistakes one can make. I have seen too many projects fail
> because of that.

Ok.

-- 

Cheers,
Mauro

Re: [RFC] Serialization flag example

[Manu Abraham]

On Thu, Apr 1, 2010 at 10:24 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:

> You'll have issues also with -alsa and -dvb parts that are present on the drivers.
>
> Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves
> like two separate drivers (each with its own bind code at V4L core), but, as there's
> just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock
> should cross between the different drivers.
>
> So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers,
> nor the lock will work as expected, at least with most of the devices.
>
> Also, although this is not a real problem, your lock is too pedantic: it is
> blocking access to several ioctl's that don't need to be locked. For example, there are
> several ioctl's that just returns static: information: capabilities, supported video standards,
> etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF
> without locking (or with a minimum lock), to allow different threads to handle the buffers.
>
> The big issue I see with this approach is that developers will become lazy on checking
> the locks inside the drivers: they'll just apply the flag and trust that all of their
> locking troubles were magically solved by the core.
>
> Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
> used internally on the driver. This approach will still be pedantic, as all ioctls will
> keep being serialized, but at least the driver will need to explicitly handle the lock,
> and the same lock can be used on other parts of the driver.


IMO, A framework shouldn't lock. Why ?

Different devices require different locking schemes, each and every
device require it in different ways. Some drivers might not even
require it, since they may be able to handle different systems
asynchronously.

Locking and such device management, will be known to the driver alone
and not to any framework. While, this may benefit some few devices the
other set of devices will eventually be broken.


Manu

Re: [RFC] Serialization flag example

[Devin Heitmueller]

On Fri, Apr 2, 2010 at 1:43 PM, Manu Abraham <abraham.manu@gmail.com> wrote:
> On Thu, Apr 1, 2010 at 10:24 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>
>> You'll have issues also with -alsa and -dvb parts that are present on the drivers.
>>
>> Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves
>> like two separate drivers (each with its own bind code at V4L core), but, as there's
>> just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock
>> should cross between the different drivers.
>>
>> So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers,
>> nor the lock will work as expected, at least with most of the devices.
>>
>> Also, although this is not a real problem, your lock is too pedantic: it is
>> blocking access to several ioctl's that don't need to be locked. For example, there are
>> several ioctl's that just returns static: information: capabilities, supported video standards,
>> etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF
>> without locking (or with a minimum lock), to allow different threads to handle the buffers.
>>
>> The big issue I see with this approach is that developers will become lazy on checking
>> the locks inside the drivers: they'll just apply the flag and trust that all of their
>> locking troubles were magically solved by the core.
>>
>> Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock,
>> used internally on the driver. This approach will still be pedantic, as all ioctls will
>> keep being serialized, but at least the driver will need to explicitly handle the lock,
>> and the same lock can be used on other parts of the driver.
>
>
> IMO, A framework shouldn't lock. Why ?
>
> Different devices require different locking schemes, each and every
> device require it in different ways. Some drivers might not even
> require it, since they may be able to handle different systems
> asynchronously.
>
> Locking and such device management, will be known to the driver alone
> and not to any framework. While, this may benefit some few devices the
> other set of devices will eventually be broken.

Hello Manu,

The argument I am trying to make is that there are numerous cases
where you should not be able to use both a particular DVB and V4L
device at the same time.  The implementation of such locking should be
handled by the v4l-dvb core, but the definition of the relationships
dictating which devices cannot be used in parallel is still the
responsibility of the driver.

This way, a bridge driver can say "this DVB device cannot be used at
the same time as this V4L device" but the actual enforcement of the
locking is done in the core.  For cases where the devices can be used
in parallel, the bridge driver doesn't have to do anything.

Devin

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

Re: [RFC] Serialization flag example

[Manu Abraham]

Hi Devin,

> Hello Manu,
>
> The argument I am trying to make is that there are numerous cases
> where you should not be able to use both a particular DVB and V4L
> device at the same time.  The implementation of such locking should be
> handled by the v4l-dvb core, but the definition of the relationships
> dictating which devices cannot be used in parallel is still the
> responsibility of the driver.
>
> This way, a bridge driver can say "this DVB device cannot be used at
> the same time as this V4L device" but the actual enforcement of the
> locking is done in the core.  For cases where the devices can be used
> in parallel, the bridge driver doesn't have to do anything.

I follow what you mean. Why I emphasized that it shouldn't be at the
core, but basically in the bridge driver:

Case 1
- there is a 1:n relation, In this case there is only 1 path and 3
users sharing that path
In such a case, You can use such a mentioned scheme, where things do
look okay, since it is only about locking a single path.

Case 2
- there is a n:n relation, in this case there are n paths and n users
In such a case, it is hard to make the core aware of all the details,
since there could be more than 1 resource of the same category;
Mapping each of them properly won't be easy, as for the same chip
driver Resource A on Card A, would mean different for Resource A on
Card B.

Case 3
- there is a m:n relation, in this case, there are m paths and n users
This case is even more painful than the previous ones.

In cases 2 & 3, the option to handle such cases is using a
configuration scheme based on the card type. I guess handling such
would be quite daunting and hard to get right. I guess it would be
much more efficient and useful to have such a feature to be made
available in the bridge driver as it is a resource of the card
configuration, rather than a common bridge resource.


Manu

Re: [RFC] Serialization flag example

[Devin Heitmueller]

On Fri, Apr 2, 2010 at 2:24 PM, Manu Abraham <abraham.manu@gmail.com> wrote:
> Hi Devin,
>
>> Hello Manu,
>>
>> The argument I am trying to make is that there are numerous cases
>> where you should not be able to use both a particular DVB and V4L
>> device at the same time.  The implementation of such locking should be
>> handled by the v4l-dvb core, but the definition of the relationships
>> dictating which devices cannot be used in parallel is still the
>> responsibility of the driver.
>>
>> This way, a bridge driver can say "this DVB device cannot be used at
>> the same time as this V4L device" but the actual enforcement of the
>> locking is done in the core.  For cases where the devices can be used
>> in parallel, the bridge driver doesn't have to do anything.
>
> I follow what you mean. Why I emphasized that it shouldn't be at the
> core, but basically in the bridge driver:
>
> Case 1
> - there is a 1:n relation, In this case there is only 1 path and 3
> users sharing that path
> In such a case, You can use such a mentioned scheme, where things do
> look okay, since it is only about locking a single path.
>
> Case 2
> - there is a n:n relation, in this case there are n paths and n users
> In such a case, it is hard to make the core aware of all the details,
> since there could be more than 1 resource of the same category;
> Mapping each of them properly won't be easy, as for the same chip
> driver Resource A on Card A, would mean different for Resource A on
> Card B.
>
> Case 3
> - there is a m:n relation, in this case, there are m paths and n users
> This case is even more painful than the previous ones.
>
> In cases 2 & 3, the option to handle such cases is using a
> configuration scheme based on the card type. I guess handling such
> would be quite daunting and hard to get right. I guess it would be
> much more efficient and useful to have such a feature to be made
> available in the bridge driver as it is a resource of the card
> configuration, rather than a common bridge resource.

Hi Manu,

I don't have any problem with a bridge choosing to implement some much
more complicated scheme to meet its own special requirements.
However, it feels like the vast majority of bridges would fall into
scenario #1, and having that functionality in the core would mean that
all of those bridges would work properly (only needing a 2 line code
change).  Hence, making the core handle the common case and still
allowing the bridge maintainer to override the logic if necessary
would seem like an ideal solution.

Nothing I have suggested precludes the bridge maintainer from *not*
adding the code making the association in the core and instead adding
his/her own locking infrastructure to the bridge driver.

Right now, I can think of five or six bridges all of which fall into
category #1.  Should we really add effectively the exact same locking
code to all those bridges, running the risk of of screwing up the
cut/paste?

Devin

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

Re: [RFC] Serialization flag example

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Fri, Apr 2, 2010 at 1:43 PM, Manu Abraham <abraham.manu@gmail.com> wrote:
>> IMO, A framework shouldn't lock.

Current DVB framework is locked with BKL. I agree that an unconditional 
lock like this is very bad. We need to get rid of it as soon as possible.

> Hello Manu,
> 
> The argument I am trying to make is that there are numerous cases
> where you should not be able to use both a particular DVB and V4L
> device at the same time.  The implementation of such locking should be
> handled by the v4l-dvb core, but the definition of the relationships
> dictating which devices cannot be used in parallel is still the
> responsibility of the driver.
> 
> This way, a bridge driver can say "this DVB device cannot be used at
> the same time as this V4L device" but the actual enforcement of the
> locking is done in the core.  For cases where the devices can be used
> in parallel, the bridge driver doesn't have to do anything.

Although both are some sort of locks, the BKL replacement lock is
basically a memory barrier to serialize data, avoiding that the driver's
internal structs to be corrupted or to return a wrong value. Only the
driver really knows what should be protected.

In the case of V4L/DVB devices, the struct to be protected is the struct *_dev
(struct core, on cx88, struct em28xx on em28xx, struct saa7134_dev, and so on).

Of course, if everything got serialized by the core, and assuming that the 
driver doesn't have IRQ's and/or other threads accessing the same data, the 
problem disappears, at the expense of a performance reduction that may or 
may not be relevant.

That's said, except for the most simple drivers, like radio ones, there's always
some sort of IRQ and/or threads evolved, touching on struct *_dev. 

For example, both cx88 and saa7134 have (depending on the hardware):
	- IRQ to announce that a data buffer was filled for video, vbi, alsa;
	- IRQ for IR;
	- IR polling;
	- video audio standard detection thread;

A lock to protect the internal data structs should protect all the above. Even
the most pedantic core-only lock won't solve it.

Yet, a lock, on core, for ioctl/poll/open/close may be part of a protection, if
the same lock is used also to protect the other usages of struct *_dev.

So, I'm OK on having an optional mutex parameter to be passed to V4L core, to provide
the BKL removal.

In the case of a V4L x DVB type of lock, this is not to protect some memory, but,
instead, to limit the usage of a hardware that is not capable of simultaneously
provide V4L and DVB streams. This is a common case on almost all devices, but, as
Hermann pointed, there are a few devices that are capable of doing both analog
and digital streams at the same time, but saa7134 driver currently doesn't support.

Some drivers, like cx88 has a sort of lock meant to protect such things. It is
implemented on res_get/res_check/res_locked/res_free. Currently, the lock is only
protecting simultaneous usage of the analog part of the driver. It works by not
allowing to start two simultaneous video streams of the same memory access type, 
for example, while allowing multiple device opens, for example to call V4L controls, 
while another application is streaming. It also allows having a mmapped or overlay
stream and a separate read() stream (used on applications like xawtv and xdtv to
record a video on PCI devices that has enough bandwidth to provide two simultaneous 
streams).

Maybe this kind of lock can be abstracted, and we can add a class of resource protectors
inside the core, for the drivers to use it when needed.

-- 

Cheers,
Mauro

Re: [RFC] Serialization flag example

[Andy Walls]

On Fri, 2010-04-02 at 10:57 +0200, Hans Verkuil wrote:

> If needed one could allow drivers to override this function. But again, start
> simple and only make it more complex if we really need to. Overengineering is
> one of the worst mistakes one can make. I have seen too many projects fail
> because of that.

Gall's Law!

http://en.wikipedia.org/wiki/Gall%27s_law


Regards,
Andy

Re: [RFC] Serialization flag example

[Andy Walls]

On Fri, 2010-04-02 at 18:15 -0300, Mauro Carvalho Chehab wrote:
> Devin Heitmueller wrote:

> In the case of a V4L x DVB type of lock, this is not to protect some memory, but,
> instead, to limit the usage of a hardware that is not capable of simultaneously
> provide V4L and DVB streams. This is a common case on almost all devices, but, as
> Hermann pointed, there are a few devices that are capable of doing both analog
> and digital streams at the same time, but saa7134 driver currently doesn't support.

I know a driver that does:

cx18 can handle simultaneous streaming of DTV and analog.

Some cards have both an analog and digital tuner, so both DTV and analog
can come from an RF source simultaneously.  (No locking needed really.)

Some cards only have one tuner, which means simultaneous streaming is
limited to DTV from RF and analog from baseband inputs.  Streaming
analog from an RF source on these cards precludes streaming of DTV.  (An
odd locking ruleset when you consider MPEG, YUV, PCM, and VBI from the
bridge chip can independently be streamed from the selected analog
source to 4 different device nodes.)

Regards,
Andy

Re: [RFC] Serialization flag example

[Mauro Carvalho Chehab]

Hans Verkuil wrote:
>> Hans Verkuil wrote:

>> 	- performance is important only for the ioctl's that directly handles
>> the streaming (dbuf/dqbuf & friends);
> 
> What performance? These calls just block waiting for a frame. How the hell
> am I suppose to test performance anyway on something like that?

They are called on the main loop for receiving buffers. As the userspace may be
doing some video processing, by introducing latency here, you may affect the
applications. By using perf subsystem, you should be able to test how much
time is spent by an ioctl.

> 
>> 	- videobuf has its own lock implementation;
>>
>> 	- a trivial mutex-based approach won't protect the stream to have
>> some parameters modified by a VIDIOC_S_* ioctl (such protection should be
>> provided by a resource locking);
> 
> Generally once streaming starts drivers should return -EBUSY for such
> calls. Nothing to do with locking in general.

Yes, that's exactly what I said. This is a resource locking: you cannot
change several stream properties while streaming (yet, you can change a
few ones, like bright, contrast, etc).

>> then, maybe the better would be to not call the hooks for those ioctls.
>> It may be useful to do some perf tests and measure the real penalty before
>> adding
>> any extra code to exclude the buffer ioctls from the hook logic.
> 
> Yuck. We want something easy to understand. Like: 'the hook is called for
> every ioctl'. Not: 'the hook is called for every ioctl except this one and
> this one and this one'. Then the driver might have to do different things
> for different ioctls just because some behind-the-scene logic dictated
> that the hook isn't called in some situations.
> 
> I have said it before and I'll say it again: the problem with V4L2 drivers
> has never been performance since all the heavy lifting is done with DMA,
> the problem is complexity. Remember: premature optimization is the root of
> all evil. If a driver really needs the last drop of performance (for what,
> I wonder?) then it can choose to just not implement those hooks and do
> everything itself.

I tend to agree with you. All I'm saying is that it is valuable to double
check the impacts before committing it.

-- 

Cheers,
Mauro

Re: [RFC] Serialization flag example

[Hans Verkuil]

> Hans Verkuil wrote:
>> On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote:
>>> On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
>>>> On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote:
>>>>> After looking at the proposed solution, I personally find the
>>>>> suggestion for a serialization flag to be quite ridiculous. As others
>>>>> have mentioned, the mere presence of the flag means that driver
>>>>> writers will gloss over any concurrency issues that might exist in
>>>>> their driver on the mere assumption that specifying the serialization
>>>>> flag will handle it all for them.
>>>> I happen to agree with this. Proper locking is difficult, but that's
>>>> not a
>>>> reason to introduce such a workaround. I'd much rather see proper
>>>> documentation for driver developers on how to implement locking
>>>> properly.
>>> I've taken a different approach in another tree:
>>>
>>> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/
>>>
>>> It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use
>>> these
>>> to do things like prio checking and taking your own mutex to serialize
>>> the
>>> ioctl call.
>>>
>>> This might be a good compromise between convenience and not hiding
>>> anything.
>>
>> I realized that something like this is needed anyway if we go ahead with
>> the
>> new control framework. That exposes controls in sysfs, but if you set a
>> control
>> from sysfs, then that bypasses the ioctl completely. So you need a way
>> to hook
>> into whatever serialization scheme you have (if any).
>>
>> It is trivial to get to the video_device struct in the control handler
>> and
>> from there you can access ioctl_ops. So calling the pre/post hooks for
>> the
>> sysfs actions is very simple.
>>
>> The prototype for the hooks needs to change, though, since accesses from
>> sysfs do not provide you with a struct file pointer.
>>
>> Something like this should work:
>>
>> int pre_hook(struct video_device *vdev, enum v4l2_priority prio, int
>> cmd);
>> void post_hook(struct video_device *vdev, int cmd);
>>
>> The prio is there to make priority checking possible. It will be
>> initially
>> unused, but as soon as the events API with the new v4l2_fh struct is
>> merged
>> we can add centralized support for this.
>
> I like this strategy.
>
> My only concern is about performance. Especially in the cases where we
> need to
> handle the command at the hooks, those methods will introduce two extra
> jumps
> to the driver and two extra switches. As the jump will go to a code
> outside
> V4L core, I suspect that they'll likely flush the L1 cache.
>
> If we consider that:
>
> 	- performance is important only for the ioctl's that directly handles
> the streaming (dbuf/dqbuf & friends);

What performance? These calls just block waiting for a frame. How the hell
am I suppose to test performance anyway on something like that?

>
> 	- videobuf has its own lock implementation;
>
> 	- a trivial mutex-based approach won't protect the stream to have
> some parameters modified by a VIDIOC_S_* ioctl (such protection should be
> provided by a resource locking);

Generally once streaming starts drivers should return -EBUSY for such
calls. Nothing to do with locking in general.

> then, maybe the better would be to not call the hooks for those ioctls.
> It may be useful to do some perf tests and measure the real penalty before
> adding
> any extra code to exclude the buffer ioctls from the hook logic.

Yuck. We want something easy to understand. Like: 'the hook is called for
every ioctl'. Not: 'the hook is called for every ioctl except this one and
this one and this one'. Then the driver might have to do different things
for different ioctls just because some behind-the-scene logic dictated
that the hook isn't called in some situations.

I have said it before and I'll say it again: the problem with V4L2 drivers
has never been performance since all the heavy lifting is done with DMA,
the problem is complexity. Remember: premature optimization is the root of
all evil. If a driver really needs the last drop of performance (for what,
I wonder?) then it can choose to just not implement those hooks and do
everything itself.

Regards,

         Hans

>
> --
>
> Cheers,
> Mauro
> --
> 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
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [RFC] Serialization flag example

[Mauro Carvalho Chehab]

Hans Verkuil wrote:
> On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote:
>> On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
>>> On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote:
>>>> After looking at the proposed solution, I personally find the
>>>> suggestion for a serialization flag to be quite ridiculous. As others
>>>> have mentioned, the mere presence of the flag means that driver
>>>> writers will gloss over any concurrency issues that might exist in
>>>> their driver on the mere assumption that specifying the serialization
>>>> flag will handle it all for them.
>>> I happen to agree with this. Proper locking is difficult, but that's not a 
>>> reason to introduce such a workaround. I'd much rather see proper 
>>> documentation for driver developers on how to implement locking properly.
>> I've taken a different approach in another tree:
>>
>> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/
>>
>> It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use these
>> to do things like prio checking and taking your own mutex to serialize the
>> ioctl call.
>>
>> This might be a good compromise between convenience and not hiding anything.
> 
> I realized that something like this is needed anyway if we go ahead with the
> new control framework. That exposes controls in sysfs, but if you set a control
> from sysfs, then that bypasses the ioctl completely. So you need a way to hook
> into whatever serialization scheme you have (if any).
> 
> It is trivial to get to the video_device struct in the control handler and
> from there you can access ioctl_ops. So calling the pre/post hooks for the
> sysfs actions is very simple.
> 
> The prototype for the hooks needs to change, though, since accesses from
> sysfs do not provide you with a struct file pointer.
> 
> Something like this should work:
> 
> int pre_hook(struct video_device *vdev, enum v4l2_priority prio, int cmd);
> void post_hook(struct video_device *vdev, int cmd);
> 
> The prio is there to make priority checking possible. It will be initially
> unused, but as soon as the events API with the new v4l2_fh struct is merged
> we can add centralized support for this.

I like this strategy. 

My only concern is about performance. Especially in the cases where we need to 
handle the command at the hooks, those methods will introduce two extra jumps
to the driver and two extra switches. As the jump will go to a code outside 
V4L core, I suspect that they'll likely flush the L1 cache. 

If we consider that:

	- performance is important only for the ioctl's that directly handles
the streaming (dbuf/dqbuf & friends);

	- videobuf has its own lock implementation;

	- a trivial mutex-based approach won't protect the stream to have
some parameters modified by a VIDIOC_S_* ioctl (such protection should be
provided by a resource locking);

then, maybe the better would be to not call the hooks for those ioctls. 
It may be useful to do some perf tests and measure the real penalty before adding
any extra code to exclude the buffer ioctls from the hook logic.

-- 

Cheers,
Mauro

Re: [RFC] Serialization flag example

[Hans Verkuil]

On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote:
> On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
> > On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote:
> > > After looking at the proposed solution, I personally find the
> > > suggestion for a serialization flag to be quite ridiculous. As others
> > > have mentioned, the mere presence of the flag means that driver
> > > writers will gloss over any concurrency issues that might exist in
> > > their driver on the mere assumption that specifying the serialization
> > > flag will handle it all for them.
> > 
> > I happen to agree with this. Proper locking is difficult, but that's not a 
> > reason to introduce such a workaround. I'd much rather see proper 
> > documentation for driver developers on how to implement locking properly.
> 
> I've taken a different approach in another tree:
> 
> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/
> 
> It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use these
> to do things like prio checking and taking your own mutex to serialize the
> ioctl call.
> 
> This might be a good compromise between convenience and not hiding anything.

I realized that something like this is needed anyway if we go ahead with the
new control framework. That exposes controls in sysfs, but if you set a control
from sysfs, then that bypasses the ioctl completely. So you need a way to hook
into whatever serialization scheme you have (if any).

It is trivial to get to the video_device struct in the control handler and
from there you can access ioctl_ops. So calling the pre/post hooks for the
sysfs actions is very simple.

The prototype for the hooks needs to change, though, since accesses from
sysfs do not provide you with a struct file pointer.

Something like this should work:

int pre_hook(struct video_device *vdev, enum v4l2_priority prio, int cmd);
void post_hook(struct video_device *vdev, int cmd);

The prio is there to make priority checking possible. It will be initially
unused, but as soon as the events API with the new v4l2_fh struct is merged
we can add centralized support for this.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [RFC] Serialization flag example

[Hans Verkuil]

On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
> On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote:
> > After looking at the proposed solution, I personally find the
> > suggestion for a serialization flag to be quite ridiculous. As others
> > have mentioned, the mere presence of the flag means that driver
> > writers will gloss over any concurrency issues that might exist in
> > their driver on the mere assumption that specifying the serialization
> > flag will handle it all for them.
> 
> I happen to agree with this. Proper locking is difficult, but that's not a 
> reason to introduce such a workaround. I'd much rather see proper 
> documentation for driver developers on how to implement locking properly.

I've taken a different approach in another tree:

http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/

It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use these
to do things like prio checking and taking your own mutex to serialize the
ioctl call.

This might be a good compromise between convenience and not hiding anything.

Regards,

	Hans


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [RFC] Serialization flag example

[Laurent Pinchart]

On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote:
> After looking at the proposed solution, I personally find the
> suggestion for a serialization flag to be quite ridiculous. As others
> have mentioned, the mere presence of the flag means that driver
> writers will gloss over any concurrency issues that might exist in
> their driver on the mere assumption that specifying the serialization
> flag will handle it all for them.

I happen to agree with this. Proper locking is difficult, but that's not a 
reason to introduce such a workaround. I'd much rather see proper 
documentation for driver developers on how to implement locking properly.

-- 
Regards,

Laurent Pinchart