[PATCH v2 3/4] libcamera: ipu3: Replace IPU3FrameInfo
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Mar 12 12:27:27 CET 2024
Quoting Jacopo Mondi (2024-03-11 12:32:31)
> Now that the pipeline handler can create a private derived class of
> Request::Private, use it to store the pipeline specific data.
>
> In the case of IPU3 we associate statistics and paramters buffer with a
> Request and a raw buffer which gets dequeued from the CIO2 and input
> to the ImgU input.
>
> This replaces the functionalities of IPU3FrameInfo which can now be
> removed.
The only comments I have here are syntactic sugar that could make using
the pipeline specific request easier but maybe that might just obfuscate
things anyway - so I still think this is a good way forwards.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
> src/libcamera/pipeline/ipu3/frames.cpp | 143 ----------------
> src/libcamera/pipeline/ipu3/frames.h | 67 --------
> src/libcamera/pipeline/ipu3/ipu3.cpp | 212 +++++++++++++++---------
> src/libcamera/pipeline/ipu3/meson.build | 1 -
> 4 files changed, 137 insertions(+), 286 deletions(-)
> delete mode 100644 src/libcamera/pipeline/ipu3/frames.cpp
> delete mode 100644 src/libcamera/pipeline/ipu3/frames.h
>
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
> deleted file mode 100644
> index a4c3477cd9ef..000000000000
> --- a/src/libcamera/pipeline/ipu3/frames.cpp
> +++ /dev/null
> @@ -1,143 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * frames.cpp - Intel IPU3 Frames helper
> - */
> -
> -#include "frames.h"
> -
> -#include <libcamera/framebuffer.h>
> -#include <libcamera/request.h>
> -
> -#include "libcamera/internal/framebuffer.h"
> -#include "libcamera/internal/pipeline_handler.h"
> -#include "libcamera/internal/v4l2_videodevice.h"
> -
> -namespace libcamera {
> -
> -LOG_DECLARE_CATEGORY(IPU3)
> -
> -IPU3Frames::IPU3Frames()
> -{
> -}
> -
> -void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers,
> - const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)
> -{
> - for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers)
> - availableParamBuffers_.push(buffer.get());
> -
> - for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers)
> - availableStatBuffers_.push(buffer.get());
> -
> - frameInfo_.clear();
> -}
> -
> -void IPU3Frames::clear()
> -{
> - availableParamBuffers_ = {};
> - availableStatBuffers_ = {};
> -}
> -
> -IPU3Frames::Info *IPU3Frames::create(Request *request)
> -{
> - unsigned int id = request->sequence();
> -
> - if (availableParamBuffers_.empty()) {
> - LOG(IPU3, Debug) << "Parameters buffer underrun";
> - return nullptr;
> - }
> -
> - if (availableStatBuffers_.empty()) {
> - LOG(IPU3, Debug) << "Statistics buffer underrun";
> - return nullptr;
> - }
> -
> - FrameBuffer *paramBuffer = availableParamBuffers_.front();
> - FrameBuffer *statBuffer = availableStatBuffers_.front();
> -
> - paramBuffer->_d()->setRequest(request);
> - statBuffer->_d()->setRequest(request);
> -
> - availableParamBuffers_.pop();
> - availableStatBuffers_.pop();
> -
> - /* \todo Remove the dynamic allocation of Info */
> - std::unique_ptr<Info> info = std::make_unique<Info>();
> -
> - info->id = id;
> - info->request = request;
> - info->rawBuffer = nullptr;
> - info->paramBuffer = paramBuffer;
> - info->statBuffer = statBuffer;
> - info->paramDequeued = false;
> - info->metadataProcessed = false;
> -
> - frameInfo_[id] = std::move(info);
> -
> - return frameInfo_[id].get();
> -}
> -
> -void IPU3Frames::remove(IPU3Frames::Info *info)
> -{
> - /* Return params and stat buffer for reuse. */
> - availableParamBuffers_.push(info->paramBuffer);
> - availableStatBuffers_.push(info->statBuffer);
> -
> - /* Delete the extended frame information. */
> - frameInfo_.erase(info->id);
> -}
> -
> -bool IPU3Frames::tryComplete(IPU3Frames::Info *info)
> -{
> - Request *request = info->request;
> -
> - if (request->hasPendingBuffers())
> - return false;
> -
> - if (!info->metadataProcessed)
> - return false;
> -
> - if (!info->paramDequeued)
> - return false;
> -
> - remove(info);
> -
> - bufferAvailable.emit();
> -
> - return true;
> -}
> -
> -IPU3Frames::Info *IPU3Frames::find(unsigned int id)
> -{
> - const auto &itInfo = frameInfo_.find(id);
> -
> - if (itInfo != frameInfo_.end())
> - return itInfo->second.get();
> -
> - LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id;
> -
> - return nullptr;
> -}
> -
> -IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)
> -{
> - for (auto const &itInfo : frameInfo_) {
> - Info *info = itInfo.second.get();
> -
> - for (auto const itBuffers : info->request->buffers())
> - if (itBuffers.second == buffer)
> - return info;
> -
> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||
> - info->statBuffer == buffer)
> - return info;
> - }
> -
> - LOG(IPU3, Fatal) << "Can't find tracking information from buffer";
> -
> - return nullptr;
> -}
> -
> -} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h
> deleted file mode 100644
> index 6e3cb915c7b8..000000000000
> --- a/src/libcamera/pipeline/ipu3/frames.h
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * frames.h - Intel IPU3 Frames helper
> - */
> -
> -#pragma once
> -
> -#include <map>
> -#include <memory>
> -#include <queue>
> -#include <vector>
> -
> -#include <libcamera/base/signal.h>
> -
> -#include <libcamera/controls.h>
> -
> -namespace libcamera {
> -
> -class FrameBuffer;
> -class IPAProxy;
> -class PipelineHandler;
> -class Request;
> -class V4L2VideoDevice;
> -struct IPABuffer;
> -
> -class IPU3Frames
> -{
> -public:
> - struct Info {
> - unsigned int id;
> - Request *request;
> -
> - FrameBuffer *rawBuffer;
> - FrameBuffer *paramBuffer;
> - FrameBuffer *statBuffer;
> -
> - ControlList effectiveSensorControls;
> -
> - bool paramDequeued;
> - bool metadataProcessed;
> - };
> -
> - IPU3Frames();
> -
> - void init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers,
> - const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);
> - void clear();
> -
> - Info *create(Request *request);
> - void remove(Info *info);
> - bool tryComplete(Info *info);
> -
> - Info *find(unsigned int id);
> - Info *find(FrameBuffer *buffer);
> -
> - Signal<> bufferAvailable;
> -
> -private:
> - std::queue<FrameBuffer *> availableParamBuffers_;
> - std::queue<FrameBuffer *> availableStatBuffers_;
> -
> - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;
> -};
> -
> -} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index fa4bd0bb73e2..0c9d3167d2e6 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -34,9 +34,9 @@
> #include "libcamera/internal/ipa_manager.h"
> #include "libcamera/internal/media_device.h"
> #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/request.h"
>
> #include "cio2.h"
> -#include "frames.h"
> #include "imgu.h"
>
> namespace libcamera {
> @@ -47,6 +47,24 @@ static const ControlInfoMap::Map IPU3Controls = {
> { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> };
>
> +class IPU3Request : public Request::Private
> +{
> +public:
> + IPU3Request(Camera *camera)
> + : Request::Private(camera)
> + {
> + }
> +
> + FrameBuffer *rawBuffer;
> + FrameBuffer *paramBuffer;
> + FrameBuffer *statBuffer;
> +
> + ControlList effectiveSensorControls;
> +
> + bool paramDequeued;
> + bool metadataProcessed;
> +};
> +
> class IPU3CameraData : public Camera::Private
> {
> public:
> @@ -57,6 +75,7 @@ public:
>
> int loadIPA();
>
> + void tryCompleteRequest(IPU3Request *request);
> void imguOutputBufferReady(FrameBuffer *buffer);
> void cio2BufferReady(FrameBuffer *buffer);
> void paramBufferReady(FrameBuffer *buffer);
> @@ -75,7 +94,6 @@ public:
> Rectangle cropRegion_;
>
> std::unique_ptr<DelayedControls> delayedCtrls_;
> - IPU3Frames frameInfos_;
>
> std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
>
> @@ -86,7 +104,17 @@ public:
>
> ControlInfoMap ipaControls_;
>
> + std::map<unsigned int, IPU3Request *> requestMap_;
> +
> + std::queue<FrameBuffer *> availableParamBuffers_;
> + std::queue<FrameBuffer *> availableStatBuffers_;
> +
> private:
> + IPU3Request *cameraRequest(Request *request)
> + {
> + return static_cast<IPU3Request *>(request->_d());
> + }
> +
> void metadataReady(unsigned int id, const ControlList &metadata);
> void paramsBufferReady(unsigned int id);
> void setSensorControls(unsigned int id, const ControlList &sensorControls,
> @@ -144,6 +172,8 @@ public:
> int start(Camera *camera, const ControlList *controls) override;
> void stopDevice(Camera *camera) override;
>
> + std::unique_ptr<Request> createRequestDevice(Camera *camera,
> + uint64_t cookie) override;
> int queueRequestDevice(Camera *camera, Request *request) override;
>
> bool match(DeviceEnumerator *enumerator) override;
> @@ -680,19 +710,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> buffer->setCookie(ipaBufferId++);
> ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> + data->availableParamBuffers_.push(buffer.get());
> }
>
> for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
> buffer->setCookie(ipaBufferId++);
> ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> + data->availableStatBuffers_.push(buffer.get());
> }
>
> data->ipa_->mapBuffers(ipaBuffers_);
>
> - data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);
> - data->frameInfos_.bufferAvailable.connect(
> - data, &IPU3CameraData::queuePendingRequests);
> -
> return 0;
> }
>
> @@ -700,8 +728,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
>
> - data->frameInfos_.clear();
> -
> std::vector<unsigned int> ids;
> for (IPABuffer &ipabuf : ipaBuffers_)
> ids.push_back(ipabuf.id);
> @@ -777,6 +803,7 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)
> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>
> freeBuffers(camera);
> + data->requestMap_.clear();
> }
>
> void IPU3CameraData::cancelPendingRequests()
> @@ -801,37 +828,76 @@ void IPU3CameraData::queuePendingRequests()
> {
> while (!pendingRequests_.empty()) {
> Request *request = pendingRequests_.front();
> + IPU3Request *ipu3Request = cameraRequest(request);
>
> - IPU3Frames::Info *info = frameInfos_.create(request);
> - if (!info)
> - break;
> + requestMap_[request->sequence()] = ipu3Request;
>
> /*
> * Queue a buffer on the CIO2, using the raw stream buffer
> * provided in the request, if any, or a CIO2 internal buffer
> * otherwise.
> + *
> + * No need to set a buffer->setRequest() as
> + * a) If the buffer is provided by the application that's
> + * already been done
> + * b) If the buffer comes from the CIO2 internal pool,
> + * CIO2Device::queueBuffer() does that for us
> */
> FrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);
> FrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);
> +
> /*
> * \todo If queueBuffer fails in queuing a buffer to the device,
> * report the request as error by cancelling the request and
> * calling PipelineHandler::completeRequest().
> */
> - if (!rawBuffer) {
> - frameInfos_.remove(info);
> + if (!rawBuffer)
> + break;
> +
> + /*
> + * Store the raw buffer queued to the CIO2 to queue it to the
> + * Imgu input when the IPA has prepared the parameters buffer.
> + */
> + ipu3Request->rawBuffer = rawBuffer;
> +
> + /*
> + * Prepare the stats and parameters buffer and associate them
> + * with the currently queued request.
> + */
> + if (availableParamBuffers_.empty()) {
> + LOG(IPU3, Warning) << "Parameters buffer underrun";
> break;
> }
>
> - info->rawBuffer = rawBuffer;
> + if (availableStatBuffers_.empty()) {
> + LOG(IPU3, Warning) << "Statistics buffer underrun";
> + break;
> + }
>
> - ipa_->queueRequest(info->id, request->controls());
> + ipu3Request->paramBuffer = availableParamBuffers_.front();
> + ipu3Request->paramDequeued = false;
> + ipu3Request->paramBuffer->_d()->setRequest(request);
> + availableParamBuffers_.pop();
> +
> + ipu3Request->statBuffer = availableStatBuffers_.front();
> + ipu3Request->metadataProcessed = false;
> + ipu3Request->statBuffer->_d()->setRequest(request);
> + availableStatBuffers_.pop();
> +
> + ipa_->queueRequest(request->sequence(), request->controls());
>
> pendingRequests_.pop();
> processingRequests_.push(request);
> }
> }
>
> +std::unique_ptr<Request> PipelineHandlerIPU3::createRequestDevice(Camera *camera,
> + uint64_t cookie)
> +{
> + auto request = std::make_unique<IPU3Request>(camera);
> + return Request::create(std::move(request), cookie);
> +}
> +
> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> {
> IPU3CameraData *data = cameraData(camera);
> @@ -1200,6 +1266,26 @@ int IPU3CameraData::loadIPA()
> return 0;
> }
>
> +void IPU3CameraData::tryCompleteRequest(IPU3Request *request)
> +{
> + if (request->hasPendingBuffers())
> + return;
> +
> + if (!request->metadataProcessed)
> + return;
> +
> + if (!request->paramDequeued)
> + return;
> +
> + availableParamBuffers_.push(request->paramBuffer);
> + availableStatBuffers_.push(request->statBuffer);
> +
> + pipe()->completeRequest(request->_o<Request>());
> +
> + /* Try queue another request now that this one has completed. */
> + queuePendingRequests();
> +}
> +
> void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,
> const ControlList &sensorControls,
> const ControlList &lensControls)
> @@ -1220,12 +1306,10 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,
>
> void IPU3CameraData::paramsBufferReady(unsigned int id)
> {
> - IPU3Frames::Info *info = frameInfos_.find(id);
> - if (!info)
> - return;
> + IPU3Request *request = requestMap_[id];
>
> /* Queue all buffers from the request aimed for the ImgU. */
> - for (auto it : info->request->buffers()) {
> + for (auto it : request->_o<Request>()->buffers()) {
> const Stream *stream = it.first;
> FrameBuffer *outbuffer = it.second;
>
> @@ -1235,25 +1319,21 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)
> imgu_->viewfinder_->queueBuffer(outbuffer);
> }
>
> - info->paramBuffer->_d()->metadata().planes()[0].bytesused =
> + request->paramBuffer->_d()->metadata().planes()[0].bytesused =
> sizeof(struct ipu3_uapi_params);
> - imgu_->param_->queueBuffer(info->paramBuffer);
> - imgu_->stat_->queueBuffer(info->statBuffer);
> - imgu_->input_->queueBuffer(info->rawBuffer);
> + imgu_->param_->queueBuffer(request->paramBuffer);
> + imgu_->stat_->queueBuffer(request->statBuffer);
> + imgu_->input_->queueBuffer(request->rawBuffer);
> }
>
> void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)
> {
> - IPU3Frames::Info *info = frameInfos_.find(id);
> - if (!info)
> - return;
> + IPU3Request *request = requestMap_[id];
>
> - Request *request = info->request;
> - request->metadata().merge(metadata);
> + request->_o<Request>()->metadata().merge(metadata);
>
> - info->metadataProcessed = true;
> - if (frameInfos_.tryComplete(info))
> - pipe()->completeRequest(request);
> + request->metadataProcessed = true;
> + tryCompleteRequest(request);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -1268,11 +1348,7 @@ void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)
> */
> void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> {
> - IPU3Frames::Info *info = frameInfos_.find(buffer);
> - if (!info)
> - return;
> -
> - Request *request = info->request;
> + Request *request = buffer->request();
>
> pipe()->completeBuffer(request, buffer);
>
> @@ -1283,8 +1359,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> cropRegion_ = *scalerCrop;
> request->metadata().set(controls::ScalerCrop, cropRegion_);
>
> - if (frameInfos_.tryComplete(info))
> - pipe()->completeRequest(request);
> + tryCompleteRequest(cameraRequest(request));
> }
>
> /**
> @@ -1296,22 +1371,20 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> */
> void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> {
> - IPU3Frames::Info *info = frameInfos_.find(buffer);
> - if (!info)
> - return;
> -
> - Request *request = info->request;
> + IPU3Request *request = cameraRequest(buffer->request());
>
> /* If the buffer is cancelled force a complete of the whole request. */
> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> - for (auto it : request->buffers()) {
> + for (auto it : request->_o<Request>()->buffers()) {
> FrameBuffer *b = it.second;
> b->_d()->cancel();
> - pipe()->completeBuffer(request, b);
> + pipe()->completeBuffer(request->_o<Request>(), b);
> }
>
> - frameInfos_.remove(info);
> - pipe()->completeRequest(request);
> + availableParamBuffers_.push(request->paramBuffer);
> + availableStatBuffers_.push(request->statBuffer);
> +
> + pipe()->completeRequest(request->_o<Request>());
> return;
> }
>
> @@ -1321,24 +1394,23 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> * \todo The sensor timestamp should be better estimated by connecting
> * to the V4L2Device::frameStart signal.
> */
> - request->metadata().set(controls::SensorTimestamp,
> - buffer->metadata().timestamp);
> + request->_o<Request>()->metadata().set(controls::SensorTimestamp,
I still think syntatic sugar could be improved here, but that can easily
be for later.
> + buffer->metadata().timestamp);
>
> - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);
> + request->effectiveSensorControls =
> + delayedCtrls_->get(buffer->metadata().sequence);
>
> - if (request->findBuffer(&rawStream_))
> - pipe()->completeBuffer(request, buffer);
> + if (request->_o<Request>()->findBuffer(&rawStream_))
> + pipe()->completeBuffer(request->_o<Request>(), buffer);
>
> - ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());
> + ipa_->fillParamsBuffer(request->_o<Request>()->sequence(),
> + request->paramBuffer->cookie());
> }
>
> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> {
> - IPU3Frames::Info *info = frameInfos_.find(buffer);
> - if (!info)
> - return;
> -
> - info->paramDequeued = true;
> + IPU3Request *request = cameraRequest(buffer->request());
> + request->paramDequeued = true;
>
> /*
> * tryComplete() will delete info if it completes the IPU3Frame.
> @@ -1346,35 +1418,25 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> *
> * \todo Improve the FrameInfo API to avoid this type of issue
> */
> - Request *request = info->request;
>
> - if (frameInfos_.tryComplete(info))
> - pipe()->completeRequest(request);
> + tryCompleteRequest(request);
> }
>
> void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> {
> - IPU3Frames::Info *info = frameInfos_.find(buffer);
> - if (!info)
> - return;
> -
> - Request *request = info->request;
> + IPU3Request *request = cameraRequest(buffer->request());
>
> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> - info->metadataProcessed = true;
> + request->metadataProcessed = true;
>
> - /*
> - * tryComplete() will delete info if it completes the IPU3Frame.
> - * In that event, we must have obtained the Request before hand.
> - */
> - if (frameInfos_.tryComplete(info))
> - pipe()->completeRequest(request);
> + tryCompleteRequest(request);
>
> return;
> }
>
> - ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),
> - info->statBuffer->cookie(), info->effectiveSensorControls);
> + ipa_->processStatsBuffer(request->_o<Request>()->sequence(),
> + request->_o<Request>()->metadata().get(controls::SensorTimestamp).value_or(0),
> + request->statBuffer->cookie(), request->effectiveSensorControls);
> }
>
> /*
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> index a1b0b31ac5bc..d60e07ae6cca 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -2,7 +2,6 @@
>
> libcamera_sources += files([
> 'cio2.cpp',
> - 'frames.cpp',
> 'imgu.cpp',
> 'ipu3.cpp',
> ])
> --
> 2.43.2
>
More information about the libcamera-devel
mailing list