[Mesa-dev] [PATCH 3/3] i965/dri: Support R8G8B8A8 and R8G8B8X8 configs

Tue Jun 27 23:49:00 UTC 2017

Daniel, I have a question for you below.

On Tue 27 Jun 2017, Kenneth Graunke wrote:
> On Tuesday, June 27, 2017 11:00:48 AM PDT Chad Versace wrote:
> > The Android framework requires support for EGLConfigs with
> > HAL_PIXEL_FORMAT_RGBX_8888 and HAL_PIXEL_FORMAT_RGBA_8888.
> > 
> > Even though all RGBX formats are disabled on gen9 by
> > brw_surface_formats.c, the new configs work correctly on Broxton thanks
> > to _mesa_format_fallback_rgbx_to_rgba().
> > 
> > On GLX, this creates no new configs, and therefore breaks no existing
> > apps. See in-patch comments for explanation. I tested with glxinfo and
> > glxgears on Skylake.
> > 
> > On Wayland, this also creates no new configs, and therfore breaks no
> > existing apps. (I tested with mesa-demos' eglinfo and es2gears_wayland
> > on Skylake). The reason differs from GLX, though. In
> > dri2_wl_add_configs_for_visual(), the format table contains only
> > B8G8R8X8, B8G8R8A8, and B5G6B5; and dri2_add_config() correctly matches
> > EGLConfig to format by inspecting channel masks.
> > 
> > On Android, in Chrome OS, I tested this on a Broxton device. I confirmed
> > that the Google Play Store's EGLSurface used HAL_PIXEL_FORMAT_RGBA_8888,
> > and that an Asteroid game's EGLSurface used HAL_PIXEL_FORMAT_RGBX_8888.
> > Both apps worked well. (Disclaimer: I didn't test this patch on Android
> > with Mesa master. I backported this patch series to an older Android
> > branch).
> > ---
> >  src/mesa/drivers/dri/i965/intel_fbo.c    | 23 +++++++++++++++++++++--
> >  src/mesa/drivers/dri/i965/intel_screen.c | 23 ++++++++++++++++++++++-
> >  2 files changed, 43 insertions(+), 3 deletions(-)
> > 

[snip]

> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> > index 8621af26378..0e03c56cf88 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -1717,7 +1717,28 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
> >     static const mesa_format formats[] = {
> >        MESA_FORMAT_B5G6R5_UNORM,
> >        MESA_FORMAT_B8G8R8A8_UNORM,
> > -      MESA_FORMAT_B8G8R8X8_UNORM
> > +      MESA_FORMAT_B8G8R8X8_UNORM,
> > +
> > +      /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
> > +       * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
> > +       * server may disagree on which format the GLXFBConfig represents,
> > +       * resulting in swapped color channels.
> > +       *
> > +       * The problem, as of 2017-05-30:
> > +       * When matching a GLXFBConfig to a __DRIconfig, GLX ignores the channel
> > +       * order and chooses the first __DRIconfig with the expected channel
> > +       * sizes. Specifically, GLX compares the GLXFBConfig's and __DRIconfig's
> > +       * __DRI_ATTRIB_{CHANNEL}_SIZE but ignores __DRI_ATTRIB_{CHANNEL}_MASK.
> 
> The series is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks!

> But, I still think this should be fixed.  This seems fragile - we're
> essentially relying on a bug in the GLX code.

Yeah, I don't like it either. I was afraid to fix the GLX code
because I was afraid of accidentally breaking some unspecified behavior
that either the X server or GLX clients implicitly relied on. GLX is
full of historical mysteries like that.

> I suppose fixing it would cause GLX to begin exposing new RGBA visuals
> (in addition to BGRA).  I don't think we want to do that - I don't see much
> point outside of Android.  It's mostly harmless, except that changing the
> visuals ends up being absurdly painful for driver developers, since the X
> server and clients need to agree on lists.  That means you can't move across
> the change point without updating the Mesa your X server uses and restarting
> the X server.
> 
> If we do fix GLX, and want to avoid exposing visuals, I think we could
> easily add an #ifdef ANDROID.  Is there some reason to not go ahead and
> do that now?

For now, there's no reason to not #ifdef ANDROID the new config formats.
And I'll squash that into this patch if you ask.

But, I intentionally did not #ifdef ANDROID the new formats just in case
Wayland wanted to use them. Currently,
src/egl/drivers/dri/platform_wayland.c will not use them. But, I suspect
that Wayland would use them (but not require them) after Daniel Stone's
patches land, in series "EGL/Wayland modifiers, format cleanup".

^^^ Daniel, is that correct?

> Also...did we fix the visual ordering problem?  Earlier you mentioned that
> this patch wouldn't work for Android, because we needed to move the RGBA
> formats above the BGRA ones (but that wouldn't work for X).  But I think
> you said you had EGL patches which fixed that, and I'm pretty sure those
> landed.  So...we're good now?

It's all good now. Android visuals are in the correct order. See
commit 5e884353e64 egl/android: Change order of EGLConfig generation (v2).

> 
> > +       *
> > +       * EGL does not suffer from this problem. It correctly compares the
> > +       * channel masks when matching EGLConfig to __DRIconfig.
> > +       */
> > +
> > +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBA_8888. */
> > +      MESA_FORMAT_R8G8B8A8_UNORM,
> > +
> > +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBX_8888. */
> > +      MESA_FORMAT_R8G8B8X8_UNORM,
> >     };
> >  
> >     /* GLX_SWAP_COPY_OML is not supported due to page flipping. */