[libcamera-devel] [PATCH] cam: drm: Skip DRM devices not capable of supporting the atomic API

[Laurent Pinchart]

Hi Eric,

On Wed, Sep 28, 2022 at 10:57:29AM +0100, Eric Curtin wrote:
> On Wed, 28 Sept 2022 at 09:52, Laurent Pinchart wrote:
> >
> > Hi Jacopo,
> >
> > On Wed, Sep 28, 2022 at 08:10:02AM +0200, Jacopo Mondi wrote:
> > > On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > The DRM helper picks the first DRM card that it can open. On platforms
> > > > that have a standalone GPU, this risks selecting a device corresponding
> > > > to the GPU instead of the display controller. Fix this by skipping
> > > > devices that don't support the KMS atomic API. Some legacy display
> > > > controllers would be skipped as well, but libcamera doesn't run on those
> > > > systems anyway, and the DRM helper doesn't support the legacy KMS
> > > > modeset API in any case.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > >  src/cam/drm.cpp | 31 +++++++++++++++++++++++--------
> > > >  1 file changed, 23 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > > > index b0602c942853..b2a6b0bba9e8 100644
> > > > --- a/src/cam/drm.cpp
> > > > +++ b/src/cam/drm.cpp
> > > > @@ -430,7 +430,8 @@ int Device::init()
> > > >  int Device::openCard()
> > > >  {
> > > >     const std::string dirName = "/dev/dri/";
> > > > -   int ret = -ENOENT;
> > > > +   bool found = false;
> > > > +   int ret;
> > > >
> > > >     /*
> > > >      * Open the first DRM/KMS device beginning with /dev/dri/card. The
> > > > @@ -449,24 +450,38 @@ int Device::openCard()
> > > >     }
> > > >
> > > >     for (struct dirent *res; (res = readdir(folder));) {
> > > > +           uint64_t cap;
> > > > +
> > > >             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;
> > > > +           if (fd_ < 0) {
> > > > +                   ret = -errno;
> > >
> > > As we don't propagate errors up, can't you use errno directly ?
> >
> > The first calls to operator<<() may modify the value of errno before the
> > strerror() call is evaluated. See
> > https://en.cppreference.com/w/cpp/language/eval_order:
> >
> > "Order of evaluation of any part of any expression, including order of
> > evaluation of function arguments is unspecified (with some exceptions
> > listed below). The compiler can evaluate operands and other
> > subexpressions in any order, and may choose another order when the same
> > expression is evaluated again."
> 
> Yeah, I think what Laurent did with the extra variable is safer, just
> in case errno changes.
> 
> > > > +                   std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> > > > +                             << strerror(-ret) << std::endl;
> > > > +                   continue;
> > > >             }
> > > >
> > > > -           ret = -errno;
> > > > -           std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> > > > -                     << strerror(-ret) << std::endl;
> > > > +           /*
> > > > +            * Skip devices that don't support the atomic API, to avoid
> > > > +            * selecting a DRM device corresponding to a GPU.
> > > > +            */
> > > > +           ret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);
> > >
> > > This is the only reference I found of drmGetCap:
> > > https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/
> > >
> > > Can it return a negative value ?
> 
> It's important to read the source code on gitlab for drm, because they
> accept gitlab Merge Requests as well as submissions via mailing list
> (like Laurent alluded to). My last patch for drm was accepted via
> gitlab, so it probably doesn't appear on the mailing list as an
> example. So looking at:
> 
> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c
> 
> I think what's most correct is `if (ret || !cap)` from looking at the
> outermost drmGetCap function, the cap.value only gets set if we return
> 0 (and as a secondary check, make sure capability is set to non-zero).
> Personally I think this is better as the reader can identify what is
> intended without going into kernel-space code. I also saw this logic
> in Chromium OS (well they do the inverse):
> 
> https://chromium.googlesource.com/chromiumos/platform/frecon/+/refs/heads/main/drm.c

It's interesting that they have made the exact same mistake as I did :-)
DRM_CLIENT_CAP_ATOMIC is a flag for the drmSetClientCap() function, not
drmGetCap(). It only works by chance.

> with this change:
> 
> Reviewed-by: Eric Curtin <ecurtin at redhat.com>
> 
> >
> > https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L1383 for
> > the implementation of the function, which just calls DRM_IOCTL_GET_CAP.
> > That ends up in drm_getcap() on the kernel side, which, for
> > DRM_CLIENT_CAP_ATOMIC, ... works by chance, as DRM_IOCTL_GET_CAP take a
> > DRM_CAP_* argument, not a DRM_CLIENT_CAP_* argument.
> > DRM_CLIENT_CAP_ATOMIC is equal to 3, which is
> > DRM_CAP_DUMB_PREFERRED_DEPTH. I'll fix this to use DRM_CAP_DUMB_BUFFER.
> > It doesn't matter much what cap we read, the important part is to pick
> > one that returns an error for non-KMS drivers.
> >
> > > > +           if (ret < 0) {
> > > > +                   drmClose(fd_);
> > > > +                   fd_ = -1;
> > > > +                   continue;
> > > > +           }
> > > > +
> > > > +           found = true;
> > > > +           break;
> > > >     }
> > > >
> > > >     closedir(folder);
> > > >
> > > > -   return ret;
> > > > +   return found ? 0 : -ENOENT;
> > > >  }
> > > >
> > > >  int Device::getResources()

-- 
Regards,

Laurent Pinchart