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

[Eric Curtin]

On Wed, 28 Sept 2022 at 09:52, Laurent Pinchart via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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

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
>