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

[Laurent Pinchart]

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

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

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