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

[Chad Versace]

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.

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

> + *
> + * 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?

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