[libcamera-devel] [PATCH 1/2] libcamera: Add a PipelineHandler::releaseDevice method

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:
> This notifies pipeline handlers when a camera is released, in case
> they want to free any resources or memory buffers.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 +++-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-

The pipeline handler writer's guide
(Documentation/guides/pipeline-handler.rst) needs to be udpated. The
vivid pipeline handler may also need some attention, but we can handle
that.

>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 96aab9d6..ec4f662d 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -46,7 +46,7 @@ public:
>  					const DeviceMatch &dm);
>  
>  	bool acquire();
> -	void release();
> +	void release(Camera *camera);
>  
>  	virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) = 0;
> @@ -74,6 +74,8 @@ protected:
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
>  	virtual void stopDevice(Camera *camera) = 0;
>  
> +	virtual void releaseDevice(Camera *camera);
> +
>  	CameraManager *manager_;
>  
>  private:
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c4f65c1a..2d947a44 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -870,7 +870,7 @@ int Camera::release()
>  		return ret == -EACCES ? -EBUSY : ret;
>  
>  	if (d->isAcquired())
> -		d->pipe_->release();
> +		d->pipe_->release(this);
>  
>  	d->setState(Private::CameraAvailable);
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 825aff5a..cfade490 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()
>  
>  /**
>   * \brief Release exclusive access to the pipeline handler
> + * \param[in] camera The camera for which to release data

This function doesn't release "data".

>   *
>   * This function releases access to the pipeline handler previously acquired by
>   * a call to acquire(). Every release() call shall match a previous successful
> @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()
>   *
>   * \sa acquire()
>   */
> -void PipelineHandler::release()
> +void PipelineHandler::release(Camera *camera)
>  {
>  	MutexLocker locker(lock_);
>  
> @@ -204,9 +205,22 @@ void PipelineHandler::release()
>  
>  	unlockMediaDevices();
>  
> +	releaseDevice(camera);
> +
>  	--useCount_;
>  }
>  
> +/**
> + * \brief Release resources associated with this camera
> + * \param[in] camera The camera for which to release resources
> + *
> + * Pipeline handlers may override this in order to perform cleanup operations
> + * when a camera is released, such as freeing memory.

This needs further documentation, to clearly explain what can or can't
be done here. Examples could help.

This being said, I'm not sure hooking up this feature in the
Camera::release() function is the right option. In addition to the lack
of balance between acquire() and release() (in this series the release()
function does more than counterbalance acquire()), the usage pattern
that was envisioned is that application can hold onto cameras for a long
time. Having to call release() to free memory means that another
application could then acquire the camera.

> + */
> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> +{
> +}
> +
>  void PipelineHandler::unlockMediaDevices()
>  {
>  	for (std::shared_ptr<MediaDevice> &media : mediaDevices_)

-- 
Regards,

Laurent Pinchart