[libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi: Repurpose RPi::Stream::reset()
Naushir Patuck
naush at raspberrypi.com
Mon Mar 21 13:24:01 CET 2022
Hi Kieran,
On Mon, 21 Mar 2022 at 11:55, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.
>
:-(
Sorry, my brain is functioning like it's still Sunday this morning. I'll
fix this up and
post an update.
> >
> >
> > > * 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 ?
>
At start time, all internal buffers would have been allocated, so any/every
buffer will be listed in availableBuffers_.
> Is there some worry about buffer state on calls to stop() not moving
> buffers into the availableBuffers_ list?
>
I think this is ok, as the resetBuffers() call on start() will (as it says
on the tin)
reset all buffer states and mark them as available again, if that makes
sense.
Regards,
Naush
>
> --
> 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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220321/59730b0b/attachment-0001.htm>
More information about the libcamera-devel
mailing list