<div dir="ltr">That would make the diff easier to read, but it doesn't make sense to lay out the functions in that order in the file. GetBufferSubData is a download function and shouldn't be part of the upload function "group" in the file. </div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 20, 2015 at 2:28 AM, Martin Peres <<a href="mailto:martin.peres@free.fr" target="_blank">martin.peres@free.fr</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 12/02/2015 04:05, Laura Ekstrand wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> v2: review by Ian Romanick - Remove "_mesa" from name of static software fallback buffer_sub_data. - Remove mappedRange from _mesa_buffer_sub_data. - Removed some cosmetic changes to a separate commit. --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 7 ++ src/mesa/main/bufferobj.c | 129 +++++++++++++++---------- src/mesa/main/bufferobj.h | 9 ++ src/mesa/main/tests/dispatch_sanity.cpp | 1 + 4 files changed, 97 insertions(+), 49 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 7779262..6d70b8e 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -28,6 +28,13 @@ <param name="usage" type="GLenum" /> </function> + <function name="NamedBufferSubData" offset="assign"> + <param name="buffer" type="GLuint" /> + <param name="offset" type="GLintptr" /> + <param name="size" type="GLsizeiptr" /> + <param name="data" type="const GLvoid *" /> + </function> +  <function name="CreateTextures" offset="assign"> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c index ac8eed1..4f89748 100644 --- a/src/mesa/main/bufferobj.c +++ b/src/mesa/main/bufferobj.c @@ -227,67 +227,58 @@ bufferobj_range_mapped(const struct gl_buffer_object *obj, * \c glClearBufferSubData. * * \param ctx GL context. - * \param target Buffer object target on which to operate. + * \param bufObj The buffer object. * \param offset Offset of the first byte of the subdata range. * \param size Size, in bytes, of the subdata range. * \param mappedRange If true, checks if an overlapping range is mapped. * If false, checks if buffer is mapped. - * \param errorNoBuffer Error code if no buffer is bound to target. * \param caller Name of calling function for recording errors. - * \return A pointer to the buffer object bound to \c target in the - * specified context or \c NULL if any of the parameter or state - * conditions are invalid. + * \return false if error, true otherwise * * \sa glBufferSubDataARB, glGetBufferSubDataARB, glClearBufferSubData */ -static struct gl_buffer_object * -buffer_object_subdata_range_good(struct gl_context * ctx, GLenum target, - GLintptrARB offset, GLsizeiptrARB size, - bool mappedRange, GLenum errorNoBuffer, - const char *caller) +static bool +buffer_object_subdata_range_good(struct gl_context *ctx, + struct gl_buffer_object *bufObj, + GLintptr offset, GLsizeiptr size, + bool mappedRange, const char *caller) { - struct gl_buffer_object *bufObj; - if (size < 0) { _mesa_error(ctx, GL_INVALID_VALUE, "%s(size < 0)", caller); - return NULL; + return false; } if (offset < 0) { _mesa_error(ctx, GL_INVALID_VALUE, "%s(offset < 0)", caller); - return NULL; + return false; } - bufObj = get_buffer(ctx, caller, target, errorNoBuffer); - if (!bufObj) - return NULL; - if (offset + size > bufObj->Size) { _mesa_error(ctx, GL_INVALID_VALUE, "%s(offset %lu + size %lu > buffer size %lu)", caller, (unsigned long) offset, (unsigned long) size, (unsigned long) bufObj->Size); - return NULL; + return false; } if (bufObj->Mappings[MAP_USER].AccessFlags & GL_MAP_PERSISTENT_BIT) - return bufObj; + return true; if (mappedRange) { if (bufferobj_range_mapped(bufObj, offset, size)) { _mesa_error(ctx, GL_INVALID_OPERATION, "%s", caller); - return NULL; + return false; } } else { if (_mesa_bufferobj_mapped(bufObj, MAP_USER)) { _mesa_error(ctx, GL_INVALID_OPERATION, "%s", caller); - return NULL; + return false; } } - return bufObj; + return true; } @@ -602,9 +593,9 @@ buffer_data_fallback(struct gl_context *ctx, GLenum target, GLsizeiptr size, * \sa glBufferSubDataARB, dd_function_table::BufferSubData. */ static void -_mesa_buffer_subdata( struct gl_context *ctx, GLintptrARB offset, - GLsizeiptrARB size, const GLvoid * data, - struct gl_buffer_object * bufObj ) +buffer_sub_data_fallback(struct gl_context *ctx, GLintptr offset, + GLsizeiptr size, const GLvoid *data, + struct gl_buffer_object *bufObj) { (void) ctx; @@ -1113,7 +1104,7 @@ _mesa_init_buffer_object_functions(struct dd_function_table *driver) driver->NewBufferObject = _mesa_new_buffer_object; driver->DeleteBuffer = _mesa_delete_buffer_object; driver->BufferData = buffer_data_fallback; - driver->BufferSubData = _mesa_buffer_subdata; + driver->BufferSubData = buffer_sub_data_fallback; driver->GetBufferSubData = _mesa_buffer_get_subdata; driver->UnmapBuffer = _mesa_buffer_unmap; @@ -1588,24 +1579,31 @@ _mesa_NamedBufferData(GLuint buffer, GLsizeiptr size, const GLvoid *data, } -void GLAPIENTRY -_mesa_BufferSubData(GLenum target, GLintptrARB offset, - GLsizeiptrARB size, const GLvoid * data) +/** + * Implementation for glBufferSubData and glNamedBufferSubData. + * + * \param ctx GL context. + * \param bufObj The buffer object. + * \param offset Offset of the first byte of the subdata range. + * \param size Size, in bytes, of the subdata range. + * \param data The data store. + * \param func Name of calling function for recording errors. + * + */ +void +_mesa_buffer_sub_data(struct gl_context *ctx, struct gl_buffer_object *bufObj, + GLintptr offset, GLsizeiptr size, const GLvoid *data, + const char *func) { - GET_CURRENT_CONTEXT(ctx); - struct gl_buffer_object *bufObj; - - bufObj = buffer_object_subdata_range_good( ctx, target, offset, size, - false, GL_INVALID_OPERATION, - "glBufferSubDataARB" ); - if (!bufObj) { + if (!buffer_object_subdata_range_good(ctx, bufObj, offset, size, + false, func)) { /* error already recorded */ return; } if (bufObj->Immutable && !(bufObj->StorageFlags & GL_DYNAMIC_STORAGE_BIT)) { - _mesa_error(ctx, GL_INVALID_OPERATION, "glBufferSubData"); + _mesa_error(ctx, GL_INVALID_OPERATION, func); return; } @@ -1618,19 +1616,50 @@ _mesa_BufferSubData(GLenum target, GLintptrARB offset, ctx->Driver.BufferSubData( ctx, offset, size, data, bufObj ); } +void GLAPIENTRY +_mesa_BufferSubData(GLenum target, GLintptr offset, + GLsizeiptr size, const GLvoid *data) +{ + GET_CURRENT_CONTEXT(ctx); + struct gl_buffer_object *bufObj; + + bufObj = get_buffer(ctx, "glBufferSubData", target, GL_INVALID_OPERATION); + if (!bufObj) + return; + + _mesa_buffer_sub_data(ctx, bufObj, offset, size, data, "glBufferSubData"); +} void GLAPIENTRY -_mesa_GetBufferSubData(GLenum target, GLintptrARB offset, - GLsizeiptrARB size, void * data) +_mesa_NamedBufferSubData(GLuint buffer, GLintptr offset, + GLsizeiptr size, const GLvoid *data) </blockquote> </div></div> In this case, I would have added _mesa_NamedBufferSubData under _mesa_GetBufferSubData to make the diff more readable. It is good practice to make the diff as short as possible and to prove that you changed as little as possible in the existing functions :) Other than that, looks good to me! Reviewed-by: Martin Peres <<a href="mailto:martin.peres@linux.intel.com" target="_blank">martin.peres@linux.intel.com</a>><div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> { GET_CURRENT_CONTEXT(ctx); struct gl_buffer_object *bufObj; - bufObj = buffer_object_subdata_range_good(ctx, target, offset, size, - false, GL_INVALID_OPERATION, - "glGetBufferSubDataARB"); - if (!bufObj) { - /* error already recorded */ + bufObj = _mesa_lookup_bufferobj_err(ctx, buffer, "glNamedBufferSubData"); + if (!bufObj) + return; + + _mesa_buffer_sub_data(ctx, bufObj, offset, size, data, + "glNamedBufferSubData"); +} + + +void GLAPIENTRY +_mesa_GetBufferSubData(GLenum target, GLintptr offset, + GLsizeiptr size, GLvoid *data) +{ + GET_CURRENT_CONTEXT(ctx); + struct gl_buffer_object *bufObj; + + bufObj = get_buffer(ctx, "glGetBufferSubData", target, + GL_INVALID_OPERATION); + if (!bufObj) + return; + + if (!buffer_object_subdata_range_good(ctx, bufObj, offset, size, false, + "glGetBufferSubData")) { return; } @@ -1704,10 +1733,12 @@ _mesa_ClearBufferSubData(GLenum target, GLenum internalformat, GLubyte clearValue[MAX_PIXEL_BYTES]; GLsizeiptr clearValueSize; - bufObj = buffer_object_subdata_range_good(ctx, target, offset, size, - true, GL_INVALID_VALUE, - "glClearBufferSubData"); - if (!bufObj) { + bufObj = get_buffer(ctx, "glClearBufferSubData", target, GL_INVALID_VALUE); + if (!bufObj) + return; + + if (!buffer_object_subdata_range_good(ctx, bufObj, offset, size, + true, "glClearBufferSubData")) { return; } diff --git a/src/mesa/main/bufferobj.h b/src/mesa/main/bufferobj.h index ddd240c..889bbb1 100644 --- a/src/mesa/main/bufferobj.h +++ b/src/mesa/main/bufferobj.h @@ -140,6 +140,11 @@ _mesa_buffer_data(struct gl_context *ctx, struct gl_buffer_object *bufObj, GLenum usage, const char *func); extern void +_mesa_buffer_sub_data(struct gl_context *ctx, struct gl_buffer_object *bufObj, + GLintptr offset, GLsizeiptr size, const GLvoid *data, + const char *func); + +extern void _mesa_buffer_unmap_all_mappings(struct gl_context *ctx, struct gl_buffer_object *bufObj); @@ -189,6 +194,10 @@ _mesa_BufferSubData(GLenum target, GLintptrARB offset, GLsizeiptrARB size, const GLvoid * data); void GLAPIENTRY +_mesa_NamedBufferSubData(GLuint buffer, GLintptr offset, + GLsizeiptr size, const GLvoid *data); + +void GLAPIENTRY _mesa_GetBufferSubData(GLenum target, GLintptrARB offset, GLsizeiptrARB size, void * data); diff --git a/src/mesa/main/tests/dispatch_sanity.cpp b/src/mesa/main/tests/dispatch_sanity.cpp index 595ee90..898a2b9 100644 --- a/src/mesa/main/tests/dispatch_sanity.cpp +++ b/src/mesa/main/tests/dispatch_sanity.cpp @@ -958,6 +958,7 @@ const struct function gl_core_functions_possible[] = { { "glCreateBuffers", 45, -1 }, { "glNamedBufferStorage", 45, -1 }, { "glNamedBufferData", 45, -1 }, + { "glNamedBufferSubData", 45, -1 }, { "glCreateTextures", 45, -1 }, { "glTextureStorage1D", 45, -1 }, { "glTextureStorage2D", 45, -1 }, </blockquote> </div></div></blockquote></div> </div>