[Mesa-dev] [PATCH 10/10] mesa: add KHR_no_error support for glBindBufferRange()
Timothy Arceri
tarceri at itsqueeze.com
Tue May 23 12:12:44 UTC 2017
On 23/05/17 20:01, Iago Toral wrote:
> On Tue, 2017-05-23 at 19:30 +1000, Timothy Arceri wrote:
>> 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.
>
> But that is necessary to implement correct behavior with
> KHR_no_error... >
>> 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?
>
> I understand, however, I think correct behavior should always be the
> priority.
The extension already behaves correctly regardless of any validation
errors because we override the error returned by glGetError() to always
be GL_NO_ERROR (except for out of memory errors). Without this, this
extension would likely never be able to get enabled.
> Also, I feel like there isn't a strong reason to not do it
> here, the current code already has a conditional and it is going to
> execute even with KHR_no_error, right >
> if (!buf && (ctx->API == API_OPENGL_CORE)) {
> ...
> }
>
> If anything, adding a "!no_error &&" to that conditional would improve
> things for KHR_no_error, since that will evaluate to FALSE with
> KHR_no_error and the compiler should be able to drop out of the
> conditional early without evaluating the other 2 elements whereas as
> now it will always evaluate the two (since we know that buf != NULL, at
> least for this path).
Right but it makes the regular path worse and just adds more code. I'd
rather leave it as is for now, maybe someone can come up with a better
way to refactor this in future.
>
>>>
>>>> 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