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

[Dave Stevenson]

Hi Umang

On Wed, 21 Jul 2021 at 06:28, Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> 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.

We're straying into undocumented territory!

My quick tests of having the preview running and swapping between
v4l2-ctl --set-ctrl=analogue_gain=480 --set-ctrl=exposure=80
v4l2-ctl --set-ctrl=analogue_gain=496 --set-ctrl=exposure=40
v4l2-ctl --set-ctrl=analogue_gain=504 --set-ctrl=exposure=20
I do see a slight difference in brightness between the steps. 496 is
slightly brighter than 480, and 504 is brighter again. The noise is
getting significant too, but that's not unexpected.

> 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

For me the images go weird on highlights when the analogue gain exceeds 499.
See images at https://photos.app.goo.gl/6BNHZnZVkWYgo6sp6 (phone
camera images of preview running. Analogue gain settings in the image
descriptions).

I've checked the raws, and those highlights are going to black there,
so it's not our ISP going crazy.

> >> 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?

The datasheet does list it as a 9 bit value (1-bit in 0x0204, 8-bits
in 0x0205), so values above 512 will be losing their top bits.

I'll certainly agree that the driver shouldn't be advertising a
maximum of 0x1fff.
480 (x16) would be the sensible max value based on the datasheet,
although there's possibly a case to extend it to 496 (x32) with some
more testing.

  Dave

> >
> >> [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: