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