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

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> 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.
> 		 */

Yes, thank you, done in v4 (the only change there).

> 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;