On Aug 24, 2015 1:25 AM, "Tapani PÃ¤lli" <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>> wrote: > > > > On 08/24/2015 08:17 AM, Ilia Mirkin wrote: >> >> >> On Aug 24, 2015 1:07 AM, "Tapani PÃ¤lli" <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a> >> <mailto:<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>>> wrote: >> Â > >> Â > >> Â > >> Â > On 08/21/2015 05:14 PM, Ilia Mirkin wrote: >> Â >> >> Â >> On Fri, Aug 21, 2015 at 3:22 AM, Tapani PÃ¤lli >> <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a> <mailto:<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>>> wrote: >> Â >>> >> Â >>> Patch refactors existing parameters check to first check common enums >> Â >>> between desktop GL and GLES 3.1 and modifies >> get_tex_level_parameter_image >> Â >>> to be compatible with enums specified in 3.1. >> Â >>> >> Â >>> Signed-off-by: Tapani PÃ¤lli <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a> >> <mailto:<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>>> >> >> Â >>> --- >> Â >>>Â Â src/mesa/main/texparam.c | 34 +++++++++++++++++++++++----------- >> Â >>>Â Â 1 file changed, 23 insertions(+), 11 deletions(-) >> Â >>> >> Â >>> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c >> Â >>> index 16739f1..947a2a1 100644 >> Â >>> --- a/src/mesa/main/texparam.c >> Â >>> +++ b/src/mesa/main/texparam.c >> Â >>> @@ -1208,20 +1208,34 @@ static GLboolean >> Â >>>Â Â legal_get_tex_level_parameter_target(struct gl_context *ctx, >> GLenum target, >> Â >>>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool dsa) >> Â >>>Â Â { >> Â >>> +Â Â /* Common targets for desktop GL and GLES 3.1. */ >> Â >>>Â Â Â switch (target) { >> Â >>> -Â Â case GL_TEXTURE_1D: >> Â >>> -Â Â case GL_PROXY_TEXTURE_1D: >> Â >>>Â Â Â case GL_TEXTURE_2D: >> Â >>> -Â Â case GL_PROXY_TEXTURE_2D: >> Â >>>Â Â Â case GL_TEXTURE_3D: >> Â >>> -Â Â case GL_PROXY_TEXTURE_3D: >> Â >>>Â Â Â Â Â return GL_TRUE; >> Â >>> +Â Â case GL_TEXTURE_2D_ARRAY_EXT: >> Â >>> +Â Â Â return (_mesa_is_gles31(ctx) || >> ctx->Extensions.EXT_texture_array); >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_POSITIVE_X_ARB: >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_NEGATIVE_X_ARB: >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_POSITIVE_Y_ARB: >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_NEGATIVE_Y_ARB: >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_POSITIVE_Z_ARB: >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_NEGATIVE_Z_ARB: >> Â >>> +Â Â Â return (_mesa_is_gles31(ctx) || >> ctx->Extensions.ARB_texture_cube_map); >> Â >>> +Â Â case GL_TEXTURE_2D_MULTISAMPLE: >> Â >>> +Â Â Â return (_mesa_is_gles31(ctx) || >> ctx->Extensions.ARB_texture_multisample); >> Â >>> +Â Â } >> Â >>> + >> Â >>> +Â Â if (!_mesa_is_desktop_gl(ctx)) >> Â >>> +Â Â Â return GL_FALSE; >> Â >>> + >> Â >>> +Â Â /* Rest of the desktop GL targets. */ >> Â >>> +Â Â switch (target) { >> Â >>> +Â Â case GL_TEXTURE_1D: >> Â >>> +Â Â case GL_PROXY_TEXTURE_1D: >> Â >>> +Â Â case GL_PROXY_TEXTURE_2D: >> Â >>> +Â Â case GL_PROXY_TEXTURE_3D: >> Â >>> +Â Â Â return GL_TRUE; >> Â >>>Â Â Â case GL_PROXY_TEXTURE_CUBE_MAP_ARB: >> Â >>>Â Â Â Â Â return ctx->Extensions.ARB_texture_cube_map; >> Â >>>Â Â Â case GL_TEXTURE_CUBE_MAP_ARRAY_ARB: >> Â >>> @@ -1232,7 +1246,6 @@ legal_get_tex_level_parameter_target(struct >> gl_context *ctx, GLenum target, >> Â >>>Â Â Â Â Â return ctx->Extensions.NV_texture_rectangle; >> Â >>>Â Â Â case GL_TEXTURE_1D_ARRAY_EXT: >> Â >>>Â Â Â case GL_PROXY_TEXTURE_1D_ARRAY_EXT: >> Â >>> -Â Â case GL_TEXTURE_2D_ARRAY_EXT: >> Â >>>Â Â Â case GL_PROXY_TEXTURE_2D_ARRAY_EXT: >> Â >>>Â Â Â Â Â return ctx->Extensions.EXT_texture_array; >> Â >>>Â Â Â case GL_TEXTURE_BUFFER: >> Â >>> @@ -1254,7 +1267,6 @@ legal_get_tex_level_parameter_target(struct >> gl_context *ctx, GLenum target, >> Â >>>Â Â Â Â Â * "target may also be TEXTURE_BUFFER, indicating the >> texture buffer." >> Â >>>Â Â Â Â Â */ >> Â >>>Â Â Â Â Â return ctx->API == API_OPENGL_CORE && ctx->Version >= 31; >> Â >>> -Â Â case GL_TEXTURE_2D_MULTISAMPLE: >> Â >>>Â Â Â case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: >> Â >>>Â Â Â case GL_PROXY_TEXTURE_2D_MULTISAMPLE: >> Â >>>Â Â Â case GL_PROXY_TEXTURE_2D_MULTISAMPLE_ARRAY: >> Â >>> @@ -1381,8 +1393,8 @@ get_tex_level_parameter_image(struct >> gl_context *ctx, >> Â >>>Â Â Â Â Â Â *params = _mesa_get_format_bits(texFormat, pname); >> Â >>>Â Â Â Â Â Â break; >> Â >>>Â Â Â Â Â case GL_TEXTURE_SHARED_SIZE: >> Â >>> -Â Â Â Â Â if (ctx->Version < 30 && >> Â >>> -Â Â Â Â Â Â Â !ctx->Extensions.EXT_texture_shared_exponent) >> Â >>> +Â Â Â Â Â if (!_mesa_is_gles31(ctx) && (ctx->Version < 30 && >> Â >>> +Â Â Â Â Â Â Â !ctx->Extensions.EXT_texture_shared_exponent)) >> Â >>>Â Â Â Â Â Â Â Â goto invalid_pname; >> Â >>>Â Â Â Â Â Â *params = texFormat == MESA_FORMAT_R9G9B9E5_FLOAT ? 5 : 0; >> Â >>>Â Â Â Â Â Â break; >> Â >>> @@ -1415,7 +1427,7 @@ get_tex_level_parameter_image(struct >> gl_context *ctx, >> Â >>>Â Â Â Â Â case GL_TEXTURE_BLUE_TYPE_ARB: >> Â >>>Â Â Â Â Â case GL_TEXTURE_ALPHA_TYPE_ARB: >> Â >>>Â Â Â Â Â case GL_TEXTURE_DEPTH_TYPE_ARB: >> Â >>> -Â Â Â Â Â if (!ctx->Extensions.ARB_texture_float) >> Â >>> +Â Â Â Â Â if (!_mesa_is_gles31(ctx) && >> !ctx->Extensions.ARB_texture_float) >> Â >>>Â Â Â Â Â Â Â Â goto invalid_pname; >> Â >>>Â Â Â Â Â Â if (_mesa_base_format_has_channel(img->_BaseFormat, pname)) >> Â >>>Â Â Â Â Â Â Â *params = _mesa_get_format_datatype(texFormat); >> Â >>> @@ -1425,13 +1437,13 @@ get_tex_level_parameter_image(struct >> gl_context *ctx, >> Â >>> >> Â >>>Â Â Â Â Â /* GL_ARB_texture_multisample */ >> Â >>>Â Â Â Â Â case GL_TEXTURE_SAMPLES: >> Â >>> -Â Â Â Â Â if (!ctx->Extensions.ARB_texture_multisample) >> Â >>> +Â Â Â Â Â if (!_mesa_is_gles31(ctx) && >> !ctx->Extensions.ARB_texture_multisample) >> Â >>>Â Â Â Â Â Â Â Â goto invalid_pname; >> Â >>>Â Â Â Â Â Â *params = img->NumSamples; >> Â >>>Â Â Â Â Â Â break; >> Â >>> >> Â >>>Â Â Â Â Â case GL_TEXTURE_FIXED_SAMPLE_LOCATIONS: >> Â >>> -Â Â Â Â Â if (!ctx->Extensions.ARB_texture_multisample) >> Â >>> +Â Â Â Â Â if (!_mesa_is_gles31(ctx) && >> !ctx->Extensions.ARB_texture_multisample) >> Â >> >> Â >> >> Â >> I think you guys have gone a little nuts with sticking _mesa_is_gles31 >> Â >> all over the place... This is accounting for drivers which want to >> Â >> expose gles3.1 but don't have ARB_texture_multisample supported. Are >> Â >> there any such drivers? Are all these changes really worthwhile? >> Â > >> Â > >> Â > This particular patch was to address >> Â > >> Â > "glGetTexLevelParameter[fi]v - needs updates to restrict to GLES enums" >> Â > >> Â > in docs/GL3.txt GLES3.1 section. >> Â > >> Â > I do understand your worry about sticking is_gles31() here and there >> but remember that most (if not all) these changes will apply also for >> gles32. In Mesa there are 232 instances of is_gles(), 156 of is_gles3() >> and only 34 instances of is_gles31() so I think the overall changes are >> still quite small, not so intrusive as it may look. >> >> I think you may have misunderstood my comment. >> >> Old code: if not texture ms >> New code: if not gles31 and not texture ms >> >> Aka "not (gles31 or texture ms)" >> >> I posit that the situation where gles31 and not texture ms will never >> ever happen. >> >> So you haven't changed the outcome of that conditional for any >> reasonable circumstances. Why are you adding that code? > > > There are some differences between ARB_texture_multisample and GLES 31, at least multisample array textures. Agreeably this is 'academical' and may not happen in real life (like many other error checks in Mesa though). I guess these could be removed if others also feel like this. Sure, but are you ever going to get a driver that supports gles31 but does not have that bit set in extensions? I don't think so. I'm concerned that tons and tons of these needless checks are being added all over. I'm all for api correctness, but this seems to be about totally academic situations. The code literally has no effect for any actual drivers in Mesa, nor any that I can imagine in the future. That's what I meant about it being ok for compute, since its a practical issue -- its not supported, but you want it to still be faked in es31. Fine. But here? What's the usecase? > > >> Â > >> Â > >> Â >> The original argument was that you wanted to pass some CTS tests >> Â >> before you actually had compute/etc shaders going... which seems >> Â >> reasonable. This all feels like going overboard though. >> Â > >> Â > >> Â > For the question if changes are worthwhile, we have gone from 0 >> passing CTS tests (+ a few crashes) to > 200 passing tests, personally I >> think this is good progress, thanks to you and others who have corrected >> errors in our changes. >> Â > >> Â > Our goal is to pass the whole suite when rest of the missing >> extensions are integrated. There are still things to do, API differences >> here and there. Nothing major but important to get done. >> Â > >> Â > >> Â > >> Â > >> Â >>Â Â -ilia >> Â >> >> Â >>>Â Â Â Â Â Â Â Â goto invalid_pname; >> Â >>>Â Â Â Â Â Â *params = img->FixedSampleLocations; >> Â >>>Â Â Â Â Â Â break; >> Â >>> -- >> Â >>> 2.4.3 >> Â >>> >> Â >>> _______________________________________________ >> Â >>> mesa-dev mailing list >> Â >>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> >> Â >>> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> Â > >> Â > >> Â > >> Â > // Tapani >>