<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Jan 21, 2015 at 6:42 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On 01/21/2015 05:40 PM, Laura Ekstrand wrote: > --- > src/mapi/glapi/gen/ARB_direct_state_access.xml | 7 ++ > src/mesa/main/bufferobj.c | 150 ++++++++++++++++--------- > src/mesa/main/bufferobj.h | 13 ++- > src/mesa/main/tests/dispatch_sanity.cpp | 1 + > 4 files changed, 113 insertions(+), 58 deletions(-) > > diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml > index 4b1ef6f..8a5cebb 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 c77029f..20c2cdc 100644 > --- a/src/mesa/main/bufferobj.c > +++ b/src/mesa/main/bufferobj.c > @@ -227,67 +227,62 @@ 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. > + * \param func Name of calling function for recording errors. > + * \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 *func) </div></div>Many places in Mesa use 'caller'. This feels like a spurious change in this patch. If we want to unify on a single name, there should be a patch that changes all the occurrences of 'func', 'caller', and 'function' to that single name. <div><div class="h5"> > { > - struct gl_buffer_object *bufObj; > - > if (size < 0) { > - _mesa_error(ctx, GL_INVALID_VALUE, "%s(size < 0)", caller); > - return NULL; > + _mesa_error(ctx, GL_INVALID_VALUE, "%s(size < 0)", func); > + return false; > } > > if (offset < 0) { > - _mesa_error(ctx, GL_INVALID_VALUE, "%s(offset < 0)", caller); > - return NULL; > + _mesa_error(ctx, GL_INVALID_VALUE, "%s(offset < 0)", func); > + 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, > + "%s(offset %lu + size %lu > buffer size %lu)", func, > (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; > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "%s(range is mapped without GL_MAP_PERSISTENT_BIT)", </div></div>I don't understand why this error text was added here or below. If it is correct, it also seems like it should be a separate patch that includes justification in the commit message. </blockquote><div>In reviews for the ARB_direct_state_access texture functions, Brian Paul and Anuj Phogat recommended that error messages, if touched by a commit, should be more verbose. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + func); > + return false; > } > } > else { > if (_mesa_bufferobj_mapped(bufObj, MAP_USER)) { > - _mesa_error(ctx, GL_INVALID_OPERATION, "%s", caller); > - return NULL; > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "%s(buffer is mapped without GL_MAP_PERSISTENT_BIT)", > + func); > + return false; > } > } > > - return bufObj; > + return true; > } > > > @@ -602,9 +597,9 @@ _mesa_BufferData_sw(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 ) > +_mesa_BufferSubData_sw(struct gl_context *ctx, GLintptr offset, > + GLsizeiptr size, const GLvoid *data, > + struct gl_buffer_object *bufObj) > { > (void) ctx; > > @@ -1113,7 +1108,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 = _mesa_BufferData_sw; > - driver->BufferSubData = _mesa_buffer_subdata; > + driver->BufferSubData = _mesa_BufferSubData_sw; > driver->GetBufferSubData = _mesa_buffer_get_subdata; > driver->UnmapBuffer = _mesa_buffer_unmap; > > @@ -1580,24 +1575,33 @@ _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 mappedRange If true, checks if an overlapping range is mapped. > + * If false, checks if buffer is mapped. > + * \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, > + bool mappedRange, const char *func) </div></div>mappedRange is always false. Is that intentional? I don't see this function used anywhere else in the series, so I think this parameter should be removed. It also looks like this function is only called from this file, so it should lose the _mesa_ and be static. </blockquote><div> mappedRange is true in _mesa_ClearBufferSubData. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Unless you have other code that will use it. In that case, there should be some explanation in the commit message. </blockquote><div>There will be meta code that uses this. Read patch 0 of ARB_dsa textures. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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, > - "glBufferSubDataARB" ); > - if (!bufObj) { > + if (!buffer_object_subdata_range_good(ctx, bufObj, offset, size, > + mappedRange, 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; > } > > @@ -1607,22 +1611,54 @@ _mesa_BufferSubData(GLenum target, GLintptrARB offset, > bufObj->Written = GL_TRUE; > > ASSERT(ctx->Driver.BufferSubData); > - ctx->Driver.BufferSubData( ctx, offset, size, data, bufObj ); > + ctx->Driver.BufferSubData(ctx, offset, size, data, bufObj); Spurious whitespace change. Drop this change. </blockquote><div>Again, in previous reviews, Brian Paul and Anuj Phogat were in favor of getting rid of this style: ( ctx, ..., bufObj ) whenever possible within a commit. Since I was in the neighborhood, I changed it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > } > > +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, false, > + "glBufferSubData"); > +} > > void GLAPIENTRY > -_mesa_GetBufferSubData(GLenum target, GLintptrARB offset, > - GLsizeiptrARB size, void * data) > +_mesa_NamedBufferSubData(GLuint buffer, GLintptr offset, > + GLsizeiptr size, const GLvoid *data) > { > 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, false, > + "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; > } > > @@ -1696,10 +1732,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 9801c87..72d3d82 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, > + bool mappedRange, const char *func); > + > +extern void > _mesa_buffer_unmap_all_mappings(struct gl_context *ctx, > struct gl_buffer_object *bufObj); > > @@ -185,8 +190,12 @@ _mesa_NamedBufferData(GLuint buffer, GLsizeiptr size, > const GLvoid *data, GLenum usage); > > void GLAPIENTRY > -_mesa_BufferSubData(GLenum target, GLintptrARB offset, > - GLsizeiptrARB size, const GLvoid * data); > +_mesa_BufferSubData(GLenum target, GLintptr offset, > + GLsizeiptr size, const GLvoid *data); </div></div>As much as I hate the ARB suffixes, this is also a spurious change. </blockquote><div>Again, I wonder if other reviewers would agree with this. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class=""><div class="h5"> > + > +void GLAPIENTRY > +_mesa_NamedBufferSubData(GLuint buffer, GLintptr offset, > + GLsizeiptr size, const GLvoid *data); > > void GLAPIENTRY > _mesa_GetBufferSubData(GLenum target, GLintptrARB offset, > diff --git a/src/mesa/main/tests/dispatch_sanity.cpp b/src/mesa/main/tests/dispatch_sanity.cpp > index 3c87b5e..c639353 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 }, > </div></div></blockquote></div> </div></div>