Re: [PATCH v2 1/1] vb2: Only requeue buffers immediately once streaming is started

[Hans Verkuil <hverkuil@xs4all.nl>]

On 07/03/2015 03:28 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Hans Verkuil wrote:
>> On 07/03/2015 02:47 PM, Sakari Ailus wrote:
>>> Buffers can be returned back to videobuf2 in driver's streamon handler. In
>>> this case vb2_buffer_done() with buffer state VB2_BUF_STATE_QUEUED will
>>> cause the driver's buf_queue vb2 operation to be called, queueing the same
>>> buffer again only to be returned to videobuf2 using vb2_buffer_done() and so
>>> on.
>>>
>>> Add a new buffer state VB2_BUF_STATE_REQUEUEING which, when used as the
>>
>> It's spelled as requeuing (no e). The verb is 'to queue', but the -ing form is
>> queuing. Check the dictionary: http://dictionary.reference.com/browse/queuing
> 
> My dictionary disagrees with yours. :-)
> 
> http://dictionary.cambridge.org/dictionary/british/queue?q=queueing

$ git grep -i queueing|wc
    655    5660   54709
$ git grep -i queuing|wc
    650    5623   55249

That's not helpful either...

On the other hand:

$ git grep -i queuing drivers/media/|wc
     19     200    1846
$ git grep -i queueing drivers/media/|wc
      2      25     203

Within drivers/media there seems to be a clear preference for queuing :-)

But yes, both spellings are OK, but for me queueing has way too many vowels!
And interestingly, my spellchecker thinks queueing is wrong.

> 
>>
>>> state argument to vb2_buffer_done(), will result in buffers queued to the
>>> driver. Using VB2_BUF_STATE_QUEUED will leave the buffer to videobuf2, as it
>>> was before "[media] vb2: allow requeuing buffers while streaming".
>>>
>>> Fixes: ce0eff016f72 ("[media] vb2: allow requeuing buffers while streaming")
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Cc: stable@vger.kernel.org # for v4.1
>>> ---
>>> since v1:
>>>
>>> - Instead of relying on q->start_streaming_called and q->streaming, add a
>>>    new buffer state VB2_BUF_STATE_REQUEUEING, as suggested by Hans. The
>>>    cobalt driver will need the new flag as it returns the buffer back to the
>>>    driver's queue, the rest will continue using VB2_BUF_STATE_QUEUED.
>>>
>>>   drivers/media/pci/cobalt/cobalt-irq.c    |  2 +-
>>>   drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++----
>>>   include/media/videobuf2-core.h           |  2 ++
>>>   3 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/cobalt/cobalt-irq.c b/drivers/media/pci/cobalt/cobalt-irq.c
>>> index e18f49e..2687cb0 100644
>>> --- a/drivers/media/pci/cobalt/cobalt-irq.c
>>> +++ b/drivers/media/pci/cobalt/cobalt-irq.c
>>> @@ -134,7 +134,7 @@ done:
>>>   	   also know about dropped frames. */
>>>   	cb->vb.v4l2_buf.sequence = s->sequence++;
>>>   	vb2_buffer_done(&cb->vb, (skip || s->unstable_frame) ?
>>> -			VB2_BUF_STATE_QUEUED : VB2_BUF_STATE_DONE);
>>> +			VB2_BUF_STATE_REQUEUEING : VB2_BUF_STATE_DONE);
>>>   }
>>>
>>>   irqreturn_t cobalt_irq_handler(int irq, void *dev_id)
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 1a096a6..ca8c041 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -1182,7 +1182,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>>>
>>>   	if (WARN_ON(state != VB2_BUF_STATE_DONE &&
>>>   		    state != VB2_BUF_STATE_ERROR &&
>>> -		    state != VB2_BUF_STATE_QUEUED))
>>> +		    state != VB2_BUF_STATE_QUEUED &&
>>> +		    state != VB2_BUF_STATE_REQUEUEING))
>>>   		state = VB2_BUF_STATE_ERROR;
>>>
>>>   #ifdef CONFIG_VIDEO_ADV_DEBUG
>>> @@ -1199,15 +1200,21 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>>>   	for (plane = 0; plane < vb->num_planes; ++plane)
>>>   		call_void_memop(vb, finish, vb->planes[plane].mem_priv);
>>>
>>> -	/* Add the buffer to the done buffers list */
>>>   	spin_lock_irqsave(&q->done_lock, flags);
>>> -	vb->state = state;
>>> -	if (state != VB2_BUF_STATE_QUEUED)
>>> +	if (state == VB2_BUF_STATE_QUEUED ||
>>> +	    state == VB2_BUF_STATE_REQUEUEING) {
>>> +		vb->state = VB2_BUF_STATE_QUEUED;
>>> +	} else {
>>> +		/* Add the buffer to the done buffers list */
>>>   		list_add_tail(&vb->done_entry, &q->done_list);
>>> +		vb->state = state;
>>> +	}
>>>   	atomic_dec(&q->owned_by_drv_count);
>>>   	spin_unlock_irqrestore(&q->done_lock, flags);
>>>
>>>   	if (state == VB2_BUF_STATE_QUEUED) {
>>> +		return;
>>> +	} else if (state == VB2_BUF_STATE_REQUEUEING) {
>>
>> No 'else' is needed here since the 'if' just returns.
> 
> Will fix.
> 

Regards,

	Hans