[Mesa-dev] [PATCH 04/12] r600g: fix texture offset computation for mapped MSAA depth buffers

[Christian König]

Am 01.07.2013 03:53, schrieb Marek Olšák:
> It was wrong, because the offset shouldn't be applied to MSAA depth buffers.
> This small cleanup should prevent such issues in the future.
>
> This fixes a lockup in "piglit/fbo-depthstencil default_fb -samples=n".
>
> The lockup was special, because it was triggered by the CPU writing to memory
> outside of the buffer range. I didn't know it was possible to hang a machine
> with the CPU. This is nuts.

WTF? Well if my understanding of our memory controller is correct it is 
possible to hang it when the CPU tries to access some non existing 
memory, but to make this happen the kernel driver must program the PCI 
BAR somehow incorrectly.

Anyway it's good that this is fixed, but I'm pretty sure we should take 
a closer ĺook what's happening here.

Christian.

> ---
>   src/gallium/drivers/r600/r600_pipe.h    |  2 --
>   src/gallium/drivers/r600/r600_texture.c | 28 ++++++++++++++--------------
>   2 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/src/gallium/drivers/r600/r600_pipe.h b/src/gallium/drivers/r600/r600_pipe.h
> index 8485cce..64a90c3 100644
> --- a/src/gallium/drivers/r600/r600_pipe.h
> +++ b/src/gallium/drivers/r600/r600_pipe.h
> @@ -787,8 +787,6 @@ void r600_init_surface_functions(struct r600_context *r600);
>   uint32_t r600_translate_texformat(struct pipe_screen *screen, enum pipe_format format,
>   				  const unsigned char *swizzle_view,
>   				  uint32_t *word4_p, uint32_t *yuv_format_p);
> -unsigned r600_texture_get_offset(struct r600_texture *rtex,
> -					unsigned level, unsigned layer);
>   struct pipe_surface *r600_create_surface_custom(struct pipe_context *pipe,
>   						struct pipe_resource *texture,
>   						const struct pipe_surface *templ,
> diff --git a/src/gallium/drivers/r600/r600_texture.c b/src/gallium/drivers/r600/r600_texture.c
> index 60d8c36..c2feb52 100644
> --- a/src/gallium/drivers/r600/r600_texture.c
> +++ b/src/gallium/drivers/r600/r600_texture.c
> @@ -116,11 +116,15 @@ static void r600_copy_from_staging_texture(struct pipe_context *ctx, struct r600
>   	}
>   }
>   
> -unsigned r600_texture_get_offset(struct r600_texture *rtex,
> -					unsigned level, unsigned layer)
> +static unsigned r600_texture_get_offset(struct r600_texture *rtex, unsigned level,
> +					const struct pipe_box *box)
>   {
> +	enum pipe_format format = rtex->resource.b.b.format;
> +
>   	return rtex->surface.level[level].offset +
> -	       layer * rtex->surface.level[level].slice_size;
> +	       box->z * rtex->surface.level[level].slice_size +
> +	       box->y / util_format_get_blockheight(format) * rtex->surface.level[level].pitch_bytes +
> +	       box->x / util_format_get_blockwidth(format) * util_format_get_blocksize(format);
>   }
>   
>   static int r600_init_surface(struct r600_screen *rscreen,
> @@ -805,7 +809,6 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   	struct r600_texture *rtex = (struct r600_texture*)texture;
>   	struct r600_transfer *trans;
>   	boolean use_staging_texture = FALSE;
> -	enum pipe_format format = texture->format;
>   	struct r600_resource *buf;
>   	unsigned offset = 0;
>   	char *map;
> @@ -849,8 +852,6 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   	trans->transfer.box = *box;
>   
>   	if (rtex->is_depth) {
> -		/* XXX: only readback the rectangle which is being mapped? */
> -		/* XXX: when discard is true, no need to read back from depth texture */
>   		struct r600_texture *staging_depth;
>   
>   		if (rtex->resource.b.b.nr_samples > 1) {
> @@ -861,6 +862,8 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   			 *
>   			 * First downsample the depth buffer to a temporary texture,
>   			 * then decompress the temporary one to staging.
> +			 *
> +			 * Only the region being mapped is transfered.
>   			 */
>   			struct pipe_resource *temp;
>   			struct pipe_resource resource;
> @@ -880,9 +883,10 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   						   0, 0, 0, box->depth, 0, 0);
>   
>   	                pipe_resource_reference((struct pipe_resource**)&temp, NULL);
> -			trans->offset = 0;
>   		}
>   		else {
> +			/* XXX: only readback the rectangle which is being mapped? */
> +			/* XXX: when discard is true, no need to read back from depth texture */
>   			if (!r600_init_flushed_depth_texture(ctx, texture, &staging_depth)) {
>   				R600_ERR("failed to create temporary texture to hold untiled copy\n");
>   				FREE(trans);
> @@ -894,7 +898,7 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   						   box->z, box->z + box->depth - 1,
>   						   0, 0);
>   
> -			trans->offset = r600_texture_get_offset(staging_depth, level, box->z);
> +			offset = r600_texture_get_offset(staging_depth, level, box);
>   		}
>   
>   		trans->transfer.stride = staging_depth->surface.level[level].pitch_bytes;
> @@ -926,9 +930,10 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   			rctx->rings.gfx.flush(rctx, 0);
>   		}
>   	} else {
> +		/* the resource is mapped directly */
>   		trans->transfer.stride = rtex->surface.level[level].pitch_bytes;
>   		trans->transfer.layer_stride = rtex->surface.level[level].slice_size;
> -		trans->offset = r600_texture_get_offset(rtex, level, box->z);
> +		offset = r600_texture_get_offset(rtex, level, box);
>   	}
>   
>   	if (trans->staging) {
> @@ -937,11 +942,6 @@ static void *r600_texture_transfer_map(struct pipe_context *ctx,
>   		buf = &rtex->resource;
>   	}
>   
> -	if (rtex->is_depth || !trans->staging)
> -		offset = trans->offset +
> -			box->y / util_format_get_blockheight(format) * trans->transfer.stride +
> -			box->x / util_format_get_blockwidth(format) * util_format_get_blocksize(format);
> -
>   	if (!(map = r600_buffer_mmap_sync_with_rings(rctx, buf, usage))) {
>   		pipe_resource_reference((struct pipe_resource**)&trans->staging, NULL);
>   		FREE(trans);