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

[Laurent Pinchart]

Hi Dave,

On Tue, Jul 20, 2021 at 08:40:55PM +0100, Dave Stevenson wrote:
> On Tue, 20 Jul 2021 at 16:05, Laurent Pinchart wrote:
> > 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 ?
> 
> The datasheet I have doesn't mention the CCS spec, nor anything about
> register 0x0086 which appears to be the CCS register for analogue gain
> code max.

I'm not surprised. CCS is relatively recent, although the specification
it's based on, SMIA++, is ancient. There are lots of sensors that are
"nearly" CCS-compatible (or SMIA++-compatible), sometimes because the
sensor vendor took a previous sensor design that was compatible with the
spec and modified it to create a new product without caring about
compatibility with the specification, or sometimes because they thought
full compatibility was hindering their creativity but decided to follow
most of the spec instead of reinventing the wheel. I don't know in which
of those category (if any) the IMX258 fits. What I know is that it is
compatible with the specification to some extent given the register set.
The 448 limit in the analog_gain_code_max register may very well be a
left-over from the sensor that the IMX258 evolved from.

> No mention of issues with exceeding x8. All references are to up to
> 24dB of analogue gain available, and 24dB of digital

OK. Let's fix the driver to limit the analog gain to 480 instead of
0x1fff then.

> > > 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 ;-)
> 
> Chance I'm afraid in this case, but feel free to ask.

Or possibly the fact that if a sensor is of particular interest to
someone, there's a good chance there could be other people interested in
it too.

> We've had a number of third parties create modules and ask for support
> in creating the relevant kernel drivers and libcamera integration.
> IMX258 happened to be one of those that David was sent recently, and I
> did the register munging to handle only having 2 lanes, and running
> from a 24MHz ext clock (the mainline driver only handles 19.2MHz).
> 
> I haven't resolved an issue with running the full res at
> 1267Mbit/s/lane yet which is the main reason I haven't merged it to
> our vendor tree or looked at sending the patches to mainline.
> I also need to tweak my patch set over exactly which lines are read
> out. The current driver configures readout of lines 0 to 3118 (3119
> lines total) with an H & V FLIP. Do we correct that to being lines 1
> to 3118 and retain the Bayer order when flipped, or drop it to 0 to
> 3117 which will change the Bayer order on existing platforms? I've
> currently done the latter, but it probably ought to be the former. The
> array is 3120 pixels high, so both are valid, but G_SELECTION (not
> currently supported on mainline) also ought to reflect it.

I've looked at the patches in your branch, and it looks pretty good
overall. I'm tempted to take it a step further and computer register
values instead of hardcoding them in tables, at least for the non
manufacturer-specific registers (those are at 0x3000 and above). I'm not
asking you to do so, I may give it a try if time permits, or find a
volunteer to appoint. This work can be done using the CCS spec without
the IMX258 documentation, so there's no breach of NDA.

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