[libcamera-devel] [PATCH 12/13] pipeline: raspberrypi: Add stream flags to RPi::Stream
Naushir Patuck
naush at raspberrypi.com
Thu Apr 27 08:53:56 CEST 2023
Hi Jacopo,
Thank you for your feedback.
On Wed, 26 Apr 2023 at 17:24, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Naush
>
> On Wed, Apr 26, 2023 at 02:10:56PM +0100, Naushir Patuck via libcamera-devel wrote:
> > Add a flags_ field to indicate stream state information in RPi::Stream.
> > This replaces the existing external_ and importOnly_ boolean flags.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++--
> > .../pipeline/rpi/common/rpi_stream.cpp | 40 ++++++++++--------
> > .../pipeline/rpi/common/rpi_stream.h | 42 ++++++++++++-------
> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 8 ++--
> > 4 files changed, 60 insertions(+), 38 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 012766b38c32..4d38d511c383 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -31,6 +31,8 @@ using namespace RPi;
> >
> > LOG_DEFINE_CATEGORY(RPI)
> >
> > +using Flag = RPi::Stream::StreamFlag;
> > +
>
> The only doubt I have is about "Flag" not being better expressed as
> "StreamFlags" in the two 'using' directives in this patch.
Ack. I'll change it to StreamFlag and reply-to this patch with the update.
Regards,
Naush
>
> The rest looks good
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
> j
>
> > namespace {
> >
> > constexpr unsigned int defaultRawBitDepth = 12;
> > @@ -504,7 +506,7 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > /* Start by freeing all buffers and reset the stream states. */
> > data->freeBuffers();
> > for (auto const stream : data->streams_)
> > - stream->setExternal(false);
> > + stream->clearFlags(Flag::External);
> >
> > std::vector<CameraData::StreamParams> rawStreams, ispStreams;
> > std::optional<BayerFormat::Packing> packing;
> > @@ -752,7 +754,7 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request)
> >
> > /* Push all buffers supplied in the Request to the respective streams. */
> > for (auto stream : data->streams_) {
> > - if (!stream->isExternal())
> > + if (!(stream->getFlags() & Flag::External))
> > continue;
> >
> > FrameBuffer *buffer = request->findBuffer(stream);
> > @@ -931,7 +933,7 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)
> > int ret;
> >
> > for (auto const stream : data->streams_) {
> > - if (!stream->isExternal()) {
> > + if (!(stream->getFlags() & Flag::External)) {
> > ret = stream->queueAllBuffers();
> > if (ret < 0)
> > return ret;
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > index b7e4130f5e56..c158843cb0ed 100644
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > @@ -14,6 +14,24 @@ LOG_DEFINE_CATEGORY(RPISTREAM)
> >
> > namespace RPi {
> >
> > +void Stream::setFlags(StreamFlags flags)
> > +{
> > + flags_ |= flags;
> > +
> > + /* Import streams cannot be external. */
> > + ASSERT(!(flags_ & StreamFlag::External) || !(flags_ & StreamFlag::ImportOnly));
> > +}
> > +
> > +void Stream::clearFlags(StreamFlags flags)
> > +{
> > + flags_ &= ~flags;
> > +}
> > +
> > +RPi::Stream::StreamFlags Stream::getFlags() const
> > +{
> > + return flags_;
> > +}
> > +
> > V4L2VideoDevice *Stream::dev() const
> > {
> > return dev_.get();
> > @@ -32,18 +50,6 @@ void Stream::resetBuffers()
> > availableBuffers_.push(buffer.get());
> > }
> >
> > -void Stream::setExternal(bool external)
> > -{
> > - /* Import streams cannot be external. */
> > - ASSERT(!external || !importOnly_);
> > - external_ = external;
> > -}
> > -
> > -bool Stream::isExternal() const
> > -{
> > - return external_;
> > -}
> > -
> > void Stream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > {
> > for (auto const &buffer : *buffers)
> > @@ -57,7 +63,7 @@ const BufferMap &Stream::getBuffers() const
> >
> > unsigned int Stream::getBufferId(FrameBuffer *buffer) const
> > {
> > - if (importOnly_)
> > + if (flags_ & StreamFlag::ImportOnly)
> > return 0;
> >
> > /* Find the buffer in the map, and return the buffer id. */
> > @@ -88,7 +94,7 @@ int Stream::prepareBuffers(unsigned int count)
> > {
> > int ret;
> >
> > - if (!importOnly_) {
> > + if (!(flags_ & StreamFlag::ImportOnly)) {
> > if (count) {
> > /* Export some frame buffers for internal use. */
> > ret = dev_->exportBuffers(count, &internalBuffers_);
> > @@ -113,7 +119,7 @@ int Stream::prepareBuffers(unsigned int count)
> > * \todo Find a better heuristic, or, even better, an exact solution to
> > * this issue.
> > */
> > - if (isExternal() || importOnly_)
> > + if ((flags_ & StreamFlag::External) || (flags_ & StreamFlag::ImportOnly))
> > count = count * 2;
> >
> > return dev_->importBuffers(count);
> > @@ -160,7 +166,7 @@ int Stream::queueBuffer(FrameBuffer *buffer)
> >
> > void Stream::returnBuffer(FrameBuffer *buffer)
> > {
> > - if (!external_) {
> > + if (!(flags_ & StreamFlag::External)) {
> > /* For internal buffers, simply requeue back to the device. */
> > queueToDevice(buffer);
> > return;
> > @@ -204,7 +210,7 @@ int Stream::queueAllBuffers()
> > {
> > int ret;
> >
> > - if (external_)
> > + if (flags_ & StreamFlag::External)
> > return 0;
> >
> > while (!availableBuffers_.empty()) {
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > index b8c74de35863..6edd304bdfe2 100644
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > @@ -12,6 +12,7 @@
> > #include <unordered_map>
> > #include <vector>
> >
> > +#include <libcamera/base/flags.h>
> > #include <libcamera/stream.h>
> >
> > #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -37,25 +38,41 @@ enum BufferMask {
> > class Stream : public libcamera::Stream
> > {
> > public:
> > + enum class StreamFlag {
> > + None = 0,
> > + /*
> > + * Indicates that this stream only imports buffers, e.g. the ISP
> > + * input stream.
> > + */
> > + ImportOnly = (1 << 0),
> > + /*
> > + * Indicates that this stream is active externally, i.e. the
> > + * buffers might be provided by (and returned to) the application.
> > + */
> > + External = (1 << 1),
> > + };
> > +
> > + using StreamFlags = Flags<StreamFlag>;
> > +
> > Stream()
> > - : id_(BufferMask::MaskID)
> > + : flags_(StreamFlag::None), id_(BufferMask::MaskID)
> > {
> > }
> >
> > - Stream(const char *name, MediaEntity *dev, bool importOnly = false)
> > - : external_(false), importOnly_(importOnly), name_(name),
> > + Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None)
> > + : flags_(flags), name_(name),
> > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID)
> > {
> > }
> >
> > + void setFlags(StreamFlags flags);
> > + void clearFlags(StreamFlags flags);
> > + StreamFlags getFlags() const;
> > +
> > V4L2VideoDevice *dev() const;
> > const std::string &name() const;
> > - bool isImporter() const;
> > void resetBuffers();
> >
> > - void setExternal(bool external);
> > - bool isExternal() const;
> > -
> > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > const BufferMap &getBuffers() const;
> > unsigned int getBufferId(FrameBuffer *buffer) const;
> > @@ -112,14 +129,7 @@ private:
> > void clearBuffers();
> > int queueToDevice(FrameBuffer *buffer);
> >
> > - /*
> > - * Indicates that this stream is active externally, i.e. the buffers
> > - * might be provided by (and returned to) the application.
> > - */
> > - bool external_;
> > -
> > - /* Indicates that this stream only imports buffers, e.g. ISP input. */
> > - bool importOnly_;
> > + StreamFlags flags_;
> >
> > /* Stream name identifier. */
> > std::string name_;
> > @@ -182,4 +192,6 @@ public:
> >
> > } /* namespace RPi */
> >
> > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(RPi::Stream::StreamFlag)
> > +
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index a085a06376a8..bd7bfb3a7663 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -23,6 +23,8 @@ namespace libcamera {
> >
> > LOG_DECLARE_CATEGORY(RPI)
> >
> > +using Flag = RPi::Stream::StreamFlag;
> > +
> > namespace {
> >
> > enum class Unicam : unsigned int { Image, Embedded };
> > @@ -320,7 +322,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer
> > }
> >
> > /* Tag the ISP input stream as an import stream. */
> > - data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
> > + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly);
> > data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> > data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
> > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
> > @@ -502,7 +504,7 @@ int Vc4CameraData::platformConfigure(const V4L2SubdeviceFormat &sensorFormat,
> > */
> > if (!rawStreams.empty()) {
> > rawStreams[0].cfg->setStream(&unicam_[Unicam::Image]);
> > - unicam_[Unicam::Image].setExternal(true);
> > + unicam_[Unicam::Image].setFlags(Flag::External);
> > }
> >
> > ret = isp_[Isp::Input].dev()->setFormat(&unicamFormat);
> > @@ -547,7 +549,7 @@ int Vc4CameraData::platformConfigure(const V4L2SubdeviceFormat &sensorFormat,
> > << ColorSpace::toString(cfg->colorSpace);
> >
> > cfg->setStream(stream);
> > - stream->setExternal(true);
> > + stream->setFlags(Flag::External);
> > }
> >
> > ispOutputTotal_ = outStreams.size();
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list