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 <hverkuil@xxxxxxxxx> wrote: >>> >>> On 11/16/2018 09:43 AM, Tomasz Figa wrote: >>>> Hi Hans, >>>> >>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xxxxxxxxx> 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