[PATCH 1/4] media: staging: rkisp1: cap: don't set next buffer from rkisp1_vb2_buf_queue

[Helen Koike]

On 7/14/20 12:11 PM, Helen Koike wrote:
> 
> 
> On 7/14/20 9:48 AM, Helen Koike wrote:
>> Hi Dafna,
>>
>> On 7/14/20 9:38 AM, Dafna Hirschfeld wrote:
>>> The function 'rkisp1_vb2_buf_queue' sets the next buffer directly
>>> in case the capture is already streaming but no frame yet arrived
>>> from the sensor. This is an optimization that tries to avoid
>>> dropping a frame.
>>> The call atomic_read(&cap->rkisp1->isp.frame_sequence) is used
>>> to check if a frame arrived. Reading the 'frame_sequence' should
>>> be avoided outside irq handlers to avoid race conditions.
>>>
>>> This patch removes this optimization. Dropping of the first
>>> frames can be avoided if userspace queues the buffers before
>>> start streaming. If userspace starts queueing buffers
>>> only after calling 'streamon' he risks frame drops anyway.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
>>> ---
>>>  drivers/staging/media/rkisp1/rkisp1-capture.c | 13 +------------
>>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index 793ec884c894..572b0949c81f 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -743,18 +743,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>>  		     ispbuf->buff_addr[RKISP1_PLANE_CB]);
>>>  
>>>  	spin_lock_irqsave(&cap->buf.lock, flags);
>>> -
>>> -	/*
>>> -	 * If there's no next buffer assigned, queue this buffer directly
>>> -	 * as the next buffer, and update the memory interface.
>>> -	 */
>>> -	if (cap->is_streaming && !cap->buf.next &&
>>> -	    atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) {
>>> -		cap->buf.next = ispbuf;
>>> -		rkisp1_set_next_buf(cap);
>>
>> Doesn't this mean we'll always drop the first frame? Since the first user buffer will only be configured to
>> the hardware after receiving the first frame? So the first frame will always go to the dummy buffer, no?
> 
> I see this is actually solved in the last patch of this series.
> 
> Maybe this can be re-order, so this patch is after 4/4.
> 
> With or without this re-ordering:

As discussed throught chat, this is not an issue, since we have a minimum amount of buffers that need
to be queued before starting the stream, so we'll never have frame_sequence == -1 (which represents the first frame)
with an empty queue, so no patch re-ordering is needed.

Regards,
Helen

> 
> Acked-by: Helen Koike <helen.koike at collabora.com>
> 
> Thanks
> Helen
> 
>>
>> Thanks
>> Helen
>>
>>> -	} else {
>>> -		list_add_tail(&ispbuf->queue, &cap->buf.queue);
>>> -	}
>>> +	list_add_tail(&ispbuf->queue, &cap->buf.queue);
>>>  	spin_unlock_irqrestore(&cap->buf.lock, flags);
>>>  }
>>>  
>>>