[libcamera-devel] [PATCH] cam: kms_sink: Once you have found a valid crtc break out of loops
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Feb 2 10:32:29 CET 2022
Hi Eric,
Quoting Eric Curtin (2022-02-01 22:16:35)
> 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.
Ok, well a fixes tag is probably enough then.
> 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.
Oh - yes please, could you add that! Then I would actually be able to
test things :-)
I think it's absolutely fine to not scale, and simply allow the image to
be presented if it fits. I'd even say if the image is bigger than the
display it would be nice if it still got displayed, centrally and simply
cropped by the display output size.
That would be far more useful than requiring an exact match IMO.
> 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".
Aha! Thankyou, I wasn't aware that you're actually building an
application here.
Can I add this to my list of applications supported by libcamera?
What are the goals of twincam? ... Just run and display cameras without
a window manager/GUI? (That sounds like exactly something I've been
after/wanting to make)
>
> >
> > > 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
I would expect that to know we use tabs not spaces... hrmmm
We have a script that handles formatting called ./utils/checkstyle.py,
and that can be enabled as a post-commit hook.
cp ./utils/hooks/post-commit .git/hooks/
> >
> > > 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
> > >
> >
>
More information about the libcamera-devel
mailing list