[libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation that camera and display must match
Eric Curtin
ecurtin at redhat.com
Tue Feb 8 12:10:29 CET 2022
On Tue, 8 Feb 2022 at 11:06, Eric Curtin <ecurtin at redhat.com> wrote:
>
> On Tue, 8 Feb 2022 at 10:56, Laurent Pinchart
> <laurent.pinchart at 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 at redhat.com>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Tested and support drawing from negative pixel range
> > > > > > kernel parameter (video=960x540 at 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
> >
More information about the libcamera-devel
mailing list