[PATCH 0/4] media: staging: rkisp1: fix possible race conditions in capture

[PATCH 0/4] media: staging: rkisp1: fix possible race conditions in capture

[Dafna Hirschfeld]

This patchset fixes several issues found in capture related
to buffer managings.

Patch 1 - The first patch removes an optimization that seems not useful.
The optimization configures the next buffer in case the capture
already streams and the first buffer is queued just before
the first irq. This is an unlikely scenario. Also
the code reads the field 'frame_sequence' which is updated
on v-start interrupt by 'rkisp1-isp'. Laurent Pinchart mentioned
in a comment that reading the frame_sequence outside of irq
handlers should be avoided due to possible race conditions.

https://patchwork.kernel.org/patch/11066513/#22823561

The entity 'rkisp1-params' also reads 'frame_sequence' outside of the irq handler
a patch(set) will be sent to fix that too.

Patch 2 - The second patch uses buf.lock to protect buf.curr and buf.next
in places where they are not protected.

Patch 3 - moves the code that manages the buffers to be together
with the code that configure the next buffer address to the registers.
This is a preparation for patch 4.

patch 4 - The function 'rkisp1_stream_start' uses the function
'rkisp1_handle_buffer' in order to manage the buffers and configure
the address registers before stream starts. But the function
'rkisp1_handle_buffer' also contains other code that is not needed for
stream start
The patch replace it with calls to rkisp1_set_next_buf.

Dafna Hirschfeld (4):
  media: staging: rkisp1: cap: don't set next buffer from
    rkisp1_vb2_buf_queue
  media: staging: rkisp1: cap: protect buf.curr and buf.next with
    buf.lock
  media: staging: rkisp1: cap: move code that manages the buffers to
    rkisp1_set_next_buf
  media: staging: rkisp1: cap: in stream start, replace calls to
    rkisp1_handle_buffer with rkisp1_set_next_buf

 drivers/staging/media/rkisp1/rkisp1-capture.c | 52 +++++++------------
 1 file changed, 20 insertions(+), 32 deletions(-)

-- 
2.17.1

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

[Dafna Hirschfeld]

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@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);
-	} else {
-		list_add_tail(&ispbuf->queue, &cap->buf.queue);
-	}
+	list_add_tail(&ispbuf->queue, &cap->buf.queue);
 	spin_unlock_irqrestore(&cap->buf.lock, flags);
 }
 
-- 
2.17.1

[PATCH 2/4] media: staging: rkisp1: cap: protect buf.curr and buf.next with buf.lock

[Dafna Hirschfeld]

The spinlock buf.lock should protect access to the buffers.
This includes the buffers in buf.queue and also buf.curr and
buf.next. The function 'rkisp1_set_next_buf' access buf.next
therefore it should also be protected with the lock.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 572b0949c81f..fa3eaeac2a00 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -617,10 +617,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
 static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 {
 	struct rkisp1_isp *isp = &cap->rkisp1->isp;
-	struct rkisp1_buffer *curr_buf = cap->buf.curr;
+	struct rkisp1_buffer *curr_buf;
 	unsigned long flags;
 
 	spin_lock_irqsave(&cap->buf.lock, flags);
+	curr_buf = cap->buf.curr;
 
 	if (curr_buf) {
 		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
@@ -640,9 +641,9 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 						 queue);
 		list_del(&cap->buf.next->queue);
 	}
-	spin_unlock_irqrestore(&cap->buf.lock, flags);
 
 	rkisp1_set_next_buf(cap);
+	spin_unlock_irqrestore(&cap->buf.lock, flags);
 }
 
 void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
-- 
2.17.1

[PATCH 3/4] media: staging: rkisp1: cap: move code that manages the buffers to rkisp1_set_next_buf

[Dafna Hirschfeld]

