[PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels into sensor helpers

[Kieran Bingham]

Quoting Laurent Pinchart (2024-07-03 13:48:19)
> On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2024-07-03 11:39:53)
> > > Move black levels for tuning files that contained a BLC block into
> > > the camera sensor helpers.
> > > 
> > > ov4689.yaml had 66 at 12bit while the datasheet states 64 at 12bit. Use the
> > > value from the datasheet (scaled to 16bit).
> > 
> > I wonder who chose 66 here. Was it measured by someone perhaps?
> > 
> > Git blame states "Mikhail Rudenko" (Added to Cc) Mikhail, any opinion on
> > this change?
> > 
> > Anyway, I don't object to this right now - I think once we release our
> > updates to the IPA and tuning tools, most sensors 'supported' in
> > libcamera will likely benefit from being '(re)tuned' correctly and
> > 'fully' (and consistently?).
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > ov5640.yaml had 256 at 12bit while the datasheet states 16 at 10bit. Looking
> > > at the commit message the 256 most likely stems from the imx219 tuning
> > > file and 16 at 10bit is the same as the 64 at 12bit from the ov4689. This
> > > seems more likely and is therefore used.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/libipa/camera_sensor_helper.cpp | 2 ++
> > >  src/ipa/rkisp1/data/ov4689.yaml         | 4 ----
> > >  src/ipa/rkisp1/data/ov5640.yaml         | 4 ----
> > >  3 files changed, 2 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > index 3d0e756927f0..982e35beaa90 100644
> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper
> > >  public:
> > >         CameraSensorHelperOv4689()
> > >         {
> > > +               blackLevel_ = 1024;
> 
> I wonder if
> 
>                 blackLevel_ = 16 << 6;

I like this idea ... except the '<< 6' isn't particularly clear at
conveying the 16 is in 10 bit.


I guess we could add a helper
	blackLevel_ = to16Bit(10 /*bit*/, 16)


#define to16Bit(depth, val) ((val) << (16 - (depth)))

Or some other magic ... but I definitely recall my confusion when I
first looked at the black levels in a datasheet and saw different values
at different bit depths, and had to convert again for RPi. The operation
is simple of course - but it's the reason/intent that may not be clear
to a new comer - so doing something to make that easier to bring up a
new sensor is a good thing IMO.

Could be something that gets tackled later though.

--
Kieran


> 
> would be more readable. I suppose what you want to see at a glance, the
> 16-bit value, or the value for the native sensor bit depth. Up to you.
> If you decide to change it, please update all helpers.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > >                 gainType_ = AnalogueGainLinear;
> > >                 gainConstants_.linear = { 1, 0, 0, 128 };
> > >         }
> > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper
> > >  public:
> > >         CameraSensorHelperOv5640()
> > >         {
> > > +               blackLevel_ = 1024;
> > >                 gainType_ = AnalogueGainLinear;
> > >                 gainConstants_.linear = { 1, 0, 0, 16 };
> > >         }
> > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml
> > > index 2068684cafcd..609012967e02 100644
> > > --- a/src/ipa/rkisp1/data/ov4689.yaml
> > > +++ b/src/ipa/rkisp1/data/ov4689.yaml
> > > @@ -6,8 +6,4 @@ algorithms:
> > >    - Agc:
> > >    - Awb:
> > >    - BlackLevelCorrection:
> > > -      R:  66
> > > -      Gr: 66
> > > -      Gb: 66
> > > -      B:  66
> > >  ...
> > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml
> > > index 897b83cb435b..4b21d412e44e 100644
> > > --- a/src/ipa/rkisp1/data/ov5640.yaml
> > > +++ b/src/ipa/rkisp1/data/ov5640.yaml
> > > @@ -6,10 +6,6 @@ algorithms:
> > >    - Agc:
> > >    - Awb:
> > >    - BlackLevelCorrection:
> > > -      R:  256
> > > -      Gr: 256
> > > -      Gb: 256
> > > -      B:  256
> > >    - ColorProcessing:
> > >    - GammaSensorLinearization:
> > >        x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
> 
> -- 
> Regards,
> 
> Laurent Pinchart