[Mesa-dev] [PATCH 10/10] mesa: add KHR_no_error support for glBindBufferRange()
Timothy Arceri
tarceri at itsqueeze.com
Tue May 23 09:30:34 UTC 2017
On 23/05/17 18:48, Iago Toral wrote:
> On Mon, 2017-05-22 at 15:47 +1000, Timothy Arceri wrote:
>> ---
>> src/mapi/glapi/gen/GL3x.xml | 2 +-
>> src/mesa/main/bufferobj.c | 103 ++++++++++++++++++++++++++++----
>> ------------
>> src/mesa/main/bufferobj.h | 3 ++
>> 3 files changed, 70 insertions(+), 38 deletions(-)
>>
>> diff --git a/src/mapi/glapi/gen/GL3x.xml
>> b/src/mapi/glapi/gen/GL3x.xml
>> index 10c157e..f488ba3 100644
>> --- a/src/mapi/glapi/gen/GL3x.xml
>> +++ b/src/mapi/glapi/gen/GL3x.xml
>> @@ -206,21 +206,21 @@
>> <param name="name" type="const GLchar *"/>
>> </function>
>>
>> <function name="BeginTransformFeedback" es2="3.0">
>> <param name="mode" type="GLenum"/>
>> </function>
>>
>> <function name="EndTransformFeedback" es2="3.0">
>> </function>
>>
>> - <function name="BindBufferRange" es2="3.0">
>> + <function name="BindBufferRange" es2="3.0" no_error="true">
>> <param name="target" type="GLenum"/>
>> <param name="index" type="GLuint"/>
>> <param name="buffer" type="GLuint"/>
>> <param name="offset" type="GLintptr"/>
>> <param name="size" type="GLsizeiptr"/>
>> </function>
>>
>> <function name="BindBufferBase" es2="3.0">
>> <param name="target" type="GLenum"/>
>> <param name="index" type="GLuint"/>
>> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
>> index 5aae579..9e656a4 100644
>> --- a/src/mesa/main/bufferobj.c
>> +++ b/src/mesa/main/bufferobj.c
>> @@ -3986,87 +3986,116 @@ bind_atomic_buffers(struct gl_context *ctx,
>>
>> if (bufObj)
>> set_atomic_buffer_binding(ctx, binding, bufObj, offset,
>> size);
>> }
>>
>> _mesa_HashUnlockMutex(ctx->Shared->BufferObjects);
>> }
>>
>> static ALWAYS_INLINE void
>> bind_buffer_range(GLenum target, GLuint index, GLuint buffer,
>> GLintptr offset,
>> - GLsizeiptr size)
>> + GLsizeiptr size, bool no_error)
>> {
>> GET_CURRENT_CONTEXT(ctx);
>> struct gl_buffer_object *bufObj;
>>
>> if (MESA_VERBOSE & VERBOSE_API) {
>> _mesa_debug(ctx, "glBindBufferRange(%s, %u, %u, %lu, %lu)\n",
>> _mesa_enum_to_string(target), index, buffer,
>> (unsigned long) offset, (unsigned long) size);
>> }
>>
>> if (buffer == 0) {
>> bufObj = ctx->Shared->NullBufferObj;
>> } else {
>> bufObj = _mesa_lookup_bufferobj(ctx, buffer);
>> }
>> if (!_mesa_handle_bind_buffer_gen(ctx, buffer,
>> &bufObj, "glBindBufferRange"))
>
> It seems that _mesa_handle_bind_buffer_gen() can still generate
> INVALID_OPERATION but I guess we seem to avoid that scenario herewith
> the if (buffer == 0) above...
>
> Anyway, would it make sense to add a no_error parameter to that
> function too? Otherwise it might be possible to mess things up in the
> future if people move or refactor code around I guess.
Hi, firstly thanks for the review. The problem is that here all the
no_error checks are optimised away because we set a compiler flag to
force inlining. Adding a no_error check to
_mesa_handle_bind_buffer_gen() will add an extra if to avoid an if so
its not worth doing.
The idea is to lower overhead by removing error checks, in most cases
I've been able to do so fairly cleanly but this was one check that was a
bit tricker so I left as is. It's possible we could do some inlining but
we need to be careful with that as it can bloat things for not much
benefit. I'd rather leave this one as is, does that seem reasonable?
>
>
>> return;
>>
>> - if (!bufObj) {
>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glBindBufferRange(invalid buffer=%u)", buffer);
>> - return;
>> - }
>> -
>> - if (buffer != 0) {
>> - if (size <= 0) {
>> - _mesa_error(ctx, GL_INVALID_VALUE,
>> "glBindBufferRange(size=%d)",
>> - (int) size);
>> + if (no_error) {
>> + switch (target) {
>> + case GL_TRANSFORM_FEEDBACK_BUFFER:
>> + _mesa_bind_buffer_range_xfb(ctx, ctx-
>>> TransformFeedback.CurrentObject,
>> + index, bufObj, offset, size);
>> return;
>> + case GL_UNIFORM_BUFFER:
>> + bind_buffer_range_uniform_buffer(ctx, index, bufObj,
>> offset, size);
>> + return;
>> + case GL_SHADER_STORAGE_BUFFER:
>> + bind_buffer_range_shader_storage_buffer(ctx, index, bufObj,
>> offset,
>> + size);
>> + return;
>> + case GL_ATOMIC_COUNTER_BUFFER:
>> + bind_atomic_buffer(ctx, index, bufObj, offset, size);
>> + return;
>> + default:
>> + unreachable("invalid BindBufferRange target with
>> KHR_no_error");
>> }
>> - }
>> -
>> - switch (target) {
>> - case GL_TRANSFORM_FEEDBACK_BUFFER:
>> - if (!_mesa_validate_buffer_range_xfb(ctx,
>> - ctx-
>>> TransformFeedback.CurrentObject,
>> - index, bufObj, offset,
>> size,
>> - false))
>> + } else {
>> + if (!bufObj) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glBindBufferRange(invalid buffer=%u)",
>> buffer);
>> return;
>> + }
>>
>> - _mesa_bind_buffer_range_xfb(ctx, ctx-
>>> TransformFeedback.CurrentObject,
>> - index, bufObj, offset, size);
>> - return;
>> - case GL_UNIFORM_BUFFER:
>> - bind_buffer_range_uniform_buffer_err(ctx, index, bufObj,
>> offset, size);
>> - return;
>> - case GL_SHADER_STORAGE_BUFFER:
>> - bind_buffer_range_shader_storage_buffer_err(ctx, index,
>> bufObj, offset,
>> - size);
>> - return;
>> - case GL_ATOMIC_COUNTER_BUFFER:
>> - bind_atomic_buffer_err(ctx, index, bufObj, offset, size,
>> - "glBindBufferRange");
>> - return;
>> - default:
>> - _mesa_error(ctx, GL_INVALID_ENUM,
>> "glBindBufferRange(target)");
>> - return;
>> + if (buffer != 0) {
>> + if (size <= 0) {
>> + _mesa_error(ctx, GL_INVALID_VALUE,
>> "glBindBufferRange(size=%d)",
>> + (int) size);
>> + return;
>> + }
>> + }
>> +
>> + switch (target) {
>> + case GL_TRANSFORM_FEEDBACK_BUFFER:
>> + if (!_mesa_validate_buffer_range_xfb(ctx,
>> + ctx-
>>> TransformFeedback.CurrentObject,
>> + index, bufObj, offset,
>> size,
>> + false))
>> + return;
>> +
>> + _mesa_bind_buffer_range_xfb(ctx, ctx-
>>> TransformFeedback.CurrentObject,
>> + index, bufObj, offset, size);
>> + return;
>> + case GL_UNIFORM_BUFFER:
>> + bind_buffer_range_uniform_buffer_err(ctx, index, bufObj,
>> offset,
>> + size);
>> + return;
>> + case GL_SHADER_STORAGE_BUFFER:
>> + bind_buffer_range_shader_storage_buffer_err(ctx, index,
>> bufObj,
>> + offset, size);
>> + return;
>> + case GL_ATOMIC_COUNTER_BUFFER:
>> + bind_atomic_buffer_err(ctx, index, bufObj, offset, size,
>> + "glBindBufferRange");
>> + return;
>> + default:
>> + _mesa_error(ctx, GL_INVALID_ENUM,
>> "glBindBufferRange(target)");
>> + return;
>> + }
>> }
>> }
>>
>> void GLAPIENTRY
>> +_mesa_BindBufferRange_no_error(GLenum target, GLuint index, GLuint
>> buffer,
>> + GLintptr offset, GLsizeiptr size)
>> +{
>> + bind_buffer_range(target, index, buffer, offset, size, true);
>> +}
>> +
>> +void GLAPIENTRY
>> _mesa_BindBufferRange(GLenum target, GLuint index,
>> GLuint buffer, GLintptr offset, GLsizeiptr
>> size)
>> {
>> - bind_buffer_range(target, index, buffer, offset, size);
>> + bind_buffer_range(target, index, buffer, offset, size, false);
>> }
>>
>> void GLAPIENTRY
>> _mesa_BindBufferBase(GLenum target, GLuint index, GLuint buffer)
>> {
>> GET_CURRENT_CONTEXT(ctx);
>> struct gl_buffer_object *bufObj;
>>
>> if (MESA_VERBOSE & VERBOSE_API) {
>> _mesa_debug(ctx, "glBindBufferBase(%s, %u, %u)\n",
>> diff --git a/src/mesa/main/bufferobj.h b/src/mesa/main/bufferobj.h
>> index f652ec3..ff44fed 100644
>> --- a/src/mesa/main/bufferobj.h
>> +++ b/src/mesa/main/bufferobj.h
>> @@ -315,20 +315,23 @@ _mesa_FlushMappedBufferRange(GLenum target,
>> GLintptr offset, GLsizeiptr length);
>>
>> void GLAPIENTRY
>> _mesa_FlushMappedNamedBufferRange_no_error(GLuint buffer, GLintptr
>> offset,
>> GLsizeiptr length);
>> void GLAPIENTRY
>> _mesa_FlushMappedNamedBufferRange(GLuint buffer, GLintptr offset,
>> GLsizeiptr length);
>>
>> void GLAPIENTRY
>> +_mesa_BindBufferRange_no_error(GLenum target, GLuint index, GLuint
>> buffer,
>> + GLintptr offset, GLsizeiptr size);
>> +void GLAPIENTRY
>> _mesa_BindBufferRange(GLenum target, GLuint index,
>> GLuint buffer, GLintptr offset, GLsizeiptr
>> size);
>>
>> void GLAPIENTRY
>> _mesa_BindBufferBase(GLenum target, GLuint index, GLuint buffer);
>>
>> void GLAPIENTRY
>> _mesa_BindBuffersRange(GLenum target, GLuint first, GLsizei count,
>> const GLuint *buffers,
>> const GLintptr *offsets, const GLsizeiptr
>> *sizes);
More information about the mesa-dev
mailing list