| Message ID | 20220601152345.37753-1-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 10 deletions(-) > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > int Device::init() > { > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > - char name[NODE_NAME_MAX]; > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > + char name[NODE_NAME_MAX] = "/dev/dri/"; > int ret; > > /* > @@ -404,14 +407,28 @@ 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) { > - ret = -errno; > - std::cerr > - << "Failed to open DRM/KMS device " << name << ": " > - << strerror(-ret) << std::endl; > - return ret; > + DIR *folder = opendir(name); > + if (folder) { > + for (struct dirent *res; (res = readdir(folder));) { > + if (strlen(res->d_name) > 4 && I feel this might be a bit simplified, maybe using std::filesystem const std::filesystem::path dri("/dev/dri"); for (const auto &dir : std::filesystem::directory_iterator(dri)) { const std::string &direntry = dir.path().filename().string(); if (direntry.find("card") == std::string::npos) continue; fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); ... } > + !strncmp(res->d_name, "card", 4)) { > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > + BASE_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); What if no card is found ? Should fd_ be initialized and here checked ? Thanks j > } > > /* > -- > 2.35.3 >
On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Eric > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > int Device::init() > > { > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > - char name[NODE_NAME_MAX]; > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > int ret; > > > > /* > > @@ -404,14 +407,28 @@ 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) { > > - ret = -errno; > > - std::cerr > > - << "Failed to open DRM/KMS device " << name << ": " > > - << strerror(-ret) << std::endl; > > - return ret; > > + DIR *folder = opendir(name); > > + if (folder) { > > + for (struct dirent *res; (res = readdir(folder));) { > > + if (strlen(res->d_name) > 4 && > > I feel this might be a bit simplified, maybe using std::filesystem If ultimately demanded or required, I'll change to std::filesystem, a quick grep through the codebase shows that we use opendir and other similar C code in all instances except for one case in the Android code though. And I would like to keep this code lean and in C if possible. In fact in V2 I might make this even leaner and just write the bytes after /dev/dri/card when needed rather than /dev/dri/ and remove the strlen. > > const std::filesystem::path dri("/dev/dri"); > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > const std::string &direntry = dir.path().filename().string(); > > if (direntry.find("card") == std::string::npos) > continue; > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > ... > } > > > + !strncmp(res->d_name, "card", 4)) { > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > + BASE_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); > > What if no card is found ? > Should fd_ be initialized and here checked ? Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > Thanks > j > > > } > > > > /* > > -- > > 2.35.3 > > >
Hi Eric, On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote: > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Eric > > > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > > > int Device::init() > > > { > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > > - char name[NODE_NAME_MAX]; > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > int ret; > > > > > > /* > > > @@ -404,14 +407,28 @@ 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) { > > > - ret = -errno; > > > - std::cerr > > > - << "Failed to open DRM/KMS device " << name << ": " > > > - << strerror(-ret) << std::endl; > > > - return ret; > > > + DIR *folder = opendir(name); > > > + if (folder) { > > > + for (struct dirent *res; (res = readdir(folder));) { > > > + if (strlen(res->d_name) > 4 && > > > > I feel this might be a bit simplified, maybe using std::filesystem > > If ultimately demanded or required, I'll change to std::filesystem, a > quick grep through the codebase shows that we use opendir and other > similar C code in all instances except for one case in the Android > code though. And I would like to keep this code lean and in C if > possible. In fact in V2 I might make this even leaner and just write > the bytes after /dev/dri/card when needed rather than /dev/dri/ and > remove the strlen. > We have moved rather recently to C++17 for the internal code, where std::filesystem has been introduced. That's maybe why it's not that used. Up to you. However I don't find the previous version much leaner compared to the version I suggested, at least from a readability point of view. Why would you like to keep this a much as C code as possible if I may ask ? Thanks j > > > > const std::filesystem::path dri("/dev/dri"); > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > const std::string &direntry = dir.path().filename().string(); > > > > if (direntry.find("card") == std::string::npos) > > continue; > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > ... > > } > > > > > + !strncmp(res->d_name, "card", 4)) { > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > + BASE_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); > > > > What if no card is found ? > > Should fd_ be initialized and here checked ? > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > > > > Thanks > > j > > > > > } > > > > > > /* > > > -- > > > 2.35.3 > > > > > >
On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Eric, > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote: > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Eric > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > > > > > int Device::init() > > > > { > > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > > > - char name[NODE_NAME_MAX]; > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > int ret; > > > > > > > > /* > > > > @@ -404,14 +407,28 @@ 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) { > > > > - ret = -errno; > > > > - std::cerr > > > > - << "Failed to open DRM/KMS device " << name << ": " > > > > - << strerror(-ret) << std::endl; > > > > - return ret; > > > > + DIR *folder = opendir(name); > > > > + if (folder) { > > > > + for (struct dirent *res; (res = readdir(folder));) { > > > > + if (strlen(res->d_name) > 4 && > > > > > > I feel this might be a bit simplified, maybe using std::filesystem > > > > If ultimately demanded or required, I'll change to std::filesystem, a > > quick grep through the codebase shows that we use opendir and other > > similar C code in all instances except for one case in the Android > > code though. And I would like to keep this code lean and in C if > > possible. In fact in V2 I might make this even leaner and just write > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and > > remove the strlen. > > > > We have moved rather recently to C++17 for the internal code, where > std::filesystem has been introduced. That's maybe why it's not that > used. I think std::filesystem makes a lot of sense where you have to support many platforms, Linux, Windows, MacOS, etc. and the underlying implementation of the filesystem calls are different, in the multi-platform case I could understand why it doesn't make sense to maintain multiple versions for each platform. But when your only platform is Linux, I feel like it's easier to call the C code directly and you get the added benefit of knowing what kind of open calls, etc. are used. I feel you don't gain much with the std::filesystem here, it's still just a loop with a string comparison. I think it adds a little complexity here even, like when you see: dir.path().string().c_str() and you just need to pass a simple string in there. Once you start using a C++ feature you never go back, I always have that little fear also. > > Up to you. However I don't find the previous version much leaner > compared to the version I suggested, at least from a readability point > of view. Why would you like to keep this a much as C code as possible > if I may ask ? > > Thanks > j > > > > > > > const std::filesystem::path dri("/dev/dri"); > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > const std::string &direntry = dir.path().filename().string(); > > > > > > if (direntry.find("card") == std::string::npos) > > > continue; > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > > > ... > > > } > > > > > > > + !strncmp(res->d_name, "card", 4)) { > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > > + BASE_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); > > > > > > What if no card is found ? > > > Should fd_ be initialized and here checked ? > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > > > > > > > Thanks > > > j > > > > > > > } > > > > > > > > /* > > > > -- > > > > 2.35.3 > > > > > > > > > >
Hi Eric On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote: > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Eric, > > > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote: > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi Eric > > > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > --- > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > > > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > > > > > > > int Device::init() > > > > > { > > > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > > > > - char name[NODE_NAME_MAX]; > > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > > int ret; > > > > > > > > > > /* > > > > > @@ -404,14 +407,28 @@ 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) { > > > > > - ret = -errno; > > > > > - std::cerr > > > > > - << "Failed to open DRM/KMS device " << name << ": " > > > > > - << strerror(-ret) << std::endl; > > > > > - return ret; > > > > > + DIR *folder = opendir(name); > > > > > + if (folder) { > > > > > + for (struct dirent *res; (res = readdir(folder));) { > > > > > + if (strlen(res->d_name) > 4 && > > > > > > > > I feel this might be a bit simplified, maybe using std::filesystem > > > > > > If ultimately demanded or required, I'll change to std::filesystem, a > > > quick grep through the codebase shows that we use opendir and other > > > similar C code in all instances except for one case in the Android > > > code though. And I would like to keep this code lean and in C if > > > possible. In fact in V2 I might make this even leaner and just write > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and > > > remove the strlen. > > > > > > > We have moved rather recently to C++17 for the internal code, where > > std::filesystem has been introduced. That's maybe why it's not that > > used. > > I think std::filesystem makes a lot of sense where you have to support > many platforms, Linux, Windows, MacOS, etc. and the underlying > implementation of the filesystem calls are different, in the > multi-platform case I could understand why it doesn't make sense to > maintain multiple versions for each platform. But when your only > platform is Linux, I feel like it's easier to call the C code directly > and you get the added benefit of knowing what kind of open calls, etc. What is the benefit in knowing that you use opendir + open plus a bunch of string comparison API and one memcpy, compared to the C++ stdlib ? I see your code doing pretty much canonical things, nothing special that benefits from knowing exactly what lib C functions are used. > are used. I feel you don't gain much with the std::filesystem here, > it's still just a loop with a string comparison. I think it adds a > little complexity here even, like when you see: > > dir.path().string().c_str() So, is + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); + constexpr size_t BASE_NAME_MAX = sizeof("card255"); + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; + char name[NODE_NAME_MAX] = "/dev/dri/"; + DIR *folder = opendir(name); + if (folder) { + for (struct dirent *res; (res = readdir(folder));) { + if (strlen(res->d_name) > 4 && + !strncmp(res->d_name, "card", 4)) { + memcpy(name + DIR_NAME_MAX - 1, res->d_name, + BASE_NAME_MAX); + + fd_ = open(name, O_RDWR | O_CLOEXEC); easier to parse than + const std::filesystem::path dri("/dev/dri"); + for (const auto &dir : std::filesystem::directory_iterator(dri)) { + const std::string &direntry = dir.path().filename().string(); + + if (direntry.find("card") == std::string::npos) + continue; + + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); I guess it's a matter of tastes. I generally don't like C++ syntax, but in this case the benefits are evident. In example reviewing what that memcpy exactly does (and let alone the fact you have to go through a copy) took me a few minutes. > > and you just need to pass a simple string in there. > > Once you start using a C++ feature you never go back, I always have > that little fear also. > Where do you have to go back to ? Anyway, it's a little patch and it's not worth a long discussion. If you understand C and you prefer it, that's fine. But I might have missed why any of the above arguments is actually relevant. Up to you ;) > > > > Up to you. However I don't find the previous version much leaner > > compared to the version I suggested, at least from a readability point > > of view. Why would you like to keep this a much as C code as possible > > if I may ask ? > > > > Thanks > > j > > > > > > > > > > const std::filesystem::path dri("/dev/dri"); > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > > const std::string &direntry = dir.path().filename().string(); > > > > > > > > if (direntry.find("card") == std::string::npos) > > > > continue; > > > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > > > > > ... > > > > } > > > > > > > > > + !strncmp(res->d_name, "card", 4)) { > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > > > + BASE_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); > > > > > > > > What if no card is found ? > > > > Should fd_ be initialized and here checked ? > > > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > > > > > > > > > > Thanks > > > > j > > > > > > > > > } > > > > > > > > > > /* > > > > > -- > > > > > 2.35.3 > > > > > > > > > > > > > > >
On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Eric > > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote: > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Eric, > > > > > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote: > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > Hi Eric > > > > > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > > --- > > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > > > > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > > > > > > > > > int Device::init() > > > > > > { > > > > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > > > > > - char name[NODE_NAME_MAX]; > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > > > int ret; > > > > > > > > > > > > /* > > > > > > @@ -404,14 +407,28 @@ 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) { > > > > > > - ret = -errno; > > > > > > - std::cerr > > > > > > - << "Failed to open DRM/KMS device " << name << ": " > > > > > > - << strerror(-ret) << std::endl; > > > > > > - return ret; > > > > > > + DIR *folder = opendir(name); > > > > > > + if (folder) { > > > > > > + for (struct dirent *res; (res = readdir(folder));) { > > > > > > + if (strlen(res->d_name) > 4 && > > > > > > > > > > I feel this might be a bit simplified, maybe using std::filesystem > > > > > > > > If ultimately demanded or required, I'll change to std::filesystem, a > > > > quick grep through the codebase shows that we use opendir and other > > > > similar C code in all instances except for one case in the Android > > > > code though. And I would like to keep this code lean and in C if > > > > possible. In fact in V2 I might make this even leaner and just write > > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and > > > > remove the strlen. > > > > > > > > > > We have moved rather recently to C++17 for the internal code, where > > > std::filesystem has been introduced. That's maybe why it's not that > > > used. > > > > I think std::filesystem makes a lot of sense where you have to support > > many platforms, Linux, Windows, MacOS, etc. and the underlying > > implementation of the filesystem calls are different, in the > > multi-platform case I could understand why it doesn't make sense to > > maintain multiple versions for each platform. But when your only > > platform is Linux, I feel like it's easier to call the C code directly > > and you get the added benefit of knowing what kind of open calls, etc. > > What is the benefit in knowing that you use opendir + open plus a > bunch of string comparison API and one memcpy, compared to the C++ > stdlib ? > > I see your code doing pretty much canonical things, nothing special > that benefits from knowing exactly what lib C functions are used. > > > are used. I feel you don't gain much with the std::filesystem here, > > it's still just a loop with a string comparison. I think it adds a > > little complexity here even, like when you see: > > > > dir.path().string().c_str() > > So, is > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > + DIR *folder = opendir(name); > + if (folder) { > + for (struct dirent *res; (res = readdir(folder));) { > + if (strlen(res->d_name) > 4 && > + !strncmp(res->d_name, "card", 4)) { > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > + BASE_NAME_MAX); > + > + fd_ = open(name, O_RDWR | O_CLOEXEC); > > easier to parse than Tbf, I think the std::filesystem examples are primarily easier to parse because of the use of std::string, rather than the use of std::filesystem. std::filesystem results in us replacing: DIR *folder = opendir(name); if (folder) { for (struct dirent *res; (res = readdir(folder));) { ... closedir(folder); with: const std::filesystem::path dri("/dev/dri"); for (const auto &dir : std::filesystem::directory_iterator(dri)) { with std::filesystem, it concerns me to read this: https://en.cppreference.com/w/cpp/filesystem/path/path "Exceptions 2, 4-8) May throw implementation-defined exceptions." I'm running this in a scenario where "/dev/dri" is often not available yet (an early starting camera for automotive), I want to know exactly what happens in that case ("man opendir" clearly states it returns null in that case), I think it will throw an exception, but based on the documentation I don't know for sure. Exception handling is slower also. I also don't know if it's path or directory_iterator that opens the directory, that's important so it can be handled correctly. The pre-existing code uses a "char name[NODE_NAME_MAX];" over an std::string, I actually think that's ok in this case, because the API defined that as the max size. > > + const std::filesystem::path dri("/dev/dri"); > + for (const auto &dir : std::filesystem::directory_iterator(dri)) { > + const std::string &direntry = dir.path().filename().string(); > + > + if (direntry.find("card") == std::string::npos) > + continue; > + > + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > I guess it's a matter of tastes. I generally don't like C++ syntax, but in > this case the benefits are evident. In example reviewing what that memcpy > exactly does (and let alone the fact you have to go through a copy) took me > a few minutes. > > > > > and you just need to pass a simple string in there. > > > > Once you start using a C++ feature you never go back, I always have > > that little fear also. > > > > Where do you have to go back to ? > > Anyway, it's a little patch and it's not worth a long discussion. > > If you understand C and you prefer it, that's fine. But I might have > missed why any of the above arguments is actually relevant. > > Up to you ;) > > > > > > > Up to you. However I don't find the previous version much leaner > > > compared to the version I suggested, at least from a readability point > > > of view. Why would you like to keep this a much as C code as possible > > > if I may ask ? > > > > > > Thanks > > > j > > > > > > > > > > > > > const std::filesystem::path dri("/dev/dri"); > > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > > > const std::string &direntry = dir.path().filename().string(); > > > > > > > > > > if (direntry.find("card") == std::string::npos) > > > > > continue; > > > > > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > > > > > > > ... > > > > > } > > > > > > > > > > > + !strncmp(res->d_name, "card", 4)) { > > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > > > > + BASE_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); > > > > > > > > > > What if no card is found ? > > > > > Should fd_ be initialized and here checked ? > > > > > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > > > > > > > > > > > > > Thanks > > > > > j > > > > > > > > > > > } > > > > > > > > > > > > /* > > > > > > -- > > > > > > 2.35.3 > > > > > > > > > > > > > > > > > > > > >
Hi Eric On Thu, Jun 02, 2022 at 02:50:33PM +0100, Eric Curtin wrote: > On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Eric > > > > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote: > > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi Eric, > > > > > > > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote: > > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > > > Hi Eric > > > > > > > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > > > --- > > > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > > > > > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > > > > > > > > > > > int Device::init() > > > > > > > { > > > > > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > > > > > > - char name[NODE_NAME_MAX]; > > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > > > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > > > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > > > > int ret; > > > > > > > > > > > > > > /* > > > > > > > @@ -404,14 +407,28 @@ 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) { > > > > > > > - ret = -errno; > > > > > > > - std::cerr > > > > > > > - << "Failed to open DRM/KMS device " << name << ": " > > > > > > > - << strerror(-ret) << std::endl; > > > > > > > - return ret; > > > > > > > + DIR *folder = opendir(name); > > > > > > > + if (folder) { > > > > > > > + for (struct dirent *res; (res = readdir(folder));) { > > > > > > > + if (strlen(res->d_name) > 4 && > > > > > > > > > > > > I feel this might be a bit simplified, maybe using std::filesystem > > > > > > > > > > If ultimately demanded or required, I'll change to std::filesystem, a > > > > > quick grep through the codebase shows that we use opendir and other > > > > > similar C code in all instances except for one case in the Android > > > > > code though. And I would like to keep this code lean and in C if > > > > > possible. In fact in V2 I might make this even leaner and just write > > > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and > > > > > remove the strlen. > > > > > > > > > > > > > We have moved rather recently to C++17 for the internal code, where > > > > std::filesystem has been introduced. That's maybe why it's not that > > > > used. > > > > > > I think std::filesystem makes a lot of sense where you have to support > > > many platforms, Linux, Windows, MacOS, etc. and the underlying > > > implementation of the filesystem calls are different, in the > > > multi-platform case I could understand why it doesn't make sense to > > > maintain multiple versions for each platform. But when your only > > > platform is Linux, I feel like it's easier to call the C code directly > > > and you get the added benefit of knowing what kind of open calls, etc. > > > > What is the benefit in knowing that you use opendir + open plus a > > bunch of string comparison API and one memcpy, compared to the C++ > > stdlib ? > > > > I see your code doing pretty much canonical things, nothing special > > that benefits from knowing exactly what lib C functions are used. > > > > > are used. I feel you don't gain much with the std::filesystem here, > > > it's still just a loop with a string comparison. I think it adds a > > > little complexity here even, like when you see: > > > > > > dir.path().string().c_str() > > > > So, is > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > + DIR *folder = opendir(name); > > + if (folder) { > > + for (struct dirent *res; (res = readdir(folder));) { > > + if (strlen(res->d_name) > 4 && > > + !strncmp(res->d_name, "card", 4)) { > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > + BASE_NAME_MAX); > > + > > + fd_ = open(name, O_RDWR | O_CLOEXEC); > > > > easier to parse than > > Tbf, I think the std::filesystem examples are primarily easier to > parse because of the use of std::string, rather than the use of > std::filesystem. std::filesystem results in us replacing: > > DIR *folder = opendir(name); > if (folder) { > for (struct dirent *res; (res = readdir(folder));) { > > ... > > closedir(folder); > > with: > > const std::filesystem::path dri("/dev/dri"); > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > with std::filesystem, it concerns me to read this: > > https://en.cppreference.com/w/cpp/filesystem/path/path > > "Exceptions > > 2, 4-8) May throw implementation-defined exceptions." > > I'm running this in a scenario where "/dev/dri" is often not available > yet (an early starting camera for automotive), I want to know exactly > what happens in that case ("man opendir" clearly states it returns > null in that case), I think it will throw an exception, but based on > the documentation I don't know for sure. Exception handling is slower > also. I also don't know if it's path or directory_iterator that opens > the directory, that's important so it can be handled correctly. You're right, it would require an if (!exists(dri)) return -ENOENT; before the for loop > > The pre-existing code uses a "char name[NODE_NAME_MAX];" over an > std::string, I actually think that's ok in this case, because the API > defined that as the max size. > > > > > + const std::filesystem::path dri("/dev/dri"); > > + for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > + const std::string &direntry = dir.path().filename().string(); > > + > > + if (direntry.find("card") == std::string::npos) > > + continue; > > + > > + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > I guess it's a matter of tastes. I generally don't like C++ syntax, but in > > this case the benefits are evident. In example reviewing what that memcpy > > exactly does (and let alone the fact you have to go through a copy) took me > > a few minutes. > > > > > > > > and you just need to pass a simple string in there. > > > > > > Once you start using a C++ feature you never go back, I always have > > > that little fear also. > > > > > > > Where do you have to go back to ? > > > > Anyway, it's a little patch and it's not worth a long discussion. > > > > If you understand C and you prefer it, that's fine. But I might have > > missed why any of the above arguments is actually relevant. > > > > Up to you ;) > > > > > > > > > > Up to you. However I don't find the previous version much leaner > > > > compared to the version I suggested, at least from a readability point > > > > of view. Why would you like to keep this a much as C code as possible > > > > if I may ask ? > > > > > > > > Thanks > > > > j > > > > > > > > > > > > > > > > const std::filesystem::path dri("/dev/dri"); > > > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > > > > const std::string &direntry = dir.path().filename().string(); > > > > > > > > > > > > if (direntry.find("card") == std::string::npos) > > > > > > continue; > > > > > > > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > > > > > > > > > ... > > > > > > } > > > > > > > > > > > > > + !strncmp(res->d_name, "card", 4)) { > > > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > > > > > + BASE_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); > > > > > > > > > > > > What if no card is found ? > > > > > > Should fd_ be initialized and here checked ? > > > > > > > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > > > > > > > > > > > > > > > > Thanks > > > > > > j > > > > > > > > > > > > > } > > > > > > > > > > > > > > /* > > > > > > > -- > > > > > > > 2.35.3 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Hello, On Fri, Jun 03, 2022 at 08:36:33AM +0200, Jacopo Mondi wrote: > On Thu, Jun 02, 2022 at 02:50:33PM +0100, Eric Curtin wrote: > > On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi wrote: > > > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote: > > > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote: > > > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote: > > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +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. 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> > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > > > > --- > > > > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- > > > > > > > > 1 file changed, 27 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > > > > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() > > > > > > > > > > > > > > > > int Device::init() > > > > > > > > { > > > > > > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); > > > > > > > > - char name[NODE_NAME_MAX]; > > > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > > > > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > > > > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > > > > > int ret; > > > > > > > > > > > > > > > > /* > > > > > > > > @@ -404,14 +407,28 @@ 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) { > > > > > > > > - ret = -errno; > > > > > > > > - std::cerr > > > > > > > > - << "Failed to open DRM/KMS device " << name << ": " > > > > > > > > - << strerror(-ret) << std::endl; > > > > > > > > - return ret; > > > > > > > > + DIR *folder = opendir(name); > > > > > > > > + if (folder) { > > > > > > > > + for (struct dirent *res; (res = readdir(folder));) { > > > > > > > > + if (strlen(res->d_name) > 4 && > > > > > > > > > > > > > > I feel this might be a bit simplified, maybe using std::filesystem > > > > > > > > > > > > If ultimately demanded or required, I'll change to std::filesystem, a > > > > > > quick grep through the codebase shows that we use opendir and other > > > > > > similar C code in all instances except for one case in the Android > > > > > > code though. And I would like to keep this code lean and in C if > > > > > > possible. In fact in V2 I might make this even leaner and just write > > > > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and > > > > > > remove the strlen. > > > > > > > > > > We have moved rather recently to C++17 for the internal code, where > > > > > std::filesystem has been introduced. That's maybe why it's not that > > > > > used. > > > > > > > > I think std::filesystem makes a lot of sense where you have to support > > > > many platforms, Linux, Windows, MacOS, etc. and the underlying > > > > implementation of the filesystem calls are different, in the > > > > multi-platform case I could understand why it doesn't make sense to > > > > maintain multiple versions for each platform. But when your only > > > > platform is Linux, I feel like it's easier to call the C code directly > > > > and you get the added benefit of knowing what kind of open calls, etc. > > > > > > What is the benefit in knowing that you use opendir + open plus a > > > bunch of string comparison API and one memcpy, compared to the C++ > > > stdlib ? > > > > > > I see your code doing pretty much canonical things, nothing special > > > that benefits from knowing exactly what lib C functions are used. > > > > > > > are used. I feel you don't gain much with the std::filesystem here, > > > > it's still just a loop with a string comparison. I think it adds a > > > > little complexity here even, like when you see: > > > > > > > > dir.path().string().c_str() > > > > > > So, is > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255"); > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; > > > + char name[NODE_NAME_MAX] = "/dev/dri/"; > > > > > > + DIR *folder = opendir(name); > > > + if (folder) { > > > + for (struct dirent *res; (res = readdir(folder));) { > > > + if (strlen(res->d_name) > 4 && > > > + !strncmp(res->d_name, "card", 4)) { > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > + BASE_NAME_MAX); > > > + > > > + fd_ = open(name, O_RDWR | O_CLOEXEC); > > > > > > easier to parse than > > > > Tbf, I think the std::filesystem examples are primarily easier to > > parse because of the use of std::string, rather than the use of > > std::filesystem. std::filesystem results in us replacing: > > > > DIR *folder = opendir(name); > > if (folder) { > > for (struct dirent *res; (res = readdir(folder));) { > > > > ... > > > > closedir(folder); > > > > with: > > > > const std::filesystem::path dri("/dev/dri"); > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > > with std::filesystem, it concerns me to read this: > > > > https://en.cppreference.com/w/cpp/filesystem/path/path > > > > "Exceptions > > > > 2, 4-8) May throw implementation-defined exceptions." > > > > I'm running this in a scenario where "/dev/dri" is often not available > > yet (an early starting camera for automotive), I want to know exactly > > what happens in that case ("man opendir" clearly states it returns > > null in that case), I think it will throw an exception, but based on > > the documentation I don't know for sure. Exception handling is slower > > also. I also don't know if it's path or directory_iterator that opens > > the directory, that's important so it can be handled correctly. The C++ filesystem API indeed seems a bit underspecified to me here. I would in practice expect the only exceptions to be thrown by path::path() to be std::bad_alloc or exceptions related to character encoding, which shouldn't be much of a concern. A path object can refer to a file or directory that doesn't exist, that won't cause the path constructor to throw an exception. The directory_iterator() constructor, however, will thrown an exception if the path doesn't exist or is not a directory. A call to fs::is_directory() would prevent that. I think I'm with Jacopo on this, not really due to std::filesystem as such, but more about usage of std::string. There's nothing performance-sensitive here, so I'd favour the readability and maintainability that std::string provides. This being said, I'll review v3. > You're right, it would require an > > if (!exists(dri)) > return -ENOENT; > > before the for loop > > > The pre-existing code uses a "char name[NODE_NAME_MAX];" over an > > std::string, I actually think that's ok in this case, because the API > > defined that as the max size. > > > > > > > > + const std::filesystem::path dri("/dev/dri"); > > > + for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > + const std::string &direntry = dir.path().filename().string(); > > > + > > > + if (direntry.find("card") == std::string::npos) > > > + continue; > > > + > > > + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > > > I guess it's a matter of tastes. I generally don't like C++ syntax, but in > > > this case the benefits are evident. In example reviewing what that memcpy > > > exactly does (and let alone the fact you have to go through a copy) took me > > > a few minutes. > > > > > > > > > > > and you just need to pass a simple string in there. > > > > > > > > Once you start using a C++ feature you never go back, I always have > > > > that little fear also. > > > > > > > > > > Where do you have to go back to ? > > > > > > Anyway, it's a little patch and it's not worth a long discussion. > > > > > > If you understand C and you prefer it, that's fine. But I might have > > > missed why any of the above arguments is actually relevant. > > > > > > Up to you ;) > > > > > > > > Up to you. However I don't find the previous version much leaner > > > > > compared to the version I suggested, at least from a readability point > > > > > of view. Why would you like to keep this a much as C code as possible > > > > > if I may ask ? > > > > > > > > > > > > > > > > > > > const std::filesystem::path dri("/dev/dri"); > > > > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) { > > > > > > > const std::string &direntry = dir.path().filename().string(); > > > > > > > > > > > > > > if (direntry.find("card") == std::string::npos) > > > > > > > continue; > > > > > > > > > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC); > > > > > > > > > > > > > > ... > > > > > > > } > > > > > > > > > > > > > > > + !strncmp(res->d_name, "card", 4)) { > > > > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name, > > > > > > > > + BASE_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); > > > > > > > > > > > > > > What if no card is found ? > > > > > > > Should fd_ be initialized and here checked ? > > > > > > > > > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot! > > > > > > > > > > > > > > } > > > > > > > >
diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device() int Device::init() { - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255"); - char name[NODE_NAME_MAX]; + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/"); + constexpr size_t BASE_NAME_MAX = sizeof("card255"); + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1; + char name[NODE_NAME_MAX] = "/dev/dri/"; int ret; /* @@ -404,14 +407,28 @@ 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) { - ret = -errno; - std::cerr - << "Failed to open DRM/KMS device " << name << ": " - << strerror(-ret) << std::endl; - return ret; + DIR *folder = opendir(name); + if (folder) { + for (struct dirent *res; (res = readdir(folder));) { + if (strlen(res->d_name) > 4 && + !strncmp(res->d_name, "card", 4)) { + memcpy(name + DIR_NAME_MAX - 1, res->d_name, + BASE_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); } /*
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> Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- src/cam/drm.cpp | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)