[PATCH v3 4/4] libcamera: rkisp1: Plumb the ConverterDW100 converter
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 26 09:16:49 CEST 2024
Quoting Umang Jain (2024-06-26 04:46:58)
> Hi Kieran,
>
> On 26/06/24 2:34 am, Kieran Bingham wrote:
> > Quoting Umang Jain (2024-06-25 07:23:27)
> >> Plumb the ConverterDW100 converter in the rkisp1 pipeline handler.
> >> If the dewarper is found, it is instantiated and buffers are exported
> >> from it, instead of RkISP1Path. Internal buffers are allocated for the
> >> RkISP1Path in case where dewarper is going to be used.
> >>
> >> The RKISP1 pipeline handler now supports scaler crop control through
> >> ConverterDW100. Register the ScalerCrop control for the cameras created
> >> in the RKISP1 pipeline handler.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 143 +++++++++++++++++-
> >> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-
> >> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++
> >> 3 files changed, 161 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >> index bfc44239..b1314d73 100644
> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >> @@ -8,9 +8,11 @@
> >> #include <algorithm>
> >> #include <array>
> >> #include <iomanip>
> >> +#include <map>
> >> #include <memory>
> >> #include <numeric>
> >> #include <queue>
> >> +#include <vector>
> >>
> >> #include <linux/media-bus-format.h>
> >> #include <linux/rkisp1-config.h>
> >> @@ -33,6 +35,7 @@
> >>
> >> #include "libcamera/internal/camera.h"
> >> #include "libcamera/internal/camera_sensor.h"
> >> +#include "libcamera/internal/converter/converter_dw100.h"
> >> #include "libcamera/internal/delayed_controls.h"
> >> #include "libcamera/internal/device_enumerator.h"
> >> #include "libcamera/internal/framebuffer.h"
> >> @@ -62,6 +65,8 @@ struct RkISP1FrameInfo {
> >>
> >> bool paramDequeued;
> >> bool metadataProcessed;
> >> +
> >> + std::optional<Rectangle> scalerCrop;
> >> };
> >>
> >> class RkISP1Frames
> >> @@ -165,6 +170,8 @@ public:
> >>
> >> private:
> >> static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
> >> + static constexpr Size kMinDewarpSize = { 176, 144 };
> >> + static constexpr Size kMaxDewarpSize = { 4096, 3072 };
> > Shouldn't these be properties of the m2m dewarp class somehow?
> >
> >>
> >> RkISP1CameraData *cameraData(Camera *camera)
> >> {
> >> @@ -181,6 +188,7 @@ private:
> >> void bufferReady(FrameBuffer *buffer);
> >> void paramReady(FrameBuffer *buffer);
> >> void statReady(FrameBuffer *buffer);
> >> + void dewarpBufferReady(FrameBuffer *buffer);
> >> void frameStart(uint32_t sequence);
> >>
> >> int allocateBuffers(Camera *camera);
> >> @@ -200,6 +208,12 @@ private:
> >> RkISP1MainPath mainPath_;
> >> RkISP1SelfPath selfPath_;
> >>
> >> + std::unique_ptr<ConverterDW100> dewarper_;
> >> +
> >> + /* Internal buffers used when dewarper is being used */
> >> + std::vector<std::unique_ptr<FrameBuffer>> mainPathBuffers_;
> >> + std::queue<FrameBuffer *> availableMainPathBuffers_;
> >> +
> >> std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
> >> std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
> >> std::queue<FrameBuffer *> availableParamBuffers_;
> >> @@ -222,6 +236,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >>
> >> FrameBuffer *paramBuffer = nullptr;
> >> FrameBuffer *statBuffer = nullptr;
> >> + FrameBuffer *mainPathBuffer = nullptr;
> >> + FrameBuffer *selfPathBuffer = nullptr;
> >>
> >> if (!isRaw) {
> >> if (pipe_->availableParamBuffers_.empty()) {
> >> @@ -239,10 +255,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >>
> >> statBuffer = pipe_->availableStatBuffers_.front();
> >> pipe_->availableStatBuffers_.pop();
> >> +
> >> + if (pipe_->dewarper_) {
> >> + mainPathBuffer = pipe_->availableMainPathBuffers_.front();
> >> + pipe_->availableMainPathBuffers_.pop();
> >> + }
> >> }
> >>
> >> - FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> >> - FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
> >> + if (!mainPathBuffer)
> >> + mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> >> + selfPathBuffer = request->findBuffer(&data->selfPathStream_);
> >>
> >> RkISP1FrameInfo *info = new RkISP1FrameInfo;
> >>
> >> @@ -268,6 +290,7 @@ int RkISP1Frames::destroy(unsigned int frame)
> >>
> >> pipe_->availableParamBuffers_.push(info->paramBuffer);
> >> pipe_->availableStatBuffers_.push(info->statBuffer);
> >> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
> >>
> >> frameInfo_.erase(info->frame);
> >>
> >> @@ -283,6 +306,7 @@ void RkISP1Frames::clear()
> >>
> >> pipe_->availableParamBuffers_.push(info->paramBuffer);
> >> pipe_->availableStatBuffers_.push(info->statBuffer);
> >> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
> >>
> >> delete info;
> >> }
> >> @@ -785,12 +809,26 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >> << " crop " << rect;
> >>
> >> std::map<unsigned int, IPAStream> streamConfig;
> >> + std::vector<std::reference_wrapper<StreamConfiguration>> outputCfgs;
> >>
> >> for (const StreamConfiguration &cfg : *config) {
> >> if (cfg.stream() == &data->mainPathStream_) {
> >> ret = mainPath_.configure(cfg, format);
> >> streamConfig[0] = IPAStream(cfg.pixelFormat,
> >> cfg.size);
> >> + /* Configure dewarp */
> >> + if (dewarper_ && !isRaw_) {
> >> + /*
> >> + * \todo Converter::configure() API should be changed
> >> + * to use std::vector<std::reference_wrapper<const StreamConfiguration>> ?
> >> + */
> >> + outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg));
> >> + dewarper_->configure(cfg, outputCfgs);
> >> + } else if (dewarper_ && isRaw_) {
> >> + LOG(RkISP1, Debug) << "Dewarper disabled on RAW configuration";
> >> + dewarper_.reset();
> > Does this remove the dewarper if someone asks for a raw stream, and mean
> > it could no longer be available if they then switch back to ask for a
> > processed stream?
> >
> > I think so ... and fixing that will require reworking quite a bit of the
> > patch below so I'll hold off here.
> >
> > I don't think the 'presence' of dewarper_ can be used to fully determine
> > if we use it or not - that' based on the configuration. And we shouldn't
> > allocate unless we're using it - and we shouldn't destroy dewarper_ if
> > we don't use if for just a single streaming session.
>
> You are correct, we shouldn't be resetting the dewarper_, if not in use.
> It should be dependent on the configuration
>
> The raw configuration is tracked by isRaw_ member variable in the
> pipeline, so probably I'll replace
>
> if (dewarper_)
> ...
>
> with
>
> if (dewarper_ && !isRaw_)
> .,..
It depends on how you implement it - but a specific flag might also be
better for
if (useDewarper) {}
or
if (dewarperEnabled) { }
to avoid having to reference the heuristic of when it's in use in many
places.
or a class helper that checks those existing flags?
if (useDewarper()) { }
Which would potentially avoid having to keep an extra flag syncrhonised.
Anyway, I'll leave that all up to you - just ideas when I read
"if (dewarper_ && !isRaw_)"
--
Kieran
> at most places - in the next version, without destroying the dewarper_.
> >
> > --
> > Kieran
> >
> >
> >> + }
> >> +
> >> } else if (hasSelfPath_) {
> >> ret = selfPath_.configure(cfg, format);
> >> streamConfig[1] = IPAStream(cfg.pixelFormat,
> >> @@ -839,6 +877,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
> >> RkISP1CameraData *data = cameraData(camera);
> >> unsigned int count = stream->configuration().bufferCount;
> >>
> >> + if (!isRaw_ && dewarper_)
> >> + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);
> >> +
> >> if (stream == &data->mainPathStream_)
> >> return mainPath_.exportBuffers(count, buffers);
> >> else if (hasSelfPath_ && stream == &data->selfPathStream_)
> >> @@ -866,6 +907,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> >> ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> >> if (ret < 0)
> >> goto error;
> >> +
> >> + /* If the dewarper is being used, allocate internal buffers for ISP */
> >> + if (dewarper_) {
> >> + ret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);
> >> + if (ret < 0)
> >> + goto error;
> >> +
> >> + for (std::unique_ptr<FrameBuffer> &buffer : mainPathBuffers_)
> >> + availableMainPathBuffers_.push(buffer.get());
> >> + }
> >> }
> >>
> >> for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> >> @@ -889,6 +940,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> >> error:
> >> paramBuffers_.clear();
> >> statBuffers_.clear();
> >> + mainPathBuffers_.clear();
> >>
> >> return ret;
> >> }
> >> @@ -903,8 +955,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> >> while (!availableParamBuffers_.empty())
> >> availableParamBuffers_.pop();
> >>
> >> + while (!availableMainPathBuffers_.empty())
> >> + availableMainPathBuffers_.pop();
> >> +
> >> paramBuffers_.clear();
> >> statBuffers_.clear();
> >> + mainPathBuffers_.clear();
> >>
> >> std::vector<unsigned int> ids;
> >> for (IPABuffer &ipabuf : data->ipaBuffers_)
> >> @@ -919,6 +975,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> >> if (stat_->releaseBuffers())
> >> LOG(RkISP1, Error) << "Failed to release stat buffers";
> >>
> >> + if (dewarper_)
> >> + mainPath_.releaseBuffers();
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -961,6 +1020,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> >> << "Failed to start statistics " << camera->id();
> >> return ret;
> >> }
> >> +
> >> + if (dewarper_) {
> >> + ret = dewarper_->start();
> >> + if (ret) {
> >> + LOG(RkISP1, Error) << "Failed to start dewarper";
> >> + return ret;
> >> + }
> >> + }
> >> }
> >>
> >> if (data->mainPath_->isEnabled()) {
> >> @@ -1015,6 +1082,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
> >> if (ret)
> >> LOG(RkISP1, Warning)
> >> << "Failed to stop parameters for " << camera->id();
> >> +
> >> + if (dewarper_)
> >> + dewarper_->stop();
> >> }
> >>
> >> ASSERT(data->queuedRequests_.empty());
> >> @@ -1045,6 +1115,13 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >> info->paramBuffer->cookie());
> >> }
> >>
> >> + const auto &crop = request->controls().get(controls::ScalerCrop);
> >> + if (crop && !isRaw_) {
> >> + int ret = dewarper_->setScalerCrop(&data->mainPathStream_, crop.value());
> >> + if (!ret)
> >> + info->scalerCrop = crop;
> >> + }
> >> +
> >> data->frame_++;
> >>
> >> return 0;
> >> @@ -1110,6 +1187,13 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> >> {
> >> ControlInfoMap::Map rkisp1Controls;
> >>
> >> + if (dewarper_) {
> >> + Rectangle maxCrop(kMaxDewarpSize);
> >> + Rectangle minCrop = kMinDewarpSize.centeredTo(maxCrop.center());
> >> +
> >> + rkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
> >> + }
> >> +
> >> /* Add the IPA registered controls to list of camera controls. */
> >> for (const auto &ipaControl : data->ipaControls_)
> >> rkisp1Controls[ipaControl.first] = ipaControl.second;
> >> @@ -1173,6 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >>
> >> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >> {
> >> + std::shared_ptr<MediaDevice> dwpMediaDevice;
> >> const MediaPad *pad;
> >>
> >> DeviceMatch dm("rkisp1");
> >> @@ -1237,6 +1322,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> >> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
> >>
> >> + /* If dewarper is present, create its instance. */
> >> + DeviceMatch dwp("dw100");
> >> + dwp.add("dw100-source");
> >> + dwp.add("dw100-sink");
> >> +
> >> + dwpMediaDevice = enumerator->search(dwp);
> >> + if (dwpMediaDevice) {
> >> + dewarper_ = std::make_unique<ConverterDW100>(std::move(dwpMediaDevice));
> >> + if (dewarper_->isValid()) {
> >> + dewarper_->outputBufferReady.connect(
> >> + this, &PipelineHandlerRkISP1::dewarpBufferReady);
> >> +
> >> + LOG(RkISP1, Info) << "Using DW100 dewarper " << dewarper_->deviceNode();
> >> + } else {
> >> + LOG(RkISP1, Debug) << "Found DW100 dewarper " << dewarper_->deviceNode()
> >> + << " but invalid";
> >> + dewarper_.reset();
> >> + }
> >> + }
> >> +
> >> /*
> >> * Enumerate all sensors connected to the ISP and create one
> >> * camera instance for each of them.
> >> @@ -1283,7 +1388,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >> return;
> >>
> >> const FrameMetadata &metadata = buffer->metadata();
> >> - Request *request = buffer->request();
> >> + Request *request = info->request;
> >>
> >> if (metadata.status != FrameMetadata::FrameCancelled) {
> >> /*
> >> @@ -1300,11 +1405,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >> data->delayedCtrls_->get(metadata.sequence);
> >> data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
> >> }
> >> +
> >> } else {
> >> if (isRaw_)
> >> info->metadataProcessed = true;
> >> }
> >>
> >> + if (dewarper_) {
> >> + /*
> >> + * Queue input and output buffers to the dewarper. The output
> >> + * buffers for the dewarper are the buffers of the request, supplied
> >> + * by the application.
> >> + */
> >> + int ret = dewarper_->queueBuffers(buffer, request->buffers());
> >> + if (ret < 0)
> >> + LOG(RkISP1, Error) << "Cannot queue buffers to dewarper: "
> >> + << strerror(-ret);
> >> +
> >> + return;
> >> + }
> >> +
> >> + completeBuffer(request, buffer);
> >> + tryCompleteRequest(info);
> >> +}
> >> +
> >> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)
> >> +{
> >> + ASSERT(activeCamera_);
> >> + RkISP1CameraData *data = cameraData(activeCamera_);
> >> + Request *request = buffer->request();
> >> +
> >> + RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());
> >> + if (!info)
> >> + return;
> >> +
> >> + if (info->scalerCrop)
> >> + request->metadata().set(controls::ScalerCrop, info->scalerCrop.value());
> >> +
> >> completeBuffer(request, buffer);
> >> tryCompleteRequest(info);
> >> }
> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> >> index c49017d1..c96ec1d6 100644
> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> >> @@ -56,7 +56,7 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = {
> >>
> >> RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> >> const Size &minResolution, const Size &maxResolution)
> >> - : name_(name), running_(false), formats_(formats),
> >> + : name_(name), running_(false), internalBufs_(false), formats_(formats),
> >> minResolution_(minResolution), maxResolution_(maxResolution),
> >> link_(nullptr)
> >> {
> >> @@ -402,10 +402,12 @@ int RkISP1Path::start()
> >> if (running_)
> >> return -EBUSY;
> >>
> >> - /* \todo Make buffer count user configurable. */
> >> - ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> >> - if (ret)
> >> - return ret;
> >> + if (!internalBufs_) {
> >> + /* \todo Make buffer count user configurable. */
> >> + ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> >> + if (ret)
> >> + return ret;
> >> + }
> >>
> >> ret = video_->streamOn();
> >> if (ret) {
> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> >> index 08edefec..b7fa82d0 100644
> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> >> @@ -55,6 +55,19 @@ public:
> >> return video_->exportBuffers(bufferCount, buffers);
> >> }
> >>
> >> + int allocateBuffers(unsigned int bufferCount,
> >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >> + {
> >> + internalBufs_ = true;
> >> + return video_->allocateBuffers(bufferCount, buffers);
> >> + }
> >> +
> >> + int releaseBuffers()
> >> + {
> >> + ASSERT(internalBufs_);
> >> + return video_->releaseBuffers();
> >> + }
> >> +
> >> int start();
> >> void stop();
> >>
> >> @@ -68,6 +81,7 @@ private:
> >>
> >> const char *name_;
> >> bool running_;
> >> + bool internalBufs_;
> >>
> >> const Span<const PixelFormat> formats_;
> >> std::set<PixelFormat> streamFormats_;
> >> --
> >> 2.44.0
> >>
>
More information about the libcamera-devel
mailing list