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

[Eric Curtin]

On Fri, 15 Apr 2022 at 17:21, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Tue, Mar 29, 2022 at 02:02:21PM +0100, Eric Curtin via libcamera-devel 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 right 0, 0 corner.
>
> Isn't that the top-left corner ?

Yes sorry will change commit message.

>
> > 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
> > 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.
> > ---
> >  src/cam/kms_sink.cpp | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index da579846..5847c4e1 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -119,17 +119,21 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> >                                                     mode.vdisplay == cfg.size.height;
> >                                      });
> >       if (iter == modes.end()) {
> > -             std::cerr
> > -                     << "No mode matching " << cfg.size.toString()
> > -                     << std::endl;
> > -             return -EINVAL;
> > +                if (modes.empty()) {
>
> There are spaces used for indentation.

Will change

>
> > +                     std::cerr << "No modes\n";
> > +                     return -EINVAL;
> > +                }
> > +
> > +                mode_ = &modes[0];
> >       }
> > +        else {
> > +                mode_ = &*iter;
> > +        }
> >
> >       int ret = configurePipeline(cfg.pixelFormat);
> >       if (ret < 0)
> >               return ret;
> >
> > -     mode_ = &*iter;
> >       size_ = cfg.size;
> >       stride_ = cfg.stride;
> >
> > @@ -297,12 +301,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);
>
> Not all drivers will support this. That's fine, it's not a regression as
> they wouldn't work anyway, but I'm annoyed that things will fail only
> when trying to display the first frame, and without a clear error
> message :-(

Yeah I tried to focus on just making sure this isn't a regression,
because for me, this helps me test this neat minimal KMS sink with no
dependencies on things like SDL and/or Qt. If people want to extend
after this, with various probing etc., great! Actually SDL is probably
a good reference there if anyone wants to tackle that, they seem to
have it figured out.

>
> Ideally we should try to probe the device to see what it supports (which
> could include scaling) and then pick the best option in
> KMSSink::configure().

Could I leave this to someone if they wanted to extend this? I was
trying to do the most minimal change to get this sink testable for
non-resolution identical devices. I need to see frames on the screen,
that's pretty much it, it's a testing tool for me.

I have zero issue with people altering this in future if they want to
do more probing, etc.

>
> >
> >               flags |= DRM::AtomicRequest::FlagAllowModeset;
> >       }
>
> --
> Regards,
>
> Laurent Pinchart
>