[libcamera-devel,v2,4/4] src: ipa: raspberrypi: Fix initial AGC oscillation for imx219 sensor

10505 diff mbox series

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/raspberrypi/data/imx219.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi David,

On 26/11/2020 12:32, 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.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@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

Given the existing repetition, I assume an extra 30000 isn't an issue,
and it looks like this is just setting the shutter time on the modes array.

I also see shutter values of 120000 on the imx477, and in 'uncalibrated'.

I am going to wildly presume the imx477 is a better sensor, so it's more
appropriate there, or simply hasn't been considered yet (which isn't a
problem as this patch directly addresses the imx219).

I'm also curious about the high value in uncalibrated though. Should it
stay high, and as it's uncalibrated, it doesn't matter, as it's up to
the user to define and calibrate specific sensors?

I assume yes and those are only discussions so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                  ],
>                  "gain":
>                  [
> @@ -144,7 +144,7 @@
>              {
>                  "shutter":
>                  [
> -                    100, 5000, 10000, 20000, 120000
> +                    100, 5000, 10000, 20000, 30000
>                  ],
>                  "gain":
>                  [
>

Hi Kieran

Just to answer briefly... yes there is stuff that needs addressing
there, but that's coming in Naush's next patch set (after
Camera::start).

There are currently some problems when the AGC/AEC requests exposure
times (or analogue gains) that the sensor can't deliver. If a sensor
reports the metadata correctly (like imx477) things still work OK,
because the AGC/AEC isn't being lied to. So you can ask for long
exposures and if you get them - great, if not - it manages without.

Our other sensors (ov5647 and imx219) aren't reporting metadata
correctly. For these, we end up assuming the sensor gives us what we
asked for, and if it didn't, the AGC gets lied to. This tends to cause
oscillations.

For now, we can prevent the oscillations by not asking for anything
that's unachievable (hence this particular patch). But it's not the
ideal solution - Naush's next patch set addresses all this!

Best regards
David

On Mon, 30 Nov 2020 at 10:47, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi David,
>
> On 26/11/2020 12:32, 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.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@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
>
> Given the existing repetition, I assume an extra 30000 isn't an issue,
> and it looks like this is just setting the shutter time on the modes array.
>
> I also see shutter values of 120000 on the imx477, and in 'uncalibrated'.
>
> I am going to wildly presume the imx477 is a better sensor, so it's more
> appropriate there, or simply hasn't been considered yet (which isn't a
> problem as this patch directly addresses the imx219).
>
> I'm also curious about the high value in uncalibrated though. Should it
> stay high, and as it's uncalibrated, it doesn't matter, as it's up to
> the user to define and calibrate specific sensors?
>
> I assume yes and those are only discussions so:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >                  ],
> >                  "gain":
> >                  [
> > @@ -144,7 +144,7 @@
> >              {
> >                  "shutter":
> >                  [
> > -                    100, 5000, 10000, 20000, 120000
> > +                    100, 5000, 10000, 20000, 30000
> >                  ],
> >                  "gain":
> >                  [
> >
>
> --
> Regards
> --
> Kieran

Hi David,

On 30/11/2020 11:15, David Plowman wrote:
> Hi Kieran
> 
> Just to answer briefly... yes there is stuff that needs addressing
> there, but that's coming in Naush's next patch set (after
> Camera::start).

Ok, no problem, understood - and I don't think this series is blocked.

> There are currently some problems when the AGC/AEC requests exposure
> times (or analogue gains) that the sensor can't deliver. If a sensor
> reports the metadata correctly (like imx477) things still work OK,
> because the AGC/AEC isn't being lied to. So you can ask for long
> exposures and if you get them - great, if not - it manages without.
> 
> Our other sensors (ov5647 and imx219) aren't reporting metadata
> correctly. For these, we end up assuming the sensor gives us what we
> asked for, and if it didn't, the AGC gets lied to. This tends to cause
> oscillations.
> 
> For now, we can prevent the oscillations by not asking for anything
> that's unachievable (hence this particular patch). But it's not the
> ideal solution - Naush's next patch set addresses all this!

Thanks, I'll chase up where we are with start() again.
I hope everyone has had enough time to mull things over on that by now.

--
Kieran

> Best regards
> David
> 
> On Mon, 30 Nov 2020 at 10:47, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>>
>> Hi David,
>>
>> On 26/11/2020 12:32, 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.
>>>
>>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>>> Reviewed-by: Naushir Patuck <naush@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
>>
>> Given the existing repetition, I assume an extra 30000 isn't an issue,
>> and it looks like this is just setting the shutter time on the modes array.
>>
>> I also see shutter values of 120000 on the imx477, and in 'uncalibrated'.
>>
>> I am going to wildly presume the imx477 is a better sensor, so it's more
>> appropriate there, or simply hasn't been considered yet (which isn't a
>> problem as this patch directly addresses the imx219).
>>
>> I'm also curious about the high value in uncalibrated though. Should it
>> stay high, and as it's uncalibrated, it doesn't matter, as it's up to
>> the user to define and calibrate specific sensors?
>>
>> I assume yes and those are only discussions so:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>>                  ],
>>>                  "gain":
>>>                  [
>>> @@ -144,7 +144,7 @@
>>>              {
>>>                  "shutter":
>>>                  [
>>> -                    100, 5000, 10000, 20000, 120000
>>> +                    100, 5000, 10000, 20000, 30000
>>>                  ],
>>>                  "gain":
>>>                  [
>>>
>>
>> --
>> Regards
>> --
>> Kieran