[Mesa-dev] [PATCH 4/6] i965/gen8: Correct HALIGN for AUX surfaces

[Ben Widawsky]

On Fri, May 29, 2015 at 05:21:14PM -0700, Chad Versace wrote:
> On Fri 29 May 2015, Chad Versace wrote:
> > On Thu 28 May 2015, Ben Widawsky wrote:
> > > This restriction was attempted in this commit:
> > > commit 47053464630888f819ef8cc44278f1a1220159b9
> > > Author: Anuj Phogat <anuj.phogat at gmail.com>
> > > Date:   Fri Feb 13 11:21:21 2015 -0800
> > > 
> > >    i965/gen8: Use HALIGN_16 if MCS is enabled for non-MSRT
> > > 
> > > However, the commit itself doesn't achieve the desired goal as determined by the
> > > asserts which the next patch adds. mcs_mt is never a value because we're in the
> > > process of allocating the mcs_mt miptree when we get to this function. I didn't
> > > check, but perhaps this would work with blorp, however, meta clears allocate the
> > > miptree structure (which AFAICT needs the alignment also) way before it
> > > allocates using meta clears where the renderbuffer is allocated way before the
> > > aux buffer.
> > > 
> > > The restriction is referenced in a few places, but the most concise one [IMO]
> > > from the spec is for Gen9. Gen8 8 loosens the restriction in that it only
> > > requires this for non-msrt surface.
> > > 
> > >    When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN 16 must
> > >    be used.
> > > 
> > > With the code before the miptree layout flag rework (patches preceding this),
> > > accomplishing this workaround is very difficult.
> > > 
> > > Cc: Anuj Phogat <anuj.phogat at gmail.com>
> > > Cc: Neil Roberts <neil at linux.intel.com>
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_tex_layout.c    | 16 ++++++++++------
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 ++++++++++++---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  4 +++-
> > >  3 files changed, 25 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > > index 72b02a2..b51c7c7 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > > @@ -41,8 +41,13 @@
> > >  
> > >  static unsigned int
> > >  intel_horizontal_texture_alignment_unit(struct brw_context *brw,
> > > -                                        struct intel_mipmap_tree *mt)
> > > +                                        struct intel_mipmap_tree *mt,
> > > +                                        uint32_t layout_flags)
> > >  {
> > > +
> > > +   if (layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16)
> > > +      return 16;
> > > +
> > 
> > This snippet makes sense. If the force flag is set, then don't bother
> > inspecting any other state.
> > 
> > >     /**
> > >      * From the "Alignment Unit Size" section of various specs, namely:
> > >      * - Gen3 Spec: "Memory Data Formats" Volume,         Section 1.20.1.4
> > > @@ -91,9 +96,6 @@ intel_horizontal_texture_alignment_unit(struct brw_context *brw,
> > >     if (brw->gen >= 7 && mt->format == MESA_FORMAT_Z_UNORM16)
> > >        return 8;
> > >  
> > > -   if (brw->gen == 8 && mt->mcs_mt && mt->num_samples <= 1)
> > > -      return 16;
> > > -
> > >     return 4;
> > >  }
> > >  
> > > @@ -459,7 +461,8 @@ brw_miptree_layout_texture_3d(struct brw_context *brw,
> > >  }
> > >  
> > >  void
> > > -brw_miptree_layout(struct brw_context *brw, struct intel_mipmap_tree *mt)
> > > +brw_miptree_layout(struct brw_context *brw, struct intel_mipmap_tree *mt,
> > > +                   uint32_t layout_flags)
> > >  {
> > >     bool multisampled = mt->num_samples > 1;
> > >     bool gen6_hiz_or_stencil = false;
> > > @@ -492,8 +495,9 @@ brw_miptree_layout(struct brw_context *brw, struct intel_mipmap_tree *mt)
> > >           mt->align_w = 128 / mt->cpp;
> > >           mt->align_h = 32;
> > >        }
> > > +      assert((layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16) == 0);
> > 
> > This assertion makes sense. 
> > 
> > >     } else {
> > > -      mt->align_w = intel_horizontal_texture_alignment_unit(brw, mt);
> > > +      mt->align_w = intel_horizontal_texture_alignment_unit(brw, mt, layout_flags);
> > >        mt->align_h =
> > >           intel_vertical_texture_alignment_unit(brw, mt->format, multisampled);
> > >     }
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 75ee19a..a1ac0cf 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -475,7 +475,10 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >     if (layout_flags & MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD)
> > >        mt->array_layout = ALL_SLICES_AT_EACH_LOD;
> > >  
> > > -   brw_miptree_layout(brw, mt);
> > > +   if (intel_miptree_is_fast_clear_capable(brw, mt))
> > > +      layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
> > > +
> > > +   brw_miptree_layout(brw, mt, layout_flags);
> > 
> > This does not make sense to me. I though HALIGN16 didn't exist before
> > Skylake, but here you're setting it on Ivybridge through Broadwell too.
> > Am I misunderstanding something?
> 
> I did some homewwork. HALIGN16 was introduced in Broadwell and does not
> exist in Haswell. So this code still looks wrong to me.

Yeah, you are right. It's accidentally correct because the only buffers which
need HALIGN8 are depth/stencil, which aren't fast clear capable. I'll fix this.
(I was trying to figure out why I had no regressions with this bug).