Hi Laurent, > Hi Mauro, > > On Tuesday 19 October 2010 12:46:11 Mauro Carvalho Chehab wrote: >> Em 19-10-2010 05:52, Laurent Pinchart escreveu: >> > On Tuesday 19 October 2010 08:54:40 Hans Verkuil wrote: >> >> On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote: >> >>> On Sat, 16 Oct 2010 10:44:56 -0300 Mauro Carvalho Chehab wrote: >> >>>> drivers/media/video/via-camera.c: In function ?viacam_open?: >> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to >> >>>> function ?videobuf_queue_sg_init? >> >>>> >> >>>> The fix for this one is trivial: >> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to >> >>>> function ?videobuf_queue_sg_init? >> >>>> >> >>>> Just add an extra NULL parameter to the function. >> >>> >> >>> So I'm looking into this stuff. The extra NULL parameter is a >> struct >> >>> >> >>> mutex, which seems to be used in one place in videobuf_waiton(): >> >>> is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock); >> >>> >> >>> /* Release vdev lock to prevent this wait from blocking outside >> access >> >>> to >> >>> >> >>> the device. */ >> >>> >> >>> if (is_ext_locked) >> >>> >> >>> mutex_unlock(q->ext_lock); >> >>> >> >>> I'd be most curious to know what the reasoning behind this code is; >> to >> >>> my uneducated eye, it looks like a real hack. How does this >> function >> >>> know who locked ext_lock? Can it really just unlock it safely? It >> >>> seems to me that this is a sign of locking issues which should >> really >> >>> be dealt with elsewhere, but, as I said, I'm uneducated, and the >> >>> changelogs don't help me much. Can somebody educate me? >> >> >> >> We are working on removing the BKL. As part of that effort it is now >> >> possible for drivers to pass a serialization mutex to the v4l core (a >> >> mutex pointer was added to struct video_device). If the core sees >> that >> >> mutex then the core will serialize all open/ioctl/read/write/etc. >> file >> >> ops. So all file ops will in that case be called with that mutex >> held. >> >> Which is fine, but if the driver has to do a blocking wait, then you >> >> need to unlock the mutex first and lock it again afterwards. And >> since >> >> videobuf does a blocking wait it needs to know about that mutex. >> >> >> >> Right now we are in the middle of the transition from BKL to using >> core >> >> locks (and some drivers will do their own locking completely). During >> >> this transition period we have drivers that provide an external lock >> >> and drivers still relying on the BKL in which case videobuf needs to >> >> handle its own locking. Hopefully in 1-2 kernel cycles we will have >> >> abolished the BKL and we can remove the videobuf internal lock and >> use >> >> the external mutex only. >> >> >> >> So yes, it is a bit of a hack but there is actually a plan behind it >> :-) >> > >> > I still believe drivers should be encouraged to handle locking on >> their >> > own. These new "big v4l lock" (one per device) should be used only to >> > remove the BKL in existing drivers. It's a hack that we should work on >> > getting rid of. >> >> It is not a "big lock": it doesn't stop other CPU's, doesn't affect >> other >> hardware, not even another V4L device. Basically, what this new lock >> does >> is to serialize access to the hardware and to the hardware-mirrored >> data. > > The lock serializes all ioctls. That's much more than protecting access to > data (both in system memory and in the hardware). Absolutely correct. That's exactly what this lock does. Serializing all this makes it much easier to prove that a driver is correctly locking everything. For 90% of all our drivers this is all that is needed. Yes, it is at a coarser level than the theoretical optimum, but basically, who cares? For that 90% it is simply sufficient and without any performance penalties. >> On several cases, if you serialize open, close, ioctl, read, write and >> mmap, the hardware will be serialized. >> >> Of course, this doesn't cover 100% of the cases where a lock is needed. >> So, >> if the driver have more fun things like kthreads, alsa, dvb, IR polling, >> etc, the driver will need to lock on other places as well. >> >> A typical V4L driver has lots of functions that need locking: open, >> close, >> read, write, mmap and almost all ioctl (depending on the driver, just a >> very few set of enum ioctl's could eventually not need an ioctl). What >> we >> found is that: >> >> 1) several developers didn't do the right thing since the beginning; > > That's not a valid reason to push new drivers for a very coarse grain > locking > scheme. Developers must not get told to be stupid and don't care about > locks > just because other developers got it wrong in the past. If people don't > get > locking right we need to educate them, not encourage them to understand > even > less of it. Locking is hard in complex drivers. Period. > >> 2) as time goes by, locks got bit roted. >> 3) some drivers were needing to touch on several locks (videobuf, their >> internal priv locks, etc), sometimes generating cases where a dead lock >> would be possible. >> >> On the tests we did so far, the v4l-core assisted lock helped to solve >> some >> locking issues on the very few drivers that were ported. Also, it caused >> a >> regression on a driver where the lock were working ;) >> >> There are basically several opinions about this new schema: some that >> think >> that this is the right thing to do, others think that think that this is >> the wrong thing or that this is acceptable only as a transition for >> BKL-free drivers. > > Indeed, and I belong to the second group. > >> IMO, I think that both ways are acceptable: a core-assisted >> "hardware-access lock" helps to avoid having lots of lock/unlock code at >> the driver, making drivers cleaner and easier to review, and reducing >> the >> risk of lock degradation with time. On the other hand, some drivers may >> require more complex locking schemas, like, for example, devices that >> support several simultaneous independent video streams may have some >> common parts used by all streams that need to be serialized, and other >> parts that can (and should) not be serialized. So, a core-assisted >> locking >> for some cases may cause unneeded long waits. > > A coarse core lock is acceptable in a transition phase to get rid of the > BKL > because we don't have the necessary resources to do it right and right > now. > Our goal should be to get rid of it in the long term as well (although we > will > probably never complete this task, as not all drivers have a wide users > and > developers base). New drivers must thus implement proper locking. > > -- > Regards, > > Laurent Pinchart > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
