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

[Geoffrey Van Landeghem]

Hi Jacopo,

If I understand correctly, even though the black level register is
only 9 bits wide, you take the bit-depth of the sensor in the mode it
is configured to work (in my case 12), and then shift it to 16-bits as
explained in the libcamera sources. If so the black level for all 3
sensors is:

/* From datasheet: 0xF0 at 12bits. */
blackLevel_ = 3840;

Correct?

Kind regards,
Geoffrey


Op wo 20 nov 2024 om 10:00 schreef Jacopo Mondi <jacopo.mondi at ideasonboard.com>:
>
> 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
> > > >