[libcamera-devel] [PATCH v2 3/3] DEMO: raspberrypi: Use custom parameters to init()
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Mar 9 02:56:06 CET 2021
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 at 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 at 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 at 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;
More information about the libcamera-devel
mailing list