[{"id":30080,"web_url":"https://patchwork.libcamera.org/comment/30080/","msgid":"<171934947401.739659.9819470925978778860@ping.linuxembedded.co.uk>","date":"2024-06-25T21:04:34","subject":"Re: [PATCH v3 4/4] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-06-25 07:23:27)\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 | 143 +++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n> 3 files changed, 161 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index bfc44239..b1314d73 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> bool paramDequeued;\n> bool metadataProcessed;\n> +\n> + std::optional scalerCrop;\n> };\n> \n> class RkISP1Frames\n> @@ -165,6 +170,8 @@ public:\n> \n> private:\n> static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> + static constexpr Size kMinDewarpSize = { 176, 144 };\n> + static constexpr Size kMaxDewarpSize = { 4096, 3072 };\n\nShouldn't these be properties of the m2m dewarp class somehow?\n\n> \n> RkISP1CameraData *cameraData(Camera *camera)\n> {\n> @@ -181,6 +188,7 @@ private:\n> void bufferReady(FrameBuffer *buffer);\n> void paramReady(FrameBuffer *buffer);\n> void statReady(FrameBuffer *buffer);\n> + void dewarpBufferReady(FrameBuffer *buffer);\n> void frameStart(uint32_t sequence);\n> \n> int allocateBuffers(Camera *camera);\n> @@ -200,6 +208,12 @@ private:\n> RkISP1MainPath mainPath_;\n> RkISP1SelfPath selfPath_;\n> \n> + std::unique_ptr dewarper_;\n> +\n> + /* Internal buffers used when dewarper is being used */\n> + std::vector> mainPathBuffers_;\n> + std::queue availableMainPathBuffers_;\n> +\n> std::vector> paramBuffers_;\n> std::vector> statBuffers_;\n> std::queue availableParamBuffers_;\n> @@ -222,6 +236,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> \n> FrameBuffer *paramBuffer = nullptr;\n> FrameBuffer *statBuffer = nullptr;\n> + FrameBuffer *mainPathBuffer = nullptr;\n> + FrameBuffer *selfPathBuffer = nullptr;\n> \n> if (!isRaw) {\n> if (pipe_->availableParamBuffers_.empty()) {\n> @@ -239,10 +255,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> \n> statBuffer = pipe_->availableStatBuffers_.front();\n> pipe_->availableStatBuffers_.pop();\n> +\n> + if (pipe_->dewarper_) {\n> + mainPathBuffer = pipe_->availableMainPathBuffers_.front();\n> + pipe_->availableMainPathBuffers_.pop();\n> + }\n> }\n> \n> - FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> - FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> + if (!mainPathBuffer)\n> + mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> + selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> \n> RkISP1FrameInfo *info = new RkISP1FrameInfo;\n> \n> @@ -268,6 +290,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n> \n> pipe_->availableParamBuffers_.push(info->paramBuffer);\n> pipe_->availableStatBuffers_.push(info->statBuffer);\n> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> \n> frameInfo_.erase(info->frame);\n> \n> @@ -283,6 +306,7 @@ void RkISP1Frames::clear()\n> \n> pipe_->availableParamBuffers_.push(info->paramBuffer);\n> pipe_->availableStatBuffers_.push(info->statBuffer);\n> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> \n> delete info;\n> }\n> @@ -785,12 +809,26 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> << \" crop \" << rect;\n> \n> std::map streamConfig;\n> + std::vector> outputCfgs;\n> \n> for (const StreamConfiguration &cfg : *config) {\n> if (cfg.stream() == &data->mainPathStream_) {\n> ret = mainPath_.configure(cfg, format);\n> streamConfig[0] = IPAStream(cfg.pixelFormat,\n> cfg.size);\n> + /* Configure dewarp */\n> + if (dewarper_ && !isRaw_) {\n> + /*\n> + * \\todo Converter::configure() API should be changed\n> + * to use std::vector> ?\n> + */\n> + outputCfgs.push_back(const_cast(cfg));\n> + dewarper_->configure(cfg, outputCfgs);\n> + } else if (dewarper_ && isRaw_) {\n> + LOG(RkISP1, Debug) << \"Dewarper disabled on RAW configuration\";\n> + dewarper_.reset();\n\nDoes this remove the dewarper if someone asks for a raw stream, and mean\nit could no longer be available if they then switch back to ask for a\nprocessed stream?\n\nI think so ... and fixing that will require reworking quite a bit of the\npatch below so I'll hold off here.\n\nI don't think the 'presence' of dewarper_ can be used to fully determine\nif we use it or not - that' based on the configuration. And we shouldn't\nallocate unless we're using it - and we shouldn't destroy dewarper_ if\nwe don't use if for just a single streaming session.\n\n--\nKieran\n\n\n> + }\n> +\n> } else if (hasSelfPath_) {\n> ret = selfPath_.configure(cfg, format);\n> streamConfig[1] = IPAStream(cfg.pixelFormat,\n> @@ -839,6 +877,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> RkISP1CameraData *data = cameraData(camera);\n> unsigned int count = stream->configuration().bufferCount;\n> \n> + if (!isRaw_ && dewarper_)\n> + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n> +\n> if (stream == &data->mainPathStream_)\n> return mainPath_.exportBuffers(count, buffers);\n> else if (hasSelfPath_ && stream == &data->selfPathStream_)\n> @@ -866,6 +907,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> ret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> if (ret < 0)\n> goto error;\n> +\n> + /* If the dewarper is being used, allocate internal buffers for ISP */\n> + if (dewarper_) {\n> + ret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n> + if (ret < 0)\n> + goto error;\n> +\n> + for (std::unique_ptr &buffer : mainPathBuffers_)\n> + availableMainPathBuffers_.push(buffer.get());\n> + }\n> }\n> \n> for (std::unique_ptr &buffer : paramBuffers_) {\n> @@ -889,6 +940,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> error:\n> paramBuffers_.clear();\n> statBuffers_.clear();\n> + mainPathBuffers_.clear();\n> \n> return ret;\n> }\n> @@ -903,8 +955,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> while (!availableParamBuffers_.empty())\n> availableParamBuffers_.pop();\n> \n> + while (!availableMainPathBuffers_.empty())\n> + availableMainPathBuffers_.pop();\n> +\n> paramBuffers_.clear();\n> statBuffers_.clear();\n> + mainPathBuffers_.clear();\n> \n> std::vector ids;\n> for (IPABuffer &ipabuf : data->ipaBuffers_)\n> @@ -919,6 +975,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> if (stat_->releaseBuffers())\n> LOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> \n> + if (dewarper_)\n> + mainPath_.releaseBuffers();\n> +\n> return 0;\n> }\n> \n> @@ -961,6 +1020,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> << \"Failed to start statistics \" << camera->id();\n> return ret;\n> }\n> +\n> + if (dewarper_) {\n> + ret = dewarper_->start();\n> + if (ret) {\n> + LOG(RkISP1, Error) << \"Failed to start dewarper\";\n> + return ret;\n> + }\n> + }\n> }\n> \n> if (data->mainPath_->isEnabled()) {\n> @@ -1015,6 +1082,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> if (ret)\n> LOG(RkISP1, Warning)\n> << \"Failed to stop parameters for \" << camera->id();\n> +\n> + if (dewarper_)\n> + dewarper_->stop();\n> }\n> \n> ASSERT(data->queuedRequests_.empty());\n> @@ -1045,6 +1115,13 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> info->paramBuffer->cookie());\n> }\n> \n> + const auto &crop = request->controls().get(controls::ScalerCrop);\n> + if (crop && !isRaw_) {\n> + int ret = dewarper_->setScalerCrop(&data->mainPathStream_, crop.value());\n> + if (!ret)\n> + info->scalerCrop = crop;\n> + }\n> +\n> data->frame_++;\n> \n> return 0;\n> @@ -1110,6 +1187,13 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> {\n> ControlInfoMap::Map rkisp1Controls;\n> \n> + if (dewarper_) {\n> + Rectangle maxCrop(kMaxDewarpSize);\n> + Rectangle minCrop = kMinDewarpSize.centeredTo(maxCrop.center());\n> +\n> + rkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> + }\n> +\n> /* Add the IPA registered controls to list of camera controls. */\n> for (const auto &ipaControl : data->ipaControls_)\n> rkisp1Controls[ipaControl.first] = ipaControl.second;\n> @@ -1173,6 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> {\n> + std::shared_ptr dwpMediaDevice;\n> const MediaPad *pad;\n> \n> DeviceMatch dm(\"rkisp1\");\n> @@ -1237,6 +1322,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> \n> + /* If dewarper is present, create its instance. */\n> + DeviceMatch dwp(\"dw100\");\n> + dwp.add(\"dw100-source\");\n> + dwp.add(\"dw100-sink\");\n> +\n> + dwpMediaDevice = enumerator->search(dwp);\n> + if (dwpMediaDevice) {\n> + dewarper_ = std::make_unique(std::move(dwpMediaDevice));\n> + if (dewarper_->isValid()) {\n> + dewarper_->outputBufferReady.connect(\n> + this, &PipelineHandlerRkISP1::dewarpBufferReady);\n> +\n> + LOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n> + } else {\n> + LOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n> + << \" but invalid\";\n> + dewarper_.reset();\n> + }\n> + }\n> +\n> /*\n> * Enumerate all sensors connected to the ISP and create one\n> * camera instance for each of them.\n> @@ -1283,7 +1388,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> return;\n> \n> const FrameMetadata &metadata = buffer->metadata();\n> - Request *request = buffer->request();\n> + Request *request = info->request;\n> \n> if (metadata.status != FrameMetadata::FrameCancelled) {\n> /*\n> @@ -1300,11 +1405,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> data->delayedCtrls_->get(metadata.sequence);\n> data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> }\n> +\n> } else {\n> if (isRaw_)\n> info->metadataProcessed = true;\n> }\n> \n> + if (dewarper_) {\n> + /*\n> + * Queue input and output buffers to the dewarper. The output\n> + * buffers for the dewarper are the buffers of the request, supplied\n> + * by the application.\n> + */\n> + int ret = dewarper_->queueBuffers(buffer, request->buffers());\n> + if (ret < 0)\n> + LOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n> + << strerror(-ret);\n> +\n> + return;\n> + }\n> +\n> + completeBuffer(request, buffer);\n> + tryCompleteRequest(info);\n> +}\n> +\n> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> +{\n> + ASSERT(activeCamera_);\n> + RkISP1CameraData *data = cameraData(activeCamera_);\n> + Request *request = buffer->request();\n> +\n> + RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> + if (!info)\n> + return;\n> +\n> + if (info->scalerCrop)\n> + request->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n> +\n> completeBuffer(request, buffer);\n> tryCompleteRequest(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> const Size &minResolution, const Size &maxResolution)\n> - : name_(name), running_(false), formats_(formats),\n> + : name_(name), running_(false), internalBufs_(false), formats_(formats),\n> minResolution_(minResolution), maxResolution_(maxResolution),\n> link_(nullptr)\n> {\n> @@ -402,10 +402,12 @@ int RkISP1Path::start()\n> if (running_)\n> return -EBUSY;\n> \n> - /* \\todo Make buffer count user configurable. */\n> - ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> - if (ret)\n> - return ret;\n> + if (!internalBufs_) {\n> + /* \\todo Make buffer count user configurable. */\n> + ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> + if (ret)\n> + return ret;\n> + }\n> \n> ret = video_->streamOn();\n> if (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> return video_->exportBuffers(bufferCount, buffers);\n> }\n> \n> + int allocateBuffers(unsigned int bufferCount,\n> + std::vector> *buffers)\n> + {\n> + internalBufs_ = true;\n> + return video_->allocateBuffers(bufferCount, buffers);\n> + }\n> +\n> + int releaseBuffers()\n> + {\n> + ASSERT(internalBufs_);\n> + return video_->releaseBuffers();\n> + }\n> +\n> int start();\n> void stop();\n> \n> @@ -68,6 +81,7 @@ private:\n> \n> const char *name_;\n> bool running_;\n> + bool internalBufs_;\n> \n> const Span formats_;\n> std::set streamFormats_;\n> -- \n> 2.44.0\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 C9404BD87C\n\tfor ;\n\tTue, 25 Jun 2024 21:04:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D69DF654AB;\n\tTue, 25 Jun 2024 23:04:38 +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 6296B654A1\n\tfor ;\n\tTue, 25 Jun 2024 23:04:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3FB3A512;\n\tTue, 25 Jun 2024 23:04:14 +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=\"APiMdfdZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719349454;\n\tbh=f+VfOu45NiS6lcbKu8QLRiZXDGh3Y/qKCrQLJtpgbHs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=APiMdfdZSILUvk9sxh53BGJ/gLnxtCamt1QASG0VtW7QJXFlYmWsDQullsRuw4rhC\n\tZ+IXG1VAyK5BuoaZzpYlhm7eqipcUBz+NRHwSzvXNNRMKaaYCTdpUCNTnxMS9OQ2VA\n\tNXtMBO5+PnpSpuS1vCIzmNmtXDbCnAS++YM4Gw98=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240625062327.50940-5-umang.jain@ideasonboard.com>","References":"<20240625062327.50940-1-umang.jain@ideasonboard.com>\n\t<20240625062327.50940-5-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH v3 4/4] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","From":"Kieran Bingham ","Cc":"Umang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Jun 2024 22:04:34 +0100","Message-ID":"<171934947401.739659.9819470925978778860@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":30081,"web_url":"https://patchwork.libcamera.org/comment/30081/","msgid":"","date":"2024-06-26T03:46:58","subject":"Re: [PATCH v3 4/4] 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 Kieran,\n\nOn 26/06/24 2:34 am, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-06-25 07:23:27)\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 | 143 +++++++++++++++++-\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n>> 3 files changed, 161 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index bfc44239..b1314d73 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>> bool paramDequeued;\n>> bool metadataProcessed;\n>> +\n>> + std::optional scalerCrop;\n>> };\n>> \n>> class RkISP1Frames\n>> @@ -165,6 +170,8 @@ public:\n>> \n>> private:\n>> static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n>> + static constexpr Size kMinDewarpSize = { 176, 144 };\n>> + static constexpr Size kMaxDewarpSize = { 4096, 3072 };\n> Shouldn't these be properties of the m2m dewarp class somehow?\n>\n>> \n>> RkISP1CameraData *cameraData(Camera *camera)\n>> {\n>> @@ -181,6 +188,7 @@ private:\n>> void bufferReady(FrameBuffer *buffer);\n>> void paramReady(FrameBuffer *buffer);\n>> void statReady(FrameBuffer *buffer);\n>> + void dewarpBufferReady(FrameBuffer *buffer);\n>> void frameStart(uint32_t sequence);\n>> \n>> int allocateBuffers(Camera *camera);\n>> @@ -200,6 +208,12 @@ private:\n>> RkISP1MainPath mainPath_;\n>> RkISP1SelfPath selfPath_;\n>> \n>> + std::unique_ptr dewarper_;\n>> +\n>> + /* Internal buffers used when dewarper is being used */\n>> + std::vector> mainPathBuffers_;\n>> + std::queue availableMainPathBuffers_;\n>> +\n>> std::vector> paramBuffers_;\n>> std::vector> statBuffers_;\n>> std::queue availableParamBuffers_;\n>> @@ -222,6 +236,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>> \n>> FrameBuffer *paramBuffer = nullptr;\n>> FrameBuffer *statBuffer = nullptr;\n>> + FrameBuffer *mainPathBuffer = nullptr;\n>> + FrameBuffer *selfPathBuffer = nullptr;\n>> \n>> if (!isRaw) {\n>> if (pipe_->availableParamBuffers_.empty()) {\n>> @@ -239,10 +255,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>> \n>> statBuffer = pipe_->availableStatBuffers_.front();\n>> pipe_->availableStatBuffers_.pop();\n>> +\n>> + if (pipe_->dewarper_) {\n>> + mainPathBuffer = pipe_->availableMainPathBuffers_.front();\n>> + pipe_->availableMainPathBuffers_.pop();\n>> + }\n>> }\n>> \n>> - FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> - FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>> + if (!mainPathBuffer)\n>> + mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> + selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>> \n>> RkISP1FrameInfo *info = new RkISP1FrameInfo;\n>> \n>> @@ -268,6 +290,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n>> \n>> pipe_->availableParamBuffers_.push(info->paramBuffer);\n>> pipe_->availableStatBuffers_.push(info->statBuffer);\n>> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> \n>> frameInfo_.erase(info->frame);\n>> \n>> @@ -283,6 +306,7 @@ void RkISP1Frames::clear()\n>> \n>> pipe_->availableParamBuffers_.push(info->paramBuffer);\n>> pipe_->availableStatBuffers_.push(info->statBuffer);\n>> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> \n>> delete info;\n>> }\n>> @@ -785,12 +809,26 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>> << \" crop \" << rect;\n>> \n>> std::map streamConfig;\n>> + std::vector> outputCfgs;\n>> \n>> for (const StreamConfiguration &cfg : *config) {\n>> if (cfg.stream() == &data->mainPathStream_) {\n>> ret = mainPath_.configure(cfg, format);\n>> streamConfig[0] = IPAStream(cfg.pixelFormat,\n>> cfg.size);\n>> + /* Configure dewarp */\n>> + if (dewarper_ && !isRaw_) {\n>> + /*\n>> + * \\todo Converter::configure() API should be changed\n>> + * to use std::vector> ?\n>> + */\n>> + outputCfgs.push_back(const_cast(cfg));\n>> + dewarper_->configure(cfg, outputCfgs);\n>> + } else if (dewarper_ && isRaw_) {\n>> + LOG(RkISP1, Debug) << \"Dewarper disabled on RAW configuration\";\n>> + dewarper_.reset();\n> Does this remove the dewarper if someone asks for a raw stream, and mean\n> it could no longer be available if they then switch back to ask for a\n> processed stream?\n>\n> I think so ... and fixing that will require reworking quite a bit of the\n> patch below so I'll hold off here.\n>\n> I don't think the 'presence' of dewarper_ can be used to fully determine\n> if we use it or not - that' based on the configuration. And we shouldn't\n> allocate unless we're using it - and we shouldn't destroy dewarper_ if\n> we don't use if for just a single streaming session.\n\nYou are correct, we shouldn't be resetting the dewarper_, if not in use. \nIt should be dependent on the configuration\n\nThe raw configuration is tracked by isRaw_ member variable in the \npipeline, so probably I'll replace\n\nif (dewarper_)\n...\n\nwith\n\nif (dewarper_ && !isRaw_)\n.,..\n\nat most places - in the next version, without destroying the dewarper_.\n>\n> --\n> Kieran\n>\n>\n>> + }\n>> +\n>> } else if (hasSelfPath_) {\n>> ret = selfPath_.configure(cfg, format);\n>> streamConfig[1] = IPAStream(cfg.pixelFormat,\n>> @@ -839,6 +877,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n>> RkISP1CameraData *data = cameraData(camera);\n>> unsigned int count = stream->configuration().bufferCount;\n>> \n>> + if (!isRaw_ && dewarper_)\n>> + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n>> +\n>> if (stream == &data->mainPathStream_)\n>> return mainPath_.exportBuffers(count, buffers);\n>> else if (hasSelfPath_ && stream == &data->selfPathStream_)\n>> @@ -866,6 +907,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>> ret = stat_->allocateBuffers(maxCount, &statBuffers_);\n>> if (ret < 0)\n>> goto error;\n>> +\n>> + /* If the dewarper is being used, allocate internal buffers for ISP */\n>> + if (dewarper_) {\n>> + ret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n>> + if (ret < 0)\n>> + goto error;\n>> +\n>> + for (std::unique_ptr &buffer : mainPathBuffers_)\n>> + availableMainPathBuffers_.push(buffer.get());\n>> + }\n>> }\n>> \n>> for (std::unique_ptr &buffer : paramBuffers_) {\n>> @@ -889,6 +940,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>> error:\n>> paramBuffers_.clear();\n>> statBuffers_.clear();\n>> + mainPathBuffers_.clear();\n>> \n>> return ret;\n>> }\n>> @@ -903,8 +955,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>> while (!availableParamBuffers_.empty())\n>> availableParamBuffers_.pop();\n>> \n>> + while (!availableMainPathBuffers_.empty())\n>> + availableMainPathBuffers_.pop();\n>> +\n>> paramBuffers_.clear();\n>> statBuffers_.clear();\n>> + mainPathBuffers_.clear();\n>> \n>> std::vector ids;\n>> for (IPABuffer &ipabuf : data->ipaBuffers_)\n>> @@ -919,6 +975,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>> if (stat_->releaseBuffers())\n>> LOG(RkISP1, Error) << \"Failed to release stat buffers\";\n>> \n>> + if (dewarper_)\n>> + mainPath_.releaseBuffers();\n>> +\n>> return 0;\n>> }\n>> \n>> @@ -961,6 +1020,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n>> << \"Failed to start statistics \" << camera->id();\n>> return ret;\n>> }\n>> +\n>> + if (dewarper_) {\n>> + ret = dewarper_->start();\n>> + if (ret) {\n>> + LOG(RkISP1, Error) << \"Failed to start dewarper\";\n>> + return ret;\n>> + }\n>> + }\n>> }\n>> \n>> if (data->mainPath_->isEnabled()) {\n>> @@ -1015,6 +1082,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n>> if (ret)\n>> LOG(RkISP1, Warning)\n>> << \"Failed to stop parameters for \" << camera->id();\n>> +\n>> + if (dewarper_)\n>> + dewarper_->stop();\n>> }\n>> \n>> ASSERT(data->queuedRequests_.empty());\n>> @@ -1045,6 +1115,13 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n>> info->paramBuffer->cookie());\n>> }\n>> \n>> + const auto &crop = request->controls().get(controls::ScalerCrop);\n>> + if (crop && !isRaw_) {\n>> + int ret = dewarper_->setScalerCrop(&data->mainPathStream_, crop.value());\n>> + if (!ret)\n>> + info->scalerCrop = crop;\n>> + }\n>> +\n>> data->frame_++;\n>> \n>> return 0;\n>> @@ -1110,6 +1187,13 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n>> {\n>> ControlInfoMap::Map rkisp1Controls;\n>> \n>> + if (dewarper_) {\n>> + Rectangle maxCrop(kMaxDewarpSize);\n>> + Rectangle minCrop = kMinDewarpSize.centeredTo(maxCrop.center());\n>> +\n>> + rkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n>> + }\n>> +\n>> /* Add the IPA registered controls to list of camera controls. */\n>> for (const auto &ipaControl : data->ipaControls_)\n>> rkisp1Controls[ipaControl.first] = ipaControl.second;\n>> @@ -1173,6 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>> \n>> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> {\n>> + std::shared_ptr dwpMediaDevice;\n>> const MediaPad *pad;\n>> \n>> DeviceMatch dm(\"rkisp1\");\n>> @@ -1237,6 +1322,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>> \n>> + /* If dewarper is present, create its instance. */\n>> + DeviceMatch dwp(\"dw100\");\n>> + dwp.add(\"dw100-source\");\n>> + dwp.add(\"dw100-sink\");\n>> +\n>> + dwpMediaDevice = enumerator->search(dwp);\n>> + if (dwpMediaDevice) {\n>> + dewarper_ = std::make_unique(std::move(dwpMediaDevice));\n>> + if (dewarper_->isValid()) {\n>> + dewarper_->outputBufferReady.connect(\n>> + this, &PipelineHandlerRkISP1::dewarpBufferReady);\n>> +\n>> + LOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n>> + } else {\n>> + LOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n>> + << \" but invalid\";\n>> + dewarper_.reset();\n>> + }\n>> + }\n>> +\n>> /*\n>> * Enumerate all sensors connected to the ISP and create one\n>> * camera instance for each of them.\n>> @@ -1283,7 +1388,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> return;\n>> \n>> const FrameMetadata &metadata = buffer->metadata();\n>> - Request *request = buffer->request();\n>> + Request *request = info->request;\n>> \n>> if (metadata.status != FrameMetadata::FrameCancelled) {\n>> /*\n>> @@ -1300,11 +1405,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> data->delayedCtrls_->get(metadata.sequence);\n>> data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n>> }\n>> +\n>> } else {\n>> if (isRaw_)\n>> info->metadataProcessed = true;\n>> }\n>> \n>> + if (dewarper_) {\n>> + /*\n>> + * Queue input and output buffers to the dewarper. The output\n>> + * buffers for the dewarper are the buffers of the request, supplied\n>> + * by the application.\n>> + */\n>> + int ret = dewarper_->queueBuffers(buffer, request->buffers());\n>> + if (ret < 0)\n>> + LOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n>> + << strerror(-ret);\n>> +\n>> + return;\n>> + }\n>> +\n>> + completeBuffer(request, buffer);\n>> + tryCompleteRequest(info);\n>> +}\n>> +\n>> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n>> +{\n>> + ASSERT(activeCamera_);\n>> + RkISP1CameraData *data = cameraData(activeCamera_);\n>> + Request *request = buffer->request();\n>> +\n>> + RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n>> + if (!info)\n>> + return;\n>> +\n>> + if (info->scalerCrop)\n>> + request->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n>> +\n>> completeBuffer(request, buffer);\n>> tryCompleteRequest(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>> const Size &minResolution, const Size &maxResolution)\n>> - : name_(name), running_(false), formats_(formats),\n>> + : name_(name), running_(false), internalBufs_(false), formats_(formats),\n>> minResolution_(minResolution), maxResolution_(maxResolution),\n>> link_(nullptr)\n>> {\n>> @@ -402,10 +402,12 @@ int RkISP1Path::start()\n>> if (running_)\n>> return -EBUSY;\n>> \n>> - /* \\todo Make buffer count user configurable. */\n>> - ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n>> - if (ret)\n>> - return ret;\n>> + if (!internalBufs_) {\n>> + /* \\todo Make buffer count user configurable. */\n>> + ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n>> + if (ret)\n>> + return ret;\n>> + }\n>> \n>> ret = video_->streamOn();\n>> if (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>> return video_->exportBuffers(bufferCount, buffers);\n>> }\n>> \n>> + int allocateBuffers(unsigned int bufferCount,\n>> + std::vector> *buffers)\n>> + {\n>> + internalBufs_ = true;\n>> + return video_->allocateBuffers(bufferCount, buffers);\n>> + }\n>> +\n>> + int releaseBuffers()\n>> + {\n>> + ASSERT(internalBufs_);\n>> + return video_->releaseBuffers();\n>> + }\n>> +\n>> int start();\n>> void stop();\n>> \n>> @@ -68,6 +81,7 @@ private:\n>> \n>> const char *name_;\n>> bool running_;\n>> + bool internalBufs_;\n>> \n>> const Span formats_;\n>> std::set streamFormats_;\n>> -- \n>> 2.44.0\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 CDA28BDB1D\n\tfor ;\n\tWed, 26 Jun 2024 03:47:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E54DF654A9;\n\tWed, 26 Jun 2024 05:47:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E8D8619CC\n\tfor ;\n\tWed, 26 Jun 2024 05:47:04 +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 A30A373E;\n\tWed, 26 Jun 2024 05:46:40 +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=\"afGAVWRL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719373601;\n\tbh=54gSsQc3jxvt5/OBZK2vTLRIfTrmRoZoHFbgzcm/jKE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=afGAVWRLXP0JvCbBLiRB/oiFHzhzj8vXX65A9Q5Kql8EVTk5LbUtV157ELwr37FO7\n\tJ0NaOMqWTQBinukzLxgECbVXUuR7LTLOO83P7SumAuVfHDfotPafQXG2gh+BD1/SpU\n\tOCtrf45Suvn7i2D/LEtqYeVKHXLtTWFhwgXYca58=","Message-ID":"","Date":"Wed, 26 Jun 2024 09:16:58 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 4/4] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240625062327.50940-1-umang.jain@ideasonboard.com>\n\t<20240625062327.50940-5-umang.jain@ideasonboard.com>\n\t<171934947401.739659.9819470925978778860@ping.linuxembedded.co.uk>","From":"Umang Jain ","In-Reply-To":"<171934947401.739659.9819470925978778860@ping.linuxembedded.co.uk>","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":30084,"web_url":"https://patchwork.libcamera.org/comment/30084/","msgid":"<171938620934.81556.16375374626951557494@ping.linuxembedded.co.uk>","date":"2024-06-26T07:16:49","subject":"Re: [PATCH v3 4/4] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-06-26 04:46:58)\n> Hi Kieran,\n> \n> On 26/06/24 2:34 am, Kieran Bingham wrote:\n> > Quoting Umang Jain (2024-06-25 07:23:27)\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 | 143 +++++++++++++++++-\n> >> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-\n> >> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++\n> >> 3 files changed, 161 insertions(+), 8 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index bfc44239..b1314d73 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> >> bool paramDequeued;\n> >> bool metadataProcessed;\n> >> +\n> >> + std::optional scalerCrop;\n> >> };\n> >> \n> >> class RkISP1Frames\n> >> @@ -165,6 +170,8 @@ public:\n> >> \n> >> private:\n> >> static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> >> + static constexpr Size kMinDewarpSize = { 176, 144 };\n> >> + static constexpr Size kMaxDewarpSize = { 4096, 3072 };\n> > Shouldn't these be properties of the m2m dewarp class somehow?\n> >\n> >> \n> >> RkISP1CameraData *cameraData(Camera *camera)\n> >> {\n> >> @@ -181,6 +188,7 @@ private:\n> >> void bufferReady(FrameBuffer *buffer);\n> >> void paramReady(FrameBuffer *buffer);\n> >> void statReady(FrameBuffer *buffer);\n> >> + void dewarpBufferReady(FrameBuffer *buffer);\n> >> void frameStart(uint32_t sequence);\n> >> \n> >> int allocateBuffers(Camera *camera);\n> >> @@ -200,6 +208,12 @@ private:\n> >> RkISP1MainPath mainPath_;\n> >> RkISP1SelfPath selfPath_;\n> >> \n> >> + std::unique_ptr dewarper_;\n> >> +\n> >> + /* Internal buffers used when dewarper is being used */\n> >> + std::vector> mainPathBuffers_;\n> >> + std::queue availableMainPathBuffers_;\n> >> +\n> >> std::vector> paramBuffers_;\n> >> std::vector> statBuffers_;\n> >> std::queue availableParamBuffers_;\n> >> @@ -222,6 +236,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> >> \n> >> FrameBuffer *paramBuffer = nullptr;\n> >> FrameBuffer *statBuffer = nullptr;\n> >> + FrameBuffer *mainPathBuffer = nullptr;\n> >> + FrameBuffer *selfPathBuffer = nullptr;\n> >> \n> >> if (!isRaw) {\n> >> if (pipe_->availableParamBuffers_.empty()) {\n> >> @@ -239,10 +255,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> >> \n> >> statBuffer = pipe_->availableStatBuffers_.front();\n> >> pipe_->availableStatBuffers_.pop();\n> >> +\n> >> + if (pipe_->dewarper_) {\n> >> + mainPathBuffer = pipe_->availableMainPathBuffers_.front();\n> >> + pipe_->availableMainPathBuffers_.pop();\n> >> + }\n> >> }\n> >> \n> >> - FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> >> - FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> >> + if (!mainPathBuffer)\n> >> + mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> >> + selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> >> \n> >> RkISP1FrameInfo *info = new RkISP1FrameInfo;\n> >> \n> >> @@ -268,6 +290,7 @@ int RkISP1Frames::destroy(unsigned int frame)\n> >> \n> >> pipe_->availableParamBuffers_.push(info->paramBuffer);\n> >> pipe_->availableStatBuffers_.push(info->statBuffer);\n> >> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> >> \n> >> frameInfo_.erase(info->frame);\n> >> \n> >> @@ -283,6 +306,7 @@ void RkISP1Frames::clear()\n> >> \n> >> pipe_->availableParamBuffers_.push(info->paramBuffer);\n> >> pipe_->availableStatBuffers_.push(info->statBuffer);\n> >> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> >> \n> >> delete info;\n> >> }\n> >> @@ -785,12 +809,26 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >> << \" crop \" << rect;\n> >> \n> >> std::map streamConfig;\n> >> + std::vector> outputCfgs;\n> >> \n> >> for (const StreamConfiguration &cfg : *config) {\n> >> if (cfg.stream() == &data->mainPathStream_) {\n> >> ret = mainPath_.configure(cfg, format);\n> >> streamConfig[0] = IPAStream(cfg.pixelFormat,\n> >> cfg.size);\n> >> + /* Configure dewarp */\n> >> + if (dewarper_ && !isRaw_) {\n> >> + /*\n> >> + * \\todo Converter::configure() API should be changed\n> >> + * to use std::vector> ?\n> >> + */\n> >> + outputCfgs.push_back(const_cast(cfg));\n> >> + dewarper_->configure(cfg, outputCfgs);\n> >> + } else if (dewarper_ && isRaw_) {\n> >> + LOG(RkISP1, Debug) << \"Dewarper disabled on RAW configuration\";\n> >> + dewarper_.reset();\n> > Does this remove the dewarper if someone asks for a raw stream, and mean\n> > it could no longer be available if they then switch back to ask for a\n> > processed stream?\n> >\n> > I think so ... and fixing that will require reworking quite a bit of the\n> > patch below so I'll hold off here.\n> >\n> > I don't think the 'presence' of dewarper_ can be used to fully determine\n> > if we use it or not - that' based on the configuration. And we shouldn't\n> > allocate unless we're using it - and we shouldn't destroy dewarper_ if\n> > we don't use if for just a single streaming session.\n> \n> You are correct, we shouldn't be resetting the dewarper_, if not in use. \n> It should be dependent on the configuration\n> \n> The raw configuration is tracked by isRaw_ member variable in the \n> pipeline, so probably I'll replace\n> \n> if (dewarper_)\n> ...\n> \n> with\n> \n> if (dewarper_ && !isRaw_)\n> .,..\n\nIt depends on how you implement it - but a specific flag might also be\nbetter for \n\n if (useDewarper) {}\n\nor\n\n if (dewarperEnabled) { }\n\nto avoid having to reference the heuristic of when it's in use in many\nplaces.\n\nor a class helper that checks those existing flags?\n if (useDewarper()) { }\n\nWhich would potentially avoid having to keep an extra flag syncrhonised.\n\nAnyway, I'll leave that all up to you - just ideas when I read\n\"if (dewarper_ && !isRaw_)\"\n\n\n--\nKieran\n\n> at most places - in the next version, without destroying the dewarper_.\n> >\n> > --\n> > Kieran\n> >\n> >\n> >> + }\n> >> +\n> >> } else if (hasSelfPath_) {\n> >> ret = selfPath_.configure(cfg, format);\n> >> streamConfig[1] = IPAStream(cfg.pixelFormat,\n> >> @@ -839,6 +877,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> >> RkISP1CameraData *data = cameraData(camera);\n> >> unsigned int count = stream->configuration().bufferCount;\n> >> \n> >> + if (!isRaw_ && dewarper_)\n> >> + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);\n> >> +\n> >> if (stream == &data->mainPathStream_)\n> >> return mainPath_.exportBuffers(count, buffers);\n> >> else if (hasSelfPath_ && stream == &data->selfPathStream_)\n> >> @@ -866,6 +907,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> >> ret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> >> if (ret < 0)\n> >> goto error;\n> >> +\n> >> + /* If the dewarper is being used, allocate internal buffers for ISP */\n> >> + if (dewarper_) {\n> >> + ret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);\n> >> + if (ret < 0)\n> >> + goto error;\n> >> +\n> >> + for (std::unique_ptr &buffer : mainPathBuffers_)\n> >> + availableMainPathBuffers_.push(buffer.get());\n> >> + }\n> >> }\n> >> \n> >> for (std::unique_ptr &buffer : paramBuffers_) {\n> >> @@ -889,6 +940,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> >> error:\n> >> paramBuffers_.clear();\n> >> statBuffers_.clear();\n> >> + mainPathBuffers_.clear();\n> >> \n> >> return ret;\n> >> }\n> >> @@ -903,8 +955,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> >> while (!availableParamBuffers_.empty())\n> >> availableParamBuffers_.pop();\n> >> \n> >> + while (!availableMainPathBuffers_.empty())\n> >> + availableMainPathBuffers_.pop();\n> >> +\n> >> paramBuffers_.clear();\n> >> statBuffers_.clear();\n> >> + mainPathBuffers_.clear();\n> >> \n> >> std::vector ids;\n> >> for (IPABuffer &ipabuf : data->ipaBuffers_)\n> >> @@ -919,6 +975,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> >> if (stat_->releaseBuffers())\n> >> LOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> >> \n> >> + if (dewarper_)\n> >> + mainPath_.releaseBuffers();\n> >> +\n> >> return 0;\n> >> }\n> >> \n> >> @@ -961,6 +1020,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> >> << \"Failed to start statistics \" << camera->id();\n> >> return ret;\n> >> }\n> >> +\n> >> + if (dewarper_) {\n> >> + ret = dewarper_->start();\n> >> + if (ret) {\n> >> + LOG(RkISP1, Error) << \"Failed to start dewarper\";\n> >> + return ret;\n> >> + }\n> >> + }\n> >> }\n> >> \n> >> if (data->mainPath_->isEnabled()) {\n> >> @@ -1015,6 +1082,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> >> if (ret)\n> >> LOG(RkISP1, Warning)\n> >> << \"Failed to stop parameters for \" << camera->id();\n> >> +\n> >> + if (dewarper_)\n> >> + dewarper_->stop();\n> >> }\n> >> \n> >> ASSERT(data->queuedRequests_.empty());\n> >> @@ -1045,6 +1115,13 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> >> info->paramBuffer->cookie());\n> >> }\n> >> \n> >> + const auto &crop = request->controls().get(controls::ScalerCrop);\n> >> + if (crop && !isRaw_) {\n> >> + int ret = dewarper_->setScalerCrop(&data->mainPathStream_, crop.value());\n> >> + if (!ret)\n> >> + info->scalerCrop = crop;\n> >> + }\n> >> +\n> >> data->frame_++;\n> >> \n> >> return 0;\n> >> @@ -1110,6 +1187,13 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> >> {\n> >> ControlInfoMap::Map rkisp1Controls;\n> >> \n> >> + if (dewarper_) {\n> >> + Rectangle maxCrop(kMaxDewarpSize);\n> >> + Rectangle minCrop = kMinDewarpSize.centeredTo(maxCrop.center());\n> >> +\n> >> + rkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> >> + }\n> >> +\n> >> /* Add the IPA registered controls to list of camera controls. */\n> >> for (const auto &ipaControl : data->ipaControls_)\n> >> rkisp1Controls[ipaControl.first] = ipaControl.second;\n> >> @@ -1173,6 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >> \n> >> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >> {\n> >> + std::shared_ptr dwpMediaDevice;\n> >> const MediaPad *pad;\n> >> \n> >> DeviceMatch dm(\"rkisp1\");\n> >> @@ -1237,6 +1322,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> >> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> >> \n> >> + /* If dewarper is present, create its instance. */\n> >> + DeviceMatch dwp(\"dw100\");\n> >> + dwp.add(\"dw100-source\");\n> >> + dwp.add(\"dw100-sink\");\n> >> +\n> >> + dwpMediaDevice = enumerator->search(dwp);\n> >> + if (dwpMediaDevice) {\n> >> + dewarper_ = std::make_unique(std::move(dwpMediaDevice));\n> >> + if (dewarper_->isValid()) {\n> >> + dewarper_->outputBufferReady.connect(\n> >> + this, &PipelineHandlerRkISP1::dewarpBufferReady);\n> >> +\n> >> + LOG(RkISP1, Info) << \"Using DW100 dewarper \" << dewarper_->deviceNode();\n> >> + } else {\n> >> + LOG(RkISP1, Debug) << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n> >> + << \" but invalid\";\n> >> + dewarper_.reset();\n> >> + }\n> >> + }\n> >> +\n> >> /*\n> >> * Enumerate all sensors connected to the ISP and create one\n> >> * camera instance for each of them.\n> >> @@ -1283,7 +1388,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> >> return;\n> >> \n> >> const FrameMetadata &metadata = buffer->metadata();\n> >> - Request *request = buffer->request();\n> >> + Request *request = info->request;\n> >> \n> >> if (metadata.status != FrameMetadata::FrameCancelled) {\n> >> /*\n> >> @@ -1300,11 +1405,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> >> data->delayedCtrls_->get(metadata.sequence);\n> >> data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> >> }\n> >> +\n> >> } else {\n> >> if (isRaw_)\n> >> info->metadataProcessed = true;\n> >> }\n> >> \n> >> + if (dewarper_) {\n> >> + /*\n> >> + * Queue input and output buffers to the dewarper. The output\n> >> + * buffers for the dewarper are the buffers of the request, supplied\n> >> + * by the application.\n> >> + */\n> >> + int ret = dewarper_->queueBuffers(buffer, request->buffers());\n> >> + if (ret < 0)\n> >> + LOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n> >> + << strerror(-ret);\n> >> +\n> >> + return;\n> >> + }\n> >> +\n> >> + completeBuffer(request, buffer);\n> >> + tryCompleteRequest(info);\n> >> +}\n> >> +\n> >> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> >> +{\n> >> + ASSERT(activeCamera_);\n> >> + RkISP1CameraData *data = cameraData(activeCamera_);\n> >> + Request *request = buffer->request();\n> >> +\n> >> + RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> >> + if (!info)\n> >> + return;\n> >> +\n> >> + if (info->scalerCrop)\n> >> + request->metadata().set(controls::ScalerCrop, info->scalerCrop.value());\n> >> +\n> >> completeBuffer(request, buffer);\n> >> tryCompleteRequest(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> >> const Size &minResolution, const Size &maxResolution)\n> >> - : name_(name), running_(false), formats_(formats),\n> >> + : name_(name), running_(false), internalBufs_(false), formats_(formats),\n> >> minResolution_(minResolution), maxResolution_(maxResolution),\n> >> link_(nullptr)\n> >> {\n> >> @@ -402,10 +402,12 @@ int RkISP1Path::start()\n> >> if (running_)\n> >> return -EBUSY;\n> >> \n> >> - /* \\todo Make buffer count user configurable. */\n> >> - ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> >> - if (ret)\n> >> - return ret;\n> >> + if (!internalBufs_) {\n> >> + /* \\todo Make buffer count user configurable. */\n> >> + ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> >> + if (ret)\n> >> + return ret;\n> >> + }\n> >> \n> >> ret = video_->streamOn();\n> >> if (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> >> return video_->exportBuffers(bufferCount, buffers);\n> >> }\n> >> \n> >> + int allocateBuffers(unsigned int bufferCount,\n> >> + std::vector> *buffers)\n> >> + {\n> >> + internalBufs_ = true;\n> >> + return video_->allocateBuffers(bufferCount, buffers);\n> >> + }\n> >> +\n> >> + int releaseBuffers()\n> >> + {\n> >> + ASSERT(internalBufs_);\n> >> + return video_->releaseBuffers();\n> >> + }\n> >> +\n> >> int start();\n> >> void stop();\n> >> \n> >> @@ -68,6 +81,7 @@ private:\n> >> \n> >> const char *name_;\n> >> bool running_;\n> >> + bool internalBufs_;\n> >> \n> >> const Span formats_;\n> >> std::set streamFormats_;\n> >> -- \n> >> 2.44.0\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 2D1D3BDB1D\n\tfor ;\n\tWed, 26 Jun 2024 07:16:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CF64654A9;\n\tWed, 26 Jun 2024 09:16:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8527D619E5\n\tfor ;\n\tWed, 26 Jun 2024 09:16:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61ED273E;\n\tWed, 26 Jun 2024 09:16:29 +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=\"HoiOMTKP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719386189;\n\tbh=TujfcYU4fEc/UJp7KEvj5DoisTPh9ixv9ihz7TrTigM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=HoiOMTKPCHMKmJTp4l5U9l3Yk6ki22t/IM7Bj5+fsQjKUJ9yyn9KfWK5CDRBPl9ka\n\tF1EbORTlPKCm53kFp4D+Kr/xYxXKgUgwW/Ao2eC0YEFU93zjEMKI001SsUR/6rDvwS\n\tYPxAnEV/yvzZuiyGEapEhRtVyEsq8zv5ZobhvUNM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240625062327.50940-1-umang.jain@ideasonboard.com>\n\t<20240625062327.50940-5-umang.jain@ideasonboard.com>\n\t<171934947401.739659.9819470925978778860@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v3 4/4] libcamera: rkisp1: Plumb the ConverterDW100\n\tconverter","From":"Kieran Bingham ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 26 Jun 2024 08:16:49 +0100","Message-ID":"<171938620934.81556.16375374626951557494@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]