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

[Kieran Bingham]

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?

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.

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

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


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
>