[libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards other than 0
Jacopo Mondi
jacopo at jmondi.org
Thu Jun 2 10:41:42 CEST 2022
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 at 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 at 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 at redhat.com>
> > > > > Signed-off-by: Eric Curtin <ecurtin at 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
> > > > >
> > > >
> > >
> >
>
More information about the libcamera-devel
mailing list