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