[libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi: Repurpose RPi::Stream::reset()
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 21 13:37:01 CET 2022
Quoting Naushir Patuck (2022-03-21 12:24:01)
> 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.
Yes, that makes sense, but I would call this the 'shotgun defence', and
shoots everything instead of just the target required, so I wonder if
there is a corresponding ASSERT() (or non-fatal print) that is worth
adding to catch a case when buffers were not in the expected state. The
clear and re-add will put them all in the right list - but if that was
preceeded by an unknown state, it can be worth noting it.
Not essential though, it's only if this is a potential for identifying
buffer state leaks or otherwise catching when buffers didn't go through
the 'normal/expected' flow.
--
Kieran
> 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
> > > >
> >
More information about the libcamera-devel
mailing list