[Mesa-dev] [v4 02/11] i965: Relax assertion of halign == 16 for lossless compressed aux

[Pohjolainen, Topi]

On Thu, May 05, 2016 at 10:59:16AM -0700, Ben Widawsky wrote:
> On Thu, May 05, 2016 at 10:51:32AM -0700, Ben Widawsky wrote:
> > On Thu, Apr 21, 2016 at 02:58:57PM +0300, Topi Pohjolainen wrote:
> > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index ae08300..b68575f 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -556,8 +556,13 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >     } else if (brw->gen >= 9 && num_samples > 1) {
> > >        layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
> > >     } else {
> > > +      const bool is_lossless_compressed_aux =
> > > +         brw->gen >= 9 && num_samples == 1 &&
> > > +         mt->format == MESA_FORMAT_R_UINT32;
> > > +
> > >        /* For now, nothing else has this requirement */
> > > -      assert((layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16) == 0);
> > > +      assert(is_lossless_compressed_aux ||
> > > +             (layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16) == 0);
> > >     }
> > >  
> > >     brw_miptree_layout(brw, mt, layout_flags);
> > 
> > I'd just drop the else condition entirely instead. The else case assertion also
> > becomes invalid when we want to fast clear multi LOD and arrayed surfaces. I'm
> > slightly worried that the assertion is somewhat frail and not future proof.
> > 
> > Also the format check makes me wonder if you want to add some assertion or
> > condition to intel_miptree_is_lossless_compressed? ie.
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 17c87a2..467a60b 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -296,6 +296,9 @@ intel_miptree_is_lossless_compressed(const struct brw_context *brw,
> >     if (mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)
> >        return false;
> >  
> > +   if (mt->format == MESA_FORMAT_R_UINT32)
> > +      return false;
> > +
> >     /* And finally distinguish between msaa and single sample case. */
> >     return mt->num_samples <= 1;
> >  }
> > 
> > Either way:
> > Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> 
> Hmm. On second thought, doesn't this still violate what the spec says?
> "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN 16 must be
> used."

I'm not sure if I understood this question correctly. The rational for the
patch is that intel_miptree_alloc_non_msrt_mcs() calls miptree creation
for the aux the buffer with MIPTREE_LAYOUT_FORCE_HALIGN16. This ends up into
the branch addressed above. Format in question in MESA_FORMAT_R_UINT32 for
which neither of the first two conditions apply:

   if (intel_miptree_supports_non_msrt_fast_clear(brw, mt)) {
      ...
   } else if (brw->gen >= 9 && num_samples > 1) {
      ...
   } else {

and hence it drops to the default where the assertion hits unless it is
relaxed.