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

[Laurent Pinchart]

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 ?

> 0 = x1
> 256 = x2
> 384 = x4
> 448 = x8
> 480 = x16.

Thanks for the confirmation.

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

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

-- 
Regards,

Laurent Pinchart