<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 11, 2016 at 12:26 PM, Topi Pohjolainen <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Possible follow-up work is to combine disable_aux_buffers and no_msrt_mcs into single enum. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> --- 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; </blockquote><div> </div><div>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". </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + /** * 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>