The function 'rkisp1_set_next_buf' configures the registers
according to 'cap->buf.next'. It is called after updating
'cap->buf.next' and 'cap->buf.curr'. This patch moves the
code that updates those fields to rkisp1_set_next_buf.
This is a preparation for later patch that change a call to
'rkisp1_handle_buffer' with a call to 'rkisp1_set_next_buf'.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 30 +++++++++----------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index fa3eaeac2a00..7f400aefe550 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -575,12 +575,16 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap)
 
 static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
 {
-	/*
-	 * Use the dummy space allocated by dma_alloc_coherent to
-	 * throw data if there is no available buffer.
-	 */
-	if (cap->buf.next) {
-		u32 *buff_addr = cap->buf.next->buff_addr;
+	cap->buf.curr = cap->buf.next;
+	cap->buf.next = NULL;
+
+	if (!list_empty(&cap->buf.queue)) {
+		u32 *buff_addr;
+
+		cap->buf.next = list_first_entry(&cap->buf.queue, struct rkisp1_buffer, queue);
+		list_del(&cap->buf.next->queue);
+
+		buff_addr = cap->buf.next->buff_addr;
 
 		rkisp1_write(cap->rkisp1,
 			     buff_addr[RKISP1_PLANE_Y],
@@ -592,6 +596,10 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
 			     buff_addr[RKISP1_PLANE_CR],
 			     cap->config->mi.cr_base_ad_init);
 	} else {
+		/*
+		 * Use the dummy space allocated by dma_alloc_coherent to
+		 * throw data if there is no available buffer.
+		 */
 		rkisp1_write(cap->rkisp1,
 			     cap->buf.dummy.dma_addr,
 			     cap->config->mi.y_base_ad_init);
@@ -632,16 +640,6 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 		cap->rkisp1->debug.frame_drop[cap->id]++;
 	}
 
-	cap->buf.curr = cap->buf.next;
-	cap->buf.next = NULL;
-
-	if (!list_empty(&cap->buf.queue)) {
-		cap->buf.next = list_first_entry(&cap->buf.queue,
-						 struct rkisp1_buffer,
-						 queue);
-		list_del(&cap->buf.next->queue);
-	}
-
 	rkisp1_set_next_buf(cap);
 	spin_unlock_irqrestore(&cap->buf.lock, flags);
 }
-- 
2.17.1

[PATCH 4/4] media: staging: rkisp1: cap: in stream start, replace calls to rkisp1_handle_buffer with rkisp1_set_next_buf

[Dafna Hirschfeld]

The function 'rkisp1_stream_start' calls 'rkisp1_handle_buffer'
in order to update the 'buf.curr' and 'buf.next' fields and
configure the device before streaming starts. This cause a wrong
increment of the debugs field 'frame_drop'. This patch replaces
the call to 'rkisp1_handle_buffer' with a call to
'rkisp1_set_next_buf'.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 7f400aefe550..c05280950ea0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -916,7 +916,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
 	cap->ops->config(cap);
 
 	/* Setup a buffer for the next frame */
-	rkisp1_handle_buffer(cap);
+	rkisp1_set_next_buf(cap);
 	cap->ops->enable(cap);
 	/* It's safe to config ACTIVE and SHADOW regs for the
 	 * first stream. While when the second is starting, do NOT
@@ -931,7 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
 		/* force cfg update */
 		rkisp1_write(rkisp1,
 			     RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT);
-		rkisp1_handle_buffer(cap);
+		rkisp1_set_next_buf(cap);
 	}
 	cap->is_streaming = true;
 }
-- 
2.17.1

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

[Helen Koike]

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@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?

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

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

Re: [PATCH 2/4] media: staging: rkisp1: cap: protect buf.curr and buf.next with buf.lock

[Helen Koike]

On 7/14/20 9:38 AM, Dafna Hirschfeld wrote:
> The spinlock buf.lock should protect access to the buffers.
> This includes the buffers in buf.queue and also buf.curr and
> buf.next. The function 'rkisp1_set_next_buf' access buf.next
> therefore it should also be protected with the lock.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 572b0949c81f..fa3eaeac2a00 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -617,10 +617,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
>  static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>  {
>  	struct rkisp1_isp *isp = &cap->rkisp1->isp;
> -	struct rkisp1_buffer *curr_buf = cap->buf.curr;
> +	struct rkisp1_buffer *curr_buf;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&cap->buf.lock, flags);
> +	curr_buf = cap->buf.curr;
>  
>  	if (curr_buf) {
>  		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
> @@ -640,9 +641,9 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>  						 queue);
>  		list_del(&cap->buf.next->queue);
>  	}
> -	spin_unlock_irqrestore(&cap->buf.lock, flags);
>  
>  	rkisp1_set_next_buf(cap);
> +	spin_unlock_irqrestore(&cap->buf.lock, flags);
>  }
>  
>  void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> 

