[Mesa-dev] [PATCH 06/21] main: Add entry point for NamedBufferData.
Ian Romanick
idr at freedesktop.org
Sat Jan 31 23:30:55 PST 2015
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 <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 | 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 },
More information about the mesa-dev
mailing list