| Message ID | 20220928102743.1484-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On Wed, 28 Sept 2022 at 11:27, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 mode setting API. Some legacy display > controllers would be skipped as well, but libcamera doesn't run on those > systems anyway. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Use DRM_CAP_DUMB_BUFFER instead of DRM_CLIENT_CAP_ATOMIC > --- > src/cam/drm.cpp | 35 +++++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > index b0602c942853..3bb950fd157a 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,42 @@ 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 modeset API, to avoid > + * selecting a DRM device corresponding to a GPU. There is no > + * modeset capability, but the kernel returns an error for most > + * caps if mode setting isn't support but the driver. The > + * DRM_CAP_DUMB_BUFFER capability is one of those, other would > + * do as well. The capability value itself isn't relevant. > + */ > + ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); > + if (ret < 0) { Since this ends up being an ioctl I guess this is fine. In modetest.c they do it the `if (ret || cap == 0)` way, but a failed ioctl returns -1 so this is fine too. Plus the author of that code in modetest.c is Laurent Pinchart in 2014 :) Reviewed-by: Eric Curtin <ecurtin@redhat.com> > + drmClose(fd_); > + fd_ = -1; > + continue; > + } > + > + found = true; > + break; > } > > closedir(folder); > > - return ret; > + return found ? 0 : -ENOENT; > } > > int Device::getResources() > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Wed, Sep 28, 2022 at 01:27:43PM +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 mode setting API. Some legacy display > controllers would be skipped as well, but libcamera doesn't run on those > systems anyway. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Use DRM_CAP_DUMB_BUFFER instead of DRM_CLIENT_CAP_ATOMIC > --- > src/cam/drm.cpp | 35 +++++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp > index b0602c942853..3bb950fd157a 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,42 @@ 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 modeset API, to avoid > + * selecting a DRM device corresponding to a GPU. There is no > + * modeset capability, but the kernel returns an error for most > + * caps if mode setting isn't support but the driver. The s/but/by ? > + * DRM_CAP_DUMB_BUFFER capability is one of those, other would > + * do as well. The capability value itself isn't relevant. That's a really awful API :) For this patch Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + */ > + ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); > + 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..3bb950fd157a 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,42 @@ 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 modeset API, to avoid + * selecting a DRM device corresponding to a GPU. There is no + * modeset capability, but the kernel returns an error for most + * caps if mode setting isn't support but the driver. The + * DRM_CAP_DUMB_BUFFER capability is one of those, other would + * do as well. The capability value itself isn't relevant. + */ + ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &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 mode setting API. Some legacy display controllers would be skipped as well, but libcamera doesn't run on those systems anyway. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Use DRM_CAP_DUMB_BUFFER instead of DRM_CLIENT_CAP_ATOMIC --- src/cam/drm.cpp | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)