[{"id":18535,"web_url":"https://patchwork.libcamera.org/comment/18535/","msgid":"<20210804070128.GF2167@pyrite.rasen.tech>","date":"2021-08-04T07:01:28","subject":"Re: [libcamera-devel] [PATCH v2 7/8] cam: kms_sink: Enable display\n\ton first frame","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Jul 30, 2021 at 04:03:05AM +0300, Laurent Pinchart wrote:\n> Not all display controllers support enabling the display without any\n> active plane. Delay display enabling to the first frame.\n\nI don't have enough experience or information to judge whether or not\nthe other choices (as you outlined in the cover letter) are better :/\n\nBut from what you explained, I think this is a fine solution.\n\n> \n> Signed-off-by: Laurent Pinchart \n\nI assume this will be squashed into 6/8 after you've confirmed that this\nis the direction that you want?\n\nReviewed-by: Paul Elder \n\n> ---\n> src/cam/kms_sink.cpp | 31 +++++++++++--------------------\n> src/cam/kms_sink.h | 2 --\n> 2 files changed, 11 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> index b8f86dcb6f4e..2c767cb40885 100644\n> --- a/src/cam/kms_sink.cpp\n> +++ b/src/cam/kms_sink.cpp\n> @@ -185,23 +185,6 @@ int KMSSink::start()\n> \t\treturn ret;\n> \t}\n> \n> -\t/* Enable the display pipeline with no plane to start with. */\n> -\trequest = std::make_unique(&dev_);\n> -\n> -\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> -\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> -\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> -\n> -\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> -\tif (ret < 0) {\n> -\t\tstd::cerr\n> -\t\t\t<< \"Failed to enable display pipeline: \"\n> -\t\t\t<< strerror(-ret) << std::endl;\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tplaneInitialized_ = false;\n> -\n> \treturn 0;\n> }\n> \n> @@ -245,10 +228,17 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)\n> \n> \tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> \n> +\tunsigned int flags = DRM::AtomicRequest::FlagAsync;\n> \tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> \tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> \n> -\tif (!planeInitialized_) {\n> +\tif (!active_ && !queued_) {\n> +\t\t/* Enable the display pipeline on the first frame. */\n> +\t\tdrmRequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> +\n> +\t\tdrmRequest->addProperty(crtc_, \"ACTIVE\", 1);\n> +\t\tdrmRequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> +\n> \t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> \t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> \t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> @@ -258,7 +248,8 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)\n> \t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> \t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> \t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> -\t\tplaneInitialized_ = true;\n> +\n> +\t\tflags |= DRM::AtomicRequest::FlagAllowModeset;\n> \t}\n> \n> \tpending_ = std::make_unique(drmRequest, camRequest);\n> @@ -266,7 +257,7 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)\n> \tstd::lock_guard lock(lock_);\n> \n> \tif (!queued_) {\n> -\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tint ret = drmRequest->commit(flags);\n> \t\tif (ret < 0)\n> \t\t\tstd::cerr\n> \t\t\t\t<< \"Failed to commit atomic request: \"\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> index 7b6ffcede28c..8536ef2e532d 100644\n> --- a/src/cam/kms_sink.h\n> +++ b/src/cam/kms_sink.h\n> @@ -63,8 +63,6 @@ private:\n> \tlibcamera::Size size_;\n> \tunsigned int stride_;\n> \n> -\tbool planeInitialized_;\n> -\n> \tstd::map> buffers_;\n> \n> \tstd::mutex lock_;\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 28C37C3235\n\tfor ;\n\tWed, 4 Aug 2021 07:01:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AFC668811;\n\tWed, 4 Aug 2021 09:01:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14F416026C\n\tfor ;\n\tWed, 4 Aug 2021 09:01:36 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9043C24F;\n\tWed, 4 Aug 2021 09:01:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BhMGj2Po\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628060495;\n\tbh=fXRotTqtM/J9gA3Opc18JYz4WAe65GFad1YnfUT9BIU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BhMGj2PoULO3EfkiPZone1kY85FtF2JZ/Ua3iSN4Z8L65mvt3Z2+UWlwUfD7+Ksb5\n\trQYSqOVZYvb5U5Z/HAe4ZMkNOwS0jMQ+D7ZgJ4UMEb0XYKyxnkgr3GTuATICyV2x1Q\n\tHjBuJCTjCFJSqIs7gKaD8yvx7rV2xniqgKhRGofU=","Date":"Wed, 4 Aug 2021 16:01:28 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210804070128.GF2167@pyrite.rasen.tech>","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210730010306.19956-8-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 7/8] cam: kms_sink: Enable display\n\ton first frame","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":18544,"web_url":"https://patchwork.libcamera.org/comment/18544/","msgid":"","date":"2021-08-04T08:58:19","subject":"Re: [libcamera-devel] [PATCH v2 7/8] cam: kms_sink: Enable display\n\ton first frame","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Wed, Aug 04, 2021 at 04:01:28PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Jul 30, 2021 at 04:03:05AM +0300, Laurent Pinchart wrote:\n> > Not all display controllers support enabling the display without any\n> > active plane. Delay display enabling to the first frame.\n> \n> I don't have enough experience or information to judge whether or not\n> the other choices (as you outlined in the cover letter) are better :/\n> \n> But from what you explained, I think this is a fine solution.\n\nI think so too :-)\n\n> > Signed-off-by: Laurent Pinchart \n> \n> I assume this will be squashed into 6/8 after you've confirmed that this\n> is the direction that you want?\n\nI can do that, yes, but I'm tempted to keep it separate so that the\nhistory could provide useful information about why the code is\nimplemented this way.\n\n> Reviewed-by: Paul Elder \n> \n> > ---\n> > src/cam/kms_sink.cpp | 31 +++++++++++--------------------\n> > src/cam/kms_sink.h | 2 --\n> > 2 files changed, 11 insertions(+), 22 deletions(-)\n> > \n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > index b8f86dcb6f4e..2c767cb40885 100644\n> > --- a/src/cam/kms_sink.cpp\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -185,23 +185,6 @@ int KMSSink::start()\n> > \t\treturn ret;\n> > \t}\n> > \n> > -\t/* Enable the display pipeline with no plane to start with. */\n> > -\trequest = std::make_unique(&dev_);\n> > -\n> > -\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> > -\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> > -\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> > -\n> > -\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> > -\tif (ret < 0) {\n> > -\t\tstd::cerr\n> > -\t\t\t<< \"Failed to enable display pipeline: \"\n> > -\t\t\t<< strerror(-ret) << std::endl;\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tplaneInitialized_ = false;\n> > -\n> > \treturn 0;\n> > }\n> > \n> > @@ -245,10 +228,17 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)\n> > \n> > \tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> > \n> > +\tunsigned int flags = DRM::AtomicRequest::FlagAsync;\n> > \tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> > \tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> > \n> > -\tif (!planeInitialized_) {\n> > +\tif (!active_ && !queued_) {\n> > +\t\t/* Enable the display pipeline on the first frame. */\n> > +\t\tdrmRequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> > +\n> > +\t\tdrmRequest->addProperty(crtc_, \"ACTIVE\", 1);\n> > +\t\tdrmRequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> > +\n> > \t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > \t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > \t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > @@ -258,7 +248,8 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)\n> > \t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > \t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > \t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > -\t\tplaneInitialized_ = true;\n> > +\n> > +\t\tflags |= DRM::AtomicRequest::FlagAllowModeset;\n> > \t}\n> > \n> > \tpending_ = std::make_unique(drmRequest, camRequest);\n> > @@ -266,7 +257,7 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)\n> > \tstd::lock_guard lock(lock_);\n> > \n> > \tif (!queued_) {\n> > -\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> > +\t\tint ret = drmRequest->commit(flags);\n> > \t\tif (ret < 0)\n> > \t\t\tstd::cerr\n> > \t\t\t\t<< \"Failed to commit atomic request: \"\n> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > index 7b6ffcede28c..8536ef2e532d 100644\n> > --- a/src/cam/kms_sink.h\n> > +++ b/src/cam/kms_sink.h\n> > @@ -63,8 +63,6 @@ private:\n> > \tlibcamera::Size size_;\n> > \tunsigned int stride_;\n> > \n> > -\tbool planeInitialized_;\n> > -\n> > \tstd::map> buffers_;\n> > \n> > \tstd::mutex lock_;","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 57868C3232\n\tfor ;\n\tWed, 4 Aug 2021 08:58:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B53E46881B;\n\tWed, 4 Aug 2021 10:58:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFE6F6026C\n\tfor ;\n\tWed, 4 Aug 2021 10:58:31 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3265824F;\n\tWed, 4 Aug 2021 10:58:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VIgkqmTT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628067511;\n\tbh=z3KP4zk/V/c830ecpMeMar/A/e/SJrbeREOdWjLpmXI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VIgkqmTTHgZz5AM2VbPACglcTr8php+M1I7tv1poKuPA/HwxvOORFiNaLpyn3OGvn\n\toPzQAQN7HOkTvD6fXeEaJimUmSyItSfpmBAjkW38kzfntA7lri174QuJzB4FWV885R\n\ttPuiDqtthEM++9vBhlZJgd+m/5aB3gPW5GbQCGiw=","Date":"Wed, 4 Aug 2021 11:58:19 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-8-laurent.pinchart@ideasonboard.com>\n\t<20210804070128.GF2167@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210804070128.GF2167@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2 7/8] cam: kms_sink: Enable display\n\ton first frame","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\" "}}]