[libcamera-devel] [PATCH v4] cam: drm: Support /dev/dri cards other than 0

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Mon, Jun 06, 2022 at 03:27:45PM +0100, Eric Curtin wrote:
> Existing code is hardcoded to card0. Since recent fedora upgrades, we
> have noticed on more than one machine that card1 is present as the
> lowest numbered device, could theoretically be higher. This technique
> tries every file starting with card and continue only when we have
> successfully opened one. These devices with card1 as the lowest device
> were simply failing when they do not see a /dev/dri/card0 file present.
> 
> Reported-by: Ian Mullins <imullins at redhat.com>
> Tested-by: Ian Mullins <imullins at redhat.com>
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> 
> ---
> Changes in v4:
> - added Tested-by tag
> - std::stringified things
> - changed if conditions to reduce indentations
> - constexpr sizes now don't include null terminator
> - just return -ENOENT if no device was successfully opened. 
> 
> Changes in v3:
> - switch back to memcpy, strncpy causing false compiler issue
> 
> Changes in v2:
> - return if no valid card found, or directory could not be opened etc.
> - memcpy to strncpy for safety
> - only adjust the numerical bytes for each iteration of loop since
>   /dev/dri/card won't change
> - initialize ret to zero
> ---
>  src/cam/drm.cpp | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index 42c5a3b1..d05b7f5f 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -8,6 +8,7 @@
>  #include "drm.h"
>  
>  #include <algorithm>
> +#include <dirent.h>
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <iostream>
> @@ -393,9 +394,15 @@ Device::~Device()
>  
>  int Device::init()
>  {
> -	constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255");
> -	char name[NODE_NAME_MAX];
> -	int ret;
> +	constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/") - 1;
> +	constexpr size_t PRE_NODE_NAME_MAX = sizeof("card") - 1;
> +	constexpr size_t POST_NODE_NAME_MAX = sizeof("255") - 1;
> +	constexpr size_t NODE_NAME_MAX =
> +		DIR_NAME_MAX + PRE_NODE_NAME_MAX + POST_NODE_NAME_MAX;
> +	std::string name;
> +	name.reserve(NODE_NAME_MAX);
> +	name = "/dev/dri/";
> +	int ret = 0;

This ret need to be initialized to 0 ?

>  
>  	/*
>  	 * Open the first DRM/KMS device. The libdrm drmOpen*() functions
> @@ -404,16 +411,37 @@ int Device::init()
>  	 * from drmOpen() is of no practical use as any modern system will
>  	 * handle that through udev or an equivalent component.
>  	 */
> -	snprintf(name, sizeof(name), "/dev/dri/card%u", 0);
> -	fd_ = open(name, O_RDWR | O_CLOEXEC);
> -	if (fd_ < 0) {
> +	DIR *folder = opendir(name.c_str());
> +	if (!folder) {
>  		ret = -errno;
> -		std::cerr
> -			<< "Failed to open DRM/KMS device " << name << ": "
> -			<< strerror(-ret) << std::endl;
> +		std::cerr << "Failed to open " << name
> +			  << " directory: " << strerror(-ret) << std::endl;
>  		return ret;
>  	}
>  
> +	name += "card";
> +	for (struct dirent *res; (res = readdir(folder));) {
> +		if (!strncmp(res->d_name, "card", 4)) {
> +			name += res->d_name + PRE_NODE_NAME_MAX;
> +			fd_ = open(name.c_str(), O_RDWR | O_CLOEXEC);
> +			if (fd_ >= 0) {
> +				break;
> +			}
> +
> +			ret = -errno;
> +			std::cerr << "Failed to open DRM/KMS device " << name
> +				  << ": " << strerror(-ret) << std::endl;
> +			name.resize(DIR_NAME_MAX + PRE_NODE_NAME_MAX);
> +		}
> +	}

Let's continue simplification of string processing :-)

	const std::string dirName{ "/dev/dri/" };
	DIR *folder = opendir(dirName.c_str());
	if (!folder) {
		ret = -errno;
		std::cerr << "Failed to open " << dirName
			  << " directory: " << strerror(-ret) << std::endl;
		return ret;
	}

	for (struct dirent *res; (res = readdir(folder)); ) {
		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)
			break;

		ret = -errno;
		std::cerr << "Failed to open DRM/KMS device " << devName
			  << ": " << strerror(-ret) << std::endl;
	}

And you can drop the constexpr size_t above. Is it less efficient ?
Probably, but this is marginal, and not in a hot path.

> +
> +	closedir(folder);
> +
> +	if (fd_ < 0) {
> +		std::cerr << "Failed to open any DRM/KMS device" << std::endl;
> +		return -ENOENT;
> +	}
> +
>  	/*
>  	 * Enable the atomic APIs. This also automatically enables the
>  	 * universal planes API.

-- 
Regards,

Laurent Pinchart