[libcamera-devel,v2,3/3] DEMO: raspberrypi: Use custom parameters to init()
diff mbox series

Message ID 20210308074828.13228-4-paul.elder@ideasonboard.com
State Not Applicable
Headers show
Series
  • Expand mojom-based code generator
Related show

Commit Message

Paul Elder March 8, 2021, 7:48 a.m. UTC 
This is just a demo to show custom parameters to init() with the
raspberrypi IPA interface.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/ipa/raspberrypi.mojom       |  5 ++-
 src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++---------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++--
 3 files changed, 32 insertions(+), 27 deletions(-)

Comments

Laurent Pinchart March 8, 2021, 8:47 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, Mar 08, 2021 at 04:48:28PM +0900, Paul Elder wrote:
> This is just a demo to show custom parameters to init() with the
> raspberrypi IPA interface.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  5 ++-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++---------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++--
>  3 files changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index f733a2cd..99a62c02 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -51,7 +51,8 @@ struct StartControls {
>  };
>  
>  interface IPARPiInterface {
> -	init(IPASettings settings) => (int32 ret);
> +	init(IPASettings settings, string sensorName)
> +		=> (int32 ret, bool metadataSupport);
>  	start(StartControls controls) => (StartControls result);
>  	stop();
>  
> @@ -77,7 +78,7 @@ interface IPARPiInterface {
>  		  map<uint32, IPAStream> streamConfig,
>  		  map<uint32, ControlInfoMap> entityControls,
>  		  ConfigInput ipaConfig)
> -		=> (ConfigOutput results, int32 ret);
> +		=> (int32 ret, ConfigOutput results);

I wonder if it would make sense to split those two changes. The change
to configure() could be reviewed and merged already, and Naush could
take this DEMO patch as an example to move data->sensorMetadata_
handling to match() and IPA init() time.

For the configure() part of this patch,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

You don't have to include an init() demo in v3 of the series (or just
v2.1 of this patch actually), this is enough.

