| Message ID | 20200922113402.12442-13-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: various bug fixes | expand |
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@collabora.com> > --- > 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
Em Tue, 22 Sep 2020 13:34:02 +0200 Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu: > 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@collabora.com> > --- > drivers/staging/media/rkisp1/TODO | 1 - > drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++ > 2 files changed, 2 insertions(+), 1 deletion(-) > > 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 FYI, There was a trivial context conflict here. Basically, the upstream version has this: * Add uapi docs. Remember to add documentation of how quantization is handled. I solved the conflict, but as some patches for rkisp1 got merged on a different pull request, and there were some uapi docs at the other PR, maybe this would need to be revisited. Thanks, Mauro
Am 27.09.20 um 11:43 schrieb Mauro Carvalho Chehab: > Em Tue, 22 Sep 2020 13:34:02 +0200 > Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu: > >> 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@collabora.com> >> --- >> drivers/staging/media/rkisp1/TODO | 1 - >> drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++ >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> 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 > > > FYI, > > There was a trivial context conflict here. Basically, the upstream > version has this: > > > * Add uapi docs. Remember to add documentation of how quantization is handled. > > I solved the conflict, but as some patches for rkisp1 got merged > on a different pull request, and there were some uapi docs at the > other PR, maybe this would need to be revisited. I added the "Configuring Quantization" documentation in Documentation/admin-guide/media/rkisp1.rst so the TODO item "* Add uapi docs. Remember to add documentation of how quantization is handled." can be removed. Thanks, Dafna > > Thanks, > Mauro >
Em Sun, 27 Sep 2020 11:54:10 +0200 Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu: > Am 27.09.20 um 11:43 schrieb Mauro Carvalho Chehab: > > Em Tue, 22 Sep 2020 13:34:02 +0200 > > Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu: > > > >> 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@collabora.com> > >> --- > >> drivers/staging/media/rkisp1/TODO | 1 - > >> drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++ > >> 2 files changed, 2 insertions(+), 1 deletion(-) > >> > >> 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 > > > > > > FYI, > > > > There was a trivial context conflict here. Basically, the upstream > > version has this: > > > > > > * Add uapi docs. Remember to add documentation of how quantization is handled. > > > > I solved the conflict, but as some patches for rkisp1 got merged > > on a different pull request, and there were some uapi docs at the > > other PR, maybe this would need to be revisited. > > I added the "Configuring Quantization" documentation in Documentation/admin-guide/media/rkisp1.rst > so the TODO item > "* Add uapi docs. Remember to add documentation of how quantization is handled." > can be removed. Ok. Please send a followup patch with a "fixes: " tag mentioned the commit that added it. Thanks, Mauro
Am 27.09.20 um 13:05 schrieb Mauro Carvalho Chehab: > Em Sun, 27 Sep 2020 11:54:10 +0200 > Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu: > >> Am 27.09.20 um 11:43 schrieb Mauro Carvalho Chehab: >>> Em Tue, 22 Sep 2020 13:34:02 +0200 >>> Dafna Hirschfeld <dafna.hirschfeld@collabora.com> escreveu: >>> >>>> 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@collabora.com> >>>> --- >>>> drivers/staging/media/rkisp1/TODO | 1 - >>>> drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++ >>>> 2 files changed, 2 insertions(+), 1 deletion(-) >>>> >>>> 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 >>> >>> >>> FYI, >>> >>> There was a trivial context conflict here. Basically, the upstream >>> version has this: >>> >>> >>> * Add uapi docs. Remember to add documentation of how quantization is handled. >>> >>> I solved the conflict, but as some patches for rkisp1 got merged >>> on a different pull request, and there were some uapi docs at the >>> other PR, maybe this would need to be revisited. >> >> I added the "Configuring Quantization" documentation in Documentation/admin-guide/media/rkisp1.rst >> so the TODO item >> "* Add uapi docs. Remember to add documentation of how quantization is handled." >> can be removed. > > Ok. Please send a followup patch with a "fixes: " tag mentioned the > commit that added it. sent just now, Thanks, Dafna > > Thanks, > Mauro >
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; }
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@collabora.com> --- drivers/staging/media/rkisp1/TODO | 1 - drivers/staging/media/rkisp1/rkisp1-capture.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)