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

[Emil Velikov]

On 24 November 2016 at 15:47, Derek Foreman <derekf at osg.samsung.com> wrote:
> 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.
>
There's no issues in Mark's testing, which is great. I'm still a bit
cautious about getting this in, so let's opt for the following:
Let's have this for 17.0.0 and if issues arise we can investigate or
simply revert if fixing it takes too long/is impossible.
How does that sound ?

In either case I'd suggest to adding a fallback in the affected application.

Thanks
Emil