[{"id":12058,"web_url":"https://patchwork.libcamera.org/comment/12058/","msgid":"<20200820082004.jregxmy6qimapdtx@uno.localdomain>","date":"2020-08-20T08:20:04","subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote:\n> In preparation of supporting both the main and self path move all link\nall links\n\n> setup to be part of the configuration step. The main path is still the\n> only possible path.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++-----------\n> 1 file changed, 42 insertions(+), 37 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a0e36a44d8d91260..38382a6894dac22a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -211,7 +211,8 @@ private:\n> \tfriend RkISP1CameraData;\n> \tfriend RkISP1Frames;\n>\n> -\tint initLinks();\n> +\tint initLinks(const Camera *camera,\n> +\t\t const RkISP1CameraConfiguration &config);\n> \tint createCamera(MediaEntity *sensor);\n> \tvoid tryCompleteRequest(Request *request);\n> \tvoid bufferReady(FrameBuffer *buffer);\n> @@ -601,28 +602,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tCameraSensor *sensor = data->sensor_;\n> \tint ret;\n>\n> -\t/*\n> -\t * Configure the sensor links: enable the link corresponding to this\n> -\t * camera and disable all the other sensor links.\n> -\t */\n> -\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> -\n> -\tfor (MediaLink *link : pad->links()) {\n> -\t\tbool enable = link->source()->entity() == sensor->entity();\n> -\n> -\t\tif (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)\n> -\t\t\tcontinue;\n> -\n> -\t\tLOG(RkISP1, Debug)\n> -\t\t\t<< (enable ? \"Enabling\" : \"Disabling\")\n> -\t\t\t<< \" link from sensor '\"\n> -\t\t\t<< link->source()->entity()->name()\n> -\t\t\t<< \"' to ISP\";\n> -\n> -\t\tret = link->setEnabled(enable);\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\t}\n> +\tret = initLinks(camera, *config);\n> +\tif (ret)\n> +\t\treturn ret;\n>\n> \t/*\n> \t * Configure the format on the sensor output and propagate it through\n> @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> * Match and Setup\n> */\n>\n> -int PipelineHandlerRkISP1::initLinks()\n> +int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> +\t\t\t\t const RkISP1CameraConfiguration &config)\n> {\n> -\tMediaLink *link;\n> +\tconst RkISP1CameraData *data = cameraData(camera);\n> +\tconst CameraSensor *sensor = data->sensor_;\n\nYou could actually pass sensor directly, don't you ?\n\n> \tint ret;\n>\n> \tret = media_->disableLinks();\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> -\tlink = media_->link(\"rkisp1_isp\", 2, \"rkisp1_resizer_mainpath\", 0);\n> -\tif (!link)\n> -\t\treturn -ENODEV;\n> +\t/*\n> +\t * Configure the sensor links: enable the link corresponding to this\n> +\t * camera.\n> +\t */\n> +\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> +\tfor (MediaLink *link : pad->links()) {\n> +\t\tif (link->source()->entity() != sensor->entity())\n> +\t\t\tcontinue;\n>\n> -\tret = link->setEnabled(true);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\t\tLOG(RkISP1, Debug)\n> +\t\t\t<< \"Enabling link from sensor '\"\n> +\t\t\t<< link->source()->entity()->name()\n> +\t\t\t<< \"' to ISP\";\n> +\n> +\t\tret = link->setEnabled(true);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\tfor (const StreamConfiguration &cfg : config) {\n> +\t\tstd::string resizer;\n> +\t\tif (cfg.stream() == &data->stream_)\n> +\t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> +\t\telse\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2, resizer, 0);\n> +\t\tif (!link)\n> +\t\t\treturn -ENODEV;\n> +\n> +\t\tret = link->setEnabled(true);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n\nI also see a significant behaviour change, the code is not only moved.\nTo be able to actually spot what has changed, could this be broken up\nin two patches ? One that moves the existing code to a separate\nfunction, the other that changes it on top ?\n\n>\n> \treturn 0;\n> }\n> @@ -1021,12 +1032,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>\n> -\t/* Configure default links. */\n> -\tif (initLinks() < 0) {\n> -\t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> -\t\treturn false;\n> -\t}\n> -\n> \t/*\n> \t * Enumerate all sensors connected to the ISP and create one\n> \t * camera instance for each of them.\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 6CB95BD87C\n\tfor ;\n\tThu, 20 Aug 2020 08:16:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C86F861FA6;\n\tThu, 20 Aug 2020 10:16:30 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DBBF61ED9\n\tfor ;\n\tThu, 20 Aug 2020 10:16:29 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 31B0720008;\n\tThu, 20 Aug 2020 08:16:27 +0000 (UTC)"],"X-Originating-IP":"82.52.18.94","Date":"Thu, 20 Aug 2020 10:20:04 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200820082004.jregxmy6qimapdtx@uno.localdomain>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200813005246.3265807-4-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12072,"web_url":"https://patchwork.libcamera.org/comment/12072/","msgid":"<20200820145559.GI6593@pendragon.ideasonboard.com>","date":"2020-08-20T14:55:59","subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote:\n> In preparation of supporting both the main and self path move all link\n> setup to be part of the configuration step. The main path is still the\n> only possible path.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++-----------\n> 1 file changed, 42 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a0e36a44d8d91260..38382a6894dac22a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -211,7 +211,8 @@ private:\n> \tfriend RkISP1CameraData;\n> \tfriend RkISP1Frames;\n> \n> -\tint initLinks();\n> +\tint initLinks(const Camera *camera,\n> +\t\t const RkISP1CameraConfiguration &config);\n> \tint createCamera(MediaEntity *sensor);\n> \tvoid tryCompleteRequest(Request *request);\n> \tvoid bufferReady(FrameBuffer *buffer);\n> @@ -601,28 +602,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tCameraSensor *sensor = data->sensor_;\n> \tint ret;\n> \n> -\t/*\n> -\t * Configure the sensor links: enable the link corresponding to this\n> -\t * camera and disable all the other sensor links.\n> -\t */\n> -\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> -\n> -\tfor (MediaLink *link : pad->links()) {\n> -\t\tbool enable = link->source()->entity() == sensor->entity();\n> -\n> -\t\tif (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)\n> -\t\t\tcontinue;\n> -\n> -\t\tLOG(RkISP1, Debug)\n> -\t\t\t<< (enable ? \"Enabling\" : \"Disabling\")\n> -\t\t\t<< \" link from sensor '\"\n> -\t\t\t<< link->source()->entity()->name()\n> -\t\t\t<< \"' to ISP\";\n> -\n> -\t\tret = link->setEnabled(enable);\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\t}\n> +\tret = initLinks(camera, *config);\n> +\tif (ret)\n> +\t\treturn ret;\n> \n> \t/*\n> \t * Configure the format on the sensor output and propagate it through\n> @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> * Match and Setup\n> */\n> \n> -int PipelineHandlerRkISP1::initLinks()\n> +int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> +\t\t\t\t const RkISP1CameraConfiguration &config)\n> {\n> -\tMediaLink *link;\n> +\tconst RkISP1CameraData *data = cameraData(camera);\n> +\tconst CameraSensor *sensor = data->sensor_;\n> \tint ret;\n> \n> \tret = media_->disableLinks();\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tlink = media_->link(\"rkisp1_isp\", 2, \"rkisp1_resizer_mainpath\", 0);\n> -\tif (!link)\n> -\t\treturn -ENODEV;\n> +\t/*\n> +\t * Configure the sensor links: enable the link corresponding to this\n> +\t * camera.\n> +\t */\n> +\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> +\tfor (MediaLink *link : pad->links()) {\n> +\t\tif (link->source()->entity() != sensor->entity())\n> +\t\t\tcontinue;\n> \n> -\tret = link->setEnabled(true);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\t\tLOG(RkISP1, Debug)\n> +\t\t\t<< \"Enabling link from sensor '\"\n> +\t\t\t<< link->source()->entity()->name()\n> +\t\t\t<< \"' to ISP\";\n> +\n> +\t\tret = link->setEnabled(true);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n\nThis will break switching between cameras, as you would have both links\nenabled. You not only need to enable the link to the sensor, but disable\nlinks to other sensors. The link disable step needs to happen first, as\nthe driver won't (or at least shouldn't) allow two links to be enabled\nat the same time.\n\n> +\n> +\tfor (const StreamConfiguration &cfg : config) {\n> +\t\tstd::string resizer;\n> +\t\tif (cfg.stream() == &data->stream_)\n> +\t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> +\t\telse\n> +\t\t\treturn -EINVAL;\n\nI'm not sure to get this change, it doesn't match the commit message. I\nsuspect it belongs to a different patch.\n\n> +\n> +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2, resizer, 0);\n> +\t\tif (!link)\n> +\t\t\treturn -ENODEV;\n> +\n> +\t\tret = link->setEnabled(true);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> \n> \treturn 0;\n> }\n> @@ -1021,12 +1032,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> \n> -\t/* Configure default links. */\n> -\tif (initLinks() < 0) {\n> -\t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> -\t\treturn false;\n> -\t}\n> -\n> \t/*\n> \t * Enumerate all sensors connected to the ISP and create one\n> \t * camera instance for each of them.","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 4D384BE173\n\tfor ;\n\tThu, 20 Aug 2020 14:56:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3FD061B4F;\n\tThu, 20 Aug 2020 16:56:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDDC760381\n\tfor ;\n\tThu, 20 Aug 2020 16:56:17 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5CB9623D;\n\tThu, 20 Aug 2020 16:56:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RkOIixfn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1597935377;\n\tbh=qztxeq+7sWITsAPjelvlCLbyavDfIMDnGbayzTlTAq4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RkOIixfndNAZ0nMOuBSPpfmRfuAhmmMTTtQQtW0W95Cm+k/K2ms1utQ6auXVmjwXy\n\tqzB90tTcLt8mKsJVhMTUG+wE2aF3oFXybCz2nSG3nfSJciIRrxLjhdNcLVLjcj7hWe\n\tBUqR6bOXiUnXBBgMwubLxXxxRg6hHL+sIQfWxiQY=","Date":"Thu, 20 Aug 2020 17:55:59 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200820145559.GI6593@pendragon.ideasonboard.com>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200813005246.3265807-4-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12483,"web_url":"https://patchwork.libcamera.org/comment/12483/","msgid":"<20200913132313.GA1127199@oden.dyn.berto.se>","date":"2020-09-13T13:23:13","subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-08-20 10:20:04 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote:\n> > In preparation of supporting both the main and self path move all link\n> all links\n> \n> > setup to be part of the configuration step. The main path is still the\n> > only possible path.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++-----------\n> > 1 file changed, 42 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index a0e36a44d8d91260..38382a6894dac22a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -211,7 +211,8 @@ private:\n> > \tfriend RkISP1CameraData;\n> > \tfriend RkISP1Frames;\n> >\n> > -\tint initLinks();\n> > +\tint initLinks(const Camera *camera,\n> > +\t\t const RkISP1CameraConfiguration &config);\n> > \tint createCamera(MediaEntity *sensor);\n> > \tvoid tryCompleteRequest(Request *request);\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> > @@ -601,28 +602,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tCameraSensor *sensor = data->sensor_;\n> > \tint ret;\n> >\n> > -\t/*\n> > -\t * Configure the sensor links: enable the link corresponding to this\n> > -\t * camera and disable all the other sensor links.\n> > -\t */\n> > -\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> > -\n> > -\tfor (MediaLink *link : pad->links()) {\n> > -\t\tbool enable = link->source()->entity() == sensor->entity();\n> > -\n> > -\t\tif (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tLOG(RkISP1, Debug)\n> > -\t\t\t<< (enable ? \"Enabling\" : \"Disabling\")\n> > -\t\t\t<< \" link from sensor '\"\n> > -\t\t\t<< link->source()->entity()->name()\n> > -\t\t\t<< \"' to ISP\";\n> > -\n> > -\t\tret = link->setEnabled(enable);\n> > -\t\tif (ret < 0)\n> > -\t\t\treturn ret;\n> > -\t}\n> > +\tret = initLinks(camera, *config);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> >\n> > \t/*\n> > \t * Configure the format on the sensor output and propagate it through\n> > @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > * Match and Setup\n> > */\n> >\n> > -int PipelineHandlerRkISP1::initLinks()\n> > +int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> > +\t\t\t\t const RkISP1CameraConfiguration &config)\n> > {\n> > -\tMediaLink *link;\n> > +\tconst RkISP1CameraData *data = cameraData(camera);\n> > +\tconst CameraSensor *sensor = data->sensor_;\n> \n> You could actually pass sensor directly, don't you ?\n\nGrate idea.\n\n> \n> > \tint ret;\n> >\n> > \tret = media_->disableLinks();\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > -\tlink = media_->link(\"rkisp1_isp\", 2, \"rkisp1_resizer_mainpath\", 0);\n> > -\tif (!link)\n> > -\t\treturn -ENODEV;\n> > +\t/*\n> > +\t * Configure the sensor links: enable the link corresponding to this\n> > +\t * camera.\n> > +\t */\n> > +\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> > +\tfor (MediaLink *link : pad->links()) {\n> > +\t\tif (link->source()->entity() != sensor->entity())\n> > +\t\t\tcontinue;\n> >\n> > -\tret = link->setEnabled(true);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > +\t\tLOG(RkISP1, Debug)\n> > +\t\t\t<< \"Enabling link from sensor '\"\n> > +\t\t\t<< link->source()->entity()->name()\n> > +\t\t\t<< \"' to ISP\";\n> > +\n> > +\t\tret = link->setEnabled(true);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfor (const StreamConfiguration &cfg : config) {\n> > +\t\tstd::string resizer;\n> > +\t\tif (cfg.stream() == &data->stream_)\n> > +\t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> > +\t\telse\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2, resizer, 0);\n> > +\t\tif (!link)\n> > +\t\t\treturn -ENODEV;\n> > +\n> > +\t\tret = link->setEnabled(true);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> \n> I also see a significant behaviour change, the code is not only moved.\n> To be able to actually spot what has changed, could this be broken up\n> in two patches ? One that moves the existing code to a separate\n> function, the other that changes it on top ?\n\nI really can't as the idea is to change the behavior. I will rewrite the \ncommit message to better describe this is not simply a movement of code \nbut change of behavior. Instead of setting up the link between DMA and \nISP at match() and sensor and ISP at configure() all links are now setup \nat the configure() time.\n\n> \n> >\n> > \treturn 0;\n> > }\n> > @@ -1021,12 +1032,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> >\n> > -\t/* Configure default links. */\n> > -\tif (initLinks() < 0) {\n> > -\t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> > -\t\treturn false;\n> > -\t}\n> > -\n> > \t/*\n> > \t * Enumerate all sensors connected to the ISP and create one\n> > \t * camera instance for each of them.\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 1A554BF01C\n\tfor ;\n\tSun, 13 Sep 2020 13:23:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A53D462D99;\n\tSun, 13 Sep 2020 15:23:16 +0200 (CEST)","from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8580A62901\n\tfor ;\n\tSun, 13 Sep 2020 15:23:15 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id w11so10583336lfn.2\n\tfor ;\n\tSun, 13 Sep 2020 06:23:15 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\to15sm2352068lfg.226.2020.09.13.06.23.13\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 13 Sep 2020 06:23:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Gi1PFVCV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=ZrQahaWZD09sR6P3tw2+Of1ovmXAZtQUOpyiBQIZxsY=;\n\tb=Gi1PFVCVWSvF6qm8Dqp5qW/CcStVRW9Z6l83SARU+E1eci3poYLrE2xfLomS0TIaXd\n\t5qUdxwyESeXy5cyytug2o+MUaCl+HkYggeGVw1Sd1CEFlHVnWYTjNRV9NNIPrC0xWGih\n\tb0BQqh0DAMcETYhrdhMJJzBbYCmvu7049YlJ8eK8KB9UNDIsmgYBctMD51CNLz4ttzOr\n\t1DcCaG09//vJV2qbTepPcd7MyB7ScCbv6Pm0ya44jPnu7laA5Gp/Q+JmmCYqUzy3t7zI\n\tBlWlqrD8659Q9cybnEgYrGKsZOUyq8uf3jEHm0FaBlSM9XbbUnFcQmMzK4RdyBNW4Y9B\n\tvwGg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=ZrQahaWZD09sR6P3tw2+Of1ovmXAZtQUOpyiBQIZxsY=;\n\tb=oG6vP0Kzy8ld52a/27APQbtJ3rtmnHhjJP1G/HN6lAIWU3cHJae/dYIxIZtxHLUTpG\n\t3TxeGOk/N2Uaj4RKnDHTzksA6fztmQUAmwQx+9gm59BVLQAh3+Lz3GtGBcRzSwB3iWgE\n\t/9G5iAeRsuydDYbe53q5dJepCRHaPbRxhc2mQU8UAmhxPiJ/Pm4Nxs+rNpEE4S5Vy9RG\n\teRo+urVFTGl2chUf6JZ2roJq0iLegJkZKxnOoOCNcPkiMsn50FoYvswUSPOhzQg71Ql1\n\tPLFN2SbpfReDGToS+c2z0XblL7a49Ja/8eMVfXuZUb4Yt4yakzXvTqTz4qgr/T+8m4dH\n\tbSnw==","X-Gm-Message-State":"AOAM533IhLXZrpwOCD57w8zLzlquSLEXr8eSf8enqIIG2WWCdQm1nG21\n\tvlDu6gwFYSGVze+fbbsrjtYILimwXvGkQg==","X-Google-Smtp-Source":"ABdhPJy7uE8BN7YGDTWXjoiqPfVgm0I1hwJscZr+KyyuAIqTk4A/HKJCOD33ZfojfTOOvt2X1cDcbw==","X-Received":"by 2002:ac2:4d2e:: with SMTP id\n\th14mr2802954lfk.541.1600003394666; \n\tSun, 13 Sep 2020 06:23:14 -0700 (PDT)","Date":"Sun, 13 Sep 2020 15:23:13 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200913132313.GA1127199@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-4-niklas.soderlund@ragnatech.se>\n\t<20200820082004.jregxmy6qimapdtx@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820082004.jregxmy6qimapdtx@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12484,"web_url":"https://patchwork.libcamera.org/comment/12484/","msgid":"<20200913132629.GB1127199@oden.dyn.berto.se>","date":"2020-09-13T13:26:29","subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-08-20 17:55:59 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote:\n> > In preparation of supporting both the main and self path move all link\n> > setup to be part of the configuration step. The main path is still the\n> > only possible path.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++-----------\n> > 1 file changed, 42 insertions(+), 37 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index a0e36a44d8d91260..38382a6894dac22a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -211,7 +211,8 @@ private:\n> > \tfriend RkISP1CameraData;\n> > \tfriend RkISP1Frames;\n> > \n> > -\tint initLinks();\n> > +\tint initLinks(const Camera *camera,\n> > +\t\t const RkISP1CameraConfiguration &config);\n> > \tint createCamera(MediaEntity *sensor);\n> > \tvoid tryCompleteRequest(Request *request);\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> > @@ -601,28 +602,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tCameraSensor *sensor = data->sensor_;\n> > \tint ret;\n> > \n> > -\t/*\n> > -\t * Configure the sensor links: enable the link corresponding to this\n> > -\t * camera and disable all the other sensor links.\n> > -\t */\n> > -\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> > -\n> > -\tfor (MediaLink *link : pad->links()) {\n> > -\t\tbool enable = link->source()->entity() == sensor->entity();\n> > -\n> > -\t\tif (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tLOG(RkISP1, Debug)\n> > -\t\t\t<< (enable ? \"Enabling\" : \"Disabling\")\n> > -\t\t\t<< \" link from sensor '\"\n> > -\t\t\t<< link->source()->entity()->name()\n> > -\t\t\t<< \"' to ISP\";\n> > -\n> > -\t\tret = link->setEnabled(enable);\n> > -\t\tif (ret < 0)\n> > -\t\t\treturn ret;\n> > -\t}\n> > +\tret = initLinks(camera, *config);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > \n> > \t/*\n> > \t * Configure the format on the sensor output and propagate it through\n> > @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > * Match and Setup\n> > */\n> > \n> > -int PipelineHandlerRkISP1::initLinks()\n> > +int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> > +\t\t\t\t const RkISP1CameraConfiguration &config)\n> > {\n> > -\tMediaLink *link;\n> > +\tconst RkISP1CameraData *data = cameraData(camera);\n> > +\tconst CameraSensor *sensor = data->sensor_;\n> > \tint ret;\n> > \n> > \tret = media_->disableLinks();\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > -\tlink = media_->link(\"rkisp1_isp\", 2, \"rkisp1_resizer_mainpath\", 0);\n> > -\tif (!link)\n> > -\t\treturn -ENODEV;\n> > +\t/*\n> > +\t * Configure the sensor links: enable the link corresponding to this\n> > +\t * camera.\n> > +\t */\n> > +\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> > +\tfor (MediaLink *link : pad->links()) {\n> > +\t\tif (link->source()->entity() != sensor->entity())\n> > +\t\t\tcontinue;\n> > \n> > -\tret = link->setEnabled(true);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > +\t\tLOG(RkISP1, Debug)\n> > +\t\t\t<< \"Enabling link from sensor '\"\n> > +\t\t\t<< link->source()->entity()->name()\n> > +\t\t\t<< \"' to ISP\";\n> > +\n> > +\t\tret = link->setEnabled(true);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> \n> This will break switching between cameras, as you would have both links\n> enabled. You not only need to enable the link to the sensor, but disable\n> links to other sensors. The link disable step needs to happen first, as\n> the driver won't (or at least shouldn't) allow two links to be enabled\n> at the same time.\n\nIt's not a change of behavior, but it's accomplished in a different way. \nWith this change all links are configured in initLinks() so a first step \nis to disable all links with a call to 'media_->disableLinks()' all that \nis left after that is to enable the desired links.\n\n> \n> > +\n> > +\tfor (const StreamConfiguration &cfg : config) {\n> > +\t\tstd::string resizer;\n> > +\t\tif (cfg.stream() == &data->stream_)\n> > +\t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> > +\t\telse\n> > +\t\t\treturn -EINVAL;\n> \n> I'm not sure to get this change, it doesn't match the commit message. I\n> suspect it belongs to a different patch.\n\nI will update the commit message as this is intended to be a part of \nthis change. It may however be reworked to remove the intermediate \nresizer variable and save one LoC ;-)\n\n> \n> > +\n> > +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2, resizer, 0);\n> > +\t\tif (!link)\n> > +\t\t\treturn -ENODEV;\n> > +\n> > +\t\tret = link->setEnabled(true);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> > \n> > \treturn 0;\n> > }\n> > @@ -1021,12 +1032,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > \n> > -\t/* Configure default links. */\n> > -\tif (initLinks() < 0) {\n> > -\t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> > -\t\treturn false;\n> > -\t}\n> > -\n> > \t/*\n> > \t * Enumerate all sensors connected to the ISP and create one\n> > \t * camera instance for each of them.\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 C09FDC3B5B\n\tfor ;\n\tSun, 13 Sep 2020 13:26:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5393162D99;\n\tSun, 13 Sep 2020 15:26:32 +0200 (CEST)","from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3998E62901\n\tfor ;\n\tSun, 13 Sep 2020 15:26:31 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id w11so10587934lfn.2\n\tfor ;\n\tSun, 13 Sep 2020 06:26:31 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tm1sm2337853lfh.99.2020.09.13.06.26.29\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 13 Sep 2020 06:26:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Lusavcu7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=lQ5ScUcG+LM+W6T5zA5EAfgt88em5tdjRw2g3in6YV8=;\n\tb=Lusavcu7apHpzh1BC4LuSo/XLVBDmR1lmy7azrGBT7arlbxeAw30qjrQ0FyB0KXfQi\n\tDwEg8xctvpLQrG+TSFLbuBKyw95vkdgG8Cr3xdpJ3dMoPOgIcX4Ghopqj0vmZ6Ttj02j\n\tioQmMOw1WTZeOjQmhdDNIcH6adLh+ApdCMy1BXBZDv+AMErAyFtM2XxH/2HjIoTrSGhi\n\t0Rl0Becq77TdiaKpzHLQESbc8RngiQUwYdlsjSaH8BSQRyfDSlTQ9tz6Wx1688FXBy+7\n\tHQYRuH3XnvgOUK8NuiuZg8X20hTqOitaCpx8gfVLn58eDvpInCo+zBwSEJv81Wti89CU\n\tLHOg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=lQ5ScUcG+LM+W6T5zA5EAfgt88em5tdjRw2g3in6YV8=;\n\tb=eJoUPThRwiVWhcHHm9UKyi511eSQFzDaC2NAfd/QoaVXgY1LEV1XZ5eREPTWPDYLF7\n\tHwKeK50aJ6Ssgm+gQnZrzt67xMCTD5V8r7jX7FuTCQR/ZYL29cASG/8TIvLes4YslNNd\n\t2giI4wALY7z72bL86ZktqqW3VWsNh9sEUGEWDua5QjKCcH9Ir+yIHURIRUP68wdwWaJH\n\tNh9EfjBYcB7Y4FrANQnNoPm2RDCe/C9h6HaKw4wi0l3tEbivS68THJMt1pUGb9wMSQa2\n\t8FhQIQ65rYyGOAg+0M0JFYGjvZliJoTgEDAagrsHWxn8ejlF87TnXagyuRTXXUx6qgSb\n\toZUw==","X-Gm-Message-State":"AOAM532UUmlr+dYgrB97tW/5U2URh1t9IdOI8d3bVU/RNo6VYyuwDRzS\n\tLUmUx19/WOgozxHvMTsY5U8Ve7A7jYEByg==","X-Google-Smtp-Source":"ABdhPJyRQRnJVF6yaR5AjtHbqK1QyYeunp4DSrhxbboNTiD/UkFylFxopp2+eO4onHqtum+q025A6Q==","X-Received":"by 2002:ac2:592d:: with SMTP id\n\tv13mr3241688lfi.124.1600003590602; \n\tSun, 13 Sep 2020 06:26:30 -0700 (PDT)","Date":"Sun, 13 Sep 2020 15:26:29 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200913132629.GB1127199@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-4-niklas.soderlund@ragnatech.se>\n\t<20200820145559.GI6593@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820145559.GI6593@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1:\n\tSetup links as part of configuration","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]