[libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards other than 0
Eric Curtin
ecurtin at redhat.com
Thu Jun 2 10:04:12 CEST 2022
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.
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
> > > >
> > >
> >
>
More information about the libcamera-devel
mailing list