[Mesa-dev] [PATCH 6/5] egl/android: continue to next device if dri2_create_screen fails

[Tomasz Figa]

On Thu, Aug 23, 2018 at 1:44 AM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> Hi Tomasz,
>
> On 21 August 2018 at 14:54, Tomasz Figa <tfiga at chromium.org> wrote:
> > Hi Emil,
> >
> > On Tue, Aug 14, 2018 at 2:05 AM 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.
> >>
> >> It 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>
> >> ---
> >>  src/egl/drivers/dri2/platform_android.c | 29 ++++++++++++++++---------
> >>  1 file changed, 19 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> >> index 50dd7a5e1b4..cac59847b89 100644
> >> --- a/src/egl/drivers/dri2/platform_android.c
> >> +++ b/src/egl/drivers/dri2/platform_android.c
> >> @@ -1295,6 +1295,25 @@ droid_open_device(_EGLDisplay *disp)
> >>           continue;
> >>        }
> >>        /* Found a device */
> >> +
> >> +      /* 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");
> >> +         close(fd);
> >> +         fd = -1;
> >> +         continue;
> >> +      }
> >> +
> >> +      if (!dri2_create_screen(disp)) {
> >> +         _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
> >> +         close(fd);
> >> +         fd = -1;
> >> +         continue;
> >> +      }
> >
> > Don't we also need to do these tests when determining if the device is
> > a suitable fallback? The fallback fd is set much earlier, in the same
> > block as the continue statement, so the code below wouldn't execute.
> >
> Let me see if I got this correctly:
>  - when a "vendor" is requested we use that, falling back to the first
> other driver where screen creation succeeds
>  - if one isn't requested, we pick the first device that can create a screen
>
> Is that right?

Yes, seems to match my idea.

Just to make sure we're not going to be masking some failures with
fallbacks, I think we should make sure that if "vendor" is requested
and found, we don't fallback, even if the matched device fails to
create a screen.

Best regards,
Tomasz