[Mesa-dev] [PATCH 1/4] common: Correct texture initialization in create_texture_for_pbo.

[Kenneth Graunke]

On Friday, February 20, 2015 01:30:53 PM Laura Ekstrand wrote:
> Solves bugs related to the driver not setting up the texture miptree
> correctly, leading to faulty PBO uploads and downloads.
> ---
>  src/mesa/drivers/common/meta_tex_subimage.c | 13 +++++++++----
>  src/mesa/drivers/dri/i965/intel_tex.c       |  3 ++-
>  src/mesa/main/dd.h                          |  1 +
>  3 files changed, 12 insertions(+), 5 deletions(-)

Hi Laura,

It looks like you're fixing several different bugs in this one patch -
and from your commit message, it's not really obvious what those bugs
are or how they manifest.

Would you mind splitting this into a couple patches and explaining each
fix in the respective commit message?

> 
> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c b/src/mesa/drivers/common/meta_tex_subimage.c
> index 68c8273..f4f7716 100644
> --- a/src/mesa/drivers/common/meta_tex_subimage.c
> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> @@ -51,7 +51,7 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>  {
>     uint32_t pbo_format;
>     GLenum internal_format;
> -   unsigned row_stride;
> +   unsigned row_stride, image_stride;
>     struct gl_buffer_object *buffer_obj;
>     struct gl_texture_object *tex_obj;
>     struct gl_texture_image *tex_image;
> @@ -74,6 +74,8 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>     pixels = _mesa_image_address3d(packing, pixels,
>                                    width, height, format, type, 0, 0, 0);
>     row_stride = _mesa_image_row_stride(packing, width, format, type);
> +   image_stride = _mesa_image_image_stride(packing, width, height, format,
> +                                           type);
>  

For example, I'm not really sure what this is attempting to solve or why
it's necessary.  It would be great to briefly explain that in the commit
message (i.e. what problem did you encounter, and why does this fix it?)

>     if (_mesa_is_bufferobj(packing->BufferObj)) {
>        *tmp_pbo = 0;
> @@ -89,7 +91,7 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>         */
>        _mesa_BindBuffer(pbo_target, *tmp_pbo);
>  
> -      _mesa_BufferData(pbo_target, row_stride * height, pixels, GL_STREAM_DRAW);
> +      _mesa_BufferData(pbo_target, image_stride * depth, pixels, GL_STREAM_DRAW);
>  
>        buffer_obj = ctx->Unpack.BufferObj;
>        pixels = NULL;
> @@ -99,9 +101,11 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>  
>     _mesa_GenTextures(1, tmp_tex);
>     tex_obj = _mesa_lookup_texture(ctx, *tmp_tex);
> -   tex_obj->Target = depth > 1 ? GL_TEXTURE_2D_ARRAY : GL_TEXTURE_2D;
> -   tex_obj->Immutable = GL_TRUE;
>     _mesa_initialize_texture_object(ctx, tex_obj, *tmp_tex, GL_TEXTURE_2D);
> +   /* This must be set after _mesa_initialize_texture_object, not before. */
> +   tex_obj->Immutable = GL_TRUE;
> +   /* This is required for interactions with ARB_texture_view. */
> +   tex_obj->NumLayers = 1;

This seems separate from the other change - gl_texture_objects with
Immutable set must also have NumLayers set to value > 0.  Otherwise, we
use (0-1)=0xFFFFFFFF as the depth when setting up SURFACE_STATE,
triggering assertions.

Thanks, Laura!

>  
>     internal_format = _mesa_get_format_base_format(pbo_format);
>  
> @@ -114,6 +118,7 @@ create_texture_for_pbo(struct gl_context *ctx, bool create_pbo,
>                                                       buffer_obj,
>                                                       (intptr_t)pixels,
>                                                       row_stride,
> +                                                     image_stride,
>                                                       read_only)) {
>        _mesa_DeleteTextures(1, tmp_tex);
>        _mesa_DeleteBuffers(1, tmp_pbo);
> diff --git a/src/mesa/drivers/dri/i965/intel_tex.c b/src/mesa/drivers/dri/i965/intel_tex.c
> index 2d3009a..3a0c09a 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex.c
> @@ -305,6 +305,7 @@ intel_set_texture_storage_for_buffer_object(struct gl_context *ctx,
>                                              struct gl_buffer_object *buffer_obj,
>                                              uint32_t buffer_offset,
>                                              uint32_t row_stride,
> +                                            uint32_t image_stride,
>                                              bool read_only)
>  {
>     struct brw_context *brw = brw_context(ctx);
> @@ -334,7 +335,7 @@ intel_set_texture_storage_for_buffer_object(struct gl_context *ctx,
>  
>     drm_intel_bo *bo = intel_bufferobj_buffer(brw, intel_buffer_obj,
>                                               buffer_offset,
> -                                             row_stride * image->Height);
> +                                             image_stride * image->Depth);
>     intel_texobj->mt =
>        intel_miptree_create_for_bo(brw, bo,
>                                    image->TexFormat,
> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
> index ec8662b..9de08e2 100644
> --- a/src/mesa/main/dd.h
> +++ b/src/mesa/main/dd.h
> @@ -429,6 +429,7 @@ struct dd_function_table {
>                                              struct gl_buffer_object *bufferObj,
>                                              uint32_t buffer_offset,
>                                              uint32_t row_stride,
> +                                            uint32_t image_stride,
>                                              bool read_only);
>  
>     /**
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150223/fccab8db/attachment-0001.sig>