[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