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@xxxxxxxxxxxxx>
---
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;
}
