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

[Laurent Pinchart]

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;

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