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

Message ID 20200714123832.28011-5-dafna.hirschfeld@collabora.com
State New
Headers show
Series
  • Untitled series #318037
Related show

Commit Message

Dafna Hirschfeld July 14, 2020, 12:38 p.m. UTC
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(-)

Comments

Helen Koike July 14, 2020, 3:11 p.m. UTC | #1
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;
>  }
>
Dafna Hirschfeld July 15, 2020, 7:36 a.m. UTC | #2
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;
>>   }
>>
Helen Koike July 15, 2020, 10:04 a.m. UTC | #3
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;
>>>   }
>>>

Patch
diff mbox series

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