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