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