[{"id":37058,"web_url":"https://patchwork.libcamera.org/comment/37058/","msgid":"<176409207161.567526.14389686667367886866@ping.linuxembedded.co.uk>","date":"2025-11-25T17:34:31","subject":"Re: [PATCH v3 17/29] libcamera: rkisp1: Use the dw100 converter\n\tmodule instead of the generic v4l2 converter","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-25 16:28:29)\n> The dewarper integration into the rkisp1 pipeline is quite complicated.\n> Simplify that by switching to the now available ConverterDW100Module. As\n> there is no other known converter in combination with th rkisp1 ISP this\n\ns/with th/with the/\n\n> is a safe step to do.\n> \n> This change also paves the way to implement dw100 specific features later.\n> \n> The input crop implemented in the dw100 kernel driver is quite limited\n> in that it doesn't allow arbitrary crop rectangles but only scale\n> factors quantized to the underlying fixed point representation and only\n> aspect ratio preserving crops.\n> \n> The vertex map based implementation allows for pixel perfect crops. The\n> only downside is that ScalerCrop can no longer be set dynamically on\n> older kernels. A corresponding warning is already implemented in the\n> converter module.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v3:\n> - Merge usage of the converter module and ScalerCrop handling into one\n> patch as it is difficult to keep them separate with the new module\n> concept\n> \n> Changes in 0.9:\n> - Code cleanup\n> - Update vertex map before start() to partially support old kernels\n> \n> Changes in 0.7:\n> - Removed double call to applyVertexMap()\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 122 ++++++-----------------\n> 1 file changed, 33 insertions(+), 89 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 138e1d5bf06b..6256a67bc31e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -36,7 +36,7 @@\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/camera_sensor_properties.h\"\n> -#include \"libcamera/internal/converter/converter_v4l2_m2m.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> @@ -232,10 +232,7 @@ private:\n> RkISP1MainPath mainPath_;\n> RkISP1SelfPath selfPath_;\n> \n> - std::unique_ptr dewarper_;\n> - Rectangle scalerMaxCrop_;\n> -\n> - std::optional activeCrop_;\n> + std::unique_ptr dewarper_;\n> \n> /* Internal buffers used when dewarper is being used */\n> std::vector> mainPathBuffers_;\n> @@ -940,6 +937,16 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> if (ret)\n> return ret;\n> \n> + /*\n> + * Apply the actual sensor crop, for proper\n> + * dewarp map calculation\n> + */\n\nspacing broke above\n\n> + Rectangle sensorCrop = outputCrop.transformedBetween(\n> + inputCrop, sensorInfo.analogCrop);\n> + if (data->usesDewarper_)\n> + dewarper_->setSensorCrop(sensorCrop);\n> + data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> +\n> std::map streamConfig;\n> std::vector> outputCfgs;\n> \n> @@ -965,13 +972,15 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> return ret;\n> \n> /*\n> - * Calculate the crop rectangle of the data\n> - * flowing into the dewarper in sensor\n> - * coordinates.\n> + * Apply a default scaler crop that keeps the\n> + * aspect ratio.\n> */\n> - scalerMaxCrop_ =\n> - outputCrop.transformedBetween(inputCrop,\n> - sensorInfo.analogCrop);\n> + Size size = cfg.size;\n> + size = sensorCrop.size().boundedToAspectRatio(size);\n> +\n> + ControlList ctrls;\n> + ctrls.set(controls::ScalerCrop, size.centeredTo(sensorCrop.center()));\n> + dewarper_->setControls(cfg.stream(), ctrls);\n> }\n> \n> ret = mainPath_.configure(ispCfg, format);\n> @@ -1165,6 +1174,9 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> actions += [&]() { stat_->streamOff(); };\n> \n> if (data->usesDewarper_) {\n> + if (controls)\n> + dewarper_->setControls(&data->mainPathStream_, *controls);\n> +\n\nOh - that's how controls get handled at start ...\n\n> ret = dewarper_->start();\n> if (ret) {\n> LOG(RkISP1, Error) << \"Failed to start dewarper\";\n> @@ -1315,28 +1327,8 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> {\n> ControlInfoMap::Map controls;\n> \n> - if (data->usesDewarper_) {\n> - std::pair cropLimits;\n> - if (dewarper_->isConfigured(&data->mainPathStream_))\n> - cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_);\n> - else\n> - cropLimits = dewarper_->inputCropBounds();\n> -\n> - /*\n> - * ScalerCrop is specified to be in Sensor coordinates.\n> - * So we need to transform the limits to sensor coordinates.\n> - * We can safely assume that the maximum crop limit contains the\n> - * full fov of the dewarper.\n> - */\n> - Rectangle min = cropLimits.first.transformedBetween(cropLimits.second,\n> - scalerMaxCrop_);\n> -\n> - controls[&controls::ScalerCrop] = ControlInfo(min,\n> - scalerMaxCrop_,\n> - scalerMaxCrop_);\n> - data->properties_.set(properties::ScalerCropMaximum, scalerMaxCrop_);\n> - activeCrop_ = scalerMaxCrop_;\n> - }\n> + if (data->usesDewarper_)\n> + dewarper_->updateControlInfos(&data->mainPathStream_, controls);\n> \n> /* Add the IPA registered controls to list of camera controls. */\n> for (const auto &ipaControl : data->ipaControls_)\n> @@ -1376,8 +1368,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> /* Initialize the camera properties. */\n> data->properties_ = data->sensor_->properties();\n> \n> - scalerMaxCrop_ = Rectangle(data->sensor_->resolution());\n> -\n> const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();\n> std::unordered_map params = {\n> { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> @@ -1472,27 +1462,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statBufferReady);\n> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramBufferReady);\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> - std::shared_ptr dwpMediaDevice = enumerator->search(dwp);\n> - if (dwpMediaDevice) {\n> - dewarper_ = std::make_unique(dwpMediaDevice);\n> - if (dewarper_->isValid()) {\n> - dewarper_->outputBufferReady.connect(\n> - this, &PipelineHandlerRkISP1::dewarpBufferReady);\n> -\n> - LOG(RkISP1, Info)\n> - << \"Found DW100 dewarper \" << dewarper_->deviceNode();\n> - } else {\n> - LOG(RkISP1, Warning)\n> - << \"Found DW100 dewarper \" << dewarper_->deviceNode()\n> - << \" but invalid\";\n> -\n> - dewarper_.reset();\n> - }\n> + dewarper_ = ConverterDW100Module::createModule(enumerator);\n> + if (dewarper_) {\n> + dewarper_->outputBufferReady.connect(\n> + this, &PipelineHandlerRkISP1::dewarpBufferReady);\n> + LOG(RkISP1, Debug) << \"Found DW100 dewarper\";\n\nThat is so much cleaner IMO.\n\nAnd it helps us work towards being able to have entity/modules that we\ncan connect to negotiate between themselves on the path to the\napplication.\n\n\n> }\n> \n> /*\n> @@ -1603,37 +1577,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> return;\n> }\n> \n> - /* Handle scaler crop control. */\n> - const auto &crop = request->controls().get(controls::ScalerCrop);\n> - if (crop) {\n> - Rectangle rect = crop.value();\n> -\n> - /*\n> - * ScalerCrop is specified to be in Sensor coordinates.\n> - * So we need to transform it into dewarper coordinates.\n> - * We can safely assume that the maximum crop limit contains the\n> - * full fov of the dewarper.\n> - */\n> - std::pair cropLimits =\n> - dewarper_->inputCropBounds(&data->mainPathStream_);\n> -\n> - rect = rect.transformedBetween(scalerMaxCrop_, cropLimits.second);\n> - int ret = dewarper_->setInputCrop(&data->mainPathStream_,\n> - &rect);\n> - rect = rect.transformedBetween(cropLimits.second, scalerMaxCrop_);\n> - if (!ret && rect != crop.value()) {\n> - /*\n> - * If the rectangle is changed by setInputCrop on the\n> - * dewarper, log a debug message and cache the actual\n> - * applied rectangle for metadata reporting.\n> - */\n> - LOG(RkISP1, Debug)\n> - << \"Applied rectangle \" << rect.toString()\n> - << \" differs from requested \" << crop.value().toString();\n> - }\n> -\n> - activeCrop_ = rect;\n> - }\n> + dewarper_->setControls(&data->mainPathStream_, request->controls());\n> \n> /*\n> * Queue input and output buffers to the dewarper. The output\n> @@ -1642,7 +1586,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> */\n> int ret = dewarper_->queueBuffers(buffer, request->buffers());\n> if (ret < 0) {\n> - LOG(RkISP1, Error) << \"Cannot queue buffers to dewarper: \"\n> + LOG(RkISP1, Error) << \"Failed to queue buffers to dewarper: -\"\n> << strerror(-ret);\n> \n> cancelDewarpRequest(info);\n> @@ -1650,7 +1594,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> return;\n> }\n> \n> - request->metadata().set(controls::ScalerCrop, activeCrop_.value());\n> + dewarper_->populateMetadata(&data->mainPathStream_, request->metadata());\n\nHard to see without additional context not available in the patch - but\nI assume dewarper_ is checked higher in the function so it's safe to use\nin these later stages.\n\n\nReviewed-by: Kieran Bingham \n\n> }\n> \n> void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> -- \n> 2.51.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 8A08AC32EF\n\tfor ;\n\tTue, 25 Nov 2025 17:34:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0F9460A9E;\n\tTue, 25 Nov 2025 18:34:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 120AF609D8\n\tfor ;\n\tTue, 25 Nov 2025 18:34:35 +0100 (CET)","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 AFA016AF;\n\tTue, 25 Nov 2025 18:32:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YCTpZ+Df\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764091945;\n\tbh=kAA7z0k6VeLsrTdvSxzn8961rKDWv1+8coaWi/+xhFY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=YCTpZ+DfzfqbHJbSOa/EUS3gmUTLKC4VF9oRVX9pDnSb+iGq5X9mii49SUlIOYk6S\n\toq1+47CP5TJwR4H6F+RN17MDcLB2aFl7NbtxoIRG40N9U0YVfxQgg8BoHDpjyHBUsI\n\tQXVz75xBcyjtWiQqJfOfIPKgin//SEVjsZxltexY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251125162851.2301793-18-stefan.klug@ideasonboard.com>","References":"<20251125162851.2301793-1-stefan.klug@ideasonboard.com>\n\t<20251125162851.2301793-18-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v3 17/29] libcamera: rkisp1: Use the dw100 converter\n\tmodule instead of the generic v4l2 converter","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Nov 2025 17:34:31 +0000","Message-ID":"<176409207161.567526.14389686667367886866@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]