[libcamera-devel] [PATCH 6/9] libcamera: pipeline: raspberrypi: Rework stream buffer logic for zero-copy
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Jul 13 13:56:34 CEST 2020
Hi Naushir,
Thanks for your patch.
On 2020-07-13 09:47:25 +0100, Naushir Patuck wrote:
> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> instead export/import all buffers. This allows the pipeline to return
> any stream buffer requested by the application as zero-copy.
>
> Advertise the Unicam Image stream as the RAW capture stream now.
>
> The RPiStream object now maintains a new list of buffers that are
> available to queue into a device. This is needed to distinguish between
> FrameBuffers allocated for internal use vs externally provided buffers.
> When a Request comes in, if a buffer is not provided for an exported
> stream, we re-use a buffer from this list.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
I'm no expert on this pipeline but from a RAW point of view I think it
looks good!
> ---
> .../pipeline/raspberrypi/raspberrypi.cpp | 181 ++++++++----------
> .../pipeline/raspberrypi/rpi_stream.h | 136 +++++++++----
> 2 files changed, 179 insertions(+), 138 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 2f4255a4..f7e16a6c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -200,7 +200,8 @@ private:
> void checkRequestCompleted();
> void tryRunPipeline();
> void tryFlushQueues();
> - FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> + FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> + RPi::RPiStream *stream);
>
> unsigned int ispOutputCount_;
> };
> @@ -523,8 +524,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> StreamConfiguration &cfg = config->at(i);
>
> if (isRaw(cfg.pixelFormat)) {
> - cfg.setStream(&data->isp_[Isp::Input]);
> - data->isp_[Isp::Input].setExternal(true);
> + cfg.setStream(&data->unicam_[Unicam::Image]);
> + /*
> + * We must set both Unicam streams as external, even
> + * though the application may only request RAW frames.
> + * This is because we match timestamps on both streams
> + * to synchronise buffers.
> + */
> + data->unicam_[Unicam::Image].setExternal(true);
> + data->unicam_[Unicam::Embedded].setExternal(true);
> continue;
> }
>
> @@ -718,14 +726,13 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> if (data->state_ == RPiCameraData::State::Stopped)
> return -EINVAL;
>
> - /* Ensure all external streams have associated buffers! */
> - for (auto &stream : data->isp_) {
> - if (!stream.isExternal())
> - continue;
> + LOG(RPI, Debug) << "queueRequestDevice: New request.";
>
> - if (!request->findBuffer(&stream)) {
> - LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> - return -ENOENT;
> + /* Push all buffers supplied in the Request to the respective streams. */
> + for (auto stream : data->streams_) {
> + if (stream->isExternal()) {
> + FrameBuffer *buffer = request->findBuffer(stream);
> + stream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer);
> }
> }
>
> @@ -814,12 +821,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> /* Initialize the camera properties. */
> data->properties_ = data->sensor_->properties();
>
> - /*
> - * List the available output streams.
> - * Currently cannot do Unicam streams!
> - */
> + /*List the available streams an application may request. */
> std::set<Stream *> streams;
> - streams.insert(&data->isp_[Isp::Input]);
> + streams.insert(&data->unicam_[Unicam::Image]);
> + streams.insert(&data->unicam_[Unicam::Embedded]);
> streams.insert(&data->isp_[Isp::Output0]);
> streams.insert(&data->isp_[Isp::Output1]);
> streams.insert(&data->isp_[Isp::Stats]);
> @@ -896,7 +901,7 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> int ret;
>
> for (auto const stream : data->streams_) {
> - ret = stream->queueBuffers();
> + ret = stream->queueAllBuffers();
> if (ret < 0)
> return ret;
> }
> @@ -912,7 +917,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> /*
> * Decide how many internal buffers to allocate. For now, simply look
> * at how many external buffers will be provided. Will need to improve
> - * this logic.
> + * this logic. However, we really must have all stream allocate the same
> + * number of buffers to simplify error handling in queueRequestDevice().
> */
> unsigned int maxBuffers = 0;
> for (const Stream *s : camera->streams())
> @@ -920,33 +926,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>
> for (auto const stream : data->streams_) {
> - if (stream->isExternal() || stream->isImporter()) {
> - /*
> - * If a stream is marked as external reserve memory to
> - * prepare to import as many buffers are requested in
> - * the stream configuration.
> - *
> - * If a stream is an internal stream with importer
> - * role, reserve as many buffers as possible.
> - */
> - unsigned int count = stream->isExternal()
> - ? stream->configuration().bufferCount
> - : maxBuffers;
> - ret = stream->importBuffers(count);
> - if (ret < 0)
> - return ret;
> - } else {
> - /*
> - * If the stream is an internal exporter allocate and
> - * export as many buffers as possible to its internal
> - * pool.
> - */
> - ret = stream->allocateBuffers(maxBuffers);
> - if (ret < 0) {
> - freeBuffers(camera);
> - return ret;
> - }
> - }
> + ret = stream->prepareBuffers(maxBuffers);
> + if (ret < 0)
> + return ret;
> }
>
> /*
> @@ -954,7 +936,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> */
> count = 0;
> - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> b->setCookie(count++);
> }
>
> @@ -963,14 +945,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> * the IPA.
> */
> count = 0;
> - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> b->setCookie(count++);
> data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> .planes = b->planes() });
> }
>
> count = 0;
> - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> b->setCookie(count++);
> data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> .planes = b->planes() });
> @@ -1081,7 +1063,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> switch (action.operation) {
> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> unsigned int bufferId = action.data[0];
> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>
> handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> /* Fill the Request metadata buffer with what the IPA has provided */
> @@ -1092,19 +1074,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>
> case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> unsigned int bufferId = action.data[0];
> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> break;
> }
>
> case RPI_IPA_ACTION_RUN_ISP: {
> unsigned int bufferId = action.data[0];
> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>
> LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> << ", timestamp: " << buffer->metadata().timestamp;
>
> - isp_[Isp::Input].dev()->queueBuffer(buffer);
> + isp_[Isp::Input].queueBuffer(buffer);
> ispOutputCount_ = 0;
> break;
> }
> @@ -1205,7 +1187,12 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> << ", buffer id " << buffer->cookie()
> << ", timestamp: " << buffer->metadata().timestamp;
>
> - handleStreamBuffer(buffer, stream);
> + /*
> + * ISP statistics buffer must not be re-queued or sent back to the
> + * application until after the IPA signals so.
> + */
> + if (stream != &isp_[Isp::Stats])
> + handleStreamBuffer(buffer, stream);
>
> /*
> * Increment the number of ISP outputs generated.
> @@ -1233,8 +1220,12 @@ void RPiCameraData::clearIncompleteRequests()
> */
> for (auto const request : requestQueue_) {
> for (auto const stream : streams_) {
> - if (stream->isExternal())
> - stream->dev()->queueBuffer(request->findBuffer(stream));
> + if (!stream->isExternal())
> + continue;
> +
> + FrameBuffer *buffer = request->findBuffer(stream);
> + if (buffer)
> + stream->queueBuffer(buffer);
> }
> }
>
> @@ -1263,7 +1254,7 @@ void RPiCameraData::clearIncompleteRequests()
> * Has the buffer already been handed back to the
> * request? If not, do so now.
> */
> - if (buffer->request())
> + if (buffer && buffer->request())
> pipe_->completeBuffer(camera_, request, buffer);
> }
>
> @@ -1275,30 +1266,24 @@ void RPiCameraData::clearIncompleteRequests()
> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> {
> if (stream->isExternal()) {
> - if (!dropFrameCount_) {
> - Request *request = buffer->request();
> + Request *request = requestQueue_.front();
> +
> + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> + /*
> + * Tag the buffer as completed, returning it to the
> + * application.
> + */
> pipe_->completeBuffer(camera_, request, buffer);
> + } else {
> + /*
> + * This buffer was not part of the Request, so we can
> + * recycle it.
> + */
> + stream->returnBuffer(buffer);
> }
> } else {
> - /* Special handling for RAW buffer Requests.
> - *
> - * The ISP input stream is alway an import stream, but if the
> - * current Request has been made for a buffer on the stream,
> - * simply memcpy to the Request buffer and requeue back to the
> - * device.
> - */
> - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> - const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> - Request *request = requestQueue_.front();
> - FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> - if (raw) {
> - raw->copyFrom(buffer);
> - pipe_->completeBuffer(camera_, request, raw);
> - }
> - }
> -
> - /* Simply requeue the buffer. */
> - stream->dev()->queueBuffer(buffer);
> + /* Simply re-queue the buffer to the requested stream. */
> + stream->queueBuffer(buffer);
> }
> }
>
> @@ -1382,7 +1367,7 @@ void RPiCameraData::tryRunPipeline()
> * current bayer buffer will be removed and re-queued to the driver.
> */
> embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> - unicam_[Unicam::Embedded].dev());
> + &unicam_[Unicam::Embedded]);
>
> if (!embeddedBuffer) {
> LOG(RPI, Debug) << "Could not find matching embedded buffer";
> @@ -1401,7 +1386,7 @@ void RPiCameraData::tryRunPipeline()
>
> embeddedBuffer = embeddedQueue_.front();
> bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> - unicam_[Unicam::Image].dev());
> + &unicam_[Unicam::Image]);
>
> if (!bayerBuffer) {
> LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> @@ -1409,11 +1394,7 @@ void RPiCameraData::tryRunPipeline()
> }
> }
>
> - /*
> - * Take the first request from the queue and action the IPA.
> - * Unicam buffers for the request have already been queued as they come
> - * in.
> - */
> + /* Take the first request from the queue and action the IPA. */
> Request *request = requestQueue_.front();
>
> /*
> @@ -1425,12 +1406,6 @@ void RPiCameraData::tryRunPipeline()
> op.controls = { request->controls() };
> ipa_->processEvent(op);
>
> - /* Queue up any ISP buffers passed into the request. */
> - for (auto &stream : isp_) {
> - if (stream.isExternal())
> - stream.dev()->queueBuffer(request->findBuffer(&stream));
> - }
> -
> /* Ready to use the buffers, pop them off the queue. */
> bayerQueue_.pop();
> embeddedQueue_.pop();
> @@ -1460,32 +1435,42 @@ void RPiCameraData::tryFlushQueues()
> * and give a chance for the hardware to return to lock-step. We do have
> * to drop all interim frames.
> */
> - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> + /* This cannot happen when Unicam streams are external. */
> + assert(!unicam_[Unicam::Image].isExternal());
> +
> LOG(RPI, Warning) << "Flushing all buffer queues!";
>
> while (!bayerQueue_.empty()) {
> - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> bayerQueue_.pop();
> }
>
> while (!embeddedQueue_.empty()) {
> - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> embeddedQueue_.pop();
> }
> }
> }
>
> FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> - V4L2VideoDevice *dev)
> + RPi::RPiStream *stream)
> {
> + /*
> + * If the unicam streams are external (both have to the same), then we
> + * can only return out the top buffer in the queue, and assume they have
> + * been synced by queuing at the same time. We cannot drop these frames,
> + * as they may have been provided externally.
> + */
> while (!q.empty()) {
> FrameBuffer *b = q.front();
> - if (b->metadata().timestamp < timestamp) {
> + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> q.pop();
> - dev->queueBuffer(b);
> - LOG(RPI, Warning) << "Dropping input frame!";
> - } else if (b->metadata().timestamp == timestamp) {
> + stream->queueBuffer(b);
> + LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> + << stream->name();
> + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> /* The calling function will pop the item from the queue. */
> return b;
> } else {
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index ed4f4987..a6f573fa 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -44,30 +44,20 @@ public:
>
> void setExternal(bool external)
> {
> + /* Import streams cannot be external. */
> + assert(!external || !importOnly_);
> external_ = external;
> }
>
> bool isExternal() const
> {
> - /*
> - * Import streams cannot be external.
> - *
> - * RAW capture is a special case where we simply copy the RAW
> - * buffer out of the request. All other buffer handling happens
> - * as if the stream is internal.
> - */
> - return external_ && !importOnly_;
> - }
> -
> - bool isImporter() const
> - {
> - return importOnly_;
> + return external_;
> }
>
> void reset()
> {
> external_ = false;
> - internalBuffers_.clear();
> + clearBuffers();
> }
>
> std::string name() const
> @@ -77,67 +67,124 @@ public:
>
> void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> {
> - externalBuffers_ = buffers;
> + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> + [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> }
>
> - const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const
> + const std::vector<FrameBuffer *> &getBuffers() const
> {
> - return external_ ? externalBuffers_ : &internalBuffers_;
> + return bufferList_;
> }
>
> void releaseBuffers()
> {
> dev_->releaseBuffers();
> - if (!external_ && !importOnly_)
> - internalBuffers_.clear();
> + clearBuffers();
> }
>
> - int importBuffers(unsigned int count)
> + int prepareBuffers(unsigned int count)
> {
> + int ret;
> +
> + if (!importOnly_) {
> + if (count) {
> + /* Export some frame buffers for internal use. */
> + ret = dev_->exportBuffers(count, &internalBuffers_);
> + if (ret < 0)
> + return ret;
> +
> + /* Add these exported buffers to the internal/external buffer list. */
> + std::transform(internalBuffers_.begin(), internalBuffers_.end(),
> + std::back_inserter(bufferList_),
> + [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> +
> + /* Add these buffers to the queue of internal usable buffers. */
> + for (auto const &buffer : internalBuffers_)
> + availableBuffers_.push(buffer.get());
> + }
> +
> + /* We must import all internal/external exported buffers. */
> + count = bufferList_.size();
> + }
> +
> return dev_->importBuffers(count);
> }
>
> - int allocateBuffers(unsigned int count)
> + int queueAllBuffers()
> {
> - return dev_->allocateBuffers(count, &internalBuffers_);
> - }
> + int ret;
>
> - int queueBuffers()
> - {
> if (external_)
> return 0;
>
> - for (auto &b : internalBuffers_) {
> - int ret = dev_->queueBuffer(b.get());
> - if (ret) {
> - LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> - << name_;
> + while (!availableBuffers_.empty()) {
> + ret = queueBuffer(availableBuffers_.front());
> + if (ret < 0)
> return ret;
> - }
> +
> + availableBuffers_.pop();
> }
>
> return 0;
> }
>
> - bool findFrameBuffer(FrameBuffer *buffer) const
> + int queueBuffer(FrameBuffer *buffer)
> {
> - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> + /*
> + * A nullptr buffer implies an external stream, but no external
> + * buffer has been supplied. So, pick one from the availableBuffers_
> + * queue.
> + */
> + if (!buffer) {
> + if (availableBuffers_.empty()) {
> + LOG(RPISTREAM, Warning) << "No buffers available for "
> + << name_;
> + return -EINVAL;
> + }
>
> + buffer = availableBuffers_.front();
> + availableBuffers_.pop();
> + }
> +
> + LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> + << " for " << name_;
> +
> + int ret = dev_->queueBuffer(buffer);
> + if (ret) {
> + LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> + << name_;
> + }
> +
> + return ret;
> + }
> +
> + void returnBuffer(FrameBuffer *buffer)
> + {
> + availableBuffers_.push(buffer);
> + }
> +
> + bool findFrameBuffer(FrameBuffer *buffer) const
> + {
> if (importOnly_)
> return false;
>
> - if (std::find_if(start, end,
> - [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> return true;
>
> return false;
> }
>
> private:
> + void clearBuffers()
> + {
> + availableBuffers_ = std::queue<FrameBuffer *>{};
> + internalBuffers_.clear();
> + bufferList_.clear();
> + }
> +
> /*
> * Indicates that this stream is active externally, i.e. the buffers
> - * are provided by the application.
> + * might be provided by (and returned to) the application.
> */
> bool external_;
> /* Indicates that this stream only imports buffers, e.g. ISP input. */
> @@ -146,10 +193,19 @@ private:
> std::string name_;
> /* The actual device stream. */
> std::unique_ptr<V4L2VideoDevice> dev_;
> - /* Internally allocated framebuffers associated with this device stream. */
> + /* All framebuffers associated with this device stream. */
> + std::vector<FrameBuffer *> bufferList_;
> + /*
> + * List of frame buffer that we can use if none have been provided by
> + * the application for external streams. This is populated by the
> + * buffers exported internally.
> + */
> + std::queue<FrameBuffer *> availableBuffers_;
> + /*
> + * This is a list of buffers exported internally. Need to keep this around
> + * as the stream needs to maintain ownership of these buffers.
> + */
> std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> - /* Externally allocated framebuffers associated with this device stream. */
> - std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> };
>
> /*
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list