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

[Laurent Pinchart]

Hi Naush,

On Fri, Jun 02, 2023 at 09:55:03AM +0100, Naushir Patuck wrote:
> On Fri, 2 Jun 2023 at 08:46, Laurent Pinchart wrote:
> > 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.

I see you've sent a v2 already, let's continue the discussion there.

> > 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?

Good point. I think we can ignore this question for now, as adding the
CFA pattern in the IPACameraSensorInfo works nicely.

> > > 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