| Message ID | 20220927232628.7544-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent 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@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 ? > + 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 ? > + 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 >
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@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." > > + 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 ? 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()
On Wed, 28 Sept 2022 at 09:52, Laurent Pinchart via libcamera-devel <libcamera-devel@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@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@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 >
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@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@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()
On Wed, 28 Sept 2022 at 11:09, Laurent Pinchart <laurent.pinchart@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@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@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 >
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; + 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); + if (ret < 0) { + drmClose(fd_); + fd_ = -1; + continue; + } + + found = true; + break; } closedir(folder); - return ret; + return found ? 0 : -ENOENT; } int Device::getResources()
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@ideasonboard.com> --- src/cam/drm.cpp | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)