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

[Robert Foss]

Hey,

On 27/08/2018 11.47, Emil Velikov wrote:
> 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.

Upon re-reading this, I'm happy with the way it is.

Feel free to add my r-b with the above typo fixed.

> 
> -Emil
>