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

[Helen Koike]

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:

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);
>>  }
>>  
>>