[PATCH v2 1/4] libcamera: libipa: camera_sensor: Provide CameraSensorHelper and CameraSensorProperties for the Sony IMX462 image sensor.

[Jacopo Mondi]

Hi Geoffrey

On Tue, Nov 19, 2024 at 10:50:53PM +0100, Geoffrey Van Landeghem wrote:
> Hi Jacopo,
>
> sorry for screwing up the git commit message. I took over the
> suggestion entirely from the V1 patch, which also got me a bit
> confused. The entire git format-patch and git email is still a bit
> awkward and needs some more getting used to it from my side. At work
> we rely on other systems for centralizing source control, but I learn
> something new and I also appreciate your feedback (and patience!).

No worries at all, it's more than fine to have a few hiccups when
first approaching mail-based patch review.

>
> Regarding the pixel size, it's indeed 2.9u pixel. I guess the most
> trustworthy resource is this one from Sony:
> https://www.sony-semicon.com/files/62/flyer_security/IMX462LQR_LQR1_Flyer.pdf
> (look for "unit cell size")

Thanks for confirming.

>
> And regarding the black level offset setting (register 300Ah). I
> checked the IMX290 and IMX327 datasheets and they match in that they
> both have F0h as default value.
> For IMX462 I couldn't find any datasheet, but since I own the sensor I
> queried it using i2c-tools, and it seems it's also defaulting to F0h:
>
> $ i2ctransfer -f -y 10 w2 at 0x1a 0x30 0x0A r1
> 0xf0

Nice, thanks for checking. Feel free to add a patch to add the black
levels to camera sensor helpers if you like to :)

Thanks
   j

>
> Kinds regards,
> Geoffrey
>
> Op di 19 nov 2024 om 13:56 schreef Jacopo Mondi <jacopo.mondi at ideasonboard.com>:
>
>
> >
> > Hi Geoffrey
> >
> > On Mon, Nov 18, 2024 at 11:53:07PM +0100, Geoffrey Van Landeghem wrote:
> > > The sensor is largely compatible with the already supported Sony IMX290 so we can reuse the same helpers for the analogue gain conversion functions.
> >
> > I presume my suggestion to read https://cbea.ms/git-commit/ first
> > wasn't helpful.
> >
> > Subject line should be shortened (I would not force it to 50 cols
> > as the website suggests, but strive for staying in at least 72), and
> > no full stop at the end.
> >
> > Same thing for the commit message.
> >
> > We can adjust it when applying the patch if you don't have to resend
> > for other reasons.
> >
> > >
> > > Signed-off-by: Geoffrey Van Landeghem <geoffrey.vl at gmail.com>
> > > ---
> > >  src/ipa/libipa/camera_sensor_helper.cpp           | 5 +++++
> > >  src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 1 +
> > >  src/libcamera/sensor/camera_sensor_properties.cpp | 4 ++++
> > >  3 files changed, 10 insertions(+)
> > >
> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > index c6169bdc..f870dc28 100644
> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > @@ -622,6 +622,11 @@ public:
> > >  };
> > >  REGISTER_CAMERA_SENSOR_HELPER("imx415", CameraSensorHelperImx415)
> > >
> > > +class CameraSensorHelperImx462 : public CameraSensorHelperImx290
> > > +{
> > > +};
> > > +REGISTER_CAMERA_SENSOR_HELPER("imx462", CameraSensorHelperImx462)
> >
> > One thing we're missing for imx290 is the black level pedestal. The
> > imx290 datasheet reports a (programmable) black level of
> > 10 bits: 0x3c
> > 12 bits: 0xf0
> >
> > Could you confirm the imx462 reports the same default values ?
> >
> > > +
> > >  class CameraSensorHelperImx477 : public CameraSensorHelper
> > >  {
> > >  public:
> > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp
> > > index e57ab538..0cc24a6d 100644
> > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp
> > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp
> > > @@ -73,3 +73,4 @@ static CamHelper *create()
> > >  }
> > >
> > >  static RegisterCamHelper reg("imx290", &create);
> > > +static RegisterCamHelper reg462("imx462", &create);
> > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > index 6d4136d0..e2305166 100644
> > > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > @@ -142,6 +142,10 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >                       .unitCellSize = { 1450, 1450 },
> > >                       .testPatternModes = {},
> > >               } },
> > > +             { "imx462", {
> > > +                     .unitCellSize = { 2900, 2900 },
> > > +                     .testPatternModes = {},
> > > +             } },
> >
> > I don't have a datasheet but a few online sources confirm a 2.9u pixel
> > size.
> >
> > The patch looks good
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > >               { "imx477", {
> > >                       .unitCellSize = { 1550, 1550 },
> > >                       .testPatternModes = {},
> > > --
> > > 2.43.0
> > >