Re: [Mesa-dev] [PATCH 1/2] mesa: add NV_image_formats extension support

[Francisco Jerez]

Ilia Mirkin  writes:

> On Mon, Nov 14, 2016 at 6:55 AM, Lionel Landwerlin
>  wrote:
>> On 11/11/16 18:39, Ilia Mirkin wrote:
>>> On Fri, Nov 11, 2016 at 10:40 AM, Lionel Landwerlin  
>>> wrote:
 diff --git a/src/mesa/main/extensions_table.h
 b/src/mesa/main/extensions_table.h
 index 2dbd7da..f58f2ad 100644
 --- a/src/mesa/main/extensions_table.h
 +++ b/src/mesa/main/extensions_table.h
 @@ -315,6 +315,7 @@ EXT(NV_depth_clamp  ,
 ARB_depth_clamp
   EXT(NV_draw_buffers , dummy_true
 ,  x ,  x ,  x , ES2, 2011)
   EXT(NV_fbo_color_attachments, dummy_true
 ,  x ,  x ,  x , ES2, 2010)
   EXT(NV_fog_distance , NV_fog_distance
 , GLL,  x ,  x ,  x , 2001)
 +EXT(NV_image_formats, NV_image_formats
 , GLL, GLC,  x ,  31, 2014)
>>>
>>> This is a GLES-only ext. You want "x" in the GLL and GLC spots.
>>>
>>> Also, is this strictly necessary? I'd recommend dropping the new
>>> boolean and just using ARB_shader_image_load_store here. That would
>>> mean this gets auto-enabled for all ES 3.1-supporting drivers. (Since
>>> there's no new functionality on top of what
>>> ARB_shader_image_load_store requires.)
>>
>>
>> Thanks.
>>
>> Though I'm a bit perplex with what you're proposing.
>> ARB_shader_image_load_store is an OpenGL 3.2 extension, how's that supposed
>> to work with applications using GLES?
>
> It's just a bit in gl_extensions. One that indicates support for the
> features required by that extension. NV_image_formats is a subset of
> ARB_shader_image_load_store functionality, so if a backend supports
> the latter, it'll also support the former. [As an aside, I believe we
> only enable ES 3.1 if gl_extensions.ARB_shader_image_load_store is
> set, so you could ultimately make this into dummy_true - either way's
> good with me.]
>

Agreed, the NV_image_formats bit seems redundant at this point because
ARB_shader_image_load_store provides roughly the same functionality.

> Cheers,
>
>   -ilia
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa: add NV_image_formats extension support

[Ilia Mirkin]

On Mon, Nov 14, 2016 at 6:55 AM, Lionel Landwerlin
 wrote:
> On 11/11/16 18:39, Ilia Mirkin wrote:
>> On Fri, Nov 11, 2016 at 10:40 AM, Lionel Landwerlin  
>> wrote:
>>> diff --git a/src/mesa/main/extensions_table.h
>>> b/src/mesa/main/extensions_table.h
>>> index 2dbd7da..f58f2ad 100644
>>> --- a/src/mesa/main/extensions_table.h
>>> +++ b/src/mesa/main/extensions_table.h
>>> @@ -315,6 +315,7 @@ EXT(NV_depth_clamp  ,
>>> ARB_depth_clamp
>>>   EXT(NV_draw_buffers , dummy_true
>>> ,  x ,  x ,  x , ES2, 2011)
>>>   EXT(NV_fbo_color_attachments, dummy_true
>>> ,  x ,  x ,  x , ES2, 2010)
>>>   EXT(NV_fog_distance , NV_fog_distance
>>> , GLL,  x ,  x ,  x , 2001)
>>> +EXT(NV_image_formats, NV_image_formats
>>> , GLL, GLC,  x ,  31, 2014)
>>
>> This is a GLES-only ext. You want "x" in the GLL and GLC spots.
>>
>> Also, is this strictly necessary? I'd recommend dropping the new
>> boolean and just using ARB_shader_image_load_store here. That would
>> mean this gets auto-enabled for all ES 3.1-supporting drivers. (Since
>> there's no new functionality on top of what
>> ARB_shader_image_load_store requires.)
>
>
> Thanks.
>
> Though I'm a bit perplex with what you're proposing.
> ARB_shader_image_load_store is an OpenGL 3.2 extension, how's that supposed
> to work with applications using GLES?

It's just a bit in gl_extensions. One that indicates support for the
features required by that extension. NV_image_formats is a subset of
ARB_shader_image_load_store functionality, so if a backend supports
the latter, it'll also support the former. [As an aside, I believe we
only enable ES 3.1 if gl_extensions.ARB_shader_image_load_store is
set, so you could ultimately make this into dummy_true - either way's
good with me.]

