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

[Naushir Patuck]

Hi David,

Thank you for your review on this and the other patches in this series!

On Thu, 10 Mar 2022 at 10:57, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for this patch!
>
> On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > 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. This reset() is now called from
> PipelineHandlerRPi::start(),
> > allowing the 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>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 ++++-
> >  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 11 +++++------
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index b17ffa235ac7..193361686d3c 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -692,7 +692,7 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >         /* Start by freeing all buffers and resetting the Unicam and ISP
> stream states. */
> >         data->freeBuffers();
> >         for (auto const stream : data->streams_)
> > -               stream->reset();
> > +               stream->setExternal(false);
>
> So I think that replacing the former calls to reset() by setExternal()
> is a good thing, because "reset" didn't give you much of a clue about
> what was being reset!
>
> I guess my only feeling about repurposing the name "reset" is that
> it's still rather vague. Could we think of a name that gives someone a
> clue as to what is being reset? Maybe "resetAvailableBuffers", or just
> "resetBuffers"?
>

Sure I'll rename this to resetBuffers in v2 to be clearer of the new
function.

Regards,
Naush


>
> >
> >         BayerFormat::Packing packing = BayerFormat::Packing::CSI2;
> >         Size maxSize, sensorSize;
> > @@ -985,6 +985,9 @@ int PipelineHandlerRPi::start(Camera *camera, const
> ControlList *controls)
> >         RPiCameraData *data = cameraData(camera);
> >         int ret;
> >
> > +       for (auto const stream : data->streams_)
> > +               stream->reset();
> > +
> >         if (data->reconfigured_) {
> >                 /* Allocate buffers for internal pipeline usage. */
> >                 ret = prepareBuffers(camera);
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index f446e1ce66c7..0840ec4f54a0 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -28,8 +28,10 @@ std::string Stream::name() const
> >
> >  void Stream::reset()
> >  {
> > -       external_ = false;
> > -       clearBuffers();
> > +       /* Add all internal buffers to the queue of usable buffers. */
> > +       availableBuffers_ = {};
> > +       for (auto const &buffer : internalBuffers_)
> > +               availableBuffers_.push(buffer.get());
> >  }
> >
> >  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());
> > +                       reset();
> >                 }
> >
> >                 /* We must import all internal/external exported
> buffers. */
> > --
> > 2.25.1
> >
>
> But either way, with or without any further changes:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220310/0f3fd8c2/attachment.htm>