Hi Hans, On Wednesday 27 November 2013 08:12:24 Hans Verkuil wrote: > On 11/26/2013 04:42 PM, Laurent Pinchart wrote: > > On Friday 22 November 2013 10:02:49 Hans Verkuil wrote: > >> On 11/21/2013 08:04 PM, Laurent Pinchart wrote: > >>> On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote: > >>>> From: Hans Verkuil <hans.verk...@cisco.com> > >>>> > >>>> Rather than taking the mmap semaphore at a relatively high-level > >>>> function, push it down to the place where it is really needed. > >>>> > >>>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with > >>>> other vb2 calls, however, I see no way that any race can happen. > >>> > >>> What about the following scenario ? Both QBUF calls are performed on the > >>> same buffer. > >>> > >>> CPU 0 CPU 1 > >>> --------------------------------------------------------------------- > >>> QBUF QBUF > >>> locks the queue mutex waits for the > >>> queue mutex > >>> vb2_qbuf > >>> vb2_queue_or_prepare_buf > >>> __vb2_qbuf > >>> checks vb->state, calls > >>> __buf_prepare > >>> call_qop(q, wait_prepare, q); > >>> unlocks the queue mutex > >>> > >>> locks the queue mutex > >>> vb2_qbuf > >>> > >>> vb2_queue_or_prepare_buf > >>> > >>> __vb2_qbuf > >>> > >>> checks vb->state, calls > >>> > >>> __buf_prepare > >>> > >>> call_qop(q, wait_prepare, q); > >>> > >>> unlocks the queue mutex > >>> queue > >>> the buffer, set buffer > >>> state > >>> to queue > >>> queue the buffer, set buffer > >>> state to queue > >>> > >>> We would thus end up queueing the buffer twice. The vb->state check > >>> needs to be performed after the brief release of the queue mutex. > >> > >> Good point, I hadn't thought about that scenario. However, using mmap_sem > >> to introduce a large critical section just to protect against state > >> changes is IMHO not the right approach. Why not introduce a > >> VB2_BUF_STATE_PREPARING state? > > > > Note that we use the queue mutex to do so, not mmap_sem. The problem is > > that we can't release the queue mutex in the middle of a critical section > > without risking being preempted by another task. Introducing a new state > > might be possible if it effectively breaks the critical section in two > > independent parts. > > > >> That's set at the start of __buf_prepare while the queue mutex is still > >> held, and which prevents other threads of queuing the same buffer again. > >> If the prepare fails, then the state is reverted back to DEQUEUED. > >> > >> __fill_v4l2_buffer() will handle the PREPARING state as if it was the > >> DEQUEUED state. > >> > >> What do you think? > > > > I'll have to review that in details given the potential complexity of > > locking issues :-) I'm not opposed to the idea, if it works I believe we > > should do it. > > Do you want to think about this first, or shall I make a new patch that you > can then review?
As the devil is in the details I'd prefer a patch. I would have to write one to think about this anyway :-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
