<div dir="ltr"> <div class="gmail_quote">---------- Forwarded message ---------- From: Laura Ekstrand <<a href="mailto:laura@jlekstrand.net">laura@jlekstrand.net</a>> 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 <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> <div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote"><div><div class="h5">On Wed, Jan 21, 2015 at 6:28 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/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> > > -  > + <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> > + > +  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) </blockquote></div></div><div>You are confused here. This function has nothing to do with swrast. It is a fallback for a DD-table entry. </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. <div><div> > { > 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 > - </div></div></blockquote></div></div><div>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? </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div> </div></div>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... </blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > 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. > + */ </div></div>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. <div><div> > + _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 }, > </div></div></blockquote></div></div></div> </div></div> </div> </div>