[Mesa-dev] [PATCH v2] st/dri: add 32-bit RGBX/RGBA formats

[Rob Herring]

On Tue, Jul 25, 2017 at 10:16 PM, Chih-Wei Huang
<cwhuang at android-x86.org> wrote:
> 2017-07-26 1:24 GMT+08:00 Rob Herring <robh at kernel.org>:
>> On Tue, Jul 25, 2017 at 10:15 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> On 25 July 2017 at 03:46, Chih-Wei Huang <cwhuang at android-x86.org> wrote:
>>>> On Tue 11 Jul 2017, Rob Herring wrote:
>>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>>
>>>>>> Add support for 32-bit RGBX/RGBA formats which are required for Android.
>>>>>>
>>>>>> The original patch (commit ccdcf91104a5) was reverted (commit
>>>>>> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
>>>>>> on further investigation by Chad Versace, moving the RGBX/RGBA configs
>>>>>> to the end is enough to prevent breaking GLX.
>>>>>>
>>>>>> The handling of RGBA/RGBX in dri_fill_st_visual is a fix from Marek
>>>>>> Olšák.
>>>>>>
>>>>>> Cc: Eric Anholt <eric at anholt.net>
>>>>>> Cc: Chad Versace <chadversary at chromium.org>
>>>>>> Cc: Mauro Rossi <issor.oruam at gmail.com>
>>>>>> Reviewed-by: Marek Olšák <marek.olsak at amd.com>
>>>>>> Signed-off-by: Rob Herring <robh at kernel.org>
>>>>>> ---
>>>>
>>>> Hi Rob,
>>>> I'm testing this patch with your gbm_gralloc and mesa 17.1.5.
>>>> Before applying this patch, the SurfaceFlinger sees
>>>> the alpha=8 in RenderEngine::chooseEglConfig()
>>>>
>>> May want to check for patches in the Android EGL (meta) library.
>>> I think, in does/did have a handful of workarounds.
>>>
>>> Is the Android-x86 one in sync with the one RobH uses?
>>
>> I double checked and I get 8-8-8-8. I'm have HWC2 enabled and
>> SurfaceFlinger is unpatched master branch.
>
> Hmm, strange.
> Which hwcomposer branch did you use and
> what GPU did you test?

The hwc2 branch with virgl. There's also some kernel changes with
fence support for virgl needed[1].

> As I explained to you (in another private email),
> I'm testing QEMU x86 virgl with your branches:
>
> http://github.com/robherring/gbm_gralloc  (master branch)
> http://github.com/robherring/drm_hwcomposer (android-m branch)
>
> If I understand your code correctly, it supports
> only HWC1, no HWC2 yet.
> I accidentally set HWC2=true in the first time testing
> but it didn't work. Setting HWC2=false works.
> (on the other hand, the chromium upstream[1] does
> switch to HWC2 now, but I can't make it work yet)

Upstream does not yet support HWC2. The patches for support are under
review on Gerrit.

But I don't think this is a HWC1 vs. HWC2 issue. The EGL config code
is the same.

> About the client (SurfaceFlinger), I believe it requests
> HAL_PIXEL_FORMAT_RGBA_8888 but finally
> HAL_PIXEL_FORMAT_RGBX_8888 is used.

Actually, by default it does not request a specific format.

> The logic I saw is:
> 1. In SurfaceFlinger::init(), it calls RenderEngine::create() with
>     hwcFormat=HAL_PIXEL_FORMAT_RGBA_8888
>    (if HWC2 is used or if HWC1 api ver >= 1.1)
> 2. RenderEngine::create() calls RenderEngine::chooseEglConfig()
>    with format=HAL_PIXEL_FORMAT_RGBA_8888
> 3. chooseEglConfig() tries to find a config and prints
>    the info as we see in the logcat. With this patch,
>    the selected config is 8-8-8-0.

This first ignores the format and requests a config with all component
sizes equal to 8. If that fails, it falls back to the requested
format. If it falls back, you should see some messages about that.

> 4. RenderEngine::create() saves the config to mEGLConfig
>    by engine->setEGLHandles()
> 5. Later, new DisplayDevice::DisplayDevice() is called with
>    with the selected config (mRenderEngine->getEGLConfig()).
>    Then it calls eglCreateWindowSurface() with the config.
> 6. eglCreateWindowSurface() checks[2] the alpha attrib
>    of this config to decide which format to be used.
>    With this patch, alpha=0 so HAL_PIXEL_FORMAT_RGBX_8888
>    is used. (if alpha > 0, HAL_PIXEL_FORMAT_RGBA_8888 is used)
>
> The differences between you and me maybe:
> * AOSP: master vs 7.1.2
> * Mesa: master(?) vs 17.1.5
> * GPU: ?? vs virgl
>
> I think AOSP doesn't matter. I don't see explicit change
> of the above logic in the master branch.

I looked and don't see a change either.

> Does Mesa version matter? May I need more patches in 17.1.5?

I don't think so. I've carried the patch for some time. The main
change was just the config ordering.

> About GPU, did you test virgl and still see 8-8-8-8?
>
> About the problem of the change, it's not explicitly.
> But some apps require RGBA_8888 still can't work.
> An example is the screen capture apps like Screenshot touch[3].

Let me try that one.

> It complains the producer output format is RGBX_8888
> but RGBA_8888 is expected:
>
> 07-26 10:18:28.236  2602  2669 E ImageReader_JNI: Producer output
> buffer format: 0x2, ImageReader configured format: 0x1
>
> I expect the patch fix it but it's not.
>
> [1]: https://chromium.googlesource.com/chromiumos/drm_hwcomposer
> [2]: https://android.googlesource.com/platform/frameworks/native/+/master/opengl/libs/EGL/eglApi.cpp#484
> [3]: https://play.google.com/store/apps/details?id=com.mdiwebma.screenshot
>
>
> --
> Chih-Wei
> Android-x86 project
> http://www.android-x86.org