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

[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@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.

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev