[Mesa-dev] [PATCH 02/11] mesa: implement GL_ARB_draw_indirect and GL_ARB_multi_draw_indirect
Chris Forbes
chrisf at ijw.co.nz
Wed Nov 6 17:06:33 PST 2013
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 <stereotype441 at gmail.com> wrote:
> 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.
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list