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

[Emil Velikov]

On 27 June 2017 at 20:43, Kenneth Graunke <kenneth at whitecape.org> 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(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
>> index caf182ca8b7..b59fc1fc7d0 100644
>> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
>> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
>> @@ -450,10 +450,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 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 <kenneth at whitecape.org>
>
Fwiw form me
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

> But, I still think this should be fixed.  This seems fragile - we're
> essentially relying on a bug in the GLX code.
>
I have some patches that add MASK checking/parsing in libGL (it's
completely ignored since code was introduced) analogous to libEGL.
Although will need to check if X needs similar fix.

> 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?
>
FWIW I wasn't too excited about adding more Android workarounds, since
nobody was addressing the existing ones :-(
With handful of those fixed (thanks Rob!) - feel free to add a guard.

> 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?
>
Yes it did - it's a common misuse of {egl/glX}ChooseConfig + pick the first one.

On second though - one may be able to address in the Android frontend
library. It can call the vendor library, fetching all the configs,
sort, filter + apply other heuristics as needed and then feed the
result back to the user. But that for another day.

Regards,
Emil