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

[Eric Curtin]

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.

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