Message ID | 20220620115238.16138-1-ecurtin@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Eric, Thank you for the patch. On Mon, Jun 20, 2022 at 12:52:39PM +0100, Eric Curtin wrote: > There is a limitation that requires input and output to be pixel > for pixel identical in terms of height and width. Remove this > limitation to enable more hardware that doesn't match. Just start > drawing from top left 0, 0 corner. Try and pick the mode closest to the > stream size. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Eric Curtin <ecurtin@redhat.com> > --- > Changes in v5: > - Tested Laurents suggested change successfully > - Added to commit message about trying to pick closest I've also sent a v5, on Sunday (retaining your autorship of course). Did I forget to CC you ? > Changes in v4: > - Change commit message to say top left > - Spaces to tabs > > Changes in v3: > - Much simplified version of the patch where we just attempt to > draw from point 0, 0. Only in the case where we do not find a > matching mode. Can expand to do centralization, scaling, etc. > in further patches if needs be. > > Changes in v2: > - Tested and support drawing from negative pixel range > kernel parameter (video=960x540@60) was useful here > --- > src/cam/kms_sink.cpp | 45 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 9 deletions(-) > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > index 7add81a6..7904bca8 100644 > --- a/src/cam/kms_sink.cpp > +++ b/src/cam/kms_sink.cpp > @@ -11,6 +11,7 @@ > #include <algorithm> > #include <assert.h> > #include <iostream> > +#include <limits.h> > #include <memory> > #include <stdint.h> > #include <string.h> > @@ -112,6 +113,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > const libcamera::StreamConfiguration &cfg = config.at(0); > > + /* Find the best mode for the stream size. */ > const std::vector<DRM::Mode> &modes = connector_->modes(); > const auto iter = std::find_if(modes.begin(), modes.end(), > [&](const DRM::Mode &mode) { > @@ -120,6 +122,32 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > }); > if (iter == modes.end()) { I think you could drop this search, as the iteration over the modes below should be enough. Would you mind testing the v5 I posted ? > std::cerr << "No mode matching " << cfg.size << std::endl; > + > + unsigned int cfgArea = cfg.size.width * cfg.size.height; > + unsigned int bestDistance = UINT_MAX; > + > + for (const DRM::Mode &mode : modes) { > + unsigned int modeArea = mode.hdisplay * mode.vdisplay; > + unsigned int distance = modeArea > cfgArea > + ? modeArea - cfgArea > + : cfgArea - modeArea; > + > + if (distance < bestDistance) { > + mode_ = &mode; > + bestDistance = distance; > + > + /* > + * If the sizes match exactly, there will be no better > + * match. > + */ > + if (distance == 0) > + break; > + } > + } > + } > + > + if (!mode_) { > + std::cerr << "No modes\n"; > return -EINVAL; > } > > @@ -127,7 +155,6 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > if (ret < 0) > return ret; > > - mode_ = &*iter; > size_ = cfg.size; > stride_ = cfg.stride; > > @@ -199,10 +226,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > return ret; > } > > - std::cout > - << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() > - << ", connector " << connector_->name() > - << " (" << connector_->id() << ")" << std::endl; > + std::cout << "Using KMS plane " << plane_->id() << ", CRTC " > + << crtc_->id() << ", connector " << connector_->name() << " (" > + << connector_->id() << "), mode " << mode_->hdisplay << "x" > + << mode_->vdisplay << "@" << mode_->vrefresh << std::endl; > > return 0; > } > @@ -295,12 +322,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest) > drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id()); > drmRequest->addProperty(plane_, "SRC_X", 0 << 16); > drmRequest->addProperty(plane_, "SRC_Y", 0 << 16); > - drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16); > - drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16); > + drmRequest->addProperty(plane_, "SRC_W", size_.width << 16); > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > drmRequest->addProperty(plane_, "CRTC_X", 0); > drmRequest->addProperty(plane_, "CRTC_Y", 0); > - drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay); > - drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay); > + drmRequest->addProperty(plane_, "CRTC_W", size_.width); > + drmRequest->addProperty(plane_, "CRTC_H", size_.height); > > flags |= DRM::AtomicRequest::FlagAllowModeset; > }
On Mon, 20 Jun 2022 at 13:00, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > Thank you for the patch. > > On Mon, Jun 20, 2022 at 12:52:39PM +0100, Eric Curtin wrote: > > There is a limitation that requires input and output to be pixel > > for pixel identical in terms of height and width. Remove this > > limitation to enable more hardware that doesn't match. Just start > > drawing from top left 0, 0 corner. Try and pick the mode closest to the > > stream size. > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Tested-by: Eric Curtin <ecurtin@redhat.com> > > --- > > Changes in v5: > > - Tested Laurents suggested change successfully > > - Added to commit message about trying to pick closest > > I've also sent a v5, on Sunday (retaining your autorship of course). Did > I forget to CC you ? Just tested that version exactly and it's perfect (and less code), feel free to add: Tested-by: Eric Curtin <ecurtin@redhat.com> to that one. Sorry, I copy pasted code snippets from there to test, because I couldn't seem to find it on patchwork. Maybe it's about time I switched from using just git-send-email and gmail for email. I should probably set up some client that makes it easier for me to download patches w/o patchwork. > > > Changes in v4: > > - Change commit message to say top left > > - Spaces to tabs > > > > Changes in v3: > > - Much simplified version of the patch where we just attempt to > > draw from point 0, 0. Only in the case where we do not find a > > matching mode. Can expand to do centralization, scaling, etc. > > in further patches if needs be. > > > > Changes in v2: > > - Tested and support drawing from negative pixel range > > kernel parameter (video=960x540@60) was useful here > > --- > > src/cam/kms_sink.cpp | 45 +++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 36 insertions(+), 9 deletions(-) > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > index 7add81a6..7904bca8 100644 > > --- a/src/cam/kms_sink.cpp > > +++ b/src/cam/kms_sink.cpp > > @@ -11,6 +11,7 @@ > > #include <algorithm> > > #include <assert.h> > > #include <iostream> > > +#include <limits.h> > > #include <memory> > > #include <stdint.h> > > #include <string.h> > > @@ -112,6 +113,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > + /* Find the best mode for the stream size. */ > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > const auto iter = std::find_if(modes.begin(), modes.end(), > > [&](const DRM::Mode &mode) { > > @@ -120,6 +122,32 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > }); > > if (iter == modes.end()) { > > I think you could drop this search, as the iteration over the modes > below should be enough. Would you mind testing the v5 I posted ? > > > std::cerr << "No mode matching " << cfg.size << std::endl; > > + > > + unsigned int cfgArea = cfg.size.width * cfg.size.height; > > + unsigned int bestDistance = UINT_MAX; > > + > > + for (const DRM::Mode &mode : modes) { > > + unsigned int modeArea = mode.hdisplay * mode.vdisplay; > > + unsigned int distance = modeArea > cfgArea > > + ? modeArea - cfgArea > > + : cfgArea - modeArea; > > + > > + if (distance < bestDistance) { > > + mode_ = &mode; > > + bestDistance = distance; > > + > > + /* > > + * If the sizes match exactly, there will be no better > > + * match. > > + */ > > + if (distance == 0) > > + break; > > + } > > + } > > + } > > + > > + if (!mode_) { > > + std::cerr << "No modes\n"; > > return -EINVAL; > > } > > > > @@ -127,7 +155,6 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > if (ret < 0) > > return ret; > > > > - mode_ = &*iter; > > size_ = cfg.size; > > stride_ = cfg.stride; > > > > @@ -199,10 +226,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > return ret; > > } > > > > - std::cout > > - << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() > > - << ", connector " << connector_->name() > > - << " (" << connector_->id() << ")" << std::endl; > > + std::cout << "Using KMS plane " << plane_->id() << ", CRTC " > > + << crtc_->id() << ", connector " << connector_->name() << " (" > > + << connector_->id() << "), mode " << mode_->hdisplay << "x" > > + << mode_->vdisplay << "@" << mode_->vrefresh << std::endl; > > > > return 0; > > } > > @@ -295,12 +322,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest) > > drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id()); > > drmRequest->addProperty(plane_, "SRC_X", 0 << 16); > > drmRequest->addProperty(plane_, "SRC_Y", 0 << 16); > > - drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16); > > - drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16); > > + drmRequest->addProperty(plane_, "SRC_W", size_.width << 16); > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > drmRequest->addProperty(plane_, "CRTC_X", 0); > > drmRequest->addProperty(plane_, "CRTC_Y", 0); > > - drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay); > > - drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay); > > + drmRequest->addProperty(plane_, "CRTC_W", size_.width); > > + drmRequest->addProperty(plane_, "CRTC_H", size_.height); > > > > flags |= DRM::AtomicRequest::FlagAllowModeset; > > } > > -- > Regards, > > Laurent Pinchart >
Hi Eric, On Mon, Jun 20, 2022 at 01:33:31PM +0100, Eric Curtin wrote: > On Mon, 20 Jun 2022 at 13:00, Laurent Pinchart wrote: > > On Mon, Jun 20, 2022 at 12:52:39PM +0100, Eric Curtin wrote: > > > There is a limitation that requires input and output to be pixel > > > for pixel identical in terms of height and width. Remove this > > > limitation to enable more hardware that doesn't match. Just start > > > drawing from top left 0, 0 corner. Try and pick the mode closest to the > > > stream size. > > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Tested-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > Changes in v5: > > > - Tested Laurents suggested change successfully > > > - Added to commit message about trying to pick closest > > > > I've also sent a v5, on Sunday (retaining your autorship of course). Did > > I forget to CC you ? > > Just tested that version exactly and it's perfect (and less code), > feel free to add: > > Tested-by: Eric Curtin <ecurtin@redhat.com> > > to that one. Thank you. I'll push the patch shortly. > Sorry, I copy pasted code snippets from there to test, because I > couldn't seem to find it on patchwork. Maybe it's about time I > switched from using just git-send-email and gmail for email. I should > probably set up some client that makes it easier for me to download > patches w/o patchwork. And we also need to setup a public-inbox instance for libcamera, so that you could use b4 to download patches from the list. > > > Changes in v4: > > > - Change commit message to say top left > > > - Spaces to tabs > > > > > > Changes in v3: > > > - Much simplified version of the patch where we just attempt to > > > draw from point 0, 0. Only in the case where we do not find a > > > matching mode. Can expand to do centralization, scaling, etc. > > > in further patches if needs be. > > > > > > Changes in v2: > > > - Tested and support drawing from negative pixel range > > > kernel parameter (video=960x540@60) was useful here > > > --- > > > src/cam/kms_sink.cpp | 45 +++++++++++++++++++++++++++++++++++--------- > > > 1 file changed, 36 insertions(+), 9 deletions(-) > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > index 7add81a6..7904bca8 100644 > > > --- a/src/cam/kms_sink.cpp > > > +++ b/src/cam/kms_sink.cpp > > > @@ -11,6 +11,7 @@ > > > #include <algorithm> > > > #include <assert.h> > > > #include <iostream> > > > +#include <limits.h> > > > #include <memory> > > > #include <stdint.h> > > > #include <string.h> > > > @@ -112,6 +113,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > > > + /* Find the best mode for the stream size. */ > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > > const auto iter = std::find_if(modes.begin(), modes.end(), > > > [&](const DRM::Mode &mode) { > > > @@ -120,6 +122,32 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > }); > > > if (iter == modes.end()) { > > > > I think you could drop this search, as the iteration over the modes > > below should be enough. Would you mind testing the v5 I posted ? > > > > > std::cerr << "No mode matching " << cfg.size << std::endl; > > > + > > > + unsigned int cfgArea = cfg.size.width * cfg.size.height; > > > + unsigned int bestDistance = UINT_MAX; > > > + > > > + for (const DRM::Mode &mode : modes) { > > > + unsigned int modeArea = mode.hdisplay * mode.vdisplay; > > > + unsigned int distance = modeArea > cfgArea > > > + ? modeArea - cfgArea > > > + : cfgArea - modeArea; > > > + > > > + if (distance < bestDistance) { > > > + mode_ = &mode; > > > + bestDistance = distance; > > > + > > > + /* > > > + * If the sizes match exactly, there will be no better > > > + * match. > > > + */ > > > + if (distance == 0) > > > + break; > > > + } > > > + } > > > + } > > > + > > > + if (!mode_) { > > > + std::cerr << "No modes\n"; > > > return -EINVAL; > > > } > > > > > > @@ -127,7 +155,6 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > if (ret < 0) > > > return ret; > > > > > > - mode_ = &*iter; > > > size_ = cfg.size; > > > stride_ = cfg.stride; > > > > > > @@ -199,10 +226,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > return ret; > > > } > > > > > > - std::cout > > > - << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() > > > - << ", connector " << connector_->name() > > > - << " (" << connector_->id() << ")" << std::endl; > > > + std::cout << "Using KMS plane " << plane_->id() << ", CRTC " > > > + << crtc_->id() << ", connector " << connector_->name() << " (" > > > + << connector_->id() << "), mode " << mode_->hdisplay << "x" > > > + << mode_->vdisplay << "@" << mode_->vrefresh << std::endl; > > > > > > return 0; > > > } > > > @@ -295,12 +322,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest) > > > drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id()); > > > drmRequest->addProperty(plane_, "SRC_X", 0 << 16); > > > drmRequest->addProperty(plane_, "SRC_Y", 0 << 16); > > > - drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16); > > > - drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16); > > > + drmRequest->addProperty(plane_, "SRC_W", size_.width << 16); > > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > > drmRequest->addProperty(plane_, "CRTC_X", 0); > > > drmRequest->addProperty(plane_, "CRTC_Y", 0); > > > - drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay); > > > - drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay); > > > + drmRequest->addProperty(plane_, "CRTC_W", size_.width); > > > + drmRequest->addProperty(plane_, "CRTC_H", size_.height); > > > > > > flags |= DRM::AtomicRequest::FlagAllowModeset; > > > }
diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp index 7add81a6..7904bca8 100644 --- a/src/cam/kms_sink.cpp +++ b/src/cam/kms_sink.cpp @@ -11,6 +11,7 @@ #include <algorithm> #include <assert.h> #include <iostream> +#include <limits.h> #include <memory> #include <stdint.h> #include <string.h> @@ -112,6 +113,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) const libcamera::StreamConfiguration &cfg = config.at(0); + /* Find the best mode for the stream size. */ const std::vector<DRM::Mode> &modes = connector_->modes(); const auto iter = std::find_if(modes.begin(), modes.end(), [&](const DRM::Mode &mode) { @@ -120,6 +122,32 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) }); if (iter == modes.end()) { std::cerr << "No mode matching " << cfg.size << std::endl; + + unsigned int cfgArea = cfg.size.width * cfg.size.height; + unsigned int bestDistance = UINT_MAX; + + for (const DRM::Mode &mode : modes) { + unsigned int modeArea = mode.hdisplay * mode.vdisplay; + unsigned int distance = modeArea > cfgArea + ? modeArea - cfgArea + : cfgArea - modeArea; + + if (distance < bestDistance) { + mode_ = &mode; + bestDistance = distance; + + /* + * If the sizes match exactly, there will be no better + * match. + */ + if (distance == 0) + break; + } + } + } + + if (!mode_) { + std::cerr << "No modes\n"; return -EINVAL; } @@ -127,7 +155,6 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) if (ret < 0) return ret; - mode_ = &*iter; size_ = cfg.size; stride_ = cfg.stride; @@ -199,10 +226,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) return ret; } - std::cout - << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() - << ", connector " << connector_->name() - << " (" << connector_->id() << ")" << std::endl; + std::cout << "Using KMS plane " << plane_->id() << ", CRTC " + << crtc_->id() << ", connector " << connector_->name() << " (" + << connector_->id() << "), mode " << mode_->hdisplay << "x" + << mode_->vdisplay << "@" << mode_->vrefresh << std::endl; return 0; } @@ -295,12 +322,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest) drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id()); drmRequest->addProperty(plane_, "SRC_X", 0 << 16); drmRequest->addProperty(plane_, "SRC_Y", 0 << 16); - drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16); - drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16); + drmRequest->addProperty(plane_, "SRC_W", size_.width << 16); + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); drmRequest->addProperty(plane_, "CRTC_X", 0); drmRequest->addProperty(plane_, "CRTC_Y", 0); - drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay); - drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay); + drmRequest->addProperty(plane_, "CRTC_W", size_.width); + drmRequest->addProperty(plane_, "CRTC_H", size_.height); flags |= DRM::AtomicRequest::FlagAllowModeset; }