<div dir="ltr">On 24 November 2013 21:00, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+static gl_format<br>
+get_image_format(GLenum format)<br>
+{<br>
+   switch (format) {<br>
+   case GL_RGBA32F:<br>
+      return MESA_FORMAT_RGBA_FLOAT32;<br>
+<br>
+   case GL_RGBA16F:<br>
+      return MESA_FORMAT_RGBA_FLOAT16;<br>
+<br>
+   case GL_RG32F:<br>
+      return MESA_FORMAT_RG_FLOAT32;<br>
+<br>
+   case GL_RG16F:<br>
+      return MESA_FORMAT_RG_FLOAT16;<br>
+<br>
+   case GL_R11F_G11F_B10F:<br>
+      return MESA_FORMAT_R11_G11_B10_FLOAT;<br>
+<br>
+   case GL_R32F:<br>
+      return MESA_FORMAT_R_FLOAT32;<br>
+<br>
+   case GL_R16F:<br>
+      return MESA_FORMAT_R_FLOAT16;<br>
+<br>
+   case GL_RGBA32UI:<br>
+      return MESA_FORMAT_RGBA_UINT32;<br>
+<br>
+   case GL_RGBA16UI:<br>
+      return MESA_FORMAT_RGBA_UINT16;<br>
+<br>
+   case GL_RGB10_A2UI:<br>
+      return MESA_FORMAT_ABGR2101010_UINT;<br></blockquote><div><br></div><div>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.<br>

<br>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?  <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


+<br>
+   case GL_RGBA8UI:<br>
+      return MESA_FORMAT_RGBA_UINT8;<br>
+<br>
+   case GL_RG32UI:<br>
+      return MESA_FORMAT_RG_UINT32;<br>
+<br>
+   case GL_RG16UI:<br>
+      return MESA_FORMAT_RG_UINT16;<br>
+<br>
+   case GL_RG8UI:<br>
+      return MESA_FORMAT_RG_UINT8;<br>
+<br>
+   case GL_R32UI:<br>
+      return MESA_FORMAT_R_UINT32;<br>
+<br>
+   case GL_R16UI:<br>
+      return MESA_FORMAT_R_UINT16;<br>
+<br>
+   case GL_R8UI:<br>
+      return MESA_FORMAT_R_UINT8;<br>
+<br>
+   case GL_RGBA32I:<br>
+      return MESA_FORMAT_RGBA_INT32;<br>
+<br>
+   case GL_RGBA16I:<br>
+      return MESA_FORMAT_RGBA_INT16;<br>
+<br>
+   case GL_RGBA8I:<br>
+      return MESA_FORMAT_RGBA_INT8;<br>
+<br>
+   case GL_RG32I:<br>
+      return MESA_FORMAT_RG_INT32;<br>
+<br>
+   case GL_RG16I:<br>
+      return MESA_FORMAT_RG_INT16;<br>
+<br>
+   case GL_RG8I:<br>
+      return MESA_FORMAT_RG_INT8;<br>
+<br>
+   case GL_R32I:<br>
+      return MESA_FORMAT_R_INT32;<br>
+<br>
+   case GL_R16I:<br>
+      return MESA_FORMAT_R_INT16;<br>
+<br>
+   case GL_R8I:<br>
+      return MESA_FORMAT_R_INT8;<br>
+<br>
+   case GL_RGBA16:<br>
+      return MESA_FORMAT_RGBA_16;<br>
+<br>
+   case GL_RGB10_A2:<br>
+      return MESA_FORMAT_ABGR2101010;<br>
+<br>
+   case GL_RGBA8:<br>
+      return MESA_FORMAT_RGBA_8;<br>
+<br>
+   case GL_RG16:<br>
+      return MESA_FORMAT_RG_16;<br>
+<br>
+   case GL_RG8:<br>
+      return MESA_FORMAT_RG_8;<br>
+<br>
+   case GL_R16:<br>
+      return MESA_FORMAT_R16;<br>
+<br>
+   case GL_R8:<br>
+      return MESA_FORMAT_R8;<br>
+<br>
+   case GL_RGBA16_SNORM:<br>
+      return MESA_FORMAT_SIGNED_RGBA_16;<br>
+<br>
+   case GL_RGBA8_SNORM:<br>
+      return MESA_FORMAT_SIGNED_RGBA_8;<br>
+<br>
+   case GL_RG16_SNORM:<br>
+      return MESA_FORMAT_SIGNED_RG_16;<br>
+<br>
+   case GL_RG8_SNORM:<br>
+      return MESA_FORMAT_SIGNED_RG_8;<br>
+<br>
+   case GL_R16_SNORM:<br>
+      return MESA_FORMAT_SIGNED_R16;<br>
+<br>
+   case GL_R8_SNORM:<br>
+      return MESA_FORMAT_SIGNED_R8;<br>
+<br>
+   default:<br>
+      return MESA_FORMAT_NONE;<br>
+   }<br>
+}<br>
+<br>
+enum image_format_class<br>
+{<br>
+   /** Not a valid image format. */<br>
+   IMAGE_FORMAT_CLASS_NONE = 0,<br>
+<br>
+   /** Classes of image formats you can cast into each other. */<br>
+   /** \{ */<br>
+   IMAGE_FORMAT_CLASS_1X8,<br>
+   IMAGE_FORMAT_CLASS_1X16,<br>
+   IMAGE_FORMAT_CLASS_1X32,<br>
+   IMAGE_FORMAT_CLASS_2X8,<br>
+   IMAGE_FORMAT_CLASS_2X16,<br>
+   IMAGE_FORMAT_CLASS_2X32,<br>
+   IMAGE_FORMAT_CLASS_11_11_10,<br></blockquote><div><br></div><div>The ARB_shader_load_store spec describes this class as "for 11/11/10 packed floating-point formats"<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


+   IMAGE_FORMAT_CLASS_4X8,<br>
+   IMAGE_FORMAT_CLASS_4X16,<br>
+   IMAGE_FORMAT_CLASS_4X32,<br>
+   IMAGE_FORMAT_CLASS_2_10_10_10<br></blockquote><div><br></div><div>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?<br>

</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+   /** \} */<br>
+};<br>
+<br>
+<br>
+static GLboolean<br>
+validate_image_unit(struct gl_context *ctx, struct gl_image_unit *u)<br>
+{<br>
+   struct gl_texture_object *t = u->TexObj;<br>
+   struct gl_texture_image *img;<br>
+<br>
+   if (!t || u->Level < t->BaseLevel ||<br>
+       u->Level > t->_MaxLevel)<br>
+      return GL_FALSE;<br>
+<br>
+   _mesa_test_texobj_completeness(ctx, t);<br>
+<br>
+   if ((u->Level == t->BaseLevel && !t->_BaseComplete) ||<br>
+       (u->Level != t->BaseLevel && !t->_MipmapComplete))<br>
+      return GL_FALSE;<br>
+<br>
+   if (u->Layered)<br>
+      img = t->Image[0][u->Level];<br>
+   else<br>
+      img = t->Image[u->Layer][u->Level];<br></blockquote><div><br></div><div>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:<br>

<br></div><div>(a) for cubemaps, we need to validate that 0 <= u->Layer < 6 before accessing t->Image, otherwise we will access garbage memory.<br></div><div><br>(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.<br>

<br></div><div>(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.<br><br></div><div>Also, the spec says:<br><br>    If the texture identified by <texture> does not have multiple layers or<br>
    faces, the entire texture level is bound, regardless of the values<br>    specified by <layered> and <layer>.<br><br></div><div>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.<br>
<br>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. <br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

+<br>
+   if (!img || img->Border ||<br>
+       get_image_format_class(img->TexFormat) == IMAGE_FORMAT_CLASS_NONE ||<br>
+       img->NumSamples > ctx->Const.MaxImageSamples)<br>
+      return GL_FALSE;<br>
+<br>
+   if (t->ImageFormatCompatibility == GL_IMAGE_FORMAT_COMPATIBILITY_BY_SIZE &&<br>
+       _mesa_get_format_bytes(img->TexFormat) !=<br>
+       _mesa_get_format_bytes(u->_ActualFormat))<br>
+      return GL_FALSE;<br>
+<br>
+   if (t->ImageFormatCompatibility == GL_IMAGE_FORMAT_COMPATIBILITY_BY_CLASS &&<br>
+       get_image_format_class(img->TexFormat) !=<br>
+       get_image_format_class(u->_ActualFormat))<br>
+      return GL_FALSE;<br></blockquote><div><br></div><div>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 bug.  How about if we do something like this instead?<br>

<br>switch (t->ImageFormatCompatibility) {<br></div><div>case GL_IMAGE_FORMAT_COMPATIBILITY_BY_SIZE:<br></div><div>   if (_mesa_get_format_bytes(img->TexFormat) !=<br></div><div>       _mesa_get_format_bytes(u->_ActualFormat)) {<br>

</div><div>      return GL_FALSE;<br>   }<br></div><div>   break;<br></div><div>case GL_IMAGE_FORMAT_COMPATIBILITY_BY_CLASS:<br></div><div>   if (get_image_format_class(img->TexFormat) !=<br></div><div>       get_image_format_class(u->_ActualFormat)) {<br>

</div><div>      return GL_FALSE;<br>   }<br></div><div>   break;<br></div><div>default:<br></div><div>   assert(!"Unexpected image format compatibility type");<br>}<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


+<br>
+   return GL_TRUE;<br>
+}<br>
+<br>
+static GLboolean<br>
+validate_bind_image_texture(struct gl_context *ctx, GLuint unit,<br>
+                            GLuint texture, GLint level, GLboolean layered,<br>
+                            GLint layer, GLenum access, GLenum format)<br>
+{<br></blockquote><div><br></div><div>Can we add the following assert:<br><br></div><div>assert(ctx->Const.MaxImageUnits <= MAX_IMAGE_UNITS);<br><br></div><div>Otherwise there's a danger that a driver might accidentally set ctx->Const.MaxImageUnits to a value greater than MAX_IMAGE_UNITS, and then some of the array accesses below would overflow.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+   if (unit > ctx->Const.MaxImageUnits) {<br></blockquote><div><br></div><div>This should be ">=".<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


+      _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(unit)");<br>
+      return GL_FALSE;<br>
+   }<br>
+<br>
+   if (level < 0) {<br>
+      _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(level)");<br>
+      return GL_FALSE;<br>
+   }<br>
+<br>
+   if (layer < 0) {<br>
+      _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(layer)");<br>
+      return GL_FALSE;<br>
+   }<br>
+<br>
+   if (access != GL_READ_ONLY &&<br>
+       access != GL_WRITE_ONLY &&<br>
+       access != GL_READ_WRITE) {<br>
+      _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(access)");<br>
+      return GL_FALSE;<br>
+   } <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+   if (!get_image_format(format)) {<br>
+      _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(format)");<br>
+      return GL_FALSE;<br>
+   }<br>
+<br>
+   return GL_TRUE;<br>
+}<br>
+<br>
+void GLAPIENTRY<br>
+_mesa_BindImageTexture(GLuint unit, GLuint texture, GLint level,<br>
+                       GLboolean layered, GLint layer, GLenum access,<br>
+                       GLenum format)<br>
+{<br>
+   GET_CURRENT_CONTEXT(ctx);<br>
+   struct gl_texture_object *t = NULL;<br>
+   struct gl_image_unit *u;<br>
+<br>
+   if (!validate_bind_image_texture(ctx, unit, texture, level,<br>
+                                    layered, layer, access, format))<br>
+      return;<br>
+<br>
+   u = &ctx->ImageUnits[unit];<br>
+<br>
+   FLUSH_VERTICES(ctx, 0);<br>
+   ctx->NewDriverState |= ctx->DriverFlags.NewImageUnits;<br>
+<br>
+   if (texture) {<br>
+      t = _mesa_lookup_texture(ctx, texture);<br>
+      if (!t) {<br>
+         _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(texture)");<br>
+         return;<br>
+      }<br>
+<br>
+      _mesa_reference_texobj(&u->TexObj, t);<br>
+      u->Level = level;<br>
+      u->Layered = layered;<br>
+      u->Layer = (layered ? 0 : layer);<br>
+      u->Access = access;<br>
+      u->Format = format;<br>
+      u->_ActualFormat = get_image_format(format);<br>
+      u->_Valid = validate_image_unit(ctx, u);<br>
+<br>
+   } else {<br>
+      _mesa_reference_texobj(&u->TexObj, NULL);<br>
+      u->_Valid = GL_FALSE;<br>
+   }<br></blockquote><div><br></div><div>I'd prefer to move the call to validate_image_unit() outside the if/then statement.  That way we exercise the code in validate_image_unit() that marks an image unit as invalid when its TexObj is NULL.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+   if (ctx->Driver.BindImageTexture)<br>
+      ctx->Driver.BindImageTexture(ctx, u, t, level, layered,<br>
+                                   layer, access, format);<br>
+}<br>
+<br></blockquote></div></div></div>