That would make the diff easier to read, but it doesn't make sense to lay
out the functions in that order in the file. GetBufferSubData is a
download function and shouldn't be part of the upload function "group" in
the file.
On Fri, Feb 20, 2015 at 2:28 AM, Martin Peres <martin.pe...@free.fr> wrote:
> On 12/02/2015 04:05, Laura Ekstrand wrote:
>
>> v2: review by Ian Romanick
>> - Remove "_mesa" from name of static software fallback
>> buffer_sub_data.
>> - Remove mappedRange from _mesa_buffer_sub_data.
>> - Removed some cosmetic changes to a separate commit.
>> ---
>> src/mapi/glapi/gen/ARB_direct_state_access.xml | 7 ++
>> src/mesa/main/bufferobj.c | 129
>> +++++++++++++++----------
>> src/mesa/main/bufferobj.h | 9 ++
>> src/mesa/main/tests/dispatch_sanity.cpp | 1 +
>> 4 files changed, 97 insertions(+), 49 deletions(-)
>>
>> diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml
>> b/src/mapi/glapi/gen/ARB_direct_state_access.xml
>> index 7779262..6d70b8e 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 ac8eed1..4f89748 100644
>> --- a/src/mesa/main/bufferobj.c
>> +++ b/src/mesa/main/bufferobj.c
>> @@ -227,67 +227,58 @@ 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.
>> + * \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 *caller)
>> {
>> - struct gl_buffer_object *bufObj;
>> -
>> if (size < 0) {
>> _mesa_error(ctx, GL_INVALID_VALUE, "%s(size < 0)", caller);
>> - return NULL;
>> + return false;
>> }
>> if (offset < 0) {
>> _mesa_error(ctx, GL_INVALID_VALUE, "%s(offset < 0)", caller);
>> - return NULL;
>> + 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,
>> (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;
>> + return false;
>> }
>> }
>> else {
>> if (_mesa_bufferobj_mapped(bufObj, MAP_USER)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION, "%s", caller);
>> - return NULL;
>> + return false;
>> }
>> }
>> - return bufObj;
>> + return true;
>> }
>> @@ -602,9 +593,9 @@ buffer_data_fallback(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 )
>> +buffer_sub_data_fallback(struct gl_context *ctx, GLintptr offset,
>> + GLsizeiptr size, const GLvoid *data,
>> + struct gl_buffer_object *bufObj)
>> {
>> (void) ctx;
>> @@ -1113,7 +1104,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 = buffer_data_fallback;
>> - driver->BufferSubData = _mesa_buffer_subdata;
>> + driver->BufferSubData = buffer_sub_data_fallback;
>> driver->GetBufferSubData = _mesa_buffer_get_subdata;
>> driver->UnmapBuffer = _mesa_buffer_unmap;
>> @@ -1588,24 +1579,31 @@ _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 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,
>> + const char *func)
>> {
>> - 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,
>> + false, 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;
>> }
>> @@ -1618,19 +1616,50 @@ _mesa_BufferSubData(GLenum target, GLintptrARB
>> offset,
>> ctx->Driver.BufferSubData( ctx, offset, size, data, bufObj );
>> }
>> +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,
>> "glBufferSubData");
>> +}
>> void GLAPIENTRY
>> -_mesa_GetBufferSubData(GLenum target, GLintptrARB offset,
>> - GLsizeiptrARB size, void * data)
>> +_mesa_NamedBufferSubData(GLuint buffer, GLintptr offset,
>> + GLsizeiptr size, const GLvoid *data)
>>
>
> In this case, I would have added _mesa_NamedBufferSubData under
> _mesa_GetBufferSubData to make the diff more readable. It is good
> practice to make the diff as short as possible and to prove that you
> changed as little as possible in the existing functions :)
>
> Other than that, looks good to me!
>
> Reviewed-by: Martin Peres <martin.pe...@linux.intel.com>
>
>
> {
>> 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,
>> + "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;
>> }
>> @@ -1704,10 +1733,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 ddd240c..889bbb1 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,
>> + const char *func);
>> +
>> +extern void
>> _mesa_buffer_unmap_all_mappings(struct gl_context *ctx,
>> struct gl_buffer_object *bufObj);
>> @@ -189,6 +194,10 @@ _mesa_BufferSubData(GLenum target, GLintptrARB
>> offset,
>> GLsizeiptrARB size, const GLvoid * data);
>> void GLAPIENTRY
>> +_mesa_NamedBufferSubData(GLuint buffer, GLintptr offset,
>> + GLsizeiptr size, const GLvoid *data);
>> +
>> +void GLAPIENTRY
>> _mesa_GetBufferSubData(GLenum target, GLintptrARB offset,
>> GLsizeiptrARB size, void * data);
>> diff --git a/src/mesa/main/tests/dispatch_sanity.cpp
>> b/src/mesa/main/tests/dispatch_sanity.cpp
>> index 595ee90..898a2b9 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 },
>>
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
