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

[Eric Curtin]

On Mon, 20 Jun 2022 at 13:00, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> 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.

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.


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