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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
