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

[Emil Velikov]

On 6 June 2017 at 21:37, 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.
> +       */
> +
Huge thank you for the comprehensive comment Chad. It will raise quite
a few eyebrows should someone consider touching any of it :-)
Guess as we add support for RGBX/RGBA on !Android EGL, the comment may
need a tweak. But that's can follow in due time.

Thanks again!
Emil
P.S. /me wonders why src/glx does not bother with any masks. Not even
during comparison...