On Aug 24, 2015 2:01 AM, "Tapani Pälli" <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>> wrote: > > > > On 08/24/2015 08:38 AM, Ilia Mirkin wrote: >> >> >> On Aug 24, 2015 1:25 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/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>> >> >> <mailto:<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>> >> <mailto:<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>> >> >> <mailto:<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? > > > Is it the same thing then for EXT_texture_array and ARB_texture_cube_map? I could send a new version with removing checks, the comment on top ensures that reader knows which enums are only for ES 3.1. Yes. Here's how I think about it -- if it's going to be included in compute_version (which those are, for es2/3) then you can assume it'll be set. Otherwise you have to check separately. I think that the es31 check will have texture ms in it, even though, like you said, its not a perfect match. That's the case for a lot of these. > > >> > >> > >> >> > >> >> > >> >> >> 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>> >> <mailto:<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 >> >> >>