---------- Forwarded message ----------
From: Laura Ekstrand <la...@jlekstrand.net>
Date: Fri, Jan 30, 2015 at 3:42 PM
Subject: Re: [Mesa-dev] [PATCH 06/21] main: Add entry point for
NamedBufferData.
To: Ian Romanick <i...@freedesktop.org>
On Wed, Jan 21, 2015 at 6:28 PM, Ian Romanick <i...@freedesktop.org> wrote:
> On 01/21/2015 05:40 PM, Laura Ekstrand wrote:
> > ---
> > src/mapi/glapi/gen/ARB_direct_state_access.xml | 9 ++-
> > src/mesa/main/bufferobj.c | 77
> ++++++++++++++++----------
> > src/mesa/main/bufferobj.h | 13 ++++-
> > src/mesa/main/tests/dispatch_sanity.cpp | 1 +
> > 4 files changed, 69 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml
> b/src/mapi/glapi/gen/ARB_direct_state_access.xml
> > index ff81c21..4b1ef6f 100644
> > --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml
> > +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml
> > @@ -21,7 +21,14 @@
> > <param name="flags" type="GLbitfield" />
> > </function>
> >
> > - <!-- Texture object functions -->
> > + <function name="NamedBufferData" offset="assign">
> > + <param name="buffer" type="GLuint" />
> > + <param name="size" type="GLsizeiptr" />
> > + <param name="data" type="const GLvoid *" />
> > + <param name="usage" type="GLenum" />
> > + </function>
> > +
> > + <!-- Texture object functions -->
>
> I think you accidentally deleted a space before this comment.
>
> >
> > <function name="CreateTextures" offset="assign">
> > <param name="target" type="GLenum" />
> > diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> > index 0977459..c77029f 100644
> > --- a/src/mesa/main/bufferobj.c
> > +++ b/src/mesa/main/bufferobj.c
> > @@ -556,9 +556,9 @@ _mesa_total_buffer_object_memory(struct gl_context
> *ctx)
> > * \sa glBufferDataARB, dd_function_table::BufferData.
> > */
> > static GLboolean
> > -_mesa_buffer_data( struct gl_context *ctx, GLenum target, GLsizeiptrARB
> size,
> > - const GLvoid * data, GLenum usage, GLenum storageFlags,
> > - struct gl_buffer_object * bufObj )
> > +_mesa_BufferData_sw(struct gl_context *ctx, GLenum target, GLsizeiptr
> size,
> > + const GLvoid *data, GLenum usage, GLenum
> storageFlags,
> > + struct gl_buffer_object *bufObj)
>
> You are confused here. This function has nothing to do with swrast. It
is a fallback for a DD-table entry.
> We should pick a different name for this. Names with this pattern are
> (almost exclusively) direct API entry points. Maybe _swrast_buffer_data?
>
> Alternately, if the "new" _mesa_buffer_data will only ever be called
> from the same file, we could just call that buffer_data and make it static.
>
> > {
> > void * new_data;
> >
> > @@ -1112,7 +1112,7 @@ _mesa_init_buffer_object_functions(struct
> dd_function_table *driver)
> > /* GL_ARB_vertex/pixel_buffer_object */
> > driver->NewBufferObject = _mesa_new_buffer_object;
> > driver->DeleteBuffer = _mesa_delete_buffer_object;
> > - driver->BufferData = _mesa_buffer_data;
> > + driver->BufferData = _mesa_BufferData_sw;
> > driver->BufferSubData = _mesa_buffer_subdata;
> > driver->GetBufferSubData = _mesa_buffer_get_subdata;
> > driver->UnmapBuffer = _mesa_buffer_unmap;
> > @@ -1474,23 +1474,22 @@ _mesa_NamedBufferStorage(GLuint buffer,
> GLsizeiptr size, const GLvoid *data,
> > }
> >
> >
> > -
> > -void GLAPIENTRY
> > -_mesa_BufferData(GLenum target, GLsizeiptrARB size,
> > - const GLvoid * data, GLenum usage)
> > +void
> > +_mesa_buffer_data(struct gl_context *ctx, struct gl_buffer_object
> *bufObj,
> > + GLenum target, GLsizeiptr size, const GLvoid *data,
> > + GLenum usage, const char *func)
> > {
> > - GET_CURRENT_CONTEXT(ctx);
> > - struct gl_buffer_object *bufObj;
> > bool valid_usage;
> >
> > if (MESA_VERBOSE & VERBOSE_API)
> > - _mesa_debug(ctx, "glBufferData(%s, %ld, %p, %s)\n",
> > + _mesa_debug(ctx, "%s(%s, %ld, %p, %s)\n",
> > + func,
> > _mesa_lookup_enum_by_nr(target),
> > (long int) size, data,
> > _mesa_lookup_enum_by_nr(usage));
> >
> > if (size < 0) {
> > - _mesa_error(ctx, GL_INVALID_VALUE, "glBufferDataARB(size < 0)");
> > + _mesa_error(ctx, GL_INVALID_VALUE, "%s(size < 0)", func);
> > return;
> > }
> >
> > @@ -1519,16 +1518,13 @@ _mesa_BufferData(GLenum target, GLsizeiptrARB
> size,
> > }
> >
> > if (!valid_usage) {
> > - _mesa_error(ctx, GL_INVALID_ENUM, "glBufferData(usage)");
> > + _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid usage: %s)", func,
> > + _mesa_lookup_enum_by_nr(usage));
> > return;
> > }
> >
> > - bufObj = get_buffer(ctx, "glBufferDataARB", target,
> GL_INVALID_OPERATION);
> > - if (!bufObj)
> > - return;
> > -
> > if (bufObj->Immutable) {
> > - _mesa_error(ctx, GL_INVALID_OPERATION, "glBufferData(immutable)");
> > + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(immutable)", func);
> > return;
> > }
> >
> > @@ -1539,25 +1535,50 @@ _mesa_BufferData(GLenum target, GLsizeiptrARB
> size,
> >
> > bufObj->Written = GL_TRUE;
> >
> > -#ifdef VBO_DEBUG
> > - printf("glBufferDataARB(%u, sz %ld, from %p, usage 0x%x)\n",
> > - bufObj->Name, size, data, usage);
> > -#endif
> > -
> > -#ifdef BOUNDS_CHECK
> > - size += 100;
> > -#endif
> > -
>
> I believe what you meant to say is that this deletion should be in a
separate commit so we can revert it later if we want to?
> This change by itself is not okay. If someone builds with BOUNDS_CHECK
> defined, the code in _mesa_MapBuffer and _mesa_UnmapBuffer will explode.
> I don't know how much utility there is to BOUNDS_CHECK or VBO_DEBUG.
> I'd be inclined to send separate patches that completely remove each of
> those.
>
> It looks like BOUNDS_CHECK was added by Brian Paul in commit 4a7fd632.
> Maybe he has an opinion...
>
>
> > ASSERT(ctx->Driver.BufferData);
> > if (!ctx->Driver.BufferData(ctx, target, size, data, usage,
> > GL_MAP_READ_BIT |
> > GL_MAP_WRITE_BIT |
> > GL_DYNAMIC_STORAGE_BIT,
> > bufObj)) {
> > - _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBufferDataARB()");
> > + _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s", func);
> > }
> > }
> >
> > +void GLAPIENTRY
> > +_mesa_BufferData(GLenum target, GLsizeiptr size,
> > + const GLvoid *data, GLenum usage)
> > +{
> > + GET_CURRENT_CONTEXT(ctx);
> > + struct gl_buffer_object *bufObj;
> > +
> > + bufObj = get_buffer(ctx, "glBufferData", target,
> GL_INVALID_OPERATION);
> > + if (!bufObj)
> > + return;
> > +
> > + _mesa_buffer_data(ctx, bufObj, target, size, data, usage,
> > + "glBufferData");
> > +}
> > +
> > +void GLAPIENTRY
> > +_mesa_NamedBufferData(GLuint buffer, GLsizeiptr size, const GLvoid
> *data,
> > + GLenum usage)
> > +{
> > + GET_CURRENT_CONTEXT(ctx);
> > + struct gl_buffer_object *bufObj;
> > +
> > + bufObj = _mesa_lookup_bufferobj_err(ctx, buffer,
> "glNamedBufferData");
> > + if (!bufObj)
> > + return;
> > +
> > + /*
> > + * In direct state access, buffer objects have an unspecified target
> since
> > + * they are not required to be bound.
> > + */
>
> Tiny nit... the opening /* should go on the same like with the start of
> the comment text. It's probably not worth going through the series to
> change the existing occurrences, but new code should use that style.
>
> > + _mesa_buffer_data(ctx, bufObj, GL_NONE, size, data, usage,
> > + "glNamedBufferData");
> > +}
> > +
> >
> > void GLAPIENTRY
> > _mesa_BufferSubData(GLenum target, GLintptrARB offset,
> > diff --git a/src/mesa/main/bufferobj.h b/src/mesa/main/bufferobj.h
> > index 0537bd4..9801c87 100644
> > --- a/src/mesa/main/bufferobj.h
> > +++ b/src/mesa/main/bufferobj.h
> > @@ -135,6 +135,11 @@ _mesa_buffer_storage(struct gl_context *ctx, struct
> gl_buffer_object *bufObj,
> > GLbitfield flags, const char *func);
> >
> > extern void
> > +_mesa_buffer_data(struct gl_context *ctx, struct gl_buffer_object
> *bufObj,
> > + GLenum target, GLsizeiptr size, const GLvoid *data,
> > + GLenum usage, const char *func);
> > +
> > +extern void
> > _mesa_buffer_unmap_all_mappings(struct gl_context *ctx,
> > struct gl_buffer_object *bufObj);
> >
> > @@ -172,8 +177,12 @@ _mesa_NamedBufferStorage(GLuint buffer, GLsizeiptr
> size, const GLvoid *data,
> > GLbitfield flags);
> >
> > void GLAPIENTRY
> > -_mesa_BufferData(GLenum target, GLsizeiptrARB size,
> > - const GLvoid * data, GLenum usage);
> > +_mesa_BufferData(GLenum target, GLsizeiptr size,
> > + const GLvoid *data, GLenum usage);
> > +
> > +void GLAPIENTRY
> > +_mesa_NamedBufferData(GLuint buffer, GLsizeiptr size,
> > + const GLvoid *data, GLenum usage);
> >
> > void GLAPIENTRY
> > _mesa_BufferSubData(GLenum target, GLintptrARB offset,
> > diff --git a/src/mesa/main/tests/dispatch_sanity.cpp
> b/src/mesa/main/tests/dispatch_sanity.cpp
> > index 131d15e..3c87b5e 100644
> > --- a/src/mesa/main/tests/dispatch_sanity.cpp
> > +++ b/src/mesa/main/tests/dispatch_sanity.cpp
> > @@ -957,6 +957,7 @@ const struct function gl_core_functions_possible[] =
> {
> > /* GL_ARB_direct_state_access */
> > { "glCreateBuffers", 45, -1 },
> > { "glNamedBufferStorage", 45, -1 },
> > + { "glNamedBufferData", 45, -1 },
> > { "glCreateTextures", 45, -1 },
> > { "glTextureStorage1D", 45, -1 },
> > { "glTextureStorage2D", 45, -1 },
> >
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
