Bug 91596 - EGL_KHR_gl_colorspace (v2) causes problem with Android-x86 GUI
Summary: EGL_KHR_gl_colorspace (v2) causes problem with Android-x86 GUI
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: EGL (show other bugs)
Version: git
Hardware: All other
: medium critical
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-10 10:12 UTC by Mauro Rossi
Modified: 2015-10-27 08:44 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
logcat, NOTE: SEGFAULTS are unrelated to GUI flashes and independent (1.61 MB, text/plain)
2015-08-10 10:12 UTC, Mauro Rossi
Details
dmesg, NOTE: segfaults are due to an independent problem (88.67 KB, text/plain)
2015-08-10 10:15 UTC, Mauro Rossi
Details
dumpsys SurfaceFlinger (12.92 KB, text/plain)
2015-08-10 10:16 UTC, Mauro Rossi
Details

Description Mauro Rossi 2015-08-10 10:12:56 UTC
Created attachment 117607 [details]
logcat, NOTE: SEGFAULTS are unrelated to GUI flashes and independent

Hi,

while testing mesa 10.7.0devel/11.0.0 with android-x86 ISO builds,
I've encountered a problem, which is affecting Android-x86 GUI on both kitkat-x86 and lollipop-x86 and on different HW (Radeon, Nvidia, Intel).

The artifacts visible are:

- GUI/Backgroung elements flashing, with B/W very rapid changes (100-200 ms cycles)

- Old frame buffers polluting the composed GUI/screens, visible using the mouse wheel

Symptoms indicate that somehow the handles to the EGL buffers or their DRI equivalents may have problems.

After some checks on git on what could cause HW independend problems,
I've found the first commit that generates the problem:

commit c2c2e9ab604793c6e01f85497f3f5bf645f962fa

    egl: implement EGL_KHR_gl_colorspace (v2)
    
    v2: add missing "break"

NOTE: since Android-x86 require at least two patches to have working Intel and Nouveau gallium_dri, main git bisecting operations was done with Radeonsi driver on lollipop-x86, where I could build the untouched mesa 11.0.0 git. 

Mauro
Comment 1 Mauro Rossi 2015-08-10 10:15:32 UTC
Created attachment 117608 [details]
dmesg, NOTE: segfaults are due to an independent problem
Comment 2 Mauro Rossi 2015-08-10 10:16:21 UTC
Created attachment 117609 [details]
dumpsys SurfaceFlinger
Comment 3 Mauro Rossi 2015-08-11 20:24:14 UTC
Hi,

as a confirmation the described Android-x86 GUI problems are solved by reverting commit c2c2e9ab604793c6e01f85497f3f5bf645f962fa.

I am available if you need further information or to test upcoming patches to EGL_KHR_gl_colorspace code.

Mauro
Comment 4 Marek Olšák 2015-09-24 18:02:51 UTC
What happens if you hide the extension on Android?
Comment 5 Mauro Rossi 2015-09-27 13:18:56 UTC
Hi Makek,

I have performed test by disabling EGL_KHR_gl_colorspace extension,
Chih-Wei has tried with following override 

export MESA_EXTENSION_OVERRIDE "-EGL_KHR_gl_colorspace"

With both methods (to my knowledge they should be equivalent) problem was not solved

We came to the conclusion that c2c2e9a need to be reverted to have usable android-x86 GUI.

Looking at code before commit c2c2e9a, when conf->dri_single_config was defined, conf->dri_double_config was set to NULL and viceversa,
it's just a check/possible lead cause, because I'm not sure what the code actually does. 

Also I don't see inizialization of the new dri_srgb_single_config/dri_srgb_double_config but please pardon me, if this does not help.

       memcpy(&conf->base, &base, sizeof base);
       if (double_buffer) {
-         conf->dri_double_config = dri_config;
-         conf->dri_single_config = NULL;
+         if (srgb)
+            conf->dri_srgb_double_config = dri_config;
+         else
+            conf->dri_double_config = dri_config;
       } else {
-         conf->dri_single_config = dri_config;
-         conf->dri_double_config = NULL;
+         if (srgb)
+            conf->dri_srgb_single_config = dri_config;
+         else
+            conf->dri_single_config = dri_config;
       }
Comment 6 Mauro Rossi 2015-09-27 13:22:08 UTC
To be precise:

