On 6 June 2017 at 21:37, 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.
> + */
> +
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...
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
