[{"id":30423,"web_url":"https://patchwork.libcamera.org/comment/30423/","msgid":"","date":"2024-07-17T13:10:22","subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Wed, Jul 10, 2024 at 01:51:51AM GMT, Umang Jain wrote:\n> Plumb the ConverterDW100 converter in the rkisp1 pipeline handler.\n> If the dewarper is found, it is instantiated and buffers are exported\n> from it, instead of RkISP1Path. Internal buffers are allocated for the\n> RkISP1Path in case where dewarper is going to be used.\n>\n> The RKISP1 pipeline handler now supports scaler crop control through\n> ConverterDW100. Register the ScalerCrop control for the cameras created\n> in the RKISP1 pipeline handler.\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 148 +++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n> 3 files changed, 165 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index bfc44239..881e10e1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -8,9 +8,11 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> +#include \n>\n> #include \n> #include \n> @@ -33,6 +35,7 @@\n>\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/converter/converter_dw100.h\"\n> #include \"libcamera/internal/delayed_controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/framebuffer.h\"\n> @@ -62,6 +65,8 @@ struct RkISP1FrameInfo {\n>\n> \tbool paramDequeued;\n> \tbool metadataProcessed;\n> +\n> +\tstd::optional scalerCrop;\n> };\n>\n> class RkISP1Frames\n> @@ -181,6 +186,7 @@ private:\n> \tvoid bufferReady(FrameBuffer *buffer);\n> \tvoid paramReady(FrameBuffer *buffer);\n> \tvoid statReady(FrameBuffer *buffer);\n> +\tvoid dewarpBufferReady(FrameBuffer *buffer);\n> \tvoid frameStart(uint32_t sequence);\n>\n> \tint allocateBuffers(Camera *camera);\n> @@ -200,6 +206,13 @@ private:\n> \tRkISP1MainPath mainPath_;\n> \tRkISP1SelfPath selfPath_;\n>\n> +\tstd::unique_ptr dewarper_;\n> +\tbool useDewarper_;\n> +\n> +\t/* Internal buffers used when dewarper is being used */\n> +\tstd::vector> mainPathBuffers_;\n> +\tstd::queue availableMainPathBuffers_;\n> +\n> \tstd::vector> paramBuffers_;\n> \tstd::vector> statBuffers_;\n> \tstd::queue availableParamBuffers_;\n> @@ -222,6 +235,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>\n> \tFrameBuffer *paramBuffer = nullptr;\n> \tFrameBuffer *statBuffer = nullptr;\n> +\tFrameBuffer *mainPathBuffer = nullptr;\n> +\tFrameBuffer *selfPathBuffer = nullptr;\n>\n> \tif (!isRaw) {\n> \t\tif (pipe_->availableParamBuffers_.empty()) {\n> @@ -239,10 +254,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>\n> \t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> \t\tpipe_->availableStatBuffers_.pop();\n> +\n> +\t\tif (pipe_->useDewarper_) {\n> +\t\t\tmainPathBuffer = pipe_->availableMainPathBuffers_.front();\n\nCan we exaust the availableMainPathBuffers_ queue? Should this be\nchecked for validity ?\n\n> +\t\t\tpipe_->availableMainPathBuffers_.pop();\n> +\t\t}\n> \t}\n>\n> -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> +\tif (!mainPathBuffer)\n> +\t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> +\tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>\n> \tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n>\n> @@ -268,6 +289,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n>\n> \tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> \tpipe_->availableStatBuffers_.push(info->statBuffer);\n> +\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>\n> \tframeInfo_.erase(info->frame);\n>\n> @@ -283,6 +305,7 @@ void RkISP1Frames::clear()\n>\n> \t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> \t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> +\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>\n> \t\tdelete info;\n> \t}\n> @@ -607,7 +630,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> */\n>\n> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> -\t: PipelineHandler(manager), hasSelfPath_(true)\n> +\t: PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)\n> {\n> }\n>\n> @@ -785,12 +808,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \" crop \" << rect;\n>\n> \tstd::map streamConfig;\n> +\tstd::vector> outputCfgs;\n>\n> \tfor (const StreamConfiguration &cfg : *config) {\n> \t\tif (cfg.stream() == &data->mainPathStream_) {\n> \t\t\tret = mainPath_.configure(cfg, format);\n> \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> \t\t\t\t\t\t cfg.size);\n> +\t\t\t/* Configure dewarp */\n> +\t\t\tif (dewarper_ && !isRaw_) {\n> +\t\t\t\toutputCfgs.push_back(const_cast(cfg));\n\nWhy a vector, can you have more than one ?\n\n> +\t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n\nAh, the need to have a vector comes from the configure() signature\n\nSeems like\n\t\t\t\tret = dewarper_->configure(cfg, { cfg });\n\nwould do\n\n> +\t\t\t\tuseDewarper_ = ret ? false : true;\n> +\t\t\t}\n> \t\t} else if (hasSelfPath_) {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> @@ -839,6 +869,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> \tRkISP1CameraData *data = cameraData(camera);\n> \tunsigned int count = stream->configuration().bufferCount;\n>\n> +\tif (useDewarper_)\n> +\t\treturn dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n> +\n\nUnconditionally ? Even if the stream is the selfPath one ?\n\n> \tif (stream == &data->mainPathStream_)\n> \t\treturn mainPath_.exportBuffers(count, buffers);\n> \telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> @@ -866,6 +899,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> \t\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> \t\tif (ret < 0)\n> \t\t\tgoto error;\n> +\n> +\t\t/* If the dewarper is being used, allocate internal buffers for ISP */\n> +\t\tif (useDewarper_) {\n> +\t\t\tret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\tgoto error;\n\nIs this right ?\n\nIt seems the model here is to allocate buffers in the main path and\nimport them in the dewarper.\n\nallocateBuffer() creates and exports the buffers but sets the video\ndevice in MMAP mode.\n\nI would be expecting instead the main path to\n1) export buffers\n2) import them back to intialize the queue in DMABUF mode\n\nFrom there buffers dequeued from the main path are queued to the\ndewarper without any need to be mapped into a userspace accesible\naddress.\n\nAm I confused maybe ?\n\n\n> +\n> +\t\t\tfor (std::unique_ptr &buffer : mainPathBuffers_)\n> +\t\t\t\tavailableMainPathBuffers_.push(buffer.get());\n> +\t\t}\n> \t}\n>\n> \tfor (std::unique_ptr &buffer : paramBuffers_) {\n> @@ -889,6 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> error:\n> \tparamBuffers_.clear();\n> \tstatBuffers_.clear();\n> +\tmainPathBuffers_.clear();\n>\n> \treturn ret;\n> }\n> @@ -903,8 +947,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> \twhile (!availableParamBuffers_.empty())\n> \t\tavailableParamBuffers_.pop();\n>\n> +\twhile (!availableMainPathBuffers_.empty())\n> +\t\tavailableMainPathBuffers_.pop();\n> +\n> \tparamBuffers_.clear();\n> \tstatBuffers_.clear();\n> +\tmainPathBuffers_.clear();\n>\n> \tstd::vector ids;\n> \tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n> @@ -919,6 +967,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> \tif (stat_->releaseBuffers())\n> \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n>\n> +\tif (useDewarper_)\n> +\t\tmainPath_.releaseBuffers();\n> +\n> \treturn 0;\n> }\n>\n> @@ -961,6 +1012,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> \t\t\treturn ret;\n> \t\t}\n> +\n> +\t\tif (useDewarper_) {\n> +\t\t\tret = dewarper_->start();\n> +\t\t\tif (ret) {\n> +\t\t\t\tLOG(RkISP1, Error) << \"Failed to start dewarper\";\n\nThe error handling path should undo all the previous actions\n\n> +\t\t\t\treturn ret;\n> +\t\t\t}\n> +\t\t}\n> \t}\n>\n> \tif (data->mainPath_->isEnabled()) {\n> @@ -1015,6 +1074,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> \t\tif (ret)\n> \t\t\tLOG(RkISP1, Warning)\n> \t\t\t\t<< \"Failed to stop parameters for \" << camera->id();\n> +\n> +\t\tif (useDewarper_)\n> +\t\t\tdewarper_->stop();\n> \t}\n>\n> \tASSERT(data->queuedRequests_.empty());\n> @@ -1045,6 +1107,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> \t\t\t\t\t info->paramBuffer->cookie());\n> \t}\n>\n> +\tconst auto &crop = request->controls().get(controls::ScalerCrop);\n> +\tif (crop && useDewarper_) {\n> +\t\tRectangle appliedRect = crop.value();\n> +\t\tint ret = dewarper_->setCrop(&data->mainPathStream_, &appliedRect);\n> +\t\tif (!ret) {\n> +\t\t\tif (appliedRect != crop.value()) {\n> +\t\t\t\t/*\n> +\t\t\t\t * \\todo How to handle these case?\n> +\t\t\t\t * Do we aim for pixel perfect set rectangles?\n> +\t\t\t\t */\n\nI think this should be reported in metadata as it has been applied and\nlet the application decide what to do there\n\n> +\t\t\t\tLOG(RkISP1, Warning)\n> +\t\t\t\t\t<< \"Applied rectangle \" << appliedRect.toString()\n> +\t\t\t\t\t<< \" differs from requested \" << crop.value().toString();\n> +\t\t\t}\n> +\n> +\t\t\tinfo->scalerCrop = appliedRect;\n> +\t\t}\n> +\t}\n> +\n> \tdata->frame_++;\n>\n> \treturn 0;\n> @@ -1110,6 +1191,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> {\n> \tControlInfoMap::Map rkisp1Controls;\n>\n> +\tif (dewarper_) {\n> +\t\tauto [minCrop, maxCrop] = dewarper_->getCropBounds(&data->mainPathStream_);\n> +\n> +\t\trkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> +\t}\n> +\n> \t/* Add the IPA registered controls to list of camera controls. */\n> \tfor (const auto &ipaControl : data->ipaControls_)\n> \t\trkisp1Controls[ipaControl.first] = ipaControl.second;\n> @@ -1173,6 +1260,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>\n> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> {\n> +\tstd::shared_ptr dwpMediaDevice;\n> \tconst MediaPad *pad;\n>\n> \tDeviceMatch dm(\"rkisp1\");\n> @@ -1237,6 +1325,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>\n> +\t/* If dewarper is present, create its instance. */\n> +\tDeviceMatch dwp(\"dw100\");\n> +\tdwp.add(\"dw100-source\");\n> +\tdwp.add(\"dw100-sink\");\n> +\n> +\tdwpMediaDevice = enumerator->search(dwp);\n> +\tif (dwpMediaDevice) {\n> +\t\tdewarper_ = std::make_unique(std::move(dwpMediaDevice));\n> +\t\tif (dewarper_->isValid()) {\n> +\t\t\tdewarper_->outputBufferReady.connect(\n> +\t\t\t\tthis, &PipelineHandlerRkISP1::dewarpBufferReady);\n> +\n> +\t\t\tLOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n> +\t\t} else {\n> +\t\t\tLOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n> +\t\t\t\t\t << \" but invalid\";\n> +\t\t\tdewarper_.reset();\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * Enumerate all sensors connected to the ISP and create one\n> \t * camera instance for each of them.\n> @@ -1283,7 +1391,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> \t\treturn;\n>\n> \tconst FrameMetadata &metadata = buffer->metadata();\n> -\tRequest *request = buffer->request();\n> +\tRequest *request = info->request;\n>\n> \tif (metadata.status != FrameMetadata::FrameCancelled) {\n> \t\t/*\n> @@ -1300,11 +1408,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> \t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> \t\t}\n> +\n\nrougue white line\n\n> \t} else {\n> \t\tif (isRaw_)\n> \t\t\tinfo->metadataProcessed = true;\n> \t}\n>\n> +\tif (useDewarper_) {\n> +\t\t/*\n> +\t\t * Queue input and output buffers to the dewarper. The output\n> +\t\t * buffers for the dewarper are the buffers of the request, supplied\n> +\t\t * by the application.\n> +\t\t */\n> +\t\tint ret = dewarper_->queueBuffers(buffer, request->buffers());\n\nWhat if buffer comes from the self path ?\n\n> +\t\tif (ret < 0)\n> +\t\t\tLOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n> +\t\t\t\t\t << strerror(-ret);\n> +\n> +\t\treturn;\n> +\t}\n> +\n> +\tcompleteBuffer(request, buffer);\n> +\ttryCompleteRequest(info);\n> +}\n> +\n> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> +{\n> +\tASSERT(activeCamera_);\n> +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tRequest *request = buffer->request();\n> +\n> +\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> +\tif (!info)\n> +\t\treturn;\n> +\n> +\tif (info->scalerCrop)\n> +\t\trequest->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n> +\n> \tcompleteBuffer(request, buffer);\n> \ttryCompleteRequest(info);\n> }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index c49017d1..c96ec1d6 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -56,7 +56,7 @@ const std::map formatToMediaBus = {\n>\n> RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> \t\t const Size &minResolution, const Size &maxResolution)\n> -\t: name_(name), running_(false), formats_(formats),\n> +\t: name_(name), running_(false), internalBufs_(false), formats_(formats),\n> \t minResolution_(minResolution), maxResolution_(maxResolution),\n> \t link_(nullptr)\n> {\n> @@ -402,10 +402,12 @@ int RkISP1Path::start()\n> \tif (running_)\n> \t\treturn -EBUSY;\n>\n> -\t/* \\todo Make buffer count user configurable. */\n> -\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!internalBufs_) {\n\nWould you still need this if you exportBuffers() from the main path ?\n\n> +\t\t/* \\todo Make buffer count user configurable. */\n> +\t\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n>\n> \tret = video_->streamOn();\n> \tif (ret) {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 08edefec..b7fa82d0 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -55,6 +55,19 @@ public:\n> \t\treturn video_->exportBuffers(bufferCount, buffers);\n> \t}\n>\n> +\tint allocateBuffers(unsigned int bufferCount,\n> +\t\t\t std::vector> *buffers)\n> +\t{\n> +\t\tinternalBufs_ = true;\n> +\t\treturn video_->allocateBuffers(bufferCount, buffers);\n\nWhat if allocateBuffers() fails ? You have sent the internalBufs_ flag\nbefore this call.\n\n> +\t}\n> +\n> +\tint releaseBuffers()\n> +\t{\n> +\t\tASSERT(internalBufs_);\n> +\t\treturn video_->releaseBuffers();\n> +\t}\n> +\n\nI think you should move these to the cpp file.\n\n> \tint start();\n> \tvoid stop();\n>\n> @@ -68,6 +81,7 @@ private:\n>\n> \tconst char *name_;\n> \tbool running_;\n> +\tbool internalBufs_;\n>\n> \tconst Span formats_;\n> \tstd::set streamFormats_;\n> --\n> 2.45.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 286BFBDB1C\n\tfor ;\n\tWed, 17 Jul 2024 13:10:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A3476336F;\n\tWed, 17 Jul 2024 15:10:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AE9D619AB\n\tfor ;\n\tWed, 17 Jul 2024 15:10:26 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 024BB66F;\n\tWed, 17 Jul 2024 15:09:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sN+MIUe0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721221788;\n\tbh=nUOGaV0yYzWGdAnt8T11Obkt44OnbITOli8LNXWpJxo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sN+MIUe0Cu9sS+WTIqc07alN/A3tYpUz5H59cJIat2EnAxGRohObwLt87+TBjBJcq\n\trVsb8Wl/4OvVNxoSta8ysFJtIHI4GO1VV3QPOIj8DOr8Wyk7l0H7WA65rxI4ruBEGT\n\tM7U9gmmk0x9mbIdOKr5EAr7RX2UaWQ36eM89H7mI=","Date":"Wed, 17 Jul 2024 15:10:22 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-6-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240709202151.152289-6-umang.jain@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30424,"web_url":"https://patchwork.libcamera.org/comment/30424/","msgid":"","date":"2024-07-18T05:22:35","subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 17/07/24 6:40 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Wed, Jul 10, 2024 at 01:51:51AM GMT, Umang Jain wrote:\n>> Plumb the ConverterDW100 converter in the rkisp1 pipeline handler.\n>> If the dewarper is found, it is instantiated and buffers are exported\n>> from it, instead of RkISP1Path. Internal buffers are allocated for the\n>> RkISP1Path in case where dewarper is going to be used.\n>>\n>> The RKISP1 pipeline handler now supports scaler crop control through\n>> ConverterDW100. Register the ScalerCrop control for the cameras created\n>> in the RKISP1 pipeline handler.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 148 +++++++++++++++++-\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n>> 3 files changed, 165 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index bfc44239..881e10e1 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -8,9 +8,11 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> +#include \n>>\n>> #include \n>> #include \n>> @@ -33,6 +35,7 @@\n>>\n>> #include \"libcamera/internal/camera.h\"\n>> #include \"libcamera/internal/camera_sensor.h\"\n>> +#include \"libcamera/internal/converter/converter_dw100.h\"\n>> #include \"libcamera/internal/delayed_controls.h\"\n>> #include \"libcamera/internal/device_enumerator.h\"\n>> #include \"libcamera/internal/framebuffer.h\"\n>> @@ -62,6 +65,8 @@ struct RkISP1FrameInfo {\n>>\n>> \tbool paramDequeued;\n>> \tbool metadataProcessed;\n>> +\n>> +\tstd::optional scalerCrop;\n>> };\n>>\n>> class RkISP1Frames\n>> @@ -181,6 +186,7 @@ private:\n>> \tvoid bufferReady(FrameBuffer *buffer);\n>> \tvoid paramReady(FrameBuffer *buffer);\n>> \tvoid statReady(FrameBuffer *buffer);\n>> +\tvoid dewarpBufferReady(FrameBuffer *buffer);\n>> \tvoid frameStart(uint32_t sequence);\n>>\n>> \tint allocateBuffers(Camera *camera);\n>> @@ -200,6 +206,13 @@ private:\n>> \tRkISP1MainPath mainPath_;\n>> \tRkISP1SelfPath selfPath_;\n>>\n>> +\tstd::unique_ptr dewarper_;\n>> +\tbool useDewarper_;\n>> +\n>> +\t/* Internal buffers used when dewarper is being used */\n>> +\tstd::vector> mainPathBuffers_;\n>> +\tstd::queue availableMainPathBuffers_;\n>> +\n>> \tstd::vector> paramBuffers_;\n>> \tstd::vector> statBuffers_;\n>> \tstd::queue availableParamBuffers_;\n>> @@ -222,6 +235,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>>\n>> \tFrameBuffer *paramBuffer = nullptr;\n>> \tFrameBuffer *statBuffer = nullptr;\n>> +\tFrameBuffer *mainPathBuffer = nullptr;\n>> +\tFrameBuffer *selfPathBuffer = nullptr;\n>>\n>> \tif (!isRaw) {\n>> \t\tif (pipe_->availableParamBuffers_.empty()) {\n>> @@ -239,10 +254,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>>\n>> \t\tstatBuffer = pipe_->availableStatBuffers_.front();\n>> \t\tpipe_->availableStatBuffers_.pop();\n>> +\n>> +\t\tif (pipe_->useDewarper_) {\n>> +\t\t\tmainPathBuffer = pipe_->availableMainPathBuffers_.front();\n> Can we exaust the availableMainPathBuffers_ queue? Should this be\n> checked for validity ?\n\nSeems rule to be applied for all available*Buffers_ in rkisp1 pipeline \nhandler?\n\n>\n>> +\t\t\tpipe_->availableMainPathBuffers_.pop();\n>> +\t\t}\n>> \t}\n>>\n>> -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>> +\tif (!mainPathBuffer)\n>> +\t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> +\tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>>\n>> \tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n>>\n>> @@ -268,6 +289,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n>>\n>> \tpipe_->availableParamBuffers_.push(info->paramBuffer);\n>> \tpipe_->availableStatBuffers_.push(info->statBuffer);\n>> +\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>>\n>> \tframeInfo_.erase(info->frame);\n>>\n>> @@ -283,6 +305,7 @@ void RkISP1Frames::clear()\n>>\n>> \t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n>> \t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n>> +\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>>\n>> \t\tdelete info;\n>> \t}\n>> @@ -607,7 +630,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> */\n>>\n>> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n>> -\t: PipelineHandler(manager), hasSelfPath_(true)\n>> +\t: PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)\n>> {\n>> }\n>>\n>> @@ -785,12 +808,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>> \t\t<< \" crop \" << rect;\n>>\n>> \tstd::map streamConfig;\n>> +\tstd::vector> outputCfgs;\n>>\n>> \tfor (const StreamConfiguration &cfg : *config) {\n>> \t\tif (cfg.stream() == &data->mainPathStream_) {\n>> \t\t\tret = mainPath_.configure(cfg, format);\n>> \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n>> \t\t\t\t\t\t cfg.size);\n>> +\t\t\t/* Configure dewarp */\n>> +\t\t\tif (dewarper_ && !isRaw_) {\n>> +\t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> Why a vector, can you have more than one ?\n\nconverter can have more than one outputs yes\n>\n>> +\t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> Ah, the need to have a vector comes from the configure() signature\n>\n> Seems like\n> \t\t\t\tret = dewarper_->configure(cfg, { cfg });\n>\n> would do\n\nconst is a problem here I think\n\n>\n>> +\t\t\t\tuseDewarper_ = ret ? false : true;\n>> +\t\t\t}\n>> \t\t} else if (hasSelfPath_) {\n>> \t\t\tret = selfPath_.configure(cfg, format);\n>> \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n>> @@ -839,6 +869,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n>> \tRkISP1CameraData *data = cameraData(camera);\n>> \tunsigned int count = stream->configuration().bufferCount;\n>>\n>> +\tif (useDewarper_)\n>> +\t\treturn dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n>> +\n> Unconditionally ? Even if the stream is the selfPath one ?\n\ndewarper only works with mainPath_ I believe.\n\nThe i.MX8MP doesn't have selfPath as far as I know? Paul, can you \ncorrect me on this - I think we had a discussion regarding that.\n\n>\n>> \tif (stream == &data->mainPathStream_)\n>> \t\treturn mainPath_.exportBuffers(count, buffers);\n>> \telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n>> @@ -866,6 +899,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>> \t\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n>> \t\tif (ret < 0)\n>> \t\t\tgoto error;\n>> +\n>> +\t\t/* If the dewarper is being used, allocate internal buffers for ISP */\n>> +\t\tif (useDewarper_) {\n>> +\t\t\tret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n>> +\t\t\tif (ret < 0)\n>> +\t\t\t\tgoto error;\n> Is this right ?\n>\n> It seems the model here is to allocate buffers in the main path and\n> import them in the dewarper.\n\nThe model here is to simply export the buffers from dewarper and \nallocate internal buffers for RkISP1Path\n\nThese internal buffers allocated (availableMainPathBuffers_), are then \nqueued to the dewarper.\n\n>\n> allocateBuffer() creates and exports the buffers but sets the video\n> device in MMAP mode.\n>\n> I would be expecting instead the main path to\n> 1) export buffers\n> 2) import them back to intialize the queue in DMABUF mode\n>\n> From there buffers dequeued from the main path are queued to the\n> dewarper without any need to be mapped into a userspace accesible\n> address.\n>\n> Am I confused maybe ?\n\nNow I'm confused with the above strategy. Do you mean that export \nbuffers from mainPath and have internal buffers allocated in the dewarper?\n\nWhen you say,\n\n2) import them back to intialize the queue in DMABUF mode\n\nImport them back where?\n\nBut certainly it's a worth while idea to have them exported from main \npath and also queue to it the dewarper. Would require some \nexperimentation from my side.\n\n>\n>\n>> +\n>> +\t\t\tfor (std::unique_ptr &buffer : mainPathBuffers_)\n>> +\t\t\t\tavailableMainPathBuffers_.push(buffer.get());\n>> +\t\t}\n>> \t}\n>>\n>> \tfor (std::unique_ptr &buffer : paramBuffers_) {\n>> @@ -889,6 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>> error:\n>> \tparamBuffers_.clear();\n>> \tstatBuffers_.clear();\n>> +\tmainPathBuffers_.clear();\n>>\n>> \treturn ret;\n>> }\n>> @@ -903,8 +947,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>> \twhile (!availableParamBuffers_.empty())\n>> \t\tavailableParamBuffers_.pop();\n>>\n>> +\twhile (!availableMainPathBuffers_.empty())\n>> +\t\tavailableMainPathBuffers_.pop();\n>> +\n>> \tparamBuffers_.clear();\n>> \tstatBuffers_.clear();\n>> +\tmainPathBuffers_.clear();\n>>\n>> \tstd::vector ids;\n>> \tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n>> @@ -919,6 +967,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>> \tif (stat_->releaseBuffers())\n>> \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n>>\n>> +\tif (useDewarper_)\n>> +\t\tmainPath_.releaseBuffers();\n>> +\n>> \treturn 0;\n>> }\n>>\n>> @@ -961,6 +1012,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n>> \t\t\t\t<< \"Failed to start statistics \" << camera->id();\n>> \t\t\treturn ret;\n>> \t\t}\n>> +\n>> +\t\tif (useDewarper_) {\n>> +\t\t\tret = dewarper_->start();\n>> +\t\t\tif (ret) {\n>> +\t\t\t\tLOG(RkISP1, Error) << \"Failed to start dewarper\";\n> The error handling path should undo all the previous actions\n>\n>> +\t\t\t\treturn ret;\n>> +\t\t\t}\n>> +\t\t}\n>> \t}\n>>\n>> \tif (data->mainPath_->isEnabled()) {\n>> @@ -1015,6 +1074,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n>> \t\tif (ret)\n>> \t\t\tLOG(RkISP1, Warning)\n>> \t\t\t\t<< \"Failed to stop parameters for \" << camera->id();\n>> +\n>> +\t\tif (useDewarper_)\n>> +\t\t\tdewarper_->stop();\n>> \t}\n>>\n>> \tASSERT(data->queuedRequests_.empty());\n>> @@ -1045,6 +1107,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n>> \t\t\t\t\t info->paramBuffer->cookie());\n>> \t}\n>>\n>> +\tconst auto &crop = request->controls().get(controls::ScalerCrop);\n>> +\tif (crop && useDewarper_) {\n>> +\t\tRectangle appliedRect = crop.value();\n>> +\t\tint ret = dewarper_->setCrop(&data->mainPathStream_, &appliedRect);\n>> +\t\tif (!ret) {\n>> +\t\t\tif (appliedRect != crop.value()) {\n>> +\t\t\t\t/*\n>> +\t\t\t\t * \\todo How to handle these case?\n>> +\t\t\t\t * Do we aim for pixel perfect set rectangles?\n>> +\t\t\t\t */\n> I think this should be reported in metadata as it has been applied and\n> let the application decide what to do there\n\nack\n>\n>> +\t\t\t\tLOG(RkISP1, Warning)\n>> +\t\t\t\t\t<< \"Applied rectangle \" << appliedRect.toString()\n>> +\t\t\t\t\t<< \" differs from requested \" << crop.value().toString();\n>> +\t\t\t}\n>> +\n>> +\t\t\tinfo->scalerCrop = appliedRect;\n>> +\t\t}\n>> +\t}\n>> +\n>> \tdata->frame_++;\n>>\n>> \treturn 0;\n>> @@ -1110,6 +1191,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n>> {\n>> \tControlInfoMap::Map rkisp1Controls;\n>>\n>> +\tif (dewarper_) {\n>> +\t\tauto [minCrop, maxCrop] = dewarper_->getCropBounds(&data->mainPathStream_);\n>> +\n>> +\t\trkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n>> +\t}\n>> +\n>> \t/* Add the IPA registered controls to list of camera controls. */\n>> \tfor (const auto &ipaControl : data->ipaControls_)\n>> \t\trkisp1Controls[ipaControl.first] = ipaControl.second;\n>> @@ -1173,6 +1260,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>>\n>> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> {\n>> +\tstd::shared_ptr dwpMediaDevice;\n>> \tconst MediaPad *pad;\n>>\n>> \tDeviceMatch dm(\"rkisp1\");\n>> @@ -1237,6 +1325,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>>\n>> +\t/* If dewarper is present, create its instance. */\n>> +\tDeviceMatch dwp(\"dw100\");\n>> +\tdwp.add(\"dw100-source\");\n>> +\tdwp.add(\"dw100-sink\");\n>> +\n>> +\tdwpMediaDevice = enumerator->search(dwp);\n>> +\tif (dwpMediaDevice) {\n>> +\t\tdewarper_ = std::make_unique(std::move(dwpMediaDevice));\n>> +\t\tif (dewarper_->isValid()) {\n>> +\t\t\tdewarper_->outputBufferReady.connect(\n>> +\t\t\t\tthis, &PipelineHandlerRkISP1::dewarpBufferReady);\n>> +\n>> +\t\t\tLOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n>> +\t\t} else {\n>> +\t\t\tLOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n>> +\t\t\t\t\t << \" but invalid\";\n>> +\t\t\tdewarper_.reset();\n>> +\t\t}\n>> +\t}\n>> +\n>> \t/*\n>> \t * Enumerate all sensors connected to the ISP and create one\n>> \t * camera instance for each of them.\n>> @@ -1283,7 +1391,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> \t\treturn;\n>>\n>> \tconst FrameMetadata &metadata = buffer->metadata();\n>> -\tRequest *request = buffer->request();\n>> +\tRequest *request = info->request;\n>>\n>> \tif (metadata.status != FrameMetadata::FrameCancelled) {\n>> \t\t/*\n>> @@ -1300,11 +1408,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n>> \t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n>> \t\t}\n>> +\n> rougue white line\n>\n>> \t} else {\n>> \t\tif (isRaw_)\n>> \t\t\tinfo->metadataProcessed = true;\n>> \t}\n>>\n>> +\tif (useDewarper_) {\n>> +\t\t/*\n>> +\t\t * Queue input and output buffers to the dewarper. The output\n>> +\t\t * buffers for the dewarper are the buffers of the request, supplied\n>> +\t\t * by the application.\n>> +\t\t */\n>> +\t\tint ret = dewarper_->queueBuffers(buffer, request->buffers());\n> What if buffer comes from the self path ?\n\nI am yet to become aware of a i.MX8MP platform where dewarper is present \n- along with main and self paths.\n\n>\n>> +\t\tif (ret < 0)\n>> +\t\t\tLOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n>> +\t\t\t\t\t << strerror(-ret);\n>> +\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tcompleteBuffer(request, buffer);\n>> +\ttryCompleteRequest(info);\n>> +}\n>> +\n>> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n>> +{\n>> +\tASSERT(activeCamera_);\n>> +\tRkISP1CameraData *data = cameraData(activeCamera_);\n>> +\tRequest *request = buffer->request();\n>> +\n>> +\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n>> +\tif (!info)\n>> +\t\treturn;\n>> +\n>> +\tif (info->scalerCrop)\n>> +\t\trequest->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n>> +\n>> \tcompleteBuffer(request, buffer);\n>> \ttryCompleteRequest(info);\n>> }\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> index c49017d1..c96ec1d6 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> @@ -56,7 +56,7 @@ const std::map formatToMediaBus = {\n>>\n>> RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n>> \t\t const Size &minResolution, const Size &maxResolution)\n>> -\t: name_(name), running_(false), formats_(formats),\n>> +\t: name_(name), running_(false), internalBufs_(false), formats_(formats),\n>> \t minResolution_(minResolution), maxResolution_(maxResolution),\n>> \t link_(nullptr)\n>> {\n>> @@ -402,10 +402,12 @@ int RkISP1Path::start()\n>> \tif (running_)\n>> \t\treturn -EBUSY;\n>>\n>> -\t/* \\todo Make buffer count user configurable. */\n>> -\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n>> -\tif (ret)\n>> -\t\treturn ret;\n>> +\tif (!internalBufs_) {\n> Would you still need this if you exportBuffers() from the main path ?\n>\n>> +\t\t/* \\todo Make buffer count user configurable. */\n>> +\t\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n>> +\t\tif (ret)\n>> +\t\t\treturn ret;\n>> +\t}\n>>\n>> \tret = video_->streamOn();\n>> \tif (ret) {\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> index 08edefec..b7fa82d0 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> @@ -55,6 +55,19 @@ public:\n>> \t\treturn video_->exportBuffers(bufferCount, buffers);\n>> \t}\n>>\n>> +\tint allocateBuffers(unsigned int bufferCount,\n>> +\t\t\t std::vector> *buffers)\n>> +\t{\n>> +\t\tinternalBufs_ = true;\n>> +\t\treturn video_->allocateBuffers(bufferCount, buffers);\n> What if allocateBuffers() fails ? You have sent the internalBufs_ flag\n> before this call.\n>\n>> +\t}\n>> +\n>> +\tint releaseBuffers()\n>> +\t{\n>> +\t\tASSERT(internalBufs_);\n>> +\t\treturn video_->releaseBuffers();\n>> +\t}\n>> +\n> I think you should move these to the cpp file.\n>\n>> \tint start();\n>> \tvoid stop();\n>>\n>> @@ -68,6 +81,7 @@ private:\n>>\n>> \tconst char *name_;\n>> \tbool running_;\n>> +\tbool internalBufs_;\n>>\n>> \tconst Span formats_;\n>> \tstd::set streamFormats_;\n>> --\n>> 2.45.2\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9B013C323E\n\tfor ;\n\tThu, 18 Jul 2024 05:22:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A123F6336F;\n\tThu, 18 Jul 2024 07:22:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93B1F619A0\n\tfor ;\n\tThu, 18 Jul 2024 07:22:40 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 339EEB3;\n\tThu, 18 Jul 2024 07:22:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"v6pUS/12\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721280122;\n\tbh=rtFsWnMS/tf8E6sAvumLg24vxQxRSG9JSF0I8RKkMGA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=v6pUS/12id/PR1nBK40I3hJf0z8JJwDRl45YkHEc5vazhSJhSp4jB4zj8ESX1s5G2\n\tP0K5Vsl25rwvcATO2Ra5gaM8sA0hjxJ53ej8+mQIHmGaaZRyGsfmoBvS53RCH98C27\n\tLJCavccxqAJyX52IHZFsIhsfcZbcbLnOOICNkRPU=","Message-ID":"","Date":"Thu, 18 Jul 2024 10:52:35 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-6-umang.jain@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30425,"web_url":"https://patchwork.libcamera.org/comment/30425/","msgid":"","date":"2024-07-18T09:54:05","subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Thu, Jul 18, 2024 at 10:52:35AM GMT, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 17/07/24 6:40 pm, Jacopo Mondi wrote:\n> > Hi Umang\n> >\n> > On Wed, Jul 10, 2024 at 01:51:51AM GMT, Umang Jain wrote:\n> > > Plumb the ConverterDW100 converter in the rkisp1 pipeline handler.\n> > > If the dewarper is found, it is instantiated and buffers are exported\n> > > from it, instead of RkISP1Path. Internal buffers are allocated for the\n> > > RkISP1Path in case where dewarper is going to be used.\n> > >\n> > > The RKISP1 pipeline handler now supports scaler crop control through\n> > > ConverterDW100. Register the ScalerCrop control for the cameras created\n> > > in the RKISP1 pipeline handler.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 148 +++++++++++++++++-\n> > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n> > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n> > > 3 files changed, 165 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index bfc44239..881e10e1 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -8,9 +8,11 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > > #include \n> > > @@ -33,6 +35,7 @@\n> > >\n> > > #include \"libcamera/internal/camera.h\"\n> > > #include \"libcamera/internal/camera_sensor.h\"\n> > > +#include \"libcamera/internal/converter/converter_dw100.h\"\n> > > #include \"libcamera/internal/delayed_controls.h\"\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > #include \"libcamera/internal/framebuffer.h\"\n> > > @@ -62,6 +65,8 @@ struct RkISP1FrameInfo {\n> > >\n> > > \tbool paramDequeued;\n> > > \tbool metadataProcessed;\n> > > +\n> > > +\tstd::optional scalerCrop;\n> > > };\n> > >\n> > > class RkISP1Frames\n> > > @@ -181,6 +186,7 @@ private:\n> > > \tvoid bufferReady(FrameBuffer *buffer);\n> > > \tvoid paramReady(FrameBuffer *buffer);\n> > > \tvoid statReady(FrameBuffer *buffer);\n> > > +\tvoid dewarpBufferReady(FrameBuffer *buffer);\n> > > \tvoid frameStart(uint32_t sequence);\n> > >\n> > > \tint allocateBuffers(Camera *camera);\n> > > @@ -200,6 +206,13 @@ private:\n> > > \tRkISP1MainPath mainPath_;\n> > > \tRkISP1SelfPath selfPath_;\n> > >\n> > > +\tstd::unique_ptr dewarper_;\n> > > +\tbool useDewarper_;\n> > > +\n> > > +\t/* Internal buffers used when dewarper is being used */\n> > > +\tstd::vector> mainPathBuffers_;\n> > > +\tstd::queue availableMainPathBuffers_;\n> > > +\n> > > \tstd::vector> paramBuffers_;\n> > > \tstd::vector> statBuffers_;\n> > > \tstd::queue availableParamBuffers_;\n> > > @@ -222,6 +235,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > >\n> > > \tFrameBuffer *paramBuffer = nullptr;\n> > > \tFrameBuffer *statBuffer = nullptr;\n> > > +\tFrameBuffer *mainPathBuffer = nullptr;\n> > > +\tFrameBuffer *selfPathBuffer = nullptr;\n> > >\n> > > \tif (!isRaw) {\n> > > \t\tif (pipe_->availableParamBuffers_.empty()) {\n> > > @@ -239,10 +254,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > >\n> > > \t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> > > \t\tpipe_->availableStatBuffers_.pop();\n> > > +\n> > > +\t\tif (pipe_->useDewarper_) {\n> > > +\t\t\tmainPathBuffer = pipe_->availableMainPathBuffers_.front();\n> > Can we exaust the availableMainPathBuffers_ queue? Should this be\n> > checked for validity ?\n>\n> Seems rule to be applied for all available*Buffers_ in rkisp1 pipeline\n> handler?\n>\n\nShould they all be fixed then ? :)\n\n> >\n> > > +\t\t\tpipe_->availableMainPathBuffers_.pop();\n> > > +\t\t}\n> > > \t}\n> > >\n> > > -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> > > +\tif (!mainPathBuffer)\n> > > +\t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > +\tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> > >\n> > > \tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> > >\n> > > @@ -268,6 +289,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n> > >\n> > > \tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> > > \tpipe_->availableStatBuffers_.push(info->statBuffer);\n> > > +\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> > >\n> > > \tframeInfo_.erase(info->frame);\n> > >\n> > > @@ -283,6 +305,7 @@ void RkISP1Frames::clear()\n> > >\n> > > \t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> > > \t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> > > +\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> > >\n> > > \t\tdelete info;\n> > > \t}\n> > > @@ -607,7 +630,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > */\n> > >\n> > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > -\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > +\t: PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)\n> > > {\n> > > }\n> > >\n> > > @@ -785,12 +808,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > \t\t<< \" crop \" << rect;\n> > >\n> > > \tstd::map streamConfig;\n> > > +\tstd::vector> outputCfgs;\n> > >\n> > > \tfor (const StreamConfiguration &cfg : *config) {\n> > > \t\tif (cfg.stream() == &data->mainPathStream_) {\n> > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > \t\t\t\t\t\t cfg.size);\n> > > +\t\t\t/* Configure dewarp */\n> > > +\t\t\tif (dewarper_ && !isRaw_) {\n> > > +\t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> > Why a vector, can you have more than one ?\n>\n> converter can have more than one outputs yes\n\nMaybe in the general case. Here it doesn't seem you can have more than one\nconfig that gets applied to the dewarper\n\n> >\n> > > +\t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> > Ah, the need to have a vector comes from the configure() signature\n> >\n> > Seems like\n> > \t\t\t\tret = dewarper_->configure(cfg, { cfg });\n> >\n> > would do\n>\n> const is a problem here I think\n>\n\ncompiles here, not sent through the CI loop.\n\nHave you had any compiler error with the above ?\n\n> >\n> > > +\t\t\t\tuseDewarper_ = ret ? false : true;\n> > > +\t\t\t}\n> > > \t\t} else if (hasSelfPath_) {\n> > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > @@ -839,6 +869,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > > \tRkISP1CameraData *data = cameraData(camera);\n> > > \tunsigned int count = stream->configuration().bufferCount;\n> > >\n> > > +\tif (useDewarper_)\n> > > +\t\treturn dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n> > > +\n> > Unconditionally ? Even if the stream is the selfPath one ?\n>\n> dewarper only works with mainPath_ I believe.\n\nIs it a pipeline handler design choice or an HW limitation ?\n\n>\n> The i.MX8MP doesn't have selfPath as far as I know? Paul, can you correct me\n> on this - I think we had a discussion regarding that.\n>\n\n8mp doesn't have a self path\nrkisp doesn't have a dewarper\n\nso this seems to be safe -for now-\n\n> >\n> > > \tif (stream == &data->mainPathStream_)\n> > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > \telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > @@ -866,6 +899,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > > \t\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> > > \t\tif (ret < 0)\n> > > \t\t\tgoto error;\n> > > +\n> > > +\t\t/* If the dewarper is being used, allocate internal buffers for ISP */\n> > > +\t\tif (useDewarper_) {\n> > > +\t\t\tret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n> > > +\t\t\tif (ret < 0)\n> > > +\t\t\t\tgoto error;\n> > Is this right ?\n> >\n> > It seems the model here is to allocate buffers in the main path and\n> > import them in the dewarper.\n>\n> The model here is to simply export the buffers from dewarper and allocate\n> internal buffers for RkISP1Path\n\nYes and I wonder if it is correct.\n\nI'm not talking about buffers exported from the dewarper to the\napplication (the one created by exportFrameBuffers() which only serves\nfor the FrameBufferAllocator to provide FrameBuffers from the\napplication).\n\nI'm talking about the buffers between the main path and the dewarper.\n\n>\n> These internal buffers allocated (availableMainPathBuffers_), are then\n> queued to the dewarper.\n>\n> >\n> > allocateBuffer() creates and exports the buffers but sets the video\n> > device in MMAP mode.\n> >\n> > I would be expecting instead the main path to\n> > 1) export buffers\n> > 2) import them back to intialize the queue in DMABUF mode\n> >\n> > From there buffers dequeued from the main path are queued to the\n> > dewarper without any need to be mapped into a userspace accesible\n> > address.\n> >\n> > Am I confused maybe ?\n>\n> Now I'm confused with the above strategy. Do you mean that export buffers\n> from mainPath and have internal buffers allocated in the dewarper?\n\nI mean allocating memory in the main path and pass them to the\ndewarper as dmabufs, which is what allocateBuffers() does, but\ninitializes the queue in MMAP mode, where you would only need DMABUF.\n\n>\n> When you say,\n>\n> 2) import them back to intialize the queue in DMABUF mode\n>\n> Import them back where?\n\nIn the mainpath itself. See how the CIO2 does that.\n\nint CIO2Device::start()\n{\n\tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n\tif (ret < 0)\n\t\treturn ret;\n\n\tret = output_->importBuffers(kBufferCount);\n\tif (ret)\n\t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n\n\nThe main idea is that both allocateBuffers() and exportBuffers()\nactually allocate memory on the device on which those functions are\ncalled. The difference is the memory type used by the videobuf2 queue.\nimportBuffers() instead does not allocate memory on the device but\ninitializes its memory type to DMABUF and prepares it to receive\nqueueBuffer() calls.\n\nWith allocateBuffers() the memory type is set to MMAP, with\nexportBuffers() is not initialized, so you have to later call\neither allocateBuffers() or importBuffers() to initialize it.\n\nAs V4L2M2MConverter import buffers at start() time\n\nint V4L2M2MConverter::V4L2M2MStream::start()\n{\n\tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n\tif (ret < 0)\n\t\treturn ret;\n\n\tret = m2m_->capture()->importBuffers(outputBufferCount_);\n\tif (ret < 0) {\n\t\tstop();\n\t\treturn ret;\n\t}\n\nI wonder if it is necessary to set the mainpath in MMAP mode or it can\noperate in dmabuf mode only by combining exportBuffers() +\nimportBuffers(). To be honest the implications in videobuf2 of using\none memory type or the other are not 100% clear to me yet.\n\nReading the V4L2VideoDevice documentation, however, allocateBuffers()\nseems to be the preferred way to implement internal buffer pools\nbetween video devices, so maybe I'm totally off-track here\n\n *\n * The V4L2VideoDevice class supports the V4L2 MMAP and DMABUF memory types:\n *\n * - The allocateBuffers() function wraps buffer allocation with the V4L2 MMAP\n * memory type. It requests buffers from the driver, allocating the\n * corresponding memory, and exports them as a set of FrameBuffer objects.\n * Upon successful return the driver's internal buffer management is\n * initialized in MMAP mode, and the video device is ready to accept\n * queueBuffer() calls.\n *\n * This is the most traditional V4L2 buffer management, and is mostly useful\n * to support internal buffer pools in pipeline handlers, either for CPU\n * consumption (such as statistics or parameters pools), or for internal\n * image buffers shared between devices.\n *\n * - The exportBuffers() function operates similarly to allocateBuffers(), but\n * leaves the driver's internal buffer management uninitialized. It uses the\n * V4L2 buffer orphaning support to allocate buffers with the MMAP method,\n * export them as a set of FrameBuffer objects, and reset the driver's\n * internal buffer management. The video device shall be initialized with\n * importBuffers() or allocateBuffers() before it can accept queueBuffer()\n * calls. The exported buffers are directly usable with any V4L2 video device\n * in DMABUF mode, or with other dmabuf importers.\n *\n * This method is mostly useful to implement buffer allocation helpers or to\n * allocate ancillary buffers, when a V4L2 video device is used in DMABUF\n * mode but no other source of buffers is available. An example use case\n * would be allocation of scratch buffers to be used in case of buffer\n * underruns on a video device that is otherwise supplied with external\n * buffers.\n *\n * - The importBuffers() function initializes the driver's buffer management to\n * import buffers in DMABUF mode. It requests buffers from the driver, but\n * doesn't allocate memory. Upon successful return, the video device is ready\n * to accept queueBuffer() calls. The buffers to be imported are provided to\n * queueBuffer(), and may be supplied externally, or come from a previous\n * exportBuffers() call.\n *\n * This is the usual buffers initialization method for video devices whose\n * buffers are exposed outside of libcamera. It is also typically used on one\n * of the two video device that participate in buffer sharing inside\n * pipelines, the other video device typically using allocateBuffers().\n *\n\n>\n> But certainly it's a worth while idea to have them exported from main path\n> and also queue to it the dewarper. Would require some experimentation from\n> my side.\n>\n> >\n> >\n> > > +\n> > > +\t\t\tfor (std::unique_ptr &buffer : mainPathBuffers_)\n> > > +\t\t\t\tavailableMainPathBuffers_.push(buffer.get());\n> > > +\t\t}\n> > > \t}\n> > >\n> > > \tfor (std::unique_ptr &buffer : paramBuffers_) {\n> > > @@ -889,6 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > > error:\n> > > \tparamBuffers_.clear();\n> > > \tstatBuffers_.clear();\n> > > +\tmainPathBuffers_.clear();\n> > >\n> > > \treturn ret;\n> > > }\n> > > @@ -903,8 +947,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> > > \twhile (!availableParamBuffers_.empty())\n> > > \t\tavailableParamBuffers_.pop();\n> > >\n> > > +\twhile (!availableMainPathBuffers_.empty())\n> > > +\t\tavailableMainPathBuffers_.pop();\n> > > +\n> > > \tparamBuffers_.clear();\n> > > \tstatBuffers_.clear();\n> > > +\tmainPathBuffers_.clear();\n> > >\n> > > \tstd::vector ids;\n> > > \tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n> > > @@ -919,6 +967,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> > > \tif (stat_->releaseBuffers())\n> > > \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> > >\n> > > +\tif (useDewarper_)\n> > > +\t\tmainPath_.releaseBuffers();\n> > > +\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -961,6 +1012,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > \t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > \t\t\treturn ret;\n> > > \t\t}\n> > > +\n> > > +\t\tif (useDewarper_) {\n> > > +\t\t\tret = dewarper_->start();\n> > > +\t\t\tif (ret) {\n> > > +\t\t\t\tLOG(RkISP1, Error) << \"Failed to start dewarper\";\n> > The error handling path should undo all the previous actions\n> >\n> > > +\t\t\t\treturn ret;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > \t}\n> > >\n> > > \tif (data->mainPath_->isEnabled()) {\n> > > @@ -1015,6 +1074,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > \t\tif (ret)\n> > > \t\t\tLOG(RkISP1, Warning)\n> > > \t\t\t\t<< \"Failed to stop parameters for \" << camera->id();\n> > > +\n> > > +\t\tif (useDewarper_)\n> > > +\t\t\tdewarper_->stop();\n> > > \t}\n> > >\n> > > \tASSERT(data->queuedRequests_.empty());\n> > > @@ -1045,6 +1107,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > \t\t\t\t\t info->paramBuffer->cookie());\n> > > \t}\n> > >\n> > > +\tconst auto &crop = request->controls().get(controls::ScalerCrop);\n> > > +\tif (crop && useDewarper_) {\n> > > +\t\tRectangle appliedRect = crop.value();\n> > > +\t\tint ret = dewarper_->setCrop(&data->mainPathStream_, &appliedRect);\n> > > +\t\tif (!ret) {\n> > > +\t\t\tif (appliedRect != crop.value()) {\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t * \\todo How to handle these case?\n> > > +\t\t\t\t * Do we aim for pixel perfect set rectangles?\n> > > +\t\t\t\t */\n> > I think this should be reported in metadata as it has been applied and\n> > let the application decide what to do there\n>\n> ack\n> >\n> > > +\t\t\t\tLOG(RkISP1, Warning)\n> > > +\t\t\t\t\t<< \"Applied rectangle \" << appliedRect.toString()\n> > > +\t\t\t\t\t<< \" differs from requested \" << crop.value().toString();\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tinfo->scalerCrop = appliedRect;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > \tdata->frame_++;\n> > >\n> > > \treturn 0;\n> > > @@ -1110,6 +1191,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> > > {\n> > > \tControlInfoMap::Map rkisp1Controls;\n> > >\n> > > +\tif (dewarper_) {\n> > > +\t\tauto [minCrop, maxCrop] = dewarper_->getCropBounds(&data->mainPathStream_);\n> > > +\n> > > +\t\trkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > > +\t}\n> > > +\n> > > \t/* Add the IPA registered controls to list of camera controls. */\n> > > \tfor (const auto &ipaControl : data->ipaControls_)\n> > > \t\trkisp1Controls[ipaControl.first] = ipaControl.second;\n> > > @@ -1173,6 +1260,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > >\n> > > bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > {\n> > > +\tstd::shared_ptr dwpMediaDevice;\n> > > \tconst MediaPad *pad;\n> > >\n> > > \tDeviceMatch dm(\"rkisp1\");\n> > > @@ -1237,6 +1325,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > >\n> > > +\t/* If dewarper is present, create its instance. */\n> > > +\tDeviceMatch dwp(\"dw100\");\n> > > +\tdwp.add(\"dw100-source\");\n> > > +\tdwp.add(\"dw100-sink\");\n> > > +\n> > > +\tdwpMediaDevice = enumerator->search(dwp);\n> > > +\tif (dwpMediaDevice) {\n> > > +\t\tdewarper_ = std::make_unique(std::move(dwpMediaDevice));\n> > > +\t\tif (dewarper_->isValid()) {\n> > > +\t\t\tdewarper_->outputBufferReady.connect(\n> > > +\t\t\t\tthis, &PipelineHandlerRkISP1::dewarpBufferReady);\n> > > +\n> > > +\t\t\tLOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n> > > +\t\t} else {\n> > > +\t\t\tLOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n> > > +\t\t\t\t\t << \" but invalid\";\n> > > +\t\t\tdewarper_.reset();\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > \t/*\n> > > \t * Enumerate all sensors connected to the ISP and create one\n> > > \t * camera instance for each of them.\n> > > @@ -1283,7 +1391,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > \t\treturn;\n> > >\n> > > \tconst FrameMetadata &metadata = buffer->metadata();\n> > > -\tRequest *request = buffer->request();\n> > > +\tRequest *request = info->request;\n> > >\n> > > \tif (metadata.status != FrameMetadata::FrameCancelled) {\n> > > \t\t/*\n> > > @@ -1300,11 +1408,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> > > \t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > > \t\t}\n> > > +\n> > rougue white line\n> >\n> > > \t} else {\n> > > \t\tif (isRaw_)\n> > > \t\t\tinfo->metadataProcessed = true;\n> > > \t}\n> > >\n> > > +\tif (useDewarper_) {\n> > > +\t\t/*\n> > > +\t\t * Queue input and output buffers to the dewarper. The output\n> > > +\t\t * buffers for the dewarper are the buffers of the request, supplied\n> > > +\t\t * by the application.\n> > > +\t\t */\n> > > +\t\tint ret = dewarper_->queueBuffers(buffer, request->buffers());\n> > What if buffer comes from the self path ?\n>\n> I am yet to become aware of a i.MX8MP platform where dewarper is present -\n> along with main and self paths.\n>\n\nOk, as said above if 8mp has no self path and rkisp1 has no dewarper\n(but a self path) we're safe -for now-\n\n> >\n> > > +\t\tif (ret < 0)\n> > > +\t\t\tLOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n> > > +\t\t\t\t\t << strerror(-ret);\n> > > +\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tcompleteBuffer(request, buffer);\n> > > +\ttryCompleteRequest(info);\n> > > +}\n> > > +\n> > > +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> > > +{\n> > > +\tASSERT(activeCamera_);\n> > > +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > > +\tRequest *request = buffer->request();\n> > > +\n> > > +\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> > > +\tif (!info)\n> > > +\t\treturn;\n> > > +\n> > > +\tif (info->scalerCrop)\n> > > +\t\trequest->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n> > > +\n> > > \tcompleteBuffer(request, buffer);\n> > > \ttryCompleteRequest(info);\n> > > }\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > index c49017d1..c96ec1d6 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > @@ -56,7 +56,7 @@ const std::map formatToMediaBus = {\n> > >\n> > > RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> > > \t\t const Size &minResolution, const Size &maxResolution)\n> > > -\t: name_(name), running_(false), formats_(formats),\n> > > +\t: name_(name), running_(false), internalBufs_(false), formats_(formats),\n> > > \t minResolution_(minResolution), maxResolution_(maxResolution),\n> > > \t link_(nullptr)\n> > > {\n> > > @@ -402,10 +402,12 @@ int RkISP1Path::start()\n> > > \tif (running_)\n> > > \t\treturn -EBUSY;\n> > >\n> > > -\t/* \\todo Make buffer count user configurable. */\n> > > -\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> > > -\tif (ret)\n> > > -\t\treturn ret;\n> > > +\tif (!internalBufs_) {\n> > Would you still need this if you exportBuffers() from the main path ?\n> >\n> > > +\t\t/* \\todo Make buffer count user configurable. */\n> > > +\t\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\t}\n> > >\n> > > \tret = video_->streamOn();\n> > > \tif (ret) {\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > index 08edefec..b7fa82d0 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > @@ -55,6 +55,19 @@ public:\n> > > \t\treturn video_->exportBuffers(bufferCount, buffers);\n> > > \t}\n> > >\n> > > +\tint allocateBuffers(unsigned int bufferCount,\n> > > +\t\t\t std::vector> *buffers)\n> > > +\t{\n> > > +\t\tinternalBufs_ = true;\n> > > +\t\treturn video_->allocateBuffers(bufferCount, buffers);\n> > What if allocateBuffers() fails ? You have sent the internalBufs_ flag\n> > before this call.\n> >\n> > > +\t}\n> > > +\n> > > +\tint releaseBuffers()\n> > > +\t{\n> > > +\t\tASSERT(internalBufs_);\n> > > +\t\treturn video_->releaseBuffers();\n> > > +\t}\n> > > +\n> > I think you should move these to the cpp file.\n> >\n> > > \tint start();\n> > > \tvoid stop();\n> > >\n> > > @@ -68,6 +81,7 @@ private:\n> > >\n> > > \tconst char *name_;\n> > > \tbool running_;\n> > > +\tbool internalBufs_;\n> > >\n> > > \tconst Span formats_;\n> > > \tstd::set streamFormats_;\n> > > --\n> > > 2.45.2\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6CF4EBDB1C\n\tfor ;\n\tThu, 18 Jul 2024 09:54:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 946396336F;\n\tThu, 18 Jul 2024 11:54:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06CE7619BD\n\tfor ;\n\tThu, 18 Jul 2024 11:54:09 +0200 (CEST)","from ideasonboard.com (unknown [91.80.77.148])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 109A0766;\n\tThu, 18 Jul 2024 11:53:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oGB/SQCp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721296411;\n\tbh=Lkh9aqTHx0+nR2rohjVYZ0bhSUHdTYdgLZR88OgUjYg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oGB/SQCpk0Nf7yeFt36CLLcLi7WZLRyRjWu3TSFujlAPhnVnujRJo4IMWluivLR52\n\tzDeNbZ7KS/HgbgFTDezqTSdKVgolcIoKRzNX+fBYdj0fkD1ZQv2g63pKIYUU0HODIe\n\tkVXuFU8iKCSUaOFm/zADQ5IEqC+V8Zd4J8vbu5YY=","Date":"Thu, 18 Jul 2024 11:54:05 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-6-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30432,"web_url":"https://patchwork.libcamera.org/comment/30432/","msgid":"","date":"2024-07-19T07:11:51","subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jul 18, 2024 at 11:54:05AM +0200, Jacopo Mondi wrote:\n> Hi Umang\n> \n> On Thu, Jul 18, 2024 at 10:52:35AM GMT, Umang Jain wrote:\n> > Hi Jacopo\n> >\n> > On 17/07/24 6:40 pm, Jacopo Mondi wrote:\n> > > Hi Umang\n> > >\n> > > On Wed, Jul 10, 2024 at 01:51:51AM GMT, Umang Jain wrote:\n> > > > Plumb the ConverterDW100 converter in the rkisp1 pipeline handler.\n> > > > If the dewarper is found, it is instantiated and buffers are exported\n> > > > from it, instead of RkISP1Path. Internal buffers are allocated for the\n> > > > RkISP1Path in case where dewarper is going to be used.\n> > > >\n> > > > The RKISP1 pipeline handler now supports scaler crop control through\n> > > > ConverterDW100. Register the ScalerCrop control for the cameras created\n> > > > in the RKISP1 pipeline handler.\n> > > >\n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 148 +++++++++++++++++-\n> > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n> > > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n> > > > 3 files changed, 165 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index bfc44239..881e10e1 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -8,9 +8,11 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \n> > > > #include \n> > > > @@ -33,6 +35,7 @@\n> > > >\n> > > > #include \"libcamera/internal/camera.h\"\n> > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > +#include \"libcamera/internal/converter/converter_dw100.h\"\n> > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > #include \"libcamera/internal/framebuffer.h\"\n> > > > @@ -62,6 +65,8 @@ struct RkISP1FrameInfo {\n> > > >\n> > > > \tbool paramDequeued;\n> > > > \tbool metadataProcessed;\n> > > > +\n> > > > +\tstd::optional scalerCrop;\n> > > > };\n> > > >\n> > > > class RkISP1Frames\n> > > > @@ -181,6 +186,7 @@ private:\n> > > > \tvoid bufferReady(FrameBuffer *buffer);\n> > > > \tvoid paramReady(FrameBuffer *buffer);\n> > > > \tvoid statReady(FrameBuffer *buffer);\n> > > > +\tvoid dewarpBufferReady(FrameBuffer *buffer);\n> > > > \tvoid frameStart(uint32_t sequence);\n> > > >\n> > > > \tint allocateBuffers(Camera *camera);\n> > > > @@ -200,6 +206,13 @@ private:\n> > > > \tRkISP1MainPath mainPath_;\n> > > > \tRkISP1SelfPath selfPath_;\n> > > >\n> > > > +\tstd::unique_ptr dewarper_;\n> > > > +\tbool useDewarper_;\n> > > > +\n> > > > +\t/* Internal buffers used when dewarper is being used */\n> > > > +\tstd::vector> mainPathBuffers_;\n> > > > +\tstd::queue availableMainPathBuffers_;\n> > > > +\n> > > > \tstd::vector> paramBuffers_;\n> > > > \tstd::vector> statBuffers_;\n> > > > \tstd::queue availableParamBuffers_;\n> > > > @@ -222,6 +235,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > > >\n> > > > \tFrameBuffer *paramBuffer = nullptr;\n> > > > \tFrameBuffer *statBuffer = nullptr;\n> > > > +\tFrameBuffer *mainPathBuffer = nullptr;\n> > > > +\tFrameBuffer *selfPathBuffer = nullptr;\n> > > >\n> > > > \tif (!isRaw) {\n> > > > \t\tif (pipe_->availableParamBuffers_.empty()) {\n> > > > @@ -239,10 +254,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > > >\n> > > > \t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> > > > \t\tpipe_->availableStatBuffers_.pop();\n> > > > +\n> > > > +\t\tif (pipe_->useDewarper_) {\n> > > > +\t\t\tmainPathBuffer = pipe_->availableMainPathBuffers_.front();\n> > > Can we exaust the availableMainPathBuffers_ queue? Should this be\n> > > checked for validity ?\n> >\n> > Seems rule to be applied for all available*Buffers_ in rkisp1 pipeline\n> > handler?\n> >\n> \n> Should they all be fixed then ? :)\n> \n> > >\n> > > > +\t\t\tpipe_->availableMainPathBuffers_.pop();\n> > > > +\t\t}\n> > > > \t}\n> > > >\n> > > > -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > > -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> > > > +\tif (!mainPathBuffer)\n> > > > +\t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > > +\tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> > > >\n> > > > \tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> > > >\n> > > > @@ -268,6 +289,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n> > > >\n> > > > \tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> > > > \tpipe_->availableStatBuffers_.push(info->statBuffer);\n> > > > +\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> > > >\n> > > > \tframeInfo_.erase(info->frame);\n> > > >\n> > > > @@ -283,6 +305,7 @@ void RkISP1Frames::clear()\n> > > >\n> > > > \t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> > > > \t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> > > > +\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> > > >\n> > > > \t\tdelete info;\n> > > > \t}\n> > > > @@ -607,7 +630,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > */\n> > > >\n> > > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > > -\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > > +\t: PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -785,12 +808,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > > \t\t<< \" crop \" << rect;\n> > > >\n> > > > \tstd::map streamConfig;\n> > > > +\tstd::vector> outputCfgs;\n> > > >\n> > > > \tfor (const StreamConfiguration &cfg : *config) {\n> > > > \t\tif (cfg.stream() == &data->mainPathStream_) {\n> > > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > > \t\t\t\t\t\t cfg.size);\n> > > > +\t\t\t/* Configure dewarp */\n> > > > +\t\t\tif (dewarper_ && !isRaw_) {\n> > > > +\t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> > > Why a vector, can you have more than one ?\n> >\n> > converter can have more than one outputs yes\n> \n> Maybe in the general case. Here it doesn't seem you can have more than one\n> config that gets applied to the dewarper\n> \n> > >\n> > > > +\t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> > > Ah, the need to have a vector comes from the configure() signature\n> > >\n> > > Seems like\n> > > \t\t\t\tret = dewarper_->configure(cfg, { cfg });\n> > >\n> > > would do\n> >\n> > const is a problem here I think\n> >\n> \n> compiles here, not sent through the CI loop.\n> \n> Have you had any compiler error with the above ?\n> \n> > >\n> > > > +\t\t\t\tuseDewarper_ = ret ? false : true;\n> > > > +\t\t\t}\n> > > > \t\t} else if (hasSelfPath_) {\n> > > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > > @@ -839,6 +869,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > \tunsigned int count = stream->configuration().bufferCount;\n> > > >\n> > > > +\tif (useDewarper_)\n> > > > +\t\treturn dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n> > > > +\n> > > Unconditionally ? Even if the stream is the selfPath one ?\n> >\n> > dewarper only works with mainPath_ I believe.\n> \n> Is it a pipeline handler design choice or an HW limitation ?\n\nAll devices that have a dewarper only have a main path (so far, at least).\n\n> \n> >\n> > The i.MX8MP doesn't have selfPath as far as I know? Paul, can you correct me\n> > on this - I think we had a discussion regarding that.\n\nYeah that's right.\n\n> >\n> \n> 8mp doesn't have a self path\n> rkisp doesn't have a dewarper\n> \n> so this seems to be safe -for now-\n\nYeah.\n\n\nPaul\n\n> \n> > >\n> > > > \tif (stream == &data->mainPathStream_)\n> > > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > > \telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > > @@ -866,6 +899,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > > > \t\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> > > > \t\tif (ret < 0)\n> > > > \t\t\tgoto error;\n> > > > +\n> > > > +\t\t/* If the dewarper is being used, allocate internal buffers for ISP */\n> > > > +\t\tif (useDewarper_) {\n> > > > +\t\t\tret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n> > > > +\t\t\tif (ret < 0)\n> > > > +\t\t\t\tgoto error;\n> > > Is this right ?\n> > >\n> > > It seems the model here is to allocate buffers in the main path and\n> > > import them in the dewarper.\n> >\n> > The model here is to simply export the buffers from dewarper and allocate\n> > internal buffers for RkISP1Path\n> \n> Yes and I wonder if it is correct.\n> \n> I'm not talking about buffers exported from the dewarper to the\n> application (the one created by exportFrameBuffers() which only serves\n> for the FrameBufferAllocator to provide FrameBuffers from the\n> application).\n> \n> I'm talking about the buffers between the main path and the dewarper.\n> \n> >\n> > These internal buffers allocated (availableMainPathBuffers_), are then\n> > queued to the dewarper.\n> >\n> > >\n> > > allocateBuffer() creates and exports the buffers but sets the video\n> > > device in MMAP mode.\n> > >\n> > > I would be expecting instead the main path to\n> > > 1) export buffers\n> > > 2) import them back to intialize the queue in DMABUF mode\n> > >\n> > > From there buffers dequeued from the main path are queued to the\n> > > dewarper without any need to be mapped into a userspace accesible\n> > > address.\n> > >\n> > > Am I confused maybe ?\n> >\n> > Now I'm confused with the above strategy. Do you mean that export buffers\n> > from mainPath and have internal buffers allocated in the dewarper?\n> \n> I mean allocating memory in the main path and pass them to the\n> dewarper as dmabufs, which is what allocateBuffers() does, but\n> initializes the queue in MMAP mode, where you would only need DMABUF.\n> \n> >\n> > When you say,\n> >\n> > 2) import them back to intialize the queue in DMABUF mode\n> >\n> > Import them back where?\n> \n> In the mainpath itself. See how the CIO2 does that.\n> \n> int CIO2Device::start()\n> {\n> \tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> \tret = output_->importBuffers(kBufferCount);\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n> \n> \n> The main idea is that both allocateBuffers() and exportBuffers()\n> actually allocate memory on the device on which those functions are\n> called. The difference is the memory type used by the videobuf2 queue.\n> importBuffers() instead does not allocate memory on the device but\n> initializes its memory type to DMABUF and prepares it to receive\n> queueBuffer() calls.\n> \n> With allocateBuffers() the memory type is set to MMAP, with\n> exportBuffers() is not initialized, so you have to later call\n> either allocateBuffers() or importBuffers() to initialize it.\n> \n> As V4L2M2MConverter import buffers at start() time\n> \n> int V4L2M2MConverter::V4L2M2MStream::start()\n> {\n> \tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> \tret = m2m_->capture()->importBuffers(outputBufferCount_);\n> \tif (ret < 0) {\n> \t\tstop();\n> \t\treturn ret;\n> \t}\n> \n> I wonder if it is necessary to set the mainpath in MMAP mode or it can\n> operate in dmabuf mode only by combining exportBuffers() +\n> importBuffers(). To be honest the implications in videobuf2 of using\n> one memory type or the other are not 100% clear to me yet.\n> \n> Reading the V4L2VideoDevice documentation, however, allocateBuffers()\n> seems to be the preferred way to implement internal buffer pools\n> between video devices, so maybe I'm totally off-track here\n> \n> *\n> * The V4L2VideoDevice class supports the V4L2 MMAP and DMABUF memory types:\n> *\n> * - The allocateBuffers() function wraps buffer allocation with the V4L2 MMAP\n> * memory type. It requests buffers from the driver, allocating the\n> * corresponding memory, and exports them as a set of FrameBuffer objects.\n> * Upon successful return the driver's internal buffer management is\n> * initialized in MMAP mode, and the video device is ready to accept\n> * queueBuffer() calls.\n> *\n> * This is the most traditional V4L2 buffer management, and is mostly useful\n> * to support internal buffer pools in pipeline handlers, either for CPU\n> * consumption (such as statistics or parameters pools), or for internal\n> * image buffers shared between devices.\n> *\n> * - The exportBuffers() function operates similarly to allocateBuffers(), but\n> * leaves the driver's internal buffer management uninitialized. It uses the\n> * V4L2 buffer orphaning support to allocate buffers with the MMAP method,\n> * export them as a set of FrameBuffer objects, and reset the driver's\n> * internal buffer management. The video device shall be initialized with\n> * importBuffers() or allocateBuffers() before it can accept queueBuffer()\n> * calls. The exported buffers are directly usable with any V4L2 video device\n> * in DMABUF mode, or with other dmabuf importers.\n> *\n> * This method is mostly useful to implement buffer allocation helpers or to\n> * allocate ancillary buffers, when a V4L2 video device is used in DMABUF\n> * mode but no other source of buffers is available. An example use case\n> * would be allocation of scratch buffers to be used in case of buffer\n> * underruns on a video device that is otherwise supplied with external\n> * buffers.\n> *\n> * - The importBuffers() function initializes the driver's buffer management to\n> * import buffers in DMABUF mode. It requests buffers from the driver, but\n> * doesn't allocate memory. Upon successful return, the video device is ready\n> * to accept queueBuffer() calls. The buffers to be imported are provided to\n> * queueBuffer(), and may be supplied externally, or come from a previous\n> * exportBuffers() call.\n> *\n> * This is the usual buffers initialization method for video devices whose\n> * buffers are exposed outside of libcamera. It is also typically used on one\n> * of the two video device that participate in buffer sharing inside\n> * pipelines, the other video device typically using allocateBuffers().\n> *\n> \n> >\n> > But certainly it's a worth while idea to have them exported from main path\n> > and also queue to it the dewarper. Would require some experimentation from\n> > my side.\n> >\n> > >\n> > >\n> > > > +\n> > > > +\t\t\tfor (std::unique_ptr &buffer : mainPathBuffers_)\n> > > > +\t\t\t\tavailableMainPathBuffers_.push(buffer.get());\n> > > > +\t\t}\n> > > > \t}\n> > > >\n> > > > \tfor (std::unique_ptr &buffer : paramBuffers_) {\n> > > > @@ -889,6 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > > > error:\n> > > > \tparamBuffers_.clear();\n> > > > \tstatBuffers_.clear();\n> > > > +\tmainPathBuffers_.clear();\n> > > >\n> > > > \treturn ret;\n> > > > }\n> > > > @@ -903,8 +947,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> > > > \twhile (!availableParamBuffers_.empty())\n> > > > \t\tavailableParamBuffers_.pop();\n> > > >\n> > > > +\twhile (!availableMainPathBuffers_.empty())\n> > > > +\t\tavailableMainPathBuffers_.pop();\n> > > > +\n> > > > \tparamBuffers_.clear();\n> > > > \tstatBuffers_.clear();\n> > > > +\tmainPathBuffers_.clear();\n> > > >\n> > > > \tstd::vector ids;\n> > > > \tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n> > > > @@ -919,6 +967,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> > > > \tif (stat_->releaseBuffers())\n> > > > \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> > > >\n> > > > +\tif (useDewarper_)\n> > > > +\t\tmainPath_.releaseBuffers();\n> > > > +\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > @@ -961,6 +1012,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > \t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > > \t\t\treturn ret;\n> > > > \t\t}\n> > > > +\n> > > > +\t\tif (useDewarper_) {\n> > > > +\t\t\tret = dewarper_->start();\n> > > > +\t\t\tif (ret) {\n> > > > +\t\t\t\tLOG(RkISP1, Error) << \"Failed to start dewarper\";\n> > > The error handling path should undo all the previous actions\n> > >\n> > > > +\t\t\t\treturn ret;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > \t}\n> > > >\n> > > > \tif (data->mainPath_->isEnabled()) {\n> > > > @@ -1015,6 +1074,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > > \t\tif (ret)\n> > > > \t\t\tLOG(RkISP1, Warning)\n> > > > \t\t\t\t<< \"Failed to stop parameters for \" << camera->id();\n> > > > +\n> > > > +\t\tif (useDewarper_)\n> > > > +\t\t\tdewarper_->stop();\n> > > > \t}\n> > > >\n> > > > \tASSERT(data->queuedRequests_.empty());\n> > > > @@ -1045,6 +1107,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > > \t\t\t\t\t info->paramBuffer->cookie());\n> > > > \t}\n> > > >\n> > > > +\tconst auto &crop = request->controls().get(controls::ScalerCrop);\n> > > > +\tif (crop && useDewarper_) {\n> > > > +\t\tRectangle appliedRect = crop.value();\n> > > > +\t\tint ret = dewarper_->setCrop(&data->mainPathStream_, &appliedRect);\n> > > > +\t\tif (!ret) {\n> > > > +\t\t\tif (appliedRect != crop.value()) {\n> > > > +\t\t\t\t/*\n> > > > +\t\t\t\t * \\todo How to handle these case?\n> > > > +\t\t\t\t * Do we aim for pixel perfect set rectangles?\n> > > > +\t\t\t\t */\n> > > I think this should be reported in metadata as it has been applied and\n> > > let the application decide what to do there\n> >\n> > ack\n> > >\n> > > > +\t\t\t\tLOG(RkISP1, Warning)\n> > > > +\t\t\t\t\t<< \"Applied rectangle \" << appliedRect.toString()\n> > > > +\t\t\t\t\t<< \" differs from requested \" << crop.value().toString();\n> > > > +\t\t\t}\n> > > > +\n> > > > +\t\t\tinfo->scalerCrop = appliedRect;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > \tdata->frame_++;\n> > > >\n> > > > \treturn 0;\n> > > > @@ -1110,6 +1191,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> > > > {\n> > > > \tControlInfoMap::Map rkisp1Controls;\n> > > >\n> > > > +\tif (dewarper_) {\n> > > > +\t\tauto [minCrop, maxCrop] = dewarper_->getCropBounds(&data->mainPathStream_);\n> > > > +\n> > > > +\t\trkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > > > +\t}\n> > > > +\n> > > > \t/* Add the IPA registered controls to list of camera controls. */\n> > > > \tfor (const auto &ipaControl : data->ipaControls_)\n> > > > \t\trkisp1Controls[ipaControl.first] = ipaControl.second;\n> > > > @@ -1173,6 +1260,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > >\n> > > > bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > {\n> > > > +\tstd::shared_ptr dwpMediaDevice;\n> > > > \tconst MediaPad *pad;\n> > > >\n> > > > \tDeviceMatch dm(\"rkisp1\");\n> > > > @@ -1237,6 +1325,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > > >\n> > > > +\t/* If dewarper is present, create its instance. */\n> > > > +\tDeviceMatch dwp(\"dw100\");\n> > > > +\tdwp.add(\"dw100-source\");\n> > > > +\tdwp.add(\"dw100-sink\");\n> > > > +\n> > > > +\tdwpMediaDevice = enumerator->search(dwp);\n> > > > +\tif (dwpMediaDevice) {\n> > > > +\t\tdewarper_ = std::make_unique(std::move(dwpMediaDevice));\n> > > > +\t\tif (dewarper_->isValid()) {\n> > > > +\t\t\tdewarper_->outputBufferReady.connect(\n> > > > +\t\t\t\tthis, &PipelineHandlerRkISP1::dewarpBufferReady);\n> > > > +\n> > > > +\t\t\tLOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n> > > > +\t\t} else {\n> > > > +\t\t\tLOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n> > > > +\t\t\t\t\t << \" but invalid\";\n> > > > +\t\t\tdewarper_.reset();\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > \t/*\n> > > > \t * Enumerate all sensors connected to the ISP and create one\n> > > > \t * camera instance for each of them.\n> > > > @@ -1283,7 +1391,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > > \t\treturn;\n> > > >\n> > > > \tconst FrameMetadata &metadata = buffer->metadata();\n> > > > -\tRequest *request = buffer->request();\n> > > > +\tRequest *request = info->request;\n> > > >\n> > > > \tif (metadata.status != FrameMetadata::FrameCancelled) {\n> > > > \t\t/*\n> > > > @@ -1300,11 +1408,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > > \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> > > > \t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > > > \t\t}\n> > > > +\n> > > rougue white line\n> > >\n> > > > \t} else {\n> > > > \t\tif (isRaw_)\n> > > > \t\t\tinfo->metadataProcessed = true;\n> > > > \t}\n> > > >\n> > > > +\tif (useDewarper_) {\n> > > > +\t\t/*\n> > > > +\t\t * Queue input and output buffers to the dewarper. The output\n> > > > +\t\t * buffers for the dewarper are the buffers of the request, supplied\n> > > > +\t\t * by the application.\n> > > > +\t\t */\n> > > > +\t\tint ret = dewarper_->queueBuffers(buffer, request->buffers());\n> > > What if buffer comes from the self path ?\n> >\n> > I am yet to become aware of a i.MX8MP platform where dewarper is present -\n> > along with main and self paths.\n> >\n> \n> Ok, as said above if 8mp has no self path and rkisp1 has no dewarper\n> (but a self path) we're safe -for now-\n> \n> > >\n> > > > +\t\tif (ret < 0)\n> > > > +\t\t\tLOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n> > > > +\t\t\t\t\t << strerror(-ret);\n> > > > +\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > > > +\tcompleteBuffer(request, buffer);\n> > > > +\ttryCompleteRequest(info);\n> > > > +}\n> > > > +\n> > > > +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> > > > +{\n> > > > +\tASSERT(activeCamera_);\n> > > > +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > > > +\tRequest *request = buffer->request();\n> > > > +\n> > > > +\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> > > > +\tif (!info)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tif (info->scalerCrop)\n> > > > +\t\trequest->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n> > > > +\n> > > > \tcompleteBuffer(request, buffer);\n> > > > \ttryCompleteRequest(info);\n> > > > }\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > index c49017d1..c96ec1d6 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > @@ -56,7 +56,7 @@ const std::map formatToMediaBus = {\n> > > >\n> > > > RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> > > > \t\t const Size &minResolution, const Size &maxResolution)\n> > > > -\t: name_(name), running_(false), formats_(formats),\n> > > > +\t: name_(name), running_(false), internalBufs_(false), formats_(formats),\n> > > > \t minResolution_(minResolution), maxResolution_(maxResolution),\n> > > > \t link_(nullptr)\n> > > > {\n> > > > @@ -402,10 +402,12 @@ int RkISP1Path::start()\n> > > > \tif (running_)\n> > > > \t\treturn -EBUSY;\n> > > >\n> > > > -\t/* \\todo Make buffer count user configurable. */\n> > > > -\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> > > > -\tif (ret)\n> > > > -\t\treturn ret;\n> > > > +\tif (!internalBufs_) {\n> > > Would you still need this if you exportBuffers() from the main path ?\n> > >\n> > > > +\t\t/* \\todo Make buffer count user configurable. */\n> > > > +\t\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> > > > +\t\tif (ret)\n> > > > +\t\t\treturn ret;\n> > > > +\t}\n> > > >\n> > > > \tret = video_->streamOn();\n> > > > \tif (ret) {\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > index 08edefec..b7fa82d0 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > @@ -55,6 +55,19 @@ public:\n> > > > \t\treturn video_->exportBuffers(bufferCount, buffers);\n> > > > \t}\n> > > >\n> > > > +\tint allocateBuffers(unsigned int bufferCount,\n> > > > +\t\t\t std::vector> *buffers)\n> > > > +\t{\n> > > > +\t\tinternalBufs_ = true;\n> > > > +\t\treturn video_->allocateBuffers(bufferCount, buffers);\n> > > What if allocateBuffers() fails ? You have sent the internalBufs_ flag\n> > > before this call.\n> > >\n> > > > +\t}\n> > > > +\n> > > > +\tint releaseBuffers()\n> > > > +\t{\n> > > > +\t\tASSERT(internalBufs_);\n> > > > +\t\treturn video_->releaseBuffers();\n> > > > +\t}\n> > > > +\n> > > I think you should move these to the cpp file.\n> > >\n> > > > \tint start();\n> > > > \tvoid stop();\n> > > >\n> > > > @@ -68,6 +81,7 @@ private:\n> > > >\n> > > > \tconst char *name_;\n> > > > \tbool running_;\n> > > > +\tbool internalBufs_;\n> > > >\n> > > > \tconst Span formats_;\n> > > > \tstd::set streamFormats_;\n> > > > --\n> > > > 2.45.2\n> > > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 78032C323E\n\tfor ;\n\tFri, 19 Jul 2024 07:12:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81CB66336F;\n\tFri, 19 Jul 2024 09:12:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC39C619A3\n\tfor ;\n\tFri, 19 Jul 2024 09:11:58 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-024.catv02.itscom.jp\n\t[175.177.49.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 548582B3;\n\tFri, 19 Jul 2024 09:11:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"M4IMbBQw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721373079;\n\tbh=inPGu0D+fhA0rXRx/5ig1C/wpYTlAl4AIi48l4QdFs8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M4IMbBQwC/Ym7LyJwH4UB/tF+mtxgh7nOxcr8mTOKGKtvWdVTIXlYDEZ6+wux6WwO\n\t2NLHe90tm+8aHUMk8n6ej4hds+KUqq9xocVxqdD3/OrQBUP9RUebecWRPDT21p82O2\n\tvPJMPssxpvweI4TaoMyKFqeglOOxOLtEpSCOvO6g=","Date":"Fri, 19 Jul 2024 16:11:51 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 5/5] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-6-umang.jain@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]