[libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle failures during IPA configuration
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Dec 7 16:14:16 CET 2020
Hi Naush, Paul,
On 07/12/2020 13:11, Naushir Patuck wrote:
> Hi Paul,
>
> Thank you for your review.
>
> On Fri, 4 Dec 2020 at 10:39, <paul.elder at ideasonboard.com
> <mailto:paul.elder at ideasonboard.com>> wrote:
>
> Hi Naush,
>
> On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
> > > If the IPA fails during configuration, return an error flag to the
> > > pipeline handler and fail the use case gracefully.
> > >
> > > At present, the IPA configuration can fail for the following
> reasons:
> > > - The sensor is not recognised, and fails to open a CamHelper
> object.
> > > - The pipeline handler did not pass in controls for the ISP and
> sensor.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com
> <mailto:naush at raspberrypi.com>>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org
> <mailto:jacopo at jmondi.org>>
> >
> > Wouldn't it be simpler to modify the configure() function to return an
> > int ? How about the following ? If it works for you I'll submit a
> proper
> > patch.
>
> From the perspective of the IPC changes, Naush's solution is more easily
> translatable. When the IPA interface will become customizable, there
> isn't a way to specify which output parameters should be return values
> vs return parameters. Thus keeping them all as return parameters is the
> better solution here.
>
> So imo this is fine to merge as-is.
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com
> <mailto:paul.elder at ideasonboard.com>>
>
>
> In which case, I think this is ready to be merged :)
Pushed.
Regards
--
Kieran
> Regards,
> Naush
>
>
>
> > Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com
> <mailto:laurent.pinchart at ideasonboard.com>>
> > Date: Tue Dec 1 03:54:18 2020 +0200
> >
> > libcamera: ipa_interface: Make configure() return an int
> >
> > It's useful for the IPAInterface::configure() function to be
> able to
> > return a status. Make it return an int, to avoid forcing IPAs
> to return
> > a status encoded in the IPAOperationData in a custom way.
> >
> > Signed-off-by: Laurent Pinchart
> <laurent.pinchart at ideasonboard.com
> <mailto:laurent.pinchart at ideasonboard.com>>
> >
> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> b/include/libcamera/internal/ipa_context_wrapper.h
> > index 8f767e844221..a00b5e7b92eb 100644
> > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > @@ -22,11 +22,11 @@ public:
> > int init(const IPASettings &settings) override;
> > int start() override;
> > void stop() override;
> > - void configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int, IPAStream>
> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result) override;
> > + int configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream>
> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *result) override;
> >
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids)
> override;
> > diff --git a/include/libcamera/ipa/ipa_interface.h
> b/include/libcamera/ipa/ipa_interface.h
> > index 322b7079070e..1d8cf8dc1c56 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -95,12 +95,12 @@ struct ipa_context_ops {
> > void (*register_callbacks)(struct ipa_context *ctx,
> > const struct ipa_callback_ops
> *callbacks,
> > void *cb_ctx);
> > - void (*configure)(struct ipa_context *ctx,
> > - const struct ipa_sensor_info *sensor_info,
> > - const struct ipa_stream *streams,
> > - unsigned int num_streams,
> > - const struct ipa_control_info_map *maps,
> > - unsigned int num_maps);
> > + int (*configure)(struct ipa_context *ctx,
> > + const struct ipa_sensor_info *sensor_info,
> > + const struct ipa_stream *streams,
> > + unsigned int num_streams,
> > + const struct ipa_control_info_map *maps,
> > + unsigned int num_maps);
> > void (*map_buffers)(struct ipa_context *ctx,
> > const struct ipa_buffer *buffers,
> > size_t num_buffers);
> > @@ -156,11 +156,11 @@ public:
> > virtual int start() = 0;
> > virtual void stop() = 0;
> >
> > - virtual void configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int,
> IPAStream> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result) = 0;
> > + virtual int configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int,
> IPAStream> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *result) = 0;
> >
> > virtual void mapBuffers(const std::vector<IPABuffer>
> &buffers) = 0;
> > virtual void unmapBuffers(const std::vector<unsigned int>
> &ids) = 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index cee532e3a747..74d7bc6e1c9b 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -115,12 +115,12 @@ void
> IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> > ctx->cb_ctx_ = cb_ctx;
> > }
> >
> > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > - const struct ipa_sensor_info
> *sensor_info,
> > - const struct ipa_stream *streams,
> > - unsigned int num_streams,
> > - const struct
> ipa_control_info_map *maps,
> > - unsigned int num_maps)
> > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > + const struct ipa_sensor_info
> *sensor_info,
> > + const struct ipa_stream *streams,
> > + unsigned int num_streams,
> > + const struct ipa_control_info_map
> *maps,
> > + unsigned int num_maps)
> > {
> > IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> *>(_ctx);
> >
> > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct
> ipa_context *_ctx,
> >
> > /* \todo Translate the ipaConfig and result. */
> > IPAOperationData ipaConfig;
> > - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> ipaConfig,
> > - nullptr);
> > + return ctx->ipa_->configure(sensorInfo, ipaStreams,
> entityControls,
> > + ipaConfig, nullptr);
> > }
> >
> > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
> b/src/ipa/libipa/ipa_interface_wrapper.h
> > index a1c701599b56..acd3160039b1 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -30,12 +30,12 @@ private:
> > static void register_callbacks(struct ipa_context *ctx,
> > const struct ipa_callback_ops
> *callbacks,
> > void *cb_ctx);
> > - static void configure(struct ipa_context *ctx,
> > - const struct ipa_sensor_info *sensor_info,
> > - const struct ipa_stream *streams,
> > - unsigned int num_streams,
> > - const struct ipa_control_info_map *maps,
> > - unsigned int num_maps);
> > + static int configure(struct ipa_context *ctx,
> > + const struct ipa_sensor_info *sensor_info,
> > + const struct ipa_stream *streams,
> > + unsigned int num_streams,
> > + const struct ipa_control_info_map *maps,
> > + unsigned int num_maps);
> > static void map_buffers(struct ipa_context *ctx,
> > const struct ipa_buffer *c_buffers,
> > size_t num_buffers);
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 9853a343c892..75f7af3430ef 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -81,11 +81,11 @@ public:
> > int start() override { return 0; }
> > void stop() override {}
> >
> > - void configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int, IPAStream>
> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &data,
> > - IPAOperationData *response) override;
> > + int configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream>
> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &data,
> > + IPAOperationData *response) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids)
> override;
> > void processEvent(const IPAOperationData &event) override;
> > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> > mode_.line_length = 1e9 * sensorInfo.lineLength /
> sensorInfo.pixelRate;
> > }
> >
> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > - [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result)
> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > + [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *result)
> > {
> > if (entityControls.empty())
> > - return;
> > + return -EINVAL;
> >
> > result->operation = 0;
> >
> > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > }
> >
> > lastMode_ = mode_;
> > + return 0;
> > }
> >
> > void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 07d7f1b2ddd8..78d78c5ac521 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -40,11 +40,11 @@ public:
> > int start() override { return 0; }
> > void stop() override {}
> >
> > - void configure(const CameraSensorInfo &info,
> > - const std::map<unsigned int, IPAStream>
> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *response) override;
> > + int configure(const CameraSensorInfo &info,
> > + const std::map<unsigned int, IPAStream>
> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *response) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids)
> override;
> > void processEvent(const IPAOperationData &event) override;
> > @@ -79,27 +79,27 @@ private:
> > * assemble one. Make sure the reported sensor information are
> relevant
> > * before accessing them.
> > */
> > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo
> &info,
> > - [[maybe_unused]] const std::map<unsigned
> int, IPAStream> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > - [[maybe_unused]] IPAOperationData *result)
> > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo
> &info,
> > + [[maybe_unused]] const std::map<unsigned
> int, IPAStream> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > + [[maybe_unused]] IPAOperationData *result)
> > {
> > if (entityControls.empty())
> > - return;
> > + return -EINVAL;
> >
> > ctrls_ = entityControls.at(0);
> >
> > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> > if (itExp == ctrls_.end()) {
> > LOG(IPARkISP1, Error) << "Can't find exposure control";
> > - return;
> > + return -EINVAL;
> > }
> >
> > const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> > if (itGain == ctrls_.end()) {
> > LOG(IPARkISP1, Error) << "Can't find gain control";
> > - return;
> > + return -EINVAL;
> > }
> >
> > autoExposure_ = true;
> > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]]
> const CameraSensorInfo &info,
> > << " Gain: " << minGain_ << "-" << maxGain_;
> >
> > setControls(0);
> > + return 0;
> > }
> >
> > void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index cf8411359e40..79c8c2fb8927 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -37,11 +37,11 @@ public:
> > int start() override;
> > void stop() override;
> >
> > - void configure([[maybe_unused]] const CameraSensorInfo
> &sensorInfo,
> > - [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > - [[maybe_unused]] const std::map<unsigned int,
> const ControlInfoMap &> &entityControls,
> > - [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > - [[maybe_unused]] IPAOperationData *result)
> override {}
> > + int configure([[maybe_unused]] const CameraSensorInfo
> &sensorInfo,
> > + [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > + [[maybe_unused]] const std::map<unsigned int,
> const ControlInfoMap &> &entityControls,
> > + [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > + [[maybe_unused]] IPAOperationData *result)
> override { return 0; }
> > void mapBuffers([[maybe_unused]] const
> std::vector<IPABuffer> &buffers) override {}
> > void unmapBuffers([[maybe_unused]] const
> std::vector<unsigned int> &ids) override {}
> > void processEvent([[maybe_unused]] const IPAOperationData
> &event) override {}
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> b/src/libcamera/ipa_context_wrapper.cpp
> > index 231300ce0bec..f63ad830c003 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
> > ctx_->ops->stop(ctx_);
> > }
> >
> > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int,
> IPAStream> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result)
> > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int,
> IPAStream> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *result)
> > {
> > if (intf_)
> > return intf_->configure(sensorInfo, streamConfig,
> > entityControls, ipaConfig,
> result);
> >
> > if (!ctx_)
> > - return;
> > + return 0;
> >
> > serializer_.reset();
> >
> > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
> CameraSensorInfo &sensorInfo,
> > }
> >
> > /* \todo Translate the ipaConfig and reponse */
> > - ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> streamConfig.size(),
> > - c_info_maps, entityControls.size());
> > + return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> > + streamConfig.size(), c_info_maps,
> > + entityControls.size());
> > }
> >
> > void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer>
> &buffers)
> > diff --git a/src/libcamera/ipa_interface.cpp
> b/src/libcamera/ipa_interface.cpp
> > index 23fc56d7d48e..516a8ecd4b53 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -347,6 +347,8 @@
> > * \param[in] num_maps The number of entries in the \a maps array
> > *
> > * \sa libcamera::IPAInterface::configure()
> > + *
> > + * \return 0 on success or a negative error code on failure
> > */
> >
> > /**
> > @@ -573,6 +575,8 @@ namespace libcamera {
> > * pipeline handler to the IPA and back. The pipeline handler may
> set the \a
> > * result parameter to null if the IPA protocol doesn't need to
> pass a result
> > * back through the configure() function.
> > + *
> > + * \return 0 on success or a negative error code on failure
> > */
> >
> > /**
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index b78a0e4535f5..ed250ce79c17 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -32,11 +32,11 @@ public:
> > }
> > int start() override { return 0; }
> > void stop() override {}
> > - void configure([[maybe_unused]] const CameraSensorInfo
> &sensorInfo,
> > - [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > - [[maybe_unused]] const std::map<unsigned int,
> const ControlInfoMap &> &entityControls,
> > - [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > - [[maybe_unused]] IPAOperationData *result)
> override {}
> > + int configure([[maybe_unused]] const CameraSensorInfo
> &sensorInfo,
> > + [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > + [[maybe_unused]] const std::map<unsigned int,
> const ControlInfoMap &> &entityControls,
> > + [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > + [[maybe_unused]] IPAOperationData *result)
> override { return 0; }
> > void mapBuffers([[maybe_unused]] const
> std::vector<IPABuffer> &buffers) override {}
> > void unmapBuffers([[maybe_unused]] const
> std::vector<unsigned int> &ids) override {}
> > void processEvent([[maybe_unused]] const IPAOperationData
> &event) override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index eead2883708d..fd91726c4840 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -29,11 +29,11 @@ public:
> > int start() override;
> > void stop() override;
> >
> > - void configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int, IPAStream>
> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result) override;
> > + int configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream>
> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *result) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids)
> override;
> > void processEvent(const IPAOperationData &event) override;
> > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
> > thread_.wait();
> > }
> >
> > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int,
> IPAStream> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result)
> > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int,
> IPAStream> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + const IPAOperationData &ipaConfig,
> > + IPAOperationData *result)
> > {
> > - ipa_->configure(sensorInfo, streamConfig, entityControls,
> ipaConfig,
> > - result);
> > + return ipa_->configure(sensorInfo, streamConfig, entityControls,
> > + ipaConfig, result);
> > }
> >
> > void IPAProxyThread::mapBuffers(const std::vector<IPABuffer>
> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp
> b/test/ipa/ipa_wrappers_test.cpp
> > index 59d991cbbf6a..ad0fb0386865 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -67,55 +67,63 @@ public:
> > report(Op_stop, TestPass);
> > }
> >
> > - void configure(const CameraSensorInfo &sensorInfo,
> > - const std::map<unsigned int, IPAStream>
> &streamConfig,
> > - const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > - [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > - [[maybe_unused]] IPAOperationData *result)
> override
> > + int configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream>
> &streamConfig,
> > + const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > + [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > + [[maybe_unused]] IPAOperationData *result)
> override
> > {
> > /* Verify sensorInfo. */
> > if (sensorInfo.outputSize.width != 2560 ||
> > sensorInfo.outputSize.height != 1940) {
> > cerr << "configure(): Invalid sensor info size "
> > << sensorInfo.outputSize.toString();
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> >
> > /* Verify streamConfig. */
> > if (streamConfig.size() != 2) {
> > cerr << "configure(): Invalid number of
> streams "
> > << streamConfig.size() << endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> >
> > auto iter = streamConfig.find(1);
> > if (iter == streamConfig.end()) {
> > cerr << "configure(): No configuration for
> stream 1" << endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> > const IPAStream *stream = &iter->second;
> > if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
> > stream->size != Size{ 1024, 768 }) {
> > cerr << "configure(): Invalid configuration
> for stream 1" << endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> >
> > iter = streamConfig.find(2);
> > if (iter == streamConfig.end()) {
> > cerr << "configure(): No configuration for
> stream 2" << endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> > stream = &iter->second;
> > if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
> > stream->size != Size{ 800, 600 }) {
> > cerr << "configure(): Invalid configuration
> for stream 2" << endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> >
> > /* Verify entityControls. */
> > auto ctrlIter = entityControls.find(42);
> > if (ctrlIter == entityControls.end()) {
> > cerr << "configure(): Controls not found" <<
> endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> >
> > const ControlInfoMap &infoMap = ctrlIter->second;
> > @@ -124,10 +132,12 @@ public:
> > infoMap.count(V4L2_CID_CONTRAST) != 1 ||
> > infoMap.count(V4L2_CID_SATURATION) != 1) {
> > cerr << "configure(): Invalid control IDs"
> << endl;
> > - return report(Op_configure, TestFail);
> > + report(Op_configure, TestFail);
> > + return -EINVAL;
> > }
> >
> > report(Op_configure, TestPass);
> > + return 0;
> > }
> >
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override
> >
> > > ---
> > > include/libcamera/ipa/raspberrypi.h | 1 +
> > > src/ipa/raspberrypi/raspberrypi.cpp | 12
> +++++++++++-
> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++
> > > 3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> > > index 2b55dbfc..86c97ffa 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -21,6 +21,7 @@ enum ConfigParameters {
> > > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > > IPA_CONFIG_SENSOR = (1 << 2),
> > > IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > > + IPA_CONFIG_FAILED = (1 << 4),
> > > };
> > >
> > > enum Operations {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 9853a343..57dd9c61 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -197,8 +197,11 @@ void IPARPi::configure(const
> CameraSensorInfo &sensorInfo,
> > > const IPAOperationData &ipaConfig,
> > > IPAOperationData *result)
> > > {
> > > - if (entityControls.empty())
> > > + if (entityControls.size() != 2) {
> > > + LOG(IPARPI, Error) << "No ISP or sensor controls
> found.";
> > > + result->operation = RPi::IPA_CONFIG_FAILED;
> > > return;
> > > + }
> > >
> > > result->operation = 0;
> > >
> > > @@ -217,6 +220,13 @@ void IPARPi::configure(const
> CameraSensorInfo &sensorInfo,
> > > if (!helper_) {
> > > helper_ =
> std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
> > >
> > > + if (!helper_) {
> > > + LOG(IPARPI, Error) << "Could not create
> camera helper for "
> > > + << cameraName;
> > > + result->operation = RPi::IPA_CONFIG_FAILED;
> > > + return;
> > > + }
> > > +
> > > /*
> > > * Pass out the sensor config to the pipeline
> handler in order
> > > * to setup the staggered writer class.
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 6fcdf557..76252806 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> > > ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> > > &result);
> > >
> > > + if (result.operation & RPi::IPA_CONFIG_FAILED) {
> > > + LOG(RPI, Error) << "IPA configuration failed!";
> > > + return -EPIPE;
> > > + }
> > > +
> > > unsigned int resultIdx = 0;
> > > if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> > > /*
>
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list