Re: [RFC PATCH 0/8] V4L BKL removal: first round

["Hans Verkuil" <hverkuil@xs4all.nl>]

> Em 15-11-2010 07:49, Hans Verkuil escreveu:
>>
>>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
>>>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
>>>>> On Sunday 14 November 2010, Hans Verkuil wrote:
>>>>>> This patch series converts 24 v4l drivers to unlocked_ioctl. These
>>>> are low
>>>>>> hanging fruit but you have to start somewhere :-)
>>>>>>
>>>>>> The first patch replaces mutex_lock in the V4L2 core by
>>>> mutex_lock_interruptible
>>>>>> for most fops.
>>>>>
>>>>> The patches all look good as far as I can tell, but I suppose the
>>>> title is
>>>>> obsolete now that the BKL has been replaced with a v4l-wide mutex,
>>>> which
>>>>> is what you are removing in the series.
>>>>
>>>> I guess I have to rename it, even though strictly speaking the branch
>>>> I'm
>>>> working in doesn't have your patch merged yet.
>>>>
>>>> BTW, replacing the BKL with a static mutex is rather scary: the BKL
>>>> gives up
>>>> the lock whenever you sleep, the mutex doesn't. Since sleeping is very
>>>> common
>>>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a
>>>> new frame
>>>> to arrive), this will make it impossible for another process to access
>>>> any
>>>> v4l2 device node while the ioctl is sleeping.
>>>>
>>>> I am not sure whether that is what you intended. Or am I missing
>>>> something?
>>>
>>> I was aware that something like this could happen, but I apparently
>>> misjudged how big the impact is. The general pattern for ioctls is that
>>> those that get called frequently do not sleep, so it can almost always
>>> be
>>> called with a mutex held.
>>
>> True in general, but most definitely not true for V4L. The all important
>> VIDIOC_DQBUF ioctl will almost always sleep.
>>
>> Mauro, I think this patch will have to be reverted and we just have to
>> do
>> the hard work ourselves.
>
> The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device
> ready
> for stream. During the qbuf/dqbuf loop, the only other ioctls that may
> appear are
> the control change ioctl's, to adjust things like bright. I doubt that his
> will
> cause a really serious trouble.

Yes, it does. Anyone who is using multiple capture/output devices at the
same time will be affected. For example, anyone who uses the davinci
dm6467 driver for both input and output. And yes, that's what we use at
work. And ship to thousands of customers. Or think about surveillance
applications where you are capturing from many streams simultaneously.

This change *does* cause serious trouble.

>
> On the other hand, currently, if BKL is disabled, the entire V4L subsystem
> is
> disabled.
>
> So, IMO, the impact of having Arnd's patch applied is less than just
> having
> almost all drivers disabled if BKL is not compiled. So, I prefer to apply
> his patch and then fix it, driver by driver, than to disable the entire
> subsystem on .37.

Can't we test for CONFIG_LOCK_KERNEL and switch to lock_kernel if set? For
now it is just a kernel config option.

That seems much more reasonable.

Regards,

          Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco