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