[libcamera-devel] [PATCH] cam: kms_sink: Once you have found a valid crtc break out of loops
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 3 00:06:22 CET 2022
Hi Eric,
On Wed, Feb 02, 2022 at 05:04:10PM +0000, Eric Curtin wrote:
> On Wed, 2 Feb 2022 at 14:35, Laurent Pinchart wrote:
> > 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.
selectPipeline() could be a better name than findPipeline() as the
function selects the components, it doesn't just look them up.
> > 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.
OK.
> > > > > +}
> > > > > +
> > > > > +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