[v4] apps: cam: Skip non-display GPUs

23458 diff mbox series

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.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/apps/cam/drm.cpp | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

On Fri, May 30, 2025 at 11:08:15AM +0200, Milan Zamazal wrote:
> 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.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Pushed, thank you.

> ---
>  src/apps/cam/drm.cpp | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp
> index 47bbb6b05..f4b470976 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;
>  
> @@ -465,15 +463,22 @@ int Device::openCard()
>  		}
>  
>  		/*
> -		 * 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.
> +		 * 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.
>  		 */
> -		ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap);
> -		if (ret < 0) {
> +		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;