[libcamera-devel] [PATCH] libcamera: ipa: Add IMX335 support
Naushir Patuck
naush at raspberrypi.com
Thu Nov 9 12:13:46 CET 2023
On Thu, 9 Nov 2023 at 10:30, Laurent Pinchart via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> On Tue, Nov 07, 2023 at 10:50:24AM +0200, Laurent Pinchart via libcamera-devel wrote:
> > On Mon, Nov 06, 2023 at 10:17:58PM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Naushir Patuck (2023-11-03 09:30:29)
> > > > On Thu, 2 Nov 2023 at 18:09, Kieran Bingham via libcamera-devel wrote:
> > > > >
> > > > > Provide support for the Sony IMX335 in both libipa and RaspberryPi IPA
> > > > > modules.
> > > >
> > > > Without an imx335.json camera tuning file present, this will not work
> > > > on the RPi platform. Was that file meant to be added in this patch?
> > >
> > > So far I've used only an unmodified 'uncalibrated.json' for both RPi4
> > > and Pi5.
> > >
> > > I've now successfully tested the module and updated kernel driver with
> > > both 2 and 4 lane operation (2 lane on RPi4, 4 lane on Pi5).
> > >
> > > I haven't yet gone through a full 'tuning' process on Raspberry Pi
> > > though as it's not my target platform for this camera module. But it's
> > > /very/ helpful that I can test on Raspberry Pi.
> > >
> > > It gives good results even using uncalibrated.json on both platforms
> > > too! (Which I believe is a testament to Raspberry Pi's IPA development!)
> > >
> > >
> > > I would always like to think that 'every camera' which can be connected
> > > should be supported on all platforms. But I don't know if that means to
> > > get a camera into libcamera it should be tuned for each platform
> > > explicitly?
> >
> > I don't think that would be a reasonable expectation, it just wouldn't
> > scale.
> >
> > There's a difference between teaching libcamera about intrinsic
> > properties of a camera sensor, which by definition are not dependent on
> > the platform it is connected to, and providing tuning data that covers
> > the combination of a camera module and an ISP. The former should simply
> > be a matter of updating a shared implementation, without a need to test
> > on all platforms, while the latter requires per-platform work.
> >
> > One issue we have today is that there are, for historical reasons, two
> > sets of helpers for camera sensors, one in libipa, and one in the
> > Raspberry Pi IPA. It seems to be time to add missing information to the
> > former and drop the latter. I'm tempted to give a strong incentive in
> > that direction by refusing new additions to the Raspberry Pi-specific
> > camera helpers.
>
> Re-reading this, I realize the wording is more aggressive than I
> intended it to be. To clarify my point, I don't call for refusing new
> additions right now, blocking this patch. I do believe that it's time to
> merge the two sets of helpers in one, as if we have an agreement on
> that, and can agree on a concrete plan, then I'll be happy.
Thank you for clarifying this!
Agree, it would be nice to consolidate these 2 helpers - hopefully we
can find a reasonably painless path forward to doing this. They key
trouble I see is trying to keep our existing structures in-place since
we use them through all our IPA and algorithms source code.
Regards,
Naush
>
> > > I'm weary of providing a Raspberry Pi 'tuning' file that implies I have
> > > performed any full tuning on the module. What are your thoughts about
> > > defaulting to use the 'uncalibrated.json' when no tuning file is
> > > identified (and printing a warning to report this?)
> > >
> > > Otherwise I would be submitting essentially
> > > 'cp uncalibrated.json imx335.json'
> > >
> > > And that would perhaps give potential future users undue belief in the
> > > tuning file.
> >
> > Defaulting to uncalibrated.json is what we do on some other platforms,
> > and I think it's better than pretending we have tuned a particular
> > camera module for a Pi 4 or Pi 5 board.
> >
> > > I'm going to go through the tuning process on a Pi now -
> > > but even with that - this would only be a very 'home lab' basic effort.
> >
> > That's of course useful too :-) It just shouldn't be a hard requirement.
> >
> > > > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > > ---
> > > > > src/ipa/libipa/camera_sensor_helper.cpp | 23 ++++++
> > > > > src/ipa/rpi/cam_helper/cam_helper_imx335.cpp | 74 ++++++++++++++++++++
> > > > > src/ipa/rpi/cam_helper/meson.build | 1 +
> > > > > src/libcamera/camera_sensor_properties.cpp | 4 ++
> > > > > 4 files changed, 102 insertions(+)
> > > > > create mode 100644 src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > > > >
> > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > > index f0ecc3830115..ddab5af6eac2 100644
> > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > > @@ -444,6 +444,29 @@ class CameraSensorHelperImx327 : public CameraSensorHelperImx290
> > > > > };
> > > > > REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx327)
> > > > >
> > > > > +class CameraSensorHelperImx335 : public CameraSensorHelper
> > > > > +{
> > > > > +public:
> > > > > + uint32_t gainCode(double gain) const override;
> > > > > + double gain(uint32_t gainCode) const override;
> > > > > +private:
> > > > > + static constexpr uint32_t maxGainCode_ = 240;
> > > > > +};
> > > > > +
> > > > > +uint32_t CameraSensorHelperImx335::gainCode(double gain) const
> > > > > +{
> > > > > + uint32_t code = 10 * std::log10(gain) * 10 / 3;
> > > > > +
> > > > > + return std::min(code, maxGainCode_);
> > > > > +}
> > > > > +
> > > > > +double CameraSensorHelperImx335::gain(uint32_t gainCode) const
> > > > > +{
> > > > > + return std::pow(10.0, gainCode / (10 * 10 / 3));
> > > > > +}
> > > > > +
> > > > > +REGISTER_CAMERA_SENSOR_HELPER("imx335", CameraSensorHelperImx335)
> > > > > +
> > > > > class CameraSensorHelperImx477 : public CameraSensorHelper
> > > > > {
> > > > > public:
> > > > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > > > > new file mode 100644
> > > > > index 000000000000..659c69d6b6c7
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > > > > @@ -0,0 +1,74 @@
> > > > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > > > +/*
> > > > > + * Copyright (C) 2023, Ideas on Board Oy.
> > > > > + *
> > > > > + * cam_helper_imx335.cpp - camera information for the Sony IMX335 sensor
> > > > > + */
> > > > > +
> > > > > +#include <assert.h>
> > > > > +
> > > > > +#include "cam_helper.h"
> > > > > +#include "math.h"
> > > > > +
> > > > > +using namespace RPiController;
> > > > > +
> > > > > +class CamHelperImx335 : public CamHelper
> > > > > +{
> > > > > +public:
> > > > > + CamHelperImx335();
> > > > > + uint32_t gainCode(double gain) const override;
> > > > > + double gain(uint32_t gainCode) const override;
> > > > > + void getDelays(int &exposureDelay, int &gainDelay,
> > > > > + int &vblankDelay, int &hblankDelay) const override;
> > > > > + unsigned int hideFramesModeSwitch() const override;
> > > > > +
> > > > > +private:
> > > > > + /*
> > > > > + * Smallest difference between the frame length and integration time,
> > > > > + * in units of lines.
> > > > > + */
> > > > > + static constexpr int frameIntegrationDiff = 4;
> > > > > + static constexpr uint32_t maxGainCode = 240;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * IMX335 Metadata isn't yet supported.
> > > > > + */
> > > > > +
> > > > > +CamHelperImx335::CamHelperImx335()
> > > > > + : CamHelper({}, frameIntegrationDiff)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +uint32_t CamHelperImx335::gainCode(double gain) const
> > > > > +{
> > > > > + uint32_t code = 10 * std::log10(gain) * 10 / 3;
> > > > > + return std::min(code, maxGainCode);
> > > > > +}
> > > > > +
> > > > > +double CamHelperImx335::gain(uint32_t gainCode) const
> > > > > +{
> > > > > + return std::pow(10.0, gainCode / (10 * 10 / 3));
> > > > > +}
> > > > > +
> > > > > +void CamHelperImx335::getDelays(int &exposureDelay, int &gainDelay,
> > > > > + int &vblankDelay, int &hblankDelay) const
> > > > > +{
> > > > > + exposureDelay = 2;
> > > > > + gainDelay = 2;
> > > > > + vblankDelay = 2;
> > > > > + hblankDelay = 2;
> > > > > +}
> > > > > +
> > > > > +unsigned int CamHelperImx335::hideFramesModeSwitch() const
> > > > > +{
> > > > > + /* One bad frame can be expected after a mode switch. */
> > > > > + return 1;
> > > > > +}
> > > > > +
> > > > > +static CamHelper *create()
> > > > > +{
> > > > > + return new CamHelperImx335();
> > > > > +}
> > > > > +
> > > > > +static RegisterCamHelper reg("imx335", &create);
> > > > > diff --git a/src/ipa/rpi/cam_helper/meson.build b/src/ipa/rpi/cam_helper/meson.build
> > > > > index bdf2db8eb742..17c25cb0e4a6 100644
> > > > > --- a/src/ipa/rpi/cam_helper/meson.build
> > > > > +++ b/src/ipa/rpi/cam_helper/meson.build
> > > > > @@ -6,6 +6,7 @@ rpi_ipa_cam_helper_sources = files([
> > > > > 'cam_helper_imx219.cpp',
> > > > > 'cam_helper_imx290.cpp',
> > > > > 'cam_helper_imx296.cpp',
> > > > > + 'cam_helper_imx335.cpp',
> > > > > 'cam_helper_imx477.cpp',
> > > > > 'cam_helper_imx519.cpp',
> > > > > 'cam_helper_imx708.cpp',
> > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > > > > index 27d6799a2686..dc76051fa349 100644
> > > > > --- a/src/libcamera/camera_sensor_properties.cpp
> > > > > +++ b/src/libcamera/camera_sensor_properties.cpp
> > > > > @@ -111,6 +111,10 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > > > .unitCellSize = { 2900, 2900 },
> > > > > .testPatternModes = {},
> > > > > } },
> > > > > + { "imx335", {
> > > > > + .unitCellSize = { 2000, 2000 },
> > > > > + .testPatternModes = {},
> > > > > + } },
> > > > > { "imx477", {
> > > > > .unitCellSize = { 1550, 1550 },
> > > > > .testPatternModes = {},
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list