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

[Jacopo Mondi]

Hi Laurent

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;
> > > +};
> > > +
> > > +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_));>


> 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