[Mesa-dev] [PATCH 08/21] main: Add entry point for NamedBufferSubData.
Laura Ekstrand
laura at jlekstrand.net
Tue Feb 3 09:40:09 PST 2015
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.
Laura
On Sat, Jan 31, 2015 at 11:46 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 01/31/2015 02:21 AM, Laura Ekstrand wrote:
> >
> >
> > On Wed, Jan 21, 2015 at 6:42 PM, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> 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>
> > > +
> > > <!-- Texture object functions -->
> > >
> > > <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.
>
> 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.
>
> > > + 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.
>
> 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.
>
> > > {
> > > - 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.
>
> 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.
>
> > > }
> > >
> > > +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 },
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150203/ef10a2f5/attachment-0001.html>
More information about the mesa-dev
mailing list