[Mesa-dev] [PATCH 1/2] i965: Fix mipmap offsets for HiZ and separate stencil buffers.

[Chad Versace]

A few comments below.

On 04/24/2012 12:23 PM, Paul Berry wrote:
> When rendering to a miplevel other than 0 within a color, depth,
> stencil, or HiZ buffer, we need to tell the GPU to render to an offset
> within the buffer, so that the data is written into the correct
> miplevel.  We do this using a coarse offset (in pages), and a fine
> adjustment (the so-called "tile_x" and "tile_y" values, which are
> measured in pixels).
> 
> We have always computed the coarse offset and fine adjustment using
> intel_renderbuffer_tile_offsets() function.  This worked fine for
> color and combined depth/stencil buffers, but failed to work properly
> when HiZ and separate stencil were in use.  It failed to work because
> there is only one set of fine adjustment controls shared by the HiZ,
> depth, and stencil buffers, so we need to choose tile_x and tile_y
> values that are compatible with the tiling of all three buffers, and
> then compute separate coarse offsets for each buffer.
> 
> This patch fixes the HiZ and separate stencil case by replacing the
> call to intel_renderbuffer_tile_offsets() with calls to two functions:
> intel_region_get_tile_masks(), which determines how much of the
> adjustment can be performed using offsets and how much can be
> performed using tile_x and tile_y, and
> intel_region_get_aligned_offset(), which computes the coarse offset.
> 
> intel_region_get_tile_offsets() is still used for color renderbuffers,
> so to avoid code duplication, I've re-worked it to use
> intel_region_get_tile_masks() and intel_region_get_aligned_offset().
> 
> On i965 Gen6, fixes piglit tests
> "texturing/depthstencil-render-miplevels 1024 X" where X is one of
> (depth, depth_and_stencil, depth_stencil_single_binding, depth_x,
> depth_x_and_stencil, stencil, stencil_and_depth, stencil_and_depth_x).
> 
> On i965 Gen7, the variants of
> "texturing/depthstencil-render-miplevels" that contain a stencil
> buffer still fail, due to another problem: Gen7 seems to ignore the 3
> LSB's of the tile_y adjustment (and possibly also tile_x).
> ---
>  src/mesa/drivers/dri/i965/brw_misc_state.c  |   96 ++++++++++++++++++++++++--
>  src/mesa/drivers/dri/i965/gen6_hiz.c        |   54 +++++++++++----
>  src/mesa/drivers/dri/i965/gen7_hiz.c        |   54 +++++++++++----
>  src/mesa/drivers/dri/i965/gen7_misc_state.c |   79 ++++++++++++++++++++--
>  src/mesa/drivers/dri/intel/intel_fbo.c      |   27 ++-----
>  src/mesa/drivers/dri/intel/intel_fbo.h      |   28 ++++++++
>  src/mesa/drivers/dri/intel/intel_regions.c  |   52 ++++++++++++++
>  src/mesa/drivers/dri/intel/intel_regions.h  |    8 ++
>  8 files changed, 338 insertions(+), 60 deletions(-)

[snip]

> diff --git a/src/mesa/drivers/dri/intel/intel_fbo.h b/src/mesa/drivers/dri/intel/intel_fbo.h
> index 724f141..503b006 100644
> --- a/src/mesa/drivers/dri/intel/intel_fbo.h
> +++ b/src/mesa/drivers/dri/intel/intel_fbo.h
> @@ -153,6 +153,34 @@ intel_flip_renderbuffers(struct gl_framebuffer *fb);
>  void
>  intel_renderbuffer_set_draw_offset(struct intel_renderbuffer *irb);
>  
> +void
> +intel_renderbuffer_fine_offset_masks(struct intel_renderbuffer *irb,
> +                                     uint32_t *fine_offset_mask_x,
> +                                     uint32_t *fine_offset_mask_y);
> +
> +/**
> + * When rendering to a texture with multiple miplevels, depth planes, or cube
> + * faces, we need to instruct the GPU to render to an offset within the
> + * texture image corresponding to the appropriate miplevel/plane/cubeface.  We
> + * do this by a combination of two techniques: by offsetting the base address
> + * of the texture image, and by supplying additional X and Y coordinate
> + * offsets to the GPU in the SURFACE_STATE structure.
> + *
> + * This function computes the additional Y coordinate offset.
> + */
> +
> +/**
> + * When rendering to a texture with multiple miplevels, depth planes, or cube
> + * faces, we need to instruct the GPU to render to an offset within the
> + * texture image corresponding to the appropriate miplevel/plane/cubeface.  We
> + * do this by a combination of two techniques: by offsetting the base address
> + * of the texture image, and by supplying additional X and Y coordinate
> + * offsets to the GPU in the SURFACE_STATE structure.
> + *
> + * This function computes the additional X coordinate offset.
> + */
> +
> +
>  uint32_t
>  intel_renderbuffer_tile_offsets(struct intel_renderbuffer *irb,
>  				uint32_t *tile_x,

Dangling comments above.

[snip]

> +/**
> + * Compute the offset (in bytes) from the start of the region to the given x
> + * and y coordinate.  For tiled regions, caller must ensure that x and y are
> + * multiples of the tile size.
> + */

I think this function needs to assert the preconditions for x and y. It's
just too easy for a caller to forget the precondition and hence introduce
an easily avoided bug.

> +uint32_t
> +intel_region_get_aligned_offset(struct intel_region *region, uint32_t x,
> +                                uint32_t y)
> +{
> +   int cpp = region->cpp;
> +   uint32_t pitch = region->pitch * cpp;
> +
> +   switch (region->tiling) {
> +   default:
> +      assert(false);
> +   case I915_TILING_NONE:
> +      return y * pitch + x * cpp;
> +   case I915_TILING_X:
> +      return y * pitch + x / (512 / cpp) * 4096;
> +   case I915_TILING_Y:
> +      return y * pitch + x / (128 / cpp) * 4096;
> +   }
> +}

One more thing. It would be nice if the duplicate offset code in
gen6_hiz.c and gen7_hiz.c were consolidated into a helper function...
but, meh. I don't care that much.