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

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Tue, Mar 29, 2022 at 02:02:21PM +0100, Eric Curtin via libcamera-devel 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 right 0, 0 corner.

Isn't that the top-left corner ?

> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> ---
> Changes in v2:
> - Tested and support drawing from negative pixel range
>   kernel parameter (video=960x540 at 60) was useful here
> 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.
> ---
>  src/cam/kms_sink.cpp | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index da579846..5847c4e1 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -119,17 +119,21 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>  						      mode.vdisplay == cfg.size.height;
>  				       });
>  	if (iter == modes.end()) {
> -		std::cerr
> -			<< "No mode matching " << cfg.size.toString()
> -			<< std::endl;
> -		return -EINVAL;
> +                if (modes.empty()) {

There are spaces used for indentation.

> +        		std::cerr << "No modes\n";
> +        		return -EINVAL;
> +                }
> +
> +                mode_ = &modes[0];
>  	}
> +        else {
> +                mode_ = &*iter;
> +        }
>  
>  	int ret = configurePipeline(cfg.pixelFormat);
>  	if (ret < 0)
>  		return ret;
>  
> -	mode_ = &*iter;
>  	size_ = cfg.size;
>  	stride_ = cfg.stride;
>  
> @@ -297,12 +301,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);

Not all drivers will support this. That's fine, it's not a regression as
they wouldn't work anyway, but I'm annoyed that things will fail only
when trying to display the first frame, and without a clear error
message :-(

Ideally we should try to probe the device to see what it supports (which
could include scaling) and then pick the best option in
KMSSink::configure().

>  
>  		flags |= DRM::AtomicRequest::FlagAllowModeset;
>  	}

-- 
Regards,

Laurent Pinchart