On Thu 08 Jun 2017, Rob Herring wrote: > On Tue, Jun 6, 2017 at 3:37 PM, Chad Versace <chadvers...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev