[Mesa-dev] [PATCH 10/10] mesa: add KHR_no_error support for glBindBufferRange()
Iago Toral
itoral at igalia.com
Tue May 23 12:19:13 UTC 2017
On Tue, 2017-05-23 at 22:12 +1000, Timothy Arceri wrote:
> 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.
Ah, I was missing this. In that case I agree this should be fine either
way,, feel free to add my Rb to it.
Iago
>
> > 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