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.vers...@linux.intel.com>
> Signed-off-by: Ben Widawsky <b...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
