<div dir="ltr"><div>Because of your comments, there will be a v2 patch series. But I won't be sending it for a week or so because I'm in the middle of implementing BlitNamedFramebuffer, which has required a lot of concentration. </div>Laura </div><div class="gmail_extra"> <div class="gmail_quote">On Sat, Jan 31, 2015 at 11:46 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 01/31/2015 02:21 AM, Laura Ekstrand wrote: > > > On Wed, Jan 21, 2015 at 6:42 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> <div><div class="h5">> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: > > 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) > > 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. > > > { > > - 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)", > > 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. > > 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></div>But I don't see the connection between the new messages and the code. bufferobj_range_mapped doesn't look at GL_MAP_PERSISTENT_BIT, but it can generate an error for a bad range. Now we generate a more verbose message that may be completely wrong... Either way, it should be it's own patch with its own explanation. It should not be buried is some other, unrelated change. <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) > > 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. > > mappedRange is true in _mesa_ClearBufferSubData. > > > Unless you have other code that will use it. In that case, there should > be some explanation in the commit message. > > There will be meta code that uses this. Read patch 0 of ARB_dsa textures. </div></div>That should be made clear in the commit message, or the function should be made public in that future series. Changesets and commit message should be self contained. I someone looks back at this code with git-blame, the won't have access to information in patch 0 of some other patch series. <div><div class="h5"> > > { > > - 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. > > 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></div>Do not mix whitespace changes / trivial clean ups with substantive changes. This is well accepted practice in open source. Since this change was with substantive changes, I had to scrutinize that line of code to make sure I wasn't missing anything. It wastes reviewer's time. <div class="HOEnZb"><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); > > As much as I hate the ARB suffixes, this is also a spurious change. > > Again, I wonder if other reviewers would agree with this. > > > > + > > +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>