[libcamera-devel] [PATCH] cam: kms_sink: Once you have found a valid crtc break out of loops

[Eric Curtin]

On Mon, 31 Jan 2022 at 23:52, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Quoting Eric Curtin (2021-12-02 10:23:55)
> > Once we have found a suitable plane and CRTC to auto-select, break out
> > of all loops, not just the inner-most loop.
> >
> > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration")
>
> Is this an optimisation, or a bug fix ? Does this make a specific change
> in the outcome?

For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on
my hardware, unless we break out of the loop the first time around. But probably
something else at play here, since as you say, this should just be an
optimization.

It was briefly discussed on another email thread in libcamera-devel months ago.

>
> The fixes tag implies it does, so it might be nice to hear here what
> happens without this patch, but it sounds like it's a worthwhile update.
>
> I haven't been able to test capture direct to DRM yet, as I haven't
> seemed to find a suitable match for camera and display, but I hope to
> try this again soon.
>
> Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it
> can't match configurations. Oh well, I'll try something else later.
> It's a shame we can't handle this with strides to be able to still
> display the image as it's smaller.

I actually removed this limitation on the version we've been writing,
which is 90%
"cam" reference implementation stripped and repurposed for red hat's needs, it's
gonna be executed by plymouth.

So the commits on December 7th here:

https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp

allow smaller resolution camera outputs to run on larger displays, so a 1080p
image such as in your case should be centralized and take up 1/4 of your 4k
screen. It's not scaling, but it was an easy way to remove a limitation for me
that enabled me to test on more hardware. Could be easily added to "cam"
also.

I've tested "twincam" on two machines, chances are, if you build and run it
on your machine on a suitable tty, it might "just work".

>
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> >  src/cam/kms_sink.cpp | 27 ++++++++++++++++-----------
> >  src/cam/kms_sink.h   |  1 +
> >  2 files changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index d30fba78..44a0f07b 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> >         return 0;
> >  }
> >
> > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format)
> >  {
> >         /*
> > -        * If the requested format has an alpha channel, also consider the X
> > -        * variant.
> > -        */
> > +         * If the requested format has an alpha channel, also consider the X
> > +         * variant.
> > +         */
>
> I think those changes broke the indentation... Looks like the tabs are
> replaced by spaces?

Unintentional, I'll fix. But I think I auto-formatted using the
checked in .clang-format file:

https://github.com/kbingham/libcamera/blob/master/.clang-format

>
> >         libcamera::PixelFormat xFormat;
> >
> >         switch (format) {
> > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> >         }
> >
> >         /*
> > -        * Find a CRTC and plane suitable for the request format and the
> > -        * connector at the end of the pipeline. Restrict the search to primary
> > -        * planes for now.
> > -        */
> > +         * Find a CRTC and plane suitable for the request format and the
> > +         * connector at the end of the pipeline. Restrict the search to primary
> > +         * planes for now.
> > +         */
>
> Same here,
>
> But splitting this to a new function looks reasonable to me.
>
> So with the comments repaired, which could be done while applying too:

I can resend patch. Thanks for reviewing.

>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> >         for (const DRM::Encoder *encoder : connector_->encoders()) {
> >                 for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> >                         for (const DRM::Plane *plane : crtc->planes()) {
> > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> >                                         crtc_ = crtc;
> >                                         plane_ = plane;
> >                                         format_ = format;
> > -                                       break;
> > +                                       return crtc;
> >                                 }
> >
> >                                 if (plane->supportsFormat(xFormat)) {
> >                                         crtc_ = crtc;
> >                                         plane_ = plane;
> >                                         format_ = xFormat;
> > -                                       break;
> > +                                       return crtc;
> >                                 }
> >                         }
> >                 }
> >         }
> >
> > -       if (!crtc_) {
> > +       return nullptr;
> > +}
> > +
> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > +{
> > +       if (!findCrtc(format)) {
> >                 std::cerr
> >                         << "Unable to find display pipeline for format "
> >                         << format.toString() << std::endl;
> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> > index 1e4290ad..a8a29399 100644
> > --- a/src/cam/kms_sink.h
> > +++ b/src/cam/kms_sink.h
> > @@ -47,6 +47,7 @@ private:
> >                 libcamera::Request *camRequest_;
> >         };
> >
> > +       const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format);
> >         int configurePipeline(const libcamera::PixelFormat &format);
> >         void requestComplete(DRM::AtomicRequest *request);
> >
> > --
> > 2.33.1
> >
>