Hi Roland, On 26/01/18 13:57, Roland Scheidegger wrote:
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,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@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).
not the internalformat_query2 piglit test. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
