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

[Emil Velikov]

On 24 August 2018 at 18:55, Robert Foss <robert.foss at collabora.com> wrote:
> 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.
>
Thanks for having a look Rob. Will fix (plus second instance below).

>> +            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?
>
I think it matches with the comment - note the function returns an int
0 on success.

We could change that esp. since others - droid_load_driver return an EGLBoolean.
Alternatively we could use if (droid_probbe_device(disp) != 0)

I'm fine either way.

-Emil