On Thu, Nov 24, 2016 at 09:34:37AM +0200, Pohjolainen, Topi wrote:
> On Wed, Nov 23, 2016 at 01:05:31PM -0800, Jason Ekstrand wrote:
> > On Wed, Nov 23, 2016 at 1:16 AM, Topi Pohjolainen
> > <[1]topi.pohjolai...@gmail.com> wrote:
> >
> > v2: Added intel_resolve_map_clear() into intel_miptree_release()
> > Signed-off-by: Topi Pohjolainen <[2]topi.pohjolai...@intel.com>
> > Reviewed-by: Jason Ekstrand <[3]ja...@jlekstrand.net> (v1)
> > ---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 64
> > +++++++++++++++++++--------
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 14 +++---
> > 2 files changed, 53 insertions(+), 25 deletions(-)
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 3895b2e..a4a7ee0 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -397,11 +397,11 @@ 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_RESOLVED;
> > mt->disable_aux_buffers = (layout_flags &
> > MIPTREE_LAYOUT_DISABLE_AUX) != 0;
> > mt->no_ccs = true;
> > mt->is_scanout = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) !=
> > 0;
> > exec_list_make_empty(&mt->hiz_map);
> > + exec_list_make_empty(&mt->color_resolve_map);
> > mt->cpp = _mesa_get_format_bytes(format);
> > mt->num_samples = num_samples;
> > mt->compressed = _mesa_is_format_compressed(format);
> > @@ -933,7 +933,7 @@ intel_update_winsys_renderbuffer_miptree(struct
> > brw_context *intel,
> > */
> > if (intel_tiling_supports_non_msrt_mcs(intel,
> > singlesample_mt->tiling) &&
> > intel_miptree_supports_non_msrt_fast_clear(intel,
> > singlesample_mt)) {
> > - singlesample_mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_
> > RESOLVED;
> > + singlesample_mt->no_ccs = false;
> > }
> > if (num_samples == 0) {
> > @@ -1048,6 +1048,7 @@ intel_miptree_release(struct intel_mipmap_tree
> > **mt)
> > free((*mt)->mcs_buf);
> > }
> > intel_resolve_map_clear(&(*mt)->hiz_map);
> > + intel_resolve_map_clear(&(*mt)->color_resolve_map);
> > intel_miptree_release(&(*mt)->plane[0]);
> > intel_miptree_release(&(*mt)->plane[1]);
> > @@ -1633,7 +1634,12 @@ intel_miptree_alloc_mcs(struct brw_context
> > *brw,
> > return false;
> > intel_miptree_init_mcs(brw, mt, 0xFF);
> > - mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_CLEAR;
> > +
> > + /* Multisampled miptrees are only supported for single level. */
> > + assert(mt->first_level == 0);
> > + intel_miptree_set_fast_clear_state(mt, mt->first_level, 0,
> > + mt->logical_depth0,
> > +
> > INTEL_FAST_CLEAR_STATE_CLEAR);
> > return true;
> > }
> > @@ -1713,7 +1719,6 @@ intel_miptree_alloc_non_msrt_mcs(struct
> > brw_context *brw,
> > * Software needs to initialize MCS with zeros."
> > */
> > intel_miptree_init_mcs(brw, mt, 0);
> > - mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
> > mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
> > }
> > @@ -2209,7 +2214,15 @@ enum intel_fast_clear_state
> > intel_miptree_get_fast_clear_state(const struct intel_mipmap_tree
> > *mt,
> > unsigned level, unsigned layer)
> > {
> > - return mt->fast_clear_state;
> > + intel_miptree_check_level_layer(mt, level, layer);
> > +
> > + const struct intel_resolve_map *item =
> > + intel_resolve_map_const_get(&mt->color_resolve_map, level,
> > layer);
> > +
> > + if (!item)
> > + return INTEL_FAST_CLEAR_STATE_RESOLVED;
> > +
> > + return item->fast_clear_state;
> > }
> > static void
> > @@ -2244,7 +2257,9 @@ intel_miptree_set_fast_clear_state(struct
> > intel_mipmap_tree *mt,
> > assert(first_layer + num_layers <= mt->physical_depth0);
> > - mt->fast_clear_state = new_state;
> > + while (num_layers--)
> >
> > nitpick: I think I'd rather a simple "for (unsigned i = 0; i <
> > num_layers; i++)" It's a bit more obvious
>
> Agreed.
>
> >
> > + intel_resolve_map_set(&mt->color_resolve_map, level,
> > + first_layer++, new_state);
> >
> > We probably want to assert here that state != RESOLVED (see below). If
> > that's not valid, we would have to switch on whether or not it's
> > RESOLVED and if it is RESOLVED, walk the list and delete things.
>
> Color resolver needs to be able to set the state as well. So we really need
> to start deleting items from the list. This worked before just because I had
> logic elsewhere checking also for the RESOLVED. I'm going to drop those and
> turn them into asserts as you proposed.
In fact intel_miptree_resolve_color() already does the list removal. What was
misleading is that brw_blorp_resolve_color() also continued calling and
setting the state to RESOLVED (which intel_miptree_resolve_color() simply
removed just after). I took the call from brw_blorp_resolve_color() away and
added the assert here against setting the state to RESOLVED.
>
> >
> > }
> > bool
> > @@ -2252,7 +2267,9 @@ intel_miptree_has_color_unresolved(const
> > struct intel_mipmap_tree *mt,
> > unsigned start_level, unsigned
> > num_levels,
> > unsigned start_layer, unsigned
> > num_layers)
> > {
> > - return mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_RESOLVED;
> > + return intel_resolve_map_find_any(&mt->color_resolve_map,
> > + start_level, num_levels,
> > + start_layer, num_layers) !=
> > NULL;
> >
> > This assumes that there are no items in the resolve map with a state of
> > RESOLVED. That's a fine assumption, but we need to make sure that it's
> > valid. I've made a bunch of comments all of this patch about that.
>
> And you really found a bug here. I guess this wan't visible before because
> I only used this for sanity check (which was just too loose). Now that I
> fixed one of the patches to use this more widely things would have broken
> down.
And this actually did work before, list was getting empty by
intel_miptree_resolve_color() removing the items.
>
> >
> > }
> > void
> > @@ -2316,17 +2333,18 @@ intel_miptree_resolve_color(struct
> > brw_context *brw,
> > if (!intel_miptree_needs_color_resolve(brw, mt, flags))
> > return false;
> > - switch (mt->fast_clear_state) {
> > - case INTEL_FAST_CLEAR_STATE_RESOLVED:
> > - /* No resolve needed */
> > + intel_miptree_check_level_layer(mt, level, layer);
> > +
> > + struct intel_resolve_map *item =
> > + intel_resolve_map_get(&mt->color_resolve_map, level, layer);
> > +
> > + if (!item || item->fast_clear_state == INTEL_FAST_CLEAR_STATE_
> > RESOLVED)
> >
> > This seems to imply that it's not. Maybe just do
> > if (!item)
> > return false;
> > assert(item->fast_clear_state != INTEL_FAST_CLEAR_STATE_RESOLVED)
> >
> > return false;
> > - case INTEL_FAST_CLEAR_STATE_UNRESOLVED:
> > - case INTEL_FAST_CLEAR_STATE_CLEAR:
> > - brw_blorp_resolve_color(brw, mt, level, layer);
> > - return true;
> > - default:
> > - unreachable("Invalid fast clear state");
> > - }
> > +
> > + brw_blorp_resolve_color(brw, mt, level, layer);
> > + intel_resolve_map_remove(item);
> > +
> > + return true;
> > }
> > void
> > @@ -2334,7 +2352,17 @@ intel_miptree_all_slices_resolve_color(struct
> > brw_context *brw,
> > struct intel_mipmap_tree
> > *mt,
> > int flags)
> > {
> > - intel_miptree_resolve_color(brw, mt, 0, 0, flags);
> > + if (!intel_miptree_needs_color_resolve(brw, mt, flags))
> > + return;
> > +
> > + foreach_list_typed_safe(struct intel_resolve_map, map, link,
> > + &mt->color_resolve_map) {
> > + if (map->fast_clear_state == INTEL_FAST_CLEAR_STATE_RESOLVED)
> >
> > Again, this should be an assert.
> >
> > + continue;
> > +
> > + brw_blorp_resolve_color(brw, mt, map->level, map->layer);
> > + intel_resolve_map_remove(map);
> > + }
> > }
> > /**
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > index 1f4ae6c..51ab664 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > @@ -554,15 +554,20 @@ struct intel_mipmap_tree
> > struct intel_miptree_hiz_buffer *hiz_buf;
> > /**
> > - * \brief Map of miptree slices to needed resolves.
> > + * \brief Maps of miptree slices to needed resolves.
> > *
> > - * This is used only when the miptree has a child HiZ miptree.
> > + * hiz_map is used only when the miptree has a child HiZ
> > miptree.
> > *
> > * Let \c mt be a depth miptree with HiZ enabled. Then the
> > resolve map is
> > * \c mt->hiz_map. The resolve map of the child HiZ miptree, \c
> > * mt->hiz_mt->hiz_map, is unused.
> > + *
> > + *
> > + * color_resolve_map is used only when the miptree uses fast
> > clear (Gen7+)
> > + * lossless compression (Gen9+).
> > */
> > struct exec_list hiz_map; /* List of intel_resolve_map. */
> > + struct exec_list color_resolve_map; /* List of
> > intel_resolve_map. */
> > /**
> > * \brief Stencil miptree for depthstencil textures.
> > @@ -606,11 +611,6 @@ struct intel_mipmap_tree
> > struct intel_mipmap_tree *plane[2];
> > /**
> > - * Fast clear state for this buffer.
> > - */
> > - enum intel_fast_clear_state fast_clear_state;
> > -
> > - /**
> > * The SURFACE_STATE bits associated with the last fast color
> > clear to this
> > * color mipmap tree, if any.
> > *
> > --
> > 2.5.5
> > _______________________________________________
> > mesa-dev mailing list
> > [4]mesa-dev@lists.freedesktop.org
> > [5]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> > References
> >
> > 1. mailto:topi.pohjolai...@gmail.com
> > 2. mailto:topi.pohjolai...@intel.com
> > 3. mailto:ja...@jlekstrand.net
> > 4. mailto:mesa-dev@lists.freedesktop.org
> > 5. 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
