[Mesa-dev] [PATCH 10/10] mesa: add KHR_no_error support for glBindBufferRange()
Iago Toral
itoral at igalia.com
Tue May 23 10:01:40 UTC 2017
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. 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).
> >
> > > 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