>  
>  	/**
>  	 * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 6348d071..ac18dcbd 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -79,16 +79,17 @@ public:
>  			munmap(lsTable_, ipa::RPi::MaxLsGridSize);
>  	}
>  
> -	int init(const IPASettings &settings) override;
> +	int init(const IPASettings &settings, const std::string &sensorName,
> +		 bool *metadataSupport) override;
>  	void start(const ipa::RPi::StartControls &data,
>  		   ipa::RPi::StartControls *result) override;
>  	void stop() override {}
>  
> -	void configure(const CameraSensorInfo &sensorInfo,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, ControlInfoMap> &entityControls,
> -		       const ipa::RPi::ConfigInput &data,
> -		       ipa::RPi::ConfigOutput *response, int32_t *ret) override;
> +	int configure(const CameraSensorInfo &sensorInfo,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, ControlInfoMap> &entityControls,
> +		      const ipa::RPi::ConfigInput &data,
> +		      ipa::RPi::ConfigOutput *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void signalStatReady(const uint32_t bufferId) override;
> @@ -164,9 +165,14 @@ private:
>  	double maxFrameDuration_;
>  };
>  
> -int IPARPi::init(const IPASettings &settings)
> +int IPARPi::init(const IPASettings &settings, const std::string &sensorName,
> +		 bool *metadataSupport)
>  {
> +	LOG(IPARPI, Debug) << "sensor name is " << sensorName;
> +
>  	tuningFile_ = settings.configurationFile;
> +
> +	*metadataSupport = true;
>  	return 0;
>  }
>  
> @@ -290,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  	mode_.max_frame_length = sensorInfo.maxFrameLength;
>  }
>  
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> -		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, ControlInfoMap> &entityControls,
> -		       const ipa::RPi::ConfigInput &ipaConfig,
> -		       ipa::RPi::ConfigOutput *result, int32_t *ret)
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> +		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, ControlInfoMap> &entityControls,
> +		      const ipa::RPi::ConfigInput &ipaConfig,
> +		      ipa::RPi::ConfigOutput *result)
>  {
>  	if (entityControls.size() != 2) {
>  		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> -		*ret = -1;
> -		return;
> +		return -1;
>  	}
>  
>  	result->params = 0;
> @@ -309,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  
>  	if (!validateSensorControls()) {
>  		LOG(IPARPI, Error) << "Sensor control validation failed.";
> -		*ret = -1;
> -		return;
> +		return -1;
>  	}
>  
>  	if (!validateIspControls()) {
>  		LOG(IPARPI, Error) << "ISP control validation failed.";
> -		*ret = -1;
> -		return;
> +		return -1;
>  	}
>  
>  	/* Setup a metadata ControlList to output metadata. */
> @@ -334,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		if (!helper_) {
>  			LOG(IPARPI, Error) << "Could not create camera helper for "
>  					   << cameraName;
> -			*ret = -1;
> -			return;
> +			return -1;
>  		}
>  
>  		/*
> @@ -403,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  
>  	lastMode_ = mode_;
>  
> -	*ret = 0;
> +	return 0;
>  }
>  
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index db91f1b5..3ff0f1cd 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1194,7 +1194,10 @@ int RPiCameraData::loadIPA()
>  
>  	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
>  
> -	return ipa_->init(settings);
> +	bool metadataSupport;
> +	int ret = ipa_->init(settings, "sensor name", &metadataSupport);
> +	LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes" : "no");
> +	return ret;
>  }
>  
>  int RPiCameraData::configureIPA(const CameraConfiguration *config)
> @@ -1250,9 +1253,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	/* Ready the IPA - it must know about the sensor resolution. */
>  	ipa::RPi::ConfigOutput result;
>  
> -	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> -			&result, &ret);
> -
> +	ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> +			      &result);
>  	if (ret < 0) {
>  		LOG(RPI, Error) << "IPA configuration failed!";
>  		return -EPIPE;
Naushir Patuck March 8, 2021, 3:31 p.m. UTC | #2 
Hi Paul and Laurent,

On Mon, 8 Mar 2021 at 08:47, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Paul,
>
> Thank you for the patch.
>
> On Mon, Mar 08, 2021 at 04:48:28PM +0900, Paul Elder wrote:
> > This is just a demo to show custom parameters to init() with the
> > raspberrypi IPA interface.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom       |  5 ++-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++---------
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++--
> >  3 files changed, 32 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> > index f733a2cd..99a62c02 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -51,7 +51,8 @@ struct StartControls {
> >  };
> >
> >  interface IPARPiInterface {
> > -     init(IPASettings settings) => (int32 ret);
> > +     init(IPASettings settings, string sensorName)
> > +             => (int32 ret, bool metadataSupport);
> >       start(StartControls controls) => (StartControls result);
> >       stop();
> >
> > @@ -77,7 +78,7 @@ interface IPARPiInterface {
> >                 map<uint32, IPAStream> streamConfig,
> >                 map<uint32, ControlInfoMap> entityControls,
> >                 ConfigInput ipaConfig)
> > -             => (ConfigOutput results, int32 ret);
> > +             => (int32 ret, ConfigOutput results);
>
> I wonder if it would make sense to split those two changes. The change
> to configure() could be reviewed and merged already, and Naush could
> take this DEMO patch as an example to move data->sensorMetadata_
> handling to match() and IPA init() time.
>

That is a reasonable approach.  I got a few things on my list to clear off
before I get to this task, so separating it to allow you to get it merged
earlier would make sense.

Regards,
Naush


>
> For the configure() part of this patch,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> You don't have to include an init() demo in v3 of the series (or just
> v2.1 of this patch actually), this is enough.
>
> >
> >       /**
> >        * \fn mapBuffers()
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 6348d071..ac18dcbd 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -79,16 +79,17 @@ public:
> >                       munmap(lsTable_, ipa::RPi::MaxLsGridSize);
> >       }
> >
> > -     int init(const IPASettings &settings) override;
> > +     int init(const IPASettings &settings, const std::string
> &sensorName,
> > +              bool *metadataSupport) override;
> >       void start(const ipa::RPi::StartControls &data,
> >                  ipa::RPi::StartControls *result) override;
> >       void stop() override {}
> >
> > -     void configure(const CameraSensorInfo &sensorInfo,
> > -                    const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                    const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> > -                    const ipa::RPi::ConfigInput &data,
> > -                    ipa::RPi::ConfigOutput *response, int32_t *ret)
> override;
> > +     int configure(const CameraSensorInfo &sensorInfo,
> > +                   const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                   const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> > +                   const ipa::RPi::ConfigInput &data,
> > +                   ipa::RPi::ConfigOutput *response) override;
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >       void signalStatReady(const uint32_t bufferId) override;
> > @@ -164,9 +165,14 @@ private:
> >       double maxFrameDuration_;
> >  };
> >
> > -int IPARPi::init(const IPASettings &settings)
> > +int IPARPi::init(const IPASettings &settings, const std::string
> &sensorName,
> > +              bool *metadataSupport)
> >  {
> > +     LOG(IPARPI, Debug) << "sensor name is " << sensorName;
> > +
> >       tuningFile_ = settings.configurationFile;
> > +
> > +     *metadataSupport = true;
> >       return 0;
> >  }
> >
> > @@ -290,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> >       mode_.max_frame_length = sensorInfo.maxFrameLength;
> >  }
> >
> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > -                    [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > -                    const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> > -                    const ipa::RPi::ConfigInput &ipaConfig,
> > -                    ipa::RPi::ConfigOutput *result, int32_t *ret)
> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > +                   [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > +                   const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> > +                   const ipa::RPi::ConfigInput &ipaConfig,
> > +                   ipa::RPi::ConfigOutput *result)
> >  {
> >       if (entityControls.size() != 2) {
> >               LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > -             *ret = -1;
> > -             return;
> > +             return -1;
> >       }
> >
> >       result->params = 0;
> > @@ -309,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >
> >       if (!validateSensorControls()) {
> >               LOG(IPARPI, Error) << "Sensor control validation failed.";
> > -             *ret = -1;
> > -             return;
> > +             return -1;
> >       }
> >
> >       if (!validateIspControls()) {
> >               LOG(IPARPI, Error) << "ISP control validation failed.";
> > -             *ret = -1;
> > -             return;
> > +             return -1;
> >       }
> >
> >       /* Setup a metadata ControlList to output metadata. */
> > @@ -334,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >               if (!helper_) {
> >                       LOG(IPARPI, Error) << "Could not create camera
> helper for "
> >                                          << cameraName;
> > -                     *ret = -1;
> > -                     return;
> > +                     return -1;
> >               }
> >
> >               /*
> > @@ -403,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >
> >       lastMode_ = mode_;
> >
> > -     *ret = 0;
> > +     return 0;
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index db91f1b5..3ff0f1cd 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1194,7 +1194,10 @@ int RPiCameraData::loadIPA()
> >
> >       IPASettings settings(ipa_->configurationFile(sensor_->model() +
> ".json"));
> >
> > -     return ipa_->init(settings);
> > +     bool metadataSupport;
> > +     int ret = ipa_->init(settings, "sensor name", &metadataSupport);
> > +     LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes"
> : "no");
> > +     return ret;
> >  }
> >
> >  int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > @@ -1250,9 +1253,8 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >       /* Ready the IPA - it must know about the sensor resolution. */
> >       ipa::RPi::ConfigOutput result;
> >
> > -     ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> > -                     &result, &ret);
> > -
> > +     ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> > +                           &result);
> >       if (ret < 0) {
> >               LOG(RPI, Error) << "IPA configuration failed!";
> >               return -EPIPE;
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Paul Elder March 9, 2021, 1:56 a.m. UTC | #3 
Hi Naush and Laurent,

On Mon, Mar 08, 2021 at 03:31:38PM +0000, Naushir Patuck wrote:
> Hi Paul and Laurent,
> 
> On Mon, 8 Mar 2021 at 08:47, Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
> 
>     Hi Paul,
> 
>     Thank you for the patch.
> 
>     On Mon, Mar 08, 2021 at 04:48:28PM +0900, Paul Elder wrote:
>     > This is just a demo to show custom parameters to init() with the
>     > raspberrypi IPA interface.
>     >
>     > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>     > ---
>     >  include/libcamera/ipa/raspberrypi.mojom       |  5 ++-
>     >  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++---------
>     >  .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++--
>     >  3 files changed, 32 insertions(+), 27 deletions(-)
>     >
>     > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/
>     ipa/raspberrypi.mojom
>     > index f733a2cd..99a62c02 100644
>     > --- a/include/libcamera/ipa/raspberrypi.mojom
>     > +++ b/include/libcamera/ipa/raspberrypi.mojom
>     > @@ -51,7 +51,8 @@ struct StartControls {
>     >  };
>     > 
>     >  interface IPARPiInterface {
>     > -     init(IPASettings settings) => (int32 ret);
>     > +     init(IPASettings settings, string sensorName)
>     > +             => (int32 ret, bool metadataSupport);
>     >       start(StartControls controls) => (StartControls result);
>     >       stop();
>     > 
>     > @@ -77,7 +78,7 @@ interface IPARPiInterface {
>     >                 map<uint32, IPAStream> streamConfig,
>     >                 map<uint32, ControlInfoMap> entityControls,
>     >                 ConfigInput ipaConfig)
>     > -             => (ConfigOutput results, int32 ret);
>     > +             => (int32 ret, ConfigOutput results);
> 
>     I wonder if it would make sense to split those two changes. The change
>     to configure() could be reviewed and merged already, and Naush could
>     take this DEMO patch as an example to move data->sensorMetadata_
>     handling to match() and IPA init() time.
> 
> 
> That is a reasonable approach.  I got a few things on my list to clear off
> before I get to this task, so separating it to allow you to get it merged
> earlier would make sense.

I didn't intend for this patch to be merged at all; it was just to show
how I intended it to be used.


Thanks,

Paul

>  
> 
> 
>     For the configure() part of this patch,
> 
>     Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>     You don't have to include an init() demo in v3 of the series (or just
>     v2.1 of this patch actually), this is enough.
> 
>     > 
>     >       /**
>     >        * \fn mapBuffers()
>     > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
>     raspberrypi.cpp
>     > index 6348d071..ac18dcbd 100644
>     > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > @@ -79,16 +79,17 @@ public:
>     >                       munmap(lsTable_, ipa::RPi::MaxLsGridSize);
>     >       }
>     > 
>     > -     int init(const IPASettings &settings) override;
>     > +     int init(const IPASettings &settings, const std::string &
>     sensorName,
>     > +              bool *metadataSupport) override;
>     >       void start(const ipa::RPi::StartControls &data,
>     >                  ipa::RPi::StartControls *result) override;
>     >       void stop() override {}
>     > 
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream> &
>     streamConfig,
>     > -                    const std::map<unsigned int, ControlInfoMap> &
>     entityControls,
>     > -                    const ipa::RPi::ConfigInput &data,
>     > -                    ipa::RPi::ConfigOutput *response, int32_t *ret)
>     override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream> &
>     streamConfig,
>     > +                   const std::map<unsigned int, ControlInfoMap> &
>     entityControls,
>     > +                   const ipa::RPi::ConfigInput &data,
>     > +                   ipa::RPi::ConfigOutput *response) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
>     >       void signalStatReady(const uint32_t bufferId) override;
>     > @@ -164,9 +165,14 @@ private:
>     >       double maxFrameDuration_;
>     >  };
>     > 
>     > -int IPARPi::init(const IPASettings &settings)
>     > +int IPARPi::init(const IPASettings &settings, const std::string &
>     sensorName,
>     > +              bool *metadataSupport)
>     >  {
>     > +     LOG(IPARPI, Debug) << "sensor name is " << sensorName;
>     > +
>     >       tuningFile_ = settings.configurationFile;
>     > +
>     > +     *metadataSupport = true;
>     >       return 0;
>     >  }
>     > 
>     > @@ -290,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo &
>     sensorInfo)
>     >       mode_.max_frame_length = sensorInfo.maxFrameLength;
>     >  }
>     > 
>     > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    const std::map<unsigned int, ControlInfoMap> &
>     entityControls,
>     > -                    const ipa::RPi::ConfigInput &ipaConfig,
>     > -                    ipa::RPi::ConfigOutput *result, int32_t *ret)
>     > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   const std::map<unsigned int, ControlInfoMap> &
>     entityControls,
>     > +                   const ipa::RPi::ConfigInput &ipaConfig,
>     > +                   ipa::RPi::ConfigOutput *result)
>     >  {
>     >       if (entityControls.size() != 2) {
>     >               LOG(IPARPI, Error) << "No ISP or sensor controls found.";
>     > -             *ret = -1;
>     > -             return;
>     > +             return -1;
>     >       }
>     > 
>     >       result->params = 0;
>     > @@ -309,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo &
>     sensorInfo,
>     > 
>     >       if (!validateSensorControls()) {
>     >               LOG(IPARPI, Error) << "Sensor control validation failed.";
>     > -             *ret = -1;
>     > -             return;
>     > +             return -1;
>     >       }
>     > 
>     >       if (!validateIspControls()) {
>     >               LOG(IPARPI, Error) << "ISP control validation failed.";
>     > -             *ret = -1;
>     > -             return;
>     > +             return -1;
>     >       }
>     > 
>     >       /* Setup a metadata ControlList to output metadata. */
>     > @@ -334,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo &
>     sensorInfo,
>     >               if (!helper_) {
>     >                       LOG(IPARPI, Error) << "Could not create camera
>     helper for "
>     >                                          << cameraName;
>     > -                     *ret = -1;
>     > -                     return;
>     > +                     return -1;
>     >               }
>     > 
>     >               /*
>     > @@ -403,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo &
>     sensorInfo,
>     > 
>     >       lastMode_ = mode_;
>     > 
>     > -     *ret = 0;
>     > +     return 0;
>     >  }
>     > 
>     >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
>     > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/
>     libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > index db91f1b5..3ff0f1cd 100644
>     > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > @@ -1194,7 +1194,10 @@ int RPiCameraData::loadIPA()
>     > 
>     >       IPASettings settings(ipa_->configurationFile(sensor_->model() +
>     ".json"));
>     > 
>     > -     return ipa_->init(settings);
>     > +     bool metadataSupport;
>     > +     int ret = ipa_->init(settings, "sensor name", &metadataSupport);
>     > +     LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes"
>     : "no");
>     > +     return ret;
>     >  }
>     > 
>     >  int RPiCameraData::configureIPA(const CameraConfiguration *config)
>     > @@ -1250,9 +1253,8 @@ int RPiCameraData::configureIPA(const
>     CameraConfiguration *config)
>     >       /* Ready the IPA - it must know about the sensor resolution. */
>     >       ipa::RPi::ConfigOutput result;
>     > 
>     > -     ipa_->configure(sensorInfo_, streamConfig, entityControls,
>     ipaConfig,
>     > -                     &result, &ret);
>     > -
>     > +     ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
>     ipaConfig,
>     > +                           &result);
>     >       if (ret < 0) {
>     >               LOG(RPI, Error) << "IPA configuration failed!";
>     >               return -EPIPE;

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
index f733a2cd..99a62c02 100644
--- a/include/libcamera/ipa/raspberrypi.mojom
+++ b/include/libcamera/ipa/raspberrypi.mojom
@@ -51,7 +51,8 @@  struct StartControls {
 };
 
 interface IPARPiInterface {
-	init(IPASettings settings) => (int32 ret);
+	init(IPASettings settings, string sensorName)
+		=> (int32 ret, bool metadataSupport);
 	start(StartControls controls) => (StartControls result);
 	stop();
 
@@ -77,7 +78,7 @@  interface IPARPiInterface {
 		  map<uint32, IPAStream> streamConfig,
 		  map<uint32, ControlInfoMap> entityControls,
 		  ConfigInput ipaConfig)
-		=> (ConfigOutput results, int32 ret);
+		=> (int32 ret, ConfigOutput results);
 
 	/**
 	 * \fn mapBuffers()
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 6348d071..ac18dcbd 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -79,16 +79,17 @@  public:
 			munmap(lsTable_, ipa::RPi::MaxLsGridSize);
 	}
 
-	int init(const IPASettings &settings) override;
+	int init(const IPASettings &settings, const std::string &sensorName,
+		 bool *metadataSupport) override;
 	void start(const ipa::RPi::StartControls &data,
 		   ipa::RPi::StartControls *result) override;
 	void stop() override {}
 
-	void configure(const CameraSensorInfo &sensorInfo,
-		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, ControlInfoMap> &entityControls,
-		       const ipa::RPi::ConfigInput &data,
-		       ipa::RPi::ConfigOutput *response, int32_t *ret) override;
+	int configure(const CameraSensorInfo &sensorInfo,
+		      const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, ControlInfoMap> &entityControls,
+		      const ipa::RPi::ConfigInput &data,
+		      ipa::RPi::ConfigOutput *response) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 	void signalStatReady(const uint32_t bufferId) override;
@@ -164,9 +165,14 @@  private:
 	double maxFrameDuration_;
 };
 
-int IPARPi::init(const IPASettings &settings)
+int IPARPi::init(const IPASettings &settings, const std::string &sensorName,
+		 bool *metadataSupport)
 {
+	LOG(IPARPI, Debug) << "sensor name is " << sensorName;
+
 	tuningFile_ = settings.configurationFile;
+
+	*metadataSupport = true;
 	return 0;
 }
 
@@ -290,16 +296,15 @@  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
 	mode_.max_frame_length = sensorInfo.maxFrameLength;
 }
 
-void IPARPi::configure(const CameraSensorInfo &sensorInfo,
-		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, ControlInfoMap> &entityControls,
-		       const ipa::RPi::ConfigInput &ipaConfig,
-		       ipa::RPi::ConfigOutput *result, int32_t *ret)
+int IPARPi::configure(const CameraSensorInfo &sensorInfo,
+		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, ControlInfoMap> &entityControls,
+		      const ipa::RPi::ConfigInput &ipaConfig,
+		      ipa::RPi::ConfigOutput *result)
 {
 	if (entityControls.size() != 2) {
 		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
-		*ret = -1;
-		return;
+		return -1;
 	}
 
 	result->params = 0;
@@ -309,14 +314,12 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 
 	if (!validateSensorControls()) {
 		LOG(IPARPI, Error) << "Sensor control validation failed.";
-		*ret = -1;
-		return;
+		return -1;
 	}
 
 	if (!validateIspControls()) {
 		LOG(IPARPI, Error) << "ISP control validation failed.";
-		*ret = -1;
-		return;
+		return -1;
 	}
 
 	/* Setup a metadata ControlList to output metadata. */
@@ -334,8 +337,7 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		if (!helper_) {
 			LOG(IPARPI, Error) << "Could not create camera helper for "
 					   << cameraName;
-			*ret = -1;
-			return;
+			return -1;
 		}
 
 		/*
@@ -403,7 +405,7 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 
 	lastMode_ = mode_;
 
-	*ret = 0;
+	return 0;
 }
 
 void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index db91f1b5..3ff0f1cd 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1194,7 +1194,10 @@  int RPiCameraData::loadIPA()
 
 	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
 
-	return ipa_->init(settings);
+	bool metadataSupport;
+	int ret = ipa_->init(settings, "sensor name", &metadataSupport);
+	LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes" : "no");
+	return ret;
 }
 
 int RPiCameraData::configureIPA(const CameraConfiguration *config)
@@ -1250,9 +1253,8 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 	/* Ready the IPA - it must know about the sensor resolution. */
 	ipa::RPi::ConfigOutput result;
 
-	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
-			&result, &ret);
-
+	ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
+			      &result);
 	if (ret < 0) {
 		LOG(RPI, Error) << "IPA configuration failed!";
 		return -EPIPE;