[libcamera-devel] [PATCH v1 1/2] pipeline: ipa: rpi: Flag if the sensor is a mono variant

[Naushir Patuck]

Hi Laurent,

On Fri, 2 Jun 2023 at 08:46, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, May 31, 2023 at 03:39:45PM +0100, Naushir Patuck via libcamera-devel wrote:
> > Add a flag to the ipa->init() interface to indicate a mono sensor
> > variant. This flag will be used in a future commit to handle controls
> > that are invalid for mono sensors.
>
> The patch itself seems to do the job, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I'm however wondering if we could do a bit better. The need to identify
> the sensor type isn't specific to Raspberry Pi, so a solution that
> covers the other IPA modules would be nice. The IPU3 and RkISP1 IPA
> interfaces both pass an IPACameraSensorInfo structure to the init()
> function. Would it make sense to extend that with a mono flag, and use
> it for the Raspberry Pi IPA module too ?

Sounds good, this would allow any IPA to get this flag.

IPACameraSensorInfo is a good choice, rpi/rkisp/ipu3 all take this in during the
configure() call. Rpi does not have IPACameraSensorInfo passed in on init, and
the monoSensor_ flag is passed in init().  But really we don't need to know the
state of the flag until configure() so it's not a problem.

>
> Another improvement would be to pass a CFA type instead of a mono flag,
> that would pave the way to support other CFA patterns (I'm thinking
> abour RGB+IR for instance). It may be too early to do so though.
>

I think I actually prefer this approach over a flag! We have the CFA in
CameraSensor::properties (properties::draft::ColorFilterArrangement) which we
can put in IPACameraSensorInfo.  The CFA pattern might change when configuring
(e.g. if a flip/mirror occurs) so this feels quite neat.

What do you think?  Let me know and I can rework this series.

> Finally, we already pass the sensor model in the IPASettings structure,
> which could be used on the IPA side to check what CFA pattern the sensor
> uses by adding that information to the camera sensor helpers. Any
> opinion about this ?

This might not work for the IMX296 as we have both mono and colour variants.
This is presuming that we only stick with one camera helper for IMX296 and the
model string does not distinguish between the two?

Regards,
Naush

>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom             | 1 +
> >  src/ipa/rpi/common/ipa_base.cpp                     | 1 +
> >  src/ipa/rpi/common/ipa_base.h                       | 1 +
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++-
> >  4 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > index ba786e647ca1..d35289ee6229 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -21,6 +21,7 @@ struct SensorConfig {
> >
> >  struct InitParams {
> >       bool lensPresent;
> > +     bool monoSensor;
> >  };
> >
> >  struct InitResult {
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index db7a0eb3a1ca..150fe433d0df 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
> >       }
> >
> >       lensPresent_ = params.lensPresent;
> > +     monoSensor_ = params.monoSensor;
> >
> >       controller_.initialise();
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> > index 6f9c46bb16b1..39d00760d012 100644
> > --- a/src/ipa/rpi/common/ipa_base.h
> > +++ b/src/ipa/rpi/common/ipa_base.h
> > @@ -87,6 +87,7 @@ private:
> >       std::map<unsigned int, MappedFrameBuffer> buffers_;
> >
> >       bool lensPresent_;
> > +     bool monoSensor_;
> >       ControlList libcameraMetadata_;
> >
> >       std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 3bb5ec531e4f..12698056cda1 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration()
> >  int CameraData::loadIPA(ipa::RPi::InitResult *result)
> >  {
> >       ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
> > +     bool monoSensor = isMonoSensor(sensor_);
> >
> >       if (!ipa_)
> >               return -ENOENT;
> > @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
> >       char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
> >       if (!configFromEnv || *configFromEnv == '\0') {
> >               std::string model = sensor_->model();
> > -             if (isMonoSensor(sensor_))
> > +             if (monoSensor)
> >                       model += "_mono";
> >               configurationFile = ipa_->configurationFile(model + ".json");
> >       } else {
> > @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
> >       ipa::RPi::InitParams params;
> >
> >       params.lensPresent = !!sensor_->focusLens();
> > +     params.monoSensor = monoSensor;
> >       int ret = platformInitIpa(params);
> >       if (ret)
> >               return ret;
>
> --
> Regards,
>
> Laurent Pinchart