[libcamera-devel] [PATCH v3 09/11] libcamera: camera: Assert pipelines complete all requests

[Kieran Bingham]

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 at 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
> 

-- 
Regards
--
Kieran