[libcamera-devel] [PATCH 1/2] ipa: camera_sensor_helper: Add AR0521 support

[Laurent Pinchart]

Hi Jacopo,

On Thu, Nov 24, 2022 at 06:00:17PM +0100, Jacopo Mondi wrote:
> On Wed, Nov 23, 2022 at 08:17:07PM +0200, Laurent Pinchart wrote:
> > On Tue, Nov 22, 2022 at 10:59:23AM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Jacopo Mondi via libcamera-devel (2022-11-07 13:22:12)
> > > > Add support for OnSemi AR0521 5Mpx image sensor to libipa.
> > > >
> > > > The sensor implments a gain model realized via a piecewise exponential
> >
> > s/implments/implements/
> >
> > > > gain function defined by a table available in the sensor's application
> > > > developer guide.
> >
> > As we're not tabulating it here, I would write
> >
> > The sensor analogue gain is implemented as a coarse and a fine factor,
> > with the coarse gain being a power of two and the fine gain being a
> > value in the [1.0, 2.0[ range. The mapping between gain codes and gain
> > values is tabulated in the datasheet, and the table values are very
> > close but not identical to the mathematical model. Compute the gain
> > using the model to keep the code shorter, if this causes precision
> > issues the calculation could be replaced with a table.
> >
> > > > Add support for the sensor using a dedicated CameraSensorHelper
> > > > instance.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/ipa/libipa/camera_sensor_helper.cpp | 45 +++++++++++++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > index 35056bec80c3..cb5cdf1d1e54 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > @@ -366,6 +366,51 @@ static constexpr double expGainDb(double step)
> > > >         return log2_10 * step / 20;
> > > >  }
> > > >
> > > > +class CameraSensorHelperAr0521 : public CameraSensorHelper
> > > > +{
> > > > +public:
> > > > +       uint32_t gainCode(double gain) const override;
> > > > +       double gain(uint32_t gainCode) const override;
> > > > +
> > > > +private:
> > > > +       const unsigned int kStep_ = 16;

Btw, static constexpr.

> > > > +};
> > > > +
> > > > +uint32_t CameraSensorHelperAr0521::gainCode(double gain) const
> > > > +{
> > > > +       unsigned int coarse = 0;
> > > > +
> > > > +       if (gain < 2.02) {
> > > > +               coarse = 0;
> > > > +       } else if (gain < 4.21) {
> >
> > For a gain of 4.2, you will get coarse = 1 and fine = 17, which will
> > overflow the fine gain value stored on 4 bits.
> >
> > > > +               coarse = 1;
> > > > +       } else if (gain < 7.95) {
> > > > +               coarse = 2;
> > > > +       } else if (gain < 15.28) {
> >
> > Nominally, the highest gain code (0x3f) corresponds to
> > (1 << 3) * (1 + 15/16) = 15.5. That's not what is tabulated in the
> > datasheet, but as long as we compute the gain (instead of tabulating it
> > in here as well), I'd stick to the nominal value.
> >
> > > > +               coarse = 3;
> > > > +       } else {
> > > > +               LOG(CameraSensorHelper, Error) << "Gain too high: " << gain;
> >
> > Is the error message needed ?
> >
> > > > +               gain = 15.28;
> > > > +               coarse = 3;
> > > > +       }
> > > > +
> > > > +       unsigned int exp = 1 << coarse;
> > > > +       unsigned int fine = kStep_ * (gain - exp) / exp;
> >
> > 	unsigned int fine = (gain / (1 << coarse) - 1) * kStep_;
> >
> > for the reason explained below (I've started the review from the bottom
> > ;-))
> >
> > You can write all this as
> >
> > 	gain = std::clamp(gain, 1.0, 15.5);
> > 	unsigned int coarse = std::log2(gain);
> > 	unsigned int fine = (gain / (1 << coarse) - 1) * kStep_;
> 
> Thanks, that's better indeed!
> 
> > > > +
> > > > +       return (coarse << 4) | (fine & 0xf);
> > >
> > > Wow - more complex than our other CameraSensorHelpers, but that's
> > > exactly why we have these CameraSensorHelper instances!
> > >
> > > While I haven't seen the documentation corresponding to this, it seems
> > > like it's doing reasonable things so:
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > +}
> > > > +
> > > > +double CameraSensorHelperAr0521::gain(uint32_t gainCode) const
> > > > +{
> > > > +       unsigned int coarse = gainCode >> 4;
> > > > +       unsigned int fine = gainCode & 0xf;
> > > > +       unsigned int exp = 1 << coarse;
> > > > +
> > > > +       return exp + (exp * fine / kStep_);
> >
> > 	return (1 << coarse) * (1 + fine /kStep_);
> 
> That's not better I'm afraid, as the integer division fine / kStep_
> gives 0
> 
> My version mitigated the problem (in facts, I started from your
> proposal and then realized something was not working) because
> (exp * fine / kStep_) doesn't get rounded to 0.
> 
> I intially tested this by writing Python code to match the results
> against the datasheet table, and I was satisfied. Now that I've
> reimplemented the test in C++ I can notice that even my version is not
> that precise as it anyway rounds to integers.
> 
> I guess the ideal solution would be
> 
>         return (1 << coarse) * (1 + fine / static_cast<double>(kStep_));>

Yes sorry I missed the cast. You could also just define kStep_ as a
double. Or probably better define the fine variable as a double. Up to
you.

> > The result is the same, but it reflects better than the gain is the
> > product of the coarse and fine gains.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > > +}
> > > > +
> > > > +REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
> > > > +
> > > >  class CameraSensorHelperImx219 : public CameraSensorHelper
> > > >  {
> > > >  public:

-- 
Regards,

Laurent Pinchart