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

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Fri, Jun 17, 2022 at 11:00:44AM +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 v6:
> - Spaces to tabs
> - Remove a brace for single line if statement
> 
> Changes in v5:
> - Split into openCard function
> - ret initialized to -ENOENT, in case directory is open with no card*
> - Add another string to simplify
> 
> 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 | 59 ++++++++++++++++++++++++++++++++++++-------------
>  src/cam/drm.h   |  2 +-
>  2 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index 42c5a3b1..41fd042b 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>
> @@ -391,26 +392,54 @@ Device::~Device()
>  		drmClose(fd_);
>  }
>  
> -int Device::init()
> +int Device::openCard()
>  {
> -	constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255");
> -	char name[NODE_NAME_MAX];
> -	int ret;
> +	const std::string dirName = "/dev/dri/";
> +	int ret = -ENOENT;
>  
>  	/*
> -	 * Open the first DRM/KMS device. The libdrm drmOpen*() functions
> -	 * require either a module name or a bus ID, which we don't have, so
> -	 * bypass them. The automatic module loading and device node creation
> -	 * from drmOpen() is of no practical use as any modern system will
> -	 * handle that through udev or an equivalent component.
> +	 * Open the first DRM/KMS device beginning with /dev/dri/card. The
> +	 * libdrm drmOpen*() functions require either a module name or a bus ID,
> +	 * which we don't have, so bypass them. The automatic module loading and
> +	 * device node creation 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(dirName.c_str());
> +	if (!folder) {
>  		ret = -errno;
> -		std::cerr
> -			<< "Failed to open DRM/KMS device " << name << ": "
> -			<< strerror(-ret) << std::endl;
> +		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) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		ret = -errno;
> +		std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> +			  << strerror(-ret) << std::endl;
> +	}
> +
> +	closedir(folder);
> +
> +	return ret;
> +}
> +
> +int Device::init()
> +{
> +	int ret = openCard();
> +	if (ret < 0) {
> +		std::cerr << "Failed to open any DRM/KMS device: "
> +			  << strerror(-ret) << std::endl;
>  		return ret;
>  	}
>  
> diff --git a/src/cam/drm.h b/src/cam/drm.h
> index de57e445..1817d053 100644
> --- a/src/cam/drm.h
> +++ b/src/cam/drm.h
> @@ -312,7 +312,7 @@ private:
>  	Device &operator=(const Device &&) = delete;
>  
>  	int getResources();
> -
> +	int openCard();

Just one small nit, I'd move this before getResources() to match the
order in the .cpp file. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

No need to resend, I'll fix this when applying.

>  	void drmEvent();
>  	static void pageFlipComplete(int fd, unsigned int sequence,
>  				     unsigned int tv_sec, unsigned int tv_usec,
> 

-- 
Regards,

Laurent Pinchart