| Message ID | 20220207150059.22515-1-ecurtin@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric, Quoting Eric Curtin (2022-02-07 15:00:59) > 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 exactly in > terms of pixels. Centralize the image. This works for the case > where camera output has more pixels than the display and > vice-versa. In the case where there are too many pixels for the > display, we take the most central part of the image cropping out > the border. Thankyou, I'm very pleased to see this patch, As I'm sure you're aware from our discussions on IRC. I haven't actually been able to test direct render with the DRM part here, so I think this will be really helpful. I can't test this right now though, but I hope to this week, so just some quick comments while I skim through. > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > > Changes in v2: > - Tested and support drawing from negative pixel range > kernel parameter (video=960x540@60) was useful here > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > src/cam/kms_sink.h | 2 ++ > 2 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > index 973cd370..8eb51454 100644 > --- a/src/cam/kms_sink.cpp > +++ b/src/cam/kms_sink.cpp > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > const libcamera::StreamConfiguration &cfg = config.at(0); > > const std::vector<DRM::Mode> &modes = connector_->modes(); > - const auto iter = std::find_if(modes.begin(), modes.end(), > - [&](const DRM::Mode &mode) { > - return mode.hdisplay == cfg.size.width && > - mode.vdisplay == cfg.size.height; > - }); Hrm, I would maybe expect to see some sort of 'best mode' search here to find the closest mode that fits the incoming image. I'm sure that might be a pattern that exists already somewhere. But I expect being able to draw on any mode, is going to make this work on more devices than only drawing on an exact mode. > - if (iter == modes.end()) { > - std::cerr > - << "No mode matching " << cfg.size.toString() > - << std::endl; > - return -EINVAL; > - } > > int ret = configurePipeline(cfg.pixelFormat); > if (ret < 0) > return ret; > > - mode_ = &*iter; > + mode_ = &modes[0]; > size_ = cfg.size; > + > + // We need to cast for the case where the camera output has more > + // pixels than the display, in this case we start drawing from a > + // negative pixel point to crop out the content to display just > + // the middle part. Our code style uses /* * */ For comment blocks. I guess our checkstyle doesn't actually pick up on that though. > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; We have a Rectangle class in include/libcamera/geometry.h which can handle centering, and might be useful here. > stride_ = cfg.stride; > > return 0; > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > + drmRequest->addProperty(plane_, "CRTC_X", x_); > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > + 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.h b/src/cam/kms_sink.h > index 4a0a872c..2c16182c 100644 > --- a/src/cam/kms_sink.h > +++ b/src/cam/kms_sink.h > @@ -61,6 +61,8 @@ private: > libcamera::PixelFormat format_; > libcamera::Size size_; > unsigned int stride_; > + int x_; // Where to start drawing camera output > + int y_; // Where to start drawing camera output /* */ comments here too please. > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > > -- > 2.34.1 >
Hi Eric, Thank you for the patch. On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote: > Quoting Eric Curtin (2022-02-07 15:00:59) > > 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 exactly in > > terms of pixels. Centralize the image. This works for the case > > where camera output has more pixels than the display and > > vice-versa. In the case where there are too many pixels for the > > display, we take the most central part of the image cropping out > > the border. > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware > from our discussions on IRC. > > I haven't actually been able to test direct render with the DRM part > here, so I think this will be really helpful. I can't test this right > now though, but I hope to this week, so just some quick comments while I > skim through. > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > > > Changes in v2: > > - Tested and support drawing from negative pixel range > > kernel parameter (video=960x540@60) was useful here > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > > src/cam/kms_sink.h | 2 ++ > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > index 973cd370..8eb51454 100644 > > --- a/src/cam/kms_sink.cpp > > +++ b/src/cam/kms_sink.cpp > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > - const auto iter = std::find_if(modes.begin(), modes.end(), > > - [&](const DRM::Mode &mode) { > > - return mode.hdisplay == cfg.size.width && > > - mode.vdisplay == cfg.size.height; > > - }); > > Hrm, I would maybe expect to see some sort of 'best mode' search here to > find the closest mode that fits the incoming image. I'm sure that might > be a pattern that exists already somewhere. > > But I expect being able to draw on any mode, is going to make this work > on more devices than only drawing on an exact mode. > > > - if (iter == modes.end()) { > > - std::cerr > > - << "No mode matching " << cfg.size.toString() > > - << std::endl; > > - return -EINVAL; > > - } > > > > int ret = configurePipeline(cfg.pixelFormat); > > if (ret < 0) > > return ret; > > > > - mode_ = &*iter; > > + mode_ = &modes[0]; > > size_ = cfg.size; > > + > > + // We need to cast for the case where the camera output has more > > + // pixels than the display, in this case we start drawing from a > > + // negative pixel point to crop out the content to display just > > + // the middle part. > > Our code style uses > /* > * > */ > > For comment blocks. I guess our checkstyle doesn't actually pick up on > that though. > > > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; Not all devices support negative CRTC_X and CRTC_Y values, nor do all devices support planes that do not align 1:1 with the entire CRTC. Furthermore, some devices support scaling. I think you'll need something a bit more elaborate here, using atomic test-only commits to try multiple configurations and pick the best one. > We have a Rectangle class in include/libcamera/geometry.h which can > handle centering, and might be useful here. > > > stride_ = cfg.stride; > > > > return 0; > > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > + drmRequest->addProperty(plane_, "CRTC_X", x_); > > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > > + 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.h b/src/cam/kms_sink.h > > index 4a0a872c..2c16182c 100644 > > --- a/src/cam/kms_sink.h > > +++ b/src/cam/kms_sink.h > > @@ -61,6 +61,8 @@ private: > > libcamera::PixelFormat format_; > > libcamera::Size size_; > > unsigned int stride_; > > + int x_; // Where to start drawing camera output > > + int y_; // Where to start drawing camera output > > /* */ comments here too please. > > > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > >
On Mon, 7 Feb 2022 at 16:11, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Eric, > > Quoting Eric Curtin (2022-02-07 15:00:59) > > 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 exactly in > > terms of pixels. Centralize the image. This works for the case > > where camera output has more pixels than the display and > > vice-versa. In the case where there are too many pixels for the > > display, we take the most central part of the image cropping out > > the border. > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware > from our discussions on IRC. > > I haven't actually been able to test direct render with the DRM part > here, so I think this will be really helpful. I can't test this right > now though, but I hope to this week, so just some quick comments while I > skim through. If you decide to test this patch try and include the other patch I posted recently also, my 2 test devices don't work without the other one. > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > > > Changes in v2: > > - Tested and support drawing from negative pixel range > > kernel parameter (video=960x540@60) was useful here > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > > src/cam/kms_sink.h | 2 ++ > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > index 973cd370..8eb51454 100644 > > --- a/src/cam/kms_sink.cpp > > +++ b/src/cam/kms_sink.cpp > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > - const auto iter = std::find_if(modes.begin(), modes.end(), > > - [&](const DRM::Mode &mode) { > > - return mode.hdisplay == cfg.size.width && > > - mode.vdisplay == cfg.size.height; > > - }); > > Hrm, I would maybe expect to see some sort of 'best mode' search here to > find the closest mode that fits the incoming image. I'm sure that might > be a pattern that exists already somewhere. I did think about modifying this search briefly once or twice, I tried to remove any sort of searching, looping I encountered in any parts of the code I re-wrote for my needs in twincam, trying to be mindful of performance. My machines typically only have one mode in that vector: 1920x1080 SInce there are many ways to skin this cat, I decided to go with the simplest way, just pick the first one. But how does this sound... Restore the original find_if and if that doesn't find anything to match exactly, just pick mode[0] and crop? Longer term (sometime after this patch), it might make more sense to add a command line option to pick a mode rather than loop through the modes. > > But I expect being able to draw on any mode, is going to make this work > on more devices than only drawing on an exact mode. > > > > - if (iter == modes.end()) { > > - std::cerr > > - << "No mode matching " << cfg.size.toString() > > - << std::endl; > > - return -EINVAL; > > - } > > > > int ret = configurePipeline(cfg.pixelFormat); > > if (ret < 0) > > return ret; > > > > - mode_ = &*iter; > > + mode_ = &modes[0]; > > size_ = cfg.size; > > + > > + // We need to cast for the case where the camera output has more > > + // pixels than the display, in this case we start drawing from a > > + // negative pixel point to crop out the content to display just > > + // the middle part. > > Our code style uses > /* > * > */ > > For comment blocks. I guess our checkstyle doesn't actually pick up on > that though. Will fix. > > > > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; > > We have a Rectangle class in include/libcamera/geometry.h which can > handle centering, and might be useful here. Will use Rectangle and Point classes. > > > stride_ = cfg.stride; > > > > return 0; > > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > + drmRequest->addProperty(plane_, "CRTC_X", x_); > > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > > + 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.h b/src/cam/kms_sink.h > > index 4a0a872c..2c16182c 100644 > > --- a/src/cam/kms_sink.h > > +++ b/src/cam/kms_sink.h > > @@ -61,6 +61,8 @@ private: > > libcamera::PixelFormat format_; > > libcamera::Size size_; > > unsigned int stride_; > > + int x_; // Where to start drawing camera output > > + int y_; // Where to start drawing camera output > > /* */ comments here too please. Will do. > > > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > > > > -- > > 2.34.1 > > >
On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > Thank you for the patch. > > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote: > > Quoting Eric Curtin (2022-02-07 15:00:59) > > > 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 exactly in > > > terms of pixels. Centralize the image. This works for the case > > > where camera output has more pixels than the display and > > > vice-versa. In the case where there are too many pixels for the > > > display, we take the most central part of the image cropping out > > > the border. > > > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware > > from our discussions on IRC. > > > > I haven't actually been able to test direct render with the DRM part > > here, so I think this will be really helpful. I can't test this right > > now though, but I hope to this week, so just some quick comments while I > > skim through. > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > > > > Changes in v2: > > > - Tested and support drawing from negative pixel range > > > kernel parameter (video=960x540@60) was useful here > > > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > > > src/cam/kms_sink.h | 2 ++ > > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > index 973cd370..8eb51454 100644 > > > --- a/src/cam/kms_sink.cpp > > > +++ b/src/cam/kms_sink.cpp > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > > - const auto iter = std::find_if(modes.begin(), modes.end(), > > > - [&](const DRM::Mode &mode) { > > > - return mode.hdisplay == cfg.size.width && > > > - mode.vdisplay == cfg.size.height; > > > - }); > > > > Hrm, I would maybe expect to see some sort of 'best mode' search here to > > find the closest mode that fits the incoming image. I'm sure that might > > be a pattern that exists already somewhere. > > > > But I expect being able to draw on any mode, is going to make this work > > on more devices than only drawing on an exact mode. > > > > > - if (iter == modes.end()) { > > > - std::cerr > > > - << "No mode matching " << cfg.size.toString() > > > - << std::endl; > > > - return -EINVAL; > > > - } > > > > > > int ret = configurePipeline(cfg.pixelFormat); > > > if (ret < 0) > > > return ret; > > > > > > - mode_ = &*iter; > > > + mode_ = &modes[0]; > > > size_ = cfg.size; > > > + > > > + // We need to cast for the case where the camera output has more > > > + // pixels than the display, in this case we start drawing from a > > > + // negative pixel point to crop out the content to display just > > > + // the middle part. > > > > Our code style uses > > /* > > * > > */ > > > > For comment blocks. I guess our checkstyle doesn't actually pick up on > > that though. > > > > > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > > > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; > > Not all devices support negative CRTC_X and CRTC_Y values, nor do all > devices support planes that do not align 1:1 with the entire CRTC. > Furthermore, some devices support scaling. I think you'll need something > a bit more elaborate here, using atomic test-only commits to try > multiple configurations and pick the best one. The main goal of this patch is to enable more camera and display combinations, I was hoping scaling would be a future separate patch. Still only 1x1 pixel alignment, just starting from a non-zero x and y point. The negative and positive x and y's worked fine on my machines. I need this patch for most of my hardware or else I get "No mode matching" problem. If we restore the find_if and use mode[0] if that fails, then we can only gain support for additional devices. How does this sound? > > > We have a Rectangle class in include/libcamera/geometry.h which can > > handle centering, and might be useful here. > > > > > stride_ = cfg.stride; > > > > > > return 0; > > > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > > + drmRequest->addProperty(plane_, "CRTC_X", x_); > > > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > > > + 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.h b/src/cam/kms_sink.h > > > index 4a0a872c..2c16182c 100644 > > > --- a/src/cam/kms_sink.h > > > +++ b/src/cam/kms_sink.h > > > @@ -61,6 +61,8 @@ private: > > > libcamera::PixelFormat format_; > > > libcamera::Size size_; > > > unsigned int stride_; > > > + int x_; // Where to start drawing camera output > > > + int y_; // Where to start drawing camera output > > > > /* */ comments here too please. > > > > > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > > > > > -- > Regards, > > Laurent Pinchart >
Hi Eric, On Tue, Feb 08, 2022 at 10:49:54AM +0000, Eric Curtin wrote: > On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart wrote: > > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote: > > > Quoting Eric Curtin (2022-02-07 15:00:59) > > > > 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 exactly in > > > > terms of pixels. Centralize the image. This works for the case > > > > where camera output has more pixels than the display and > > > > vice-versa. In the case where there are too many pixels for the > > > > display, we take the most central part of the image cropping out > > > > the border. > > > > > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware > > > from our discussions on IRC. > > > > > > I haven't actually been able to test direct render with the DRM part > > > here, so I think this will be really helpful. I can't test this right > > > now though, but I hope to this week, so just some quick comments while I > > > skim through. > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > > > > > Changes in v2: > > > > - Tested and support drawing from negative pixel range > > > > kernel parameter (video=960x540@60) was useful here > > > > > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > > > > src/cam/kms_sink.h | 2 ++ > > > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > > index 973cd370..8eb51454 100644 > > > > --- a/src/cam/kms_sink.cpp > > > > +++ b/src/cam/kms_sink.cpp > > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > > > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > > > - const auto iter = std::find_if(modes.begin(), modes.end(), > > > > - [&](const DRM::Mode &mode) { > > > > - return mode.hdisplay == cfg.size.width && > > > > - mode.vdisplay == cfg.size.height; > > > > - }); > > > > > > Hrm, I would maybe expect to see some sort of 'best mode' search here to > > > find the closest mode that fits the incoming image. I'm sure that might > > > be a pattern that exists already somewhere. > > > > > > But I expect being able to draw on any mode, is going to make this work > > > on more devices than only drawing on an exact mode. > > > > > > > - if (iter == modes.end()) { > > > > - std::cerr > > > > - << "No mode matching " << cfg.size.toString() > > > > - << std::endl; > > > > - return -EINVAL; > > > > - } > > > > > > > > int ret = configurePipeline(cfg.pixelFormat); > > > > if (ret < 0) > > > > return ret; > > > > > > > > - mode_ = &*iter; > > > > + mode_ = &modes[0]; > > > > size_ = cfg.size; > > > > + > > > > + // We need to cast for the case where the camera output has more > > > > + // pixels than the display, in this case we start drawing from a > > > > + // negative pixel point to crop out the content to display just > > > > + // the middle part. > > > > > > Our code style uses > > > /* > > > * > > > */ > > > > > > For comment blocks. I guess our checkstyle doesn't actually pick up on > > > that though. > > > > > > > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > > > > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; > > > > Not all devices support negative CRTC_X and CRTC_Y values, nor do all > > devices support planes that do not align 1:1 with the entire CRTC. > > Furthermore, some devices support scaling. I think you'll need something > > a bit more elaborate here, using atomic test-only commits to try > > multiple configurations and pick the best one. > > The main goal of this patch is to enable more camera and display combinations, > I was hoping scaling would be a future separate patch. Scaling can be done later indeed, that's not a problem. > Still only 1x1 pixel > alignment, just starting from a non-zero x and y point. The negative and positive > x and y's worked fine on my machines. I need this patch for most of my > hardware or else I get "No mode matching" problem. > > If we restore the find_if and use mode[0] if that fails, then we can only gain support > for additional devices. How does this sound? We certainly don't want to break existing working use cases for devices that don't support negative coordinates, or planes that don't span the entire CRTC. I think that searching for an optimal mode unconditionally first will help there. > > > We have a Rectangle class in include/libcamera/geometry.h which can > > > handle centering, and might be useful here. > > > > > > > stride_ = cfg.stride; > > > > > > > > return 0; > > > > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > > > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > > > + drmRequest->addProperty(plane_, "CRTC_X", x_); > > > > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > > > > + 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.h b/src/cam/kms_sink.h > > > > index 4a0a872c..2c16182c 100644 > > > > --- a/src/cam/kms_sink.h > > > > +++ b/src/cam/kms_sink.h > > > > @@ -61,6 +61,8 @@ private: > > > > libcamera::PixelFormat format_; > > > > libcamera::Size size_; > > > > unsigned int stride_; > > > > + int x_; // Where to start drawing camera output > > > > + int y_; // Where to start drawing camera output > > > > > > /* */ comments here too please. > > > > > > > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > > > >
On Tue, 8 Feb 2022 at 10:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > On Tue, Feb 08, 2022 at 10:49:54AM +0000, Eric Curtin wrote: > > On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart wrote: > > > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote: > > > > Quoting Eric Curtin (2022-02-07 15:00:59) > > > > > 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 exactly in > > > > > terms of pixels. Centralize the image. This works for the case > > > > > where camera output has more pixels than the display and > > > > > vice-versa. In the case where there are too many pixels for the > > > > > display, we take the most central part of the image cropping out > > > > > the border. > > > > > > > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware > > > > from our discussions on IRC. > > > > > > > > I haven't actually been able to test direct render with the DRM part > > > > here, so I think this will be really helpful. I can't test this right > > > > now though, but I hope to this week, so just some quick comments while I > > > > skim through. > > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > --- > > > > > > > > > > Changes in v2: > > > > > - Tested and support drawing from negative pixel range > > > > > kernel parameter (video=960x540@60) was useful here > > > > > > > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > > > > > src/cam/kms_sink.h | 2 ++ > > > > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > > > index 973cd370..8eb51454 100644 > > > > > --- a/src/cam/kms_sink.cpp > > > > > +++ b/src/cam/kms_sink.cpp > > > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > > > > > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > > > > - const auto iter = std::find_if(modes.begin(), modes.end(), > > > > > - [&](const DRM::Mode &mode) { > > > > > - return mode.hdisplay == cfg.size.width && > > > > > - mode.vdisplay == cfg.size.height; > > > > > - }); > > > > > > > > Hrm, I would maybe expect to see some sort of 'best mode' search here to > > > > find the closest mode that fits the incoming image. I'm sure that might > > > > be a pattern that exists already somewhere. > > > > > > > > But I expect being able to draw on any mode, is going to make this work > > > > on more devices than only drawing on an exact mode. > > > > > > > > > - if (iter == modes.end()) { > > > > > - std::cerr > > > > > - << "No mode matching " << cfg.size.toString() > > > > > - << std::endl; > > > > > - return -EINVAL; > > > > > - } > > > > > > > > > > int ret = configurePipeline(cfg.pixelFormat); > > > > > if (ret < 0) > > > > > return ret; > > > > > > > > > > - mode_ = &*iter; > > > > > + mode_ = &modes[0]; > > > > > size_ = cfg.size; > > > > > + > > > > > + // We need to cast for the case where the camera output has more > > > > > + // pixels than the display, in this case we start drawing from a > > > > > + // negative pixel point to crop out the content to display just > > > > > + // the middle part. > > > > > > > > Our code style uses > > > > /* > > > > * > > > > */ > > > > > > > > For comment blocks. I guess our checkstyle doesn't actually pick up on > > > > that though. > > > > > > > > > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > > > > > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; > > > > > > Not all devices support negative CRTC_X and CRTC_Y values, nor do all > > > devices support planes that do not align 1:1 with the entire CRTC. > > > Furthermore, some devices support scaling. I think you'll need something > > > a bit more elaborate here, using atomic test-only commits to try > > > multiple configurations and pick the best one. > > > > The main goal of this patch is to enable more camera and display combinations, > > I was hoping scaling would be a future separate patch. > > Scaling can be done later indeed, that's not a problem. > > > Still only 1x1 pixel > > alignment, just starting from a non-zero x and y point. The negative and positive > > x and y's worked fine on my machines. I need this patch for most of my > > hardware or else I get "No mode matching" problem. > > > > If we restore the find_if and use mode[0] if that fails, then we can only gain support > > for additional devices. How does this sound? > > We certainly don't want to break existing working use cases for devices > that don't support negative coordinates, or planes that don't span the > entire CRTC. I think that searching for an optimal mode unconditionally > first will help there. This solution would ensure we don't break existing working use cases as it would only attempt alternate x and y points in the cases that would previously have printed "No mode matching". Another option is to print from Point 0, 0 every time, starting drawing from the corner. The problem with this is, it's ugly, you are very close to the bezels of this display (in case some pixels are hard to view near the bezel, one of my displays is like this) and in the case where there are more camera pixels than display pixels, you get output from the corner of the camera rather than the centre. The corner part of the image is probably not as focused as the centre, lower quality, etc. > > > > > We have a Rectangle class in include/libcamera/geometry.h which can > > > > handle centering, and might be useful here. > > > > > > > > > stride_ = cfg.stride; > > > > > > > > > > return 0; > > > > > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > > > > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > > > > + drmRequest->addProperty(plane_, "CRTC_X", x_); > > > > > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > > > > > + 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.h b/src/cam/kms_sink.h > > > > > index 4a0a872c..2c16182c 100644 > > > > > --- a/src/cam/kms_sink.h > > > > > +++ b/src/cam/kms_sink.h > > > > > @@ -61,6 +61,8 @@ private: > > > > > libcamera::PixelFormat format_; > > > > > libcamera::Size size_; > > > > > unsigned int stride_; > > > > > + int x_; // Where to start drawing camera output > > > > > + int y_; // Where to start drawing camera output > > > > > > > > /* */ comments here too please. > > > > > > > > > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > > > > > > > -- > Regards, > > Laurent Pinchart >
On Tue, 8 Feb 2022 at 11:06, Eric Curtin <ecurtin@redhat.com> wrote: > > On Tue, 8 Feb 2022 at 10:56, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Eric, > > > > On Tue, Feb 08, 2022 at 10:49:54AM +0000, Eric Curtin wrote: > > > On Mon, 7 Feb 2022 at 22:48, Laurent Pinchart wrote: > > > > On Mon, Feb 07, 2022 at 04:11:50PM +0000, Kieran Bingham wrote: > > > > > Quoting Eric Curtin (2022-02-07 15:00:59) > > > > > > 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 exactly in > > > > > > terms of pixels. Centralize the image. This works for the case > > > > > > where camera output has more pixels than the display and > > > > > > vice-versa. In the case where there are too many pixels for the > > > > > > display, we take the most central part of the image cropping out > > > > > > the border. > > > > > > > > > > Thankyou, I'm very pleased to see this patch, As I'm sure you're aware > > > > > from our discussions on IRC. > > > > > > > > > > I haven't actually been able to test direct render with the DRM part > > > > > here, so I think this will be really helpful. I can't test this right > > > > > now though, but I hope to this week, so just some quick comments while I > > > > > skim through. > > > > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > > --- > > > > > > > > > > > > Changes in v2: > > > > > > - Tested and support drawing from negative pixel range > > > > > > kernel parameter (video=960x540@60) was useful here > > > > > > > > > > > > src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ > > > > > > src/cam/kms_sink.h | 2 ++ > > > > > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > > > > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > > > > index 973cd370..8eb51454 100644 > > > > > > --- a/src/cam/kms_sink.cpp > > > > > > +++ b/src/cam/kms_sink.cpp > > > > > > @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > > > const libcamera::StreamConfiguration &cfg = config.at(0); > > > > > > > > > > > > const std::vector<DRM::Mode> &modes = connector_->modes(); > > > > > > - const auto iter = std::find_if(modes.begin(), modes.end(), > > > > > > - [&](const DRM::Mode &mode) { > > > > > > - return mode.hdisplay == cfg.size.width && > > > > > > - mode.vdisplay == cfg.size.height; > > > > > > - }); > > > > > > > > > > Hrm, I would maybe expect to see some sort of 'best mode' search here to > > > > > find the closest mode that fits the incoming image. I'm sure that might > > > > > be a pattern that exists already somewhere. > > > > > > > > > > But I expect being able to draw on any mode, is going to make this work > > > > > on more devices than only drawing on an exact mode. > > > > > > > > > > > - if (iter == modes.end()) { > > > > > > - std::cerr > > > > > > - << "No mode matching " << cfg.size.toString() > > > > > > - << std::endl; > > > > > > - return -EINVAL; > > > > > > - } > > > > > > > > > > > > int ret = configurePipeline(cfg.pixelFormat); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > > > > > > > - mode_ = &*iter; > > > > > > + mode_ = &modes[0]; > > > > > > size_ = cfg.size; > > > > > > + > > > > > > + // We need to cast for the case where the camera output has more > > > > > > + // pixels than the display, in this case we start drawing from a > > > > > > + // negative pixel point to crop out the content to display just > > > > > > + // the middle part. > > > > > > > > > > Our code style uses > > > > > /* > > > > > * > > > > > */ > > > > > > > > > > For comment blocks. I guess our checkstyle doesn't actually pick up on > > > > > that though. > > > > > > > > > > > + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; > > > > > > + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; > > > > > > > > Not all devices support negative CRTC_X and CRTC_Y values, nor do all > > > > devices support planes that do not align 1:1 with the entire CRTC. > > > > Furthermore, some devices support scaling. I think you'll need something > > > > a bit more elaborate here, using atomic test-only commits to try > > > > multiple configurations and pick the best one. > > > > > > The main goal of this patch is to enable more camera and display combinations, > > > I was hoping scaling would be a future separate patch. > > > > Scaling can be done later indeed, that's not a problem. > > > > > Still only 1x1 pixel > > > alignment, just starting from a non-zero x and y point. The negative and positive > > > x and y's worked fine on my machines. I need this patch for most of my > > > hardware or else I get "No mode matching" problem. > > > > > > If we restore the find_if and use mode[0] if that fails, then we can only gain support > > > for additional devices. How does this sound? > > > > We certainly don't want to break existing working use cases for devices > > that don't support negative coordinates, or planes that don't span the > > entire CRTC. I think that searching for an optimal mode unconditionally > > first will help there. > > This solution would ensure we don't break existing working use cases > as it would only attempt alternate x and y points in the cases that > would previously have printed "No mode matching". > > Another option is to print from Point 0, 0 every time, starting *Another option is to draw from Point 0, 0 every time, starting > drawing from the corner. The problem with this is, it's ugly, you are > very close to the bezels of this display (in case some pixels are hard > to view near the bezel, one of my displays is like this) and in the > case where there are more camera pixels than display pixels, you get > output from the corner of the camera rather than the centre. The > corner part of the image is probably not as focused as the centre, > lower quality, etc. > > > > > > > > We have a Rectangle class in include/libcamera/geometry.h which can > > > > > handle centering, and might be useful here. > > > > > > > > > > > stride_ = cfg.stride; > > > > > > > > > > > > return 0; > > > > > > @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); > > > > > > + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); > > > > > > + drmRequest->addProperty(plane_, "CRTC_X", x_); > > > > > > + drmRequest->addProperty(plane_, "CRTC_Y", y_); > > > > > > + 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.h b/src/cam/kms_sink.h > > > > > > index 4a0a872c..2c16182c 100644 > > > > > > --- a/src/cam/kms_sink.h > > > > > > +++ b/src/cam/kms_sink.h > > > > > > @@ -61,6 +61,8 @@ private: > > > > > > libcamera::PixelFormat format_; > > > > > > libcamera::Size size_; > > > > > > unsigned int stride_; > > > > > > + int x_; // Where to start drawing camera output > > > > > > + int y_; // Where to start drawing camera output > > > > > > > > > > /* */ comments here too please. > > > > > > > > > > > std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_; > > > > > > > > > > -- > > Regards, > > > > Laurent Pinchart > >
diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp index 973cd370..8eb51454 100644 --- a/src/cam/kms_sink.cpp +++ b/src/cam/kms_sink.cpp @@ -113,24 +113,20 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) const libcamera::StreamConfiguration &cfg = config.at(0); const std::vector<DRM::Mode> &modes = connector_->modes(); - const auto iter = std::find_if(modes.begin(), modes.end(), - [&](const DRM::Mode &mode) { - return mode.hdisplay == cfg.size.width && - mode.vdisplay == cfg.size.height; - }); - if (iter == modes.end()) { - std::cerr - << "No mode matching " << cfg.size.toString() - << std::endl; - return -EINVAL; - } int ret = configurePipeline(cfg.pixelFormat); if (ret < 0) return ret; - mode_ = &*iter; + mode_ = &modes[0]; size_ = cfg.size; + + // We need to cast for the case where the camera output has more + // pixels than the display, in this case we start drawing from a + // negative pixel point to crop out the content to display just + // the middle part. + x_ = (mode_->hdisplay - static_cast<int>(size_.width)) / 2; + y_ = (mode_->vdisplay - static_cast<int>(size_.height)) / 2; stride_ = cfg.stride; return 0; @@ -297,12 +293,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_, "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_, "SRC_W", size_.width << 16); + drmRequest->addProperty(plane_, "SRC_H", size_.height << 16); + drmRequest->addProperty(plane_, "CRTC_X", x_); + drmRequest->addProperty(plane_, "CRTC_Y", y_); + 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.h b/src/cam/kms_sink.h index 4a0a872c..2c16182c 100644 --- a/src/cam/kms_sink.h +++ b/src/cam/kms_sink.h @@ -61,6 +61,8 @@ private: libcamera::PixelFormat format_; libcamera::Size size_; unsigned int stride_; + int x_; // Where to start drawing camera output + int y_; // Where to start drawing camera output std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
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 exactly in terms of pixels. Centralize the image. This works for the case where camera output has more pixels than the display and vice-versa. In the case where there are too many pixels for the display, we take the most central part of the image cropping out the border. Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- Changes in v2: - Tested and support drawing from negative pixel range kernel parameter (video=960x540@60) was useful here src/cam/kms_sink.cpp | 32 ++++++++++++++------------------ src/cam/kms_sink.h | 2 ++ 2 files changed, 16 insertions(+), 18 deletions(-)