[{"id":22069,"web_url":"https://patchwork.libcamera.org/comment/22069/","msgid":"","date":"2022-01-27T19:52:39","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Thu, 2 Dec 2021 at 10:26, Eric Curtin wrote:\n>\n> Once we have found a suitable plane and CRTC to auto-select, break out\n> of all loops, not just the inner-most loop.\n>\n> Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n> Signed-off-by: Eric Curtin \n> ---\n> src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> src/cam/kms_sink.h | 1 +\n> 2 files changed, 17 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> index d30fba78..44a0f07b 100644\n> --- a/src/cam/kms_sink.cpp\n> +++ b/src/cam/kms_sink.cpp\n> @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> return 0;\n> }\n>\n> -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> {\n> /*\n> - * If the requested format has an alpha channel, also consider the X\n> - * variant.\n> - */\n> + * If the requested format has an alpha channel, also consider the X\n> + * variant.\n> + */\n> libcamera::PixelFormat xFormat;\n>\n> switch (format) {\n> @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> }\n>\n> /*\n> - * Find a CRTC and plane suitable for the request format and the\n> - * connector at the end of the pipeline. Restrict the search to primary\n> - * planes for now.\n> - */\n> + * Find a CRTC and plane suitable for the request format and the\n> + * connector at the end of the pipeline. Restrict the search to primary\n> + * planes for now.\n> + */\n> for (const DRM::Encoder *encoder : connector_->encoders()) {\n> for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> for (const DRM::Plane *plane : crtc->planes()) {\n> @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> crtc_ = crtc;\n> plane_ = plane;\n> format_ = format;\n> - break;\n> + return crtc;\n> }\n>\n> if (plane->supportsFormat(xFormat)) {\n> crtc_ = crtc;\n> plane_ = plane;\n> format_ = xFormat;\n> - break;\n> + return crtc;\n> }\n> }\n> }\n> }\n>\n> - if (!crtc_) {\n> + return nullptr;\n> +}\n> +\n> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +{\n> + if (!findCrtc(format)) {\n> std::cerr\n> << \"Unable to find display pipeline for format \"\n> << format.toString() << std::endl;\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> index 1e4290ad..a8a29399 100644\n> --- a/src/cam/kms_sink.h\n> +++ b/src/cam/kms_sink.h\n> @@ -47,6 +47,7 @@ private:\n> libcamera::Request *camRequest_;\n> };\n>\n> + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> int configurePipeline(const libcamera::PixelFormat &format);\n> void requestComplete(DRM::AtomicRequest *request);\n>\n> --\n> 2.33.1\n>\n\nAlmost forgot about this patch, just a friendly reminder.","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 069B5BDCBF\n\tfor ;\n\tThu, 27 Jan 2022 19:53:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13094609AD;\n\tThu, 27 Jan 2022 20:53:02 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A57E6017A\n\tfor ;\n\tThu, 27 Jan 2022 20:52:59 +0100 (CET)","from mail-ot1-f72.google.com (mail-ot1-f72.google.com\n\t[209.85.210.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-610-0BWKOJpkMFaoV5NyY5yNrg-1; Thu, 27 Jan 2022 14:52:57 -0500","by mail-ot1-f72.google.com with SMTP id\n\tw18-20020a9d5a92000000b005a2408be392so2071248oth.18\n\tfor ;\n\tThu, 27 Jan 2022 11:52:56 -0800 (PST)"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"eB22UHVk\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1643313178;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=YiSs4uXvcs2SZY4n88VVCEgSkHkGVA601qRMVOKG/pg=;\n\tb=eB22UHVkfbS8Twg4v2acprGn847orc+W5dzegOB/3AcXNPRGxZcpXf89Guv4HZzJspFdFn\n\tFaigJF6gg6Zqcr+UmRUDrSIah/ew8jLnG/Fu7MYGGYsd5FlDjePja+MF0uv/cuB0TNEYzK\n\tC2GzinFpUTmo/mrEJbXW6M/xxVFxTQs=","X-MC-Unique":"0BWKOJpkMFaoV5NyY5yNrg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=YiSs4uXvcs2SZY4n88VVCEgSkHkGVA601qRMVOKG/pg=;\n\tb=3XzUXqYum5bzlPtL5kMGSXqX31Ls3ed/KCLfelhLVdPwwqHMtyzGWiz2Bi1C8m2SDE\n\tiblXA15sHhhAL2HpuWgHuwerKCgAYVbBIUa1EcbFHGa/32lMmMFPBOwN5qr9fMUq6xoN\n\tVboUb0PcpCqB01e/DY0l+XxjxY7TeG49Fjfsw6tP1U3MfgvOJqaYKuvvrLHhF1rYm//a\n\tfCD0I55eVH6nqDxFV23f0gKSGn/6xzAA94GBR9VMOw+GdT8jUaUNKoFxqYdU+WAWxgnN\n\tDVhJVi74ew27IHQ2k0RNJbDZWs4vTcRxaC+JVN2V4i3is03MK3qgYk+OoI8bAPzpWaj1\n\t4I0A==","X-Gm-Message-State":"AOAM532BDQvk8giuarOF2DflRjGatPxzCkhDX4BIanAaB8g9sZld/tik\n\tVrc9WoEhplDWbz06Gj61SHG/++gZHbP7QoU7s9umjZMfkgEFwHR3o3RCRWdx8VoWVUQVeK2RnSw\n\tpPmaOqGE2rS4CmLh7/2PIUx45itFyEQttygwtyM/re16g6KVwjA==","X-Received":["by 2002:a9d:6b8c:: with SMTP id\n\tb12mr3126559otq.289.1643313175873; \n\tThu, 27 Jan 2022 11:52:55 -0800 (PST)","by 2002:a9d:6b8c:: with SMTP id\n\tb12mr3126528otq.289.1643313175213; \n\tThu, 27 Jan 2022 11:52:55 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJx3YOKh5LF4Eve2dSLjH2tB5MgoUeXgKqlgyh+eimj0QARIEipfeJ3SLeHrsAiXwuGiwBu4kaS1I/9IHvEHJ70=","MIME-Version":"1.0","References":"<20211202102355.178664-1-ecurtin@redhat.com>","In-Reply-To":"<20211202102355.178664-1-ecurtin@redhat.com>","From":"Eric Curtin ","Date":"Thu, 27 Jan 2022 19:52:39 +0000","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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":22076,"web_url":"https://patchwork.libcamera.org/comment/22076/","msgid":"<164367311833.115113.3798223528432112778@Monstersaurus>","date":"2022-01-31T23:51:58","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Eric,\n\nQuoting Eric Curtin (2021-12-02 10:23:55)\n> Once we have found a suitable plane and CRTC to auto-select, break out\n> of all loops, not just the inner-most loop.\n> \n> Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n\nIs this an optimisation, or a bug fix ? Does this make a specific change\nin the outcome?\n\nThe fixes tag implies it does, so it might be nice to hear here what\nhappens without this patch, but it sounds like it's a worthwhile update.\n\nI haven't been able to test capture direct to DRM yet, as I haven't\nseemed to find a suitable match for camera and display, but I hope to\ntry this again soon.\n\nAha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\ncan't match configurations. Oh well, I'll try something else later.\nIt's a shame we can't handle this with strides to be able to still\ndisplay the image as it's smaller.\n\n> Signed-off-by: Eric Curtin \n> ---\n> src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> src/cam/kms_sink.h | 1 +\n> 2 files changed, 17 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> index d30fba78..44a0f07b 100644\n> --- a/src/cam/kms_sink.cpp\n> +++ b/src/cam/kms_sink.cpp\n> @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> return 0;\n> }\n> \n> -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> {\n> /*\n> - * If the requested format has an alpha channel, also consider the X\n> - * variant.\n> - */\n> + * If the requested format has an alpha channel, also consider the X\n> + * variant.\n> + */\n\nI think those changes broke the indentation... Looks like the tabs are\nreplaced by spaces?\n\n> libcamera::PixelFormat xFormat;\n> \n> switch (format) {\n> @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> }\n> \n> /*\n> - * Find a CRTC and plane suitable for the request format and the\n> - * connector at the end of the pipeline. Restrict the search to primary\n> - * planes for now.\n> - */\n> + * Find a CRTC and plane suitable for the request format and the\n> + * connector at the end of the pipeline. Restrict the search to primary\n> + * planes for now.\n> + */\n\nSame here,\n\nBut splitting this to a new function looks reasonable to me.\n\nSo with the comments repaired, which could be done while applying too:\n\n\nReviewed-by: Kieran Bingham \n\n\n> for (const DRM::Encoder *encoder : connector_->encoders()) {\n> for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> for (const DRM::Plane *plane : crtc->planes()) {\n> @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> crtc_ = crtc;\n> plane_ = plane;\n> format_ = format;\n> - break;\n> + return crtc;\n> }\n> \n> if (plane->supportsFormat(xFormat)) {\n> crtc_ = crtc;\n> plane_ = plane;\n> format_ = xFormat;\n> - break;\n> + return crtc;\n> }\n> }\n> }\n> }\n> \n> - if (!crtc_) {\n> + return nullptr;\n> +}\n> +\n> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +{\n> + if (!findCrtc(format)) {\n> std::cerr\n> << \"Unable to find display pipeline for format \"\n> << format.toString() << std::endl;\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> index 1e4290ad..a8a29399 100644\n> --- a/src/cam/kms_sink.h\n> +++ b/src/cam/kms_sink.h\n> @@ -47,6 +47,7 @@ private:\n> libcamera::Request *camRequest_;\n> };\n> \n> + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> int configurePipeline(const libcamera::PixelFormat &format);\n> void requestComplete(DRM::AtomicRequest *request);\n> \n> -- \n> 2.33.1\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 8D161BDCBF\n\tfor ;\n\tMon, 31 Jan 2022 23:52:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8830609C6;\n\tTue, 1 Feb 2022 00:52:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DDE3604F1\n\tfor ;\n\tTue, 1 Feb 2022 00:52:01 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E61899DE;\n\tTue, 1 Feb 2022 00:52:00 +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=\"Kbe4aRif\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643673121;\n\tbh=Uq25ty+22Baz36qeh1f901IyzZjBLfUbwVRvlF1kcjM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Kbe4aRifKMqilffKTPoBrf3gvnEXYUikJp3ZPH0Htu5MQzZ0d9yA3ya19rhv6kJu4\n\tiTjnhedRhlLduhqgP6UUjpMCyucYfWLyQugPD/+T0p4f67mHDUHaTpChhbIPlN0szy\n\tCdL+e0aE3b3izm+nJrgdq9GCZHLTzuX1kGs9hK4A=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211202102355.178664-1-ecurtin@redhat.com>","References":"<20211202102355.178664-1-ecurtin@redhat.com>","From":"Kieran Bingham ","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Date":"Mon, 31 Jan 2022 23:51:58 +0000","Message-ID":"<164367311833.115113.3798223528432112778@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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":22097,"web_url":"https://patchwork.libcamera.org/comment/22097/","msgid":"","date":"2022-02-01T22:16:35","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Mon, 31 Jan 2022 at 23:52, Kieran Bingham\n wrote:\n>\n> Hi Eric,\n>\n> Quoting Eric Curtin (2021-12-02 10:23:55)\n> > Once we have found a suitable plane and CRTC to auto-select, break out\n> > of all loops, not just the inner-most loop.\n> >\n> > Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n>\n> Is this an optimisation, or a bug fix ? Does this make a specific change\n> in the outcome?\n\nFor me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on\nmy hardware, unless we break out of the loop the first time around. But probably\nsomething else at play here, since as you say, this should just be an\noptimization.\n\nIt was briefly discussed on another email thread in libcamera-devel months ago.\n\n>\n> The fixes tag implies it does, so it might be nice to hear here what\n> happens without this patch, but it sounds like it's a worthwhile update.\n>\n> I haven't been able to test capture direct to DRM yet, as I haven't\n> seemed to find a suitable match for camera and display, but I hope to\n> try this again soon.\n>\n> Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\n> can't match configurations. Oh well, I'll try something else later.\n> It's a shame we can't handle this with strides to be able to still\n> display the image as it's smaller.\n\nI actually removed this limitation on the version we've been writing,\nwhich is 90%\n\"cam\" reference implementation stripped and repurposed for red hat's needs, it's\ngonna be executed by plymouth.\n\nSo the commits on December 7th here:\n\nhttps://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp\n\nallow smaller resolution camera outputs to run on larger displays, so a 1080p\nimage such as in your case should be centralized and take up 1/4 of your 4k\nscreen. It's not scaling, but it was an easy way to remove a limitation for me\nthat enabled me to test on more hardware. Could be easily added to \"cam\"\nalso.\n\nI've tested \"twincam\" on two machines, chances are, if you build and run it\non your machine on a suitable tty, it might \"just work\".\n\n>\n> > Signed-off-by: Eric Curtin \n> > ---\n> > src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> > src/cam/kms_sink.h | 1 +\n> > 2 files changed, 17 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > index d30fba78..44a0f07b 100644\n> > --- a/src/cam/kms_sink.cpp\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > return 0;\n> > }\n> >\n> > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> > {\n> > /*\n> > - * If the requested format has an alpha channel, also consider the X\n> > - * variant.\n> > - */\n> > + * If the requested format has an alpha channel, also consider the X\n> > + * variant.\n> > + */\n>\n> I think those changes broke the indentation... Looks like the tabs are\n> replaced by spaces?\n\nUnintentional, I'll fix. But I think I auto-formatted using the\nchecked in .clang-format file:\n\nhttps://github.com/kbingham/libcamera/blob/master/.clang-format\n\n>\n> > libcamera::PixelFormat xFormat;\n> >\n> > switch (format) {\n> > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > }\n> >\n> > /*\n> > - * Find a CRTC and plane suitable for the request format and the\n> > - * connector at the end of the pipeline. Restrict the search to primary\n> > - * planes for now.\n> > - */\n> > + * Find a CRTC and plane suitable for the request format and the\n> > + * connector at the end of the pipeline. Restrict the search to primary\n> > + * planes for now.\n> > + */\n>\n> Same here,\n>\n> But splitting this to a new function looks reasonable to me.\n>\n> So with the comments repaired, which could be done while applying too:\n\nI can resend patch. Thanks for reviewing.\n\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > for (const DRM::Encoder *encoder : connector_->encoders()) {\n> > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> > for (const DRM::Plane *plane : crtc->planes()) {\n> > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > crtc_ = crtc;\n> > plane_ = plane;\n> > format_ = format;\n> > - break;\n> > + return crtc;\n> > }\n> >\n> > if (plane->supportsFormat(xFormat)) {\n> > crtc_ = crtc;\n> > plane_ = plane;\n> > format_ = xFormat;\n> > - break;\n> > + return crtc;\n> > }\n> > }\n> > }\n> > }\n> >\n> > - if (!crtc_) {\n> > + return nullptr;\n> > +}\n> > +\n> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > +{\n> > + if (!findCrtc(format)) {\n> > std::cerr\n> > << \"Unable to find display pipeline for format \"\n> > << format.toString() << std::endl;\n> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > index 1e4290ad..a8a29399 100644\n> > --- a/src/cam/kms_sink.h\n> > +++ b/src/cam/kms_sink.h\n> > @@ -47,6 +47,7 @@ private:\n> > libcamera::Request *camRequest_;\n> > };\n> >\n> > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> > int configurePipeline(const libcamera::PixelFormat &format);\n> > void requestComplete(DRM::AtomicRequest *request);\n> >\n> > --\n> > 2.33.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7BF76BF415\n\tfor ;\n\tTue, 1 Feb 2022 22:17:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D36A0609E2;\n\tTue, 1 Feb 2022 23:16:59 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 167FC609B9\n\tfor ;\n\tTue, 1 Feb 2022 23:16:56 +0100 (CET)","from mail-ot1-f69.google.com (mail-ot1-f69.google.com\n\t[209.85.210.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-465-ittlqCHMMg6hOd5QcWl7-g-1; Tue, 01 Feb 2022 17:16:52 -0500","by mail-ot1-f69.google.com with SMTP id\n\tm28-20020a05683023bc00b005a485c04f3fso5510960ots.4\n\tfor ;\n\tTue, 01 Feb 2022 14:16:52 -0800 (PST)"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"LYDIwJDA\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1643753815;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=GDBUaPCmO1rdnUlm9ShQj0zWJDJE0TjRqvE/SNJuBq8=;\n\tb=LYDIwJDAyk3KoA7k+HQvsYh8H7AZDdk9q0hE90Ne6at4FHFF1nBpZu60bdZoxg1fdgdoht\n\tezwOUwYSfovKdKAOFaYLbzN1i2qUCrrIz7dNEgLd+5lFjTWT1b7rlZ/S2NaQ7uQsKNPAhN\n\t2GgbVrcyMkoxc+fnFsY6Zqp+0pJXCNE=","X-MC-Unique":"ittlqCHMMg6hOd5QcWl7-g-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=GDBUaPCmO1rdnUlm9ShQj0zWJDJE0TjRqvE/SNJuBq8=;\n\tb=UwJZAcgKu0oTnb2raze4/YhkZCkInG++YKyH1WT5bTQhqg5wcgLdkD5i29y1AO+vWe\n\t+xJPAzO4ehOHtG04TAKCgSbaZ/EOjdBdKZet9R8ySVKSUPE1G5OU/tmk0PggnQUTGjxU\n\tSxKhKnYdB5z8kquQHK6HMaW6qv2x67YJY7dxv1SvA+xg/v3Yk65CIjjFh/59NvW+VFys\n\tbU29EYTVgdJYfeNSaIXX6CDQiVnT2lg2RG6p26uEiLUGO2d9mmu+brRRKKPV9ZxrgzQz\n\tvkXU7mYp39vs8VTR+a8wQeP+0Ug/r+Xnrp2cKZfm5LS8q+X3Rw5yPd/lEw7v8AxjvsNs\n\tL1Mg==","X-Gm-Message-State":"AOAM530sneOWiGQzMc7hXmxoFtKglgT4djmaGItRdJ/u3nByUUhoiHDE\n\tSUktTUZS5XymYDYLk+Wsr2KN7q21Vd+qz5tch7kkRnvuM6yxBx8cVkEuPUchPK4DU0oTng9Jb48\n\t6/imQ03rRyTGMQFXO7QG9WysPxukZgBTHTg8zVeMv4zCnTa5f4A==","X-Received":["by 2002:a05:6808:1184:: with SMTP id\n\tj4mr2800632oil.200.1643753811851; \n\tTue, 01 Feb 2022 14:16:51 -0800 (PST)","by 2002:a05:6808:1184:: with SMTP id\n\tj4mr2800610oil.200.1643753811417; \n\tTue, 01 Feb 2022 14:16:51 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJwdo1kAueZedKaSbLVyvYCLpBR1xC/sdR8B3oETD0i2CJRvM45bgyFUweDmD1aC6Gt/qJ9RcOAxuIN2BABRsaY=","MIME-Version":"1.0","References":"<20211202102355.178664-1-ecurtin@redhat.com>\n\t<164367311833.115113.3798223528432112778@Monstersaurus>","In-Reply-To":"<164367311833.115113.3798223528432112778@Monstersaurus>","From":"Eric Curtin ","Date":"Tue, 1 Feb 2022 22:16:35 +0000","Message-ID":"","To":"Kieran Bingham ","X-Mimecast-Spam-Score":"1","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22100,"web_url":"https://patchwork.libcamera.org/comment/22100/","msgid":"<164379434967.208029.6322727414918390480@Monstersaurus>","date":"2022-02-02T09:32:29","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Eric,\n\nQuoting Eric Curtin (2022-02-01 22:16:35)\n> On Mon, 31 Jan 2022 at 23:52, Kieran Bingham\n> wrote:\n> >\n> > Hi Eric,\n> >\n> > Quoting Eric Curtin (2021-12-02 10:23:55)\n> > > Once we have found a suitable plane and CRTC to auto-select, break out\n> > > of all loops, not just the inner-most loop.\n> > >\n> > > Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n> >\n> > Is this an optimisation, or a bug fix ? Does this make a specific change\n> > in the outcome?\n> \n> For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on\n> my hardware, unless we break out of the loop the first time around. But probably\n> something else at play here, since as you say, this should just be an\n> optimization.\n\nOk, well a fixes tag is probably enough then.\n\n> It was briefly discussed on another email thread in libcamera-devel months ago.\n> \n> >\n> > The fixes tag implies it does, so it might be nice to hear here what\n> > happens without this patch, but it sounds like it's a worthwhile update.\n> >\n> > I haven't been able to test capture direct to DRM yet, as I haven't\n> > seemed to find a suitable match for camera and display, but I hope to\n> > try this again soon.\n> >\n> > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\n> > can't match configurations. Oh well, I'll try something else later.\n> > It's a shame we can't handle this with strides to be able to still\n> > display the image as it's smaller.\n> \n> I actually removed this limitation on the version we've been writing,\n> which is 90%\n> \"cam\" reference implementation stripped and repurposed for red hat's needs, it's\n> gonna be executed by plymouth.\n> \n> So the commits on December 7th here:\n> \n> https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp\n> \n> allow smaller resolution camera outputs to run on larger displays, so a 1080p\n> image such as in your case should be centralized and take up 1/4 of your 4k\n> screen. It's not scaling, but it was an easy way to remove a limitation for me\n> that enabled me to test on more hardware. Could be easily added to \"cam\"\n> also.\n\nOh - yes please, could you add that! Then I would actually be able to\ntest things :-)\n\nI think it's absolutely fine to not scale, and simply allow the image to\nbe presented if it fits. I'd even say if the image is bigger than the\ndisplay it would be nice if it still got displayed, centrally and simply\ncropped by the display output size.\n\nThat would be far more useful than requiring an exact match IMO.\n\n> I've tested \"twincam\" on two machines, chances are, if you build and run it\n> on your machine on a suitable tty, it might \"just work\".\n\nAha! Thankyou, I wasn't aware that you're actually building an\napplication here.\n\nCan I add this to my list of applications supported by libcamera?\n\nWhat are the goals of twincam? ... Just run and display cameras without\na window manager/GUI? (That sounds like exactly something I've been\nafter/wanting to make)\n\n\n\n> \n> >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> > > src/cam/kms_sink.h | 1 +\n> > > 2 files changed, 17 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > index d30fba78..44a0f07b 100644\n> > > --- a/src/cam/kms_sink.cpp\n> > > +++ b/src/cam/kms_sink.cpp\n> > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > return 0;\n> > > }\n> > >\n> > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> > > {\n> > > /*\n> > > - * If the requested format has an alpha channel, also consider the X\n> > > - * variant.\n> > > - */\n> > > + * If the requested format has an alpha channel, also consider the X\n> > > + * variant.\n> > > + */\n> >\n> > I think those changes broke the indentation... Looks like the tabs are\n> > replaced by spaces?\n> \n> Unintentional, I'll fix. But I think I auto-formatted using the\n> checked in .clang-format file:\n> \n> https://github.com/kbingham/libcamera/blob/master/.clang-format\n\nI would expect that to know we use tabs not spaces... hrmmm\n\nWe have a script that handles formatting called ./utils/checkstyle.py,\nand that can be enabled as a post-commit hook.\n\ncp ./utils/hooks/post-commit .git/hooks/\n\n> >\n> > > libcamera::PixelFormat xFormat;\n> > >\n> > > switch (format) {\n> > > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > }\n> > >\n> > > /*\n> > > - * Find a CRTC and plane suitable for the request format and the\n> > > - * connector at the end of the pipeline. Restrict the search to primary\n> > > - * planes for now.\n> > > - */\n> > > + * Find a CRTC and plane suitable for the request format and the\n> > > + * connector at the end of the pipeline. Restrict the search to primary\n> > > + * planes for now.\n> > > + */\n> >\n> > Same here,\n> >\n> > But splitting this to a new function looks reasonable to me.\n> >\n> > So with the comments repaired, which could be done while applying too:\n> \n> I can resend patch. Thanks for reviewing.\n> \n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > > for (const DRM::Encoder *encoder : connector_->encoders()) {\n> > > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> > > for (const DRM::Plane *plane : crtc->planes()) {\n> > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > crtc_ = crtc;\n> > > plane_ = plane;\n> > > format_ = format;\n> > > - break;\n> > > + return crtc;\n> > > }\n> > >\n> > > if (plane->supportsFormat(xFormat)) {\n> > > crtc_ = crtc;\n> > > plane_ = plane;\n> > > format_ = xFormat;\n> > > - break;\n> > > + return crtc;\n> > > }\n> > > }\n> > > }\n> > > }\n> > >\n> > > - if (!crtc_) {\n> > > + return nullptr;\n> > > +}\n> > > +\n> > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > +{\n> > > + if (!findCrtc(format)) {\n> > > std::cerr\n> > > << \"Unable to find display pipeline for format \"\n> > > << format.toString() << std::endl;\n> > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > index 1e4290ad..a8a29399 100644\n> > > --- a/src/cam/kms_sink.h\n> > > +++ b/src/cam/kms_sink.h\n> > > @@ -47,6 +47,7 @@ private:\n> > > libcamera::Request *camRequest_;\n> > > };\n> > >\n> > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> > > int configurePipeline(const libcamera::PixelFormat &format);\n> > > void requestComplete(DRM::AtomicRequest *request);\n> > >\n> > > --\n> > > 2.33.1\n> > >\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C09B4BDCBF\n\tfor ;\n\tWed, 2 Feb 2022 09:32:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE99E609C3;\n\tWed, 2 Feb 2022 10:32:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24C026020F\n\tfor ;\n\tWed, 2 Feb 2022 10:32:33 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A5BFE2F3;\n\tWed, 2 Feb 2022 10:32:32 +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=\"euJjfTn7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643794352;\n\tbh=/uFM+giNn3U1ArNVnZuCBpQTmD833l/qbObBJcjqFUI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=euJjfTn7DHBN13Sep/ghzlWZAWid+ZzAP8q9HssLrKV1vJT9D4z2nigxdeevXf4g/\n\tMddfg/fIjlRmtJ0J5+pUmayCyAKOkCdI6Mqr+MAS5sryIxXareVzpkuyusgQmiJNH0\n\t4UHTFKSxI9h4CsjiFi5mM+WfIOY+0secHWhem14U=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211202102355.178664-1-ecurtin@redhat.com>\n\t<164367311833.115113.3798223528432112778@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Eric Curtin ","Date":"Wed, 02 Feb 2022 09:32:29 +0000","Message-ID":"<164379434967.208029.6322727414918390480@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22102,"web_url":"https://patchwork.libcamera.org/comment/22102/","msgid":"","date":"2022-02-02T12:49:44","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Wed, 2 Feb 2022 at 09:32, Kieran Bingham\n wrote:\n>\n> Hi Eric,\n>\n> Quoting Eric Curtin (2022-02-01 22:16:35)\n> > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham\n> > wrote:\n> > >\n> > > Hi Eric,\n> > >\n> > > Quoting Eric Curtin (2021-12-02 10:23:55)\n> > > > Once we have found a suitable plane and CRTC to auto-select, break out\n> > > > of all loops, not just the inner-most loop.\n> > > >\n> > > > Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n> > >\n> > > Is this an optimisation, or a bug fix ? Does this make a specific change\n> > > in the outcome?\n> >\n> > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on\n> > my hardware, unless we break out of the loop the first time around. But probably\n> > something else at play here, since as you say, this should just be an\n> > optimization.\n>\n> Ok, well a fixes tag is probably enough then.\n>\n> > It was briefly discussed on another email thread in libcamera-devel months ago.\n> >\n> > >\n> > > The fixes tag implies it does, so it might be nice to hear here what\n> > > happens without this patch, but it sounds like it's a worthwhile update.\n> > >\n> > > I haven't been able to test capture direct to DRM yet, as I haven't\n> > > seemed to find a suitable match for camera and display, but I hope to\n> > > try this again soon.\n> > >\n> > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\n> > > can't match configurations. Oh well, I'll try something else later.\n> > > It's a shame we can't handle this with strides to be able to still\n> > > display the image as it's smaller.\n> >\n> > I actually removed this limitation on the version we've been writing,\n> > which is 90%\n> > \"cam\" reference implementation stripped and repurposed for red hat's needs, it's\n> > gonna be executed by plymouth.\n> >\n> > So the commits on December 7th here:\n> >\n> > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp\n> >\n> > allow smaller resolution camera outputs to run on larger displays, so a 1080p\n> > image such as in your case should be centralized and take up 1/4 of your 4k\n> > screen. It's not scaling, but it was an easy way to remove a limitation for me\n> > that enabled me to test on more hardware. Could be easily added to \"cam\"\n> > also.\n>\n> Oh - yes please, could you add that! Then I would actually be able to\n> test things :-)\n>\n> I think it's absolutely fine to not scale, and simply allow the image to\n> be presented if it fits. I'd even say if the image is bigger than the\n> display it would be nice if it still got displayed, centrally and simply\n> cropped by the display output size.\n\nIt will attempt to display large images on smaller displays, not sure\nthe math or code is correct for that though, haven't tried that. Would be\nuseful also like you say.\n\n>\n> That would be far more useful than requiring an exact match IMO.\n>\n> > I've tested \"twincam\" on two machines, chances are, if you build and run it\n> > on your machine on a suitable tty, it might \"just work\".\n>\n> Aha! Thankyou, I wasn't aware that you're actually building an\n> application here.\n>\n> Can I add this to my list of applications supported by libcamera?\n\nYes, of course.\n\n>\n> What are the goals of twincam? ... Just run and display cameras without\n> a window manager/GUI? (That sounds like exactly something I've been\n> after/wanting to make)\n\nIt's part of Red Hat's automotive effort, as there's a two second goal\nwe have to\nhit to start our camera from cold boot, including firmware loading,\neverything. But\nI'm open minded if people find it useful for other things and it\nproves useful for\nother use cases, great! So that's one goal the two second thing, but\nafter that we\nwill see. Some things off the top of my mind are scaling, pixel format\nconverters,\netc. Want to support many different types of camera of course, at present I only\nhave UVC camera to test with unfortunately. But would like to test with CSI/MIPI\ncamera on aarch64, as that's more commonly used in automotive I think.\n\nThere is also a separate gstreamer effort at Red Hat as you know I\nguess, we will\nlikely try to transition to that once boot is complete.\n\nBut everything is open to change of course.\n\n>\n>\n>\n> >\n> > >\n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> > > > src/cam/kms_sink.h | 1 +\n> > > > 2 files changed, 17 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > > index d30fba78..44a0f07b 100644\n> > > > --- a/src/cam/kms_sink.cpp\n> > > > +++ b/src/cam/kms_sink.cpp\n> > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > return 0;\n> > > > }\n> > > >\n> > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> > > > {\n> > > > /*\n> > > > - * If the requested format has an alpha channel, also consider the X\n> > > > - * variant.\n> > > > - */\n> > > > + * If the requested format has an alpha channel, also consider the X\n> > > > + * variant.\n> > > > + */\n> > >\n> > > I think those changes broke the indentation... Looks like the tabs are\n> > > replaced by spaces?\n> >\n> > Unintentional, I'll fix. But I think I auto-formatted using the\n> > checked in .clang-format file:\n> >\n> > https://github.com/kbingham/libcamera/blob/master/.clang-format\n>\n> I would expect that to know we use tabs not spaces... hrmmm\n>\n> We have a script that handles formatting called ./utils/checkstyle.py,\n> and that can be enabled as a post-commit hook.\n\nThanks for letting me know\n\n>\n> cp ./utils/hooks/post-commit .git/hooks/\n>\n> > >\n> > > > libcamera::PixelFormat xFormat;\n> > > >\n> > > > switch (format) {\n> > > > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > }\n> > > >\n> > > > /*\n> > > > - * Find a CRTC and plane suitable for the request format and the\n> > > > - * connector at the end of the pipeline. Restrict the search to primary\n> > > > - * planes for now.\n> > > > - */\n> > > > + * Find a CRTC and plane suitable for the request format and the\n> > > > + * connector at the end of the pipeline. Restrict the search to primary\n> > > > + * planes for now.\n> > > > + */\n> > >\n> > > Same here,\n> > >\n> > > But splitting this to a new function looks reasonable to me.\n> > >\n> > > So with the comments repaired, which could be done while applying too:\n> >\n> > I can resend patch. Thanks for reviewing.\n> >\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > >\n> > > > for (const DRM::Encoder *encoder : connector_->encoders()) {\n> > > > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> > > > for (const DRM::Plane *plane : crtc->planes()) {\n> > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > crtc_ = crtc;\n> > > > plane_ = plane;\n> > > > format_ = format;\n> > > > - break;\n> > > > + return crtc;\n> > > > }\n> > > >\n> > > > if (plane->supportsFormat(xFormat)) {\n> > > > crtc_ = crtc;\n> > > > plane_ = plane;\n> > > > format_ = xFormat;\n> > > > - break;\n> > > > + return crtc;\n> > > > }\n> > > > }\n> > > > }\n> > > > }\n> > > >\n> > > > - if (!crtc_) {\n> > > > + return nullptr;\n> > > > +}\n> > > > +\n> > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > +{\n> > > > + if (!findCrtc(format)) {\n> > > > std::cerr\n> > > > << \"Unable to find display pipeline for format \"\n> > > > << format.toString() << std::endl;\n> > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > > index 1e4290ad..a8a29399 100644\n> > > > --- a/src/cam/kms_sink.h\n> > > > +++ b/src/cam/kms_sink.h\n> > > > @@ -47,6 +47,7 @@ private:\n> > > > libcamera::Request *camRequest_;\n> > > > };\n> > > >\n> > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> > > > int configurePipeline(const libcamera::PixelFormat &format);\n> > > > void requestComplete(DRM::AtomicRequest *request);\n> > > >\n> > > > --\n> > > > 2.33.1\n> > > >\n> > >\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A8F7ABDCBF\n\tfor ;\n\tWed, 2 Feb 2022 12:50:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9D9F609C3;\n\tWed, 2 Feb 2022 13:50:08 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C034F609B5\n\tfor ;\n\tWed, 2 Feb 2022 13:50:06 +0100 (CET)","from mail-ot1-f70.google.com (mail-ot1-f70.google.com\n\t[209.85.210.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-219-HrVucY2EORSyhJnQHIs6KQ-1; Wed, 02 Feb 2022 07:50:01 -0500","by mail-ot1-f70.google.com with SMTP id\n\te110-20020a9d01f7000000b0059ecb99d288so10946596ote.12\n\tfor ;\n\tWed, 02 Feb 2022 04:50:01 -0800 (PST)"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"ILTq2DQn\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1643806205;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=LpcBHeYT8TKJGfHYcKOcCuZ2K3yRoL6HjTVD/GDSeRU=;\n\tb=ILTq2DQn1CXKHwtBvx1CWPa0ZfF/7nWlutFKnC/srQVa17X9hC4E6uOQ+KuwuP/Tds450M\n\t0HcEYMXarif0SSELZqJMwgb207j7eYwXV4sTg5uw1oh4TaF02Wm0worxKfEMqYrpxpLDlo\n\ttcmlh7ONpfyYHWMbUyrqXyZPQulciIM=","X-MC-Unique":"HrVucY2EORSyhJnQHIs6KQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=LpcBHeYT8TKJGfHYcKOcCuZ2K3yRoL6HjTVD/GDSeRU=;\n\tb=AvvB6SjkxLYDc4Jm7qVURhbhnlKdd6MBzsPSwvnJm0jii4NU/0d7mGFdBc+N/6qiBh\n\tuOLye8WbJb4DRjybiwC7If63f9OABvQao9WljfCIrADnfVZ/XRiZKqCv5DQUfy72RPQT\n\t2aRlAa9a+cGg0ntQ/Y83rbERebjnFcZf5K+9x1pT4sGS2VhKmQ9CNZlHi2r8U6LOGwvt\n\tnBje9CwNX17hqhX5d/abZsQDRFJOQ+JZLGWLHDe7CWWhvykppfo4mTAhOwcZkcooeMid\n\th+npA5685xTy7OsF7i5/wago/wuuDWyf+cq7intnBa9YJD3G6Tq+GCsDIyow0fDQap66\n\t7vOA==","X-Gm-Message-State":"AOAM530cX0pI3OOl/weaxVCUxYL1UfE6l6ZfzNQ2QLmrE2UOACJ13iih\n\t43EFIvb4CFBp+L5U9PRIoo6mXbB9gYAP4+3qo5wnUlorTnzwx6G8bHDG5tH3owE/lwbs/CeJXQq\n\tf2t+CnvcAHRuupOZR68dfZElv79/u5e+wdS+TymAv2pxLDG5IEA==","X-Received":["by 2002:a05:6808:1184:: with SMTP id\n\tj4mr4490077oil.200.1643806200342; \n\tWed, 02 Feb 2022 04:50:00 -0800 (PST)","by 2002:a05:6808:1184:: with SMTP id\n\tj4mr4490062oil.200.1643806199954; \n\tWed, 02 Feb 2022 04:49:59 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJzRVafqU/3LG4nhhQ2s3s5e3cVaX7RJKNcgGy4znmZUU5dMvW65zURp0DCn+j0grwdfTyCtVQzRq6STLq5N8IQ=","MIME-Version":"1.0","References":"<20211202102355.178664-1-ecurtin@redhat.com>\n\t<164367311833.115113.3798223528432112778@Monstersaurus>\n\t\n\t<164379434967.208029.6322727414918390480@Monstersaurus>","In-Reply-To":"<164379434967.208029.6322727414918390480@Monstersaurus>","From":"Eric Curtin ","Date":"Wed, 2 Feb 2022 12:49:44 +0000","Message-ID":"","To":"Kieran Bingham ","X-Mimecast-Spam-Score":"1","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22103,"web_url":"https://patchwork.libcamera.org/comment/22103/","msgid":"","date":"2022-02-02T14:34:55","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote:\n> On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote:\n> > Quoting Eric Curtin (2021-12-02 10:23:55)\n> > > Once we have found a suitable plane and CRTC to auto-select, break out\n> > > of all loops, not just the inner-most loop.\n> > >\n> > > Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n> >\n> > Is this an optimisation, or a bug fix ? Does this make a specific change\n> > in the outcome?\n> \n> For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on\n> my hardware, unless we break out of the loop the first time around. But probably\n> something else at play here, since as you say, this should just be an\n> optimization.\n\nWith the existing code base, we pick the last suitable CRTC, while with\nthis patch, we'll pick the first one. Breaking from the loop once a\nsuitable CRTC is found is a good idea, but if it fixes an issue with\nyour setup, it means that at least one of the CRTCs that are considered\nsuitable is actually not. We may thus break your platform inadvertently\nin the future when reworking this code. It would be good to go to the\nbottom of this, and fix the CRTC selection to not rely on luck. It can\nbe done separately from this patch.\n\n> It was briefly discussed on another email thread in libcamera-devel months ago.\n> \n> > The fixes tag implies it does, so it might be nice to hear here what\n> > happens without this patch, but it sounds like it's a worthwhile update.\n> >\n> > I haven't been able to test capture direct to DRM yet, as I haven't\n> > seemed to find a suitable match for camera and display, but I hope to\n> > try this again soon.\n> >\n> > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\n> > can't match configurations. Oh well, I'll try something else later.\n> > It's a shame we can't handle this with strides to be able to still\n> > display the image as it's smaller.\n> \n> I actually removed this limitation on the version we've been writing,\n> which is 90%\n> \"cam\" reference implementation stripped and repurposed for red hat's needs, it's\n> gonna be executed by plymouth.\n> \n> So the commits on December 7th here:\n> \n> https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp\n> \n> allow smaller resolution camera outputs to run on larger displays, so a 1080p\n> image such as in your case should be centralized and take up 1/4 of your 4k\n> screen. It's not scaling, but it was an easy way to remove a limitation for me\n> that enabled me to test on more hardware. Could be easily added to \"cam\"\n> also.\n> \n> I've tested \"twincam\" on two machines, chances are, if you build and run it\n> on your machine on a suitable tty, it might \"just work\".\n> \n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> > > src/cam/kms_sink.h | 1 +\n> > > 2 files changed, 17 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > index d30fba78..44a0f07b 100644\n> > > --- a/src/cam/kms_sink.cpp\n> > > +++ b/src/cam/kms_sink.cpp\n> > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > return 0;\n> > > }\n> > >\n> > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> > > {\n\n[snip]\n\n> > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > crtc_ = crtc;\n> > > plane_ = plane;\n> > > format_ = format;\n> > > - break;\n> > > + return crtc;\n> > > }\n> > >\n> > > if (plane->supportsFormat(xFormat)) {\n> > > crtc_ = crtc;\n> > > plane_ = plane;\n> > > format_ = xFormat;\n> > > - break;\n> > > + return crtc;\n> > > }\n> > > }\n> > > }\n> > > }\n> > >\n> > > - if (!crtc_) {\n> > > + return nullptr;\n\nThe function returns a DRM::Crtc and is called findCrtc(), but it also\nfinds a plane, selects a formatn and store them all in member variables.\nThat's a bit confusing. I would rename the function to findPipeline()\n(for lack of a better name) and make it return an integer (0 on success,\n-EPIPE on error) or a std::tuple. The former is likely easier.\n\nAnother option is\n\ndiff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\nindex d30fba783c96..aaffa372f050 100644\n--- a/src/cam/kms_sink.cpp\n+++ b/src/cam/kms_sink.cpp\n@@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n \t\t\t\t\tcrtc_ = crtc;\n \t\t\t\t\tplane_ = plane;\n \t\t\t\t\tformat_ = format;\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tgoto found;\n \t\t\t\t}\n\n \t\t\t\tif (plane->supportsFormat(xFormat)) {\n \t\t\t\t\tcrtc_ = crtc;\n \t\t\t\t\tplane_ = plane;\n \t\t\t\t\tformat_ = xFormat;\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tgoto found;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n@@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n \t\treturn -EPIPE;\n \t}\n\n+found:\n \tstd::cout\n \t\t<< \"Using KMS plane \" << plane_->id() << \", CRTC \" << crtc_->id()\n \t\t<< \", connector \" << connector_->name()\n\nwhich would be even simpler. It would cause issues if we were to add\nlocal variables after the goto statements and before the label, but we\ncan worry about that later.\n\n> > > +}\n> > > +\n> > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > +{\n> > > + if (!findCrtc(format)) {\n> > > std::cerr\n> > > << \"Unable to find display pipeline for format \"\n> > > << format.toString() << std::endl;\n> > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > index 1e4290ad..a8a29399 100644\n> > > --- a/src/cam/kms_sink.h\n> > > +++ b/src/cam/kms_sink.h\n> > > @@ -47,6 +47,7 @@ private:\n> > > libcamera::Request *camRequest_;\n> > > };\n> > >\n> > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> > > int configurePipeline(const libcamera::PixelFormat &format);\n> > > void requestComplete(DRM::AtomicRequest *request);\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 45506BF415\n\tfor ;\n\tWed, 2 Feb 2022 14:35:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F3B3609DF;\n\tWed, 2 Feb 2022 15:35:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98759609B5\n\tfor ;\n\tWed, 2 Feb 2022 15:35:18 +0100 (CET)","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 0ED212F3;\n\tWed, 2 Feb 2022 15:35:17 +0100 (CET)"],"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=\"hBHuWu6t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643812518;\n\tbh=lMj8WFis1xGmE2xZGNdgDEZ0LFZ5119cJNisuRV83X4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hBHuWu6tkogcIepqyu+0rg6/QP5j97itrCmB2PxGcI+kvt2pJ5Kl1OeRiGb1khKkD\n\tbvvB14jAGvswGRe6HRGk2G+cWgiBUvl9b1rEisz6SGDGJCkMWEidVwvXF5y+8/GweR\n\t+1zpToXUN0jNsCLavgkJ/uPAd8z49QVcKI5t6FTc=","Date":"Wed, 2 Feb 2022 16:34:55 +0200","From":"Laurent Pinchart ","To":"Eric Curtin ","Message-ID":"","References":"<20211202102355.178664-1-ecurtin@redhat.com>\n\t<164367311833.115113.3798223528432112778@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22105,"web_url":"https://patchwork.libcamera.org/comment/22105/","msgid":"","date":"2022-02-02T17:04:10","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Wed, 2 Feb 2022 at 14:35, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote:\n> > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote:\n> > > Quoting Eric Curtin (2021-12-02 10:23:55)\n> > > > Once we have found a suitable plane and CRTC to auto-select, break out\n> > > > of all loops, not just the inner-most loop.\n> > > >\n> > > > Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n> > >\n> > > Is this an optimisation, or a bug fix ? Does this make a specific change\n> > > in the outcome?\n> >\n> > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on\n> > my hardware, unless we break out of the loop the first time around. But probably\n> > something else at play here, since as you say, this should just be an\n> > optimization.\n>\n> With the existing code base, we pick the last suitable CRTC, while with\n> this patch, we'll pick the first one. Breaking from the loop once a\n> suitable CRTC is found is a good idea, but if it fixes an issue with\n> your setup, it means that at least one of the CRTCs that are considered\n> suitable is actually not. We may thus break your platform inadvertently\n> in the future when reworking this code. It would be good to go to the\n> bottom of this, and fix the CRTC selection to not rely on luck. It can\n> be done separately from this patch.\n>\n> > It was briefly discussed on another email thread in libcamera-devel months ago.\n> >\n> > > The fixes tag implies it does, so it might be nice to hear here what\n> > > happens without this patch, but it sounds like it's a worthwhile update.\n> > >\n> > > I haven't been able to test capture direct to DRM yet, as I haven't\n> > > seemed to find a suitable match for camera and display, but I hope to\n> > > try this again soon.\n> > >\n> > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\n> > > can't match configurations. Oh well, I'll try something else later.\n> > > It's a shame we can't handle this with strides to be able to still\n> > > display the image as it's smaller.\n> >\n> > I actually removed this limitation on the version we've been writing,\n> > which is 90%\n> > \"cam\" reference implementation stripped and repurposed for red hat's needs, it's\n> > gonna be executed by plymouth.\n> >\n> > So the commits on December 7th here:\n> >\n> > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp\n> >\n> > allow smaller resolution camera outputs to run on larger displays, so a 1080p\n> > image such as in your case should be centralized and take up 1/4 of your 4k\n> > screen. It's not scaling, but it was an easy way to remove a limitation for me\n> > that enabled me to test on more hardware. Could be easily added to \"cam\"\n> > also.\n> >\n> > I've tested \"twincam\" on two machines, chances are, if you build and run it\n> > on your machine on a suitable tty, it might \"just work\".\n> >\n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> > > > src/cam/kms_sink.h | 1 +\n> > > > 2 files changed, 17 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > > index d30fba78..44a0f07b 100644\n> > > > --- a/src/cam/kms_sink.cpp\n> > > > +++ b/src/cam/kms_sink.cpp\n> > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > return 0;\n> > > > }\n> > > >\n> > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> > > > {\n>\n> [snip]\n>\n> > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > crtc_ = crtc;\n> > > > plane_ = plane;\n> > > > format_ = format;\n> > > > - break;\n> > > > + return crtc;\n> > > > }\n> > > >\n> > > > if (plane->supportsFormat(xFormat)) {\n> > > > crtc_ = crtc;\n> > > > plane_ = plane;\n> > > > format_ = xFormat;\n> > > > - break;\n> > > > + return crtc;\n> > > > }\n> > > > }\n> > > > }\n> > > > }\n> > > >\n> > > > - if (!crtc_) {\n> > > > + return nullptr;\n>\n> The function returns a DRM::Crtc and is called findCrtc(), but it also\n> finds a plane, selects a formatn and store them all in member variables.\n> That's a bit confusing. I would rename the function to findPipeline()\n> (for lack of a better name) and make it return an integer (0 on success,\n> -EPIPE on error) or a std::tuple libcamera::PixelFormat>. The former is likely easier.\n\nYup, I was thinking of changing the name in the next version of the\npatch funnily enough, because you're right, it does more than select\nctrc. I'll do the int return, just to keep it simple.\n\n>\n> Another option is\n>\n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> index d30fba783c96..aaffa372f050 100644\n> --- a/src/cam/kms_sink.cpp\n> +++ b/src/cam/kms_sink.cpp\n> @@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> crtc_ = crtc;\n> plane_ = plane;\n> format_ = format;\n> - break;\n> + goto found;\n> }\n>\n> if (plane->supportsFormat(xFormat)) {\n> crtc_ = crtc;\n> plane_ = plane;\n> format_ = xFormat;\n> - break;\n> + goto found;\n> }\n> }\n> }\n> @@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> return -EPIPE;\n> }\n>\n> +found:\n> std::cout\n> << \"Using KMS plane \" << plane_->id() << \", CRTC \" << crtc_->id()\n> << \", connector \" << connector_->name()\n>\n> which would be even simpler. It would cause issues if we were to add\n> local variables after the goto statements and before the label, but we\n> can worry about that later.\n\nYeah I actually started with this, changed it just in case there were some\ngoto haters reviewing this, writing a function with returns is how to write\na goto without writing a goto. I'll write the function just to keep the\nfunctions a little smaller.\n\n>\n> > > > +}\n> > > > +\n> > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > +{\n> > > > + if (!findCrtc(format)) {\n> > > > std::cerr\n> > > > << \"Unable to find display pipeline for format \"\n> > > > << format.toString() << std::endl;\n> > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > > index 1e4290ad..a8a29399 100644\n> > > > --- a/src/cam/kms_sink.h\n> > > > +++ b/src/cam/kms_sink.h\n> > > > @@ -47,6 +47,7 @@ private:\n> > > > libcamera::Request *camRequest_;\n> > > > };\n> > > >\n> > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> > > > int configurePipeline(const libcamera::PixelFormat &format);\n> > > > void requestComplete(DRM::AtomicRequest *request);\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 5A952BDCBF\n\tfor ;\n\tWed, 2 Feb 2022 17:04:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF0F7609C3;\n\tWed, 2 Feb 2022 18:04:34 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5508D609B5\n\tfor ;\n\tWed, 2 Feb 2022 18:04:31 +0100 (CET)","from mail-oi1-f199.google.com (mail-oi1-f199.google.com\n\t[209.85.167.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-644-wWyzbbS8NfyFdc6JGc8KUA-1; Wed, 02 Feb 2022 12:04:27 -0500","by mail-oi1-f199.google.com with SMTP id\n\tq10-20020a056808200a00b002cefffffe5eso12302550oiw.11\n\tfor ;\n\tWed, 02 Feb 2022 09:04:27 -0800 (PST)"],"Authentication-Results":["lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"ClPZjJce\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1643821470;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=aiH4SiZP3KVrYhtAB54u3xyEWSmjxnpCn4oVYfTbNz8=;\n\tb=ClPZjJceQ55yZZB2SuRDkh0FKJMZlzHlRkYqIQFgmf43oKWNKMwMrG+mZzzSYc33a4OY1L\n\t6nXkUr4HuokhgpYu/bpJfd4UBFSLpnHWsrDn5tvwcV1OvwZ+leC0azroaUHqsWDw2lIP6U\n\tjRGWaAJJwa+6b8IwmWydZjD8TMZaDcE=","X-MC-Unique":"wWyzbbS8NfyFdc6JGc8KUA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=aiH4SiZP3KVrYhtAB54u3xyEWSmjxnpCn4oVYfTbNz8=;\n\tb=EQevuzcqpXPdT+NxgJELmF6JIllUvsQy1T8BijDxc6xqXRBxKBxfDoWH2tGsxVuH6R\n\tyzFgT0RwDaU3UmpHK6c3KHfnFBvlon4HqW7cwoZkBJ413sse4lmJOFy4kZ7VGlnlxbeh\n\tSDeoCXaTugsW9fm/AmIU1wyn+7sD5P91AoXMoCEJEEIQM1+BlYWXLqCvXDGdNG3Lwj9o\n\tZlbXjf6bZ/8r8hcMnlABY6OUJrrUZ1jZ1iwlOmq2aoNMrQCUL0yEQtoU8/Y3pTDLAN6R\n\tU1OfiAsbTTcsUn33fcVNg4vYTb3OjbBGf/+MwooKfzRGCD6SK4uUUQ8TvFDhsENo/3kd\n\tuPHA==","X-Gm-Message-State":"AOAM532HDWUc/KC471R5IBwASK1/Bxe44Rutr8vAOKLw+z+TCjRLIzGx\n\tTs0R722Ichx4v3OUr10TXtQFvOQMIxL64VV7x2tPPMMB0l6p/NWLG/q8IluII+q3GJhTkm6EUXt\n\t5Bm2nG0uiCXGcb2LXOT0C7paX7PLOrQajn2sY3+5ox/rdY8m2zg==","X-Received":["by 2002:a9d:6a06:: with SMTP id g6mr17034442otn.39.1643821466271;\n\tWed, 02 Feb 2022 09:04:26 -0800 (PST)","by 2002:a9d:6a06:: with SMTP id g6mr17034414otn.39.1643821465826;\n\tWed, 02 Feb 2022 09:04:25 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJzUKTrk1WC0O2GZn4KA34Gqzltcg2t4Se6wePatnfkHfUNV/iaiDY0eQDkj9RORWKzOA0uNGyI79bAbzTec/rI=","MIME-Version":"1.0","References":"<20211202102355.178664-1-ecurtin@redhat.com>\n\t<164367311833.115113.3798223528432112778@Monstersaurus>\n\t\n\t","In-Reply-To":"","From":"Eric Curtin ","Date":"Wed, 2 Feb 2022 17:04:10 +0000","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22108,"web_url":"https://patchwork.libcamera.org/comment/22108/","msgid":"","date":"2022-02-02T23:06:22","subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Wed, Feb 02, 2022 at 05:04:10PM +0000, Eric Curtin wrote:\n> On Wed, 2 Feb 2022 at 14:35, Laurent Pinchart wrote:\n> > On Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote:\n> > > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote:\n> > > > Quoting Eric Curtin (2021-12-02 10:23:55)\n> > > > > Once we have found a suitable plane and CRTC to auto-select, break out\n> > > > > of all loops, not just the inner-most loop.\n> > > > >\n> > > > > Fixes: 1de0f90dd432 (\"cam: kms_sink: Print display pipelineconfiguration\")\n> > > >\n> > > > Is this an optimisation, or a bug fix ? Does this make a specific change\n> > > > in the outcome?\n> > >\n> > > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on\n> > > my hardware, unless we break out of the loop the first time around. But probably\n> > > something else at play here, since as you say, this should just be an\n> > > optimization.\n> >\n> > With the existing code base, we pick the last suitable CRTC, while with\n> > this patch, we'll pick the first one. Breaking from the loop once a\n> > suitable CRTC is found is a good idea, but if it fixes an issue with\n> > your setup, it means that at least one of the CRTCs that are considered\n> > suitable is actually not. We may thus break your platform inadvertently\n> > in the future when reworking this code. It would be good to go to the\n> > bottom of this, and fix the CRTC selection to not rely on luck. It can\n> > be done separately from this patch.\n> >\n> > > It was briefly discussed on another email thread in libcamera-devel months ago.\n> > >\n> > > > The fixes tag implies it does, so it might be nice to hear here what\n> > > > happens without this patch, but it sounds like it's a worthwhile update.\n> > > >\n> > > > I haven't been able to test capture direct to DRM yet, as I haven't\n> > > > seemed to find a suitable match for camera and display, but I hope to\n> > > > try this again soon.\n> > > >\n> > > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it\n> > > > can't match configurations. Oh well, I'll try something else later.\n> > > > It's a shame we can't handle this with strides to be able to still\n> > > > display the image as it's smaller.\n> > >\n> > > I actually removed this limitation on the version we've been writing,\n> > > which is 90%\n> > > \"cam\" reference implementation stripped and repurposed for red hat's needs, it's\n> > > gonna be executed by plymouth.\n> > >\n> > > So the commits on December 7th here:\n> > >\n> > > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp\n> > >\n> > > allow smaller resolution camera outputs to run on larger displays, so a 1080p\n> > > image such as in your case should be centralized and take up 1/4 of your 4k\n> > > screen. It's not scaling, but it was an easy way to remove a limitation for me\n> > > that enabled me to test on more hardware. Could be easily added to \"cam\"\n> > > also.\n> > >\n> > > I've tested \"twincam\" on two machines, chances are, if you build and run it\n> > > on your machine on a suitable tty, it might \"just work\".\n> > >\n> > > > > Signed-off-by: Eric Curtin \n> > > > > ---\n> > > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------\n> > > > > src/cam/kms_sink.h | 1 +\n> > > > > 2 files changed, 17 insertions(+), 11 deletions(-)\n> > > > >\n> > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > > > index d30fba78..44a0f07b 100644\n> > > > > --- a/src/cam/kms_sink.cpp\n> > > > > +++ b/src/cam/kms_sink.cpp\n> > > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)\n> > > > > {\n> >\n> > [snip]\n> >\n> > > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > > crtc_ = crtc;\n> > > > > plane_ = plane;\n> > > > > format_ = format;\n> > > > > - break;\n> > > > > + return crtc;\n> > > > > }\n> > > > >\n> > > > > if (plane->supportsFormat(xFormat)) {\n> > > > > crtc_ = crtc;\n> > > > > plane_ = plane;\n> > > > > format_ = xFormat;\n> > > > > - break;\n> > > > > + return crtc;\n> > > > > }\n> > > > > }\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > - if (!crtc_) {\n> > > > > + return nullptr;\n> >\n> > The function returns a DRM::Crtc and is called findCrtc(), but it also\n> > finds a plane, selects a formatn and store them all in member variables.\n> > That's a bit confusing. I would rename the function to findPipeline()\n> > (for lack of a better name) and make it return an integer (0 on success,\n> > -EPIPE on error) or a std::tuple > libcamera::PixelFormat>. The former is likely easier.\n> \n> Yup, I was thinking of changing the name in the next version of the\n> patch funnily enough, because you're right, it does more than select\n> ctrc. I'll do the int return, just to keep it simple.\n\nselectPipeline() could be a better name than findPipeline() as the\nfunction selects the components, it doesn't just look them up.\n\n> > Another option is\n> >\n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > index d30fba783c96..aaffa372f050 100644\n> > --- a/src/cam/kms_sink.cpp\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > crtc_ = crtc;\n> > plane_ = plane;\n> > format_ = format;\n> > - break;\n> > + goto found;\n> > }\n> >\n> > if (plane->supportsFormat(xFormat)) {\n> > crtc_ = crtc;\n> > plane_ = plane;\n> > format_ = xFormat;\n> > - break;\n> > + goto found;\n> > }\n> > }\n> > }\n> > @@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > return -EPIPE;\n> > }\n> >\n> > +found:\n> > std::cout\n> > << \"Using KMS plane \" << plane_->id() << \", CRTC \" << crtc_->id()\n> > << \", connector \" << connector_->name()\n> >\n> > which would be even simpler. It would cause issues if we were to add\n> > local variables after the goto statements and before the label, but we\n> > can worry about that later.\n> \n> Yeah I actually started with this, changed it just in case there were some\n> goto haters reviewing this, writing a function with returns is how to write\n> a goto without writing a goto. I'll write the function just to keep the\n> functions a little smaller.\n\nOK.\n\n> > > > > +}\n> > > > > +\n> > > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > > > > +{\n> > > > > + if (!findCrtc(format)) {\n> > > > > std::cerr\n> > > > > << \"Unable to find display pipeline for format \"\n> > > > > << format.toString() << std::endl;\n> > > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > > > index 1e4290ad..a8a29399 100644\n> > > > > --- a/src/cam/kms_sink.h\n> > > > > +++ b/src/cam/kms_sink.h\n> > > > > @@ -47,6 +47,7 @@ private:\n> > > > > libcamera::Request *camRequest_;\n> > > > > };\n> > > > >\n> > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);\n> > > > > int configurePipeline(const libcamera::PixelFormat &format);\n> > > > > void requestComplete(DRM::AtomicRequest *request);\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 41013BF415\n\tfor ;\n\tWed, 2 Feb 2022 23:06:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 847F0609E1;\n\tThu, 3 Feb 2022 00:06:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20281609B5\n\tfor ;\n\tThu, 3 Feb 2022 00:06:47 +0100 (CET)","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 2C38349C;\n\tThu, 3 Feb 2022 00:06:45 +0100 (CET)"],"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=\"AzoJT/0o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643843206;\n\tbh=sN+240BsZRncangGtYg03sARJQUypxrKiNEDikRVZWg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AzoJT/0owSDA8/ShPnaDo75JZ+gkueTMQ2H7/sVGZJaF9vEZ0CwlQxrBcFlGLP7e6\n\t2lqOt87LQdx+7IzU4rGOn53CKVjBWjskhcJ+Y31FWKKw3yCi/VssjIM00GkPNzlZrw\n\tsGS3/s+g2t+IqF2tsVgjBoyaIKZPjBDR10d5tJdk=","Date":"Thu, 3 Feb 2022 01:06:22 +0200","From":"Laurent Pinchart ","To":"Eric Curtin ","Message-ID":"","References":"<20211202102355.178664-1-ecurtin@redhat.com>\n\t<164367311833.115113.3798223528432112778@Monstersaurus>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] cam: kms_sink: Once you have found a\n\tvalid crtc break out of loops","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]