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

[Tomasz Figa]

Hi Rob,

Finally got to review this. Please see my comments inline.

On Fri, May 11, 2018 at 10:48 PM Robert Foss <robert.foss at collabora.com>
wrote:
[snip]
> +EGLBoolean
> +droid_load_driver(_EGLDisplay *disp)

Since this is not EGL-facing, I'd personally use bool.

> +{
> +   struct dri2_egl_display *dri2_dpy = disp->DriverData;
> +   const char *err;
> +
> +   dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> +   if (dri2_dpy->driver_name == NULL) {
> +      err = "DRI2: failed to get driver name";
> +      goto error;

It shouldn't be an error if there is no driver for given render node. We
should just skip it and try next one, which I believe would be achieved by
just returning false here.

> +   }
> +
> +   dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) ==
DRM_NODE_RENDER;
> +
> +   if (!dri2_dpy->is_render_node) {
> +   #ifdef HAVE_DRM_GRALLOC
> +       /* Handle control nodes using __DRI_DRI2_LOADER extension and GEM
names
> +        * for backwards compatibility with drm_gralloc. (Do not use on
new
> +        * systems.) */
> +       dri2_dpy->loader_extensions = droid_dri2_loader_extensions;
> +       if (!dri2_load_driver(disp)) {
> +          err = "DRI2: failed to load driver";
> +          goto error;
> +       }
> +   #else
> +       err = "DRI2: handle is not for a render node";
> +       goto error;
> +   #endif
> +   } else {
> +       dri2_dpy->loader_extensions = droid_image_loader_extensions;
> +       if (!dri2_load_driver_dri3(disp)) {
> +          err = "DRI3: failed to load driver";
> +          goto error;
> +       }
> +    }
> +
> +   return EGL_TRUE;
> +
> +error:
> +   free(dri2_dpy->driver_name);
> +   dri2_dpy->driver_name = NULL;
> +   return _eglError(EGL_NOT_INITIALIZED, err);

Hmm, if we signal EGL error here, we should break the probing loop and just
bail out. This would suggest that a boolean is not the right type for this
function to return. Perhaps something like negative error, 0 for skip and 1
for success would make sense?

Also, how does it play with the _eglError() called from the error path of
dri2_initialize_android()?

> +}
> +
> +static int
> +droid_probe_driver(_EGLDisplay *disp, int fd)
> +{
> +   struct dri2_egl_display *dri2_dpy = disp->DriverData;
> +   dri2_dpy->fd = fd;
> +
> +   if (!droid_load_driver(disp))
> +      return false;

Given my other suggestion about distinguishing failure, render node skip
and success, I think it should be more like this:

    int ret = droid_load_driver(disp);
    if (ret <= 0)
       return ret;

Or actually, maybe we don't really need to go as far as loading the driver.
I'd say it should be enough to just check if we have a driver for the
device by looking at what loader_get_driver_for_fd() returns. (In that
case, we can ignore my comment about returning error on
loader_get_driver_for_fd() failure in droid_load_driver(), since the
skipping would be handling only here.)

> +
> +   /* Since this probe can succeed, but another filter may fail,

What another filter could fail? I can see the vendor name being checked
before calling this function.

The free() below is actually needed, just the comment is off. We need to
free the name to be able to probe remaining nodes, without leaking the name.

> +      this string needs to be deallocated either way.
> +      Once an FD has been found, this string will be set a second time.
*/
> +   free(dri2_dpy->driver_name);

Don't we also need to unload the driver?

> +   dri2_dpy->driver_name = NULL;
> +   return true;

To match the change above:

    return 1;

> +}
> +
> +static int
> +droid_probe_device(_EGLDisplay *disp, int fd, drmDevicePtr dev, char
*vendor)
> +{
> +   drmVersionPtr ver = drmGetVersion(fd);
> +       if (!ver)
> +               goto fail;

Something wrong with indentation here.

> +
> +   size_t vendor_len = strlen(vendor);
> +   if (vendor_len != 0 && strncmp(vendor, ver->name, vendor_len))
> +      goto fail;

Maybe it's just me, but I don't see any point in using strncmp() if the
length argument is obtained by calling strlen() first. Especially if the
strlen() call is on a string that comes from some external code
(property_get()).

Perhaps we could just call strncmp() with PROPERTY_VALUE_MAX? This would
actually play nice with my other comment about using NULL for vendor
string, if the property is not present.

Also nit: The label could be named in a more meaningful way, e.g.
err_free_version.

> +
> +   if (!droid_probe_driver(disp, fd))
> +      goto fail;
> +
> +   drmFreeVersion(ver);
> +   return true;
> +
> +fail:
> +   drmFreeVersion(ver);
> +   return false;

Given my other suggestion about distinguishing failure, render node skip
and success, I think it should be more like this:

    ret = droid_probe_driver(disp, fd);
err_free_version:
    drmFreeVersion(ver);
    return ret;

> +}
> +
> +static int
> +droid_open_device(_EGLDisplay *disp)
> +{
> +   const int MAX_DRM_DEVICES = 32;
> +   int prop_set, num_devices, ret;
> +   int fd = -1, fallback_fd = -1;
> +
> +   char vendor_name[PROPERTY_VALUE_MAX];
> +   property_get("drm.gpu.vendor_name", vendor_name, NULL);

vendor_name[] is uninitialized. property_get() with NULL 3rd argument
wouldn't touch the 2nd argument if the property is missing.

However, I'd recommend checking the return value of property_get() and pass
NULL as vendor_name to droid_probe_device() if it's <= 0. The check for
existence of the filter will be simpler with this.

> +
> +   drmDevicePtr devices[MAX_DRM_DEVICES];
> +   num_devices = drmGetDevices2(0, devices, MAX_DRM_DEVICES);
> +
> +   if (num_devices < 0) {
> +      _eglLog(_EGL_WARNING, "Unable to find DRM devices, error %d",
num_devices);
> +      return -1;
> +   }
> +
> +   if (num_devices == 0) {
> +      _eglLog(_EGL_WARNING, "Failed to find any DRM devices");
> +      return -1;
> +   }
> +
> +   for (int i = 0; i < num_devices; i++) {
> +      char *dev_path = devices[i]->nodes[DRM_NODE_RENDER];
> +      fd = loader_open_device(dev_path);
> +      if (fd == -1) {
> +         _eglLog(_EGL_WARNING, "%s() Failed to open DRM device %s",
> +                 __func__, dev_path);
> +         continue;
> +      }
> +
> +      if (!droid_probe_device(disp, fd, devices[i], vendor_name))
> +         goto next;
> +
> +      break;

Hmm.

Why not just

    if (droid_probe_device(disp, fd, devices[i], vendor_name))
       break;

?

But actually, we need to distinguish the cases of failure and simply render
node not matching any drivers. We shouldn't use such render node as
fallback.

    int ret = droid_probe_device(...);
    if (!ret)
       break; // Found desired render node.
    if (ret < 0)
       continue; // Did not match any drivers.
    // Matched a driver, but not our filter. Consider as fallback.

> +
> +next:
> +      if (fallback_fd == -1) {
> +         fallback_fd = fd;
> +         fd = -1;
> +      } else {
> +         close(fd);
> +         fd = -1;
> +      }

fd = -1; could be just put here, after the if.

> +      continue;

No need for this continue.

Best regards,
Tomasz