[Mesa-dev] [RFC] egl/android: Add DRM node probing and filtering

[Tomasz Figa]

Hi Rob,

On Thu, Apr 19, 2018 at 1:03 AM Robert Foss <robert.foss at collabora.com>
wrote:

> This patch both adds support for probing & filtering DRM nodes
> and switches away from using the GRALLOC_MODULE_PERFORM_GET_DRM_FD
> gralloc call.

> Currently the filtering is based just on the driver name,
> and the desired name is supplied using the "drm.gpu.vendor_name"
> Android property.

> The filtering itself is done using the newly introduced
> libdrm drmHandleMatch() call.

> Signed-off-by: Robert Foss <robert.foss at collabora.com>
> ---

> This patch is based on[1], which contains a new libdrm function,
> called drmHandleMatch(), which allows for matching an opened
> drm node handle against some desired properties.

> A choice that was made for this patch was to add support for
> falling back to to DRM nodes that have failed the filtering,
> if no node passes the filter.
> If this wouldn't be useful to anyone, I would suggest ripping it
> out since it is a little bit ugly&complex.

This is actually not only useful, but quite of a requirement for Chrome OS,
where we use the same Android image for devices with different GPU vendors.
(Technically we could hack it up by injecting some per-board properties,
but it would be painful from maintenance point of view.)

But actually, the way it is implemented in this patch will not work. The
code assumes that the first DRM node that could be opened
(loader_open_device()) is fine, but if you look at what we do in our
Chromium patch [2], it is actually necessary to check if Mesa includes a
driver for it (loader_get_driver_for_fd()).

[2]
https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/780797



> [1] https://www.spinics.net/lists/dri-devel/msg172497.html


>   src/egl/drivers/dri2/platform_android.c | 72
++++++++++++++++++++++++++++-----
>   1 file changed, 62 insertions(+), 10 deletions(-)

> diff --git a/src/egl/drivers/dri2/platform_android.c
b/src/egl/drivers/dri2/platform_android.c
> index 7f1a496ea24..0b082fe5dcc 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -27,6 +27,7 @@
>    * DEALINGS IN THE SOFTWARE.
>    */

> +#include <cutils/properties.h>
>   #include <errno.h>
>   #include <dlfcn.h>
>   #include <fcntl.h>
> @@ -1117,18 +1118,69 @@ droid_add_configs_for_visuals(_EGLDriver *drv,
_EGLDisplay *dpy)
>   static int
>   droid_open_device(struct dri2_egl_display *dri2_dpy)
>   {
> -   int fd = -1, err = -EINVAL;
> -
> -   if (dri2_dpy->gralloc->perform)
> -         err = dri2_dpy->gralloc->perform(dri2_dpy->gralloc,
> -
  GRALLOC_MODULE_PERFORM_GET_DRM_FD,
> -                                          &fd);
> -   if (err || fd < 0) {
> -      _eglLog(_EGL_WARNING, "fail to get drm fd");
> -      fd = -1;
> +   int prop_set, num_devices, ret;
> +   const int node_type = DRM_NODE_RENDER;
> +   int fd = -1, fallback_fd = -1;
> +
> +   const int MAX_DRM_DEVICES = 32;
> +   char vendor_name[PROPERTY_VALUE_MAX];
> +   prop_set = property_get("drm.gpu.vendor_name", vendor_name, NULL);
> +   drmVersion ver_filter;
> +   ver_filter.name = vendor_name;
> +
> +   drmDevicePtr devices[MAX_DRM_DEVICES];
> +   num_devices = drmGetDevices2(0, devices, MAX_DRM_DEVICES);
> +   if (num_devices < 0) {
> +      _eglLog(_EGL_WARNING, "Failed to find any DRM devices");
> +      return -1;
> +   }
> +
> +   for (int i = 0; i < num_devices; i++) {
> +      /* Filter out DRM_NODE_ types we aren't interested in */
> +      if (!(devices[i]->available_nodes & (1 << node_type))) {
> +         continue;
> +      }
> +
> +      /* Open DRM node FD */
> +      fd = loader_open_device(devices[i]->nodes[node_type]);
> +      if (fd == -1 && errno == EINVAL) {
> +         _eglLog(_EGL_WARNING, "%s()  node #%d failed to open",
__func__, i);
> +         continue;
> +      }

What should happen if (fd == -1 && errno != EINVAL)? I don't think we
should run the code below in such case.

> +
> +      /* See if FD matches the driver vendor we want */
> +      if (prop_set && !drmHandleMatch(fd, &ver_filter, NULL)){
> +         _eglLog(_EGL_WARNING, "%s()  node #%d  FD=%d does not match
filter", __func__, i , fd);

How about printing the actual node (i.e. devices[i]->nodes[node_type]),
rather than index in devices array?

Also, should it be _EGL_DEBUG? I'd expect it to be a normal event, which
should not concern the user.

> +         goto next;
> +      }
> +
> +      /* Successfully found matching FD */
> +      close(fallback_fd);
> +      fallback_fd = -1;
> +      break;
> +
> +next:
> +      if (fallback_fd == -1) {
> +         fallback_fd = fd;
> +         fd = -1;
> +      } else {
> +         close(fd);
> +         fd = -1;
> +      }
> +      continue;
> +   }

How about simplifying the code a bit:

+   for (int i = 0; i < num_devices; i++) {
+      /* Filter out DRM_NODE_ types we aren't interested in */
+      if (!(devices[i]->available_nodes & (1 << node_type))) {
+         continue;
+      }
+
+      /* Open DRM node FD */
+      fd = loader_open_device(devices[i]->nodes[node_type]);
+      if (fd < 0) {
+         _eglLog(_EGL_WARNING, "%s()  node #%d failed to open", __func__,
i);
+         continue;
+      }
+
+      if (fallback_fd < 0)
+         fallback_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
+
+      /* See if FD matches the driver vendor we want */
+      if (!prop_set || drmHandleMatch(fd, &ver_filter, NULL))
+         /* Successfully found matching FD */
+         break;
+
+      _eglLog(_EGL_WARNING, "%s()  node #%d  FD=%d does not match filter",
__func__, i , fd);
+      close(fd);
+      fd = -1;
+   }
+
+   if (fd >= 0) {
+      close(fallback_fd);
+      fallback_fd = -1;
+   } else {
+      _eglLog(_EGL_WARNING, "Failed to open desired DRM handle, using
fallback");
+      fd = fallback_fd;
+   }
> +
> +   if (fd < 0) {
> +      _eglLog(_EGL_WARNING, "Failed to open desired DRM handle, using
fallback");
> +      return fallback_fd;
>      }

Best regards,
Tomasz