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

[Eric Curtin]

On Sat, 18 Jun 2022 at 21:06, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Fri, Jun 17, 2022 at 11:00:44AM +0100, Eric Curtin 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>
> > Tested-by: Ian Mullins <imullins at redhat.com>
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> > Changes in v6:
> > - Spaces to tabs
> > - Remove a brace for single line if statement
> >
> > Changes in v5:
> > - Split into openCard function
> > - ret initialized to -ENOENT, in case directory is open with no card*
> > - Add another string to simplify
> >
> > Changes in v4:
> > - added Tested-by tag
> > - std::stringified things
> > - changed if conditions to reduce indentations
> > - constexpr sizes now don't include null terminator
> > - just return -ENOENT if no device was successfully opened.
> >
> > 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 | 59 ++++++++++++++++++++++++++++++++++++-------------
> >  src/cam/drm.h   |  2 +-
> >  2 files changed, 45 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index 42c5a3b1..41fd042b 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>
> > @@ -391,26 +392,54 @@ Device::~Device()
> >               drmClose(fd_);
> >  }
> >
> > -int Device::init()
> > +int Device::openCard()
> >  {
> > -     constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255");
> > -     char name[NODE_NAME_MAX];
> > -     int ret;
> > +     const std::string dirName = "/dev/dri/";
> > +     int ret = -ENOENT;
> >
> >       /*
> > -      * Open the first DRM/KMS device. The libdrm drmOpen*() functions
> > -      * require either a module name or a bus ID, which we don't have, so
> > -      * bypass them. The automatic module loading and device node creation
> > -      * from drmOpen() is of no practical use as any modern system will
> > -      * handle that through udev or an equivalent component.
> > +      * Open the first DRM/KMS device beginning with /dev/dri/card. The
> > +      * libdrm drmOpen*() functions require either a module name or a bus ID,
> > +      * which we don't have, so bypass them. The automatic module loading and
> > +      * device node creation 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) {
> > +     DIR *folder = opendir(dirName.c_str());
> > +     if (!folder) {
> >               ret = -errno;
> > -             std::cerr
> > -                     << "Failed to open DRM/KMS device " << name << ": "
> > -                     << strerror(-ret) << std::endl;
> > +             std::cerr << "Failed to open " << dirName
> > +                       << " directory: " << strerror(-ret) << std::endl;
> > +             return ret;
> > +     }
> > +
> > +     for (struct dirent *res; (res = readdir(folder));) {
> > +             if (strncmp(res->d_name, "card", 4))
> > +                     continue;
> > +
> > +             const std::string devName = dirName + res->d_name;
> > +             fd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);
> > +             if (fd_ >= 0) {
> > +                     ret = 0;
> > +                     break;
> > +             }
> > +
> > +             ret = -errno;
> > +             std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> > +                       << strerror(-ret) << std::endl;
> > +     }
> > +
> > +     closedir(folder);
> > +
> > +     return ret;
> > +}
> > +
> > +int Device::init()
> > +{
> > +     int ret = openCard();
> > +     if (ret < 0) {
> > +             std::cerr << "Failed to open any DRM/KMS device: "
> > +                       << strerror(-ret) << std::endl;
> >               return ret;
> >       }
> >
> > diff --git a/src/cam/drm.h b/src/cam/drm.h
> > index de57e445..1817d053 100644
> > --- a/src/cam/drm.h
> > +++ b/src/cam/drm.h
> > @@ -312,7 +312,7 @@ private:
> >       Device &operator=(const Device &&) = delete;
> >
> >       int getResources();
> > -
> > +     int openCard();
>
> Just one small nit, I'd move this before getResources() to match the
> order in the .cpp file. With that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> No need to resend, I'll fix this when applying.

Ok thanks Laurent.

>
> >       void drmEvent();
> >       static void pageFlipComplete(int fd, unsigned int sequence,
> >                                    unsigned int tv_sec, unsigned int tv_usec,
> >
>
> --
> Regards,
>
> Laurent Pinchart
>