[libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards other than 0

[Jacopo Mondi]

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.

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