Re: [PATCH 3/4] media: staging: rkisp1: cap: move code that manages the buffers to rkisp1_set_next_buf

[Helen Koike]

On 7/14/20 9:38 AM, Dafna Hirschfeld wrote:
> The function 'rkisp1_set_next_buf' configures the registers
> according to 'cap->buf.next'. It is called after updating
> 'cap->buf.next' and 'cap->buf.curr'. This patch moves the
> code that updates those fields to rkisp1_set_next_buf.
> This is a preparation for later patch that change a call to
> 'rkisp1_handle_buffer' with a call to 'rkisp1_set_next_buf'.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 30 +++++++++----------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index fa3eaeac2a00..7f400aefe550 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -575,12 +575,16 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap)
>  
>  static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
>  {
> -	/*
> -	 * Use the dummy space allocated by dma_alloc_coherent to
> -	 * throw data if there is no available buffer.
> -	 */
> -	if (cap->buf.next) {
> -		u32 *buff_addr = cap->buf.next->buff_addr;
> +	cap->buf.curr = cap->buf.next;
> +	cap->buf.next = NULL;
> +
> +	if (!list_empty(&cap->buf.queue)) {
> +		u32 *buff_addr;
> +
> +		cap->buf.next = list_first_entry(&cap->buf.queue, struct rkisp1_buffer, queue);
> +		list_del(&cap->buf.next->queue);
> +
> +		buff_addr = cap->buf.next->buff_addr;
>  
>  		rkisp1_write(cap->rkisp1,
>  			     buff_addr[RKISP1_PLANE_Y],
> @@ -592,6 +596,10 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
>  			     buff_addr[RKISP1_PLANE_CR],
>  			     cap->config->mi.cr_base_ad_init);
>  	} else {
> +		/*
> +		 * Use the dummy space allocated by dma_alloc_coherent to
> +		 * throw data if there is no available buffer.
> +		 */
>  		rkisp1_write(cap->rkisp1,
>  			     cap->buf.dummy.dma_addr,
>  			     cap->config->mi.y_base_ad_init);
> @@ -632,16 +640,6 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>  		cap->rkisp1->debug.frame_drop[cap->id]++;
>  	}
>  
> -	cap->buf.curr = cap->buf.next;
> -	cap->buf.next = NULL;
> -
> -	if (!list_empty(&cap->buf.queue)) {
> -		cap->buf.next = list_first_entry(&cap->buf.queue,
> -						 struct rkisp1_buffer,
> -						 queue);
> -		list_del(&cap->buf.next->queue);
> -	}
> -
>  	rkisp1_set_next_buf(cap);
>  	spin_unlock_irqrestore(&cap->buf.lock, flags);
>  }
> 

Re: [PATCH 4/4] media: staging: rkisp1: cap: in stream start, replace calls to rkisp1_handle_buffer with rkisp1_set_next_buf

[Helen Koike]

Hi Dafna,

Thanks for the patch, just a small thing below.

On 7/14/20 9:38 AM, Dafna Hirschfeld wrote:
> The function 'rkisp1_stream_start' calls 'rkisp1_handle_buffer'
> in order to update the 'buf.curr' and 'buf.next' fields and
> configure the device before streaming starts. This cause a wrong
> increment of the debugs field 'frame_drop'. This patch replaces
> the call to 'rkisp1_handle_buffer' with a call to
> 'rkisp1_set_next_buf'.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 7f400aefe550..c05280950ea0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -916,7 +916,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>  	cap->ops->config(cap);
>  
>  	/* Setup a buffer for the next frame */
> -	rkisp1_handle_buffer(cap);
> +	rkisp1_set_next_buf(cap);

You should also protect with the cap->buf.lock, or move the lock protection to rkisp1_set_next_buf() and update patch 2/4.

Regards,
Helen

>  	cap->ops->enable(cap);
>  	/* It's safe to config ACTIVE and SHADOW regs for the
>  	 * first stream. While when the second is starting, do NOT
> @@ -931,7 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>  		/* force cfg update */
>  		rkisp1_write(rkisp1,
>  			     RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT);
> -		rkisp1_handle_buffer(cap);
> +		rkisp1_set_next_buf(cap);
>  	}
>  	cap->is_streaming = true;
>  }
> 

