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

[Roland Scheidegger]

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).

Roland