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@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?
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
