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

[Kieran Bingham]

Quoting Kieran Bingham (2022-03-21 11:51:37)
> 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).

Aha, it's not the only call site, there's the one up in start() as well
of course.

So at start() time, is there any possiblity that some buffer isn't
already in availableBuffers_ ? and if not ... why not ?

Is there some worry about buffer state on calls to stop() not moving
buffers into the availableBuffers_ list?

--
Kieran


> 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
> >