[Mesa-dev] [PATCH] meta: Compute correct buffer size with SkipRows/SkipPixels

Neil Roberts neil at linux.intel.com
Tue Sep 1 05:09:33 PDT 2015


Good catch and it seems like a nice way to fix it.

Reviewed-by: Neil Roberts <neil at linux.intel.com>

I wonder if it might be worth avoiding copying the padding and pack the
rows more tightly in the temporary buffer. Ie, we would allocate a
buffer of align(width*cpp)*height*depth and copy the rows in one at a
time instead of memcpying the whole buffer. As it stands for example if
you are using the stride to make a 16x16 texture of a subregion of a
1024x1024 buffer we will pointlessly make a temporary buffer of 1024x16.
I'm not saying we should hold up this fix for that though.

Regards,
- Neil

Chris Wilson <chris at chris-wilson.co.uk> writes:

> From: Jason Ekstrand <jason.ekstrand at intel.com>
>
> If the user is specifying a subregion of a buffer using SKIP_ROWS and
> SKIP_PIXELS, we must compute the buffer size carefully as the end of the
> last row may be much shorter than stride*image_height*depth. The current
> code tries to memcpy from beyond the end of the user data, for example
> causing:
>
> ==28136== Invalid read of size 8
> ==28136==    at 0x4C2D94E: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:915)
> ==28136==    by 0xB4ADFE3: brw_bo_write (brw_batch.c:1856)
> ==28136==    by 0xB5B3531: brw_buffer_data (intel_buffer_objects.c:208)
> ==28136==    by 0xB0F6275: _mesa_buffer_data (bufferobj.c:1600)
> ==28136==    by 0xB0F6346: _mesa_BufferData (bufferobj.c:1631)
> ==28136==    by 0xB37A1EE: create_texture_for_pbo (meta_tex_subimage.c:103)
> ==28136==    by 0xB37A467: _mesa_meta_pbo_TexSubImage (meta_tex_subimage.c:176)
> ==28136==    by 0xB5C8D61: intelTexSubImage (intel_tex_subimage.c:195)
> ==28136==    by 0xB254AB4: _mesa_texture_sub_image (teximage.c:3654)
> ==28136==    by 0xB254C9F: texsubimage (teximage.c:3712)
> ==28136==    by 0xB2550E9: _mesa_TexSubImage2D (teximage.c:3853)
> ==28136==    by 0x401CA0: UploadTexSubImage2D (teximage.c:171)
> ==28136==  Address 0xd8bfbe0 is 0 bytes after a block of size 1,024 alloc'd
> ==28136==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
> ==28136==    by 0x402014: PerfDraw (teximage.c:270)
> ==28136==    by 0x402648: Draw (glmain.c:182)
> ==28136==    by 0x8385E63: ??? (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x83896C8: fgEnumWindows (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x838641C: glutMainLoopEvent (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x8386C1C: glutMainLoop (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x4019C1: main (glmain.c:262)
> ==28136==
> ==28136== Invalid read of size 8
> ==28136==    at 0x4C2D940: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:915)
> ==28136==    by 0xB4ADFE3: brw_bo_write (brw_batch.c:1856)
> ==28136==    by 0xB5B3531: brw_buffer_data (intel_buffer_objects.c:208)
> ==28136==    by 0xB0F6275: _mesa_buffer_data (bufferobj.c:1600)
> ==28136==    by 0xB0F6346: _mesa_BufferData (bufferobj.c:1631)
> ==28136==    by 0xB37A1EE: create_texture_for_pbo (meta_tex_subimage.c:103)
> ==28136==    by 0xB37A467: _mesa_meta_pbo_TexSubImage (meta_tex_subimage.c:176)
> ==28136==    by 0xB5C8D61: intelTexSubImage (intel_tex_subimage.c:195)
> ==28136==    by 0xB254AB4: _mesa_texture_sub_image (teximage.c:3654)
> ==28136==    by 0xB254C9F: texsubimage (teximage.c:3712)
> ==28136==    by 0xB2550E9: _mesa_TexSubImage2D (teximage.c:3853)
> ==28136==    by 0x401CA0: UploadTexSubImage2D (teximage.c:171)
> ==28136==  Address 0xd8bfbe8 is 8 bytes after a block of size 1,024 alloc'd
> ==28136==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
> ==28136==    by 0x402014: PerfDraw (teximage.c:270)
> ==28136==    by 0x402648: Draw (glmain.c:182)
> ==28136==    by 0x8385E63: ??? (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x83896C8: fgEnumWindows (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x838641C: glutMainLoopEvent (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x8386C1C: glutMainLoop (in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0)
> ==28136==    by 0x4019C1: main (glmain.c:262)
> ==28136==
>
> Fixes: 7f396189f073d626c5f7a2c232dac92b65f5a23f
> Cc: Jason Ekstrand <jason.ekstrand at intel.com>
> Cc: Neil Roberts <neil at linux.intel.com>
> ---
>  src/mesa/drivers/common/meta_tex_subimage.c | 35 +++++++++++++++++------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c b/src/mesa/drivers/common/meta_tex_subimage.c
> index 16d8f5d..e2351c6 100644
> --- a/src/mesa/drivers/common/meta_tex_subimage.c
> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> @@ -46,8 +46,9 @@
>  #include "varray.h"
>  
>  static struct gl_texture_image *
> -create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
> -                       GLenum pbo_target, int width, int height,
> +create_texture_for_pbo(struct gl_context *ctx,
> +                       bool create_pbo, GLenum pbo_target,
> +                       int dims, int width, int height, int depth,
>                         GLenum format, GLenum type, const void *pixels,
>                         const struct gl_pixelstore_attrib *packing,
>                         GLuint *tmp_pbo, GLuint *tmp_tex)
> @@ -73,8 +74,12 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>        return NULL;
>  
>     /* Account for SKIP_PIXELS, SKIP_ROWS, ALIGNMENT, and SKIP_IMAGES */
> -   pixels = _mesa_image_address3d(packing, pixels,
> -                                  width, height, format, type, 0, 0, 0);
> +   uint32_t first_pixel = _mesa_image_offset(dims, packing, width, height,
> +                                             format, type,
> +                                             0, 0, 0);
> +   uint32_t last_pixel =  _mesa_image_offset(dims, packing, width, height,
> +                                             format, type,
> +                                             depth-1, height-1, width);
>     row_stride = _mesa_image_row_stride(packing, width, format, type);
>  
>     if (_mesa_is_bufferobj(packing->BufferObj)) {
> @@ -97,14 +102,18 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>         * data to avoid unnecessary data copying in _mesa_BufferData().
>         */
>        if (is_pixel_pack)
> -         _mesa_BufferData(pbo_target, row_stride * height, NULL,
> +         _mesa_BufferData(pbo_target,
> +                          last_pixel - first_pixel,
> +                          NULL,
>                            GL_STREAM_READ);
>        else
> -         _mesa_BufferData(pbo_target, row_stride * height, pixels,
> +         _mesa_BufferData(pbo_target,
> +                          last_pixel - first_pixel,
> +                          (char *)pixels + first_pixel,
>                            GL_STREAM_DRAW);
>  
>        buffer_obj = packing->BufferObj;
> -      pixels = NULL;
> +      first_pixel = 0;
>  
>        _mesa_BindBuffer(pbo_target, 0);
>     }
> @@ -126,7 +135,7 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>     read_only = pbo_target == GL_PIXEL_UNPACK_BUFFER;
>     if (!ctx->Driver.SetTextureStorageForBufferObject(ctx, tex_obj,
>                                                       buffer_obj,
> -                                                     (intptr_t)pixels,
> +                                                     first_pixel,
>                                                       row_stride,
>                                                       read_only)) {
>        _mesa_DeleteTextures(1, tmp_tex);
> @@ -147,7 +156,7 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint dims,
>                             const struct gl_pixelstore_attrib *packing)
>  {
>     GLuint pbo = 0, pbo_tex = 0, fbos[2] = { 0, 0 };
> -   int full_height, image_height;
> +   int image_height;
>     struct gl_texture_image *pbo_tex_image;
>     GLenum status;
>     bool success = false;
> @@ -171,11 +180,10 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint dims,
>      * property.
>      */
>     image_height = packing->ImageHeight == 0 ? height : packing->ImageHeight;
> -   full_height = image_height * (depth - 1) + height;
>  
>     pbo_tex_image = create_texture_for_pbo(ctx, create_pbo,
>                                            GL_PIXEL_UNPACK_BUFFER,
> -                                          width, full_height,
> +                                          dims, width, height, depth,
>                                            format, type, pixels, packing,
>                                            &pbo, &pbo_tex);
>     if (!pbo_tex_image)
> @@ -277,7 +285,7 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
>                                const struct gl_pixelstore_attrib *packing)
>  {
>     GLuint pbo = 0, pbo_tex = 0, fbos[2] = { 0, 0 };
> -   int full_height, image_height;
> +   int image_height;
>     struct gl_texture_image *pbo_tex_image;
>     struct gl_renderbuffer *rb = NULL;
>     GLenum dstBaseFormat = _mesa_unpack_format_to_base_format(format);
> @@ -324,10 +332,9 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
>      * property.
>      */
>     image_height = packing->ImageHeight == 0 ? height : packing->ImageHeight;
> -   full_height = image_height * (depth - 1) + height;
>  
>     pbo_tex_image = create_texture_for_pbo(ctx, false, GL_PIXEL_PACK_BUFFER,
> -                                          width, full_height * depth,
> +                                          dims, width, height, depth,
>                                            format, type, pixels, packing,
>                                            &pbo, &pbo_tex);
>     if (!pbo_tex_image)
> -- 
> 2.5.1


More information about the mesa-dev mailing list