[libcamera-devel] [PATCH v3 3/5] pipeline: raspberrypi: Free buffers in the RPiCamera destructor and re-configure

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Mar 21, 2022 at 10:27:00AM +0000, Naushir Patuck via libcamera-devel wrote:
> Currently, all framebuffer allocations get freed and cleared on a stop in
> PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called
> without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and
> prepare all the buffers again, which is unnecessary.
> 
> Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but
> insted doing it in PipelineHandlerRPi::configure(), as the buffers might have
> to be resized.
> 
> Add a flag to indicate that buffer allocations need to be done on the next
> call to PipelineHandlerRPi::start().
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 33 +++++++++++++------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 2281b43fc3ac..92043962494b 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -185,10 +185,15 @@ public:
>  	RPiCameraData(PipelineHandler *pipe)
>  		: Camera::Private(pipe), state_(State::Stopped),
>  		  supportsFlips_(false), flipsAlterBayerOrder_(false),
> -		  dropFrameCount_(0), ispOutputCount_(0)
> +		  dropFrameCount_(0), buffersAllocated_(false), ispOutputCount_(0)
>  	{
>  	}
>  
> +	~RPiCameraData()
> +	{
> +		freeBuffers();
> +	}
> +
>  	void freeBuffers();
>  	void frameStarted(uint32_t sequence);
>  
> @@ -280,6 +285,9 @@ public:
>  	 */
>  	std::optional<int32_t> notifyGainsUnity_;
>  
> +	/* Have internal buffers been allocated? */
> +	bool buffersAllocated_;
> +
>  private:
>  	void checkRequestCompleted();
>  	void fillRequestMetadata(const ControlList &bufferControls,
> @@ -682,7 +690,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	RPiCameraData *data = cameraData(camera);
>  	int ret;
>  
> -	/* Start by resetting the Unicam and ISP stream states. */
> +	/* Start by freeing all buffers and reset the Unicam and ISP stream states. */
> +	data->freeBuffers();
>  	for (auto const stream : data->streams_)
>  		stream->reset();
>  
> @@ -982,12 +991,16 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  	RPiCameraData *data = cameraData(camera);
>  	int ret;
>  
> -	/* Allocate buffers for internal pipeline usage. */
> -	ret = prepareBuffers(camera);
> -	if (ret) {
> -		LOG(RPI, Error) << "Failed to allocate buffers";
> -		stop(camera);
> -		return ret;
> +	if (!data->buffersAllocated_) {
> +		/* Allocate buffers for internal pipeline usage. */
> +		ret = prepareBuffers(camera);
> +		if (ret) {
> +			LOG(RPI, Error) << "Failed to allocate buffers";
> +			data->freeBuffers();
> +			stop(camera);
> +			return ret;
> +		}
> +		data->buffersAllocated_ = true;
>  	}
>  
>  	/* Check if a ScalerCrop control was specified. */
> @@ -1055,8 +1068,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
>  
>  	/* Stop the IPA. */
>  	data->ipa_->stop();
> -
> -	data->freeBuffers();
>  }
>  
>  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> @@ -1461,6 +1472,8 @@ void RPiCameraData::freeBuffers()
>  
>  	for (auto const stream : streams_)
>  		stream->releaseBuffers();
> +
> +	buffersAllocated_ = false;
>  }
>  
>  void RPiCameraData::frameStarted(uint32_t sequence)

-- 
Regards,

Laurent Pinchart