Hi Dafna, On Tue, Sep 22, 2020 at 01:34:02PM +0200, Dafna Hirschfeld wrote: > The function 'rkisp1_stream_start' calls 'rkisp1_set_next_buf' > which access the buffers, so the call should be protected by > taking the cap->buf.lock. > After this patch, all locks are reviewed and commented so remove > the TODO item "review and comment every lock" > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx> > --- > drivers/staging/media/rkisp1/TODO | 1 - > drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++ > 2 files changed, 2 insertions(+), 1 deletion(-) > Thank you for the patch. Please see my comments inline. > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > index f0c90d1c86a8..9662e9b51c7f 100644 > --- a/drivers/staging/media/rkisp1/TODO > +++ b/drivers/staging/media/rkisp1/TODO > @@ -1,6 +1,5 @@ > * Fix pad format size for statistics and parameters entities. > * Fix checkpatch errors. > -* Review and comment every lock > * Handle quantization > * streaming paths (mainpath and selfpath) check if the other path is streaming > in several places of the code, review this, specially that it doesn't seem it > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index 012c0825a386..b9e56dab77af 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -913,6 +913,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap) > cap->ops->config(cap); > > /* Setup a buffer for the next frame */ > + spin_lock_irq(&cap->buf.lock); > rkisp1_set_next_buf(cap); > cap->ops->enable(cap); > /* It's safe to config ACTIVE and SHADOW regs for the > @@ -930,6 +931,7 @@ static void rkisp1_stream_start(struct rkisp1_capture *cap) > RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); > rkisp1_set_next_buf(cap); > } > + spin_unlock_irq(&cap->buf.lock); > cap->is_streaming = true; Should the is_streaming flag be protected in some way? Also, similarly to the other nodes, is the flag actually needed? Best regards, Tomasz
