[PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence

[Dafna Hirschfeld]

Hi,

Am 25.09.20 um 22:42 schrieb Tomasz Figa:
> Hi Dafna,
> 
> On Tue, Sep 22, 2020 at 01:33:56PM +0200, Dafna Hirschfeld wrote:
>> The isp.frame_sequence is now read only from the irq handlers
>> that are all fired from the same interrupt, so there is not need
>> for atomic operation.
>> In addition, the frame seq incrementation is moved from
>> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
>> clearer and the incorrect inline comment is removed.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
>> Acked-by: Helen Koike <helen.koike at collabora.com>
>>
>> ---
>> changes since v2:
>> add a closing "}" to if condition
>> remove usless inline comment
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>>   5 files changed, 9 insertions(+), 16 deletions(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 0632582a95b4..1c762a369b63 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>>   	curr_buf = cap->buf.curr;
>>   
>>   	if (curr_buf) {
>> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
>> +		curr_buf->vb.sequence = isp->frame_sequence;
> 
> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
> memory-intensive system load, like video encoding and graphics rendering,
> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
> the V_START for the next frame.
> 
> I recall you did some testing back in time [1], showing that the two are
> interleaved. Do you remember what CAPTURE resolution was it?

I ran the testing again, I added a patch to allow streaming simultanously from
both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
Then I ran two tests:
stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR

the pixelformat for both is 422P.
I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
The functionality can probably only be tested on the scarlet.

Thanks,
Dafna



> 
>>   		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>>   		curr_buf->vb.field = V4L2_FIELD_NONE;
>>   		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 232bee92d0eb..51c92a251ea5 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -131,7 +131,7 @@ struct rkisp1_isp {
>>   	const struct rkisp1_isp_mbus_info *src_fmt;
>>   	struct mutex ops_lock; /* serialize the subdevice ops */
>>   	bool is_dphy_errctrl_disabled;
>> -	atomic_t frame_sequence;
>> +	__u32 frame_sequence;
> 
> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
> types. For kernel types please just use the plain u32.
> 
> Best regards,
> Tomasz
>