On Sat, Oct 29, 2016 at 12:47:02AM -0700, Jason Ekstrand wrote:
> On Tue, Oct 11, 2016 at 12:26 PM, Topi Pohjolainen
> <[1]topi.pohjolai...@gmail.com> wrote:
>
> Currently the status bits for fast clear include the flag telling
> if non-multisampled mcs buffer should be used at all. Once the
> state tracking is changed to follow individual levels/layers one
> still needs to have the mcs enabling information in the miptree.
> Therefore simply split it out to its own boolean.
>
> Thank you! The fast clear state bits have been somewhat confused for
> some time. Between disable_aux_buffers, intel_fast_clear_state,
> intel_msaa_layout, and the hiz resolve tracking, we have a bunch of
> different fields that track memory layout, what aux is available, and
> the current state of affairs and some of them are trying to track two
> things at once. I hope to fix that some day but this is definitely a
> step in the right direction.
>
> Possible follow-up work is to combine disable_aux_buffers and
> no_msrt_mcs into single enum.
>
> Yeah, I think that's a good direction. I did something similar in my
> Vulkan CCS series where I had an "aux usage" field in the image that
> provided a sort of "global" picture of the usage.
>
> Signed-off-by: Topi Pohjolainen <[2]topi.pohjolai...@intel.com>
> ---
> src/mesa/drivers/dri/i965/brw_blorp.c | 2 +-
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 14 +++++++++-----
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 12 ++++++------
> 3 files changed, 16 insertions(+), 12 deletions(-)
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> b/src/mesa/drivers/dri/i965/brw_blorp.c
> index c55bbc8..6332788 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> @@ -780,7 +780,7 @@ do_single_blorp_clear(struct brw_context *brw,
> struct gl_framebuffer *fb,
> if (set_write_disables(irb, ctx->Color.ColorMask[buf],
> color_write_disable))
> can_fast_clear = false;
> - if (irb->mt->fast_clear_state == INTEL_FAST_CLEAR_STATE_NO_MCS
> ||
> + if (irb->mt->no_msrt_mcs ||
> !brw_is_color_fast_clear_compatible(brw, irb->mt,
> &ctx->Color.ClearColor))
> can_fast_clear = false;
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index ce72e2c..4fb07e9 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -374,8 +374,9 @@ intel_miptree_create_layout(struct brw_context
> *brw,
> mt->logical_width0 = width0;
> mt->logical_height0 = height0;
> mt->logical_depth0 = depth0;
> - mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_NO_MCS;
> + mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
> mt->disable_aux_buffers = (layout_flags &
> MIPTREE_LAYOUT_DISABLE_AUX) != 0;
> + mt->no_msrt_mcs = true;
> mt->is_scanout = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) !=
> 0;
> exec_list_make_empty(&mt->hiz_map);
> mt->cpp = _mesa_get_format_bytes(format);
> @@ -787,7 +788,7 @@ intel_miptree_create(struct brw_context *brw,
> */
> if (intel_tiling_supports_non_msrt_mcs(brw, mt->tiling) &&
> intel_miptree_supports_non_msrt_fast_clear(brw, mt)) {
> - mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
> + mt->no_msrt_mcs = false;
> assert(brw->gen < 8 || mt->halign == 16 || num_samples <= 1);
> /* On Gen9+ clients are not currently capable of consuming
> compressed
> @@ -1582,6 +1583,7 @@ intel_miptree_alloc_non_msrt_mcs(struct
> brw_context *brw,
> {
> assert(mt->mcs_mt == NULL);
> assert(!mt->disable_aux_buffers);
> + assert(!mt->no_msrt_mcs);
> /* The format of the MCS buffer is opaque to the driver; all
> that matters
> * is that we get its size and pitch right. We'll pretend that
> the format
> @@ -2126,6 +2128,9 @@ intel_miptree_resolve_color(struct brw_context
> *brw,
> unsigned level, unsigned layer,
> int flags)
> {
> + if (mt->no_msrt_mcs)
> + return false;
> +
> intel_miptree_check_color_resolve(mt, level, layer);
> /* From gen9 onwards there is new compression scheme for single
> sampled
> @@ -2137,7 +2142,6 @@ intel_miptree_resolve_color(struct brw_context
> *brw,
> return false;
> switch (mt->fast_clear_state) {
> - case INTEL_FAST_CLEAR_STATE_NO_MCS:
> case INTEL_FAST_CLEAR_STATE_RESOLVED:
> /* No resolve needed */
> return false;
> @@ -2187,7 +2191,7 @@ intel_miptree_make_shareable(struct
> brw_context *brw,
> if (mt->mcs_mt) {
> intel_miptree_all_slices_resolve_color(brw, mt, 0);
> intel_miptree_release(&mt->mcs_mt);
> - mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_NO_MCS;
> + mt->no_msrt_mcs = true;
> }
> }
> @@ -3268,7 +3272,7 @@ intel_miptree_get_aux_isl_surf(struct
> brw_context *brw,
> } else if (intel_miptree_is_lossless_compressed(brw, mt)) {
> assert(brw->gen >= 9);
> *usage = ISL_AUX_USAGE_CCS_E;
> - } else if (mt->fast_clear_state !=
> INTEL_FAST_CLEAR_STATE_NO_MCS) {
> + } else if (!mt->no_msrt_mcs) {
> *usage = ISL_AUX_USAGE_CCS_D;
> } else {
> unreachable("Invalid MCS miptree");
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index bfb8ad5..57d7b80 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -220,12 +220,6 @@ enum intel_msaa_layout
> enum intel_fast_clear_state
> {
> /**
> - * There is no MCS buffer for this miptree, and one should never
> be
> - * allocated.
> - */
> - INTEL_FAST_CLEAR_STATE_NO_MCS,
> -
> - /**
> * No deferred clears are pending for this miptree, and the
> contents of the
> * color buffer are entirely correct. An MCS buffer may or may
> not exist
> * for this miptree. If it does exist, it is entirely in the
> "no deferred
> @@ -676,6 +670,12 @@ struct intel_mipmap_tree
> bool disable_aux_buffers;
> /**
> + * Fast clear and lossless compression are always disabled for
> this
> + * miptree.
> + */
> + bool no_msrt_mcs;
>
> Please choose a different name for this boolean. What you have right
> now unabreviates to "no multisampled render target MCS" but it's for
> single-sampled. The single-sampled color compression surface is
> referred to in the gen8 docs and prior as a non-multisampled MCS which
> we abbreviate various places in the driver as non_msrt_mcs which is 1
> character off the name of this field. When I read this I can't tell if
> you're saying "no non-msrt MCS" or "no-msrt MCS enabled".
> One option would be "no_ccs". I know that the docs overload CCS to
> also include multisampled surfaces. However, in ISL, we chose the
> convention of CCS means single-sampled and MCS means multisampled. I'm
> also open to other names.
To me "no_ccs" sounds pretty good - that is exactly what the comment says.
Changed locally.
>
> +
> + /**
> * Tells if the underlying buffer is to be also consumed by
> entities other
> * than the driver. This allows logic to turn off features such
> as lossless
> * compression which is not currently understood by client
> applications.
> --
> 2.5.5
> _______________________________________________
> mesa-dev mailing list
> [3]mesa-dev@lists.freedesktop.org
> [4]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> References
>
> 1. mailto:topi.pohjolai...@gmail.com
> 2. mailto:topi.pohjolai...@intel.com
> 3. mailto:mesa-dev@lists.freedesktop.org
> 4. https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