Re: [PATCH 4/4] media: staging: rkisp1: cap: in stream start, replace calls to rkisp1_handle_buffer with rkisp1_set_next_buf

[Dafna Hirschfeld]

Hi,

On 14.07.20 17:11, Helen Koike wrote:
> Hi Dafna,
> 
> Thanks for the patch, just a small thing below.
> 
> On 7/14/20 9:38 AM, Dafna Hirschfeld wrote:
>> The function 'rkisp1_stream_start' calls 'rkisp1_handle_buffer'
>> in order to update the 'buf.curr' and 'buf.next' fields and
>> configure the device before streaming starts. This cause a wrong
>> increment of the debugs field 'frame_drop'. This patch replaces
>> the call to 'rkisp1_handle_buffer' with a call to
>> 'rkisp1_set_next_buf'.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 7f400aefe550..c05280950ea0 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -916,7 +916,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>>   	cap->ops->config(cap);
>>   
>>   	/* Setup a buffer for the next frame */
>> -	rkisp1_handle_buffer(cap);
>> +	rkisp1_set_next_buf(cap);
> 
> You should also protect with the cap->buf.lock, or move the lock protection to rkisp1_set_next_buf() and update patch 2/4.

I am not sure if protection here is needed. The streaming is not enabled yet, so
it is promised that we don't run the isr in parallel and ioctls are already synchronized by the framework
so I think it is safe not to use locking here , but I'm not sure actually.

Thanks,
Dafna


> 
> Regards,
> Helen
> 
>>   	cap->ops->enable(cap);
>>   	/* It's safe to config ACTIVE and SHADOW regs for the
>>   	 * first stream. While when the second is starting, do NOT
>> @@ -931,7 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>>   		/* force cfg update */
>>   		rkisp1_write(rkisp1,
>>   			     RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT);
>> -		rkisp1_handle_buffer(cap);
>> +		rkisp1_set_next_buf(cap);
>>   	}
>>   	cap->is_streaming = true;
>>   }
>>

Re: [PATCH 4/4] media: staging: rkisp1: cap: in stream start, replace calls to rkisp1_handle_buffer with rkisp1_set_next_buf

[Helen Koike]

On 7/15/20 4:36 AM, Dafna Hirschfeld wrote:
> Hi,
> 
> On 14.07.20 17:11, Helen Koike wrote:
>> Hi Dafna,
>>
>> Thanks for the patch, just a small thing below.
>>
>> On 7/14/20 9:38 AM, Dafna Hirschfeld wrote:
>>> The function 'rkisp1_stream_start' calls 'rkisp1_handle_buffer'
>>> in order to update the 'buf.curr' and 'buf.next' fields and
>>> configure the device before streaming starts. This cause a wrong
>>> increment of the debugs field 'frame_drop'. This patch replaces
>>> the call to 'rkisp1_handle_buffer' with a call to
>>> 'rkisp1_set_next_buf'.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index 7f400aefe550..c05280950ea0 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -916,7 +916,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>>>       cap->ops->config(cap);
>>>         /* Setup a buffer for the next frame */
>>> -    rkisp1_handle_buffer(cap);
>>> +    rkisp1_set_next_buf(cap);
>>
>> You should also protect with the cap->buf.lock, or move the lock protection to rkisp1_set_next_buf() and update patch 2/4.
> 
> I am not sure if protection here is needed. The streaming is not enabled yet, so
> it is promised that we don't run the isr in parallel and ioctls are already synchronized by the framework
> so I think it is safe not to use locking here , but I'm not sure actually.

I was just wondering in case we re-start a stream quickly after stopping it, but since rkisp1_stream_stop() actually waits
for the hardware, so I don't think there is an issue indeed.

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> 
> Thanks,
> Dafna
> 
> 
>>
>> Regards,
>> Helen
>>
>>>       cap->ops->enable(cap);
>>>       /* It's safe to config ACTIVE and SHADOW regs for the
>>>        * first stream. While when the second is starting, do NOT
>>> @@ -931,7 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap)
>>>           /* force cfg update */
>>>           rkisp1_write(rkisp1,
>>>                    RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT);
>>> -        rkisp1_handle_buffer(cap);
>>> +        rkisp1_set_next_buf(cap);
>>>       }
>>>       cap->is_streaming = true;
>>>   }
>>>

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