[libcamera-devel] [PATCH v5] cam: kms_sink: Remove limitation that camera and display must match

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Mon, Jun 20, 2022 at 12:52:39PM +0100, Eric Curtin wrote:
> There is a limitation that requires input and output to be pixel
> for pixel identical in terms of height and width. Remove this
> limitation to enable more hardware that doesn't match. Just start
> drawing from top left 0, 0 corner. Try and pick the mode closest to the
> stream size.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Tested-by: Eric Curtin <ecurtin at redhat.com>
> ---
> Changes in v5:
> - Tested Laurents suggested change successfully
> - Added to commit message about trying to pick closest

I've also sent a v5, on Sunday (retaining your autorship of course). Did
I forget to CC you ?

> Changes in v4:
> - Change commit message to say top left
> - Spaces to tabs
> 
> Changes in v3:
> - Much simplified version of the patch where we just attempt to
>   draw from point 0, 0. Only in the case where we do not find a
>   matching mode. Can expand to do centralization, scaling, etc.
>   in further patches if needs be.
> 
> Changes in v2:
> - Tested and support drawing from negative pixel range
>   kernel parameter (video=960x540 at 60) was useful here
> ---
>  src/cam/kms_sink.cpp | 45 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index 7add81a6..7904bca8 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -11,6 +11,7 @@
>  #include <algorithm>
>  #include <assert.h>
>  #include <iostream>
> +#include <limits.h>
>  #include <memory>
>  #include <stdint.h>
>  #include <string.h>
> @@ -112,6 +113,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>  
>  	const libcamera::StreamConfiguration &cfg = config.at(0);
>  
> +	/* Find the best mode for the stream size. */
>  	const std::vector<DRM::Mode> &modes = connector_->modes();
>  	const auto iter = std::find_if(modes.begin(), modes.end(),
>  				       [&](const DRM::Mode &mode) {
> @@ -120,6 +122,32 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>  				       });
>  	if (iter == modes.end()) {

I think you could drop this search, as the iteration over the modes
below should be enough. Would you mind testing the v5 I posted ?

>  		std::cerr << "No mode matching " << cfg.size << std::endl;
> +
> +		unsigned int cfgArea = cfg.size.width * cfg.size.height;
> +		unsigned int bestDistance = UINT_MAX;
> +
> +		for (const DRM::Mode &mode : modes) {
> +			unsigned int modeArea = mode.hdisplay * mode.vdisplay;
> +			unsigned int distance = modeArea > cfgArea
> +							? modeArea - cfgArea
> +							: cfgArea - modeArea;
> +
> +			if (distance < bestDistance) {
> +				mode_ = &mode;
> +				bestDistance = distance;
> +
> +				/*
> +				 * If the sizes match exactly, there will be no better
> +				 * match.
> +				 */
> +				if (distance == 0)
> +					break;
> +			}
> +		}
> +	}
> +
> +	if (!mode_) {
> +		std::cerr << "No modes\n";
>  		return -EINVAL;
>  	}
>  
> @@ -127,7 +155,6 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>  	if (ret < 0)
>  		return ret;
>  
> -	mode_ = &*iter;
>  	size_ = cfg.size;
>  	stride_ = cfg.stride;
>  
> @@ -199,10 +226,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
>  		return ret;
>  	}
>  
> -	std::cout
> -		<< "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id()
> -		<< ", connector " << connector_->name()
> -		<< " (" << connector_->id() << ")" << std::endl;
> +	std::cout << "Using KMS plane " << plane_->id() << ", CRTC "
> +		  << crtc_->id() << ", connector " << connector_->name() << " ("
> +		  << connector_->id() << "), mode " << mode_->hdisplay << "x"
> +		  << mode_->vdisplay << "@" << mode_->vrefresh << std::endl;
>  
>  	return 0;
>  }
> @@ -295,12 +322,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
>  		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
>  		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
>  		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> -		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> -		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> +		drmRequest->addProperty(plane_, "SRC_W", size_.width << 16);
> +		drmRequest->addProperty(plane_, "SRC_H", size_.height << 16);
>  		drmRequest->addProperty(plane_, "CRTC_X", 0);
>  		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> -		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> -		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> +		drmRequest->addProperty(plane_, "CRTC_W", size_.width);
> +		drmRequest->addProperty(plane_, "CRTC_H", size_.height);
>  
>  		flags |= DRM::AtomicRequest::FlagAllowModeset;
>  	}

-- 
Regards,

Laurent Pinchart