[libcamera-devel] [PATCH] cam: drm: Skip DRM devices not capable of supporting the atomic API
Eric Curtin
ecurtin at redhat.com
Wed Sep 28 12:35:56 CEST 2022
On Wed, 28 Sept 2022 at 11:09, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
Sorry yes, I'll wait for this fix even. Yikes...
>
> > 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
>
More information about the libcamera-devel
mailing list