[libcamera-devel,v6] cam: drm: Support /dev/dri cards other than 0
diff mbox series

Message ID 20220617100043.8753-1-ecurtin@redhat.com
State Accepted
Headers show
Series
  • [libcamera-devel,v6] cam: drm: Support /dev/dri cards other than 0
Related show

Commit Message

Eric Curtin June 17, 2022, 10 a.m. UTC 
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 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(-)

Comments

Eric Curtin June 20, 2022, 8:54 a.m. UTC | #1 
On Sat, 18 Jun 2022 at 21:06, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@redhat.com>
> > Tested-by: Ian Mullins <imullins@redhat.com>
> > Signed-off-by: Eric Curtin <ecurtin@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@ideasonboard.com>
>
> No need to resend, I'll fix this when applying.

Ok thanks Laurent.

>
> >       void drmEvent();
> >       static void pageFlipComplete(int fd, unsigned int sequence,
> >                                    unsigned int tv_sec, unsigned int tv_usec,
> >
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

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();
 	void drmEvent();
 	static void pageFlipComplete(int fd, unsigned int sequence,
 				     unsigned int tv_sec, unsigned int tv_usec,