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 voidbind_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);
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
