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

[Ben Widawsky]

On Fri, May 06, 2016 at 10:19:58AM +0300, Pohjolainen, Topi wrote:
> 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.
> 

Forgive me, I misread the assertion actually. You already have my r-b whether or
not you drop the else entirely...

-- 
Ben Widawsky, Intel Open Source Technology Center