Message ID | 20200704005227.21782-13-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote: > The IPA is meant to be started when starting the camera, and stopped > when stopping it. It was so far start early in order to handle the s/start/started ? > IPAInterface::processEvent() call related to lens shading table > allocation before IPAInterface::configure() to pass the table to the > IPA. Now that the lens shading table is passed through configure(), > starting the IPA early isn't needed anymore. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 31 ++++++++++--------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 9babf9f4a19c..c877820a6e1e 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -303,10 +303,6 @@ public: > vcsm_.free(lsTable_); > lsTable_ = nullptr; > } > - > - /* Stop the IPA proxy thread. */ > - if (ipa_) > - ipa_->stop(); > } > > void frameStarted(uint32_t sequence); > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) > ret = queueAllBuffers(camera); > if (ret) { > LOG(RPI, Error) << "Failed to queue buffers"; > + stop(camera); > + return ret; > + } > + > + /* Start the IPA. */ > + ret = data->ipa_->start(); > + if (ret) { > + LOG(RPI, Error) > + << "Failed to start IPA for " << camera->name(); > + stop(camera); > return ret; > } > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) > V4L2DeviceFormat sensorFormat; > data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); > ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); > - if (ret) > + if (ret) { > + stop(camera); Do we want to stop the whole camera or just the IPA here ? I think you meant data->ipa_->stop(), right ? With this fixed Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > return ret; > + } > > /* Enable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > data->bayerQueue_ = std::queue<FrameBuffer *>{}; > data->embeddedQueue_ = std::queue<FrameBuffer *>{}; > > + /* Stop the IPA. */ > + data->ipa_->stop(); > + > freeBuffers(camera); > } > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() > .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") > }; > > - ipa_->init(settings); > - > - /* > - * Startup the IPA thread now. Without this call, none of the IPA API > - * functions will run. > - * > - * It only gets stopped in the class destructor. > - */ > - return ipa_->start(); > + return ipa_->init(settings); > } > > int RPiCameraData::configureIPA() > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote: > On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote: > > The IPA is meant to be started when starting the camera, and stopped > > when stopping it. It was so far start early in order to handle the > > s/start/started ? Indeed. > > IPAInterface::processEvent() call related to lens shading table > > allocation before IPAInterface::configure() to pass the table to the > > IPA. Now that the lens shading table is passed through configure(), > > starting the IPA early isn't needed anymore. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 31 ++++++++++--------- > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 9babf9f4a19c..c877820a6e1e 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -303,10 +303,6 @@ public: > > vcsm_.free(lsTable_); > > lsTable_ = nullptr; > > } > > - > > - /* Stop the IPA proxy thread. */ > > - if (ipa_) > > - ipa_->stop(); > > } > > > > void frameStarted(uint32_t sequence); > > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) > > ret = queueAllBuffers(camera); > > if (ret) { > > LOG(RPI, Error) << "Failed to queue buffers"; > > + stop(camera); > > + return ret; > > + } > > + > > + /* Start the IPA. */ > > + ret = data->ipa_->start(); > > + if (ret) { > > + LOG(RPI, Error) > > + << "Failed to start IPA for " << camera->name(); > > + stop(camera); > > return ret; > > } > > > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) > > V4L2DeviceFormat sensorFormat; > > data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); > > ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); > > - if (ret) > > + if (ret) { > > + stop(camera); > > Do we want to stop the whole camera or just the IPA here ? > I think you meant data->ipa_->stop(), right ? The idea is that the PipelineHandlerRPi::stop() function should clean up everything that start() may have done, and can be called in start() when an error occurs. To make this easier I need IPAProxy::stop() to be safe to call when the IPA is already stopped. I'll send a patch to do so. > With this fixed > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > return ret; > > + } > > > > /* Enable SOF event generation. */ > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > > data->bayerQueue_ = std::queue<FrameBuffer *>{}; > > data->embeddedQueue_ = std::queue<FrameBuffer *>{}; > > > > + /* Stop the IPA. */ > > + data->ipa_->stop(); > > + > > freeBuffers(camera); > > } > > > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() > > .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") > > }; > > > > - ipa_->init(settings); > > - > > - /* > > - * Startup the IPA thread now. Without this call, none of the IPA API > > - * functions will run. > > - * > > - * It only gets stopped in the class destructor. > > - */ > > - return ipa_->start(); > > + return ipa_->init(settings); > > } > > > > int RPiCameraData::configureIPA()
Hi Laurent, On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote: > > On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote: > > > The IPA is meant to be started when starting the camera, and stopped > > > when stopping it. It was so far start early in order to handle the > > > > s/start/started ? > > Indeed. > > > > IPAInterface::processEvent() call related to lens shading table > > > allocation before IPAInterface::configure() to pass the table to the > > > IPA. Now that the lens shading table is passed through configure(), > > > starting the IPA early isn't needed anymore. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 31 ++++++++++--------- > > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 9babf9f4a19c..c877820a6e1e 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -303,10 +303,6 @@ public: > > > vcsm_.free(lsTable_); > > > lsTable_ = nullptr; > > > } > > > - > > > - /* Stop the IPA proxy thread. */ > > > - if (ipa_) > > > - ipa_->stop(); > > > } > > > > > > void frameStarted(uint32_t sequence); > > > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) > > > ret = queueAllBuffers(camera); > > > if (ret) { > > > LOG(RPI, Error) << "Failed to queue buffers"; > > > + stop(camera); > > > + return ret; > > > + } > > > + > > > + /* Start the IPA. */ > > > + ret = data->ipa_->start(); > > > + if (ret) { > > > + LOG(RPI, Error) > > > + << "Failed to start IPA for " << camera->name(); > > > + stop(camera); > > > return ret; > > > } > > > > > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) > > > V4L2DeviceFormat sensorFormat; > > > data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); > > > ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); > > > - if (ret) > > > + if (ret) { > > > + stop(camera); > > > > Do we want to stop the whole camera or just the IPA here ? > > I think you meant data->ipa_->stop(), right ? > > The idea is that the PipelineHandlerRPi::stop() function should clean up > everything that start() may have done, and can be called in start() when > an error occurs. To make this easier I need IPAProxy::stop() to be safe > to call when the IPA is already stopped. I'll send a patch to do so. > Does this mean all the error paths in the start() function should call stop() ? > > With this fixed > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Please reain my tag here! Thanks j > > > return ret; > > > + } > > > > > > /* Enable SOF event generation. */ > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > > > data->bayerQueue_ = std::queue<FrameBuffer *>{}; > > > data->embeddedQueue_ = std::queue<FrameBuffer *>{}; > > > > > > + /* Stop the IPA. */ > > > + data->ipa_->stop(); > > > + > > > freeBuffers(camera); > > > } > > > > > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() > > > .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") > > > }; > > > > > > - ipa_->init(settings); > > > - > > > - /* > > > - * Startup the IPA thread now. Without this call, none of the IPA API > > > - * functions will run. > > > - * > > > - * It only gets stopped in the class destructor. > > > - */ > > > - return ipa_->start(); > > > + return ipa_->init(settings); > > > } > > > > > > int RPiCameraData::configureIPA() > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Jul 06, 2020 at 01:37:56PM +0200, Jacopo Mondi wrote: > On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote: > > On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote: > >> On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote: > >>> The IPA is meant to be started when starting the camera, and stopped > >>> when stopping it. It was so far start early in order to handle the > >> > >> s/start/started ? > > > > Indeed. > > > >>> IPAInterface::processEvent() call related to lens shading table > >>> allocation before IPAInterface::configure() to pass the table to the > >>> IPA. Now that the lens shading table is passed through configure(), > >>> starting the IPA early isn't needed anymore. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>> --- > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 31 ++++++++++--------- > >>> 1 file changed, 17 insertions(+), 14 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> index 9babf9f4a19c..c877820a6e1e 100644 > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> @@ -303,10 +303,6 @@ public: > >>> vcsm_.free(lsTable_); > >>> lsTable_ = nullptr; > >>> } > >>> - > >>> - /* Stop the IPA proxy thread. */ > >>> - if (ipa_) > >>> - ipa_->stop(); > >>> } > >>> > >>> void frameStarted(uint32_t sequence); > >>> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) > >>> ret = queueAllBuffers(camera); > >>> if (ret) { > >>> LOG(RPI, Error) << "Failed to queue buffers"; > >>> + stop(camera); > >>> + return ret; > >>> + } > >>> + > >>> + /* Start the IPA. */ > >>> + ret = data->ipa_->start(); > >>> + if (ret) { > >>> + LOG(RPI, Error) > >>> + << "Failed to start IPA for " << camera->name(); > >>> + stop(camera); > >>> return ret; > >>> } > >>> > >>> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) > >>> V4L2DeviceFormat sensorFormat; > >>> data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); > >>> ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); > >>> - if (ret) > >>> + if (ret) { > >>> + stop(camera); > >> > >> Do we want to stop the whole camera or just the IPA here ? > >> I think you meant data->ipa_->stop(), right ? > > > > The idea is that the PipelineHandlerRPi::stop() function should clean up > > everything that start() may have done, and can be called in start() when > > an error occurs. To make this easier I need IPAProxy::stop() to be safe > > to call when the IPA is already stopped. I'll send a patch to do so. > > Does this mean all the error paths in the start() function should call > stop() ? Yes, that's the idea. > >> With this fixed > >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Please reain my tag here! > > >>> return ret; > >>> + } > >>> > >>> /* Enable SOF event generation. */ > >>> data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > >>> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > >>> data->bayerQueue_ = std::queue<FrameBuffer *>{}; > >>> data->embeddedQueue_ = std::queue<FrameBuffer *>{}; > >>> > >>> + /* Stop the IPA. */ > >>> + data->ipa_->stop(); > >>> + > >>> freeBuffers(camera); > >>> } > >>> > >>> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() > >>> .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") > >>> }; > >>> > >>> - ipa_->init(settings); > >>> - > >>> - /* > >>> - * Startup the IPA thread now. Without this call, none of the IPA API > >>> - * functions will run. > >>> - * > >>> - * It only gets stopped in the class destructor. > >>> - */ > >>> - return ipa_->start(); > >>> + return ipa_->init(settings); > >>> } > >>> > >>> int RPiCameraData::configureIPA()
Hi Laurent, Thank you for the patch. On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The IPA is meant to be started when starting the camera, and stopped > when stopping it. It was so far start early in order to handle the > IPAInterface::processEvent() call related to lens shading table > allocation before IPAInterface::configure() to pass the table to the > IPA. Now that the lens shading table is passed through configure(), > starting the IPA early isn't needed anymore. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> One question, what is the overhead of ipa_->start() and ipa_->stop(). If we have to do this on a mode switch preparing for a capture (where we call pipeline_handler::stop() and pipeline_handler::start()), it might increase capture latency. Perhaps this does not matter, or we can address if it is indeed a problem. Otherwise, Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 31 ++++++++++--------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 9babf9f4a19c..c877820a6e1e 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -303,10 +303,6 @@ public: > vcsm_.free(lsTable_); > lsTable_ = nullptr; > } > - > - /* Stop the IPA proxy thread. */ > - if (ipa_) > - ipa_->stop(); > } > > void frameStarted(uint32_t sequence); > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) > ret = queueAllBuffers(camera); > if (ret) { > LOG(RPI, Error) << "Failed to queue buffers"; > + stop(camera); > + return ret; > + } > + > + /* Start the IPA. */ > + ret = data->ipa_->start(); > + if (ret) { > + LOG(RPI, Error) > + << "Failed to start IPA for " << camera->name(); > + stop(camera); > return ret; > } > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) > V4L2DeviceFormat sensorFormat; > data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); > ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); > - if (ret) > + if (ret) { > + stop(camera); > return ret; > + } > > /* Enable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > data->bayerQueue_ = std::queue<FrameBuffer *>{}; > data->embeddedQueue_ = std::queue<FrameBuffer *>{}; > > + /* Stop the IPA. */ > + data->ipa_->stop(); > + > freeBuffers(camera); > } > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() > .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") > }; > > - ipa_->init(settings); > - > - /* > - * Startup the IPA thread now. Without this call, none of the IPA API > - * functions will run. > - * > - * It only gets stopped in the class destructor. > - */ > - return ipa_->start(); > + return ipa_->init(settings); > } > > int RPiCameraData::configureIPA() > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Jul 08, 2020 at 12:12:58PM +0100, Naushir Patuck wrote: > On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote: > > > > The IPA is meant to be started when starting the camera, and stopped > > when stopping it. It was so far start early in order to handle the > > IPAInterface::processEvent() call related to lens shading table > > allocation before IPAInterface::configure() to pass the table to the > > IPA. Now that the lens shading table is passed through configure(), > > starting the IPA early isn't needed anymore. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > One question, what is the overhead of ipa_->start() and ipa_->stop(). > If we have to do this on a mode switch preparing for a capture (where > we call pipeline_handler::stop() and pipeline_handler::start()), it > might increase capture latency. Perhaps this does not matter, or we > can address if it is indeed a problem. Otherwise, There's a small overhead, but I think it's offset by IPAInterface::configure() becoming a direct call instead of a cross-thread call. It may be more of a problem for closed IPA modules isolated in a separate process though, it's a good point. I could reconsider that, but I'd rather first align all implementations on the current model, and then rework start/stop (possibly with the addition of prepare/finish to handle the expensive operations that are not related to starting and stopping the camera itself) on top. > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 31 ++++++++++--------- > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 9babf9f4a19c..c877820a6e1e 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -303,10 +303,6 @@ public: > > vcsm_.free(lsTable_); > > lsTable_ = nullptr; > > } > > - > > - /* Stop the IPA proxy thread. */ > > - if (ipa_) > > - ipa_->stop(); > > } > > > > void frameStarted(uint32_t sequence); > > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) > > ret = queueAllBuffers(camera); > > if (ret) { > > LOG(RPI, Error) << "Failed to queue buffers"; > > + stop(camera); > > + return ret; > > + } > > + > > + /* Start the IPA. */ > > + ret = data->ipa_->start(); > > + if (ret) { > > + LOG(RPI, Error) > > + << "Failed to start IPA for " << camera->name(); > > + stop(camera); > > return ret; > > } > > > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) > > V4L2DeviceFormat sensorFormat; > > data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); > > ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); > > - if (ret) > > + if (ret) { > > + stop(camera); > > return ret; > > + } > > > > /* Enable SOF event generation. */ > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > > data->bayerQueue_ = std::queue<FrameBuffer *>{}; > > data->embeddedQueue_ = std::queue<FrameBuffer *>{}; > > > > + /* Stop the IPA. */ > > + data->ipa_->stop(); > > + > > freeBuffers(camera); > > } > > > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() > > .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") > > }; > > > > - ipa_->init(settings); > > - > > - /* > > - * Startup the IPA thread now. Without this call, none of the IPA API > > - * functions will run. > > - * > > - * It only gets stopped in the class destructor. > > - */ > > - return ipa_->start(); > > + return ipa_->init(settings); > > } > > > > int RPiCameraData::configureIPA()
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 9babf9f4a19c..c877820a6e1e 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -303,10 +303,6 @@ public: vcsm_.free(lsTable_); lsTable_ = nullptr; } - - /* Stop the IPA proxy thread. */ - if (ipa_) - ipa_->stop(); } void frameStarted(uint32_t sequence); @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera) ret = queueAllBuffers(camera); if (ret) { LOG(RPI, Error) << "Failed to queue buffers"; + stop(camera); + return ret; + } + + /* Start the IPA. */ + ret = data->ipa_->start(); + if (ret) { + LOG(RPI, Error) + << "Failed to start IPA for " << camera->name(); + stop(camera); return ret; } @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera) V4L2DeviceFormat sensorFormat; data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat); ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat); - if (ret) + if (ret) { + stop(camera); return ret; + } /* Enable SOF event generation. */ data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera) data->bayerQueue_ = std::queue<FrameBuffer *>{}; data->embeddedQueue_ = std::queue<FrameBuffer *>{}; + /* Stop the IPA. */ + data->ipa_->stop(); + freeBuffers(camera); } @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA() .configurationFile = ipa_->configurationFile(sensor_->model() + ".json") }; - ipa_->init(settings); - - /* - * Startup the IPA thread now. Without this call, none of the IPA API - * functions will run. - * - * It only gets stopped in the class destructor. - */ - return ipa_->start(); + return ipa_->init(settings); } int RPiCameraData::configureIPA()