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

[Eric Curtin]

On Fri, 3 Jun 2022 at 22:52, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Thu, Jun 02, 2022 at 11:00:17AM +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.
>
> Out of curiosity, why is that ?

I don't know, I didn't mind too much, because this patch fixed it and
it seemed like the more correct way anyway. CC'ing Javier, just in
case he knows, being a DRM guru. I've seen it on 2 machines I own that
updated recently and Ian also saw it on his machine.

>
> > 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>
> >
> > ---
> > Changes in v3:
> > - switch back to memcpy, strncpy causing false compiler issue
> >
> > Changes in v2:
> > - return if no valid card found, or directory could not be opened etc.
> > - memcpy to strncpy for safety
> > - only adjust the numerical bytes for each iteration of loop since
> >   /dev/dri/card won't change
> > - initialize ret to zero
> > ---
> >  src/cam/drm.cpp | 46 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 36 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index 42c5a3b1..7975a502 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,9 +394,13 @@ Device::~Device()
> >
> >  int Device::init()
> >  {
> > -     constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255");
> > -     char name[NODE_NAME_MAX];
> > -     int ret;
> > +     constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/");
> > +     constexpr size_t PRE_NODE_NAME_MAX = sizeof("card");
> > +     constexpr size_t POST_NODE_NAME_MAX = sizeof("255");
> > +     constexpr size_t NODE_NAME_MAX =
> > +             DIR_NAME_MAX + PRE_NODE_NAME_MAX + POST_NODE_NAME_MAX - 2;
> > +     char name[NODE_NAME_MAX] = "/dev/dri/";
>
> This looks pretty error-prone compared to std::string, let's use the C++
> API here instead. This isn't performance-sensitive code.

Will std::stringify it.

>
> std::filesystem could be nice too, to avoid the manual closedir(), but
> that's a lesser concern.
>
> > +     int ret = 0;
> >
> >       /*
> >        * Open the first DRM/KMS device. The libdrm drmOpen*() functions
> > @@ -404,14 +409,35 @@ 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);
> > +     DIR *folder = opendir(name);
> > +     memcpy(name + DIR_NAME_MAX - 1, "card", PRE_NODE_NAME_MAX);
> > +     if (folder) {
>
> You can turn this into a
>
>         if (!folder) {
>                 ...
>         }
>
> to lower the indentation below.

Ok

>
> > +             for (struct dirent *res; (res = readdir(folder));) {
> > +                     if (!strncmp(res->d_name, "card", 4)) {
>
> Same thing here,
>
>                 if (strncmp(...))
>                         continue;
>
> (or rather the equivalent after switching to std::string)
>
> > +                             memcpy(name + DIR_NAME_MAX +
> > +                                             PRE_NODE_NAME_MAX - 2,
> > +                                     res->d_name + PRE_NODE_NAME_MAX - 1,
> > +                                     POST_NODE_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);
> > +     }
> > +
> >       if (fd_ < 0) {
> > -             ret = -errno;
> > -             std::cerr
> > -                     << "Failed to open DRM/KMS device " << name << ": "
> > -                     << strerror(-ret) << std::endl;
> > -             return ret;
> > +             std::cerr << "Unable to open any DRM/KMS device" << std::endl;
> > +             return ret ? ret : -ENOENT;
>
> I don't think it's very meaningful to pick the last error here, you can
> just return -ENOENT

Ok.

>
> >       }
>
> It could be interesting to try all available DRM devices to find the
> requested connector, as well as allowing selection of a particular
> device on the command line. That's an improvement candidate for the
> future, if a need arises.
>
> >
> >       /*
>
> --
> Regards,
>
> Laurent Pinchart
>