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

16178 diff mbox series

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@redhat.com>
Tested-by: Ian Mullins <imullins@redhat.com>
Signed-off-by: Eric Curtin <ecurtin@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(-)

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@redhat.com>
> Tested-by: Ian Mullins <imullins@redhat.com>
> Signed-off-by: Eric Curtin <ecurtin@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.