| Message ID | 20210325134231.1400051-12-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Mar 25, 2021 at 01:42:29PM +0000, Kieran Bingham wrote: > When the camera manager calls stop on a pipeline, it is expected that > the pipeline handler guarantees all requests are returned back to the > application before the camera has stopped. > > Ensure that this guarantee is met by providing an accessor on the > pipeline handler to validate that all pending requests are removed. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > Note: > > I wanted to make PipelineHandler::active() a const function, but then > I'm unable to call it through invokeMethod(). Does invokeMethod support > calling const functions? Or did I do something obviously wrong? What's the compilation error ? > --- > include/libcamera/internal/pipeline_handler.h | 1 + > src/libcamera/camera.cpp | 3 +++ > src/libcamera/pipeline_handler.cpp | 17 +++++++++++++++++ > 3 files changed, 21 insertions(+) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 9bdda8f3b799..1410a06ebabe 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -80,6 +80,7 @@ public: > > virtual int start(Camera *camera, const ControlList *controls) = 0; > virtual void stop(Camera *camera) = 0; > + bool active(const Camera *camera); > > int queueRequest(Request *request); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 7f7956ba732f..99b01633ff5f 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1086,6 +1086,9 @@ int Camera::stop() > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > this); > > + ASSERT(!d->pipe_->invokeMethod(&PipelineHandler::active, > + ConnectionTypeBlocking, this)); > + As the pipeline handler has stopped, there should be no race, can we call ASSERT(!d->pipe_->active()); directly ? That would also solve your const problem :-) > d->setState(Private::CameraConfigured); > > return 0; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index e3d4975d9ffd..c1ea4374b445 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -357,6 +357,23 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const > * \context This function is called from the CameraManager thread. > */ > > +/** > + * \brief Determine if the camera has any requests pending > + * \param[in] camera The camera to check > + * > + * This method determines if there are any requests queued to the pipeline > + * awaiting processing. > + * > + * \context This function is called from the CameraManager thread. > + * This would need to be updated. > + * \return True if there are pending requests, or false otherwise > + */ > +bool PipelineHandler::active(const Camera *camera) > +{ > + const CameraData *data = cameraData(camera); > + return !data->queuedRequests_.empty(); > +} > + > /** > * \fn PipelineHandler::queueRequest() > * \brief Queue a request
On 26/03/2021 02:42, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Mar 25, 2021 at 01:42:29PM +0000, Kieran Bingham wrote: >> When the camera manager calls stop on a pipeline, it is expected that >> the pipeline handler guarantees all requests are returned back to the >> application before the camera has stopped. >> >> Ensure that this guarantee is met by providing an accessor on the >> pipeline handler to validate that all pending requests are removed. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> Note: >> >> I wanted to make PipelineHandler::active() a const function, but then >> I'm unable to call it through invokeMethod(). Does invokeMethod support >> calling const functions? Or did I do something obviously wrong? > > What's the compilation error ? With the following diff: +++ b/include/libcamera/internal/pipeline_handler.h - bool active(const Camera *camera); + bool active(const Camera *camera) const; +++ b/src/libcamera/pipeline_handler.cpp -bool PipelineHandler::active(const Camera *camera) +bool PipelineHandler::active(const Camera *camera) const I get: > In file included from ../src/libcamera/camera.cpp:18: > ../src/libcamera/camera.cpp: In member function ‘int libcamera::Camera::stop()’: > ../src/libcamera/camera.cpp:1087:40: error: no matching function for call to ‘libcamera::PipelineHandler::invokeMethod(bool (libcamera::PipelineHandler::*)(const libcamera::Camera*) const, libcamera::ConnectionType, libcamera::Camera*)’ > 1087 | ConnectionTypeBlocking, this)); > | ^ > ../include/libcamera/internal/log.h:125:8: note: in definition of macro ‘ASSERT’ > 125 | if (!(condition)) \ > | ^~~~~~~~~ > In file included from ../include/libcamera/camera.h:17, > from ../src/libcamera/camera.cpp:8: > ../include/libcamera/object.h:36:4: note: candidate: ‘template<class T, class R, class ... FuncArgs, class ... Args, std::enable_if_t<std::is_base_of<libcamera::Object, T>::value>* <anonymous> > R libcamera::Object::invokeMethod(R (T::*)(FuncArgs ...), libcamera::ConnectionType, Args ...)’ > 36 | R invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type, > | ^~~~~~~~~~~~ > ../include/libcamera/object.h:36:4: note: template argument deduction/substitution failed: > In file included from ../src/libcamera/camera.cpp:18: > ../src/libcamera/camera.cpp:1087:40: note: types ‘R (T::)(FuncArgs ...)’ and ‘bool (libcamera::PipelineHandler::)(const libcamera::Camera*) const’ have incompatible cv-qualifiers > 1087 | ConnectionTypeBlocking, this)); > | ^ > ../include/libcamera/internal/log.h:125:8: note: in definition of macro ‘ASSERT’ > 125 | if (!(condition)) \ > | ^~~~~~~~~ > [11/107] Compiling C++ object src/libcamera/libcamera.so.p/meson-generated_.._proxy_raspberrypi_ipa_proxy.cpp.o > ninja: build stopped: subcommand failed. > make: *** [Makefile:51: libcamera] Error 1 But I don't really care, as I like your suggestion below: > >> --- >> include/libcamera/internal/pipeline_handler.h | 1 + >> src/libcamera/camera.cpp | 3 +++ >> src/libcamera/pipeline_handler.cpp | 17 +++++++++++++++++ >> 3 files changed, 21 insertions(+) >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >> index 9bdda8f3b799..1410a06ebabe 100644 >> --- a/include/libcamera/internal/pipeline_handler.h >> +++ b/include/libcamera/internal/pipeline_handler.h >> @@ -80,6 +80,7 @@ public: >> >> virtual int start(Camera *camera, const ControlList *controls) = 0; >> virtual void stop(Camera *camera) = 0; >> + bool active(const Camera *camera); >> >> int queueRequest(Request *request); >> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index 7f7956ba732f..99b01633ff5f 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -1086,6 +1086,9 @@ int Camera::stop() >> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, >> this); >> >> + ASSERT(!d->pipe_->invokeMethod(&PipelineHandler::active, >> + ConnectionTypeBlocking, this)); >> + > > As the pipeline handler has stopped, there should be no race, can we > call > > ASSERT(!d->pipe_->active()); > > directly ? That would also solve your const problem :-) Well, then that's even better! > >> d->setState(Private::CameraConfigured); >> >> return 0; >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >> index e3d4975d9ffd..c1ea4374b445 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -357,6 +357,23 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const >> * \context This function is called from the CameraManager thread. >> */ >> >> +/** >> + * \brief Determine if the camera has any requests pending >> + * \param[in] camera The camera to check >> + * >> + * This method determines if there are any requests queued to the pipeline >> + * awaiting processing. >> + * >> + * \context This function is called from the CameraManager thread. >> + * > > This would need to be updated. Certainly. > >> + * \return True if there are pending requests, or false otherwise >> + */ >> +bool PipelineHandler::active(const Camera *camera) >> +{ >> + const CameraData *data = cameraData(camera); >> + return !data->queuedRequests_.empty(); >> +} >> + >> /** >> * \fn PipelineHandler::queueRequest() >> * \brief Queue a request >
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 9bdda8f3b799..1410a06ebabe 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -80,6 +80,7 @@ public: virtual int start(Camera *camera, const ControlList *controls) = 0; virtual void stop(Camera *camera) = 0; + bool active(const Camera *camera); int queueRequest(Request *request); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 7f7956ba732f..99b01633ff5f 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1086,6 +1086,9 @@ int Camera::stop() d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, this); + ASSERT(!d->pipe_->invokeMethod(&PipelineHandler::active, + ConnectionTypeBlocking, this)); + d->setState(Private::CameraConfigured); return 0; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index e3d4975d9ffd..c1ea4374b445 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -357,6 +357,23 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const * \context This function is called from the CameraManager thread. */ +/** + * \brief Determine if the camera has any requests pending + * \param[in] camera The camera to check + * + * This method determines if there are any requests queued to the pipeline + * awaiting processing. + * + * \context This function is called from the CameraManager thread. + * + * \return True if there are pending requests, or false otherwise + */ +bool PipelineHandler::active(const Camera *camera) +{ + const CameraData *data = cameraData(camera); + return !data->queuedRequests_.empty(); +} + /** * \fn PipelineHandler::queueRequest() * \brief Queue a request
When the camera manager calls stop on a pipeline, it is expected that the pipeline handler guarantees all requests are returned back to the application before the camera has stopped. Ensure that this guarantee is met by providing an accessor on the pipeline handler to validate that all pending requests are removed. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Note: I wanted to make PipelineHandler::active() a const function, but then I'm unable to call it through invokeMethod(). Does invokeMethod support calling const functions? Or did I do something obviously wrong? --- include/libcamera/internal/pipeline_handler.h | 1 + src/libcamera/camera.cpp | 3 +++ src/libcamera/pipeline_handler.cpp | 17 +++++++++++++++++ 3 files changed, 21 insertions(+)