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

[Eric Engestrom]

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);
	         }
	#ifdef HAVE_DRI3
	      }
	#endif
	   } else {
	      initialized = dri2_initialize_x11_swrast(drv, disp);
	   }
	
	   return initialized;
	}

Let me rewrite that in plain english:
- if env doesn't say "always software":
  - if DRI3 is compiled in and env doesn't disable it, try dri3
  - if that failed (or was compiled out), try dri2
  - if that failed, try swrast
- otherwise, try swrast

New code:

	EGLBoolean
	dri2_initialize_x11(_EGLDriver *drv, _EGLDisplay *disp)
	{
	   EGLBoolean initialized = EGL_FALSE;
	
	   if (!getenv("LIBGL_ALWAYS_SOFTWARE")) {
	#ifdef HAVE_DRI3
	      if (!getenv("LIBGL_DRI3_DISABLE"))
	         initialized = dri2_initialize_x11_dri3(drv, disp);
	#endif
	
	      if (!initialized)
	         initialized = dri2_initialize_x11_dri2(drv, disp);
	   }
	
	   if (!initialized)
	      initialized = dri2_initialize_x11_swrast(drv, disp);
	
	   return initialized;
	}

Hopefully the same description applies as well, but with (I think)
a much nicer code, and the two swrast paths folded into one.


> 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?

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.)

Cheers,
  Eric

[1] src/egl/main/egldriver.c:294