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

Tue Feb 20 14:16:14 UTC 2018

On 26/01/18 11:31, Antia Puentes wrote:
> 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.

This was discussed in 
https://github.com/KhronosGroup/OpenGL-API/issues/32 and the conclusion 
is that the format
is defined as color-renderable when EXT_texture_shared_exponent is 
exposed, it would fail later during the framebuffer
completeness check for implementations which do not support rendering to 
it (FRAMEBUFFER_UNSUPPORTED).

If there are no further comments, I would reapply this patch by the end 
of the week,
adding the following wording to the commit message:

     RGB9_E5 should be accepted by RenderbufferStorage if the
     EXT_texture_shared_exponent is exposed. It is left to the
     implementations to return GL_FRAMEBUFFER_UNSUPPORTED_EXT
     when checking the framebuffer completeness if they do not
     support rendering in this format.

     Discussed in:
     https://github.com/KhronosGroup/OpenGL-API/issues/32

I have sent a patch to Piglit to take the clarified behaviour into 
account in the ARB_internalformat_query(2) tests.

Sorry for the noise.