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 <kenn...@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. */
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
