[Mesa-dev] [PATCH 08/21] main: Add entry point for NamedBufferSubData.
Ian Romanick
idr at freedesktop.org
Wed Jan 21 18:42:03 PST 2015
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.
> + 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.
Unless you have other code that will use it. In that case, there should
be some explanation in the commit message.
> {
> - 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.
> }
>
> +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.
> +
> +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 },
>
More information about the mesa-dev
mailing list