[libcamera-devel] [PATCH v2] cam: kms_sink: Remove limitation that camera and display must match

[Eric Curtin]

On Mon, 7 Feb 2022 at 16:11, Kieran Bingham
<kieran.bingham at 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 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.

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
> >
>