[PATCH v3] apps: cam: Skip non-display GPUs

[Laurent Pinchart]

On Fri, May 30, 2025 at 09:05:41AM +0200, Mattijs Korpershoek wrote:
> On jeu., mai 29, 2025 at 15:08, Kieran Bingham wrote:
> > Quoting Milan Zamazal (2025-05-29 09:23:26)
> >> Device::openCard() in the cam DRM helpers looks for a /dev/dri/card*
> >> device that can be opened and that doesn't fail when asked about
> >> DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is
> >> supported by the device).
> >> 
> >> There can be matching devices that are not display devices.  This can
> >> lead to selection of such a device and inability to use KMS output with
> >> `cam' application.  The ultimate goal is to display something on the
> >> device and later the KMS sink will fail if there is no connector
> >> attached to the device (although it can actually fail earlier, when
> >> trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not
> >> supported).
> >> 
> >> Let's avoid selecting devices without connectors, CRTCs or encoders.
> >> The added check makes the original check for DRM_CAP_DUMB_BUFFER API
> >> most likely unnecessary, let's remove it.
> >> 
> >> Changes in v3:
> >> - Added checks for the presence of CRTCs and encoders.
> >> - Removed the check for DRM_CAP_DUMB_BUFFER.
> >> 
> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> ---
> >>  src/apps/cam/drm.cpp | 22 ++++++++++------------
> >>  1 file changed, 10 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp
> >> index 47bbb6b05..c40c31410 100644
> >> --- a/src/apps/cam/drm.cpp
> >> +++ b/src/apps/cam/drm.cpp
> >> @@ -450,8 +450,6 @@ int Device::openCard()
> >>         }
> >>  
> >>         for (struct dirent *res; (res = readdir(folder));) {
> >> -               uint64_t cap;
> >> -
> >>                 if (strncmp(res->d_name, "card", 4))
> >>                         continue;
> >>  
> >> @@ -464,16 +462,16 @@ int Device::openCard()
> >>                         continue;
> >>                 }
> >>  
> >> -               /*
> >> -                * 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 by 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) {
> >> +               /* Skip devices without connectors. */
> >
> > This all sounds reasonable to me, though this comment now skips devices
> > without connectors/crts/encoders.
> >
> > Perhaps we should update it while applying?
> >
> > 		/* Skip non-display devices */
> 
> I agree with the fixup.

I would expand it a bit

 		/*
		 * Skip non-display devices. While this could in theory be done
		 * by checking for support of the mode setting API, some
		 * out-of-tree render-only GPU drivers (namely powervr)
		 * incorrectly set the DRIVER_MODESET driver feature. Check for
		 * the presence of at least one CRTC, encoder and connector
		 * instead.
		 */

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> If it's applied, then:
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek at kernel.org>
> 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >> +               std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{
> >> +                       drmModeGetResources(fd_),
> >> +                       &drmModeFreeResources
> >> +               };
> >> +               if (!resources ||
> >> +                   resources->count_connectors <= 0 ||
> >> +                   resources->count_crtcs <= 0 ||
> >> +                   resources->count_encoders <= 0) {
> >> +                       resources.reset();
> >>                         drmClose(fd_);
> >>                         fd_ = -1;
> >>                         continue;

-- 
Regards,

Laurent Pinchart