Paul Berry <stereotype...@gmail.com> writes: > On 24 November 2013 21:00, Francisco Jerez <curroje...@riseup.net> 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 <texture> does not have multiple layers or > faces, the entire texture level is bound, regardless of the values > specified by <layered> and <layer>. > > 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 bug. How about if we > do something like this instead? > > switch (t->ImageFormatCompatibility) { > case GL_IMAGE_FORMAT_COMPATIBILITY_BY_SIZE: > if (_mesa_get_format_bytes(img->TexFormat) != > _mesa_get_format_bytes(u->_ActualFormat)) { > return GL_FALSE; > } > break; > case GL_IMAGE_FORMAT_COMPATIBILITY_BY_CLASS: > if (get_image_format_class(img->TexFormat) != > get_image_format_class(u->_ActualFormat)) { > return GL_FALSE; > } > break; > default: > assert(!"Unexpected image format compatibility type"); > } > Hm... Why not. > >> + >> + return GL_TRUE; >> +} >> + >> +static GLboolean >> +validate_bind_image_texture(struct gl_context *ctx, GLuint unit, >> + GLuint texture, GLint level, GLboolean >> layered, >> + GLint layer, GLenum access, GLenum format) >> +{ >> > > Can we add the following assert: > > assert(ctx->Const.MaxImageUnits <= MAX_IMAGE_UNITS); > > 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. > Okay. > >> + if (unit > ctx->Const.MaxImageUnits) { >> > > This should be ">=". > I could swear I had fixed that one already. > >> + _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(unit)"); >> + return GL_FALSE; >> + } >> + >> + if (level < 0) { >> + _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(level)"); >> + return GL_FALSE; >> + } >> + >> + if (layer < 0) { >> + _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(layer)"); >> + return GL_FALSE; >> + } >> + >> + if (access != GL_READ_ONLY && >> + access != GL_WRITE_ONLY && >> + access != GL_READ_WRITE) { >> + _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(access)"); >> + return GL_FALSE; >> + } >> > + >> + if (!get_image_format(format)) { >> + _mesa_error(ctx, GL_INVALID_VALUE, "glBindImageTexture(format)"); >> + return GL_FALSE; >> + } >> + >> + return GL_TRUE; >> +} >> + >> +void GLAPIENTRY >> +_mesa_BindImageTexture(GLuint unit, GLuint texture, GLint level, >> + GLboolean layered, GLint layer, GLenum access, >> + GLenum format) >> +{ >> + GET_CURRENT_CONTEXT(ctx); >> + struct gl_texture_object *t = NULL; >> + struct gl_image_unit *u; >> + >> + if (!validate_bind_image_texture(ctx, unit, texture, level, >> + layered, layer, access, format)) >> + return; >> + >> + u = &ctx->ImageUnits[unit]; >> + >> + FLUSH_VERTICES(ctx, 0); >> + ctx->NewDriverState |= ctx->DriverFlags.NewImageUnits; >> + >> + if (texture) { >> + t = _mesa_lookup_texture(ctx, texture); >> + if (!t) { >> + _mesa_error(ctx, GL_INVALID_VALUE, >> "glBindImageTexture(texture)"); >> + return; >> + } >> + >> + _mesa_reference_texobj(&u->TexObj, t); >> + u->Level = level; >> + u->Layered = layered; >> + u->Layer = (layered ? 0 : layer); >> + u->Access = access; >> + u->Format = format; >> + u->_ActualFormat = get_image_format(format); >> + u->_Valid = validate_image_unit(ctx, u); >> + >> + } else { >> + _mesa_reference_texobj(&u->TexObj, NULL); >> + u->_Valid = GL_FALSE; >> + } >> > > 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. > The code looks slightly clearer to me as it is, but I don't really have a strong preference. > >> + >> + if (ctx->Driver.BindImageTexture) >> + ctx->Driver.BindImageTexture(ctx, u, t, level, layered, >> + layer, access, format); >> +} >> + >> Thanks. [1] http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=10026c1e80371a27438930d7134f76dec56da53d
pgpg5JqTIbHUo.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev