[{"id":12768,"web_url":"https://patchwork.libcamera.org/comment/12768/","msgid":"<20200925141453.nptxod5zlhpawyeg@uno.localdomain>","date":"2020-09-25T14:14:53","subject":"Re: [libcamera-devel] [PATCH v3 04/22] 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 Fri, Sep 25, 2020 at 03:41:49AM +0200, Niklas Söderlund wrote:\n> In preparation of supporting both the main and self path configure all\n> the media graph links as a part of the configuration step. Before this\n> change the link between ISP and DMA engine was setup at match time as\n> the only supported path was the main path and only the link between\n> sensor and ISP was updated at part of the configuration step.\n>\n> The main path is still the only path between ISP and DMA engine that is\n> possible to enable.\n>\n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> ---\n> * Changes since v2\n> - Simplify link selection per Laurent's suggestion. This is reworked\n> once more in a later patch but makes thins one looks nicer for now.\n>\n> * Changes since v1\n> - Pass sensor directly to initLinks().\n> - Remove string variable form link selection.\n> - Rewrite commit message.\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------\n> 1 file changed, 40 insertions(+), 37 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -213,7 +213,8 @@ private:\n> \tfriend RkISP1CameraData;\n> \tfriend RkISP1Frames;\n>\n> -\tint initLinks();\n> +\tint initLinks(const Camera *camera, const CameraSensor *sensor,\n> +\t\t const RkISP1CameraConfiguration &config);\n> \tint createCamera(MediaEntity *sensor);\n> \tvoid tryCompleteRequest(Request *request);\n> \tvoid bufferReady(FrameBuffer *buffer);\n> @@ -598,28 +599,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, sensor, *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> @@ -922,22 +904,49 @@ 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 CameraSensor *sensor,\n> +\t\t\t\t const RkISP1CameraConfiguration &config)\n> {\n> -\tMediaLink *link;\n> +\tRkISP1CameraData *data = cameraData(camera);\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\nThe previous version disables links that are not used, while this one\nonly enables them. Is this intended ?\n\nThanks\n j\n\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\tfor (const StreamConfiguration &cfg : config) {\n> +\t\tif (cfg.stream() != &data->stream_)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2,\n> +\t\t\t\t\t \"rkisp1_resizer_mainpath\", 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> @@ -1019,12 +1028,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 21AACC3B5B\n\tfor ;\n\tFri, 25 Sep 2020 14:11:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95B0563021;\n\tFri, 25 Sep 2020 16:11:03 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E17662FD8\n\tfor ;\n\tFri, 25 Sep 2020 16:11:02 +0200 (CEST)","from uno.localdomain (host-87-18-63-10.retail.telecomitalia.it\n\t[87.18.63.10]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 0D8DB60006;\n\tFri, 25 Sep 2020 14:11:00 +0000 (UTC)"],"X-Originating-IP":"87.18.63.10","Date":"Fri, 25 Sep 2020 16:14:53 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200925141453.nptxod5zlhpawyeg@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-5-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 04/22] 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":12780,"web_url":"https://patchwork.libcamera.org/comment/12780/","msgid":"<20200925161607.GB1757254@oden.dyn.berto.se>","date":"2020-09-25T16:16:07","subject":"Re: [libcamera-devel] [PATCH v3 04/22] 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\nThanks for your feedback.\n\nOn 2020-09-25 16:14:53 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Fri, Sep 25, 2020 at 03:41:49AM +0200, Niklas Söderlund wrote:\n> > In preparation of supporting both the main and self path configure all\n> > the media graph links as a part of the configuration step. Before this\n> > change the link between ISP and DMA engine was setup at match time as\n> > the only supported path was the main path and only the link between\n> > sensor and ISP was updated at part of the configuration step.\n> >\n> > The main path is still the only path between ISP and DMA engine that is\n> > possible to enable.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > * Changes since v2\n> > - Simplify link selection per Laurent's suggestion. This is reworked\n> > once more in a later patch but makes thins one looks nicer for now.\n> >\n> > * Changes since v1\n> > - Pass sensor directly to initLinks().\n> > - Remove string variable form link selection.\n> > - Rewrite commit message.\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------\n> > 1 file changed, 40 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -213,7 +213,8 @@ private:\n> > \tfriend RkISP1CameraData;\n> > \tfriend RkISP1Frames;\n> >\n> > -\tint initLinks();\n> > +\tint initLinks(const Camera *camera, const CameraSensor *sensor,\n> > +\t\t const RkISP1CameraConfiguration &config);\n> > \tint createCamera(MediaEntity *sensor);\n> > \tvoid tryCompleteRequest(Request *request);\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> > @@ -598,28 +599,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, sensor, *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> > @@ -922,22 +904,49 @@ 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 CameraSensor *sensor,\n> > +\t\t\t\t const RkISP1CameraConfiguration &config)\n> > {\n> > -\tMediaLink *link;\n> > +\tRkISP1CameraData *data = cameraData(camera);\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> \n> The previous version disables links that are not used, while this one\n> only enables them. Is this intended ?\n\nYes, the call to disableLinks() above disables all links so there is no \npoint in disabling an already disabled link.\n\n> \n> Thanks\n> j\n> \n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfor (const StreamConfiguration &cfg : config) {\n> > +\t\tif (cfg.stream() != &data->stream_)\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2,\n> > +\t\t\t\t\t \"rkisp1_resizer_mainpath\", 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> > @@ -1019,12 +1028,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 3B6BEC3B5B\n\tfor ;\n\tFri, 25 Sep 2020 16:17:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFFBC6303C;\n\tFri, 25 Sep 2020 18:17:14 +0200 (CEST)","from mail-lf1-f67.google.com (mail-lf1-f67.google.com\n\t[209.85.167.67])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02F6D62FD8\n\tfor ;\n\tFri, 25 Sep 2020 18:17:09 +0200 (CEST)","by mail-lf1-f67.google.com with SMTP id x69so3444470lff.3\n\tfor ;\n\tFri, 25 Sep 2020 09:17:09 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tn20sm2647644lfh.1.2020.09.25.09.16.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 25 Sep 2020 09:16:08 -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=\"CzcfE3Zb\"; 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=RsUs3G5jF6JhWzHGQdOABVzwvGvede4me7h9pxughU4=;\n\tb=CzcfE3ZbpeiA1mGFjiQvPnBbPInWNYJ/0FNfDAFyiTwpjb0OidiCRiOS3nJMmHvLuY\n\txySgrVdoCX5vmE4n2z7GaoqJ98xiA/OA01oAVRqkL5QEQo43SI2J5K2Uzj1A22hN14Jf\n\ta8Oci8IOD8E1ojqyF7QPRwuh1VKdXYST+UmOWF0QRQCqaRmja3v6kXpuQtTMR9IRF2ty\n\tv+3pKPpBtIBuSTj5xdgC/m4kGsB7GAXw4ITIL4XQChaP3uUmN8ztBXNMNpIyJ41sILj+\n\toUcnG5+kj9qsWh13UDFkQWl11jheLqFunVXjl/73A5W5jnK8tU7nOGjwODLDYpAwv1sB\n\tqCeQ==","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=RsUs3G5jF6JhWzHGQdOABVzwvGvede4me7h9pxughU4=;\n\tb=imEFimgygTR3h2XLCEifq84i84/Gqa+O73LBUh9gf2sbAJVOO7WSzhldlESwnJBqCC\n\trPwDi7Ba4ifp3xtNEoo2VBmPNuPUVenLaVmxSdgOsOcdRLRmGLQnpH2UdwiFlKmfvkeU\n\tYxWXcfYOluX+LNADf6ILFJXFgriW2vvIvUtjnqB+R5W6ZuPmrgLNMx2C6RNKLn+gen7t\n\tn8NVY1hItQWyFrwcheg9vsXpq6RfLVh992pEaxXeMVbaGuEoJZNLHdpsvIUdnkSe1aTs\n\tw6h3/PBvgPrk8Kjd8+gT3m1TTfSoAOdXZi6TV+bes5MnIObmMTQfKZh0Ax+UR6Ebs2Kk\n\tFMgg==","X-Gm-Message-State":"AOAM530Ds4CuWwsxgdgmewoYQDelwkIZRacwWFH/D56eDLPIYcctKE1V\n\tjwSTo6Qm64EKnJLS3dxmtYN/Hw==","X-Google-Smtp-Source":"ABdhPJyJg6SXY5FVyXcXxdcmHh2dSmyEv9i3sEU+1PEUjBtKBvLRB/FXcW4EkHch8wy+kxm8nCnKJw==","X-Received":"by 2002:a19:9cf:: with SMTP id 198mr1786790lfj.214.1601050569347;\n\tFri, 25 Sep 2020 09:16:09 -0700 (PDT)","Date":"Fri, 25 Sep 2020 18:16:07 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200925161607.GB1757254@oden.dyn.berto.se>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-5-niklas.soderlund@ragnatech.se>\n\t<20200925141453.nptxod5zlhpawyeg@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925141453.nptxod5zlhpawyeg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 04/22] 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":12794,"web_url":"https://patchwork.libcamera.org/comment/12794/","msgid":"<20200928081740.xsebutrlgcolzacd@uno.localdomain>","date":"2020-09-28T08:17:40","subject":"Re: [libcamera-devel] [PATCH v3 04/22] 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 Fri, Sep 25, 2020 at 06:16:07PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2020-09-25 16:14:53 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Fri, Sep 25, 2020 at 03:41:49AM +0200, Niklas Söderlund wrote:\n> > > In preparation of supporting both the main and self path configure all\n> > > the media graph links as a part of the configuration step. Before this\n> > > change the link between ISP and DMA engine was setup at match time as\n> > > the only supported path was the main path and only the link between\n> > > sensor and ISP was updated at part of the configuration step.\n> > >\n> > > The main path is still the only path between ISP and DMA engine that is\n> > > possible to enable.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > * Changes since v2\n> > > - Simplify link selection per Laurent's suggestion. This is reworked\n> > > once more in a later patch but makes thins one looks nicer for now.\n> > >\n> > > * Changes since v1\n> > > - Pass sensor directly to initLinks().\n> > > - Remove string variable form link selection.\n> > > - Rewrite commit message.\n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------\n> > > 1 file changed, 40 insertions(+), 37 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -213,7 +213,8 @@ private:\n> > > \tfriend RkISP1CameraData;\n> > > \tfriend RkISP1Frames;\n> > >\n> > > -\tint initLinks();\n> > > +\tint initLinks(const Camera *camera, const CameraSensor *sensor,\n> > > +\t\t const RkISP1CameraConfiguration &config);\n> > > \tint createCamera(MediaEntity *sensor);\n> > > \tvoid tryCompleteRequest(Request *request);\n> > > \tvoid bufferReady(FrameBuffer *buffer);\n> > > @@ -598,28 +599,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, sensor, *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> > > @@ -922,22 +904,49 @@ 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 CameraSensor *sensor,\n> > > +\t\t\t\t const RkISP1CameraConfiguration &config)\n> > > {\n> > > -\tMediaLink *link;\n> > > +\tRkISP1CameraData *data = cameraData(camera);\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> >\n> > The previous version disables links that are not used, while this one\n> > only enables them. Is this intended ?\n>\n> Yes, the call to disableLinks() above disables all links so there is no\n> point in disabling an already disabled link.\n>\n\nAh right, sorry, missed that!\n\n> >\n> > Thanks\n> > j\n> >\n> > > +\t\tif (ret < 0)\n> > > +\t\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tfor (const StreamConfiguration &cfg : config) {\n> > > +\t\tif (cfg.stream() != &data->stream_)\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\tMediaLink *link = media_->link(\"rkisp1_isp\", 2,\n> > > +\t\t\t\t\t \"rkisp1_resizer_mainpath\", 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> > > @@ -1019,12 +1028,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\n>\n> --\n> Regards,\n> Niklas Söderlund","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 2FD22C3B5B\n\tfor ;\n\tMon, 28 Sep 2020 08:13:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A73660BF7;\n\tMon, 28 Sep 2020 10:13:47 +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 B4A6F6035F\n\tfor ;\n\tMon, 28 Sep 2020 10:13:45 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 47DF920003;\n\tMon, 28 Sep 2020 08:13:44 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 28 Sep 2020 10:17:40 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928081740.xsebutrlgcolzacd@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-5-niklas.soderlund@ragnatech.se>\n\t<20200925141453.nptxod5zlhpawyeg@uno.localdomain>\n\t<20200925161607.GB1757254@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925161607.GB1757254@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 04/22] 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\" "}}]