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

[Laurent Pinchart]

On Wed, Nov 20, 2024 at 11:06:58PM +0100, Geoffrey Van Landeghem wrote:
> 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?

That's right, we express all black level values normalized to a 16 bits
depth.

> Op wo 20 nov 2024 om 10:00 schreef Jacopo Mondi:
> > 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 :)
> >
> > > Op di 19 nov 2024 om 13:56 schreef Jacopo Mondi <jacopo.mondi at ideasonboard.com>:
> > > > 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>
> > > >
> > > > >               { "imx477", {
> > > > >                       .unitCellSize = { 1550, 1550 },
> > > > >                       .testPatternModes = {},

-- 
Regards,

Laurent Pinchart