[libcamera-devel] [PATCH] cam: kms_sink: Once you have found a valid crtc break out of loops
Eric Curtin
ecurtin at redhat.com
Wed Feb 2 18:04:10 CET 2022
On Wed, 2 Feb 2022 at 14:35, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote:
> > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote:
> > > 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.
>
> With the existing code base, we pick the last suitable CRTC, while with
> this patch, we'll pick the first one. Breaking from the loop once a
> suitable CRTC is found is a good idea, but if it fixes an issue with
> your setup, it means that at least one of the CRTCs that are considered
> suitable is actually not. We may thus break your platform inadvertently
> in the future when reworking this code. It would be good to go to the
> bottom of this, and fix the CRTC selection to not rely on luck. It can
> be done separately from this patch.
>
> > 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)
> > > > {
>
> [snip]
>
> > > > @@ -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;
>
> The function returns a DRM::Crtc and is called findCrtc(), but it also
> finds a plane, selects a formatn and store them all in member variables.
> That's a bit confusing. I would rename the function to findPipeline()
> (for lack of a better name) and make it return an integer (0 on success,
> -EPIPE on error) or a std::tuple<DRM::Crtc *, DRM::Plane *,
> libcamera::PixelFormat>. The former is likely easier.
Yup, I was thinking of changing the name in the next version of the
patch funnily enough, because you're right, it does more than select
ctrc. I'll do the int return, just to keep it simple.
>
> Another option is
>
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index d30fba783c96..aaffa372f050 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> crtc_ = crtc;
> plane_ = plane;
> format_ = format;
> - break;
> + goto found;
> }
>
> if (plane->supportsFormat(xFormat)) {
> crtc_ = crtc;
> plane_ = plane;
> format_ = xFormat;
> - break;
> + goto found;
> }
> }
> }
> @@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> return -EPIPE;
> }
>
> +found:
> std::cout
> << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id()
> << ", connector " << connector_->name()
>
> which would be even simpler. It would cause issues if we were to add
> local variables after the goto statements and before the label, but we
> can worry about that later.
Yeah I actually started with this, changed it just in case there were some
goto haters reviewing this, writing a function with returns is how to write
a goto without writing a goto. I'll write the function just to keep the
functions a little smaller.
>
> > > > +}
> > > > +
> > > > +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);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list