[libcamera-devel] [PATCH v2] cam: drm: Skip DRM devices not capable of mode setting

[Jacopo Mondi]

Hi Laurent,

On Wed, Sep 28, 2022 at 01:27:43PM +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 mode setting API. Some legacy display
> controllers would be skipped as well, but libcamera doesn't run on those
> systems anyway.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v1:
>
> - Use DRM_CAP_DUMB_BUFFER instead of DRM_CLIENT_CAP_ATOMIC
> ---
>  src/cam/drm.cpp | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index b0602c942853..3bb950fd157a 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,42 @@ 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;
> +			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 modeset API, to avoid
> +		 * selecting a DRM device corresponding to a GPU. There is no
> +		 * modeset capability, but the kernel returns an error for most
> +		 * caps if mode setting isn't support but the driver. The

s/but/by ?

> +		 * DRM_CAP_DUMB_BUFFER capability is one of those, other would
> +		 * do as well. The capability value itself isn't relevant.

That's a really awful API :)

For this patch
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j


> +		 */
> +		ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap);
> +		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
>