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

Laura Ekstrand laura at jlekstrand.net
Wed Mar 11 16:15:57 PDT 2015


On Tue, Mar 10, 2015 at 11:34 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 | 119
> ++++++++++++++++++++++++++++++++++++++--------------
>  src/mesa/main/pbo.h |  14 +++++++
>  2 files changed, 101 insertions(+), 32 deletions(-)
>
> diff --git a/src/mesa/main/pbo.c b/src/mesa/main/pbo.c
> index 259f763..46121a2 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.
>
Maybe say something like "If there are errors, return false, else return
true"?  This is important because in other parts of the driver (like
main/teximage.c), error checking functions return GL_TRUE if an error is
found and GL_FALSE if one isn't found.  (Confusing, I know :< )

>   * \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,24 +183,85 @@ _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);
> -      } else {
> -         _mesa_error(ctx, GL_INVALID_OPERATION,
>
You changed the order of the conditions from the original version.  In the
original, if (_mesa_is_bufferobj), then say "out of bounds PBO access."  If
(!_mesa_is_bufferobj), say "out of bounds access: bufSize is too small."
You have them switched around here.

>                       "%s(out of bounds access: bufSize (%d) is too
> small)",
>                       where, clientMemSize);
> +      } else {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "%s(out of bounds access)",
> +                     where);
>        }
> -      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);
> -      return NULL;
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(PBO is mapped)",
> +                  where);
> +      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.
>
Same here as above.  Say something about the return value (true or false).

> + */
> +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(invalid PBO access)",
> +                  where);
> +      return false;
> +   }
> +
> +   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);
> +      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);
> @@ -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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150311/eb11ab3e/attachment-0001.html>


More information about the mesa-dev mailing list