[libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi: Repurpose RPi::Stream::reset()

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2022-03-21 10:27:01)
> The original use of RPi::Stream::reset() was to clear the external flag state
> and free/clear out the framebuffers for the stream. However, the latter is now
> done through PipelineHandlerRPi::configure(). Rework
> PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() instead of
> RPi::Stream::reset() to achieve the same thing.
> 
> Repurpose RPi::Stream::reset() to instead reset the state of the buffer handling
> logic, where all internally allocated buffers are put back into the queue of
> available buffers. As such, rename the function to RPi::Stream::resetbuffers()
> This resetbuffers() is now called from PipelineHandlerRPi::start(), allowing the

/resetbuffers/resetBuffers/ twice above. But could be handled while
applying.


> pipeline handler to correctly deal with start()/stop()/start() sequences and
> reusing the buffers allocated on the first 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>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--
>  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 13 ++++++-------
>  src/libcamera/pipeline/raspberrypi/rpi_stream.h    |  2 +-
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 92043962494b..0ff6acdceb66 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -693,7 +693,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>         /* Start by freeing all buffers and reset the Unicam and ISP stream states. */
>         data->freeBuffers();
>         for (auto const stream : data->streams_)
> -               stream->reset();
> +               stream->setExternal(false);
>  
>         BayerFormat::Packing packing = BayerFormat::Packing::CSI2;
>         Size maxSize, sensorSize;
> @@ -991,6 +991,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>         RPiCameraData *data = cameraData(camera);
>         int ret;
>  
> +       for (auto const stream : data->streams_)
> +               stream->resetBuffers();
> +
>         if (!data->buffersAllocated_) {
>                 /* Allocate buffers for internal pipeline usage. */
>                 ret = prepareBuffers(camera);
> @@ -1031,7 +1034,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
>  
>         /*
> -        * Reset the delayed controls with the gain and exposure values set by
> +        * resetBuffers the delayed controls with the gain and exposure values set by

Possibly a little too much of a match on the find/replace?

Could easily be dropped while applying.


>          * the IPA.
>          */
>         data->delayedCtrls_->reset();
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index f446e1ce66c7..7a93efaa29da 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -26,10 +26,12 @@ std::string Stream::name() const
>         return name_;
>  }
>  
> -void Stream::reset()
> +void Stream::resetBuffers()
>  {
> -       external_ = false;
> -       clearBuffers();
> +       /* Add all internal buffers to the queue of usable buffers. */
> +       availableBuffers_ = {};

I think this is ok, but I get a bit worried when I see blanket buffer
state changes like that.

> +       for (auto const &buffer : internalBuffers_)
> +               availableBuffers_.push(buffer.get());

Is there any other action that is required to update the state of the
buffer if it was currently in use when this gets called? (Currently I
think the only call site is below so it's fine).


With the above fixes (that can be fixed while applying if suitable)

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

>  }
>  
>  void Stream::setExternal(bool external)
> @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count)
>  
>                         /* Add these exported buffers to the internal/external buffer list. */
>                         setExportedBuffers(&internalBuffers_);
> -
> -                       /* Add these buffers to the queue of internal usable buffers. */
> -                       for (auto const &buffer : internalBuffers_)
> -                               availableBuffers_.push(buffer.get());
> +                       resetBuffers();
>                 }
>  
>                 /* We must import all internal/external exported buffers. */
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index d6f49d34f8c8..c37f7e82eef6 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -45,7 +45,7 @@ public:
>         V4L2VideoDevice *dev() const;
>         std::string name() const;
>         bool isImporter() const;
> -       void reset();
> +       void resetBuffers();
>  
>         void setExternal(bool external);
>         bool isExternal() const;
> -- 
> 2.25.1
>