[Mesa-dev] [PATCH 3/4] mesa: Separate PBO validation checks from buffer mapping, to allow reuse

Laura Ekstrand laura at jlekstrand.net
Mon Mar 9 17:34:55 PDT 2015


On Thu, Mar 5, 2015 at 12:20 AM, Eduardo Lima Mitev <elima at igalia.com>
wrote:

> Internal PBO functions such as _mesa_map_validate_pbo_source() and
> _mesa_validate_pbo_compressed_teximage() perform validation and buffer
> mapping
> within the same call.
>
> This patch takes out the validation into separate functions to allow reuse
> of functionality by other code (i.e, gl(Compressed)Tex(Sub)Image).
> ---
>  src/mesa/main/pbo.c | 117
> ++++++++++++++++++++++++++++++++++++++--------------
>  src/mesa/main/pbo.h |  14 +++++++
>  2 files changed, 100 insertions(+), 31 deletions(-)
>
> diff --git a/src/mesa/main/pbo.c b/src/mesa/main/pbo.c
> index 259f763..5ae248c 100644
> --- a/src/mesa/main/pbo.c
> +++ b/src/mesa/main/pbo.c
> @@ -164,23 +164,18 @@ _mesa_map_pbo_source(struct gl_context *ctx,
>     return buf;
>  }
>
> -
>  /**
> - * Combine PBO-read validation and mapping.
> + * Perform PBO validation for read operations with uncompressed textures.
>   * If any GL errors are detected, they'll be recorded and NULL returned.
>   * \sa _mesa_validate_pbo_access
> - * \sa _mesa_map_pbo_source
> - * A call to this function should have a matching call to
> - * _mesa_unmap_pbo_source().
>   */
> -const GLvoid *
> -_mesa_map_validate_pbo_source(struct gl_context *ctx,
> -                              GLuint dimensions,
> -                              const struct gl_pixelstore_attrib *unpack,
> -                              GLsizei width, GLsizei height, GLsizei
> depth,
> -                              GLenum format, GLenum type,
> -                              GLsizei clientMemSize,
> -                              const GLvoid *ptr, const char *where)
> +bool
> +_mesa_validate_pbo_source(struct gl_context *ctx, GLuint dimensions,
> +                          const struct gl_pixelstore_attrib *unpack,
> +                          GLsizei width, GLsizei height, GLsizei depth,
> +                          GLenum format, GLenum type,
> +                          GLsizei clientMemSize,
> +                          const GLvoid *ptr, const char *where)
>  {
>     assert(dimensions == 1 || dimensions == 2 || dimensions == 3);
>
> @@ -188,26 +183,87 @@ _mesa_map_validate_pbo_source(struct gl_context *ctx,
>                                    format, type, clientMemSize, ptr)) {
>        if (_mesa_is_bufferobj(unpack->BufferObj)) {
>           _mesa_error(ctx, GL_INVALID_OPERATION,
> -                     "%s(out of bounds PBO access)", where);
>
Changing the error to "%s%uD, where, dimensions" is not ideal because the
other function that calls _mesa_map_validate_pbo_source is
_mesa_PolygonStipple, and printing "glPolygonStipple2D" doesn't make sense
because the name of the function is "glPolygonStipple."

> +                     "%s%uD(out of bounds PBO access)",
> +                     where, dimensions);
>        } else {
>           _mesa_error(ctx, GL_INVALID_OPERATION,
>
Why did you remove "bufSize (%d) is too small"?  If we are going to remove
that, maybe we should remove the if, then, else block because the error
messages are pretty much the same.  I recommend keeping the original error
messages for both and moving the "%sD" up to the calling functions (for
example, texture_error_check).

> -                     "%s(out of bounds access: bufSize (%d) is too
> small)",
> -                     where, clientMemSize);
> +                     "%s%uD(out of bounds access)",
> +                     where, dimensions);
>        }
> -      return NULL;
> +      return false;
>     }
>
>     if (!_mesa_is_bufferobj(unpack->BufferObj)) {
>        /* non-PBO access: no further validation to be done */
> -      return ptr;
> +      return true;
>     }
>
>     if (_mesa_check_disallowed_mapping(unpack->BufferObj)) {
>        /* buffer is already mapped - that's an error */
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(PBO is mapped)", where);
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s%uD(PBO is mapped)",
> +                  where, dimensions);
> +      return false;
> +   }
> +
> +   return true;
> +}
> +
> +/**
> + * Perform PBO validation for read operations with compressed textures.
> + * If any GL errors are detected, they'll be recorded and NULL returned.
> + */
> +bool
> +_mesa_validate_pbo_source_compressed(struct gl_context *ctx, GLuint
> dimensions,
> +                                     const struct gl_pixelstore_attrib
> *unpack,
> +                                     GLsizei imageSize, const GLvoid
> *pixels,
> +                                     const char *where)
> +{
> +   if (!_mesa_is_bufferobj(unpack->BufferObj)) {
> +      /* not using a PBO */
> +      return true;
> +   }
> +
> +   if ((const GLubyte *) pixels + imageSize >
> +       ((const GLubyte *) 0) + unpack->BufferObj->Size) {
> +      /* out of bounds read! */
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s%uD(invalid PBO access)",
> +                  where, dimensions);
>
This should be return false.

>        return NULL;
>     }
>
> +   if (_mesa_check_disallowed_mapping(unpack->BufferObj)) {
> +      /* buffer is already mapped - that's an error */
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s%uD(PBO is mapped)",
> +                  where, dimensions);
> +      return false;
> +   }
> +
> +   return true;
> +}
> +
> +/**
> + * Perform PBO-read mapping.
> + * If any GL errors are detected, they'll be recorded and NULL returned.
> + * \sa _mesa_validate_pbo_source
> + * \sa _mesa_map_pbo_source
> + * A call to this function should have a matching call to
> + * _mesa_unmap_pbo_source().
> + */
> +const GLvoid *
> +_mesa_map_validate_pbo_source(struct gl_context *ctx,
> +                              GLuint dimensions,
> +                              const struct gl_pixelstore_attrib *unpack,
> +                              GLsizei width, GLsizei height, GLsizei
> depth,
> +                              GLenum format, GLenum type,
> +                              GLsizei clientMemSize,
> +                              const GLvoid *ptr, const char *where)
> +{
> +   if (!_mesa_validate_pbo_source(ctx, dimensions, unpack,
> +                                  width, height, depth, format, type,
> +                                  clientMemSize, ptr, where)) {
> +     return NULL;
> +   }
> +
>     ptr = _mesa_map_pbo_source(ctx, unpack, ptr);
>     return ptr;
>  }
> @@ -381,28 +437,27 @@ _mesa_validate_pbo_compressed_teximage(struct
> gl_context *ctx,
>  {
>     GLubyte *buf;
>
> +   if (!_mesa_validate_pbo_source_compressed(ctx, dimensions, packing,
> +                                             imageSize, pixels,
> funcName)) {
> +     /* error is already set during validation */
> +      return NULL;
> +   }
> +
>     if (!_mesa_is_bufferobj(packing->BufferObj)) {
>        /* not using a PBO - return pointer unchanged */
>        return pixels;
>     }
> -   if ((const GLubyte *) pixels + imageSize >
> -       ((const GLubyte *) 0) + packing->BufferObj->Size) {
> -      /* out of bounds read! */
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s%uD(invalid PBO access)",
> -                  funcName, dimensions);
> -      return NULL;
> -   }
>
>     buf = (GLubyte*) ctx->Driver.MapBufferRange(ctx, 0,
>                                                packing->BufferObj->Size,
>                                                GL_MAP_READ_BIT,
>                                                packing->BufferObj,
>                                                 MAP_INTERNAL);
> -   if (!buf) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s%uD(PBO is mapped)",
> funcName,
> -                  dimensions);
> -      return NULL;
> -   }
> +
> +   /* Validation above already checked that PBO is not mapped, so buffer
> +    * should not be null.
> +    */
> +   assert(buf);
>
>     return ADD_POINTERS(buf, pixels);
>  }
> diff --git a/src/mesa/main/pbo.h b/src/mesa/main/pbo.h
> index 9851ef1..b3f24e6 100644
> --- a/src/mesa/main/pbo.h
> +++ b/src/mesa/main/pbo.h
> @@ -92,4 +92,18 @@ _mesa_unmap_teximage_pbo(struct gl_context *ctx,
>                           const struct gl_pixelstore_attrib *unpack);
>
>
> +extern bool
> +_mesa_validate_pbo_source(struct gl_context *ctx, GLuint dimensions,
> +                          const struct gl_pixelstore_attrib *unpack,
> +                          GLsizei width, GLsizei height, GLsizei depth,
> +                          GLenum format, GLenum type,
> +                          GLsizei clientMemSize,
> +                          const GLvoid *ptr, const char *where);
> +
> +extern bool
> +_mesa_validate_pbo_source_compressed(struct gl_context *ctx, GLuint
> dimensions,
> +                                     const struct gl_pixelstore_attrib
> *unpack,
> +                                     GLsizei imageSize, const GLvoid *ptr,
> +                                     const char *where);
> +
>  #endif
> --
> 2.1.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150309/757209f7/attachment-0001.html>


More information about the mesa-dev mailing list