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