We came to the conclusion that c2c2e9a need to be reverted on android-x86 git external/mesa upcoming project in lollipop-x86 branch

Mauro
Comment 7 Marek Olšák 2015-09-28 22:46:36 UTC
MESA_EXTENSION_OVERRIDE won't work. You need to set KHR_gl_colorspace = false manually in the code.
Comment 8 Mauro Rossi 2015-10-01 21:45:51 UTC
Hi Ilia,

Sorry I noticed my previous sentence was not clear,
I have performed the test by disabling EGL_KHR_gl_colorspace, independently frome Chih-Wei attempt with MESA_EXTENSION_OVERRIDE, by applying the following changes that were suggested to me by Emil:

--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -586,7 +586,7 @@ dri2_setup_screen(_EGLDisplay *disp)

   if (dri2_renderer_query_integer(dri2_dpy,
                                   __DRI2_RENDERER_HAS_FRAMEBUFFER_SRGB))
-      disp->Extensions.KHR_gl_colorspace = EGL_TRUE;
+      disp->Extensions.KHR_gl_colorspace = EGL_FALSE;

   if (dri2_dpy->dri2 && dri2_dpy->dri2->base.version >= 3) {
      disp->Extensions.KHR_create_context = EGL_TRUE;

The result was still flashing/phantom elements in android GUI.
So there may be some other effect induced by c2c2e9a commit.

Are all the dri_single/dri_double srgb/non srgb correctly set?

What is strange is that all kind of chipsets are affected.

Mauro
Comment 9 Mauro Rossi 2015-10-01 21:48:34 UTC
>> Hi Ilia,

My greeting also to Ilia, but you are Marek
Sorry

Mauro
Comment 10 Emil Velikov 2015-10-01 23:09:06 UTC
In the few moments that android-x86 was up I've noticed a very interesting commit [1]. In case you don't have a local copy, it enables a workaround #10194508, as seen here [2].

Not sure if the android-x86 version has the extra qcom 'hacks' so it might be worth experimenting on the topic - adding/reverting the qcom commit, reverting the original patch [1], all that whist toggling on/off the extension ;-)


[1] http://git.android-x86.org/?p=platform/frameworks/native.git;a=commit;h=39aa8695464e496ec4f6d22e9af54457bcfdf4ae
[2] https://github.com/omnirom/android_frameworks_native/blob/HEAD/opengl/libs/EGL/eglApi.cpp#L431
Comment 11 Chih-Wei Huang 2015-10-02 18:41:38 UTC
(In reply to Emil Velikov from comment #10)
> In the few moments that android-x86 was up I've noticed a very interesting
> commit [1]. In case you don't have a local copy, it enables a workaround
> #10194508, as seen here [2].
> 
> Not sure if the android-x86 version has the extra qcom 'hacks' so it might
> be worth experimenting on the topic - adding/reverting the qcom commit,
> reverting the original patch [1], all that whist toggling on/off the
> extension ;-)
> 
> 
> [1]
> http://git.android-x86.org/?p=platform/frameworks/native.git;a=commit;
> h=39aa8695464e496ec4f6d22e9af54457bcfdf4ae
> [2]
> https://github.com/omnirom/android_frameworks_native/blob/HEAD/opengl/libs/
> EGL/eglApi.cpp#L431

OK, you pointed out an old workaround we applied long time ago.
Without it, we just got black screen on i915/i965 GPU and
garbled screen on radeon.
Credit to Chia-I Wu who found the workaround.
Otherwise android-x86 has already dead.

Actually I reverted that workaround each time I tried a new Mesa version.
However, it's no help. We still need the workaround.

I just tried to revert the workaround with the extension enabled/disabled.
Nothing changed. We still need the workaround.

Mesa still doesn't support PIXEL_FORMAT_RGBA_8888?
Comment 12 Ilia Mirkin 2015-10-02 19:54:14 UTC
Looking over that patch, I noticed this hunk:

       memcpy(&conf->base, &base, sizeof base);
       if (double_buffer) {
-         conf->dri_double_config = dri_config;
-         conf->dri_single_config = NULL;
+         if (srgb)
+            conf->dri_srgb_double_config = dri_config;
+         else
+            conf->dri_double_config = dri_config;
       } else {
-         conf->dri_single_config = dri_config;
-         conf->dri_double_config = NULL;
+         if (srgb)
+            conf->dri_srgb_single_config = dri_config;
+         else
+            conf->dri_single_config = dri_config;
       }

