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

[Kieran Bingham]

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 at 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 at raspberrypi.com>
>>> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 at ideasonboard.com>
>>
>>>                  ],
>>>                  "gain":
>>>                  [
>>> @@ -144,7 +144,7 @@
>>>              {
>>>                  "shutter":
>>>                  [
>>> -                    100, 5000, 10000, 20000, 120000
>>> +                    100, 5000, 10000, 20000, 30000
>>>                  ],
>>>                  "gain":
>>>                  [
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran