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

[Paul Berry]

On 26 April 2012 16:40, Chad Versace <chad.versace at linux.intel.com> wrote:

> 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.
>

Oh, yeah.  Those were left over from an old draft of the code.  Removed.


>
> [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.
>

Good point.


>
> > +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.
>

Yeah, good point.  This is somewhat improved after all of the refactoring I
did for MSAA.  Tell you what, once the dust settles from that, let's have
another look and see if it's still worth consolidating more of the code.

Updated patch to follow.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120502/6455df42/attachment-0001.html>