On Tue, Mar 4, 2014 at 2:14 AM, Ian Romanick <i...@freedesktop.org> wrote: > On 03/02/2014 12:09 AM, Ilia Mirkin wrote: >> >> This adds enough code to let drivers implement texture clearing, but >> doesn't actually do that for any of them. > > > There's always the usual... this should be split into two patches. :) > Generally the changes in gl_API, dispatch_sanity, and "stub" functions is > the first patch. Changes to dd.h is a patch. Implementing the functions is > a third.
OK, will do. > > >> >> Signed-off-by: Ilia Mirkin <imir...@alum.mit.edu> >> --- >> >> Thought I'd give this a shot. Am I on the right track here? Is the dd API >> reasonable? I haven't written a piglit test for all the "generic" error >> cases >> yet, but I'll get to that in a bit. I'm a little confused between all the >> different formats on a teximage... BaseFormat appears to be some >> generalization of the format, but what's the difference between >> InternalFormat >> and TexFormat? Just the type, but otherwise equivalent? Or are they >> representing different things? > > > BaseFormat is the generic format. So, GL_RGB or GL_RED are the base formats > that go with GL_RGB8 or GL_RED16. internalFormat is usually the GL format > (e.g., GL_RGBA32F), and TexFormat is usually the Mesa format (e.g., > MESA_FORMAT_RGBA_FLOAT32). So TexFormat == InternalFormat, right? (Just different types) > > >> BTW, this is the first time I'm adding stuff to mesa/main, so forgive me >> if I >> left out a step. >> >> src/mapi/glapi/gen/ARB_clear_texture.xml | 34 ++++++++ >> src/mapi/glapi/gen/gl_API.xml | 2 + >> src/mesa/main/dd.h | 11 +++ >> src/mesa/main/formatquery.c | 9 +++ >> src/mesa/main/mtypes.h | 1 + >> src/mesa/main/tests/dispatch_sanity.cpp | 4 + >> src/mesa/main/teximage.c | 131 >> +++++++++++++++++++++++++++++++ >> src/mesa/main/teximage.h | 10 +++ >> 8 files changed, 202 insertions(+) >> create mode 100644 src/mapi/glapi/gen/ARB_clear_texture.xml >> >> diff --git a/src/mapi/glapi/gen/ARB_clear_texture.xml >> b/src/mapi/glapi/gen/ARB_clear_texture.xml >> new file mode 100644 >> index 0000000..bd9116f >> --- /dev/null >> +++ b/src/mapi/glapi/gen/ARB_clear_texture.xml >> @@ -0,0 +1,34 @@ >> +<?xml version="1.0"?> >> +<!DOCTYPE OpenGLAPI SYSTEM "gl_API.dtd"> >> + >> +<OpenGLAPI> >> + >> +<category name="GL_ARB_clear_texture" number="145"> >> + >> + <enum name="CLEAR_TEXTURE" value="0x9365"/> >> + >> + <function name ="ClearTexImage" offset="assign"> >> + <param name="texture" type="GLuint"/> >> + <param name="level" type="GLint"/> >> + <param name="format" type="GLenum"/> >> + <param name="type" type="GLenum"/> >> + <param name="data" type="const GLvoid *"/> >> + </function> >> + >> + <function name ="ClearTexSubImage" offset="assign"> >> + <param name="texture" type="GLuint"/> >> + <param name="level" type="GLint"/> >> + <param name="xoffset" type="GLint"/> >> + <param name="yoffset" type="GLint"/> >> + <param name="zoffset" type="GLint"/> >> + <param name="width" type="GLsizei"/> >> + <param name="height" type="GLsizei"/> >> + <param name="depth" type="GLsizei"/> >> + <param name="format" type="GLenum"/> >> + <param name="type" type="GLenum"/> >> + <param name="data" type="const GLvoid *"/> >> + </function> >> + >> +</category> >> + >> +</OpenGLAPI> >> diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml >> index 7e1946e..15f8e32 100644 >> --- a/src/mapi/glapi/gen/gl_API.xml >> +++ b/src/mapi/glapi/gen/gl_API.xml >> @@ -8515,6 +8515,8 @@ >> </function> >> </category> >> >> +<xi:include href="ARB_clear_texture.xml" >> xmlns:xi="http://www.w3.org/2001/XInclude"/> >> + >> <!-- Non-ARB extensions sorted by extension number. --> >> >> <category name="GL_EXT_blend_color" number="2"> >> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h >> index 9715241..d824bca 100644 >> --- a/src/mesa/main/dd.h >> +++ b/src/mesa/main/dd.h >> @@ -275,6 +275,17 @@ struct dd_function_table { >> GLint level, mesa_format format, >> GLint width, GLint height, >> GLint depth, GLint border); >> + >> + >> + void (*ClearTexSubImage)(struct gl_context *ctx, >> + struct gl_texture_image *texImage, >> + GLint xoffset, GLint yoffset, GLint zoffset, >> + GLsizei width, GLsizei height, GLsizei depth, >> + GLenum format, GLenum type, const GLvoid >> *data); >> + >> + GLenum (*QuerySupportForClearTex)(struct gl_context *ctx, >> + GLenum internalFormat); > > > I think I'd rather add a generic query function. GL_ARB_query_internalformt2 > add a large number of target/internalFormat/pname queries. I'm having > difficulty deciding whether it's better to just pipe this interface directly > into the driver or use something slightly more abstract. Maybe add a > function like > > GLenum (*QueryFormatSupport)(struct gl_context *ctx, > GLenum internalFormat, > GLenum pname); Others have also pointed out to me that query2 isn't supported, so I'm just going to drop it for now. > > > >> + >> /*@}*/ >> >> >> diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c >> index 40eca87..7997cb4 100644 >> --- a/src/mesa/main/formatquery.c >> +++ b/src/mesa/main/formatquery.c >> @@ -140,6 +140,15 @@ _mesa_GetInternalformativ(GLenum target, GLenum >> internalformat, GLenum pname, >> count = 1; >> break; >> } >> + case GL_CLEAR_TEXTURE: >> + if (ctx->Extensions.ARB_clear_texture) { >> + const GLenum support = ctx->Driver.QuerySupportForClearTex( >> + ctx, internalformat); >> + buffer[0] = (GLint) support; >> + count = 1; >> + break; >> + } >> + /* fallthrough */ >> default: >> _mesa_error(ctx, GL_INVALID_ENUM, >> "glGetInternalformativ(pname=%s)", >> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h >> index 7246b1e..e4a3837 100644 >> --- a/src/mesa/main/mtypes.h >> +++ b/src/mesa/main/mtypes.h >> @@ -3490,6 +3490,7 @@ struct gl_extensions >> GLboolean ARB_base_instance; >> GLboolean ARB_blend_func_extended; >> GLboolean ARB_buffer_storage; >> + GLboolean ARB_clear_texture; >> GLboolean ARB_color_buffer_float; >> GLboolean ARB_compute_shader; >> GLboolean ARB_conservative_depth; >> diff --git a/src/mesa/main/tests/dispatch_sanity.cpp >> b/src/mesa/main/tests/dispatch_sanity.cpp >> index d1b0011..8690b5d 100644 >> --- a/src/mesa/main/tests/dispatch_sanity.cpp >> +++ b/src/mesa/main/tests/dispatch_sanity.cpp >> @@ -936,6 +936,10 @@ const struct function gl_core_functions_possible[] = >> { >> /* GL_ARB_buffer_storage */ >> { "glBufferStorage", 43, -1 }, >> >> + /* GL_ARB_clear_texture */ >> + { "glClearTexImage", 13, -1 }, >> + { "glClearTexSubImage", 13, -1 }, >> + >> { NULL, 0, -1 } >> }; >> >> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c >> index 0519d22..9171187 100644 >> --- a/src/mesa/main/teximage.c >> +++ b/src/mesa/main/teximage.c >> @@ -3768,6 +3768,137 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint >> level, >> x, y, width, height); >> } >> >> +static struct gl_texture_image * >> +get_tex_image(struct gl_context *ctx, GLuint texture, GLint level, GLint >> zoffset) >> +{ >> + GLenum target = 0; >> + struct gl_texture_object *texObj = _mesa_lookup_texture(ctx, texture); >> + struct gl_texture_image *texImage; >> + >> + if (!texture || !texObj) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid texture)"); >> + return NULL; >> + } >> + >> + switch (texObj->Target) { >> + case GL_TEXTURE_BUFFER: >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(texture can't be buffer)"); >> + return NULL; >> + case GL_TEXTURE_CUBE_MAP_ARB: >> + case GL_TEXTURE_CUBE_MAP_ARRAY: >> + target = GL_TEXTURE_CUBE_MAP_POSITIVE_X + zoffset; >> + break; >> + default: >> + break; >> + } >> + >> + texImage = _mesa_select_tex_image(ctx, texObj, target, level); >> + if (!texImage) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(missing image)"); >> + return NULL; >> + } >> + >> + return texImage; >> +} >> + >> +static void >> +clear_tex_sub_image(struct gl_context *ctx, struct gl_texture_image >> *texImage, >> + GLint xoffset, GLint yoffset, GLint zoffset, >> + GLsizei width, GLsizei height, GLsizei depth, >> + GLenum format, GLenum type, const void *data) >> +{ >> + if (!texImage) >> + return; >> + >> + if (_mesa_is_compressed_format(ctx, texImage->InternalFormat)) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(compressed format)"); > > > This will cause the wrong function name to get logged for glClearTexImage. > Similar code in other parts of Mesa will pass a 'const char *caller' to the > utility function. OK, didn't think it was important, but it's also really easy to fix, so I'll do that. (When I first wrote the code, it was not going to be easy to fix, but I since moved all the code into the helper as you see it here.) > > >> + return; >> + } >> + >> + if (texImage->_BaseFormat == GL_DEPTH_COMPONENT && >> + format != GL_DEPTH_COMPONENT) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid format)"); >> + return; >> + } >> + if (texImage->_BaseFormat == GL_DEPTH_STENCIL && >> + format != GL_DEPTH_STENCIL) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid format)"); >> + return; >> + } >> + if (texImage->_BaseFormat == GL_STENCIL_INDEX && >> + format != GL_STENCIL_INDEX) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid format)"); >> + return; >> + } >> + if (texImage->_BaseFormat == GL_RGBA && >> + (format == GL_DEPTH_COMPONENT || >> + format == GL_DEPTH_STENCIL || >> + format == GL_STENCIL_INDEX)) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid format)"); >> + return; >> + } >> + if (_mesa_is_format_integer_color(texImage->InternalFormat) != >> + _mesa_is_format_integer_color(format)) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid format)"); >> + return; >> + } > > > I think the format validation is missing some bits. What will happen if the > application passes a completely garbage value for format, > texImage->_BaseFormat is GL_RGBA and texImage->InternalFormat is not an > integer color format? Nothing good :) I believe that Brian's suggested changes to the validation logic should fix this. > > >> + >> + if (xoffset < -texImage->Border || >> + xoffset + width > texImage->Width - texImage->Border || >> + yoffset < -texImage->Border || >> + yoffset + height > texImage->Height - texImage->Border || >> + zoffset < -texImage->Border || >> + zoffset + depth > texImage->Depth - texImage->Border) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glClearTexSubImage(invalid bounds)"); >> + } >> + >> + ctx->Driver.ClearTexSubImage(ctx, texImage, xoffset, yoffset, zoffset, >> + width, height, depth, format, type, >> data); >> +} >> + >> +void GLAPIENTRY >> +_mesa_ClearTexSubImage( GLuint texture, GLint level, >> + GLint xoffset, GLint yoffset, GLint zoffset, >> + GLsizei width, GLsizei height, GLsizei depth, >> + GLenum format, GLenum type, const void *data ) >> +{ >> + GET_CURRENT_CONTEXT(ctx); >> + struct gl_texture_image *texImage = >> + get_tex_image(ctx, texture, level, zoffset); >> + >> + clear_tex_sub_image(ctx, texImage, xoffset, yoffset, zoffset, >> + width, height, depth, format, type, data); >> +} >> + >> +void GLAPIENTRY >> +_mesa_ClearTexImage( GLuint texture, GLint level, >> + GLenum format, GLenum type, const void *data ) >> +{ >> + GET_CURRENT_CONTEXT(ctx); >> + >> + struct gl_texture_image *texImage >> + = get_tex_image(ctx, texture, level, 0); >> + if (!texImage) >> + return; >> + >> + /* XXX figure out which dimensions the texImage has, and pass in 0's >> for >> + * the border. >> + */ >> + clear_tex_sub_image(ctx, texImage, >> + -texImage->Border, -texImage->Border, >> -texImage->Border, >> + texImage->Width, texImage->Height, >> texImage->Depth, >> + format, type, data); > > > I would just pass 0s. Mesa doesn't support textures with borders. Should texImage->Border be removed? I figured it was easy to support borders here, and the driver can deal with any issues. [I also haven't the slightest was texture borders are, beyond the obvious -- borders on textures. Why do they exist?] > > >> +} >> >> >> >> diff --git a/src/mesa/main/teximage.h b/src/mesa/main/teximage.h >> index 5f8a477..4d01ff9 100644 >> --- a/src/mesa/main/teximage.h >> +++ b/src/mesa/main/teximage.h >> @@ -261,6 +261,16 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint level, >> >> >> extern void GLAPIENTRY >> +_mesa_ClearTexSubImage( GLuint texture, GLint level, >> + GLint xoffset, GLint yoffset, GLint zoffset, >> + GLsizei width, GLsizei height, GLsizei depth, >> + GLenum format, GLenum type, const void *data ); >> + >> +extern void GLAPIENTRY >> +_mesa_ClearTexImage( GLuint texture, GLint level, >> + GLenum format, GLenum type, const void *data ); >> + >> +extern void GLAPIENTRY >> _mesa_CompressedTexImage1D(GLenum target, GLint level, >> GLenum internalformat, GLsizei width, >> GLint border, GLsizei imageSize, >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