Cheers,

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

Re: [Mesa-dev] [PATCH 1/2] mesa: add NV_image_formats extension support

[Lionel Landwerlin]

On 11/11/16 18:39, Ilia Mirkin wrote:

On Fri, Nov 11, 2016 at 10:40 AM, Lionel Landwerlin
 wrote:

Signed-off-by: Lionel Landwerlin 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98480
---
  src/compiler/glsl/glsl_parser.yy |  5 +--
  src/compiler/glsl/glsl_parser_extras.cpp | 58 
  src/compiler/glsl/glsl_parser_extras.h   |  4 +++
  src/mesa/main/extensions_table.h |  1 +
  src/mesa/main/mtypes.h   |  1 +
  src/mesa/main/shaderimage.c  | 22 
  6 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
index 7d709c7..0055c7d 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1339,8 +1339,9 @@ layout_qualifier_id:
  };

  for (unsigned i = 0; i < ARRAY_SIZE(map); i++) {
-   if (state->is_version(map[i].required_glsl,
- map[i].required_essl) &&
+   if ((state->is_version(map[i].required_glsl,
+  map[i].required_essl) ||
+state->check_additional_es31_image_format(map[i].format)) 
&&
 match_layout_qualifier($1, map[i].name, state) == 0) {
$$.flags.q.explicit_image_format = 1;
$$.image_format = map[i].format;
diff --git a/src/compiler/glsl/glsl_parser_extras.cpp 
b/src/compiler/glsl/glsl_parser_extras.cpp
index db659adf..8d639e6 100644
--- a/src/compiler/glsl/glsl_parser_extras.cpp
+++ b/src/compiler/glsl/glsl_parser_extras.cpp
@@ -28,6 +28,7 @@
  #include "main/core.h" /* for struct gl_context */
  #include "main/context.h"
  #include "main/debug_output.h"
+#include "main/formats.h"
  #include "main/shaderobj.h"
  #include "util/u_atomic.h" /* for p_atomic_cmpxchg */
  #include "util/ralloc.h"
@@ -355,6 +356,62 @@ _mesa_glsl_parse_state::check_version(unsigned 
required_glsl_version,
  }

  /**
+ * Determines whether a given format is supported using the
+ * GL_NV_image_formats extension.
+ */
+bool
+_mesa_glsl_parse_state::check_additional_es31_image_format(GLenum format) const
+{
+   if (!this->NV_image_formats_enable || !is_version(0, 310))
+  return false;

NV_image_formats_enable can't be true without ES 3.1. (Note that this
is an explicit #extension foo:enable action, not just the existence of
the ext. These enables ensure that there's an appropriate context.)


+
+   switch (format) {
+   case GL_RG32F:
+   case GL_RG16F:
+   case GL_R11F_G11F_B10F:
+   case GL_R16F:
+   case GL_RGB10_A2:
+   case GL_RG8:
+   case GL_RG8_SNORM:
+   case GL_R8:
+   case GL_R8_SNORM:
+  return true;
+
+   case GL_RG32I:
+   case GL_RG16I:
+   case GL_RG8I:
+   case GL_R16I:
+   case GL_R8I:
+  return true;
+
+   case GL_RGB10_A2UI:
+   case GL_RG32UI:
+   case GL_RG16UI:
+   case GL_RG8UI:
+   case GL_R16UI:
+   case GL_R8UI:
+  return true;
+
+   case GL_R16:
+   case GL_R16_SNORM:
+   case GL_RG16:
+   case GL_RG16_SNORM:
+   case GL_RGBA16:
+   case GL_RGBA16_SNORM:
+  /**
+   * GL_NV_image_formats tells us that we should not expose any of the
+   * normalized 16bits formats unless we have GL_EXT_texture_norm16 or
+   * equivalent functionality. Since we don't have support for that
+   * extension, just disable those for now.
+   */
+  return false;
+
+   default:
+  return false;
+   }

Why not just add something to the format map where these are parsed,
to include the info on which formats are allowed with which extra
extensions. (A single bool for now, maybe a bitfield later when
EXT_texture_norm16 gets added.)

IMHO that'd be much more compact.


+}
+
+/**
   * Process a GLSL #version directive.
   *
   * \param version is the integer that follows the #version token.
@@ -687,6 +744,7 @@ static const _mesa_glsl_extension 
_mesa_glsl_supported_extensions[] = {
 EXT_AEP(EXT_texture_buffer),
 EXT_AEP(EXT_texture_cube_map_array),
 EXT(MESA_shader_integer_functions),
+   EXT(NV_image_formats),
  };

  #undef EXT
diff --git a/src/compiler/glsl/glsl_parser_extras.h 
b/src/compiler/glsl/glsl_parser_extras.h
index 53abbbc..a3e1aa5 100644
--- a/src/compiler/glsl/glsl_parser_extras.h
+++ b/src/compiler/glsl/glsl_parser_extras.h
@@ -204,6 +204,8 @@ struct _mesa_glsl_parse_state {
return true;
 }

+   bool check_additional_es31_image_format(GLenum format) const;
+
 bool has_atomic_counters() const
 {
return ARB_shader_atomic_counters_enable || is_version(420, 310);
@@ -765,6 +767,8 @@ struct _mesa_glsl_parse_state {
 bool MESA_shader_framebuffer_fetch_non_coherent_warn;
 bool MESA_shader_integer_functions_enable;
 bool MESA_shader_integer_functions_warn;
+   bool NV_image_formats_enable;
+   bool NV_image_formats_warn;
 /*@}*/

 /** Extensions supported by the OpenGL 

Re: [Mesa-dev] [PATCH 1/2] mesa: add NV_image_formats extension support

[Ilia Mirkin]

On Fri, Nov 11, 2016 at 10:40 AM, Lionel Landwerlin
 wrote:
> Signed-off-by: Lionel Landwerlin 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98480
> ---
>  src/compiler/glsl/glsl_parser.yy |  5 +--
>  src/compiler/glsl/glsl_parser_extras.cpp | 58 
> 
>  src/compiler/glsl/glsl_parser_extras.h   |  4 +++
>  src/mesa/main/extensions_table.h |  1 +
>  src/mesa/main/mtypes.h   |  1 +
>  src/mesa/main/shaderimage.c  | 22 
>  6 files changed, 83 insertions(+), 8 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_parser.yy 
> b/src/compiler/glsl/glsl_parser.yy
> index 7d709c7..0055c7d 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -1339,8 +1339,9 @@ layout_qualifier_id:
>  };
>
>  for (unsigned i = 0; i < ARRAY_SIZE(map); i++) {
> -   if (state->is_version(map[i].required_glsl,
> - map[i].required_essl) &&
> +   if ((state->is_version(map[i].required_glsl,
> +  map[i].required_essl) ||
> +
> state->check_additional_es31_image_format(map[i].format)) &&
> match_layout_qualifier($1, map[i].name, state) == 0) {
>$$.flags.q.explicit_image_format = 1;
>$$.image_format = map[i].format;
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp 
> b/src/compiler/glsl/glsl_parser_extras.cpp
> index db659adf..8d639e6 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -28,6 +28,7 @@
>  #include "main/core.h" /* for struct gl_context */
>  #include "main/context.h"
>  #include "main/debug_output.h"
> +#include "main/formats.h"
>  #include "main/shaderobj.h"
>  #include "util/u_atomic.h" /* for p_atomic_cmpxchg */
>  #include "util/ralloc.h"
> @@ -355,6 +356,62 @@ _mesa_glsl_parse_state::check_version(unsigned 
> required_glsl_version,
>  }
>
>  /**
> + * Determines whether a given format is supported using the
> + * GL_NV_image_formats extension.
> + */
> +bool
> +_mesa_glsl_parse_state::check_additional_es31_image_format(GLenum format) 
> const
> +{
> +   if (!this->NV_image_formats_enable || !is_version(0, 310))
> +  return false;

NV_image_formats_enable can't be true without ES 3.1. (Note that this
is an explicit #extension foo:enable action, not just the existence of
the ext. These enables ensure that there's an appropriate context.)

> +
> +   switch (format) {
> +   case GL_RG32F:
> +   case GL_RG16F:
> +   case GL_R11F_G11F_B10F:
> +   case GL_R16F:
> +   case GL_RGB10_A2:
> +   case GL_RG8:
> +   case GL_RG8_SNORM:
> +   case GL_R8:
> +   case GL_R8_SNORM:
> +  return true;
> +
> +   case GL_RG32I:
> +   case GL_RG16I:
> +   case GL_RG8I:
> +   case GL_R16I:
> +   case GL_R8I:
> +  return true;
> +
> +   case GL_RGB10_A2UI:
> +   case GL_RG32UI:
> +   case GL_RG16UI:
> +   case GL_RG8UI:
> +   case GL_R16UI:
> +   case GL_R8UI:
> +  return true;
> +
> +   case GL_R16:
> +   case GL_R16_SNORM:
> +   case GL_RG16:
> +   case GL_RG16_SNORM:
> +   case GL_RGBA16:
> +   case GL_RGBA16_SNORM:
> +  /**
> +   * GL_NV_image_formats tells us that we should not expose any of the
> +   * normalized 16bits formats unless we have GL_EXT_texture_norm16 or
> +   * equivalent functionality. Since we don't have support for that
> +   * extension, just disable those for now.
> +   */
> +  return false;
> +
> +   default:
> +  return false;
> +   }

Why not just add something to the format map where these are parsed,
to include the info on which formats are allowed with which extra
extensions. (A single bool for now, maybe a bitfield later when
EXT_texture_norm16 gets added.)

IMHO that'd be much more compact.

> +}
> +
> +/**
>   * Process a GLSL #version directive.
>   *
>   * \param version is the integer that follows the #version token.
> @@ -687,6 +744,7 @@ static const _mesa_glsl_extension 
> _mesa_glsl_supported_extensions[] = {
> EXT_AEP(EXT_texture_buffer),
> EXT_AEP(EXT_texture_cube_map_array),
> EXT(MESA_shader_integer_functions),
> +   EXT(NV_image_formats),
>  };
>
>  #undef EXT
> diff --git a/src/compiler/glsl/glsl_parser_extras.h 
> b/src/compiler/glsl/glsl_parser_extras.h
> index 53abbbc..a3e1aa5 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -204,6 +204,8 @@ struct _mesa_glsl_parse_state {
>return true;
> }
>
> +   bool check_additional_es31_image_format(GLenum format) const;
> +
> bool has_atomic_counters() const
> {
>return ARB_shader_atomic_counters_enable || is_version(420, 310);
> @@ -765,6 +767,8 @@ struct _mesa_glsl_parse_state {
> bool MESA_shader_framebuffer_fetch_non_coherent_warn;
> bool