[Mesa-dev] [PATCH 02/11] mesa: implement GL_ARB_draw_indirect and GL_ARB_multi_draw_indirect
Paul Berry
stereotype441 at gmail.com
Wed Nov 6 16:44:59 PST 2013
On 4 November 2013 06:57, Brian Paul <brianp at vmware.com> wrote:
> On 11/04/2013 02:09 AM, Chris Forbes wrote:
>
>> From: Christoph Bumiller <e0425955 at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131106/d787c14c/attachment-0001.html>
More information about the mesa-dev
mailing list