On 20/02/2015 22:17, Laura Ekstrand wrote:
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.

Fair enough! That was a minor nitpick anyway and I did not expect you to fix it, hence why I gave my R-b ;)


On Fri, Feb 20, 2015 at 2:28 AM, Martin Peres <martin.pe...@free.fr <mailto: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
    <mailto: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

Reply via email to