[Mesa-dev] [PATCH v3 3/3] egl/android: Add DRM node probing and filtering

[Tomasz Figa]

Hi Rob,

Thanks for sending v3. Please see few more comments inline.

On Sun, Jun 10, 2018 at 2:28 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.
>
> Signed-off-by: Robert Foss <robert.foss at collabora.com>
> ---
>
> Changes since v2:
>  - Switch from drmGetDevices2 to manual renderD node iteration
>  - Add probe_res enum to communicate probing results better
>  - Avoid using _eglError() in internal static functions
>  - Avoid actually loading the driver while probing, just verify
>    that it exists.
>  - Replace strlen call with the assumed length PROPERTY_VALUE_MAX

[snip]

> +static probe_ret_t
> +droid_probe_device(_EGLDisplay *disp, int fd, char *vendor)
> +{
> +   int ret;
> +
> +   drmVersionPtr ver = drmGetVersion(fd);
> +   if (!ver)
> +      return probe_error;
> +
> +   if (vendor != NULL && ver->name != NULL &&
> +       strncmp(vendor, ver->name, PROPERTY_VALUE_MAX) != 0) {

Shouldn't we fail the match if vendor != NULL, but ver->name == NULL? i.e.

if (vendor && (!ver->name || strcmp(...)) { ...

> +          ret = probe_filtered_out;
> +          goto cleanup;
> +       }
> +
> +
> +   if (!droid_probe_driver(fd)) {
> +      ret = probe_no_driver;
> +      goto cleanup;
> +   }
> +
> +   ret = probe_success;
> +
> +cleanup:
> +   drmFreeVersion(ver);
> +   return ret;
> +}
> +
> +static int
> +droid_open_device(_EGLDisplay *disp)
> +{
> +   const int MAX_DRM_DEVICES = 32;
> +   int prop_set, num_devices;
> +   int fd = -1, fallback_fd = -1;
> +
> +   char *vendor_name = NULL;
> +   char vendor_buf[PROPERTY_VALUE_MAX];
> +   if (property_get("drm.gpu.vendor_name", vendor_buf, NULL) > 0);
> +      vendor_name = vendor_buf;
> +
> +   const char *drm_dir_name = "/dev/dri";
> +   DIR *sysdir = opendir(drm_dir_name);
> +   if (!sysdir)
> +       return -errno;
> +
> +   struct dirent *dent;
> +   while ((dent = readdir(sysdir))) {
> +      char dev_path[128];
> +      char *render_dev_prefix = "renderD";
> +      size_t prefix_len = strlen(render_dev_prefix);

If a const array like below is used instead

const char render_dev_prefix[] = "renderD";

you can just use sizeof(render_dev_prefix), without the need for strlen().

> +
> +      if (strncmp(render_dev_prefix, dent->d_name, prefix_len) != 0)
> +         continue;
> +
> +      sprintf(dev_path, "%s/%s", drm_dir_name, dent->d_name);

snprintf(dev_path, sizeof(dev_path), ...);

> +      fd = loader_open_device(dev_path);
> +      if (fd == -1) {

nit: Perhaps fd < 0, just to be safe?

> +         _eglLog(_EGL_WARNING, "%s() Failed to open DRM device %s",
> +                 __func__, dev_path);
> +         continue;
> +      }
> +
> +      int ret = droid_probe_device(disp, fd, vendor_name);
> +      switch (ret) {
> +      case probe_success:
> +         goto success;
> +      case probe_filtered_out:
> +         goto allow_fallback;

The 2 lines of code at the "allow_fallback" label could be just moved here.

> +      case probe_error:
> +      case probe_no_driver:

Do we need 2 separate cases for these? Just one "probe_fail" should be enough.

> +         goto next;

If we move the fallback handling to "probe_filtered_out" case, we
could remove the "next" label too and simply "break" here.

> +      }
> +
> +allow_fallback:
> +      if (fallback_fd == -1)
> +         fallback_fd = fd;
> +next:
> +      if (fallback_fd != fd)
> +         close(fd);
> +      fd = -1;
> +      continue;
> +   }
> +
> +success:
> +   closedir(sysdir);
> +
> +   if (fallback_fd < 0 && fd < 0) {
> +      _eglLog(_EGL_WARNING, "Failed to open any DRM device");
> +      return -1;
> +   }
> +
> +   if (fd < 0) {
> +      _eglLog(_EGL_WARNING, "Failed to open desired DRM device, using fallback");
> +      return fallback_fd;
> +   }
> +
> +   close(fallback_fd);
> +   return fd;
> +}

[Leaving context for readability.]

Best regards,
Tomasz