[libcamera-devel,v2,3/6] pipeline: raspberrypi: Free buffers in the RPiCamera destructor and re-configure

15473 diff mbox series

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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 33 +++++++++++++------
 1 file changed, 23 insertions(+), 10 deletions(-)

Quoting Naushir Patuck via libcamera-devel (2022-03-17 14:08:24)
> 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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.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..082273828894 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), reallocate_(true), ispOutputCount_(0)
>         {
>         }
>  
> +       ~RPiCameraData()
> +       {
> +               freeBuffers();
> +       }
> +
>         void freeBuffers();
>         void frameStarted(uint32_t sequence);
>  
> @@ -280,6 +285,9 @@ public:
>          */
>         std::optional<int32_t> notifyGainsUnity_;
>  
> +       /* Has this camera been reconfigured? */
> +       bool reallocate_;
> +
>  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->reallocate_) {

On the first iteration, this isn't a reallocation, so I would have
probably used a variable called 'hasBuffers_', and 
	if (!data->hasBuffers_)

or something to declare the state rather than the action.

But this looks like it works all the same.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +               /* 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->reallocate_ = false;
>         }
>  
>         /* 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();
> +
> +       reallocate_ = true;
>  }
>  
>  void RPiCameraData::frameStarted(uint32_t sequence)
> -- 
> 2.25.1
>