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

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Thu, Jun 02, 2022 at 11:00:17AM +0100, Eric Curtin via libcamera-devel 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.

Out of curiosity, why is that ?

> 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>
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> 
> ---
> 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, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index 42c5a3b1..7975a502 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,13 @@ 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/");
> +	constexpr size_t PRE_NODE_NAME_MAX = sizeof("card");
> +	constexpr size_t POST_NODE_NAME_MAX = sizeof("255");
> +	constexpr size_t NODE_NAME_MAX =
> +		DIR_NAME_MAX + PRE_NODE_NAME_MAX + POST_NODE_NAME_MAX - 2;
> +	char name[NODE_NAME_MAX] = "/dev/dri/";

This looks pretty error-prone compared to std::string, let's use the C++
API here instead. This isn't performance-sensitive code.

std::filesystem could be nice too, to avoid the manual closedir(), but
that's a lesser concern.

> +	int ret = 0;
>  
>  	/*
>  	 * Open the first DRM/KMS device. The libdrm drmOpen*() functions
> @@ -404,14 +409,35 @@ 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);
> +	DIR *folder = opendir(name);
> +	memcpy(name + DIR_NAME_MAX - 1, "card", PRE_NODE_NAME_MAX);
> +	if (folder) {

You can turn this into a

	if (!folder) {
		...
	}

to lower the indentation below.

> +		for (struct dirent *res; (res = readdir(folder));) {
> +			if (!strncmp(res->d_name, "card", 4)) {

Same thing here,

		if (strncmp(...))
			continue;

(or rather the equivalent after switching to std::string)

> +				memcpy(name + DIR_NAME_MAX +
> +						PRE_NODE_NAME_MAX - 2,
> +					res->d_name + PRE_NODE_NAME_MAX - 1,
> +					POST_NODE_NAME_MAX);
> +				fd_ = open(name, O_RDWR | O_CLOEXEC);
> +				if (fd_ < 0) {
> +					ret = -errno;
> +					std::cerr
> +						<< "Failed to open DRM/KMS device "
> +						<< name << ": "
> +						<< strerror(-ret) << std::endl;
> +					continue;
> +				}
> +
> +				break;
> +			}
> +		}
> +
> +		closedir(folder);
> +	}
> +
>  	if (fd_ < 0) {
> -		ret = -errno;
> -		std::cerr
> -			<< "Failed to open DRM/KMS device " << name << ": "
> -			<< strerror(-ret) << std::endl;
> -		return ret;
> +		std::cerr << "Unable to open any DRM/KMS device" << std::endl;
> +		return ret ? ret : -ENOENT;

I don't think it's very meaningful to pick the last error here, you can
just return -ENOENT

>  	}

It could be interesting to try all available DRM devices to find the
requested connector, as well as allowing selection of a particular
device on the command line. That's an improvement candidate for the
future, if a need arises.

>  
>  	/*

-- 
Regards,

Laurent Pinchart