[Mesa-dev] [PATCH] i915: Add XRGB8888 format to intel_screen_make_configs

[Derek Foreman]

On 24/11/16 08:53 AM, Emil Velikov wrote:
> On 24 November 2016 at 06:22, Boyan Ding <boyan.j.ding at gmail.com> wrote:
>> 2016-11-24 13:29 GMT+08:00 Derek Foreman <derekf at osg.samsung.com>:
>>> On 23/11/16 07:18 PM, Boyan Ding wrote:
>>>>
>>>> 2016-11-24 7:01 GMT+08:00 Derek Foreman <derekf at osg.samsung.com>:
>>>>>
>>>>> This is a copy of commit 536003c11e4cb1172c540932ce3cce06f03bf44e
>>>>> except for i915.
>>>>>
>>>>> Original log for the i965 commit follows:
>>>>>
>>>>>  Some application, such as drm backend of weston, uses XRGB8888 config as
>>>>>  default. i965 doesn't provide this format, but before commit 65c8965d,
>>>>>  the drm platform of EGL takes ARGB8888 as XRGB8888. Now that commit
>>>>>  65c8965d makes EGL recognize format correctly so weston won't start
>>>>>  because it can't find XRGB8888. Add XRGB8888 format to i965 just as
>>>>>  other drivers do.
>>>>>
>>>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>>>> ---
>>>>>  src/mesa/drivers/dri/i915/intel_screen.c | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/mesa/drivers/dri/i915/intel_screen.c
>>>>> b/src/mesa/drivers/dri/i915/intel_screen.c
>>>>> index 1b80df0..5c7c06a 100644
>>>>> --- a/src/mesa/drivers/dri/i915/intel_screen.c
>>>>> +++ b/src/mesa/drivers/dri/i915/intel_screen.c
>>>>> @@ -1044,7 +1044,8 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
>>>>>  {
>>>>>     static const mesa_format formats[] = {
>>>>>        MESA_FORMAT_B5G6R5_UNORM,
>>>>> -      MESA_FORMAT_B8G8R8A8_UNORM
>>>>> +      MESA_FORMAT_B8G8R8A8_UNORM,
>>>>> +      MESA_FORMAT_B8G8R8X8_UNORM
>>>>>     };
>>>>>
>>>>>     /* GLX_SWAP_COPY_OML is not supported due to page flipping. */
>>>>> --
>>>>> 2.10.2
>>>>>
>>>>
>>>> Hi Derek,
>>>>
>>>> I sent exactly the same patch one and half years ago at [1], but
>>>> withdrew it because it seems no one got interested in that and I don't
>>>> have the hardware to test. If you're sure it is correct, this gets my
>>>>
>>>> Acked-by: Boyan Ding <boyan.j.ding at gmail.com>
>>>
>>>
>>> I'm sorry, I didn't see your patch.  It makes more sense to me that I give
>>> you my RB on that patch in place of your Ack on mine.  I don't want to take
>>> credit for a problem you solved over a year ago. :)
>>>
>>> I don't have appropriate hardware but this has been tested for me by an
>>> Enlightenment user who was unable to use our GL backend because it's trying
>>> to use XRGB.
>>>
>>> Weston ran for him but logged a warning about falling back to an ARGB
>>> visual, which led me to the discovery that this had only been changed for
>>> i965.
>>>
>>> I'll try to get him to reply with a "Tested-by" tomorrow.
>>>
>>>
>>
>> Thanks for the kind reply, but I wonder if a more thorough testing
>> should be done before applying this. I remember its i965 counterpart
>> (commit 28090b30d) did cause some problem (Bug 90791) although it seems
>> not the fault on its own.
>>
>> We're facing the same problem I faced last year when posting this
>> patch -- I didn't have the authority to say that it was okay, and I
>> didn't have the hardware to test on as Emil once suggested[1].
>>
> Precisely - the patch on it's own was perfectly reasonable but it will
> likely cause issues like i965 one.

Yikes, that's a long list of side effects.  This is something I can work 
around in enlightenment anyway, by doing the same manner of fallback 
weston does (fall back to an ARGB visual if an XRGB is unavailable).

Please let me know if it's preferred that I just do it there instead.

Thanks for taking to time to compile that list.
Derek

> Derek, skimming through the i965 history a very lengthy list of
> regressions/fixes comes up[1] as the format was advertised as
> supported.
>
> Considering we don't get much testing, it would be great to avoid
> breaking the world by fixing EGL/drm users (Wayland compositors,
> other).
>
> Ian, Mark Janes,
> Gents, do you have the hardware/chance to test this patch ?
>
> Thanks
> Emil
>
> [1]
> 28090b30dd6b5977de085f48c620574214b6b4ba i965: Add XRGB8888 format to
> intel_screen_make_configs
> * Adds the format since things broke as EGL/drm was fixed to correctly
> honour the formats commit 65c8965d033 (egl: Take alpha bits into
> account when selecting GBM formats)
>
> 8da79b8378ae87474d8c47ad955e4833edf98359 i965: Fix HW blitter pitch limits
> c2d0606827412b710dcaed80268fc665de8c9c5d i915: Blit RGBX<->RGBA
> drawpixels // should read i965
> 922c0c9fd526ce19b87bc74a3159dec7705c1de1 i965: Export format
> comparison for blitting between miptrees
> * Fixes serious performance issue due to the extra format.
>
> bd38f91f8d80897ca91979962d80d4bc0acef586 i965: do_blit_drawpixels:
> decode array formats
> * Fixes the last commit above
>
> 2cebaac479d49cd6df4e97b466ba14bab3f30db1 i965: Don't use tiled_memcpy
> to download from RGBX or BGRX surfaces
> * Disable those since it causes regressions
>
> 3f10774cbab1e803f8aa3d6d24f8f6f98b8128c3 i965: Check base format to
> determine whether to use tiled memcpy
> * Aims to rework/fix the above.
>
> c769efda939e06338d41e1046a5f954c690951d5 i965: Add
> MESA_FORMAT_B8G8R8X8_SRGB to brw_format_for_mesa_format
> 43f4be5f06b7a96b96a3a7b43f5112139a1f423a i965: Add B8G8R8X8_SRGB to
> the alpha format override
> 839793680f99b8387bee9489733d5071c10f3ace i965: Use
> MESA_FORMAT_B8G8R8X8_SRGB for RGB visuals
> * Aim to rework/fix the mesa <> hardware format mappings as a side
> effect of the new XRGB8888 advertised
>
> 61cdb7665f7bd147533cdc5977750d990c2eafd5 Revert "i965: Use
> MESA_FORMAT_B8G8R8X8_SRGB for RGB visuals"
> * Revert the last one since it breaks kde/kwin (and others iirc)
>
> 35ade36c88e5aaa0b18c3cc911d9a4de3a60a80b dri/i965: fix incorrect
> rgbFormat in intelCreateBuffer().
> * Correctly select the correct (mesa) format based on the loader
> request -> UNORM vs SRGB, XRGB vs ARGB and *RGB vs *BGR
>