On 11/19/2018 09:44 AM, Hans Verkuil wrote:
> On 11/19/2018 06:27 AM, Tomasz Figa wrote:
>> On Fri, Nov 16, 2018 at 6:45 PM Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>
>>> On 11/16/2018 09:43 AM, Tomasz Figa wrote:
>>>> Hi Hans,
>>>>
>>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>>>
>>>>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>>>>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>>>>
>>>>> However, if userspace dup()ped the video device filehandle, then it is
>>>>> possible to read or call DQBUF from two filehandles at the same time.
>>>>>
>>>>
>>>> What side effects would reading have?
>>>>
>>>> As for another DQBUF in parallel, perhaps that's actually a valid
>>>> operation that should be handled? I can imagine that one could want to
>>>> have multiple threads dequeuing buffers as they become available, so
>>>> that no dispatch thread is needed.
>>>
>>> I think parallel DQBUFs can be done, but it has never been tested, nor
>>> has vb2 been designed with that in mind. I also don't see the use-case
>>> since if you have, say, two DQBUFs in parallel, then it will be random
>>> which DQBUF gets which frame.
>>>
>>
>> Any post processing that operates only on single frame data would be
>> able to benefit from multiple threads, with results ordered after the
>> processing, based on timestamps.
>>
>> Still, if that's not something we've ever claimed as supported and
>> couldn't work correctly with current code, it sounds fair to
>> completely forbid it for now.
>>
>>> If we ever see a need for this, then that needs to be designed and tested
>>> properly.
>>>
>>>>
>>>>> It is also possible to call REQBUFS from one filehandle while the other
>>>>> is waiting for a buffer. This will remove all the buffers and reallocate
>>>>> new ones. Removing all the buffers isn't the problem here (that's already
>>>>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>>>>> aware that the buffers have changed.
>>>>>
>>>>> This is fixed by setting a flag whenever the lock is released while
>>>>> waiting
>>>>> for a buffer to arrive. And checking the flag where needed so we can
>>>>> return
>>>>> -EBUSY.
>>>>
>>>> Maybe it would make more sense to actually handle those side effects?
>>>> Such waiting DQBUF would then just fail in the same way as if it
>>>> couldn't get a buffer (or if it's blocking, just retry until a correct
>>>> buffer becomes available?).
>>>
>>> That sounds like a good idea, but it isn't.
>>>
>>> With this patch you can't call REQBUFS to reallocate buffers while a thread
>>> is waiting for a buffer.
>>>
>>> If I allow this, then the problem moves to when the thread that called
>>> REQBUFS
>>> calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF
>>> will
>>> mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS
>>> ensure
>>> that only the DQBUF that relied on the old buffers is stopped?
>>>
>>> It sounds nice, but the more I think about it, the more problems I see with
>>> it.
>>>
>>> I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
>>> thread is still waiting for a buffer.
>>>
>>> That said, I think one test is missing in vb2_core_create_bufs: there too it
>>> should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
>>> REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
>>> buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
>>>
>>> Admittedly, that would require some extremely unfortunate scheduling, but
>>> it is easy enough to check this.
>>
>> I thought a bit more about this and I agree with you. We should keep
>> things as simple as possible.
>>
>> Another thing that came to my mind is that the problematic scenario
>> described in the commit message can happen only if queue->lock ==
>> dev->lock. I wonder how likely it would be to mandate queue->lock !=
>> dev->lock?
>
> My plan is to switch vivid to that model. Expect patches for that today.
> One thing I noticed is that there is an issue with calling queue_setup
> in that case. I have a separate patch for that, so just read it when I
> post it.
Note that this specific scenario can happen regardless of whether
queue->lock == dev->lock or not.
Regards,
Hans
