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

[Stefan Klug]

On Wed, Jul 03, 2024 at 03:48:19PM +0300, Laurent Pinchart wrote:
> 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>

That is a difficult one. I'd like to stick to the current version.
Without it, I think I would have missed that all Omnivision black levels
are the same, even though the documentation mentions 64 at 12 and 16 at 10.
Would it help to include comments above with the origin of the
information?

Regards,
Stefan

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