[Mesa-dev] [PATCH 3/5] mesa: add _mesa_bufferobj_range_mapped function

[Brian Paul]

On 12/10/2013 06:13 AM, Pi Tabred wrote:
> Add function to test if the buffer is already mapped and
> if so, if the to be mapped range overlaps with the mapped range.

That could be worded better.  How about: "and if so, if the mapped
range overlaps the given range."


> Modify the _mesa_InvalidateBufferSubData function to use
> the new function.
> ---
>   src/mesa/main/bufferobj.c | 52 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> index 8b5ebc4..bfeed83 100644
> --- a/src/mesa/main/bufferobj.c
> +++ b/src/mesa/main/bufferobj.c
> @@ -241,6 +241,40 @@ buffer_object_subdata_range_good( struct gl_context * ctx, GLenum target,
>
>
>   /**
> + * Tests if to be used range is already mapped.
> + * The regions do not overlap if and only if the end of the discard
> + * region is before the mapped region or the start of the discard region
> + * is after the mapped region.

Test if the buffer is mapped, and if so, the mapped range overlaps the 
given range.

> + *
> + * Note that 'end' and 'mappEnd' are the first byte *after* the discard
> + * region and the mapped region, repsectively.  It is okay for that byte
> + * to be mapped (for 'end') or discarded (for 'mapEnd').

I think you can drop that paragraph.  It doesn't really belong in the 
function documentation since it's an implementation detail.  Plus it has 
some typos.


> + *
> + * \param obj     Buffer object target on which to operate.
> + * \param offset  Offset of the first byte of the subdata range.
> + * \param size    Size, in bytes, of the subdata range.
> + * \return   true if ranges overlap, false otherwise
> + *
> + */
> +static GLboolean
> +_mesa_bufferobj_range_mapped( const struct gl_buffer_object *obj,
> +                              GLintptr offset, GLsizeiptr size )

Remove space after ( and before ).  And static functions usually don't 
have the _mesa_ prefix.


> +{
> +   if (_mesa_bufferobj_mapped(obj))
> +   {

Opening brace belongs on same line as the 'if'.


> +      const GLintptr end = offset + size;
> +      const GLintptr mapEnd = obj->Offset + obj->Length;
> +
> +      if (!(end <= obj->Offset || offset >= mapEnd))
> +      {

Move brace.

> +         return GL_TRUE;
> +      }
> +   }
> +   return GL_FALSE;
> +}

The function could be declared as returning bool/true/false.


> +
> +
> +/**
>    * Allocate and initialize a new buffer object.
>    *
>    * Default callback for the \c dd_function_table::NewBufferObject() hook.
> @@ -2331,23 +2365,11 @@ _mesa_InvalidateBufferSubData(GLuint buffer, GLintptr offset,
>       *     mapped by MapBuffer, or if the invalidate range intersects the range
>       *     currently mapped by MapBufferRange."
>       */
> -   if (_mesa_bufferobj_mapped(bufObj)) {
> -      const GLintptr mapEnd = bufObj->Offset + bufObj->Length;
> -
> -      /* The regions do not overlap if and only if the end of the discard
> -       * region is before the mapped region or the start of the discard region
> -       * is after the mapped region.
> -       *
> -       * Note that 'end' and 'mapEnd' are the first byte *after* the discard
> -       * region and the mapped region, repsectively.  It is okay for that byte
> -       * to be mapped (for 'end') or discarded (for 'mapEnd').
> -       */
> -      if (!(end <= bufObj->Offset || offset >= mapEnd)) {
> -         _mesa_error(ctx, GL_INVALID_OPERATION,
> +   if (_mesa_bufferobj_range_mapped(bufObj, offset, length)) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>                        "glInvalidateBufferSubData(intersection with mapped "
>                        "range)");
> -         return;
> -      }
> +      return;
>      }
>
>      /* We don't actually do anything for this yet.  Just return after
>