On Mon, Nov 19, 2018 at 6:54 PM Hans Verkuil <hverk...@xs4all.nl> wrote: > > 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.
Ah, good point. Somehow I assumed that only QBUF/DQBUF would use queue->lock, while anything else would use dev->lock, but that's not the case. Then I can't find any simpler and/or more general fix for now, so I'm okay with this. Another note, don't we need similar error in case of REQBUFS(0), while DQBUF() is waiting? Current patch seems to add one only for count != 0. Best regards, Tomasz
