I'm hoping I don't need the save_ stuff at all, if I only enable this in core contexts. Is that OK?
On Thu, Nov 7, 2013 at 1:44 PM, Paul Berry <stereotype...@gmail.com> wrote: > On 4 November 2013 06:57, Brian Paul <bri...@vmware.com> wrote: >> >> On 11/04/2013 02:09 AM, Chris Forbes wrote: >>> >>> From: Christoph Bumiller <e0425...@student.tuwien.ac.at> >>> >>> >>> diff --git a/src/mapi/glapi/gen/ARB_draw_indirect.xml >>> b/src/mapi/glapi/gen/ARB_draw_indirect.xml >>> new file mode 100644 >>> index 0000000..7de03cd >>> --- /dev/null >>> +++ b/src/mapi/glapi/gen/ARB_draw_indirect.xml >>> @@ -0,0 +1,45 @@ >>> +<?xml version="1.0"?> >>> +<!DOCTYPE OpenGLAPI SYSTEM "gl_API.dtd"> >>> + >>> +<OpenGLAPI> >>> + >>> +<category name="GL_ARB_draw_indirect" number="87"> >>> + >>> + <enum name="DRAW_INDIRECT_BUFFER" value="0x8F3F"/> >>> + <enum name="DRAW_INDIRECT_BUFFER_BINDING" value="0x8F43"/> >>> + >>> + <function name="DrawArraysIndirect" offset="assign" exec="dynamic"> >>> + <param name="mode" type="GLenum"/> >>> + <param name="indirect" type="const GLvoid *"/> >> >> >> The use of GLvoid has been removed in glext.h and I'd be OK with doing the >> same in the dispatch code. > > > I'd be in favor of that too, but I think it should be done as a separate > patch series--at the moment all the dispatch xml consistently uses GLvoid, > so IMHO we should stay consistent for now, and then later switch everything > over to "void" in one fell swoop. > >> >> >> >> >>> + </function> >>> + >>> + <function name="DrawElementsIndirect" offset="assign" >>> exec="dynamic"> >>> + <param name="mode" type="GLenum"/> >>> + <param name="type" type="GLenum"/> >>> + <param name="indirect" type="const GLvoid *"/> >>> + </function> >>> + >>> +</category> >>> + >>> + >>> +<category name="GL_ARB_multi_draw_indirect" number="133"> >>> + >>> + <function name="MultiDrawArraysIndirect" offset="assign" >>> exec="dynamic"> >>> + <param name="mode" type="GLenum"/> >>> + <param name="indirect" type="const GLvoid *"/> >>> + <param name="primcount" type="GLsizei"/> >>> + <param name="stride" type="GLsizei"/> >>> + </function> >>> + >>> + <function name="MultiDrawElementsIndirect" offset="assign" >>> exec="dynamic"> >>> + <param name="mode" type="GLenum"/> >>> + <param name="type" type="GLenum"/> >>> + <param name="indirect" type="const GLvoid *"/> >>> + <param name="primcount" type="GLsizei"/> >>> + <param name="stride" type="GLsizei"/> >>> + </function> >>> + >>> +</category> >>> + >>> + >>> +</OpenGLAPI> >>> diff --git a/src/mapi/glapi/gen/Makefile.am >>> b/src/mapi/glapi/gen/Makefile.am >>> index cbbf659..0c67513 100644 >>> --- a/src/mapi/glapi/gen/Makefile.am >>> +++ b/src/mapi/glapi/gen/Makefile.am >>> @@ -98,6 +98,7 @@ API_XML = \ >>> ARB_draw_buffers.xml \ >>> ARB_draw_buffers_blend.xml \ >>> ARB_draw_elements_base_vertex.xml \ >>> + ARB_draw_indirect.xml \ >>> ARB_draw_instanced.xml \ >>> ARB_ES2_compatibility.xml \ >>> ARB_ES3_compatibility.xml \ >>> diff --git a/src/mapi/glapi/gen/gl_API.xml >>> b/src/mapi/glapi/gen/gl_API.xml >>> index 69014c5..7cab5ba 100644 >>> --- a/src/mapi/glapi/gen/gl_API.xml >>> +++ b/src/mapi/glapi/gen/gl_API.xml >>> @@ -8241,6 +8241,8 @@ >>> >>> <!-- ARB extensions #86...#93 --> >>> >>> +<xi:include href="ARB_draw_indirect.xml" >>> xmlns:xi="http://www.w3.org/2001/XInclude"/> >>> + >>> <category name="GL_ARB_transform_feedback3" number="94"> >>> <enum name="MAX_TRANSFORM_FEEDBACK_BUFFERS" value="0x8E70"/> >>> <enum name="MAX_VERTEX_STREAMS" value="0x8E71"/> >>> @@ -8466,7 +8468,7 @@ >>> >>> <xi:include href="ARB_invalidate_subdata.xml" >>> xmlns:xi="http://www.w3.org/2001/XInclude"/> >>> >>> -<!-- ARB extensions #133...#138 --> >>> +<!-- ARB extensions #134...#138 --> >>> >>> <xi:include href="ARB_texture_buffer_range.xml" >>> xmlns:xi="http://www.w3.org/2001/XInclude"/> >>> >>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c >>> index f285c97..f3128cb 100644 >>> --- a/src/mesa/main/api_validate.c >>> +++ b/src/mesa/main/api_validate.c >>> @@ -837,3 +837,156 @@ _mesa_validate_DrawTransformFeedback(struct >>> gl_context *ctx, >>> >>> return GL_TRUE; >>> } >>> + >> >> >> Maybe put a comment on this describing what 'size' is. >> >> >>> +static GLboolean >>> +valid_draw_indirect(struct gl_context *ctx, >>> + GLenum mode, const GLvoid *indirect, >>> + GLsizei size, const char *name) >>> +{ >>> + const GLsizeiptr end = (GLsizeiptr)indirect + size; >>> + >>> + if (!_mesa_valid_prim_mode(ctx, mode, name)) >>> + return GL_FALSE; >>> + >>> + if ((GLsizeiptr)indirect & (sizeof(GLuint) - 1)) { >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "%s(indirect is not aligned)", name); >>> + return GL_FALSE; >>> + } >>> + >>> + if (_mesa_is_bufferobj(ctx->DrawIndirectBuffer)) { >>> + if (_mesa_bufferobj_mapped(ctx->DrawIndirectBuffer)) { >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "%s(DRAW_INDIRECT_BUFFER is mapped)", name); >>> + return GL_FALSE; >>> + } >>> + if (ctx->DrawIndirectBuffer->Size < end) { >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "%s(DRAW_INDIRECT_BUFFER too small)", name); >>> + return GL_FALSE; >>> + } >>> + } else { >>> + if (ctx->API != API_OPENGL_COMPAT) { >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "%s: no buffer bound to DRAW_INDIRECT_BUFFER", >>> name); >>> + return GL_FALSE; >>> + } >>> + } >>> + >>> + if (!check_valid_to_render(ctx, name)) >>> + return GL_FALSE; >>> + >>> + return GL_TRUE; >>> +} >>> + >>> +static inline GLboolean >>> +valid_draw_indirect_elements(struct gl_context *ctx, >>> + GLenum mode, GLenum type, const GLvoid >>> *indirect, >>> + GLsizeiptr size, const char *name) >>> +{ >>> + if (!valid_elements_type(ctx, type, name)) >>> + return GL_FALSE; >>> + >>> + /* >>> + * Unlike regular DrawElementsInstancedBaseVertex commands, the >>> indices >>> + * may not come from a client array and must come from an index >>> buffer. >>> + * If no element array buffer is bound, an INVALID_OPERATION error is >>> + * generated. >>> + */ >>> + if (!_mesa_is_bufferobj(ctx->Array.ArrayObj->ElementArrayBufferObj)) >>> { >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "%s(no buffer bound to GL_ELEMENT_ARRAY_BUFFER)", >>> name); >>> + return GL_FALSE; >>> + } >>> + >>> + return valid_draw_indirect(ctx, mode, indirect, size, name); >>> +} >>> + >>> +static inline GLboolean >>> +valid_draw_indirect_multi(struct gl_context *ctx, >>> + GLsizei primcount, GLsizei stride, >>> + const char *name) >>> +{ >>> + if (primcount < 0) { >>> + _mesa_error(ctx, GL_INVALID_VALUE, "%s(primcount < 0)", name); >>> + return GL_FALSE; >>> + } >>> + >>> + if (stride % 4) { >>> + _mesa_error(ctx, GL_INVALID_VALUE, "%s(stride %% 4)", name); >>> + return GL_FALSE; >>> + } >>> + >>> + return GL_TRUE; >>> +} >>> + >>> +GLboolean >>> +_mesa_validate_DrawArraysIndirect(struct gl_context *ctx, >>> + GLenum mode, >>> + const GLvoid *indirect) >>> +{ >>> + FLUSH_CURRENT(ctx, 0); >>> + >>> + return valid_draw_indirect(ctx, mode, >>> + indirect, 4 * sizeof(GLuint), >> >> >> 4 here and 5 below, etc. seem like magic numbers. How about declaring and >> using a local var such as "const unsigned drawArraysNumParams = 4;" to >> clarify? >> >> >> >>> + "glDrawArraysIndirect"); >>> +} >>> + >>> +GLboolean >>> +_mesa_validate_DrawElementsIndirect(struct gl_context *ctx, >>> + GLenum mode, GLenum type, >>> + const GLvoid *indirect) >>> +{ >>> + FLUSH_CURRENT(ctx, 0); >>> + >>> + return valid_draw_indirect_elements(ctx, mode, type, >>> + indirect, 5 * sizeof(GLuint), >>> + "glDrawElementsIndirect"); >>> +} >>> + >>> +GLboolean >>> +_mesa_validate_MultiDrawArraysIndirect(struct gl_context *ctx, >>> + GLenum mode, >>> + const GLvoid *indirect, >>> + GLsizei primcount, GLsizei >>> stride) >>> +{ >>> + GLsizeiptr size = 0; >>> + if (primcount) /* &(last command) + sizeof(DrawArraysIndirectCommand) >>> */ >> >> >> Is the commented code a debug left-over? >> >> >> >>> + size = (primcount - 1) * stride + 4 * sizeof(GLuint); >>> + >>> + FLUSH_CURRENT(ctx, 0); >>> + >>> + if (!valid_draw_indirect_multi(ctx, primcount, stride, >>> + "glMultiDrawArraysIndirect")) >>> + return GL_FALSE; >>> + >>> + if (!valid_draw_indirect(ctx, mode, indirect, size, >>> + "glMultiDrawArraysIndirect")) >>> + return GL_FALSE; >>> + >>> + return GL_TRUE; >>> +} >>> + >>> +GLboolean >>> +_mesa_validate_MultiDrawElementsIndirect(struct gl_context *ctx, >>> + GLenum mode, GLenum type, >>> + const GLvoid *indirect, >>> + GLsizei primcount, GLsizei >>> stride) >>> +{ >>> + GLsizeiptr size = 0; >> >> >> Insert blank line here. >> >> >> >>> + if (primcount) /* &(last command) + >>> sizeof(DrawElementsIndirectCommand) */ >>> + size = (primcount - 1) * stride + 5 * sizeof(GLuint); >>> + >>> + FLUSH_CURRENT(ctx, 0); >>> + >>> + if (!valid_draw_indirect_multi(ctx, primcount, stride, >>> + "glMultiDrawElementsIndirect")) >>> + return GL_FALSE; >>> + >>> + if (!valid_draw_indirect_elements(ctx, mode, type, >>> + indirect, size, >>> + "glMultiDrawElementsIndirect")) >>> + return GL_FALSE; >>> + >>> + return GL_TRUE; >>> +} >>> diff --git a/src/mesa/main/api_validate.h b/src/mesa/main/api_validate.h >>> index f2b753c..8238df1 100644 >>> --- a/src/mesa/main/api_validate.h >>> +++ b/src/mesa/main/api_validate.h >>> @@ -87,5 +87,31 @@ _mesa_validate_DrawTransformFeedback(struct gl_context >>> *ctx, >>> GLuint stream, >>> GLsizei numInstances); >>> >>> +extern GLboolean >>> +_mesa_validate_DrawArraysIndirect(struct gl_context *ctx, >>> + GLenum mode, >>> + const GLvoid *indirect); >>> + >>> +extern GLboolean >>> +_mesa_validate_DrawElementsIndirect(struct gl_context *ctx, >>> + GLenum mode, >>> + GLenum type, >>> + const GLvoid *indirect); >>> + >>> +extern GLboolean >>> +_mesa_validate_MultiDrawArraysIndirect(struct gl_context *ctx, >>> + GLenum mode, >>> + const GLvoid *indirect, >>> + GLsizei primcount, >>> + GLsizei stride); >>> + >>> +extern GLboolean >>> +_mesa_validate_MultiDrawElementsIndirect(struct gl_context *ctx, >>> + GLenum mode, >>> + GLenum type, >>> + const GLvoid *indirect, >>> + GLsizei primcount, >>> + GLsizei stride); >>> + >>> >>> #endif >>> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c >>> index 1f55061..69ea1c0 100644 >>> --- a/src/mesa/main/bufferobj.c >>> +++ b/src/mesa/main/bufferobj.c >>> @@ -86,6 +86,10 @@ get_buffer_target(struct gl_context *ctx, GLenum >>> target) >>> return &ctx->CopyReadBuffer; >>> case GL_COPY_WRITE_BUFFER: >>> return &ctx->CopyWriteBuffer; >>> + case GL_DRAW_INDIRECT_BUFFER: >>> + if (ctx->Extensions.ARB_draw_indirect) >>> + return &ctx->DrawIndirectBuffer; >>> + break; >>> case GL_TRANSFORM_FEEDBACK_BUFFER: >>> if (ctx->Extensions.EXT_transform_feedback) { >>> return &ctx->TransformFeedback.CurrentBuffer; >>> @@ -626,6 +630,9 @@ _mesa_init_buffer_objects( struct gl_context *ctx ) >>> _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, >>> ctx->Shared->NullBufferObj); >>> >>> + _mesa_reference_buffer_object(ctx, &ctx->DrawIndirectBuffer, >>> + ctx->Shared->NullBufferObj); >>> + >>> for (i = 0; i < MAX_COMBINED_UNIFORM_BUFFERS; i++) { >>> _mesa_reference_buffer_object(ctx, >>> >>> &ctx->UniformBufferBindings[i].BufferObject, >>> @@ -873,6 +880,11 @@ _mesa_DeleteBuffers(GLsizei n, const GLuint *ids) >>> _mesa_BindBuffer( GL_ELEMENT_ARRAY_BUFFER_ARB, 0 ); >>> } >>> >>> + /* unbind ARB_draw_indirect binding point */ >>> + if (ctx->DrawIndirectBuffer == bufObj) { >>> + _mesa_BindBuffer( GL_DRAW_INDIRECT_BUFFER, 0 ); >>> + } >>> + >>> /* unbind ARB_copy_buffer binding points */ >>> if (ctx->CopyReadBuffer == bufObj) { >>> _mesa_BindBuffer( GL_COPY_READ_BUFFER, 0 ); >>> diff --git a/src/mesa/main/dlist.c b/src/mesa/main/dlist.c >>> index 5956419..c953ffb 100644 >>> --- a/src/mesa/main/dlist.c >>> +++ b/src/mesa/main/dlist.c >>> @@ -1356,6 +1356,42 @@ >>> save_DrawElementsInstancedBaseVertexBaseInstance(GLenum mode, >>> "glDrawElementsInstancedBaseVertexBaseInstance() during >>> display list compile"); >>> } >>> >>> +/* GL_ARB_draw_indirect. */ >>> +static void GLAPIENTRY >>> +save_DrawArraysIndirect(GLenum mode, const GLvoid *indirect) >>> +{ >>> + GET_CURRENT_CONTEXT(ctx); >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "glDrawArraysIndirect() during display list compile"); >>> +} >>> + >>> +static void GLAPIENTRY >>> +save_DrawElementsIndirect(GLenum mode, GLenum type, const GLvoid >>> *indirect) >>> +{ >>> + GET_CURRENT_CONTEXT(ctx); >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "glDrawElementsIndirect() during display list compile"); >>> +} >>> + >>> +/* GL_ARB_multi_draw_indirect. */ >>> +static void GLAPIENTRY >>> +save_MultiDrawArraysIndirect(GLenum mode, const GLvoid *indirect, >>> + GLsizei primcount, GLsizei stride) >>> +{ >>> + GET_CURRENT_CONTEXT(ctx); >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "glMultiDrawArraysIndirect() during display list >>> compile"); >>> +} >>> + >>> +static void GLAPIENTRY >>> +save_MultiDrawElementsIndirect(GLenum mode, GLenum type, >>> + const GLvoid *indirect, >>> + GLsizei primcount, GLsizei stride) >>> +{ >>> + GET_CURRENT_CONTEXT(ctx); >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "glMultiDrawElementsIndirect() during display list >>> compile"); >>> +} >> >> >> I thought that when we build/init the display list 'save' dispatch table, >> the unused entries point to a generic function that generates >> GL_INVALID_OPERATION. In that case, I don't think we'd need these >> functions. I'd have to dig into the code to be sure though. > > > I don't think that's the case--_mesa_initialize_save_table() starts off by > making a copy of the Exec table, so stuff that is allowed in Exec mode but > disallowed in display lists needs to be overridden. I think these "save_" > functions are fine. > > I agree with Brian's other comments, though (comment explaining "size", > explain the magic numbers 4 and 5, and mysterious commented out code in > _mesa_validate_MultiDrawArraysIndirect and > _mesa_validate_MultiDrawElementsIndirect). > > I also agree with Ian's comment that the if test in get_buffer_target() > needs to include "&& ctx->API == API_OPENGL_CORE. > > More comments on this patch to follow in a separate email. > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
