On 02/01/2015 08:25 AM, Ian Romanick wrote: > Did you intend to reply off-list? All discussions about code changes > should be on the public list.
Never mind. I just saw the extra fwd... > On 01/31/2015 12:42 AM, Laura Ekstrand wrote: >> >> >> On Wed, Jan 21, 2015 at 6:28 PM, Ian Romanick <i...@freedesktop.org >> <mailto: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. > > I am not confused. I know what it is. The name you picked does not > follow Mesa coding standards. > >> 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? > > No. I meant what I said. This hunk *by itself* is wrong. > >> 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
