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

[Chad Versace]

On Thu 08 Jun 2017, Rob Herring wrote:
> On Tue, Jun 6, 2017 at 3:37 PM, Chad Versace <chadversary at chromium.org> 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    | 24 ++++++++++++++++++++++--
> >  src/mesa/drivers/dri/i965/intel_screen.c | 23 ++++++++++++++++++++++-
> >  2 files changed, 44 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
> > index 16d1325736..e56d30a2c0 100644
> > --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> > +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> > @@ -34,6 +34,7 @@
> >  #include "main/teximage.h"
> >  #include "main/image.h"
> >  #include "main/condrender.h"
> > +#include "main/format_fallback.h"
> >  #include "util/hash_table.h"
> >  #include "util/set.h"
> >
> > @@ -450,10 +451,29 @@ intel_create_winsys_renderbuffer(struct intel_screen *screen,
> >
> >     _mesa_init_renderbuffer(rb, 0);
> >     rb->ClassID = INTEL_RB_CLASS;
> > +   rb->NumSamples = num_samples;
> > +
> > +   /* The base format and internal format must be derived from the user-visible
> > +    * format (that is, the gl_config's format), even if we internally use
> > +    * choose a different format for the renderbuffer. Otherwise, rendering may
> > +    * use incorrect channel write masks.
> > +    */
> >     rb->_BaseFormat = _mesa_get_format_base_format(format);
> > -   rb->Format = format;
> >     rb->InternalFormat = rb->_BaseFormat;
> > -   rb->NumSamples = num_samples;
> > +
> > +   rb->Format = format;
> > +   if (!screen->mesa_format_supports_render[rb->Format]) {
> > +      /* The glRenderbufferStorage paths in core Mesa detect if the driver
> > +       * does not support the user-requested format, and then searches for
> > +       * a falback format. The DRI code bypasses core Mesa, though. So we do
> > +       * the fallbacks here.
> > +       *
> > +       * We must support MESA_FORMAT_R8G8B8X8 on Android because the Android
> > +       * framework requires HAL_PIXEL_FORMAT_RGBX8888 winsys surfaces.
> > +       */
> > +      rb->Format = _mesa_format_fallback_rgbx_to_rgba(rb->Format);
> > +      assert(screen->mesa_format_supports_render[rb->Format]);
> > +   }
> >
> >     /* intel-specific methods */
> >     rb->Delete = intel_delete_renderbuffer;
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> > index 563065b91f..a9d132f868 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -1547,7 +1547,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.
> > +       *
> > +       * EGL does not suffer from this problem. It correctly compares the
> > +       * channel masks when matching EGLConfig to __DRIconfig.
> > +       */
> 
> When I originally added this for gallium (commit ccdcf91104a5f), I
> happened to add these formats to the beginning rather than end of the
> list. Is simply changing the order really enough to avoid the issue
> with GLX?

According to my experiments with i965, definitely yes.

When I place the new formats at the beginning of the array, the glxgears
colors are swapped. When I place the new formats at the end of the
array, the glxgears color are correct.

To understand why, I stepped through, with gdb, the mesa:src/glx code
that associates X visuals to DRI configs. I don't claim to fully
understand how that code works, but I felt confident enough to write the
above comment and claim that GLX ignores the new formats, as long as
they occur at the end.