[{"id":22128,"web_url":"https://patchwork.libcamera.org/comment/22128/","msgid":"<164425031049.2921404.8759196469199018654@Monstersaurus>","date":"2022-02-07T16:11:50","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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-07 15:00:59)\n> There is a limitation that requires input and output to be pixel\n> for pixel identical in terms of height and width. Remove this\n> limitation to enable more hardware that doesn't match exactly in\n> terms of pixels. Centralize the image. This works for the case\n> where camera output has more pixels than the display and\n> vice-versa. In the case where there are too many pixels for the\n> display, we take the most central part of the image cropping out\n> the border.\n\nThankyou, I'm very pleased to see this patch, As I'm sure you're aware\nfrom our discussions on IRC.\n\nI haven't actually been able to test direct render with the DRM part\nhere, so I think this will be really helpful. I can't test this right\nnow though, but I hope to this week, so just some quick comments while I\nskim through.\n\n\n> Signed-off-by: Eric Curtin \n> ---\n> \n> Changes in v2:\n> - Tested and support drawing from negative pixel range\n> kernel parameter (video=960x540@60) was useful here\n> \n> src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> src/cam/kms_sink.h | 2 ++\n> 2 files changed, 16 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> index 973cd370..8eb51454 100644\n> --- a/src/cam/kms_sink.cpp\n> +++ b/src/cam/kms_sink.cpp\n> @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> const libcamera::StreamConfiguration &cfg = config.at(0);\n> \n> const std::vector &modes = connector_->modes();\n> - const auto iter = std::find_if(modes.begin(), modes.end(),\n> - [&](const DRM::Mode &mode) {\n> - return mode.hdisplay == cfg.size.width &&\n> - mode.vdisplay == cfg.size.height;\n> - });\n\nHrm, I would maybe expect to see some sort of 'best mode' search here to\nfind the closest mode that fits the incoming image. I'm sure that might\nbe a pattern that exists already somewhere.\n\nBut I expect being able to draw on any mode, is going to make this work\non more devices than only drawing on an exact mode.\n\n\n> - if (iter == modes.end()) {\n> - std::cerr\n> - << \"No mode matching \" << cfg.size.toString()\n> - << std::endl;\n> - return -EINVAL;\n> - }\n> \n> int ret = configurePipeline(cfg.pixelFormat);\n> if (ret < 0)\n> return ret;\n> \n> - mode_ = &*iter;\n> + mode_ = &modes[0];\n> size_ = cfg.size;\n> +\n> + // We need to cast for the case where the camera output has more\n> + // pixels than the display, in this case we start drawing from a\n> + // negative pixel point to crop out the content to display just\n> + // the middle part.\n\nOur code style uses\n /*\n *\n */\n\nFor comment blocks. I guess our checkstyle doesn't actually pick up on\nthat though.\n\n\n> + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n\nWe have a Rectangle class in include/libcamera/geometry.h which can\nhandle centering, and might be useful here.\n\n> stride_ = cfg.stride;\n> \n> return 0;\n> @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> \n> flags |= DRM::AtomicRequest::FlagAllowModeset;\n> }\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> index 4a0a872c..2c16182c 100644\n> --- a/src/cam/kms_sink.h\n> +++ b/src/cam/kms_sink.h\n> @@ -61,6 +61,8 @@ private:\n> libcamera::PixelFormat format_;\n> libcamera::Size size_;\n> unsigned int stride_;\n> + int x_; // Where to start drawing camera output\n> + int y_; // Where to start drawing camera output\n\n/* */ comments here too please.\n \n> std::map> buffers_;\n> \n> -- \n> 2.34.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 3EA57BDCBF\n\tfor ;\n\tMon, 7 Feb 2022 16:11:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AE9161075;\n\tMon, 7 Feb 2022 17:11:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1022B60E58\n\tfor ;\n\tMon, 7 Feb 2022 17:11:53 +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 AC45DA50;\n\tMon, 7 Feb 2022 17:11:52 +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=\"rxIQoxHL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644250312;\n\tbh=GWBq+ejU12ZZfBvFI0HiEM0or/Frbhhlk4UBsvWCRfA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rxIQoxHL568L5BOfMbfd0E0tcVRP00HH+UIqbQGT0TKqbftvS88aUNch8qE8/NpdF\n\tnOsjOnakCV41ev3Ik2oF6FmQ48ZDlRr0r/yBKAUsUgiNRebwmV+bP98nCT2BRtMCQv\n\tuoe7Lz4cRhtMlsq1wo5UIK1PeFcdgB41eB76PS6A=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220207150059.22515-1-ecurtin@redhat.com>","References":"<20220207150059.22515-1-ecurtin@redhat.com>","From":"Kieran Bingham ","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Date":"Mon, 07 Feb 2022 16:11:50 +0000","Message-ID":"<164425031049.2921404.8759196469199018654@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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":22130,"web_url":"https://patchwork.libcamera.org/comment/22130/","msgid":"","date":"2022-02-07T22:48:30","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nThank you for the patch.\n\nOn Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote:\n> Quoting Eric Curtin (2022-02-07 15:00:59)\n> > There is a limitation that requires input and output to be pixel\n> > for pixel identical in terms of height and width. Remove this\n> > limitation to enable more hardware that doesn't match exactly in\n> > terms of pixels. Centralize the image. This works for the case\n> > where camera output has more pixels than the display and\n> > vice-versa. In the case where there are too many pixels for the\n> > display, we take the most central part of the image cropping out\n> > the border.\n> \n> Thankyou, I'm very pleased to see this patch, As I'm sure you're aware\n> from our discussions on IRC.\n> \n> I haven't actually been able to test direct render with the DRM part\n> here, so I think this will be really helpful. I can't test this right\n> now though, but I hope to this week, so just some quick comments while I\n> skim through.\n> \n> > Signed-off-by: Eric Curtin \n> > ---\n> > \n> > Changes in v2:\n> > - Tested and support drawing from negative pixel range\n> > kernel parameter (video=960x540@60) was useful here\n> > \n> > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> > src/cam/kms_sink.h | 2 ++\n> > 2 files changed, 16 insertions(+), 18 deletions(-)\n> > \n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > index 973cd370..8eb51454 100644\n> > --- a/src/cam/kms_sink.cpp\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > const libcamera::StreamConfiguration &cfg = config.at(0);\n> > \n> > const std::vector &modes = connector_->modes();\n> > - const auto iter = std::find_if(modes.begin(), modes.end(),\n> > - [&](const DRM::Mode &mode) {\n> > - return mode.hdisplay == cfg.size.width &&\n> > - mode.vdisplay == cfg.size.height;\n> > - });\n> \n> Hrm, I would maybe expect to see some sort of 'best mode' search here to\n> find the closest mode that fits the incoming image. I'm sure that might\n> be a pattern that exists already somewhere.\n> \n> But I expect being able to draw on any mode, is going to make this work\n> on more devices than only drawing on an exact mode.\n> \n> > - if (iter == modes.end()) {\n> > - std::cerr\n> > - << \"No mode matching \" << cfg.size.toString()\n> > - << std::endl;\n> > - return -EINVAL;\n> > - }\n> > \n> > int ret = configurePipeline(cfg.pixelFormat);\n> > if (ret < 0)\n> > return ret;\n> > \n> > - mode_ = &*iter;\n> > + mode_ = &modes[0];\n> > size_ = cfg.size;\n> > +\n> > + // We need to cast for the case where the camera output has more\n> > + // pixels than the display, in this case we start drawing from a\n> > + // negative pixel point to crop out the content to display just\n> > + // the middle part.\n> \n> Our code style uses\n> /*\n> *\n> */\n> \n> For comment blocks. I guess our checkstyle doesn't actually pick up on\n> that though.\n> \n> > + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> > + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n\nNot all devices support negative CRTC_X and CRTC_Y values, nor do all\ndevices support planes that do not align 1:1 with the entire CRTC.\nFurthermore, some devices support scaling. I think you'll need something\na bit more elaborate here, using atomic test-only commits to try\nmultiple configurations and pick the best one.\n\n> We have a Rectangle class in include/libcamera/geometry.h which can\n> handle centering, and might be useful here.\n> \n> > stride_ = cfg.stride;\n> > \n> > return 0;\n> > @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> > + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> > + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> > + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> > + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> > + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> > \n> > flags |= DRM::AtomicRequest::FlagAllowModeset;\n> > }\n> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > index 4a0a872c..2c16182c 100644\n> > --- a/src/cam/kms_sink.h\n> > +++ b/src/cam/kms_sink.h\n> > @@ -61,6 +61,8 @@ private:\n> > libcamera::PixelFormat format_;\n> > libcamera::Size size_;\n> > unsigned int stride_;\n> > + int x_; // Where to start drawing camera output\n> > + int y_; // Where to start drawing camera output\n> \n> /* */ comments here too please.\n> \n> > std::map> buffers_;\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 08CC2BDCBF\n\tfor ;\n\tMon, 7 Feb 2022 22:48:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62A2F61071;\n\tMon, 7 Feb 2022 23:48:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35553609B9\n\tfor ;\n\tMon, 7 Feb 2022 23:48:33 +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 A0A69499;\n\tMon, 7 Feb 2022 23:48:32 +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=\"Zhk5GITR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644274112;\n\tbh=X4BO3xDDlWh16CmechONQiJyg8vVY6jIogK2P7W+zdk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zhk5GITREl9mOZ7JYknWSvcRDQKXdZXH+18+kF3lSCIfy7ewC21s8TOeTifZkLrsU\n\teQ+Sf4oewvGmSRJgrTKjbbE4n3YqhUhvmkOjnyAyW43JORqPcl7MkU7PXo6rzbMjnm\n\tA3Wfh2U++Fs/QCR1ZidbgT36ppPKZhmElgyCTHHg=","Date":"Tue, 8 Feb 2022 00:48:30 +0200","From":"Laurent Pinchart ","To":"Eric Curtin ","Message-ID":"","References":"<20220207150059.22515-1-ecurtin@redhat.com>\n\t<164425031049.2921404.8759196469199018654@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164425031049.2921404.8759196469199018654@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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":22144,"web_url":"https://patchwork.libcamera.org/comment/22144/","msgid":"","date":"2022-02-08T10:41:41","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Mon, 7 Feb 2022 at 16:11, Kieran Bingham\n wrote:\n>\n> Hi Eric,\n>\n> Quoting Eric Curtin (2022-02-07 15:00:59)\n> > There is a limitation that requires input and output to be pixel\n> > for pixel identical in terms of height and width. Remove this\n> > limitation to enable more hardware that doesn't match exactly in\n> > terms of pixels. Centralize the image. This works for the case\n> > where camera output has more pixels than the display and\n> > vice-versa. In the case where there are too many pixels for the\n> > display, we take the most central part of the image cropping out\n> > the border.\n>\n> Thankyou, I'm very pleased to see this patch, As I'm sure you're aware\n> from our discussions on IRC.\n>\n> I haven't actually been able to test direct render with the DRM part\n> here, so I think this will be really helpful. I can't test this right\n> now though, but I hope to this week, so just some quick comments while I\n> skim through.\n\nIf you decide to test this patch try and include the other patch I posted\nrecently also, my 2 test devices don't work without the other one.\n\n>\n>\n> > Signed-off-by: Eric Curtin \n> > ---\n> >\n> > Changes in v2:\n> > - Tested and support drawing from negative pixel range\n> > kernel parameter (video=960x540@60) was useful here\n> >\n> > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> > src/cam/kms_sink.h | 2 ++\n> > 2 files changed, 16 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > index 973cd370..8eb51454 100644\n> > --- a/src/cam/kms_sink.cpp\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > const libcamera::StreamConfiguration &cfg = config.at(0);\n> >\n> > const std::vector &modes = connector_->modes();\n> > - const auto iter = std::find_if(modes.begin(), modes.end(),\n> > - [&](const DRM::Mode &mode) {\n> > - return mode.hdisplay == cfg.size.width &&\n> > - mode.vdisplay == cfg.size.height;\n> > - });\n>\n> Hrm, I would maybe expect to see some sort of 'best mode' search here to\n> find the closest mode that fits the incoming image. I'm sure that might\n> be a pattern that exists already somewhere.\n\nI did think about modifying this search briefly once or twice, I tried\nto remove any sort of searching,\nlooping I encountered in any parts of the code I re-wrote for my needs\nin twincam, trying to be mindful\nof performance.\n\nMy machines typically only have one mode in that vector: 1920x1080\n\nSInce there are many ways to skin this cat, I decided to go with the\nsimplest way, just pick the first\none. But how does this sound... Restore the original find_if and if\nthat doesn't find anything to match\nexactly, just pick mode[0] and crop?\n\nLonger term (sometime after this patch), it might make more sense to\nadd a command line option to\npick a mode rather than loop through the modes.\n\n>\n> But I expect being able to draw on any mode, is going to make this work\n> on more devices than only drawing on an exact mode.\n>\n>\n> > - if (iter == modes.end()) {\n> > - std::cerr\n> > - << \"No mode matching \" << cfg.size.toString()\n> > - << std::endl;\n> > - return -EINVAL;\n> > - }\n> >\n> > int ret = configurePipeline(cfg.pixelFormat);\n> > if (ret < 0)\n> > return ret;\n> >\n> > - mode_ = &*iter;\n> > + mode_ = &modes[0];\n> > size_ = cfg.size;\n> > +\n> > + // We need to cast for the case where the camera output has more\n> > + // pixels than the display, in this case we start drawing from a\n> > + // negative pixel point to crop out the content to display just\n> > + // the middle part.\n>\n> Our code style uses\n> /*\n> *\n> */\n>\n> For comment blocks. I guess our checkstyle doesn't actually pick up on\n> that though.\n\nWill fix.\n\n>\n>\n> > + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> > + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n>\n> We have a Rectangle class in include/libcamera/geometry.h which can\n> handle centering, and might be useful here.\n\nWill use Rectangle and Point classes.\n\n>\n> > stride_ = cfg.stride;\n> >\n> > return 0;\n> > @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> > + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> > + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> > + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> > + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> > + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> >\n> > flags |= DRM::AtomicRequest::FlagAllowModeset;\n> > }\n> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > index 4a0a872c..2c16182c 100644\n> > --- a/src/cam/kms_sink.h\n> > +++ b/src/cam/kms_sink.h\n> > @@ -61,6 +61,8 @@ private:\n> > libcamera::PixelFormat format_;\n> > libcamera::Size size_;\n> > unsigned int stride_;\n> > + int x_; // Where to start drawing camera output\n> > + int y_; // Where to start drawing camera output\n>\n> /* */ comments here too please.\n\nWill do.\n\n>\n> > std::map> buffers_;\n> >\n> > --\n> > 2.34.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 9F1ACBF415\n\tfor ;\n\tTue, 8 Feb 2022 10:42:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56A5F610C3;\n\tTue, 8 Feb 2022 11:42:03 +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 89AE7610B3\n\tfor ;\n\tTue, 8 Feb 2022 11:42:01 +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-56-RN3yyItrMc-nMAEI-MoTmg-1; Tue, 08 Feb 2022 05:41:59 -0500","by mail-ot1-f70.google.com with SMTP id\n\tr16-20020a9d7510000000b0059ea94f86eeso7914676otk.8\n\tfor ;\n\tTue, 08 Feb 2022 02:41:58 -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=\"Cc77O0Wo\"; 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=1644316920;\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=CbVPPmudU1K0EhIhrwfB081yxXDdHcit4DAHPBJpYNY=;\n\tb=Cc77O0WoXDMo2C+xKV7Kdot7JbZ3hURHGfE6cGy6tNlYiqqc7DtdSCNKW3WRMMFN2PfHfe\n\tlmDsAjx4yQTrYqMFH0xCN9TAQedUi1k24LJr0uVbhbIZ2Wp1J+sTXrYphvGHy3/pUAVwh/\n\tXAmj4yifIQ/1jgDdlalNH1mM8i8UFrA=","X-MC-Unique":"RN3yyItrMc-nMAEI-MoTmg-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=CbVPPmudU1K0EhIhrwfB081yxXDdHcit4DAHPBJpYNY=;\n\tb=6K5RbOLmIedl2AVUTAU/xCoLG9gJvaAscxTfYMCYHrq2pDfa9+PVPajMM32NGEq27S\n\tnu39B201HQERC84bzCsfm+/xeRtCYUsqIycbQYYRcHwqCu6UJnk3ue7lPug7hFwo2s9W\n\tdquxeKcRwhlS44T8RC+0kUG4XZS16cduaYIpHYBpkjat8WOHYUboNHLxzBqH5vagPN/a\n\ttti8TmHGvYLc/77km6693eorMUQzL/XqKIQg00Q42bD3uhjmxUG3p68io0acJfNdCxzZ\n\t8NAhXUYpBc+hDDWT9DLKONF/qd0JvOXC/hAsOLpsilqt8WWW6Jbmm0ks9/IKfZyKkre7\n\t5hGA==","X-Gm-Message-State":"AOAM533okP8e662HaS7wp5LQxbQAWtFuULZM4sgqLvHuEdP4r6Qp5dRT\n\tD/k2DpkrXW6GcGeC17UBKrJb99UOPxO3u5fk2UD7f8pZRU1DYDXCVUsHO+RqIqO4kDDA14Vu3/6\n\tpSmmsseRjK/1Yjs4qOjVjNW177gIS7/ixZMQSr4rFpmDi9nEM3w==","X-Received":["by 2002:a05:6870:d4c5:: with SMTP id\n\tl5mr161610oai.168.1644316917966; \n\tTue, 08 Feb 2022 02:41:57 -0800 (PST)","by 2002:a05:6870:d4c5:: with SMTP id\n\tl5mr161604oai.168.1644316917627; \n\tTue, 08 Feb 2022 02:41:57 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJzaXTscvf94V5m6anSMmTmSoCiYfHikVpMTLicNxPs6NFHvDJX4pAXNcJLpHuYq7M4vUcGd+52tghJuZB3xa4k=","MIME-Version":"1.0","References":"<20220207150059.22515-1-ecurtin@redhat.com>\n\t<164425031049.2921404.8759196469199018654@Monstersaurus>","In-Reply-To":"<164425031049.2921404.8759196469199018654@Monstersaurus>","From":"Eric Curtin ","Date":"Tue, 8 Feb 2022 10:41:41 +0000","Message-ID":"","To":"Kieran Bingham ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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":22146,"web_url":"https://patchwork.libcamera.org/comment/22146/","msgid":"","date":"2022-02-08T10:49:54","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> Thank you for the patch.\n>\n> On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote:\n> > Quoting Eric Curtin (2022-02-07 15:00:59)\n> > > There is a limitation that requires input and output to be pixel\n> > > for pixel identical in terms of height and width. Remove this\n> > > limitation to enable more hardware that doesn't match exactly in\n> > > terms of pixels. Centralize the image. This works for the case\n> > > where camera output has more pixels than the display and\n> > > vice-versa. In the case where there are too many pixels for the\n> > > display, we take the most central part of the image cropping out\n> > > the border.\n> >\n> > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware\n> > from our discussions on IRC.\n> >\n> > I haven't actually been able to test direct render with the DRM part\n> > here, so I think this will be really helpful. I can't test this right\n> > now though, but I hope to this week, so just some quick comments while I\n> > skim through.\n> >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > >\n> > > Changes in v2:\n> > > - Tested and support drawing from negative pixel range\n> > > kernel parameter (video=960x540@60) was useful here\n> > >\n> > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> > > src/cam/kms_sink.h | 2 ++\n> > > 2 files changed, 16 insertions(+), 18 deletions(-)\n> > >\n> > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > index 973cd370..8eb51454 100644\n> > > --- a/src/cam/kms_sink.cpp\n> > > +++ b/src/cam/kms_sink.cpp\n> > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > const libcamera::StreamConfiguration &cfg = config.at(0);\n> > >\n> > > const std::vector &modes = connector_->modes();\n> > > - const auto iter = std::find_if(modes.begin(), modes.end(),\n> > > - [&](const DRM::Mode &mode) {\n> > > - return mode.hdisplay == cfg.size.width &&\n> > > - mode.vdisplay == cfg.size.height;\n> > > - });\n> >\n> > Hrm, I would maybe expect to see some sort of 'best mode' search here to\n> > find the closest mode that fits the incoming image. I'm sure that might\n> > be a pattern that exists already somewhere.\n> >\n> > But I expect being able to draw on any mode, is going to make this work\n> > on more devices than only drawing on an exact mode.\n> >\n> > > - if (iter == modes.end()) {\n> > > - std::cerr\n> > > - << \"No mode matching \" << cfg.size.toString()\n> > > - << std::endl;\n> > > - return -EINVAL;\n> > > - }\n> > >\n> > > int ret = configurePipeline(cfg.pixelFormat);\n> > > if (ret < 0)\n> > > return ret;\n> > >\n> > > - mode_ = &*iter;\n> > > + mode_ = &modes[0];\n> > > size_ = cfg.size;\n> > > +\n> > > + // We need to cast for the case where the camera output has more\n> > > + // pixels than the display, in this case we start drawing from a\n> > > + // negative pixel point to crop out the content to display just\n> > > + // the middle part.\n> >\n> > Our code style uses\n> > /*\n> > *\n> > */\n> >\n> > For comment blocks. I guess our checkstyle doesn't actually pick up on\n> > that though.\n> >\n> > > + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> > > + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n>\n> Not all devices support negative CRTC_X and CRTC_Y values, nor do all\n> devices support planes that do not align 1:1 with the entire CRTC.\n> Furthermore, some devices support scaling. I think you'll need something\n> a bit more elaborate here, using atomic test-only commits to try\n> multiple configurations and pick the best one.\n\nThe main goal of this patch is to enable more camera and display combinations,\nI was hoping scaling would be a future separate patch. Still only 1x1 pixel\nalignment, just starting from a non-zero x and y point. The negative\nand positive\nx and y's worked fine on my machines. I need this patch for most of my\nhardware or else I get \"No mode matching\" problem.\n\nIf we restore the find_if and use mode[0] if that fails, then we can\nonly gain support\nfor additional devices. How does this sound?\n\n>\n> > We have a Rectangle class in include/libcamera/geometry.h which can\n> > handle centering, and might be useful here.\n> >\n> > > stride_ = cfg.stride;\n> > >\n> > > return 0;\n> > > @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > > drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > > drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > > drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > > - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > > - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > > - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > > - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > > - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > > - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > > + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> > > + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> > > + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> > > + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> > > + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> > > + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> > >\n> > > flags |= DRM::AtomicRequest::FlagAllowModeset;\n> > > }\n> > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > index 4a0a872c..2c16182c 100644\n> > > --- a/src/cam/kms_sink.h\n> > > +++ b/src/cam/kms_sink.h\n> > > @@ -61,6 +61,8 @@ private:\n> > > libcamera::PixelFormat format_;\n> > > libcamera::Size size_;\n> > > unsigned int stride_;\n> > > + int x_; // Where to start drawing camera output\n> > > + int y_; // Where to start drawing camera output\n> >\n> > /* */ comments here too please.\n> >\n> > > std::map> buffers_;\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 875F9BDCBF\n\tfor ;\n\tTue, 8 Feb 2022 10:50:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4C59610B6;\n\tTue, 8 Feb 2022 11:50:15 +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 1F859610B3\n\tfor ;\n\tTue, 8 Feb 2022 11:50:14 +0100 (CET)","from mail-oo1-f71.google.com (mail-oo1-f71.google.com\n\t[209.85.161.71]) 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-529-j6gYFKxqPCO6bhdC7oJs9A-1; Tue, 08 Feb 2022 05:50:11 -0500","by mail-oo1-f71.google.com with SMTP id\n\tt12-20020a4ab58c000000b002dcbee240efso11011343ooo.10\n\tfor ;\n\tTue, 08 Feb 2022 02:50:11 -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=\"duJQQa8z\"; 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=1644317412;\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=9+S97q9n/Hb2xYpbvR/e5DX2gvSbiJ/FCNMYF/ksrDw=;\n\tb=duJQQa8zAbB4CjfThOqaTXymxFaoSwu/zbqfYiWPUzsCD/c/4PzylW8Zj59kobFr49y8SZ\n\tAlLtCzNAMopPyDFkSCbTyeBqLmwvy6uKL4otCE6WKWSIXN7b/rI4KhR0xaEHvTJG2ddCnO\n\tlec6UU4TfJslm7dpnB2wlI0lqIDSYgc=","X-MC-Unique":"j6gYFKxqPCO6bhdC7oJs9A-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=9+S97q9n/Hb2xYpbvR/e5DX2gvSbiJ/FCNMYF/ksrDw=;\n\tb=gwwBIFwOps1HORKn/TcIWzGB0rWlb3dNt0IL24Tr1TzC7bVVvfdMwJxNzH4xGEQ0vr\n\tA4LwCM1mn5010rYErXQ4XNBoR0lFH3/eWE6/xMF5rlEo5J66LEtoeF+PCyTKSZKGJene\n\t5Wwr0zoebfBj1sLxp+SElTavVOQ/OthL5cMtGZNPS5DYG8skEZah2IfrA1veT/KegxBU\n\t3njC0e8qWnfe3no0dzjr4QHLJ+VlHsu3SaDKpFbXLVq66KaybFzyuE2wbvpVivfrOD/a\n\t0MMXcr/r6ehj+bpqxHsuPSHgaOaw0jnDm/PKloJckZzmtl5E6TcqUtJet/kPGzn8ygZl\n\t1VEQ==","X-Gm-Message-State":"AOAM531pbJmFIQWWU5iiNrjw3khAzJEa8fuuEOLTNtbJKYhuR95jy9Pm\n\tbU+6L4pIa9RC8Hfq68z8zd7YxQuMk1qJWAQzaYQN8j0kRWnbxF1TyWIXDWpicnFMMkZPzAstU3t\n\ti6oewgPoQ9fwZwuupHFKdd4Vt6lt6UcKbcdqaAfwJbn9kJKGP5Q==","X-Received":["by 2002:a05:6870:a899:: with SMTP id\n\teb25mr172289oab.39.1644317410837; \n\tTue, 08 Feb 2022 02:50:10 -0800 (PST)","by 2002:a05:6870:a899:: with SMTP id\n\teb25mr172278oab.39.1644317410285; \n\tTue, 08 Feb 2022 02:50:10 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJwMVjd5bPHPwyPgcVUD2RK3MZaBSTWMErmUIxqpID05f2KRRiMFPLl+Nw+mqE8OQt5PQPxTeT6GLSywWbTELSw=","MIME-Version":"1.0","References":"<20220207150059.22515-1-ecurtin@redhat.com>\n\t<164425031049.2921404.8759196469199018654@Monstersaurus>\n\t","In-Reply-To":"","From":"Eric Curtin ","Date":"Tue, 8 Feb 2022 10:49:54 +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 v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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":22147,"web_url":"https://patchwork.libcamera.org/comment/22147/","msgid":"","date":"2022-02-08T10:56:46","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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 08, 2022 at 10:49:54AM +0000, Eric Curtin wrote:\n> On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart wrote:\n> > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote:\n> > > Quoting Eric Curtin (2022-02-07 15:00:59)\n> > > > There is a limitation that requires input and output to be pixel\n> > > > for pixel identical in terms of height and width. Remove this\n> > > > limitation to enable more hardware that doesn't match exactly in\n> > > > terms of pixels. Centralize the image. This works for the case\n> > > > where camera output has more pixels than the display and\n> > > > vice-versa. In the case where there are too many pixels for the\n> > > > display, we take the most central part of the image cropping out\n> > > > the border.\n> > >\n> > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware\n> > > from our discussions on IRC.\n> > >\n> > > I haven't actually been able to test direct render with the DRM part\n> > > here, so I think this will be really helpful. I can't test this right\n> > > now though, but I hope to this week, so just some quick comments while I\n> > > skim through.\n> > >\n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > >\n> > > > Changes in v2:\n> > > > - Tested and support drawing from negative pixel range\n> > > > kernel parameter (video=960x540@60) was useful here\n> > > >\n> > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> > > > src/cam/kms_sink.h | 2 ++\n> > > > 2 files changed, 16 insertions(+), 18 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > > index 973cd370..8eb51454 100644\n> > > > --- a/src/cam/kms_sink.cpp\n> > > > +++ b/src/cam/kms_sink.cpp\n> > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > >\n> > > > const std::vector &modes = connector_->modes();\n> > > > - const auto iter = std::find_if(modes.begin(), modes.end(),\n> > > > - [&](const DRM::Mode &mode) {\n> > > > - return mode.hdisplay == cfg.size.width &&\n> > > > - mode.vdisplay == cfg.size.height;\n> > > > - });\n> > >\n> > > Hrm, I would maybe expect to see some sort of 'best mode' search here to\n> > > find the closest mode that fits the incoming image. I'm sure that might\n> > > be a pattern that exists already somewhere.\n> > >\n> > > But I expect being able to draw on any mode, is going to make this work\n> > > on more devices than only drawing on an exact mode.\n> > >\n> > > > - if (iter == modes.end()) {\n> > > > - std::cerr\n> > > > - << \"No mode matching \" << cfg.size.toString()\n> > > > - << std::endl;\n> > > > - return -EINVAL;\n> > > > - }\n> > > >\n> > > > int ret = configurePipeline(cfg.pixelFormat);\n> > > > if (ret < 0)\n> > > > return ret;\n> > > >\n> > > > - mode_ = &*iter;\n> > > > + mode_ = &modes[0];\n> > > > size_ = cfg.size;\n> > > > +\n> > > > + // We need to cast for the case where the camera output has more\n> > > > + // pixels than the display, in this case we start drawing from a\n> > > > + // negative pixel point to crop out the content to display just\n> > > > + // the middle part.\n> > >\n> > > Our code style uses\n> > > /*\n> > > *\n> > > */\n> > >\n> > > For comment blocks. I guess our checkstyle doesn't actually pick up on\n> > > that though.\n> > >\n> > > > + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> > > > + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n> >\n> > Not all devices support negative CRTC_X and CRTC_Y values, nor do all\n> > devices support planes that do not align 1:1 with the entire CRTC.\n> > Furthermore, some devices support scaling. I think you'll need something\n> > a bit more elaborate here, using atomic test-only commits to try\n> > multiple configurations and pick the best one.\n> \n> The main goal of this patch is to enable more camera and display combinations,\n> I was hoping scaling would be a future separate patch.\n\nScaling can be done later indeed, that's not a problem.\n\n> Still only 1x1 pixel\n> alignment, just starting from a non-zero x and y point. The negative and positive\n> x and y's worked fine on my machines. I need this patch for most of my\n> hardware or else I get \"No mode matching\" problem.\n> \n> If we restore the find_if and use mode[0] if that fails, then we can only gain support\n> for additional devices. How does this sound?\n\nWe certainly don't want to break existing working use cases for devices\nthat don't support negative coordinates, or planes that don't span the\nentire CRTC. I think that searching for an optimal mode unconditionally\nfirst will help there.\n\n> > > We have a Rectangle class in include/libcamera/geometry.h which can\n> > > handle centering, and might be useful here.\n> > >\n> > > > stride_ = cfg.stride;\n> > > >\n> > > > return 0;\n> > > > @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > > > drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > > > drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > > > drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > > > - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > > > - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > > > - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > > > - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > > > - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > > > - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > > > + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> > > > + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> > > > + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> > > > + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> > > > + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> > > > + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> > > >\n> > > > flags |= DRM::AtomicRequest::FlagAllowModeset;\n> > > > }\n> > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > > index 4a0a872c..2c16182c 100644\n> > > > --- a/src/cam/kms_sink.h\n> > > > +++ b/src/cam/kms_sink.h\n> > > > @@ -61,6 +61,8 @@ private:\n> > > > libcamera::PixelFormat format_;\n> > > > libcamera::Size size_;\n> > > > unsigned int stride_;\n> > > > + int x_; // Where to start drawing camera output\n> > > > + int y_; // Where to start drawing camera output\n> > >\n> > > /* */ comments here too please.\n> > >\n> > > > std::map> buffers_;\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 79D47BF415\n\tfor ;\n\tTue, 8 Feb 2022 10:56:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD4D9610B6;\n\tTue, 8 Feb 2022 11:56:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5ED8D610AD\n\tfor ;\n\tTue, 8 Feb 2022 11:56:49 +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 D2E33480;\n\tTue, 8 Feb 2022 11:56:48 +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=\"bC2dCAFD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644317809;\n\tbh=U8d7vH40VUD+PBuC87owfbfwtFZZ68UZau8++rx/5y4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bC2dCAFDXoqWvt0Sz+8WtZBq0WMc12EojjQSb/U+L1gzXmeEDEc1rAyhJQR4rs1UN\n\tGTtzzXxqOXZpsjzN6EelRFhDzfwWDyEpMGOFjt5ANELcAfWieykaA6fYE6SK4SVef0\n\t1787IclZPprWrQWwe3jPyRyytLjjAFJ9GxcEIVcA=","Date":"Tue, 8 Feb 2022 12:56:46 +0200","From":"Laurent Pinchart ","To":"Eric Curtin ","Message-ID":"","References":"<20220207150059.22515-1-ecurtin@redhat.com>\n\t<164425031049.2921404.8759196469199018654@Monstersaurus>\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 v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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":22148,"web_url":"https://patchwork.libcamera.org/comment/22148/","msgid":"","date":"2022-02-08T11:06:01","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Tue, 8 Feb 2022 at 10:56, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Tue, Feb 08, 2022 at 10:49:54AM +0000, Eric Curtin wrote:\n> > On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart wrote:\n> > > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote:\n> > > > Quoting Eric Curtin (2022-02-07 15:00:59)\n> > > > > There is a limitation that requires input and output to be pixel\n> > > > > for pixel identical in terms of height and width. Remove this\n> > > > > limitation to enable more hardware that doesn't match exactly in\n> > > > > terms of pixels. Centralize the image. This works for the case\n> > > > > where camera output has more pixels than the display and\n> > > > > vice-versa. In the case where there are too many pixels for the\n> > > > > display, we take the most central part of the image cropping out\n> > > > > the border.\n> > > >\n> > > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware\n> > > > from our discussions on IRC.\n> > > >\n> > > > I haven't actually been able to test direct render with the DRM part\n> > > > here, so I think this will be really helpful. I can't test this right\n> > > > now though, but I hope to this week, so just some quick comments while I\n> > > > skim through.\n> > > >\n> > > > > Signed-off-by: Eric Curtin \n> > > > > ---\n> > > > >\n> > > > > Changes in v2:\n> > > > > - Tested and support drawing from negative pixel range\n> > > > > kernel parameter (video=960x540@60) was useful here\n> > > > >\n> > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> > > > > src/cam/kms_sink.h | 2 ++\n> > > > > 2 files changed, 16 insertions(+), 18 deletions(-)\n> > > > >\n> > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > > > index 973cd370..8eb51454 100644\n> > > > > --- a/src/cam/kms_sink.cpp\n> > > > > +++ b/src/cam/kms_sink.cpp\n> > > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > > const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > > >\n> > > > > const std::vector &modes = connector_->modes();\n> > > > > - const auto iter = std::find_if(modes.begin(), modes.end(),\n> > > > > - [&](const DRM::Mode &mode) {\n> > > > > - return mode.hdisplay == cfg.size.width &&\n> > > > > - mode.vdisplay == cfg.size.height;\n> > > > > - });\n> > > >\n> > > > Hrm, I would maybe expect to see some sort of 'best mode' search here to\n> > > > find the closest mode that fits the incoming image. I'm sure that might\n> > > > be a pattern that exists already somewhere.\n> > > >\n> > > > But I expect being able to draw on any mode, is going to make this work\n> > > > on more devices than only drawing on an exact mode.\n> > > >\n> > > > > - if (iter == modes.end()) {\n> > > > > - std::cerr\n> > > > > - << \"No mode matching \" << cfg.size.toString()\n> > > > > - << std::endl;\n> > > > > - return -EINVAL;\n> > > > > - }\n> > > > >\n> > > > > int ret = configurePipeline(cfg.pixelFormat);\n> > > > > if (ret < 0)\n> > > > > return ret;\n> > > > >\n> > > > > - mode_ = &*iter;\n> > > > > + mode_ = &modes[0];\n> > > > > size_ = cfg.size;\n> > > > > +\n> > > > > + // We need to cast for the case where the camera output has more\n> > > > > + // pixels than the display, in this case we start drawing from a\n> > > > > + // negative pixel point to crop out the content to display just\n> > > > > + // the middle part.\n> > > >\n> > > > Our code style uses\n> > > > /*\n> > > > *\n> > > > */\n> > > >\n> > > > For comment blocks. I guess our checkstyle doesn't actually pick up on\n> > > > that though.\n> > > >\n> > > > > + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> > > > > + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n> > >\n> > > Not all devices support negative CRTC_X and CRTC_Y values, nor do all\n> > > devices support planes that do not align 1:1 with the entire CRTC.\n> > > Furthermore, some devices support scaling. I think you'll need something\n> > > a bit more elaborate here, using atomic test-only commits to try\n> > > multiple configurations and pick the best one.\n> >\n> > The main goal of this patch is to enable more camera and display combinations,\n> > I was hoping scaling would be a future separate patch.\n>\n> Scaling can be done later indeed, that's not a problem.\n>\n> > Still only 1x1 pixel\n> > alignment, just starting from a non-zero x and y point. The negative and positive\n> > x and y's worked fine on my machines. I need this patch for most of my\n> > hardware or else I get \"No mode matching\" problem.\n> >\n> > If we restore the find_if and use mode[0] if that fails, then we can only gain support\n> > for additional devices. How does this sound?\n>\n> We certainly don't want to break existing working use cases for devices\n> that don't support negative coordinates, or planes that don't span the\n> entire CRTC. I think that searching for an optimal mode unconditionally\n> first will help there.\n\nThis solution would ensure we don't break existing working use cases\nas it would only attempt alternate x and y points in the cases that\nwould previously have printed \"No mode matching\".\n\nAnother option is to print from Point 0, 0 every time, starting\ndrawing from the corner. The problem with this is, it's ugly, you are\nvery close to the bezels of this display (in case some pixels are hard\nto view near the bezel, one of my displays is like this) and in the\ncase where there are more camera pixels than display pixels, you get\noutput from the corner of the camera rather than the centre. The\ncorner part of the image is probably not as focused as the centre,\nlower quality, etc.\n\n>\n> > > > We have a Rectangle class in include/libcamera/geometry.h which can\n> > > > handle centering, and might be useful here.\n> > > >\n> > > > > stride_ = cfg.stride;\n> > > > >\n> > > > > return 0;\n> > > > > @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > > > > drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > > > > drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > > > > drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > > > > - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > > > > - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > > > > - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > > > > - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > > > > - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > > > > - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > > > > + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> > > > > + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> > > > > + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> > > > > + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> > > > > + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> > > > > + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> > > > >\n> > > > > flags |= DRM::AtomicRequest::FlagAllowModeset;\n> > > > > }\n> > > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > > > index 4a0a872c..2c16182c 100644\n> > > > > --- a/src/cam/kms_sink.h\n> > > > > +++ b/src/cam/kms_sink.h\n> > > > > @@ -61,6 +61,8 @@ private:\n> > > > > libcamera::PixelFormat format_;\n> > > > > libcamera::Size size_;\n> > > > > unsigned int stride_;\n> > > > > + int x_; // Where to start drawing camera output\n> > > > > + int y_; // Where to start drawing camera output\n> > > >\n> > > > /* */ comments here too please.\n> > > >\n> > > > > std::map> buffers_;\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 2FEA9BDCBF\n\tfor ;\n\tTue, 8 Feb 2022 11:06:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B937610BF;\n\tTue, 8 Feb 2022 12:06:21 +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 8B9AB60E58\n\tfor ;\n\tTue, 8 Feb 2022 12:06:20 +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-659-rdvwGyj0M2y4s2DG8mmjgg-1; Tue, 08 Feb 2022 06:06:18 -0500","by mail-ot1-f70.google.com with SMTP id\n\tp17-20020a0568301d5100b005a3cd7c1e0bso7942203oth.7\n\tfor ;\n\tTue, 08 Feb 2022 03:06:18 -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=\"Qs2Wwhwp\"; 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=1644318379;\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=RKWhjb7i699DP//5dhGRvWsIE+nJGHIdoHgV/6spnIU=;\n\tb=Qs2WwhwpcsvGwZ8kZWUnHTQLY25lgzMHcU1UwnWvatZlC7r/yi8Lnmzyj/qugWTdhaWk6n\n\tZTE17zUHVXVDYIO8p6Cr7RSk0nJkRWUbrgVrpQHnwjwJGBdQNtUALhfrsIcMdaClKFiYgZ\n\t9HqsirZXmSLpRCSMs2eq04ro74TVVbo=","X-MC-Unique":"rdvwGyj0M2y4s2DG8mmjgg-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=RKWhjb7i699DP//5dhGRvWsIE+nJGHIdoHgV/6spnIU=;\n\tb=khVS/HPF+MduZlIoO3MfFSgA1C7W5HLplUA+rA/lBXb+IUoODqWu7bRP4dFVVpvVL+\n\t+SqL1VwN/sgZLEQJ3eAwGqsH11AEpzHks49MX8ntuuepiPFHDOUmayY7lHdkr4FdADUS\n\tS8E8b8gVOhk3UzJcw9pp1Djn/WcswB0812z6iK0azyH6Odp0HV/JbnaKcLZElnxk2jMB\n\tzOlvjET1iNiV2f7+25SmeVeYUfLxKiFPKtCN3oNAxyhYmdrkMBI7BxhqYJSGJDyvP15h\n\tyo48KlQD2S/pblSzuoa1hBI1vfuNCDYgtCtm+ShPEFk0wRKNEbY0XSvtGDjumIOlM6uO\n\tITXQ==","X-Gm-Message-State":"AOAM530XcMG8JR4aD3qz83qr+roUcdQwDD0O+skJzdzk8MGEAfPmmDv0\n\tAWLpS3/5Ny3Xvp0GkLzYBSPxFlndJQWBH2rS3WWu/yl6AADUKt00TcMyYysE7KDqCo39Alhk5w3\n\tVqz/wDfVgpN57yeYLz0UTb1HAHGYf5D5Dai4RHGYka+VFaFaNrQ==","X-Received":["by 2002:a05:6870:d4c5:: with SMTP id\n\tl5mr186924oai.168.1644318377424; \n\tTue, 08 Feb 2022 03:06:17 -0800 (PST)","by 2002:a05:6870:d4c5:: with SMTP id\n\tl5mr186918oai.168.1644318377145; \n\tTue, 08 Feb 2022 03:06:17 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJx2xHVM42eEiTkbS7NyhTFOi16XjXCjBQeSsMlTOp1QIVl+lA8QdPuKqzImlm+U/ASHdfmLMjDAwUYtSkhXrBA=","MIME-Version":"1.0","References":"<20220207150059.22515-1-ecurtin@redhat.com>\n\t<164425031049.2921404.8759196469199018654@Monstersaurus>\n\t\n\t\n\t","In-Reply-To":"","From":"Eric Curtin ","Date":"Tue, 8 Feb 2022 11:06:01 +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 v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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":22149,"web_url":"https://patchwork.libcamera.org/comment/22149/","msgid":"","date":"2022-02-08T11:10:29","subject":"Re: [libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Tue, 8 Feb 2022 at 11:06, Eric Curtin wrote:\n>\n> On Tue, 8 Feb 2022 at 10:56, Laurent Pinchart\n> wrote:\n> >\n> > Hi Eric,\n> >\n> > On Tue, Feb 08, 2022 at 10:49:54AM +0000, Eric Curtin wrote:\n> > > On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart wrote:\n> > > > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote:\n> > > > > Quoting Eric Curtin (2022-02-07 15:00:59)\n> > > > > > There is a limitation that requires input and output to be pixel\n> > > > > > for pixel identical in terms of height and width. Remove this\n> > > > > > limitation to enable more hardware that doesn't match exactly in\n> > > > > > terms of pixels. Centralize the image. This works for the case\n> > > > > > where camera output has more pixels than the display and\n> > > > > > vice-versa. In the case where there are too many pixels for the\n> > > > > > display, we take the most central part of the image cropping out\n> > > > > > the border.\n> > > > >\n> > > > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware\n> > > > > from our discussions on IRC.\n> > > > >\n> > > > > I haven't actually been able to test direct render with the DRM part\n> > > > > here, so I think this will be really helpful. I can't test this right\n> > > > > now though, but I hope to this week, so just some quick comments while I\n> > > > > skim through.\n> > > > >\n> > > > > > Signed-off-by: Eric Curtin \n> > > > > > ---\n> > > > > >\n> > > > > > Changes in v2:\n> > > > > > - Tested and support drawing from negative pixel range\n> > > > > > kernel parameter (video=960x540@60) was useful here\n> > > > > >\n> > > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------\n> > > > > > src/cam/kms_sink.h | 2 ++\n> > > > > > 2 files changed, 16 insertions(+), 18 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > > > > > index 973cd370..8eb51454 100644\n> > > > > > --- a/src/cam/kms_sink.cpp\n> > > > > > +++ b/src/cam/kms_sink.cpp\n> > > > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > > > const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > > > >\n> > > > > > const std::vector &modes = connector_->modes();\n> > > > > > - const auto iter = std::find_if(modes.begin(), modes.end(),\n> > > > > > - [&](const DRM::Mode &mode) {\n> > > > > > - return mode.hdisplay == cfg.size.width &&\n> > > > > > - mode.vdisplay == cfg.size.height;\n> > > > > > - });\n> > > > >\n> > > > > Hrm, I would maybe expect to see some sort of 'best mode' search here to\n> > > > > find the closest mode that fits the incoming image. I'm sure that might\n> > > > > be a pattern that exists already somewhere.\n> > > > >\n> > > > > But I expect being able to draw on any mode, is going to make this work\n> > > > > on more devices than only drawing on an exact mode.\n> > > > >\n> > > > > > - if (iter == modes.end()) {\n> > > > > > - std::cerr\n> > > > > > - << \"No mode matching \" << cfg.size.toString()\n> > > > > > - << std::endl;\n> > > > > > - return -EINVAL;\n> > > > > > - }\n> > > > > >\n> > > > > > int ret = configurePipeline(cfg.pixelFormat);\n> > > > > > if (ret < 0)\n> > > > > > return ret;\n> > > > > >\n> > > > > > - mode_ = &*iter;\n> > > > > > + mode_ = &modes[0];\n> > > > > > size_ = cfg.size;\n> > > > > > +\n> > > > > > + // We need to cast for the case where the camera output has more\n> > > > > > + // pixels than the display, in this case we start drawing from a\n> > > > > > + // negative pixel point to crop out the content to display just\n> > > > > > + // the middle part.\n> > > > >\n> > > > > Our code style uses\n> > > > > /*\n> > > > > *\n> > > > > */\n> > > > >\n> > > > > For comment blocks. I guess our checkstyle doesn't actually pick up on\n> > > > > that though.\n> > > > >\n> > > > > > + x_ = (mode_->hdisplay - static_cast(size_.width)) / 2;\n> > > > > > + y_ = (mode_->vdisplay - static_cast(size_.height)) / 2;\n> > > >\n> > > > Not all devices support negative CRTC_X and CRTC_Y values, nor do all\n> > > > devices support planes that do not align 1:1 with the entire CRTC.\n> > > > Furthermore, some devices support scaling. I think you'll need something\n> > > > a bit more elaborate here, using atomic test-only commits to try\n> > > > multiple configurations and pick the best one.\n> > >\n> > > The main goal of this patch is to enable more camera and display combinations,\n> > > I was hoping scaling would be a future separate patch.\n> >\n> > Scaling can be done later indeed, that's not a problem.\n> >\n> > > Still only 1x1 pixel\n> > > alignment, just starting from a non-zero x and y point. The negative and positive\n> > > x and y's worked fine on my machines. I need this patch for most of my\n> > > hardware or else I get \"No mode matching\" problem.\n> > >\n> > > If we restore the find_if and use mode[0] if that fails, then we can only gain support\n> > > for additional devices. How does this sound?\n> >\n> > We certainly don't want to break existing working use cases for devices\n> > that don't support negative coordinates, or planes that don't span the\n> > entire CRTC. I think that searching for an optimal mode unconditionally\n> > first will help there.\n>\n> This solution would ensure we don't break existing working use cases\n> as it would only attempt alternate x and y points in the cases that\n> would previously have printed \"No mode matching\".\n>\n> Another option is to print from Point 0, 0 every time, starting\n\n*Another option is to draw from Point 0, 0 every time, starting\n\n> drawing from the corner. The problem with this is, it's ugly, you are\n> very close to the bezels of this display (in case some pixels are hard\n> to view near the bezel, one of my displays is like this) and in the\n> case where there are more camera pixels than display pixels, you get\n> output from the corner of the camera rather than the centre. The\n> corner part of the image is probably not as focused as the centre,\n> lower quality, etc.\n>\n> >\n> > > > > We have a Rectangle class in include/libcamera/geometry.h which can\n> > > > > handle centering, and might be useful here.\n> > > > >\n> > > > > > stride_ = cfg.stride;\n> > > > > >\n> > > > > > return 0;\n> > > > > > @@ -297,12 +293,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > > > > > drmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > > > > > drmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > > > > > drmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > > > > > - drmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > > > > > - drmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > > > > > - drmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > > > > > - drmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > > > > > - drmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > > > > > - drmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > > > > > + drmRequest->addProperty(plane_, \"SRC_W\", size_.width << 16);\n> > > > > > + drmRequest->addProperty(plane_, \"SRC_H\", size_.height << 16);\n> > > > > > + drmRequest->addProperty(plane_, \"CRTC_X\", x_);\n> > > > > > + drmRequest->addProperty(plane_, \"CRTC_Y\", y_);\n> > > > > > + drmRequest->addProperty(plane_, \"CRTC_W\", size_.width);\n> > > > > > + drmRequest->addProperty(plane_, \"CRTC_H\", size_.height);\n> > > > > >\n> > > > > > flags |= DRM::AtomicRequest::FlagAllowModeset;\n> > > > > > }\n> > > > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > > > > > index 4a0a872c..2c16182c 100644\n> > > > > > --- a/src/cam/kms_sink.h\n> > > > > > +++ b/src/cam/kms_sink.h\n> > > > > > @@ -61,6 +61,8 @@ private:\n> > > > > > libcamera::PixelFormat format_;\n> > > > > > libcamera::Size size_;\n> > > > > > unsigned int stride_;\n> > > > > > + int x_; // Where to start drawing camera output\n> > > > > > + int y_; // Where to start drawing camera output\n> > > > >\n> > > > > /* */ comments here too please.\n> > > > >\n> > > > > > std::map> buffers_;\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 E1E7CBF415\n\tfor ;\n\tTue, 8 Feb 2022 11:10:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4ACD560E66;\n\tTue, 8 Feb 2022 12:10:51 +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 5082E60E58\n\tfor ;\n\tTue, 8 Feb 2022 12:10:49 +0100 (CET)","from mail-oo1-f69.google.com (mail-oo1-f69.google.com\n\t[209.85.161.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-100-C0pQSy71PJuI4sbrvIl-rw-1; Tue, 08 Feb 2022 06:10:46 -0500","by mail-oo1-f69.google.com with SMTP id\n\th13-20020a4aa74d000000b002e99030d358so11069187oom.6\n\tfor ;\n\tTue, 08 Feb 2022 03:10:46 -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=\"KH13Y0fe\"; 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=1644318648;\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=ItCqDiK2Q67IZ4sgipg2lijR20umpII5MXGxntxW97g=;\n\tb=KH13Y0fediN8DKn6uR08PUPmNvtMo5Je8w9z9DDKvsnulBu6h9xCpL+lRqZzL8T2+M1N0B\n\ty8dDVV5f5fpHKbgAfldxsrrTTZHsalLVOdS4YfduVEfLrfx1XKi+JzehYdsZwpZlI+lOQ0\n\tpGi5W0gjq93POCjOGIt5OGpxs8hD45E=","X-MC-Unique":"C0pQSy71PJuI4sbrvIl-rw-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=ItCqDiK2Q67IZ4sgipg2lijR20umpII5MXGxntxW97g=;\n\tb=3TnFJuyFlFrdmqTYsTTP3eBKSyiGOO9qcpLwFmVZmBQrobP9dUWwvq1h1YWn3M8cPP\n\tp4yif6VQ8VqgkSbajI7udGGcfXMO7MFFl0+qcuBWEcY1rhk6xJ+oO6M1mxLvBJMYsK7W\n\tWnDqZYMruk5Qy6Bh2769WzdpIr21Yryvb2aztIWEqulC2zf/XEg6rBVoD7GD4pzGrcKO\n\tKLguySR9zeRVfnvucXZrAcAgP8ZtpVTHQHKFNB4ZwsCa4/ZUDW39l8+4mSiuWKUrWLIu\n\tUUG9j+JUMqmjpa/1Psm23CuQQ1QErjY1fCVkoJrWWT6x3VhufAtejTpBmDncrjhXF0ZC\n\tiWuw==","X-Gm-Message-State":"AOAM532/J/a3iPXSNJvvy5OiF/62VIRqbJMJSilAa0j9yOLsok5L/mve\n\ttDxDQEBF4o4lfeglceXYRTCQmmx9GyjABA5Wdu/gGXJwzfFDZmso0aNd7katKlkKtWSuS7Asd3G\n\t0xicn8XovxHBjiuHWTwsR93iO7U4N9hW9FRwe0Z/e5vxRG2UjAA==","X-Received":["by 2002:a54:4486:: with SMTP id v6mr239160oiv.121.1644318645924; \n\tTue, 08 Feb 2022 03:10:45 -0800 (PST)","by 2002:a54:4486:: with SMTP id v6mr239150oiv.121.1644318645633; \n\tTue, 08 Feb 2022 03:10:45 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJwURV1njaWF1l+UJZGvDsC8/dIic9nQzHis49f57vydEY0x+m+QD+AdHEkSmQaEZ9Tv5o5wFuA9PkdUfHa8t24=","MIME-Version":"1.0","References":"<20220207150059.22515-1-ecurtin@redhat.com>\n\t<164425031049.2921404.8759196469199018654@Monstersaurus>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Eric Curtin ","Date":"Tue, 8 Feb 2022 11:10:29 +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 v2] cam: kms_sink: Remove limitation\n\tthat camera and display must match","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\" "}}]