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

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

Commit Message

Eric Curtin June 16, 2022, 9:45 p.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 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 | 62 ++++++++++++++++++++++++++++++++++++-------------
 src/cam/drm.h   |  2 +-
 2 files changed, 47 insertions(+), 17 deletions(-)

Comments

Eric Curtin June 17, 2022, 8:27 a.m. UTC | #1 
On Fri, 17 Jun 2022 at 07:53, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Eric,
>
> On Thu, Jun 16, 2022 at 10:45:52PM +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 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 | 62 ++++++++++++++++++++++++++++++++++++-------------
> >  src/cam/drm.h   |  2 +-
> >  2 files changed, 47 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index 42c5a3b1..39c19aa8 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,55 @@ 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.
> > -      */
> > -     snprintf(name, sizeof(name), "/dev/dri/card%u", 0);
> > -     fd_ = open(name, O_RDWR | O_CLOEXEC);
> > -     if (fd_ < 0) {
> > +         * 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.
> > +         */
>
> Weird indentation ?

Yeah you are right, it appeared on my text editor the same as the old
indentation. It's spaces instead of tabs here. It's a pity
clang-format doesn't seem to fix this, nor does checkstyle pick this
up. Have to do it manually, maybe it's because the tools are ignoring
comments indentation, since everything between /* */ is just
beautifying a comment.

>
> > +     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;
> > +             }
>
> No need for {}. Does checkstyle warn you ?

Nope it doesn't, but I can fix manually.

>
> > +
> > +             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);
>
> Looks like fd_ is still not initialized. Maybe it's not an issue
> anymore as you now return earlier in init(), but it would have made the
> code simpler, avoiding you to initialize ret as you could:
>
>
>         fd_ = -1;
>         for (res; res = readdir(folder)) {
>                 if (res->d_name != "card)
>                         continue;
>
>                 fd_ = open(..);
>                 if (fd != -1)
>                         break;
>
>                 std::cerr << "Failed " ... << strerror(errno);
>         }
>
>         return fd_ == -1 ? -ENOENT : 0;
>
> Up to you

I could initialize fd_ here also, but it would be superfluous if we
added here (or become superfluous in the constructor if we added
whatever way you want to look at it), I think it's better to have a
single source of truth, if we initialize here, the construct
initialization becomes irrelevant, it may as well be -12312. When a
KMSSink constructor gets called, a Device constructor gets called
(setting fd_ to -1) and dev_.init() is called in the first line of the
KMSSink constructor. In this code, you can pretty much treat
Device::init() and the constructor as one, they get called in quick
succession, but init() gives you a return code which is handy for
bailing out early.

>
> > +
> > +     return ret;
> > +}
> > +
> > +int Device::init()
> > +{
> > +     int ret = openCard();
> > +     if (ret < 0) {
> > +             std::cerr << "Failed to open any DRM/KMS device: "
> > +                       << strerror(-ret) << std::endl;
>
> Duplicated error message ? The openCard() is already verbose enough
> maybe ?

The main reason this error message is here, if there was no message
here. in the case you successfully open /dev/dri/, but there is no
entry in that directory beginning with card*. If we remove this, there
would be no error message in this case.

>
> >               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,
> > --
> > 2.35.3
> >
>

Patch
diff mbox series 

diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
index 42c5a3b1..39c19aa8 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,55 @@  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.
-	 */
-	snprintf(name, sizeof(name), "/dev/dri/card%u", 0);
-	fd_ = open(name, O_RDWR | O_CLOEXEC);
-	if (fd_ < 0) {
+         * 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.
+         */
+	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,