[libcamera-devel] [PATCH] cam: drm: Skip DRM devices not capable of supporting the atomic API

[Jacopo Mondi]

Hi Laurent

On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The DRM helper picks the first DRM card that it can open. On platforms
> that have a standalone GPU, this risks selecting a device corresponding
> to the GPU instead of the display controller. Fix this by skipping
> devices that don't support the KMS atomic API. Some legacy display
> controllers would be skipped as well, but libcamera doesn't run on those
> systems anyway, and the DRM helper doesn't support the legacy KMS
> modeset API in any case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/cam/drm.cpp | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index b0602c942853..b2a6b0bba9e8 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -430,7 +430,8 @@ int Device::init()
>  int Device::openCard()
>  {
>  	const std::string dirName = "/dev/dri/";
> -	int ret = -ENOENT;
> +	bool found = false;
> +	int ret;
>
>  	/*
>  	 * Open the first DRM/KMS device beginning with /dev/dri/card. The
> @@ -449,24 +450,38 @@ int Device::openCard()
>  	}
>
>  	for (struct dirent *res; (res = readdir(folder));) {
> +		uint64_t cap;
> +
>  		if (strncmp(res->d_name, "card", 4))
>  			continue;
>
>  		const std::string devName = dirName + res->d_name;
>  		fd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);
> -		if (fd_ >= 0) {
> -			ret = 0;
> -			break;
> +		if (fd_ < 0) {
> +			ret = -errno;

As we don't propagate errors up, can't you use errno directly ?

> +			std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> +				  << strerror(-ret) << std::endl;
> +			continue;
>  		}
>
> -		ret = -errno;
> -		std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> -			  << strerror(-ret) << std::endl;
> +		/*
> +		 * Skip devices that don't support the atomic API, to avoid
> +		 * selecting a DRM device corresponding to a GPU.
> +		 */
> +		ret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);

This is the only reference I found of drmGetCap:
https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/

Can it return a negative value ?


> +		if (ret < 0) {
> +			drmClose(fd_);
> +			fd_ = -1;
> +			continue;
> +		}
> +
> +		found = true;
> +		break;
>  	}
>
>  	closedir(folder);
>
> -	return ret;
> +	return found ? 0 : -ENOENT;
>  }
>
>  int Device::getResources()
> --
> Regards,
>
> Laurent Pinchart
>