[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