[PATCH v7 4/4] libcamera: rkisp1: Plumb the dw100 dewarper as V4L2M2M converter
Umang Jain
umang.jain at ideasonboard.com
Thu Sep 26 09:43:24 CEST 2024
Hi Laurent,
On 26/09/24 12:16 am, Laurent Pinchart wrote:
> On Wed, Sep 25, 2024 at 01:29:42PM +0530, Umang Jain wrote:
>> On 25/09/24 1:20 pm, Stefan Klug wrote:
>>> On Fri, Sep 06, 2024 at 12:04:44PM +0530, Umang Jain wrote:
>>>> Plumb the dw100 dewarper as a V4L2M2M 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
>>>> the converter. 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 | 183 ++++++++++++++++++++++-
>>>> 1 file changed, 176 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> index 651258e3..0a794d63 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> @@ -6,10 +6,12 @@
>>>> */
>>>>
>>>> #include <algorithm>
>>>> +#include <map>
>>>> #include <memory>
>>>> #include <numeric>
>>>> #include <optional>
>>>> #include <queue>
>>>> +#include <vector>
>>>>
>>>> #include <linux/media-bus-format.h>
>>>> #include <linux/rkisp1-config.h>
>>>> @@ -32,6 +34,7 @@
>>>>
>>>> #include "libcamera/internal/camera.h"
>>>> #include "libcamera/internal/camera_sensor.h"
>>>> +#include "libcamera/internal/converter/converter_v4l2_m2m.h"
>>>> #include "libcamera/internal/delayed_controls.h"
>>>> #include "libcamera/internal/device_enumerator.h"
>>>> #include "libcamera/internal/framebuffer.h"
>>>> @@ -180,6 +183,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);
>>>> @@ -199,6 +203,15 @@ private:
>>>> RkISP1MainPath mainPath_;
>>>> RkISP1SelfPath selfPath_;
>>>>
>>>> + std::unique_ptr<V4L2M2MConverter> dewarper_;
>>>> + bool useDewarper_;
>>>> +
>>>> + std::optional<Rectangle> activeCrop_;
>>>> +
>>>> + /* 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_;
>>>> @@ -221,6 +234,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()) {
>>>> @@ -238,10 +253,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;
>>>>
>>>> @@ -267,6 +288,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);
>>>>
>>>> @@ -282,6 +304,7 @@ void RkISP1Frames::clear()
>>>>
>>>> pipe_->availableParamBuffers_.push(info->paramBuffer);
>>>> pipe_->availableStatBuffers_.push(info->statBuffer);
>>>> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
>>>>
>>>> delete info;
>>>> }
>>>> @@ -600,7 +623,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>>> */
>>>>
>>>> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>>>> - : PipelineHandler(manager), hasSelfPath_(true)
>>>> + : PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)
>>>> {
>>>> }
>>>>
>>>> @@ -778,12 +801,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,
>>>> @@ -833,10 +863,19 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
>>>> RkISP1CameraData *data = cameraData(camera);
>>>> unsigned int count = stream->configuration().bufferCount;
>>>>
>>>> - if (stream == &data->mainPathStream_)
>>>> - return mainPath_.exportBuffers(count, buffers);
>>>> - else if (hasSelfPath_ && stream == &data->selfPathStream_)
>>>> + if (stream == &data->mainPathStream_) {
>>>> + /*
>>>> + * Currently, i.MX8MP is the only platform with DW100 dewarper.
>>>> + * It has mainpath and no self path. Hence, export buffers from
>>>> + * dewarper just for the main path stream, for now.
>>>> + */
>>>> + if (useDewarper_)
>>>> + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);
>>>> + else
>>>> + return mainPath_.exportBuffers(count, buffers);
>>>> + } else if (hasSelfPath_ && stream == &data->selfPathStream_) {
>>>> return selfPath_.exportBuffers(count, buffers);
>>>> + }
>>>>
>>>> return -EINVAL;
>>>> }
>>>> @@ -860,6 +899,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>>>> ret = stat_->allocateBuffers(maxCount, &statBuffers_);
>>> Is there a way for the user to prevent usage of the dewarper? It
>>> increases latency and doubles the required buffers. Now that I write
>>> about it. To configure actual lens dewarping I envision something like a
>>> "dewarp" algorithm in the tuning file. Maybe that is the best place for
>>> that decision...
>> I think somewhere in the previous interations, we decided that if the
>> dewarper is present, it should be discovered and used. At that point, we
>> failed to justify any use cases, where a user might have to disable the
>> dewarper 'intentionally'.
>>
>> If there is use case, we can surely allow disabling the dewarper (and
>> shouldn't be a tedious patch on top, I believe). But, The justification
>> has to be there.
> If the camera doesn't have a dewarping map, and if the user doesn't
> request digital zoom, then I think the dewarper should be bypassed
> instead of introducing delays.
Behavior seems okay to me. Bypassing the dewarper should be handled when
plumbing the vertex map, I think ? Stefan, what do you think?
>
> I would still like to get to the bottom of using the resizer to
> implement digital zoom.
Any specific reasons? Dewarper gives us better digital zoom
capabilities limits, last I checked.
>
>>>> if (ret < 0)
>>>> goto error;
>>>> +
>>>> + /* If the dewarper is being used, allocate internal buffers for 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_) {
>>>> @@ -883,6 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>>>> error:
>>>> paramBuffers_.clear();
>>>> statBuffers_.clear();
>>>> + mainPathBuffers_.clear();
>>>>
>>>> return ret;
>>>> }
>>>> @@ -897,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_)
>>>> @@ -954,6 +1008,15 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>>>> return ret;
>>>> }
>>>> actions += [&]() { stat_->streamOff(); };
>>>> +
>>>> + if (useDewarper_) {
>>>> + ret = dewarper_->start();
>>>> + if (ret) {
>>>> + LOG(RkISP1, Error) << "Failed to start dewarper";
>>>> + return ret;
>>>> + }
>>>> + }
>>>> + actions += [&]() { dewarper_->stop(); };
>>>> }
>>>>
>>>> if (data->mainPath_->isEnabled()) {
>>>> @@ -1000,6 +1063,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());
>>>> @@ -1110,6 +1176,16 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
>>>> {
>>>> ControlInfoMap::Map controls;
>>>>
>>>> + if (dewarper_) {
>>>> + std::pair<Rectangle, Rectangle> cropLimits =
>>>> + dewarper_->inputCropBounds(&data->mainPathStream_);
>>>> +
>>>> + controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first,
>>>> + cropLimits.second,
>>>> + cropLimits.second);
>>>> + activeCrop_ = cropLimits.second;
>>>> + }
>>>> +
>>> I'm a bit uncertain here. The current dewarper kernel driver produces
>>> quite unexpected and difficult to explain results (modified crop
>>> rectangle, changed pixel aspect ratio). So to me it feels like we
>>> shouldn't expose that hardware control directly to the user, but do an
>>> internal implementation (based on the same libcamera control). Could we
>>> split just the logic that adds the control into a separate patch?
>> I don't understand internal implementation based on same libcamera
>> control? How would that look like?
>>
>> So, you mean to say, we don't feed any scaler crop rectangles from
>> application, but do it internally ?
>>
>>> Another approach would be to merge it in and improve later. But then we
>>> might break the interface for users of the feature... I lean towards
>>> splitting the control and merging the rest. In that case we should
>>> disable the dewarper by default, as the user has no added benefit
>>> without the control. Still I think all the plumbing should definitely
>>> go in.
>> I don't foresee breaking the interface for users. But there can be
>> improvements on top of this (for e.g. mapping the V4L2 Selection flags)
>> and letting the application control that via an interface.
>>
>> https://docs.kernel.org/userspace-api/media/v4l/v4l2-selection-flags.html
>>
>>>> /* Add the IPA registered controls to list of camera controls. */
>>>> for (const auto &ipaControl : data->ipaControls_)
>>>> controls[ipaControl.first] = ipaControl.second;
>>>> @@ -1236,6 +1312,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");
>>>> +
>>>> + std::shared_ptr<MediaDevice> dwpMediaDevice = enumerator->search(dwp);
>>>> + if (dwpMediaDevice) {
>>>> + dewarper_ = std::make_unique<V4L2M2MConverter>(dwpMediaDevice.get());
>>>> + if (dewarper_->isValid()) {
>>>> + dewarper_->outputBufferReady.connect(
>>>> + this, &PipelineHandlerRkISP1::dewarpBufferReady);
>>>> +
>>>> + LOG(RkISP1, Info) << "Using DW100 dewarper " << dewarper_->deviceNode();
>>>> + } else {
>>>> + LOG(RkISP1, Warning) << "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.
>>>> @@ -1282,7 +1378,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) {
>>>> /*
>>>> @@ -1304,6 +1400,79 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>>>> info->metadataProcessed = true;
>>>> }
>>>>
>>>> + if (!useDewarper_) {
>>>> + completeBuffer(request, buffer);
>>>> + tryCompleteRequest(info);
>>>> +
>>>> + return;
>>>> + }
>>>> +
>>>> + /* Do not queue cancelled frames to dewarper. */
>>>> + if (metadata.status == FrameMetadata::FrameCancelled) {
>>>> + /*
>>>> + * i.MX8MP is the only known platform with dewarper. It has
>>>> + * no self path. Hence, only main path buffer completion is
>>>> + * required.
>>>> + *
>>>> + * Also, we cannot completeBuffer(request, buffer) as buffer
>>>> + * here, is an internal buffer (between ISP and dewarper) and
>>>> + * is not associated to the any specific request. The request
>>>> + * buffer associated with main path stream is the one that
>>>> + * is required to be completed (not the internal buffer).
>>>> + */
>>>> + for (auto it : request->buffers()) {
>>>> + if (it.first == &data->mainPathStream_)
>>>> + completeBuffer(request, it.second);
>>>> + }
>>>> +
>>>> + tryCompleteRequest(info);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* Handle scaler crop control. */
>>>> + const auto &crop = request->controls().get(controls::ScalerCrop);
>>>> + if (crop) {
>>>> + Rectangle appliedRect = crop.value();
>>>> +
>>>> + int ret = dewarper_->setInputCrop(&data->mainPathStream_,
>>>> + &appliedRect);
>>>> + if (!ret && appliedRect != crop.value()) {
>>>> + /*
>>>> + * If the rectangle is changed by setInputCrop on the
>>>> + * dewarper, log a debug message and cache the actual
>>>> + * applied rectangle for metadata reporting.
>>>> + */
>>>> + LOG(RkISP1, Debug)
>>>> + << "Applied rectangle " << appliedRect.toString()
>>>> + << " differs from requested " << crop.value().toString();
>>>> + }
>>>> +
>>>> + activeCrop_ = appliedRect;
>>>> + }
>>>> +
>>>> + /*
>>>> + * 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);
>>>> +}
>>>> +
>>>> +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;
>>>> +
>>>> + request->metadata().set(controls::ScalerCrop, activeCrop_.value());
>>> I couldn't easily see if there are cases where more than one buffer are
>>> queued to the dewarper. In that case the activeCrop_ could be the one
>>> applied to the next frame. If it is ensured that that is not the case it
>>> might be worth a comment.
>>>
>>>> +
>>>> completeBuffer(request, buffer);
>>>> tryCompleteRequest(info);
>>>> }
More information about the libcamera-devel
mailing list