[libcamera-devel] [PATCH v3] cam: kms-sink: Once we have found a suitable pipeline to select, return

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Mon, Feb 07, 2022 at 03:01:36PM +0000, Eric Curtin wrote:
> The break only breaks out of the innermost loop which is not the most
> optimal. It also breaks cam on my machines.

I'd expand this a bit.

cam: kms_sink: Use the first suitable pipeline found

When searching for a suitable pipeline, we mistakenly only break from
the inner loop. This results in the last suitable output being selected.
Pick the first one instead.

> Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration")
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> ---
> 
> Changes in v3:
> - Add "cam: kms_sink:" tag
> 
> Changes in v2:
> - Change function name to selectPipeline
> - Return int rather than pointer
> - Formatting
> - Mention in commit message that it fixes a bug on my machine also
> 
>  src/cam/kms_sink.cpp | 18 ++++++++++++------
>  src/cam/kms_sink.h   |  1 +
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index d30fba78..973cd370 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -136,7 +136,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>  	return 0;
>  }
>  
> -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> +int KMSSink::selectPipeline(const libcamera::PixelFormat &format)
>  {
>  	/*
>  	 * If the requested format has an alpha channel, also consider the X
> @@ -174,25 +174,31 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
>  					crtc_ = crtc;
>  					plane_ = plane;
>  					format_ = format;
> -					break;
> +					return 0;
>  				}
>  
>  				if (plane->supportsFormat(xFormat)) {
>  					crtc_ = crtc;
>  					plane_ = plane;
>  					format_ = xFormat;
> -					break;
> +					return 0;
>  				}
>  			}
>  		}
>  	}
>  
> -	if (!crtc_) {
> +	return -EPIPE;
> +}
> +
> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> +{
> +	const int ret = selectPipeline(format);
> +	if (ret) {
>  		std::cerr
>  			<< "Unable to find display pipeline for format "
>  			<< format.toString() << std::endl;
>  
> -		return -EPIPE;
> +		return ret;
>  	}
>  
>  	std::cout
> @@ -200,7 +206,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
>  		<< ", connector " << connector_->name()
>  		<< " (" << connector_->id() << ")" << std::endl;
>  
> -	return 0;
> +	return ret;

This change isn't needed.

With these changes,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

If you agree with those changes there's no need to resubmit, I'll make
those small modifications before pushing.

>  }
>  
>  int KMSSink::start()
> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> index 1e4290ad..4a0a872c 100644
> --- a/src/cam/kms_sink.h
> +++ b/src/cam/kms_sink.h
> @@ -47,6 +47,7 @@ private:
>  		libcamera::Request *camRequest_;
>  	};
>  
> +	int selectPipeline(const libcamera::PixelFormat &format);
>  	int configurePipeline(const libcamera::PixelFormat &format);
>  	void requestComplete(DRM::AtomicRequest *request);
>  

-- 
Regards,

Laurent Pinchart