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

[Emil Velikov]

On 29 August 2018 at 07:55, Tomasz Figa <tfiga at chromium.org> wrote:
> Hi Emil,
>
> On Fri, Aug 24, 2018 at 9:25 PM Emil Velikov <emil.l.velikov at gmail.com> 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 ;-)
>
> Yeah, it was surprisingly difficult to get it right and I missed that
> in the review. Thanks for figuring this out.
>
>> ---
>>  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");
>
> Failing to create a screen is probably critical enough to be worth a
> message, even if just probing, but failing to load a driver would like
> mean that there is no driver for the device, so perhaps we don't need
> to clutter the log in such case?
>
I'm fine either way - merely copying what everyone else does ;-)

>> +      return -1;
>> +   }
>> +
>> +   if (!dri2_create_screen(disp)) {
>> +      _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
>
> I don't remember if I got an answer to this question, sorry if I did:
>
> Is there really nothing to clean up after droid_load_driver()? Not
> leaking anything here, when probing though the subsequent devices?
>
No other driver does such handling so there's no separate helper.
I'll pull out the bits from dri2_display_destroy (a dlclose + free)
and into a preparatory patch, when sending the next revision.

>> +         close(fd);
>> +         fd = -1;
>>           continue;
>>        }
>> -      /* Found a device */
>>        break;
>
> A break at the end of a loop is really confusing to read. Could we
> rewrite the last block to avoid it? E.g.
>
>    if (!droid_probe_device(disp)) {
>       /* Found a device */
>       break;
>    }
>    /* No explicit request - attempt the next device. */
>    close(fd);
>    fd = -1;
> }
>
Sure thing.

Will try to send out next revision tomorrow.

Thanks
Emil