[{"id":38357,"web_url":"https://patchwork.libcamera.org/comment/38357/","msgid":"<0793a989-e701-4b58-abc7-d26ea8e38108@ideasonboard.com>","date":"2026-03-13T15:19:45","subject":"Re: [PATCH v4 3/7] libcamera: mali-c55: Split TPG and Inline camera\n\thandling","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2026. 03. 13. 12:33 keltezéssel, Jacopo Mondi írta:\n> In order to prepare to support memory input cameras, split the handling of\n> the TPG and Inline camera cases.\n> \n> The Mali C55 pipeline handler uses the entity and subdevice stored in the\n> CameraData to support both the handling of the TPG and of the Inline (CSI-2\n> + sensor) use cases. Adding support for memory cameras by using the CRU unit\n> would add yet-another special case making the code harder to follow and\n> more prone to errors.\n> \n> Split the handling of the TPG and Inline cameras by introducing an\n> std::variant<> variable and to deflect the functions called on the\n> camera data to the correct type by using overloaded std::visit<>().\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 276 ++++++++++++++++-----------\n> 1 file changed, 167 insertions(+), 109 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> index c209b0b070b1..6581d13bbc52 100644\n> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> @@ -11,12 +11,14 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> #include \n> \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -92,17 +94,90 @@ struct MaliC55FrameInfo {\n> class MaliC55CameraData : public Camera::Private\n> {\n> public:\n> -\tMaliC55CameraData(PipelineHandler *pipe, MediaEntity *entity)\n> -\t\t: Camera::Private(pipe), entity_(entity)\n> +\tstruct Tpg {\n> +\t\tstd::vector sizes(unsigned int mbusCode) const;\n> +\n> +\t\tSize resolution_;\n> +\t\tstd::unique_ptr sd_;\n> +\t};\n> +\n> +\tstruct Inline {\n> +\t\tstd::unique_ptr csi2_;\n> +\t\tstd::unique_ptr sensor_;\n> +\t};\n> +\tusing CameraType = std::variant;\n> +\n> +\tMaliC55CameraData(PipelineHandler *pipe)\n> +\t\t: Camera::Private(pipe)\n> \t{\n> \t}\n> \n> -\tint init();\n> \tint loadIPA();\n> \n> -\t/* Deflect these functionalities to either TPG or CameraSensor. */\n> -\tstd::vector sizes(unsigned int mbusCode) const;\n> -\tSize resolution() const;\n> +\tint initTpg(MediaEntity *entity);\n> +\tint initInline(MediaEntity *entity);\n> +\n> +\tstd::vector sizes(unsigned int mbusCode) const\n> +\t{\n> +\t\treturn std::visit(utils::overloaded{\n> +\t\t\t\t[&](const Tpg &tpg) -> std::vector {\n> +\t\t\t\t\treturn tpg.sizes(mbusCode);\n> +\t\t\t\t},\n> +\t\t\t\t[&](const Inline &in) -> std::vector {\n> +\t\t\t\t\treturn in.sensor_->sizes(mbusCode);\n> +\t\t\t\t} },\n> +\t\t\t\tinput_);\n\nIt's a minor thing, but I personally like the following indentation a lot better:\n\nreturn std::visit(utils::overloaded{\n\t[&](...) {\n\t\t....\n\t},\n\t[&](...) {\n\t\t...\n\t},\n}, input_);\n\nIf checkstyle prefers the current one, then please ignore.\n\n\n> +\t}\n> [...]\n> \n> void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls)\n> {\n> -\tif (!sensor_)\n> +\tif (std::holds_alternative(input_))\n> \t\treturn;\n> \n> \tIPACameraSensorInfo sensorInfo;\n> -\tint ret = sensor_->sensorInfo(&sensorInfo);\n> +\tint ret = sensor()->sensorInfo(&sensorInfo);\n\nFor example here, and in some other places it, seems better to me to do something like this:\n\n CameraSensor *sensor = data->sensor();\n if (!sensor)\n return;\n\n ...\n sensor->sensorInfo(...);\n\nassuming MaliC55CameraData::sensor() is changed to not to use ASSERT(). It would be similar\nto what the previous version does: `if (!sensor_)`.\n\nGenerally I think if both work, it's better to use `std::get_if` + checking for nullptr\ninstead of `std::holds_alternative()` + doing something else to get the actual data.\n\n\n> \tif (ret) {\n> \t\tLOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> \t\treturn;\n> @@ -379,7 +436,7 @@ int MaliC55CameraData::loadIPA()\n> \tint ret;\n> \n> \t/* Do not initialize IPA for TPG. */\n> -\tif (!sensor_)\n> +\tif (std::holds_alternative(input_))\n> \t\treturn 0;\n> \n> \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> @@ -388,20 +445,20 @@ int MaliC55CameraData::loadIPA()\n> \n> \tipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);\n> \n> -\tstd::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + \".yaml\",\n> +\tstd::string ipaTuningFile = ipa_->configurationFile(sensor()->model() + \".yaml\",\n> \t\t\t\t\t\t\t \"uncalibrated.yaml\");\n> \n> \t/* We need to inform the IPA of the sensor configuration */\n> \tipa::mali_c55::IPAConfigInfo ipaConfig{};\n> \n> -\tret = sensor_->sensorInfo(&ipaConfig.sensorInfo);\n> +\tret = sensor()->sensorInfo(&ipaConfig.sensorInfo);\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tipaConfig.sensorControls = sensor_->controls();\n> +\tipaConfig.sensorControls = sensor()->controls();\n> \n> \tControlInfoMap ipaControls;\n> -\tret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig,\n> +\tret = ipa_->init({ ipaTuningFile, sensor()->model() }, ipaConfig,\n> \t\t\t &ipaControls);\n> \tif (ret) {\n> \t\tLOG(MaliC55, Error) << \"Failed to initialise the Mali-C55 IPA\";\n> @@ -444,13 +501,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> \t * The TPG doesn't support flips, so we only need to calculate a\n> \t * transform if we have a sensor.\n> \t */\n> -\tif (data_->sensor_) {\n> +\tif (std::holds_alternative(data_->input_)) {\n> +\t\tcombinedTransform_ = Transform::Rot0;\n> +\t} else {\n> \t\tOrientation requestedOrientation = orientation;\n> -\t\tcombinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> +\t\tcombinedTransform_ = data_->sensor()->computeTransform(&orientation);\n> \t\tif (orientation != requestedOrientation)\n> \t\t\tstatus = Adjusted;\n> -\t} else {\n> -\t\tcombinedTransform_ = Transform::Rot0;\n> \t}\n> \n> \t/* Only 2 streams available. */\n> @@ -927,11 +984,12 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> \n> \t/* Link the graph depending if we are operating the TPG or a sensor. */\n> \tMaliC55CameraData *data = cameraData(camera);\n> -\tif (data->csi_) {\n> -\t\tconst MediaEntity *csiEntity = data->csi_->entity();\n> -\t\tret = csiEntity->getPadByIndex(1)->links()[0]->setEnabled(true);\n> +\tif (std::holds_alternative(data->input_)) {\n> +\t\tconst MediaEntity *tpgEntity = data->subdev()->entity();\n> +\t\tret = tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true);\n> \t} else {\n> -\t\tret = data->entity_->getPadByIndex(0)->links()[0]->setEnabled(true);\n> +\t\tconst MediaEntity *csi2Entity = data->csi2()->entity();\n> +\t\tret = csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true);\n> \t}\n\nI think the above part is definitely a place where std::visit could be used. E.g.\n(after all patches):\n\n\tret = std::visit(utils::overloaded {\n\t\t[](MaliC55CameraData::Tpg &x) {\n\t\t\tconst MediaEntity *tpgEntity = x.sd_->entity();\n\t\t\treturn tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true);\n\t\t},\n\t\t[](MaliC55CameraData::Inline &x) {\n\t\t\tconst MediaEntity *csi2Entity = x.csi2_->entity();\n\t\t\treturn csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true);\n\t\t},\n\t\t[&](MaliC55CameraData::Memory &) {\n\t\t\tconst MediaEntity *ivcEntity = ivcSd_->entity();\n\t\t\treturn ivcEntity->getPadByIndex(1)->links()[0]->setEnabled(true);\n\t\t},\n\t}, data->input_);\n\n\n> \tif (ret)\n> \t\treturn ret;\n> @@ -939,26 +997,24 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> \tMaliC55CameraConfiguration *maliConfig =\n> \t\tstatic_cast(config);\n> \tV4L2SubdeviceFormat subdevFormat = maliConfig->sensorFormat_;\n> -\tret = data->sd_->getFormat(0, &subdevFormat);\n> -\tif (ret)\n> -\t\treturn ret;\n> \n> -\tif (data->sensor_) {\n> -\t\tret = data->sensor_->setFormat(&subdevFormat,\n> -\t\t\t\t\t maliConfig->combinedTransform());\n> +\t/* Apply format to the origin of the pipeline and propagate it. */\n> +\tif (std::holds_alternative(data->input_)) {\n> +\t\tret = data->subdev()->setFormat(0, &subdevFormat);\n> +\t} else {\n> +\t\tret = data->sensor()->setFormat(&subdevFormat,\n> +\t\t\t\t\t\tmaliConfig->combinedTransform());\n> \t\tif (ret)\n> \t\t\treturn ret;\n> -\t}\n> \n> -\tif (data->csi_) {\n> -\t\tret = data->csi_->setFormat(0, &subdevFormat);\n> +\t\tret = data->csi2()->setFormat(0, &subdevFormat);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \n> -\t\tret = data->csi_->getFormat(1, &subdevFormat);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tret = data->csi2()->getFormat(1, &subdevFormat);\n> \t}\n\nI have the same impression here as well.\n\n\n> +\tif (ret)\n> +\t\treturn ret;\n> \n> \tV4L2DeviceFormat statsFormat;\n> \tret = stats_->getFormat(&statsFormat);\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 28E8EBDCC1\n\tfor ;\n\tFri, 13 Mar 2026 15:19:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F1D762689;\n\tFri, 13 Mar 2026 16:19:50 +0100 (CET)","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 BC9CF6261B\n\tfor ;\n\tFri, 13 Mar 2026 16:19:48 +0100 (CET)","from [192.168.33.26] (185.182.214.153.nat.pool.zt.hu\n\t[185.182.214.153])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AB335A5;\n\tFri, 13 Mar 2026 16:18:39 +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=\"MMPsbdRC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1773415119;\n\tbh=DXI748XwWQTTBGA39BSXqgc0GNUyaWDdT+xz0Jny61Y=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=MMPsbdRCNGfRAq00bKdBb+oE7kVgR2l1fFD0iefDd2hj9/NZImPGeTOoTqBTUXtKk\n\tQNPwkDCJ3glzOTnnFbvQrvDpS/ZophMfkFsRAv25GVYz5+OASQYjaiKKKZu/jlAzqC\n\tFyl+39YPcrTzSj2ATGSSzmJzYCuTUtswK/t6JstQ=","Message-ID":"<0793a989-e701-4b58-abc7-d26ea8e38108@ideasonboard.com>","Date":"Fri, 13 Mar 2026 16:19:45 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 3/7] libcamera: mali-c55: Split TPG and Inline camera\n\thandling","To":"Jacopo Mondi ,\n\tDaniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20260313-mali-cru-v4-0-c0d9bc8cd8fa@ideasonboard.com>\n\t<20260313-mali-cru-v4-3-c0d9bc8cd8fa@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20260313-mali-cru-v4-3-c0d9bc8cd8fa@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":38360,"web_url":"https://patchwork.libcamera.org/comment/38360/","msgid":"","date":"2026-03-13T16:03:52","subject":"Re: [PATCH v4 3/7] libcamera: mali-c55: Split TPG and Inline camera\n\thandling","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Fri, Mar 13, 2026 at 04:19:45PM +0100, Barnabás Pőcze wrote:\n> Hi\n>\n> 2026. 03. 13. 12:33 keltezéssel, Jacopo Mondi írta:\n> > In order to prepare to support memory input cameras, split the handling of\n> > the TPG and Inline camera cases.\n> >\n> > The Mali C55 pipeline handler uses the entity and subdevice stored in the\n> > CameraData to support both the handling of the TPG and of the Inline (CSI-2\n> > + sensor) use cases. Adding support for memory cameras by using the CRU unit\n> > would add yet-another special case making the code harder to follow and\n> > more prone to errors.\n> >\n> > Split the handling of the TPG and Inline cameras by introducing an\n> > std::variant<> variable and to deflect the functions called on the\n> > camera data to the correct type by using overloaded std::visit<>().\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 276 ++++++++++++++++-----------\n> > 1 file changed, 167 insertions(+), 109 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > index c209b0b070b1..6581d13bbc52 100644\n> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > @@ -11,12 +11,14 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > @@ -92,17 +94,90 @@ struct MaliC55FrameInfo {\n> > class MaliC55CameraData : public Camera::Private\n> > {\n> > public:\n> > -\tMaliC55CameraData(PipelineHandler *pipe, MediaEntity *entity)\n> > -\t\t: Camera::Private(pipe), entity_(entity)\n> > +\tstruct Tpg {\n> > +\t\tstd::vector sizes(unsigned int mbusCode) const;\n> > +\n> > +\t\tSize resolution_;\n> > +\t\tstd::unique_ptr sd_;\n> > +\t};\n> > +\n> > +\tstruct Inline {\n> > +\t\tstd::unique_ptr csi2_;\n> > +\t\tstd::unique_ptr sensor_;\n> > +\t};\n> > +\tusing CameraType = std::variant;\n> > +\n> > +\tMaliC55CameraData(PipelineHandler *pipe)\n> > +\t\t: Camera::Private(pipe)\n> > \t{\n> > \t}\n> > -\tint init();\n> > \tint loadIPA();\n> > -\t/* Deflect these functionalities to either TPG or CameraSensor. */\n> > -\tstd::vector sizes(unsigned int mbusCode) const;\n> > -\tSize resolution() const;\n> > +\tint initTpg(MediaEntity *entity);\n> > +\tint initInline(MediaEntity *entity);\n> > +\n> > +\tstd::vector sizes(unsigned int mbusCode) const\n> > +\t{\n> > +\t\treturn std::visit(utils::overloaded{\n> > +\t\t\t\t[&](const Tpg &tpg) -> std::vector {\n> > +\t\t\t\t\treturn tpg.sizes(mbusCode);\n> > +\t\t\t\t},\n> > +\t\t\t\t[&](const Inline &in) -> std::vector {\n> > +\t\t\t\t\treturn in.sensor_->sizes(mbusCode);\n> > +\t\t\t\t} },\n> > +\t\t\t\tinput_);\n>\n> It's a minor thing, but I personally like the following indentation a lot better:\n>\npersonal preferences I would say\n\n> return std::visit(utils::overloaded{\n> \t[&](...) {\n> \t\t....\n> \t},\n> \t[&](...) {\n> \t\t...\n> \t},\n> }, input_);\n>\n> If checkstyle prefers the current one, then please ignore.\n\ncheckstyle wants it one more indentation level to the right.\nAnything goes for me, it is a detail.\n\n>\n>\n> > +\t}\n> > [...]\n> > void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls)\n> > {\n> > -\tif (!sensor_)\n> > +\tif (std::holds_alternative(input_))\n> > \t\treturn;\n> > \tIPACameraSensorInfo sensorInfo;\n> > -\tint ret = sensor_->sensorInfo(&sensorInfo);\n> > +\tint ret = sensor()->sensorInfo(&sensorInfo);\n>\n> For example here, and in some other places it, seems better to me to do something like this:\n>\n> CameraSensor *sensor = data->sensor();\n> if (!sensor)\n> return;\n>\n> ...\n> sensor->sensorInfo(...);\n>\n> assuming MaliC55CameraData::sensor() is changed to not to use ASSERT(). It would be similar\n> to what the previous version does: `if (!sensor_)`.\n\n> Generally I think if both work, it's better to use `std::get_if` + checking for nullptr\n> instead of `std::holds_alternative()` + doing something else to get the actual data.\n>\n\nWhy is it better ?\n\nDoens't std::variant<> allow you to be explicit in checking the\ncontained value instead of doing the same but through other means ?\n\nIn this case I think\n\n\tif (std::holds_alternative(input_))\n\nit's more clear than\n\n if (!sensor)\n\nas I immediately know what I'm checking for, instead of having to dig\nout in what case !sensor means Tpg, Inline or Memory.\n\n>\n> > \tif (ret) {\n> > \t\tLOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> > \t\treturn;\n> > @@ -379,7 +436,7 @@ int MaliC55CameraData::loadIPA()\n> > \tint ret;\n> > \t/* Do not initialize IPA for TPG. */\n> > -\tif (!sensor_)\n> > +\tif (std::holds_alternative(input_))\n> > \t\treturn 0;\n> > \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > @@ -388,20 +445,20 @@ int MaliC55CameraData::loadIPA()\n> > \tipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);\n> > -\tstd::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + \".yaml\",\n> > +\tstd::string ipaTuningFile = ipa_->configurationFile(sensor()->model() + \".yaml\",\n> > \t\t\t\t\t\t\t \"uncalibrated.yaml\");\n> > \t/* We need to inform the IPA of the sensor configuration */\n> > \tipa::mali_c55::IPAConfigInfo ipaConfig{};\n> > -\tret = sensor_->sensorInfo(&ipaConfig.sensorInfo);\n> > +\tret = sensor()->sensorInfo(&ipaConfig.sensorInfo);\n> > \tif (ret)\n> > \t\treturn ret;\n> > -\tipaConfig.sensorControls = sensor_->controls();\n> > +\tipaConfig.sensorControls = sensor()->controls();\n> > \tControlInfoMap ipaControls;\n> > -\tret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig,\n> > +\tret = ipa_->init({ ipaTuningFile, sensor()->model() }, ipaConfig,\n> > \t\t\t &ipaControls);\n> > \tif (ret) {\n> > \t\tLOG(MaliC55, Error) << \"Failed to initialise the Mali-C55 IPA\";\n> > @@ -444,13 +501,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> > \t * The TPG doesn't support flips, so we only need to calculate a\n> > \t * transform if we have a sensor.\n> > \t */\n> > -\tif (data_->sensor_) {\n> > +\tif (std::holds_alternative(data_->input_)) {\n> > +\t\tcombinedTransform_ = Transform::Rot0;\n> > +\t} else {\n> > \t\tOrientation requestedOrientation = orientation;\n> > -\t\tcombinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> > +\t\tcombinedTransform_ = data_->sensor()->computeTransform(&orientation);\n> > \t\tif (orientation != requestedOrientation)\n> > \t\t\tstatus = Adjusted;\n> > -\t} else {\n> > -\t\tcombinedTransform_ = Transform::Rot0;\n> > \t}\n> > \t/* Only 2 streams available. */\n> > @@ -927,11 +984,12 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> > \t/* Link the graph depending if we are operating the TPG or a sensor. */\n> > \tMaliC55CameraData *data = cameraData(camera);\n> > -\tif (data->csi_) {\n> > -\t\tconst MediaEntity *csiEntity = data->csi_->entity();\n> > -\t\tret = csiEntity->getPadByIndex(1)->links()[0]->setEnabled(true);\n> > +\tif (std::holds_alternative(data->input_)) {\n> > +\t\tconst MediaEntity *tpgEntity = data->subdev()->entity();\n> > +\t\tret = tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true);\n> > \t} else {\n> > -\t\tret = data->entity_->getPadByIndex(0)->links()[0]->setEnabled(true);\n> > +\t\tconst MediaEntity *csi2Entity = data->csi2()->entity();\n> > +\t\tret = csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true);\n> > \t}\n>\n> I think the above part is definitely a place where std::visit could be used. E.g.\n> (after all patches):\n>\n> \tret = std::visit(utils::overloaded {\n> \t\t[](MaliC55CameraData::Tpg &x) {\n> \t\t\tconst MediaEntity *tpgEntity = x.sd_->entity();\n> \t\t\treturn tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true);\n> \t\t},\n> \t\t[](MaliC55CameraData::Inline &x) {\n> \t\t\tconst MediaEntity *csi2Entity = x.csi2_->entity();\n> \t\t\treturn csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true);\n> \t\t},\n> \t\t[&](MaliC55CameraData::Memory &) {\n> \t\t\tconst MediaEntity *ivcEntity = ivcSd_->entity();\n> \t\t\treturn ivcEntity->getPadByIndex(1)->links()[0]->setEnabled(true);\n> \t\t},\n> \t}, data->input_);\n>\n\nI can do it.\n\nI'll ask you to test the series though, I don't have a device right\nnow, I'll be away next week and I don't want this series which has\nbeen around for 1 year to be delayed again for minor details.\n\n>\n> > \tif (ret)\n> > \t\treturn ret;\n> > @@ -939,26 +997,24 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> > \tMaliC55CameraConfiguration *maliConfig =\n> > \t\tstatic_cast(config);\n> > \tV4L2SubdeviceFormat subdevFormat = maliConfig->sensorFormat_;\n> > -\tret = data->sd_->getFormat(0, &subdevFormat);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\tif (data->sensor_) {\n> > -\t\tret = data->sensor_->setFormat(&subdevFormat,\n> > -\t\t\t\t\t maliConfig->combinedTransform());\n> > +\t/* Apply format to the origin of the pipeline and propagate it. */\n> > +\tif (std::holds_alternative(data->input_)) {\n> > +\t\tret = data->subdev()->setFormat(0, &subdevFormat);\n> > +\t} else {\n> > +\t\tret = data->sensor()->setFormat(&subdevFormat,\n> > +\t\t\t\t\t\tmaliConfig->combinedTransform());\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > -\t}\n> > -\tif (data->csi_) {\n> > -\t\tret = data->csi_->setFormat(0, &subdevFormat);\n> > +\t\tret = data->csi2()->setFormat(0, &subdevFormat);\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > -\t\tret = data->csi_->getFormat(1, &subdevFormat);\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\t\tret = data->csi2()->getFormat(1, &subdevFormat);\n> > \t}\n>\n> I have the same impression here as well.\n>\n\nok\n\nThanks\n j\n\n>\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > \tV4L2DeviceFormat statsFormat;\n> > \tret = stats_->getFormat(&statsFormat);\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 1AAB9BDCC1\n\tfor ;\n\tFri, 13 Mar 2026 16:03:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B8FA62695;\n\tFri, 13 Mar 2026 17:03:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9ABBB6261B\n\tfor ;\n\tFri, 13 Mar 2026 17:03:55 +0100 (CET)","from ideasonboard.com (unknown [37.159.122.93])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 316A1103D;\n\tFri, 13 Mar 2026 17:02:46 +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=\"Y7SqB+Vu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1773417766;\n\tbh=d+yjrHTGKIPs+Ta4gkYgUMDXJZ4VSKSX2rEOO7RKGO4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y7SqB+VuKoHEQPr/xRvMrO6NaBow8T+o5JJXe4yLqzeNMu6v1cjgqVWazKNRmu5vt\n\tL4pIV+szLYKUDPolJF9blp15bzKxOEAuWTj580DRcn+eMqeNn5rri0MUHEhyNWNQfB\n\tNRubm3XnxnVg0HMe9lyIg8GmRCBL5QdNdNY7U70k=","Date":"Fri, 13 Mar 2026 17:03:52 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tDaniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 3/7] libcamera: mali-c55: Split TPG and Inline camera\n\thandling","Message-ID":"","References":"<20260313-mali-cru-v4-0-c0d9bc8cd8fa@ideasonboard.com>\n\t<20260313-mali-cru-v4-3-c0d9bc8cd8fa@ideasonboard.com>\n\t<0793a989-e701-4b58-abc7-d26ea8e38108@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<0793a989-e701-4b58-abc7-d26ea8e38108@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":38361,"web_url":"https://patchwork.libcamera.org/comment/38361/","msgid":"","date":"2026-03-18T09:05:31","subject":"Re: [PATCH v4 3/7] libcamera: mali-c55: Split TPG and Inline camera\n\thandling","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2026. 03. 13. 17:03 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Fri, Mar 13, 2026 at 04:19:45PM +0100, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2026. 03. 13. 12:33 keltezéssel, Jacopo Mondi írta:\n>>> In order to prepare to support memory input cameras, split the handling of\n>>> the TPG and Inline camera cases.\n>>>\n>>> The Mali C55 pipeline handler uses the entity and subdevice stored in the\n>>> CameraData to support both the handling of the TPG and of the Inline (CSI-2\n>>> + sensor) use cases. Adding support for memory cameras by using the CRU unit\n>>> would add yet-another special case making the code harder to follow and\n>>> more prone to errors.\n>>>\n>>> Split the handling of the TPG and Inline cameras by introducing an\n>>> std::variant<> variable and to deflect the functions called on the\n>>> camera data to the correct type by using overloaded std::visit<>().\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 276 ++++++++++++++++-----------\n>>> 1 file changed, 167 insertions(+), 109 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>>> index c209b0b070b1..6581d13bbc52 100644\n>>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>>> [...]\n>>> void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls)\n>>> {\n>>> -\tif (!sensor_)\n>>> +\tif (std::holds_alternative(input_))\n>>> \t\treturn;\n>>> \tIPACameraSensorInfo sensorInfo;\n>>> -\tint ret = sensor_->sensorInfo(&sensorInfo);\n>>> +\tint ret = sensor()->sensorInfo(&sensorInfo);\n>>\n>> For example here, and in some other places it, seems better to me to do something like this:\n>>\n>> CameraSensor *sensor = data->sensor();\n>> if (!sensor)\n>> return;\n>>\n>> ...\n>> sensor->sensorInfo(...);\n>>\n>> assuming MaliC55CameraData::sensor() is changed to not to use ASSERT(). It would be similar\n>> to what the previous version does: `if (!sensor_)`.\n> \n>> Generally I think if both work, it's better to use `std::get_if` + checking for nullptr\n>> instead of `std::holds_alternative()` + doing something else to get the actual data.\n>>\n> \n> Why is it better ?\n> \n> Doens't std::variant<> allow you to be explicit in checking the\n> contained value instead of doing the same but through other means ?\n> \n> In this case I think\n> \n> \tif (std::holds_alternative(input_))\n> \n> it's more clear than\n> \n> if (!sensor)\n> \n> as I immediately know what I'm checking for, instead of having to dig\n> out in what case !sensor means Tpg, Inline or Memory.\n> [...]\n\nI see, I was thinking along the lines of \"if there is a sensor, then do this\", in that\ncase one could argue that it is not actually that important whether it is A, B, C, or D,\nthe only important thing is: is there a sensor? But in any case, it's a very minor thing.\n\n\nRegards,\nBarnabás Pőcze","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 32825BD87C\n\tfor ;\n\tWed, 18 Mar 2026 09:05:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2165962723;\n\tWed, 18 Mar 2026 10:05:37 +0100 (CET)","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 5103762010\n\tfor ;\n\tWed, 18 Mar 2026 10:05:35 +0100 (CET)","from [192.168.33.40] (185.182.214.153.nat.pool.zt.hu\n\t[185.182.214.153])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5BB4D460;\n\tWed, 18 Mar 2026 10:04:22 +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=\"ZyyibvXR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1773824662;\n\tbh=Hu2Hkfz8DJlVnc4fAmc09stUP8LeT4jepWIvjPOyOn4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ZyyibvXR2+ZEwjrKm11VrqZd4FT9cU672bfId6IU9IBBNt1Q4c+OyqbhhoKBVWG92\n\tPUPvoKwlhWlwnZp6TzqIgQ18kVHUWa3y+bTUkHvUwT9NDc/EK5ES3a/M/tkFljVITZ\n\tQE6R0KmXCyDuAr4LSS3zHAm0yl2eglts+SGsItec=","Message-ID":"","Date":"Wed, 18 Mar 2026 10:05:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 3/7] libcamera: mali-c55: Split TPG and Inline camera\n\thandling","To":"Jacopo Mondi ","Cc":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20260313-mali-cru-v4-0-c0d9bc8cd8fa@ideasonboard.com>\n\t<20260313-mali-cru-v4-3-c0d9bc8cd8fa@ideasonboard.com>\n\t<0793a989-e701-4b58-abc7-d26ea8e38108@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]