[Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
---
src/mapi/glapi/gen/gl_genexec.py | 1 +
src/mesa/Makefile.sources| 1 +
src/mesa/main/shaderimage.c | 457 +++
src/mesa/main/shaderimage.h | 42
4 files changed, 501 insertions(+)
create mode 100644 src/mesa/main/shaderimage.c
create mode 100644 src/mesa/main/shaderimage.h
diff --git a/src/mapi/glapi/gen/gl_genexec.py b/src/mapi/glapi/gen/gl_genexec.py
index 3ce190f..334a4bf 100644
--- a/src/mapi/glapi/gen/gl_genexec.py
+++ b/src/mapi/glapi/gen/gl_genexec.py
@@ -107,6 +107,7 @@ header = """/**
#include "main/varray.h"
#include "main/viewport.h"
#include "main/shaderapi.h"
+#include "main/shaderimage.h"
#include "main/uniforms.h"
#include "main/syncobj.h"
#include "main/formatquery.h"
diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index a84f8a7..7c67145 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -81,6 +81,7 @@ MAIN_FILES = \
$(SRCDIR)main/scissor.c \
$(SRCDIR)main/set.c \
$(SRCDIR)main/shaderapi.c \
+ $(SRCDIR)main/shaderimage.c \
$(SRCDIR)main/shaderobj.c \
$(SRCDIR)main/shader_query.cpp \
$(SRCDIR)main/shared.c \
diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
new file mode 100644
index 000..627366b
--- /dev/null
+++ b/src/mesa/main/shaderimage.c
@@ -0,0 +1,457 @@
+/*
+ * Copyright 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ *Francisco Jerez
+ */
+
+#include
+
+#include "shaderimage.h"
+#include "mtypes.h"
+#include "formats.h"
+#include "errors.h"
+#include "context.h"
+#include "texobj.h"
+
+/*
+ * Define endian-invariant aliases for some mesa formats that are
+ * defined in terms of their channel layout from LSB to MSB in a
+ * 32-bit word. The actual byte offsets matter here because the user
+ * is allowed to bit-cast one format into another and get predictable
+ * results.
+ */
+#ifdef MESA_BIG_ENDIAN
+# define MESA_FORMAT_RGBA_8 MESA_FORMAT_RGBA
+# define MESA_FORMAT_RG_16 MESA_FORMAT_RG1616
+# define MESA_FORMAT_RG_8 MESA_FORMAT_RG88
+# define MESA_FORMAT_SIGNED_RGBA_8 MESA_FORMAT_SIGNED_RGBA
+# define MESA_FORMAT_SIGNED_RG_16 MESA_FORMAT_SIGNED_RG1616
+# define MESA_FORMAT_SIGNED_RG_8 MESA_FORMAT_SIGNED_RG88
+#else
+# define MESA_FORMAT_RGBA_8 MESA_FORMAT_RGBA_REV
+# define MESA_FORMAT_RG_16 MESA_FORMAT_GR1616
+# define MESA_FORMAT_RG_8 MESA_FORMAT_GR88
+# define MESA_FORMAT_SIGNED_RGBA_8 MESA_FORMAT_SIGNED_RGBA_REV
+# define MESA_FORMAT_SIGNED_RG_16 MESA_FORMAT_SIGNED_GR1616
+# define MESA_FORMAT_SIGNED_RG_8 MESA_FORMAT_SIGNED_RG88_REV
+#endif
+
+static gl_format
+get_image_format(GLenum format)
+{
+ switch (format) {
+ case GL_RGBA32F:
+ return MESA_FORMAT_RGBA_FLOAT32;
+
+ case GL_RGBA16F:
+ return MESA_FORMAT_RGBA_FLOAT16;
+
+ case GL_RG32F:
+ return MESA_FORMAT_RG_FLOAT32;
+
+ case GL_RG16F:
+ return MESA_FORMAT_RG_FLOAT16;
+
+ case GL_R11F_G11F_B10F:
+ return MESA_FORMAT_R11_G11_B10_FLOAT;
+
+ case GL_R32F:
+ return MESA_FORMAT_R_FLOAT32;
+
+ case GL_R16F:
+ return MESA_FORMAT_R_FLOAT16;
+
+ case GL_RGBA32UI:
+ return MESA_FORMAT_RGBA_UINT32;
+
+ case GL_RGBA16UI:
+ return MESA_FORMAT_RGBA_UINT16;
+
+ case GL_RGB10_A2UI:
+ return MESA_FORMAT_ABGR2101010_UINT;
+
+ case GL_RGBA8UI:
+ return MESA_FORMAT_RGBA_UINT8;
+
+ case GL_RG32UI:
+ return MESA_FORMAT_RG_UINT32;
+
+ case GL_RG16UI:
+ return MESA_FORMAT_RG_UINT16;
+
+ case GL_RG8UI:
+ return MESA_FORMAT_RG_UINT8;
+
+ case GL_R32UI:
+ return MESA_FORMAT_R_UINT32;
+
+ case GL_R16UI:
+ return MESA_FORMAT_R_UINT16;
+
+ case GL_R8UI:
+ return MESA_FORMAT_R_UINT8;
+
+ case GL_RGBA32I:
+ return MESA_FORMAT_RGBA_INT32;
+
+ case GL_RGBA16I:
+ return MESA_FORMAT_RGBA_INT16;
+
+ case
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
On Monday 25 November 2013, Francisco Jerez wrote:
> ---
> src/mapi/glapi/gen/gl_genexec.py | 1 +
> src/mesa/Makefile.sources| 1 +
> src/mesa/main/shaderimage.c | 457
> +++
> src/mesa/main/shaderimage.h | 42
> 4 files changed, 501 insertions(+)
> create mode 100644 src/mesa/main/shaderimage.c
> create mode 100644 src/mesa/main/shaderimage.h
>
> diff --git a/src/mapi/glapi/gen/gl_genexec.py
> b/src/mapi/glapi/gen/gl_genexec.py
> index 3ce190f..334a4bf 100644
> --- a/src/mapi/glapi/gen/gl_genexec.py
> +++ b/src/mapi/glapi/gen/gl_genexec.py
> @@ -107,6 +107,7 @@ header = """/**
> #include "main/varray.h"
> #include "main/viewport.h"
> #include "main/shaderapi.h"
> +#include "main/shaderimage.h"
> #include "main/uniforms.h"
> #include "main/syncobj.h"
> #include "main/formatquery.h"
> diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
> index a84f8a7..7c67145 100644
> --- a/src/mesa/Makefile.sources
> +++ b/src/mesa/Makefile.sources
> @@ -81,6 +81,7 @@ MAIN_FILES = \
> $(SRCDIR)main/scissor.c \
> $(SRCDIR)main/set.c \
> $(SRCDIR)main/shaderapi.c \
> + $(SRCDIR)main/shaderimage.c \
> $(SRCDIR)main/shaderobj.c \
> $(SRCDIR)main/shader_query.cpp \
> $(SRCDIR)main/shared.c \
> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
> new file mode 100644
> index 000..627366b
> --- /dev/null
> +++ b/src/mesa/main/shaderimage.c
> @@ -0,0 +1,457 @@
> +/*
> + * Copyright 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + *Francisco Jerez
> + */
> +
> +#include
> +
> +#include "shaderimage.h"
> +#include "mtypes.h"
> +#include "formats.h"
> +#include "errors.h"
> +#include "context.h"
> +#include "texobj.h"
> +
> +/*
> + * Define endian-invariant aliases for some mesa formats that are
> + * defined in terms of their channel layout from LSB to MSB in a
> + * 32-bit word. The actual byte offsets matter here because the user
> + * is allowed to bit-cast one format into another and get predictable
> + * results.
> + */
> +#ifdef MESA_BIG_ENDIAN
> +# define MESA_FORMAT_RGBA_8 MESA_FORMAT_RGBA
> +# define MESA_FORMAT_RG_16 MESA_FORMAT_RG1616
> +# define MESA_FORMAT_RG_8 MESA_FORMAT_RG88
> +# define MESA_FORMAT_SIGNED_RGBA_8 MESA_FORMAT_SIGNED_RGBA
> +# define MESA_FORMAT_SIGNED_RG_16 MESA_FORMAT_SIGNED_RG1616
> +# define MESA_FORMAT_SIGNED_RG_8 MESA_FORMAT_SIGNED_RG88
> +#else
> +# define MESA_FORMAT_RGBA_8 MESA_FORMAT_RGBA_REV
> +# define MESA_FORMAT_RG_16 MESA_FORMAT_GR1616
> +# define MESA_FORMAT_RG_8 MESA_FORMAT_GR88
> +# define MESA_FORMAT_SIGNED_RGBA_8 MESA_FORMAT_SIGNED_RGBA_REV
> +# define MESA_FORMAT_SIGNED_RG_16 MESA_FORMAT_SIGNED_GR1616
> +# define MESA_FORMAT_SIGNED_RG_8 MESA_FORMAT_SIGNED_RG88_REV
> +#endif
> +
> +static gl_format
> +get_image_format(GLenum format)
> +{
> + switch (format) {
> + case GL_RGBA32F:
> + return MESA_FORMAT_RGBA_FLOAT32;
> +
> + case GL_RGBA16F:
> + return MESA_FORMAT_RGBA_FLOAT16;
> +
> + case GL_RG32F:
> + return MESA_FORMAT_RG_FLOAT32;
> +
> + case GL_RG16F:
> + return MESA_FORMAT_RG_FLOAT16;
> +
> + case GL_R11F_G11F_B10F:
> + return MESA_FORMAT_R11_G11_B10_FLOAT;
> +
> + case GL_R32F:
> + return MESA_FORMAT_R_FLOAT32;
> +
> + case GL_R16F:
> + return MESA_FORMAT_R_FLOAT16;
> +
> + case GL_RGBA32UI:
> + return MESA_FORMAT_RGBA_UINT32;
> +
> + case GL_RGBA16UI:
> + return MESA_FORMAT_RGBA_UINT16;
> +
> + case GL_RGB10_A2UI:
> + return MESA_FORMAT_ABGR2101010_UINT;
> +
> + case GL_RGBA8UI:
> + return MESA_FORMAT_RGBA_UINT8;
> +
> + case GL_RG32UI:
> + return MESA_FORMAT_RG_UINT32;
> +
> + case GL_RG16UI:
> + return MESA_FORMAT_RG_UINT16;
> +
> + case GL_RG8UI:
> + retu
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
Fredrik Höglund writes:
>[...]
>> +}
>> +
>> +void GLAPIENTRY
>> +_mesa_MemoryBarrier(GLbitfield barriers)
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> +
>> + if (ctx->Driver.MemoryBarrier)
>> + ctx->Driver.MemoryBarrier(ctx, barriers);
>> +}
>
> Is this the best place to implement this entry point?
> It's not specific to ARB_shader_image_load_store.
Right. It's also going to be used for shader storage blocks, which we
don't support yet -- right now it's only going to have an effect on
shader images, though I'm fine with moving the definition somewhere else
now, if you have a better suggestion. Any ideas?
pgp9aXc4ndftZ.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
On 24 November 2013 21:00, Francisco Jerez wrote:
> +static gl_format
> +get_image_format(GLenum format)
> +{
> + switch (format) {
> + case GL_RGBA32F:
> + return MESA_FORMAT_RGBA_FLOAT32;
> +
> + case GL_RGBA16F:
> + return MESA_FORMAT_RGBA_FLOAT16;
> +
> + case GL_RG32F:
> + return MESA_FORMAT_RG_FLOAT32;
> +
> + case GL_RG16F:
> + return MESA_FORMAT_RG_FLOAT16;
> +
> + case GL_R11F_G11F_B10F:
> + return MESA_FORMAT_R11_G11_B10_FLOAT;
> +
> + case GL_R32F:
> + return MESA_FORMAT_R_FLOAT32;
> +
> + case GL_R16F:
> + return MESA_FORMAT_R_FLOAT16;
> +
> + case GL_RGBA32UI:
> + return MESA_FORMAT_RGBA_UINT32;
> +
> + case GL_RGBA16UI:
> + return MESA_FORMAT_RGBA_UINT16;
> +
> + case GL_RGB10_A2UI:
> + return MESA_FORMAT_ABGR2101010_UINT;
>
I don't understand the naming conventions of the GL and MESA formats well
enough to be sure about this, but I'm troubled by the inconsistency between
the two case statements above. In the GL_RGBA16UI case, the MESA format
lists the components in the same order as the GL format (RGBA). But in the
GL_RGB10_A2UI case, the MESA format lists the components in the opposite
order as the GL format (ABGR instead of RGBA). Unless there are some
really counterintuitive naming conventions at work here, it seems like
these cases can't both be right.
Assuming the above code is really correct, could you add some comments
explaining the apparent inconsistency? Also, do you have piglit tests that
validate the above code?
> +
> + case GL_RGBA8UI:
> + return MESA_FORMAT_RGBA_UINT8;
> +
> + case GL_RG32UI:
> + return MESA_FORMAT_RG_UINT32;
> +
> + case GL_RG16UI:
> + return MESA_FORMAT_RG_UINT16;
> +
> + case GL_RG8UI:
> + return MESA_FORMAT_RG_UINT8;
> +
> + case GL_R32UI:
> + return MESA_FORMAT_R_UINT32;
> +
> + case GL_R16UI:
> + return MESA_FORMAT_R_UINT16;
> +
> + case GL_R8UI:
> + return MESA_FORMAT_R_UINT8;
> +
> + case GL_RGBA32I:
> + return MESA_FORMAT_RGBA_INT32;
> +
> + case GL_RGBA16I:
> + return MESA_FORMAT_RGBA_INT16;
> +
> + case GL_RGBA8I:
> + return MESA_FORMAT_RGBA_INT8;
> +
> + case GL_RG32I:
> + return MESA_FORMAT_RG_INT32;
> +
> + case GL_RG16I:
> + return MESA_FORMAT_RG_INT16;
> +
> + case GL_RG8I:
> + return MESA_FORMAT_RG_INT8;
> +
> + case GL_R32I:
> + return MESA_FORMAT_R_INT32;
> +
> + case GL_R16I:
> + return MESA_FORMAT_R_INT16;
> +
> + case GL_R8I:
> + return MESA_FORMAT_R_INT8;
> +
> + case GL_RGBA16:
> + return MESA_FORMAT_RGBA_16;
> +
> + case GL_RGB10_A2:
> + return MESA_FORMAT_ABGR2101010;
> +
> + case GL_RGBA8:
> + return MESA_FORMAT_RGBA_8;
> +
> + case GL_RG16:
> + return MESA_FORMAT_RG_16;
> +
> + case GL_RG8:
> + return MESA_FORMAT_RG_8;
> +
> + case GL_R16:
> + return MESA_FORMAT_R16;
> +
> + case GL_R8:
> + return MESA_FORMAT_R8;
> +
> + case GL_RGBA16_SNORM:
> + return MESA_FORMAT_SIGNED_RGBA_16;
> +
> + case GL_RGBA8_SNORM:
> + return MESA_FORMAT_SIGNED_RGBA_8;
> +
> + case GL_RG16_SNORM:
> + return MESA_FORMAT_SIGNED_RG_16;
> +
> + case GL_RG8_SNORM:
> + return MESA_FORMAT_SIGNED_RG_8;
> +
> + case GL_R16_SNORM:
> + return MESA_FORMAT_SIGNED_R16;
> +
> + case GL_R8_SNORM:
> + return MESA_FORMAT_SIGNED_R8;
> +
> + default:
> + return MESA_FORMAT_NONE;
> + }
> +}
> +
> +enum image_format_class
> +{
> + /** Not a valid image format. */
> + IMAGE_FORMAT_CLASS_NONE = 0,
> +
> + /** Classes of image formats you can cast into each other. */
> + /** \{ */
> + IMAGE_FORMAT_CLASS_1X8,
> + IMAGE_FORMAT_CLASS_1X16,
> + IMAGE_FORMAT_CLASS_1X32,
> + IMAGE_FORMAT_CLASS_2X8,
> + IMAGE_FORMAT_CLASS_2X16,
> + IMAGE_FORMAT_CLASS_2X32,
> + IMAGE_FORMAT_CLASS_11_11_10,
>
The ARB_shader_load_store spec describes this class as "for 11/11/10 packed
floating-point formats"
> + IMAGE_FORMAT_CLASS_4X8,
> + IMAGE_FORMAT_CLASS_4X16,
> + IMAGE_FORMAT_CLASS_4X32,
> + IMAGE_FORMAT_CLASS_2_10_10_10
>
and it describes this class as "for 10/10/10/2 packed formats". Is there a
good reason why we're reversing the order of the bit counts in one case but
not the other?
> + /** \} */
> +};
> +
> +
> +static GLboolean
> +validate_image_unit(struct gl_context *ctx, struct gl_image_unit *u)
> +{
> + struct gl_texture_object *t = u->TexObj;
> + struct gl_texture_image *img;
> +
> + if (!t || u->Level < t->BaseLevel ||
> + u->Level > t->_MaxLevel)
> + return GL_FALSE;
> +
> + _mesa_test_texobj_completeness(ctx, t);
> +
> + if ((u->Level == t->BaseLevel && !t->_BaseComplete) ||
> + (u->Level != t->BaseLevel && !t->_MipmapComplete))
> + return GL_FALSE;
> +
> + if (u->Layered)
> + img = t->Image[0][u->Level];
> + else
> + img = t->Image[u->Layer][u->Level];
>
This can't be right, because u->Layer can be much larger than
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
On Tuesday 26 November 2013, Francisco Jerez wrote:
> Fredrik Höglund writes:
> >[...]
> >> +}
> >> +
> >> +void GLAPIENTRY
> >> +_mesa_MemoryBarrier(GLbitfield barriers)
> >> +{
> >> + GET_CURRENT_CONTEXT(ctx);
> >> +
> >> + if (ctx->Driver.MemoryBarrier)
> >> + ctx->Driver.MemoryBarrier(ctx, barriers);
> >> +}
> >
> > Is this the best place to implement this entry point?
> > It's not specific to ARB_shader_image_load_store.
>
> Right. It's also going to be used for shader storage blocks, which we
> don't support yet -- right now it's only going to have an effect on
> shader images, though I'm fine with moving the definition somewhere else
> now, if you have a better suggestion. Any ideas?
And ARB_buffer_storage.
I think I would put it in texturebarrier.c and rename the file to
memorybarrier.c. _mesa_TextureBarrierNV can be changed to use
the same driver hook as _mesa_MemoryBarrier.
Fredrik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
Fredrik Höglund writes:
> On Tuesday 26 November 2013, Francisco Jerez wrote:
>> Fredrik Höglund writes:
>> >[...]
>> >> +}
>> >> +
>> >> +void GLAPIENTRY
>> >> +_mesa_MemoryBarrier(GLbitfield barriers)
>> >> +{
>> >> + GET_CURRENT_CONTEXT(ctx);
>> >> +
>> >> + if (ctx->Driver.MemoryBarrier)
>> >> + ctx->Driver.MemoryBarrier(ctx, barriers);
>> >> +}
>> >
>> > Is this the best place to implement this entry point?
>> > It's not specific to ARB_shader_image_load_store.
>>
>> Right. It's also going to be used for shader storage blocks, which we
>> don't support yet -- right now it's only going to have an effect on
>> shader images, though I'm fine with moving the definition somewhere else
>> now, if you have a better suggestion. Any ideas?
>
> And ARB_buffer_storage.
>
Ehm... Yes and no. The point of glMemoryBarrier() is to serialize
image writes and atomics with other operations that might manipulate the
texture data bound to an image unit [or, if
ARB_shader_storage_buffer_object is supported, to serialize storage
block writes and atomics with other operations]. ARB_buffer_storage
interacts with it because its persistent mappings are among those memory
operations that can be serialized with shader storage blocks and images.
> I think I would put it in texturebarrier.c and rename the file to
> memorybarrier.c. _mesa_TextureBarrierNV can be changed to use the
> same driver hook as _mesa_MemoryBarrier.
>
Well, in unextended GL glMemoryBarrier doesn't necessarily have any
interaction with fragment shader outputs, so glTextureBarrierNV() is not
necessarily equivalent to glMemoryBarrier(GL_TEXTURE_FETCH_BARRIER_BIT).
We probably want to keep both driver hooks around. Other than that I'm
OK with renaming texturebarrier.c and moving the definition of
glMemoryBarrier, though I don't think it's extremely urgent because
glMemoryBarrier is specific to ARB_shader_image_load_store at this
point. Feel free to send a follow-up patch series making the necessary
changes.
> Fredrik
pgpMZk6qOz3qn.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
Paul Berry writes:
> On 24 November 2013 21:00, Francisco Jerez wrote:
>[...]
>> +
>> + case GL_RGBA16UI:
>> + return MESA_FORMAT_RGBA_UINT16;
>> +
>> + case GL_RGB10_A2UI:
>> + return MESA_FORMAT_ABGR2101010_UINT;
>>
>
> I don't understand the naming conventions of the GL and MESA formats well
> enough to be sure about this, but I'm troubled by the inconsistency between
> the two case statements above. In the GL_RGBA16UI case, the MESA format
> lists the components in the same order as the GL format (RGBA). But in the
> GL_RGB10_A2UI case, the MESA format lists the components in the opposite
> order as the GL format (ABGR instead of RGBA). Unless there are some
> really counterintuitive naming conventions at work here, it seems like
> these cases can't both be right.
>
Yes. Mesa's formats mix two naming conventions: MESA_FORMAT_XYZW_N
specifies the XYZW components in memory order, each of N bits, while
MESA_FORMAT_XYZWNMPQ specifies the XYZW components from MSB to LSB in an
endianness-dependent manner. The GL_RGB10_A2UI internal format refers
to an RGBA_INTEGER format of type UNSIGNED_INT_2_10_10_10_REV, which
matches the Mesa format above -- See table 8.8 in the GL4.4 spec.
> Assuming the above code is really correct, could you add some comments
> explaining the apparent inconsistency? Also, do you have piglit tests that
> validate the above code?
>
No, sorry.
>[...]
>> + IMAGE_FORMAT_CLASS_11_11_10,
>>
>
> The ARB_shader_load_store spec describes this class as "for 11/11/10 packed
> floating-point formats"
>
>
>> + IMAGE_FORMAT_CLASS_4X8,
>> + IMAGE_FORMAT_CLASS_4X16,
>> + IMAGE_FORMAT_CLASS_4X32,
>> + IMAGE_FORMAT_CLASS_2_10_10_10
>>
>
> and it describes this class as "for 10/10/10/2 packed formats". Is there a
> good reason why we're reversing the order of the bit counts in one case but
> not the other?
>
Yeah... We should probably follow the same naming convention in both
classes.
>
>> + /** \} */
>> +};
>> +
>> +
>> +static GLboolean
>> +validate_image_unit(struct gl_context *ctx, struct gl_image_unit *u)
>> +{
>> + struct gl_texture_object *t = u->TexObj;
>> + struct gl_texture_image *img;
>> +
>> + if (!t || u->Level < t->BaseLevel ||
>> + u->Level > t->_MaxLevel)
>> + return GL_FALSE;
>> +
>> + _mesa_test_texobj_completeness(ctx, t);
>> +
>> + if ((u->Level == t->BaseLevel && !t->_BaseComplete) ||
>> + (u->Level != t->BaseLevel && !t->_MipmapComplete))
>> + return GL_FALSE;
>> +
>> + if (u->Layered)
>> + img = t->Image[0][u->Level];
>> + else
>> + img = t->Image[u->Layer][u->Level];
>>
>
> This can't be right, because u->Layer can be much larger than 6, but the
> size of the t->Image[] array is 6. I believe what we need to do instead is:
>
> (a) for cubemaps, we need to validate that 0 <= u->Layer < 6 before
> accessing t->Image, otherwise we will access garbage memory.
>
> (b) for non-cubemaps (e.g. 2D arrays), all the layers for each miplevel are
> stored in a single gl_texture_image, so we need to access
> t->Image[0][u->Level], and then check that 0 <= u->Layer < t->Depth.
>
> (c) I'm not sure how cubemap arrays are handled by the Mesa front end, so
> I'm not sure what is correct to do for them.
>
I've addressed these issues here [1], I can send a v2 to the mailing
list if you have more suggestions specific to the new version.
> Also, the spec says:
>
> If the texture identified by does not have multiple layers or
> faces, the entire texture level is bound, regardless of the values
> specified by and .
>
> I think that means that if the texture is non-layered, we should always set
> img = t->Image[0][u->Level], regardless of the setting of u->Layered.
>
> Note: as a follow up, it might be worth creating a
> _mesa_tex_target_is_layered() function so that we don't have to duplicate
> the knowledge of which texture targets are layered in multiple places.
>
OK, I've done that. It looks like the code dealing with layered
framebuffers could benefit from the same function.
>
>> +
>> + if (!img || img->Border ||
>> + get_image_format_class(img->TexFormat) == IMAGE_FORMAT_CLASS_NONE
>> ||
>> + img->NumSamples > ctx->Const.MaxImageSamples)
>> + return GL_FALSE;
>> +
>> + if (t->ImageFormatCompatibility ==
>> GL_IMAGE_FORMAT_COMPATIBILITY_BY_SIZE &&
>> + _mesa_get_format_bytes(img->TexFormat) !=
>> + _mesa_get_format_bytes(u->_ActualFormat))
>> + return GL_FALSE;
>> +
>> + if (t->ImageFormatCompatibility ==
>> GL_IMAGE_FORMAT_COMPATIBILITY_BY_CLASS &&
>> + get_image_format_class(img->TexFormat) !=
>> + get_image_format_class(u->_ActualFormat))
>> + return GL_FALSE;
>>
>
> If some bug elsewhere in Mesa causes t->ImageFormatCompatibility to get set
> to something other than GL_IMAGE_FORMAT_COMPATIBILITY_BY_SIZE or
> GL_IMAGE_FORMAT_COMPATIBILITY_BY_CLASS, then the code above will silently
> accept the texture and we probably won't notice the b
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
On 7 December 2013 17:11, Francisco Jerez wrote: > Paul Berry writes: > > > On 24 November 2013 21:00, Francisco Jerez > wrote: > >[...] > >> + > >> + case GL_RGBA16UI: > >> + return MESA_FORMAT_RGBA_UINT16; > >> + > >> + case GL_RGB10_A2UI: > >> + return MESA_FORMAT_ABGR2101010_UINT; > >> > > > > I don't understand the naming conventions of the GL and MESA formats well > > enough to be sure about this, but I'm troubled by the inconsistency > between > > the two case statements above. In the GL_RGBA16UI case, the MESA format > > lists the components in the same order as the GL format (RGBA). But in > the > > GL_RGB10_A2UI case, the MESA format lists the components in the opposite > > order as the GL format (ABGR instead of RGBA). Unless there are some > > really counterintuitive naming conventions at work here, it seems like > > these cases can't both be right. > > > > Yes. Mesa's formats mix two naming conventions: MESA_FORMAT_XYZW_N > specifies the XYZW components in memory order, each of N bits, while > MESA_FORMAT_XYZWNMPQ specifies the XYZW components from MSB to LSB in an > endianness-dependent manner. The GL_RGB10_A2UI internal format refers > to an RGBA_INTEGER format of type UNSIGNED_INT_2_10_10_10_REV, which > matches the Mesa format above -- See table 8.8 in the GL4.4 spec. > Wow, that explains why I never managed to pick up the convention by osmosis. Thanks for explaining. BTW, after reading through format_pack.c, I found some exceptions to what you said above. The following formats are always packed by format_pack.c in reverse memory order, regardless of endianness: - MESA_FORMAT_RGB888 - MESA_FORMAT_BGR888 - MESA_FORMAT_SRGB8 - MESA_FORMAT_XBGR16161616_UNORM - MESA_FORMAT_XBGR16161616_SNORM - MESA_FORMAT_XBGR16161616_FLOAT - MESA_FORMAT_XBGR32323232_FLOAT Which means they get packed correctly on little-endian systems but backwards on big-endian systems. This seems like it's a bug in format_pack.c, so it's not relevant to your patch :) > > > This can't be right, because u->Layer can be much larger than 6, but the > > size of the t->Image[] array is 6. I believe what we need to do instead > is: > > > > (a) for cubemaps, we need to validate that 0 <= u->Layer < 6 before > > accessing t->Image, otherwise we will access garbage memory. > > > > (b) for non-cubemaps (e.g. 2D arrays), all the layers for each miplevel > are > > stored in a single gl_texture_image, so we need to access > > t->Image[0][u->Level], and then check that 0 <= u->Layer < t->Depth. > > > > (c) I'm not sure how cubemap arrays are handled by the Mesa front end, so > > I'm not sure what is correct to do for them. > > > > I've addressed these issues here [1], I can send a v2 to the mailing > list if you have more suggestions specific to the new version. > Personally, I don't need to see a v2. I looked at what you currently have on your branch (a801b805b3016eb905028726ea20a3338c4d7662), and it looks good to me. It's Reviewed-by: Paul Berry ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.
Paul Berry writes: > On 7 December 2013 17:11, Francisco Jerez wrote: > >> Paul Berry writes: >[...] > > Wow, that explains why I never managed to pick up the convention by > osmosis. Thanks for explaining. > > BTW, after reading through format_pack.c, I found some exceptions to what > you said above. The following formats are always packed by format_pack.c > in reverse memory order, regardless of endianness: > > - MESA_FORMAT_RGB888 > - MESA_FORMAT_BGR888 > - MESA_FORMAT_SRGB8 > - MESA_FORMAT_XBGR16161616_UNORM > - MESA_FORMAT_XBGR16161616_SNORM > - MESA_FORMAT_XBGR16161616_FLOAT > - MESA_FORMAT_XBGR32323232_FLOAT > > Which means they get packed correctly on little-endian systems but > backwards on big-endian systems. This seems like it's a bug in > format_pack.c, so it's not relevant to your patch :) I suspect that format_pack.c is correct and those formats were meant to be packed with a fixed memory ordering, it's just the naming that seems to be inconsistent. pgpSjgvFN9CuG.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