Previously the "other" configs would get cleared out whereas they aren't anymore.
Comment 13 Ilia Mirkin 2015-10-02 19:54:56 UTC
(In reply to Ilia Mirkin from comment #12)
> Looking over that patch, I noticed this hunk:
> 
>        memcpy(&conf->base, &base, sizeof base);
>        if (double_buffer) {
> -         conf->dri_double_config = dri_config;
> -         conf->dri_single_config = NULL;
> +         if (srgb)
> +            conf->dri_srgb_double_config = dri_config;
> +         else
> +            conf->dri_double_config = dri_config;
>        } else {
> -         conf->dri_single_config = dri_config;
> -         conf->dri_double_config = NULL;
> +         if (srgb)
> +            conf->dri_srgb_single_config = dri_config;
> +         else
> +            conf->dri_single_config = dri_config;
>        }
> 
> Previously the "other" configs would get cleared out whereas they aren't
> anymore.

Er nevermind. Before it was malloc'd but now it's calloc'd.
Comment 14 Emil Velikov 2015-10-02 22:03:12 UTC
(In reply to Ilia Mirkin from comment #13)
> Er nevermind. Before it was malloc'd but now it's calloc'd.

The most creepy part is that with the extension disabled, we can never get to the srgb configs, as GLColorspace will be LINEAR. Even if the user requests one, we'll error out.
Comment 15 Emil Velikov 2015-10-02 22:21:54 UTC
(In reply to Chih-Wei Huang from comment #11)
> Mesa still doesn't support PIXEL_FORMAT_RGBA_8888?

It should, depending on how exactly you define the formats. I've looked a while back for some android documentation but I came short. Have you seen any ?

Upon closer look the mappings in drm_format_from_hal and get_pipe_format look very funny.
Comment 16 Emil Velikov 2015-10-02 22:48:13 UTC
(In reply to Emil Velikov from comment #14)
> (In reply to Ilia Mirkin from comment #13)
> > Er nevermind. Before it was malloc'd but now it's calloc'd.
> 
> The most creepy part is that with the extension disabled, we can never get
> to the srgb configs, as GLColorspace will be LINEAR. Even if the user
> requests one, we'll error out.

Actually I stand corrected - if one disables the extension, we'll still expose/list the srgb configs. With this patch [1] + the one in comment 8 things should be back to normal. Now we need to figure out why things go crazy in the first place (hint check the format mappings in comment 15).

[1] http://patchwork.freedesktop.org/patch/60961/
Comment 17 Rob Clark 2015-10-04 17:42:11 UTC
(In reply to Emil Velikov from comment #15)
> (In reply to Chih-Wei Huang from comment #11)
> > Mesa still doesn't support PIXEL_FORMAT_RGBA_8888?
> 
> It should, depending on how exactly you define the formats. I've looked a
> while back for some android documentation but I came short. Have you seen
> any ?
> 
> Upon closer look the mappings in drm_format_from_hal and get_pipe_format
> look very funny.

fwiw, there is a bogus patch that needs to be reverted in drm_gralloc, see:

https://github.com/robclark/drm_gralloc/commit/34bf9d8566aaf97a0f2adb127836134078abd340
Comment 18 Varad Gautam 2015-10-06 16:30:11 UTC
Hello Mauro,

Could you try [1] on your setup? The flashing seems to go away with freedreno with the patch.

[1] http://patchwork.freedesktop.org/patch/61151/
Comment 19 Mauro Rossi 2015-10-07 00:31:43 UTC
Hi,

tested the patch by restoring conf->base.SurfaceType = 0;

it works on NV8x, HD7750 and Intel 946GZ, problem solved.

Thanks a lot.
Mauro


diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 461735f..2e68a65 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -312,6 +312,8 @@ dri2_add_config(_EGLDisplay *disp, const __DRIconfig *dri_config, int id,
          else
             conf->dri_single_config = dri_config;
       }
+
+      conf->base.SurfaceType = 0;  
       conf->base.ConfigID = config_id;
 
       _eglLinkConfig(&conf->base);
Comment 20 Chih-Wei Huang 2015-10-07 07:54:02 UTC
(In reply to Rob Clark from comment #17)
> (In reply to Emil Velikov from comment #15)
> > (In reply to Chih-Wei Huang from comment #11)
> > > Mesa still doesn't support PIXEL_FORMAT_RGBA_8888?
> > 
> > It should, depending on how exactly you define the formats. I've looked a

The android's definition.

> > while back for some android documentation but I came short. Have you seen
> > any ?

Please read about line 454
https://android.googlesource.com/platform/frameworks/native/+/android-5.1.1_r1/opengl/libs/EGL/eglApi.cpp

If WORKAROUND_BUG_10194508 is not enabled,
HAL_PIXEL_FORMAT_RGBA_8888 is chosen and then
we only got black screen since
SurfaceFlinger crashed due to no surface.

> > Upon closer look the mappings in drm_format_from_hal and get_pipe_format
> > look very funny.

What does it mean? What's funny?

> fwiw, there is a bogus patch that needs to be reverted in drm_gralloc, see:
> 
> https://github.com/robclark/drm_gralloc/commit/
> 34bf9d8566aaf97a0f2adb127836134078abd340

That's a try-and-error patch.
Without it we just got black screen for vmwgfx.
So what's your suggested solution?
Comment 21 Marek Olšák 2015-10-07 12:57:58 UTC
This shouldn't be closed, because the fix hasn't been committed yet.

(In reply to Mauro Rossi from comment #19)
> Hi,
> 
> tested the patch by restoring conf->base.SurfaceType = 0;
> 
> it works on NV8x, HD7750 and Intel 946GZ, problem solved.
> 
> Thanks a lot.
> Mauro
> 
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c
> b/src/egl/drivers/dri2/egl_dri2.c
> index 461735f..2e68a65 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -312,6 +312,8 @@ dri2_add_config(_EGLDisplay *disp, const __DRIconfig
> *dri_config, int id,
>           else
>              conf->dri_single_config = dri_config;
>        }
> +
> +      conf->base.SurfaceType = 0;  
>        conf->base.ConfigID = config_id;
>  
>        _eglLinkConfig(&conf->base);
Comment 22 Rob Clark 2015-10-07 17:28:55 UTC
(In reply to Chih-Wei Huang from comment #20)
> (In reply to Rob Clark from comment #17)
> > (In reply to Emil Velikov from comment #15)
> > > Upon closer look the mappings in drm_format_from_hal and get_pipe_format
> > > look very funny.
> 
> What does it mean? What's funny?

well, mapping both HAL_PIXEL_FORMAT_RGB_888 and HAL_PIXEL_FORMAT_BGRA_8888 to DRM_FORMAT_XRGB8888, for example..

tbh I'm not sure what "endianess" HAL_PIXEL_FORMAT_* thinks about, but if HAL_PIXEL_FORMAT_RGBA_8888 -> DRM_FORMAT_RGBA8888, then it seems odd that HAL_PIXEL_FORMAT_RGBX_8888 -> DRM_FORMAT_XBGR8888..

> > fwiw, there is a bogus patch that needs to be reverted in drm_gralloc, see:
> > 
> > https://github.com/robclark/drm_gralloc/commit/
> > 34bf9d8566aaf97a0f2adb127836134078abd340
> 
> That's a try-and-error patch.
> Without it we just got black screen for vmwgfx.
> So what's your suggested solution?

just revert the patch to the way it was before.  If you are still getting a black screen with current mesa then we should debug what is wrong in vmware gallium driver..
Comment 23 Chih-Wei Huang 2015-10-27 08:44:25 UTC
(In reply to Emil Velikov from comment #15)
> (In reply to Chih-Wei Huang from comment #11)
> > Mesa still doesn't support PIXEL_FORMAT_RGBA_8888?
> 
> It should, depending on how exactly you define the formats. I've looked a
> while back for some android documentation but I came short. Have you seen
> any ?


HAL_PIXEL_FORMAT_RGBA_8888 is mapped to
DRM_FORMAT_RGBA8888 in drm_galloc.
Doesn't it look good?

So whether if Mesa supports DRM_FORMAT_RGBA8888?

 
> Upon closer look the mappings in drm_format_from_hal and get_pipe_format
> look very funny.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.