[libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper for IMX258

[Umang Jain]

Hi Laurent, Dave

On 7/20/21 8:35 PM, Laurent Pinchart wrote:
> Hi Dave,
>
> On Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:
>> On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:
>>> On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:
>>>> Extend the CameraSensorHelper factory with support for an
>>>> IMX258 sensor as found in the Nautilus Chromebook.
>>>>
>>>> The values are read by hacking the IMX258 kernel driver.
>>>> The values for analog gain constants are obtained by reading the
>>>> register indexes, corresponding to the analog gain constants, as
>>>> mentioned in MIPI CCS v1.1 specification.
>>> I'd expand this to explain why.
>>>
>>> The imx258 kernel driver hints that the IMX258 sensor may be compatible
>>> with the MIPI CCS specification, as the register set matches. The gain
>>> parameter values have been retrieved from the sensor by reading the
>>> analog gain read-only registers as defined by CCS.
>>>
>>> This being said, given that the maximum gain value doesn't match the
>>> driver, I think we need to run additional tests to figure out if this is
>>> actually correct. Setting the gain to 0, 256, 384 and 448 should result
>>> in gain values of x1, x2, x4 and x8 according to the coefficients, and
>>> it should be possible to validate that by capturing images with a fixed
>>> exposure time and calculate the average luminance for the different gain
>>> values. The exposure time should be picked to have a luminance value of
>>> about 10% when the gain is x1, as lower valus would result in measuring
>>> noise rather than actual data, and higher values would saturate at
>>> higher gains.
>>>
>>> If the formula holds with those values, you could try setting the gain
>>> to values higher than 448, up to the maximum value of 0x1fff accepted by
>>> the driver, to see what happens.
>>>
>>> If 448 is indeed the maximum value, then we should fix the driver
>>> accordingly.
>> Do these comments mean you're working without a datasheet?
> Not completely, but the documentation we have doesn't specify the analog
> gain model.
>
>> We do have a datasheet for IMX258 (sorry, can't share it), and David
>> has IMX258 largely working on the Pi with some extra patches due to
>> only having 2 data lanes available or missing controls [1].
>>
>> The gain formula is stated in the datasheet as:
>> Analog gain = 512 / (512 – X)
>> (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).
>>
>> Range 0 to 480.
> That's interesting, as reading the analog_gain_code_max register returns
> 448. The register is documented as the "maximum recommended analog Gain
> code", so maybe the sensor supports up to x16, but doesn't recommend
> going over x8 ?
I also tested x32 and x64 and notice that it still keeps on picking up 
the gain. See below.
>
>> 0 = x1
>> 256 = x2
>> 384 = x4
>> 448 = x8
>> 480 = x16.
> Thanks for the confirmation.

I have a question, if we are considering x16 as the "extended" limit, as 
per our testing, I went further with:
496 = x32
504 = x64

coefficients, Does it make sense? I tested the x32 and x64 with exposure 
of 200 lines, the image is quite bright (over-exposed) but also can 
appears a better if I lower the exposure to 100 lines.

Laurent, with x64 specially, the purplish-tint that I have 
currently(with master), goes away. Not sure if it means anything. I also 
understand, that "bright" and "better" are just relative terms here.
https://pasteboard.co/Kc9o3jk.jpg

>> Tested empirically, an exposure of 200 lines with analogue gain 480
>> (x16) is giving the same intensity image as exposure of 400 lines with
>> analogue gain 448 (x8).
> That's exactly what I wanted to test :-) It also confirms the x16 limit.

I tested x1, x2, x4, x8 and even x16 coefficients and I get similar 
results. No issue till 480 = x16.

Picking value above 512 (for e.g. 544) will drop the gain and brings 
back to x0, x1 levels. So I assume, x16 is a decent limit, that should 
be reported by driver?

>
>> [1] Slightly hacky patches and need cleaning up, but look on our
>> shared kernel tree and branch 6by9/imx258.
> Next time I have to work on a sensor without full documentation, I'll
> check there first ;-)
>
>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>> ---
>>>>   src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>>>> index 709835a8..c43368df 100644
>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>>> @@ -295,6 +295,16 @@ public:
>>>>   };
>>>>   REGISTER_CAMERA_SENSOR_HELPER("imx219", CameraSensorHelperImx219)
>>>>
>>>> +class CameraSensorHelperImx258 : public CameraSensorHelper
>>>> +{
>>>> +public:
>>>> +        CameraSensorHelperImx258()
>>>> +        {
>>>> +                analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };
>>>> +        }
>>>> +};
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258)
>>>> +
>>>>   class CameraSensorHelperOv5670 : public CameraSensorHelper
>>>>   {
>>>>   public: