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

[Laurent Pinchart]

On Wed, Jul 03, 2024 at 03:11:04PM +0200, Stefan Klug wrote:
> 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?

I wouldn't mind comments. Up to you.

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