[Mesa-dev] [PATCH] egl/android: rework device probing

[Robert Foss]

Hey Emil,

On 24/08/2018 14.21, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Unlike the other platforms, here we aim do guess if the device that we
> somewhat arbitrarily picked, is supported or not.
> 
> In particular: when a vendor is _not_ requested we loop through all
> devices, picking the first one which can create a DRI screen.
> 
> When a vendor is requested - we use that and do _not_ fall-back to any
> other device.
> 
> The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
> EGL_MESA_query_renderer are MIA, this is the best we can do for the
> moment.
> 
> With those (proposed) extensions userspace will be able to create a
> separate EGL display for each device, query device details and make the
> conscious decision which one to use.
> 
> Cc: Robert Foss <robert.foss at collabora.com>
> Cc: Tomasz Figa <tfiga at chromium.org>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> Thanks for the clarification Tomasz. The original code was using a
> fall-back even a vendor was explicitly requested, confusing me a bit ;-)
> ---
>   src/egl/drivers/dri2/platform_android.c | 71 +++++++++++++++----------
>   1 file changed, 43 insertions(+), 28 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index 1f9fe27ab85..5bf627dec7d 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -1420,13 +1420,32 @@ droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
>      return 0;
>   }
>   
> +static int
> +droid_probe_device(_EGLDisplay *disp)
> +{
> +  /* Check that the device is supported, by attempting to:
> +   * - load the dri module
> +   * - and, create a screen
> +   */
> +   if (!droid_load_driver(disp)) {
> +      _eglLog(_EGL_WARNING, "DRI2: failed to load driver");
> +      return -1;
> +   }
> +
> +   if (!dri2_create_screen(disp)) {
> +      _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
> +      return -1;
> +   }
> +   return 0;
> +}
> +
>   static int
>   droid_open_device(_EGLDisplay *disp)
>   {
>   #define MAX_DRM_DEVICES 32
>      drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
>      int prop_set, num_devices;
> -   int fd = -1, fallback_fd = -1;
> +   int fd = -1;
>   
>      char *vendor_name = NULL;
>      char vendor_buf[PROPERTY_VALUE_MAX];
> @@ -1451,33 +1470,39 @@ droid_open_device(_EGLDisplay *disp)
>            continue;
>         }
>   
> -      if (vendor_name && droid_filter_device(disp, fd, vendor_name)) {
> -         /* Match requested, but not found - set as fallback */
> -         if (fallback_fd == -1) {
> -            fallback_fd = fd;
> -         } else {
> +      /* If a vendor is explicitly provided, we use only that.
> +       * Otherwise we fall-back the first device that is supported.
> +       */
> +      if (vendor_name) {
> +         if (droid_filter_device(disp, fd, vendor_name)) {
> +            /* Device does not match - try next device */
>               close(fd);
>               fd = -1;
> +            continue;
>            }
> -
> +         /* If the requested device matches use it, regardless if
> +          * init fails. Do not fall-back to any other device.
> +          */
> +         if (droid_probbe_device(disp)) {

Typo in function name.

> +            close(fd);
> +            fd = -1;
> +         }

Isn't the above comment saying that the if statement just below it shouldn't
be there? Or am I misparsing something?

> +         break;
> +      }
> +      /* No explicit request - attempt the next device */
> +      if (droid_probbe_device(disp)) {

Typo in function name.

> +         close(fd);
> +         fd = -1;
>            continue;
>         }
> -      /* Found a device */
>         break;
>      }
>      drmFreeDevices(devices, num_devices);
>   
> -   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;
> -   }
> +   if (fd < 0)
> +      _eglLog(_EGL_WARNING, "Failed to open %s DRM device",
> +            vendor_name ? "desired": "any");
>   
> -   close(fallback_fd);
>      return fd;
>   #undef MAX_DRM_DEVICES
>   }
> @@ -1519,16 +1544,6 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)
>         goto cleanup;
>      }
>   
> -   if (!droid_load_driver(disp)) {
> -      err = "DRI2: failed to load driver";
> -      goto cleanup;
> -   }
> -
> -   if (!dri2_create_screen(disp)) {
> -      err = "DRI2: failed to create screen";
> -      goto cleanup;
> -   }
> -
>      if (!dri2_setup_extensions(disp)) {
>         err = "DRI2: failed to setup extensions";
>         goto cleanup;
>