[libcamera-devel,3/3] src: ipa: raspberrypi: Fix initial AGC oscillation for imx219 sensor
diff mbox series

Message ID 20201125113640.20246-4-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi AGC improvements
Related show

Commit Message

David Plowman Nov. 25, 2020, 11:36 a.m. UTC
The exposure times in the exposure modes were causing AGC oscillations
because the algorithm was demanding long unachievable exposure times
but, without working sensor metadata, thought it was getting them when
actually it was not. We fix it by making the exposure profile request
only achievable exposure times, as we do for the ov5647 tuning.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/data/imx219.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Naushir Patuck Nov. 26, 2020, 9:27 a.m. UTC | #1
Hi David,

Thank you for your patch.  You beat me to it, I had a similar change in my
framerate set (but limited to 66ms) :)

On Wed, 25 Nov 2020 at 11:36, David Plowman <david.plowman@raspberrypi.com>
wrote:

> The exposure times in the exposure modes were causing AGC oscillations
> because the algorithm was demanding long unachievable exposure times
> but, without working sensor metadata, thought it was getting them when
> actually it was not. We fix it by making the exposure profile request
> only achievable exposure times, as we do for the ov5647 tuning.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  src/ipa/raspberrypi/data/imx219.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/data/imx219.json
> b/src/ipa/raspberrypi/data/imx219.json
> index b03a7beb..212f8b9a 100644
> --- a/src/ipa/raspberrypi/data/imx219.json
> +++ b/src/ipa/raspberrypi/data/imx219.json
> @@ -133,7 +133,7 @@
>              {
>                  "shutter":
>                  [
> -                    100, 10000, 30000, 60000, 120000
> +                    100, 10000, 30000, 30000, 30000
>                  ],
>                  "gain":
>                  [
> @@ -144,7 +144,7 @@
>              {
>                  "shutter":
>                  [
> -                    100, 5000, 10000, 20000, 120000
> +                    100, 5000, 10000, 20000, 30000
>                  ],
>                  "gain":
>                  [
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Nov. 26, 2020, 10:20 a.m. UTC | #2
Hi David,

Thank you for the patch.

On Wed, Nov 25, 2020 at 11:36:40AM +0000, David Plowman wrote:
> The exposure times in the exposure modes were causing AGC oscillations
> because the algorithm was demanding long unachievable exposure times
> but, without working sensor metadata, thought it was getting them when
> actually it was not. We fix it by making the exposure profile request
> only achievable exposure times, as we do for the ov5647 tuning.

This looks good to me, and I assume it will still work with sensor
embedded data, as unachievable exposure times are, well, unachievable
:-)

Is the process to select shutter values documented in the RPi camera
documentation, or the tuning tool ? If someone wants to bring up a new
sensor, how can we ensure a similar bug will not creep in ?

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/ipa/raspberrypi/data/imx219.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/data/imx219.json b/src/ipa/raspberrypi/data/imx219.json
> index b03a7beb..212f8b9a 100644
> --- a/src/ipa/raspberrypi/data/imx219.json
> +++ b/src/ipa/raspberrypi/data/imx219.json
> @@ -133,7 +133,7 @@
>              {
>                  "shutter":
>                  [
> -                    100, 10000, 30000, 60000, 120000
> +                    100, 10000, 30000, 30000, 30000
>                  ],
>                  "gain":
>                  [
> @@ -144,7 +144,7 @@
>              {
>                  "shutter":
>                  [
> -                    100, 5000, 10000, 20000, 120000
> +                    100, 5000, 10000, 20000, 30000
>                  ],
>                  "gain":
>                  [
Naushir Patuck Nov. 26, 2020, 10:37 a.m. UTC | #3
Hi Laurent,

On Thu, 26 Nov 2020 at 10:20, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi David,
>
> Thank you for the patch.
>
> On Wed, Nov 25, 2020 at 11:36:40AM +0000, David Plowman wrote:
> > The exposure times in the exposure modes were causing AGC oscillations
> > because the algorithm was demanding long unachievable exposure times
> > but, without working sensor metadata, thought it was getting them when
> > actually it was not. We fix it by making the exposure profile request
> > only achievable exposure times, as we do for the ov5647 tuning.
>
> This looks good to me, and I assume it will still work with sensor
> embedded data, as unachievable exposure times are, well, unachievable
> :-)
>
> Is the process to select shutter values documented in the RPi camera
> documentation, or the tuning tool ? If someone wants to bring up a new
> sensor, how can we ensure a similar bug will not creep in ?
>

My work on FPS control does fix this problem in a generic way for
non-embedded data sensors.  This is done by ensuring we only send exposure
values that will be validated based on vblank limits, thereby ensuring the
return path (without embedded data) will be given the same values used by
the sensor device.   For embedded data sensors,  it all "just works", hence
our preference to use it where available ;-)

Regards,
Naush



>
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> >  src/ipa/raspberrypi/data/imx219.json | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/data/imx219.json
> b/src/ipa/raspberrypi/data/imx219.json
> > index b03a7beb..212f8b9a 100644
> > --- a/src/ipa/raspberrypi/data/imx219.json
> > +++ b/src/ipa/raspberrypi/data/imx219.json
> > @@ -133,7 +133,7 @@
> >              {
> >                  "shutter":
> >                  [
> > -                    100, 10000, 30000, 60000, 120000
> > +                    100, 10000, 30000, 30000, 30000
> >                  ],
> >                  "gain":
> >                  [
> > @@ -144,7 +144,7 @@
> >              {
> >                  "shutter":
> >                  [
> > -                    100, 5000, 10000, 20000, 120000
> > +                    100, 5000, 10000, 20000, 30000
> >                  ],
> >                  "gain":
> >                  [
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Nov. 26, 2020, 1:02 p.m. UTC | #4
Hi Naush,

On Thu, Nov 26, 2020 at 10:37:47AM +0000, Naushir Patuck wrote:
> On Thu, 26 Nov 2020 at 10:20, Laurent Pinchart wrote:
> > On Wed, Nov 25, 2020 at 11:36:40AM +0000, David Plowman wrote:
> > > The exposure times in the exposure modes were causing AGC oscillations
> > > because the algorithm was demanding long unachievable exposure times
> > > but, without working sensor metadata, thought it was getting them when
> > > actually it was not. We fix it by making the exposure profile request
> > > only achievable exposure times, as we do for the ov5647 tuning.
> >
> > This looks good to me, and I assume it will still work with sensor
> > embedded data, as unachievable exposure times are, well, unachievable
> > :-)
> >
> > Is the process to select shutter values documented in the RPi camera
> > documentation, or the tuning tool ? If someone wants to bring up a new
> > sensor, how can we ensure a similar bug will not creep in ?
> 
> My work on FPS control does fix this problem in a generic way for
> non-embedded data sensors. This is done by ensuring we only send exposure
> values that will be validated based on vblank limits, thereby ensuring the
> return path (without embedded data) will be given the same values used by
> the sensor device. For embedded data sensors, it all "just works", hence
> our preference to use it where available ;-)

Wouldn't algorithms still be able to do a better job when splitting
sensitivity control between exposure time and gain if they know
beforehand what exposure times are achievable, instead of waiting for
the embedded data to report the actual value later ? My concern here is
that I don't see any reason why it would be desirable to specify
unachievable shutter times in the tuning parameters, so we should try to
avoid it, especially if it can degrade performances. Making sure we
document this properly would be a good first step.

> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > ---
> > >  src/ipa/raspberrypi/data/imx219.json | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/data/imx219.json b/src/ipa/raspberrypi/data/imx219.json
> > > index b03a7beb..212f8b9a 100644
> > > --- a/src/ipa/raspberrypi/data/imx219.json
> > > +++ b/src/ipa/raspberrypi/data/imx219.json
> > > @@ -133,7 +133,7 @@
> > >              {
> > >                  "shutter":
> > >                  [
> > > -                    100, 10000, 30000, 60000, 120000
> > > +                    100, 10000, 30000, 30000, 30000
> > >                  ],
> > >                  "gain":
> > >                  [
> > > @@ -144,7 +144,7 @@
> > >              {
> > >                  "shutter":
> > >                  [
> > > -                    100, 5000, 10000, 20000, 120000
> > > +                    100, 5000, 10000, 20000, 30000
> > >                  ],
> > >                  "gain":
> > >                  [
Naushir Patuck Nov. 26, 2020, 2:07 p.m. UTC | #5
Hi Laurent,

On Thu, 26 Nov 2020 at 13:03, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> On Thu, Nov 26, 2020 at 10:37:47AM +0000, Naushir Patuck wrote:
> > On Thu, 26 Nov 2020 at 10:20, Laurent Pinchart wrote:
> > > On Wed, Nov 25, 2020 at 11:36:40AM +0000, David Plowman wrote:
> > > > The exposure times in the exposure modes were causing AGC
> oscillations
> > > > because the algorithm was demanding long unachievable exposure times
> > > > but, without working sensor metadata, thought it was getting them
> when
> > > > actually it was not. We fix it by making the exposure profile request
> > > > only achievable exposure times, as we do for the ov5647 tuning.
> > >
> > > This looks good to me, and I assume it will still work with sensor
> > > embedded data, as unachievable exposure times are, well, unachievable
> > > :-)
> > >
> > > Is the process to select shutter values documented in the RPi camera
> > > documentation, or the tuning tool ? If someone wants to bring up a new
> > > sensor, how can we ensure a similar bug will not creep in ?
> >
> > My work on FPS control does fix this problem in a generic way for
> > non-embedded data sensors. This is done by ensuring we only send exposure
> > values that will be validated based on vblank limits, thereby ensuring
> the
> > return path (without embedded data) will be given the same values used by
> > the sensor device. For embedded data sensors, it all "just works", hence
> > our preference to use it where available ;-)
>
> Wouldn't algorithms still be able to do a better job when splitting
> sensitivity control between exposure time and gain if they know
> beforehand what exposure times are achievable, instead of waiting for
> the embedded data to report the actual value later ?


Yes, this would certainly help.  In practise we make do with digital gain
to make up the difference in the Raspberry Pi AGC.  This is again tied into
the whole subject of framerate control.  At the end of the day, framerate
(and vblank time) is what limits the shutter times available to use.  Once
we have framerates available to play with, these can be passed into the AGC
algorithm to restrict exposure times.

However, this will not stop a user from setting up exposure profiles that
request larger shutter speeds than what is achievable given fps
constraints.  In these cases, the user will not get what he wants, unless
he sets the framerate appropriately.

Regards,
Naush



> My concern here is
> that I don't see any reason why it would be desirable to specify
> unachievable shutter times in the tuning parameters, so we should try to
> avoid it, especially if it can degrade performances. Making sure we
> document this properly would be a good first step.
>




>
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > > ---
> > > >  src/ipa/raspberrypi/data/imx219.json | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/data/imx219.json
> b/src/ipa/raspberrypi/data/imx219.json
> > > > index b03a7beb..212f8b9a 100644
> > > > --- a/src/ipa/raspberrypi/data/imx219.json
> > > > +++ b/src/ipa/raspberrypi/data/imx219.json
> > > > @@ -133,7 +133,7 @@
> > > >              {
> > > >                  "shutter":
> > > >                  [
> > > > -                    100, 10000, 30000, 60000, 120000
> > > > +                    100, 10000, 30000, 30000, 30000
> > > >                  ],
> > > >                  "gain":
> > > >                  [
> > > > @@ -144,7 +144,7 @@
> > > >              {
> > > >                  "shutter":
> > > >                  [
> > > > -                    100, 5000, 10000, 20000, 120000
> > > > +                    100, 5000, 10000, 20000, 30000
> > > >                  ],
> > > >                  "gain":
> > > >                  [
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series

diff --git a/src/ipa/raspberrypi/data/imx219.json b/src/ipa/raspberrypi/data/imx219.json
index b03a7beb..212f8b9a 100644
--- a/src/ipa/raspberrypi/data/imx219.json
+++ b/src/ipa/raspberrypi/data/imx219.json
@@ -133,7 +133,7 @@ 
             {
                 "shutter":
                 [
-                    100, 10000, 30000, 60000, 120000
+                    100, 10000, 30000, 30000, 30000
                 ],
                 "gain":
                 [
@@ -144,7 +144,7 @@ 
             {
                 "shutter":
                 [
-                    100, 5000, 10000, 20000, 120000
+                    100, 5000, 10000, 20000, 30000
                 ],
                 "gain":
                 [