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

[Laurent Pinchart]

Hi Eric,

On Mon, Jun 20, 2022 at 01:33:31PM +0100, Eric Curtin wrote:
> On Mon, 20 Jun 2022 at 13:00, Laurent Pinchart wrote:
> > On Mon, Jun 20, 2022 at 12:52:39PM +0100, Eric Curtin 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 left 0, 0 corner. Try and pick the mode closest to the
> > > stream size.
> > >
> > > Suggested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Tested-by: Eric Curtin <ecurtin at redhat.com>
> > > ---
> > > Changes in v5:
> > > - Tested Laurents suggested change successfully
> > > - Added to commit message about trying to pick closest
> >
> > I've also sent a v5, on Sunday (retaining your autorship of course). Did
> > I forget to CC you ?
> 
> Just tested that version exactly and it's perfect (and less code),
> feel free to add:
> 
> Tested-by: Eric Curtin <ecurtin at redhat.com>
> 
> to that one.

Thank you. I'll push the patch shortly.

> Sorry, I copy pasted code snippets from there to test, because I
> couldn't seem to find it on patchwork. Maybe it's about time I
> switched from using just git-send-email and gmail for email. I should
> probably set up some client that makes it easier for me to download
> patches w/o patchwork.

And we also need to setup a public-inbox instance for libcamera, so that
you could use b4 to download patches from the list.

> > > Changes in v4:
> > > - Change commit message to say top left
> > > - Spaces to tabs
> > >
> > > 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.
> > >
> > > 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 | 45 +++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 36 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > > index 7add81a6..7904bca8 100644
> > > --- a/src/cam/kms_sink.cpp
> > > +++ b/src/cam/kms_sink.cpp
> > > @@ -11,6 +11,7 @@
> > >  #include <algorithm>
> > >  #include <assert.h>
> > >  #include <iostream>
> > > +#include <limits.h>
> > >  #include <memory>
> > >  #include <stdint.h>
> > >  #include <string.h>
> > > @@ -112,6 +113,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> > >
> > >       const libcamera::StreamConfiguration &cfg = config.at(0);
> > >
> > > +     /* Find the best mode for the stream size. */
> > >       const std::vector<DRM::Mode> &modes = connector_->modes();
> > >       const auto iter = std::find_if(modes.begin(), modes.end(),
> > >                                      [&](const DRM::Mode &mode) {
> > > @@ -120,6 +122,32 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> > >                                      });
> > >       if (iter == modes.end()) {
> >
> > I think you could drop this search, as the iteration over the modes
> > below should be enough. Would you mind testing the v5 I posted ?
> >
> > >               std::cerr << "No mode matching " << cfg.size << std::endl;
> > > +
> > > +             unsigned int cfgArea = cfg.size.width * cfg.size.height;
> > > +             unsigned int bestDistance = UINT_MAX;
> > > +
> > > +             for (const DRM::Mode &mode : modes) {
> > > +                     unsigned int modeArea = mode.hdisplay * mode.vdisplay;
> > > +                     unsigned int distance = modeArea > cfgArea
> > > +                                                     ? modeArea - cfgArea
> > > +                                                     : cfgArea - modeArea;
> > > +
> > > +                     if (distance < bestDistance) {
> > > +                             mode_ = &mode;
> > > +                             bestDistance = distance;
> > > +
> > > +                             /*
> > > +                              * If the sizes match exactly, there will be no better
> > > +                              * match.
> > > +                              */
> > > +                             if (distance == 0)
> > > +                                     break;
> > > +                     }
> > > +             }
> > > +     }
> > > +
> > > +     if (!mode_) {
> > > +             std::cerr << "No modes\n";
> > >               return -EINVAL;
> > >       }
> > >
> > > @@ -127,7 +155,6 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> > >       if (ret < 0)
> > >               return ret;
> > >
> > > -     mode_ = &*iter;
> > >       size_ = cfg.size;
> > >       stride_ = cfg.stride;
> > >
> > > @@ -199,10 +226,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > >               return ret;
> > >       }
> > >
> > > -     std::cout
> > > -             << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id()
> > > -             << ", connector " << connector_->name()
> > > -             << " (" << connector_->id() << ")" << std::endl;
> > > +     std::cout << "Using KMS plane " << plane_->id() << ", CRTC "
> > > +               << crtc_->id() << ", connector " << connector_->name() << " ("
> > > +               << connector_->id() << "), mode " << mode_->hdisplay << "x"
> > > +               << mode_->vdisplay << "@" << mode_->vrefresh << std::endl;
> > >
> > >       return 0;
> > >  }
> > > @@ -295,12 +322,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);
> > >
> > >               flags |= DRM::AtomicRequest::FlagAllowModeset;
> > >       }

-- 
Regards,

Laurent Pinchart