[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:06:01 CET 2022
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
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