Hi Emil,
On Fri, Aug 24, 2018 at 9:25 PM Emil Velikov <emil.l.veli...@gmail.com> wrote:
>
> From: Emil Velikov <emil.veli...@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.f...@collabora.com>
> Cc: Tomasz Figa <tf...@chromium.org>
> Signed-off-by: Emil Velikov <emil.veli...@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?
> + 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?
> + 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: droid_probe_device
> + close(fd);
> + fd = -1;
> + }
> + break;
> + }
> + /* No explicit request - attempt the next device */
> + if (droid_probbe_device(disp)) {
typo: droid_probe_device
> + 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;
}
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
