Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On Mon, May 11, 2015 at 4:57 PM, Ilia Mirkin imir...@alum.mit.edu wrote: On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: Shouldn't this be like this instead (and make sure ARB_texture_multisample is enabled for ES3.1)? @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = !ctx-Extensions.ARB_texture_multisample; error = false when you have a driver that supports texture_ms, but you have a gles1/2/3 context, whereas you wanted error = true there... I would expect ctx-Extensions.ARB_texture_multisample to be false in any pre-GLES 3.1 context, since ARB_texture_multisample is written against the OpenGL 3.1 specification, and not any Open GL ES flavor. Are you saying that we do not mask these booleans against what the context can support? Are these purely about what the driver can manage? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On Mon, May 11, 2015 at 11:07 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, May 11, 2015 at 4:57 PM, Ilia Mirkin imir...@alum.mit.edu wrote: On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: Shouldn't this be like this instead (and make sure ARB_texture_multisample is enabled for ES3.1)? @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = !ctx-Extensions.ARB_texture_multisample; error = false when you have a driver that supports texture_ms, but you have a gles1/2/3 context, whereas you wanted error = true there... I would expect ctx-Extensions.ARB_texture_multisample to be false in any pre-GLES 3.1 context, since ARB_texture_multisample is written against the OpenGL 3.1 specification, and not any Open GL ES flavor. Are you saying that we do not mask these booleans against what the context can support? Are these purely about what the driver can manage? Correct. They are exclusively set by the driver and never modified again. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On Mon, May 11, 2015 at 5:21 PM, Ilia Mirkin imir...@alum.mit.edu wrote: On Mon, May 11, 2015 at 11:07 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, May 11, 2015 at 4:57 PM, Ilia Mirkin imir...@alum.mit.edu wrote: On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: Shouldn't this be like this instead (and make sure ARB_texture_multisample is enabled for ES3.1)? @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = !ctx-Extensions.ARB_texture_multisample; error = false when you have a driver that supports texture_ms, but you have a gles1/2/3 context, whereas you wanted error = true there... I would expect ctx-Extensions.ARB_texture_multisample to be false in any pre-GLES 3.1 context, since ARB_texture_multisample is written against the OpenGL 3.1 specification, and not any Open GL ES flavor. Are you saying that we do not mask these booleans against what the context can support? Are these purely about what the driver can manage? Correct. They are exclusively set by the driver and never modified again. OK. Then yeah, my comments were obviously wrong. Thanks for setting me straight! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, May 11, 2015 at 3:03 PM, Marta Lofstedt marta.lofst...@linux.intel.com wrote: From: Marta Lofstedt marta.lofst...@intel.com GLES 3.1 must be allowed to use multisampled frambuffer textures. Signed-off-by: Marta Lofstedt marta.lofst...@intel.com --- src/mesa/main/fbobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c index 27cf97f..14a015e 100644 --- a/src/mesa/main/fbobject.c +++ b/src/mesa/main/fbobject.c @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = (_mesa_is_gles(ctx) +|| !ctx-Extensions.ARB_texture_multisample) +!_mesa_is_gles31(ctx); This seems correct. error = true when old condition, but not when es3.1 even if the old condition holds true. If the old condition is false, then the new addition doesn't matter. Personally I would have written this as error = _mesa_is_gles(ctx) !mesa_is_gles31(ctx) || !ctx-Extensions.ARB_texture_multisample; The nice thing about this is that it will force an error even in the very hypothetical situation where a driver doesn't expose ARB_texture_multisample, but a GLES3.1 context was created (e.g. via an override flag). break; default: error = GL_TRUE; Shouldn't this be like this instead (and make sure ARB_texture_multisample is enabled for ES3.1)? @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = !ctx-Extensions.ARB_texture_multisample; error = false when you have a driver that supports texture_ms, but you have a gles1/2/3 context, whereas you wanted error = true there... -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On Mon, May 11, 2015 at 3:03 PM, Marta Lofstedt marta.lofst...@linux.intel.com wrote: From: Marta Lofstedt marta.lofst...@intel.com GLES 3.1 must be allowed to use multisampled frambuffer textures. Signed-off-by: Marta Lofstedt marta.lofst...@intel.com --- src/mesa/main/fbobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c index 27cf97f..14a015e 100644 --- a/src/mesa/main/fbobject.c +++ b/src/mesa/main/fbobject.c @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = (_mesa_is_gles(ctx) +|| !ctx-Extensions.ARB_texture_multisample) +!_mesa_is_gles31(ctx); break; default: error = GL_TRUE; Shouldn't this be like this instead (and make sure ARB_texture_multisample is enabled for ES3.1)? @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = !ctx-Extensions.ARB_texture_multisample; break; default: error = GL_TRUE; ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On 05/11/2015 07:57 AM, Ilia Mirkin wrote: On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, May 11, 2015 at 3:03 PM, Marta Lofstedt marta.lofst...@linux.intel.com wrote: From: Marta Lofstedt marta.lofst...@intel.com GLES 3.1 must be allowed to use multisampled frambuffer textures. Signed-off-by: Marta Lofstedt marta.lofst...@intel.com --- src/mesa/main/fbobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c index 27cf97f..14a015e 100644 --- a/src/mesa/main/fbobject.c +++ b/src/mesa/main/fbobject.c @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = (_mesa_is_gles(ctx) +|| !ctx-Extensions.ARB_texture_multisample) +!_mesa_is_gles31(ctx); This seems correct. error = true when old condition, but not when es3.1 even if the old condition holds true. If the old condition is false, then the new addition doesn't matter. Personally I would have written this as error = _mesa_is_gles(ctx) !mesa_is_gles31(ctx) || !ctx-Extensions.ARB_texture_multisample; The nice thing about this is that it will force an error even in the very hypothetical situation where a driver doesn't expose ARB_texture_multisample, but a GLES3.1 context was created (e.g. via an override flag). I think this is a mis-feature. Most of the ARB extensions are supersets of the ES3.1 functionality, so we could someday have a driver that does one but not the other. break; default: error = GL_TRUE; Shouldn't this be like this instead (and make sure ARB_texture_multisample is enabled for ES3.1)? @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = !ctx-Extensions.ARB_texture_multisample; error = false when you have a driver that supports texture_ms, but you have a gles1/2/3 context, whereas you wanted error = true there... -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On Mon, May 11, 2015 at 4:34 PM, Ian Romanick i...@freedesktop.org wrote: On 05/11/2015 07:57 AM, Ilia Mirkin wrote: On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, May 11, 2015 at 3:03 PM, Marta Lofstedt marta.lofst...@linux.intel.com wrote: From: Marta Lofstedt marta.lofst...@intel.com GLES 3.1 must be allowed to use multisampled frambuffer textures. Signed-off-by: Marta Lofstedt marta.lofst...@intel.com --- src/mesa/main/fbobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c index 27cf97f..14a015e 100644 --- a/src/mesa/main/fbobject.c +++ b/src/mesa/main/fbobject.c @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = (_mesa_is_gles(ctx) +|| !ctx-Extensions.ARB_texture_multisample) +!_mesa_is_gles31(ctx); This seems correct. error = true when old condition, but not when es3.1 even if the old condition holds true. If the old condition is false, then the new addition doesn't matter. Personally I would have written this as error = _mesa_is_gles(ctx) !mesa_is_gles31(ctx) || !ctx-Extensions.ARB_texture_multisample; The nice thing about this is that it will force an error even in the very hypothetical situation where a driver doesn't expose ARB_texture_multisample, but a GLES3.1 context was created (e.g. via an override flag). I think this is a mis-feature. Most of the ARB extensions are supersets of the ES3.1 functionality, so we could someday have a driver that does one but not the other. OK, well, I won't fight this further -- I think I've made my point, and I believe you've understood it. Since you disagree, happy to let it go. I can't help but add though that in the situation where the ARB extension is a superset in terms of actual backend driver functionality, not just small frontend differences (e.g. ARB_gpu_shader5), we should add more enables that allow the (in this case) ES3.1 functionality. This enables a driver to be developed over time and lets people use version overrides without resulting in crashes or other driver weirdness. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures
On 05/11/2015 01:43 PM, Ilia Mirkin wrote: On Mon, May 11, 2015 at 4:34 PM, Ian Romanick i...@freedesktop.org wrote: On 05/11/2015 07:57 AM, Ilia Mirkin wrote: On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, May 11, 2015 at 3:03 PM, Marta Lofstedt marta.lofst...@linux.intel.com wrote: From: Marta Lofstedt marta.lofst...@intel.com GLES 3.1 must be allowed to use multisampled frambuffer textures. Signed-off-by: Marta Lofstedt marta.lofst...@intel.com --- src/mesa/main/fbobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c index 27cf97f..14a015e 100644 --- a/src/mesa/main/fbobject.c +++ b/src/mesa/main/fbobject.c @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment, break; case GL_TEXTURE_2D_MULTISAMPLE: case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: - error = _mesa_is_gles(ctx) -|| !ctx-Extensions.ARB_texture_multisample; + error = (_mesa_is_gles(ctx) +|| !ctx-Extensions.ARB_texture_multisample) +!_mesa_is_gles31(ctx); This seems correct. error = true when old condition, but not when es3.1 even if the old condition holds true. If the old condition is false, then the new addition doesn't matter. Personally I would have written this as error = _mesa_is_gles(ctx) !mesa_is_gles31(ctx) || !ctx-Extensions.ARB_texture_multisample; The nice thing about this is that it will force an error even in the very hypothetical situation where a driver doesn't expose ARB_texture_multisample, but a GLES3.1 context was created (e.g. via an override flag). I think this is a mis-feature. Most of the ARB extensions are supersets of the ES3.1 functionality, so we could someday have a driver that does one but not the other. OK, well, I won't fight this further -- I think I've made my point, and I believe you've understood it. Since you disagree, happy to let it go. I can't help but add though that in the situation where the ARB extension is a superset in terms of actual backend driver functionality, not just small frontend differences (e.g. ARB_gpu_shader5), we should add more enables that allow the (in this case) ES3.1 functionality. This enables a driver to be developed over time and lets people use version overrides without resulting in crashes or other driver weirdness. Perhaps. Similar situations have been handled in a couple different ways in the past. Sometimes we'll add a more complex function to determine whether something is enabled (e.g., _mesa_has_geometry_shaders). Sometimes a driver will on enable a certain extension for a certain kind of context (e.g., ARB_base_instance in the i965 driver). And there are other cases where we have multiple extension bits (e.g., OES_texture_float). -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
