[Mesa-dev] [PATCH 3/6] i965: Extract tiling from fast clear decision

[Ben Widawsky]

On Fri, May 29, 2015 at 12:27:17PM -0700, Chad Versace wrote:
> On Thu 28 May 2015, Ben Widawsky wrote:
> > There are several constraints when determining if one can fast clear a surface.
> > Some of these are alignment, pixel density, tiling formats, and others that vary
> > by generation. The helper function which exists today does a suitable job,
> > however it conflates "BO properties" with "Miptree properties" when using
> > tiling. I consider the former to be attributes of the physical surface, things
> > which are determined through BO allocation, and the latter being attributes
> > which are derived from the API, and having nothing to do with the underlying
> > surface.
> > 
> > Tiling properties are a distinct operation from the creation of a miptree, and
> > by removing this, we gain flexibility throughout the code to make determinations
> > about when we can or cannot fast clear strictly on the miptree.
> 
> I don't understand the above sentence. "Tiling properties" cannot be
> a distinct operation, because "tiling properties" is not an operation at
> all. I'm not being a grammar jerk. I'm just dense and don't understand
> this paragraph's claims.
> 

I think I meant to add "Determining" in the front. But now I'd like to modify it
further. How about:

Determining tiling properties and creating miptrees are related operations 
(when we allocate a BO for a miptree) with some disjoint constraints. By 
extracting the decisions into two distinct choices (tiling vs. miptree 
properties), we gain flexibility throughout the code to make determinations
about when we can or cannot fast clear strictly on the miptree.

> > To signify this change, I've also renamed the function to indicate it is a
> > distinction made on the miptree. I am torn as to whether or not it was a good
> > idea to remove "non_msrt" since it's a really nice thing for grep.
> > 
> > Cc: Chad Versace <chad.versace at linux.intel.com>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 37 +++++++++++++++++++--------
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 11 ++++----
> >  2 files changed, 32 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 68d405c..75ee19a 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -158,15 +158,33 @@ intel_get_non_msrt_mcs_alignment(struct brw_context *brw,
> >     }
> >  }
> >  
> > +/**
> > + * Determine the BO backing the miptree has a suitable tile format. For Gen7,
> > + * and 8 this means any tiled format.
> 
> Hmm... "a suitable tile format" for what? How about this rephrasing?
> 
>     "Does the hardware support non-MSRT for this tiling format"
> 

On further look, I think the function documentation/comment is totally redundant.
How about I just move the PRM citation into the function definition and delete
the "Determine..."

> > + *
> > + * From the Ivy Bridge PRM, Vol2 Part1 11.7 "MCS Buffer for Render Target(s)",
> > + * beneath the "Fast Color Clear" bullet (p326):
> > + *
> > + *     - Support is limited to tiled render targets.
> > + */
> > +bool
> > +intel_is_non_msrt_mcs_tile_supported(struct brw_context *brw,
> > +                                     unsigned tiling)
> 
> The function name is misleading. It hints that there exists such a thing
> as an "MCS tile". Perhaps rename it to
> intel_tiling_supports_non_msrt_mcs?
> 

Agreed, it fits on one line now too. Further reinforces my above statement :-)

> > +{
> > +   if (brw->gen >= 9)
> > +      return tiling == I915_TILING_Y;
> > +
> > +   return tiling != I915_TILING_NONE;
> 
> To be complete, the if-ladder should be...
> 
>     if (brw->gen >= 9) {
>         return tiling == I915_TILIING_Y;
>     } else if (brw->gen >= 7) {
>         return tiling != I915_TILING_NONE;
>     } else {
>         return false;
>     }
> 
> ...because gen6 and below do not support MCS (iirc).
> 
> > +}
> >  
> >  /**
> >   * For a single-sampled render target ("non-MSRT"), determine if an MCS buffer
> > - * can be used.
> > + * can be used. This doesn't (and should not) inquire about the BO properties of
> > + * the buffer.
> 
> The comment stutters and confuses me, because acronym expansion results
> in:
> 
>     This doesn't (and should not) inquire about the buffer object
>     properties of the buffer.
> 
> Which is equivalent to:
> 
>     This doesn't (and should not) inquire about the buffer's buffer
>     object properties.
> 
> Instead of referring the "buffer's buffer object properties", I think
> it's much clearer to refer to the "properties of the miptree's BO":
> 
>     This doesn't (and should not) inspect any properties of the
>     miptree's BO.
> 
> 
> Other than the naming and documentation nitpicks, the code itself looks
> good to me. I like the clean separation the patch introduces.

All the other suggestions work for me.