[Mesa-dev] [PATCH] mesa: add missing RGB9_E5 format in _mesa_base_fbo_format

Fri Jan 26 14:13:48 UTC 2018

On 26/01/18 14:19, Roland Scheidegger wrote:
> Am 26.01.2018 um 14:13 schrieb Antia Puentes:
>> Hi Roland,
>>
>> On 26/01/18 13:57, Roland Scheidegger wrote:
>>> Am 26.01.2018 um 11:31 schrieb Antia Puentes:
>>>> On 25/01/18 18:56, Roland Scheidegger wrote:
>>>>
>>>>> Am 25.01.2018 um 17:56 schrieb Roland Scheidegger:
>>>>>> Am 25.01.2018 um 16:30 schrieb Michel Dänzer:
>>>>>>> On 2018-01-24 05:38 PM, Juan A. Suarez Romero wrote:
>>>>>>>> This fixes KHR-GL45.internalformat.renderbuffer.rgb9_e5.
>>>>>>>> ---
>>>>>>>>     src/mesa/main/fbobject.c | 3 +++
>>>>>>>>     1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>>>>>>>> index d23916d1ad7..c72204e11a0 100644
>>>>>>>> --- a/src/mesa/main/fbobject.c
>>>>>>>> +++ b/src/mesa/main/fbobject.c
>>>>>>>> @@ -1976,6 +1976,9 @@ _mesa_base_fbo_format(const struct gl_context
>>>>>>>> *ctx, GLenum internalFormat)
>>>>>>>>                    ctx->Extensions.ARB_texture_float) ||
>>>>>>>>                   _mesa_is_gles3(ctx) /* EXT_color_buffer_float */ )
>>>>>>>>              ? GL_RGBA : 0;
>>>>>>>> +   case GL_RGB9_E5:
>>>>>>>> +      return (_mesa_is_desktop_gl(ctx) &&
>>>>>>>> ctx->Extensions.EXT_texture_shared_exponent)
>>>>>>>> +         ? GL_RGB: 0;
>>>>>>>>        case GL_ALPHA16F_ARB:
>>>>>>>>        case GL_ALPHA32F_ARB:
>>>>>>>>           return ctx->API == API_OPENGL_COMPAT &&
>>>>>>>>
>>>>>>> Unfortunately, this broke the "spec at arb_internalformat_query2@samples
>>>>>>> and num_sample_counts pname checks" piglit tests with radeonsi and
>>>>>>> llvmpipe, see below.
>>>>>>>
>>>>>>> Any idea what might need to be done in Gallium to fix this?
>>>>>>>
>>>>>>>
>>>>>>>        32 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
>>>>>>> GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        32 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        32 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
>>>>>>> params[0] = (1,GL_TRUE), supported=1
>>>>>>>        64 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
>>>>>>> GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        64 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        64 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
>>>>>>> params[0] = (1,GL_TRUE), supported=1
>>>>>>> PIGLIT: {"subtest": {"GL_NUM_SAMPLE_COUNTS" : "fail"}}
>>>>>>>        32 bit failing case: pname = GL_SAMPLES, target =
>>>>>>> GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        32 bit failing case: pname = GL_SAMPLES, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        32 bit failing case: pname = GL_SAMPLES, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
>>>>>>> params[0] = (1,GL_TRUE), supported=1
>>>>>>>        64 bit failing case: pname = GL_SAMPLES, target =
>>>>>>> GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        64 bit failing case: pname = GL_SAMPLES, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
>>>>>>> (1,GL_TRUE), supported=1
>>>>>>>        64 bit failing case: pname = GL_SAMPLES, target =
>>>>>>> GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
>>>>>>> params[0] = (1,GL_TRUE), supported=1
>>>>>>> PIGLIT: {"subtest": {"GL_SAMPLES" : "fail"}}
>>>>>>> PIGLIT: {"result": "fail" }
>>>>>>>
>>>>>>>
>>>>>> Purely coincidentally, I was trying to clean up the formatquery code
>>>>>> recently (should help some failures with r600 too), and I think these
>>>>>> cleanups would fix it.
>>>>>> Basically outright say "no" to target/pname combinations which don't
>>>>>> make sense rather than trying to find a format suitable for another
>>>>>> target and then asking the driver for the nonsense combination, plus
>>>>>> some other small bits like not validating things again (sometimes, a
>>>>>> third time...).
>>>>>> Albeit it will cause some breakage with the piglit test, which I
>>>>>> believe
>>>>>> is a test error, but that might be open for debate...
>>>>>> (For TEXTURE_BUFFER and the internalformat size/type queries, do you
>>>>>> return valid values or unsupported? The problem here is ARB_tbo
>>>>>> says you
>>>>>> can't get these values via the equivalent GetTexLevelParameter
>>>>>> queries,
>>>>>> whereas with GL 3.1 you can. And internalformat_query2 says it returns
>>>>>> "the same information" as GetTexLevelParameter, albeit it's not
>>>>>> entirely
>>>>>> true in any case since the equivalent of the internalformat stencil
>>>>>> type
>>>>>> doesn't even exist. My stance would be that valid values should be
>>>>>> reported even without GL 3.1, but the piglit test thinks differently.)
>>>>>>
>>>>> Err, actually this won't fix it I suppose - because rgb9e5 now is a
>>>>> valid fbo format. Was that commit really correct? It does not make
>>>>> sense
>>>>> to me, rgb9e5 cannot be a fbo/renderable format. Or was this just
>>>>> working around issues in formatquery.c (which I try to address with
>>>>> this
>>>>> patch)?
>>>>>
>>>> I believe the commit is wrong, _mesa_base_fbo_format_ is used to
>>>> validate the internalformat
>>>> passed to RenderbufferStorage, which in the OpenGL 4.6 is said that
>>>> needs to be:
>>>>
>>>> An INVALID_ENUM error is generated if internalformat is not one of the
>>>> color-renderable, depth-renderable, or stencil-renderable formats
>>>> defined in
>>>> section 9.4.
>>>>
>>>> and that's exactly the error returned by the "renderbuffer_storage"
>>>> function if _mesa_base_fbo_format
>>>> returns 0.
>>>>
>>>> RGB9_E5 is not renderable, that is stated in the same specification (Bug
>>>> 9338). Juan, I believe the CTS test is
>>>> wrong and this commit should be reverted.
>>>>
>>> FWIW (and ignoring what the test actually does) I'm not sure the test is
>>> really wrong, there may well be a bug in the internalformat query
>>> implementation. The problem I think is that it will report
>>> RGB9E5/RENDERBUFFER as supported (unless the hw driver disagrees), due
>>> to mix and match of fbo and tex formats, but then if you ask for the
>>> internalformat sizes etc. it will come back as 0 since then it will only
>>> consider the fbo formats. This is what I try to address (among others).
>> My comment was referring to the CTS test Juan was trying to fix with the
>> commit,
>> KHR-GL4*.internalformat.renderbuffer.rgb9_e5, that is the one I believe
>> is wrong,
>> not  the internalformat_query2 piglit test.
>>
> Doesn't that use internalforamt query (the first version) too? I would
> think it might trip up on such issues as well.

It does not. You can find the code here:
https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/openglcts/modules/common/glcInternalformatTests.cpp#L1019

There are several fixes to the test pending to be reviewed, but basically that is the code. FWIW, I have filled a bug to remove the RGB9_E5 format from the test.

Regards.