[Mesa-dev] [PATCH mesa] egl/x11: cleanup init code

[Emil Velikov]

On 13 December 2016 at 18:40, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> On Thursday, 2016-12-08 20:17:21 +0000, Emil Velikov wrote:
>> On 8 December 2016 at 00:30, Eric Engestrom <eric at engestrom.ch> wrote:
>> > No functional change, just rewriting it in an easier-to-understand way.
>> >
>> I might be mislead by the diff, but it seems that there's functionality change.
>
> Diffs are not always the easiest to read, let's see if code and english
> work better :)
>
>>
>> > Signed-off-by: Eric Engestrom <eric at engestrom.ch>
>> > ---
>> >  src/egl/drivers/dri2/platform_x11.c | 24 ++++++++++--------------
>> >  1 file changed, 10 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
>> > index df39ca8..db7d3b9 100644
>> > --- a/src/egl/drivers/dri2/platform_x11.c
>> > +++ b/src/egl/drivers/dri2/platform_x11.c
>> > @@ -1467,24 +1467,20 @@ dri2_initialize_x11_dri2(_EGLDriver *drv, _EGLDisplay *disp)
>> >  EGLBoolean
>> >  dri2_initialize_x11(_EGLDriver *drv, _EGLDisplay *disp)
>> >  {
>> > -   EGLBoolean initialized = EGL_TRUE;
>> > +   EGLBoolean initialized = EGL_FALSE;
>> >
>> > -   int x11_dri2_accel = (getenv("LIBGL_ALWAYS_SOFTWARE") == NULL);
>> > -
>> > -   if (x11_dri2_accel) {
>> > +   if (!getenv("LIBGL_ALWAYS_SOFTWARE")) {
>> >  #ifdef HAVE_DRI3
>> > -      if (getenv("LIBGL_DRI3_DISABLE") != NULL ||
>> > -          !dri2_initialize_x11_dri3(drv, disp)) {
>> > +      if (!getenv("LIBGL_DRI3_DISABLE"))
>> > +         initialized = dri2_initialize_x11_dri3(drv, disp);
>> >  #endif
>> > +
>> > +      if (!initialized)
>> > -         if (!dri2_initialize_x11_dri2(drv, disp)) {
>> > +         initialized = dri2_initialize_x11_dri2(drv, disp);
>> > -            initialized = dri2_initialize_x11_swrast(drv, disp);
>> > -         }
>> > -#ifdef HAVE_DRI3
>> > -      }
>> > -#endif
>> > -   } else {
>> > +   }
>> > +
>> > +   if (!initialized)
>> Namely here: by dropping the else, we fall-back to swrast if dri3/dri2 fails.
>
> The current code looks like this:
>
>         EGLBoolean
>         dri2_initialize_x11(_EGLDriver *drv, _EGLDisplay *disp)
>         {
>            EGLBoolean initialized = EGL_TRUE;
>
>            int x11_dri2_accel = (getenv("LIBGL_ALWAYS_SOFTWARE") == NULL);
>
>            if (x11_dri2_accel) {
>         #ifdef HAVE_DRI3
>               if (getenv("LIBGL_DRI3_DISABLE") != NULL ||
>                   !dri2_initialize_x11_dri3(drv, disp)) {
>         #endif
>                  if (!dri2_initialize_x11_dri2(drv, disp)) {
>                     initialized = dri2_initialize_x11_swrast(drv, disp);
Seems like I've missed this line in the original code.

Thanks for the patience and correction Eric ! R-b and pushed the patch.

>> Not too sure how much we should care bth, although I have been
>> wondering if we should not start honouring
>> _egl_display::Options::UseFallback.
>
> I didn't know about that option. The way I understand its usage, this
> would mean changing the last `if` (the swrast one) to:
>         if (!initialized && disp->Options.UseFallback)
>
> Is that correct?
>
I think it should be something like

if (UseFallback)
   return init_swrast();

if (!getenv(DRI3_DISABLE)
  initialized = init_dri3();

if (!initialized)
  initialized = init_dri2();

return initialized;

> Although if I understand [1] correctly, the only effect this would have
> would be to always call init() once for dri3 then dri2, then if that
> failed dri3 again, dri2 again and then swrast...
>
> (I could also have the first `if` include a `!disp->Options.UseFallback`
> if that helps.)
>
With my understanding (above) one will get direct, explicit control
and relation between the main code and the backend.

At the same time not sure how much one should care, so...
Emil