[PATCH v6 5/5] libcamera: rkisp1: Plumb the ConverterDW100 converter
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Aug 4 17:33:08 CEST 2024
Hi Umang,
On Sat, Aug 03, 2024 at 12:49:32AM +0300, Laurent Pinchart wrote:
> On Fri, Jul 26, 2024 at 05:17:15PM +0530, Umang Jain wrote:
> > 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 | 154 ++++++++++++++++++++++-
> > 1 file changed, 150 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 25f2cc97..32ec0fdf 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
> > @@ -181,6 +186,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 +206,13 @@ private:
> > RkISP1MainPath mainPath_;
> > RkISP1SelfPath selfPath_;
> >
> > + std::unique_ptr<ConverterDW100> dewarper_;
> > + bool useDewarper_;
> > +
> > + /* 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 +235,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 +254,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >
> > statBuffer = pipe_->availableStatBuffers_.front();
> > pipe_->availableStatBuffers_.pop();
> > +
> > + if (pipe_->useDewarper_) {
> > + 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 +289,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 +305,7 @@ void RkISP1Frames::clear()
> >
> > pipe_->availableParamBuffers_.push(info->paramBuffer);
> > pipe_->availableStatBuffers_.push(info->statBuffer);
> > + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
> >
> > delete info;
> > }
> > @@ -607,7 +630,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > */
> >
> > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> > - : PipelineHandler(manager), hasSelfPath_(true)
> > + : PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)
> > {
> > }
> >
> > @@ -785,12 +808,19 @@ 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_) {
> > + outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg));
> > + ret = dewarper_->configure(cfg, outputCfgs);
> > + useDewarper_ = ret ? false : true;
> > + }
> > } else if (hasSelfPath_) {
> > ret = selfPath_.configure(cfg, format);
> > streamConfig[1] = IPAStream(cfg.pixelFormat,
> > @@ -839,6 +869,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
> > RkISP1CameraData *data = cameraData(camera);
> > unsigned int count = stream->configuration().bufferCount;
> >
> > + if (useDewarper_)
> > + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);
> > +
> > if (stream == &data->mainPathStream_)
> > return mainPath_.exportBuffers(count, buffers);
> > else if (hasSelfPath_ && stream == &data->selfPathStream_)
> > @@ -866,6 +899,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 */
>
> s/ISP/ISP./
>
> > + if (useDewarper_) {
> > + ret = mainPath_.exportBuffers(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 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > error:
> > paramBuffers_.clear();
> > statBuffers_.clear();
> > + mainPathBuffers_.clear();
> >
> > return ret;
> > }
> > @@ -903,8 +947,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_)
> > @@ -961,6 +1009,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> > << "Failed to start statistics " << camera->id();
> > return ret;
> > }
> > +
> > + if (useDewarper_) {
> > + ret = dewarper_->start();
>
> You don't stop the dewarper in the error paths below.
>
> > + if (ret) {
> > + LOG(RkISP1, Error) << "Failed to start dewarper";
> > + return ret;
>
> And there's no error handling here either.
I've just posted "[PATCH v3 0/2] libcamera: Simplify error handling with
ScopeExitActions class" which may be useful for you here.
> > + }
> > + }
> > }
> >
> > if (data->mainPath_->isEnabled()) {
> > @@ -1015,6 +1071,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
> > if (ret)
> > LOG(RkISP1, Warning)
> > << "Failed to stop parameters for " << camera->id();
> > +
> > + if (useDewarper_)
> > + dewarper_->stop();
> > }
> >
> > ASSERT(data->queuedRequests_.empty());
> > @@ -1045,6 +1104,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> > info->paramBuffer->cookie());
> > }
> >
> > + const auto &crop = request->controls().get(controls::ScalerCrop);
> > + if (crop && useDewarper_) {
> > + Rectangle appliedRect = crop.value();
> > + int ret = dewarper_->setInputCrop(&data->mainPathStream_, &appliedRect);
>
> This doesn't seem right, you're applying the crop rectangle too early.
>
> > + if (!ret) {
> > + if (appliedRect != crop.value()) {
> > + /*
> > + * \todo How to handle these case?
> > + * Do we aim for pixel perfect set rectangles?
> > + */
>
> This needs to be addressed, we need a decision (and a rationale), and we
> need to document it and handle the outcome accordingly. A warning
> message isn't a good solution.
>
> > + LOG(RkISP1, Warning)
> > + << "Applied rectangle " << appliedRect.toString()
> > + << " differs from requested " << crop.value().toString();
> > + }
> > +
> > + info->scalerCrop = appliedRect;
> > + }
> > + }
> > +
> > data->frame_++;
> >
> > return 0;
> > @@ -1110,6 +1188,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> > {
> > ControlInfoMap::Map rkisp1Controls;
> >
> > + if (dewarper_) {
> > + auto [minCrop, maxCrop] = dewarper_->inputCropBounds(&data->mainPathStream_);
> > +
> > + 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;
>
> Declare the variable below, when you assign it.
>
> > const MediaPad *pad;
> >
> > DeviceMatch dm("rkisp1");
> > @@ -1250,6 +1335,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";
> LOG(RkISP1, Debug)
> << "Found DW100 dewarper " << dewarper_->deviceNode()
> << " but invalid";
>
> I think this should be a warning at least.
>
> > + dewarper_.reset();
> > + }
> > + }
> > +
> > /*
> > * Enumerate all sensors connected to the ISP and create one
> > * camera instance for each of them.
> > @@ -1296,7 +1401,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> > return;
> >
> > const FrameMetadata &metadata = buffer->metadata();
> > - Request *request = buffer->request();
> > + Request *request = info->request;
>
> Is this because internal buffers have no request associated with them ?
>
> >
> > if (metadata.status != FrameMetadata::FrameCancelled) {
> > /*
> > @@ -1313,11 +1418,52 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> > data->delayedCtrls_->get(metadata.sequence);
> > data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
> > }
> > +
>
> Not needed.
>
> > } else {
> > if (isRaw_)
> > info->metadataProcessed = true;
> > }
> >
> > + if (useDewarper_) {
> > + /* Do not queue cancelled frames to dewarper. */
> > + if (metadata.status == FrameMetadata::FrameCancelled) {
> > + for (auto it : request->buffers())
> > + completeBuffer(request, it.second);
>
> Will this work with multiple streams, won't the other stream also try to
> complete its own buffer ? Or do you assume here that there's a single
> stream, given that the DW100 is only found in the i.M8XMP ? A comment
> would help, but even better, you should try to complete only the buffer
> for this stream instead of completing all buffers. That should make the
> code more future-proof.
>
> Update: after reading the whole patch, there are clear assumptions that
> there will be a single stream when the converter is used. I suppose
> that's OK, but they're expressed in different ways that make the code
> sometimes more complex to follow. For instance in
> PipelineHandlerRkISP1::exportFrameBuffers() you completely bypass the
> main/self path check when there's a converter, while here you iterate
> over all buffers in the request as if there could be multiple streams.
> Things may benefit from being cleaned up a bit to keep the code in a
> maintainable state.
>
> > +
> > + tryCompleteRequest(info);
> > + return;
> > + }
> > +
> > + /*
> > + * 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);
>
> You can move this above and call it with
>
> if (!useDewarper_) {
> ...
> return;
> }
>
> and lower the indentation level of the rest of the code.
>
> > +}
> > +
> > +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());
>
> This means that the scaler crop metadata will be set only when a request
> has been queued with the scaler crop control. From that point onwards,
> all request that complete will have the scaler crop metadata. Shouldn't
> you set it for every frame ?
>
> > +
> > completeBuffer(request, buffer);
> > tryCompleteRequest(info);
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list