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

[Tomasz Figa <tfiga@chromium.org>]

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@collabora.com>
> Acked-by: Helen Koike <helen.koike@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?

>  		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