On Wed, Apr 25, 2018 at 02:53:26PM -0700, Nanley Chery wrote:
> On Wed, Apr 25, 2018 at 02:26:18PM -0700, Rafael Antognolli wrote:
> > On Tue, Apr 24, 2018 at 05:48:45PM -0700, Nanley Chery wrote:
> > > Determine the predicate for updating the indirect depth value in the
> > > loop which inspects whether or not we need to resolve any slices.
> > > ---
> > > src/mesa/drivers/dri/i965/brw_clear.c | 43
> > > +++++++++++++----------------------
> > > 1 file changed, 16 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
> > > b/src/mesa/drivers/dri/i965/brw_clear.c
> > > index 6521141d7f6..e372d28926e 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_clear.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_clear.c
> > > @@ -108,7 +108,6 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > > struct intel_mipmap_tree *mt = depth_irb->mt;
> > > struct gl_renderbuffer_attachment *depth_att =
> > > &fb->Attachment[BUFFER_DEPTH];
> > > const struct gen_device_info *devinfo = &brw->screen->devinfo;
> > > - bool same_clear_value = true;
> > >
> > > if (devinfo->gen < 6)
> > > return false;
> > > @@ -174,9 +173,16 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > > const uint32_t num_layers = depth_att->Layered ?
> > > depth_irb->layer_count : 1;
> > >
> > > /* If we're clearing to a new clear value, then we need to resolve
> > > any clear
> > > - * flags out of the HiZ buffer into the real depth buffer.
> > > + * flags out of the HiZ buffer into the real depth buffer and update
> > > the
> > > + * miptree's clear value.
> > > */
> >
> > I got confused by this comment here. I think your addition to the
> > comment is fine, but the original one wasn't very descriptive of what's
> > going on (at least it wasn't obvious to me).
> >
> > Since you are already changing it, maybe we can improve it to something
> > like:
> >
> > /* If we are clearing to a new clear value, the levels/layers being
> > * cleared don't need resolving because they will stay in the clear
> > * state, and only the miptree's clear vale needs updating. However, if
> > * some levels/layers were already in a clear state, but are not being
> > * cleared now, and the clear value is changing, then we need to resolve
> > * their clear flags out of the HiZ buffer into the real depth buffer.
> > */
> >
>
> I see. The original comment does fail to mention that we don't resolve
> the level/layer range being cleared.
>
> > I'm not sure if this actually helps or if it just makes the comment
> > unnecessarily complex.
> >
>
> I think we can fix this while keeping the comment simple. What do you
> think about one of these:
>
> /* If we're clearing to a new clear value, then we need to resolve
> * any clear flags that are outside of the specified range and then
> * update the miptree's clear value.
> */
>
> /* If we're clearing to a new clear value, then we need to resolve
> * any clear flags that are outside of the level/layer range
> * specified for clearing and then update the miptree's clear value.
> */
Ah, excelent, either of those options are great imho! Choose whatever
you want.
Rafael
> > On a second thought, this doesn't need to be changed in this commit if
> > you don't want to. We can just send a new one later clarifying these
> > points, and we could also update the comment where the resolve happens
> > to clarify that it should only happen to layers not being cleared now.
> >
> > In any case, this patch is a nice cleanup.
> >
> > Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>
> >
>
> Thanks!
>
> -Nanley
>
> > > if (mt->fast_clear_color.f32[0] != clear_value) {
> > > + /* BLORP updates the indirect clear color buffer when we do fast
> > > clears.
> > > + * If we won't do a fast clear, we'll have to update it ourselves.
> > > Start
> > > + * off assuming we won't perform a fast clear.
> > > + */
> > > + bool blorp_will_update_indirect_color = false;
> > > +
> > > for (uint32_t level = mt->first_level; level <= mt->last_level;
> > > level++) {
> > > if (!intel_miptree_level_has_hiz(mt, level))
> > > continue;
> > > @@ -184,16 +190,20 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > > const unsigned level_layers = brw_get_num_logical_layers(mt,
> > > level);
> > >
> > > for (uint32_t layer = 0; layer < level_layers; layer++) {
> > > + const enum isl_aux_state aux_state =
> > > + intel_miptree_get_aux_state(mt, level, layer);
> > > +
> > > if (level == depth_irb->mt_level &&
> > > layer >= depth_irb->mt_layer &&
> > > layer < depth_irb->mt_layer + num_layers) {
> > > +
> > > + if (aux_state != ISL_AUX_STATE_CLEAR)
> > > + blorp_will_update_indirect_color = true;
> > > +
> > > /* We're going to clear this layer anyway. Leave it
> > > alone. */
> > > continue;
> > > }
> > >
> > > - enum isl_aux_state aux_state =
> > > - intel_miptree_get_aux_state(mt, level, layer);
> > > -
> > > if (aux_state != ISL_AUX_STATE_CLEAR &&
> > > aux_state != ISL_AUX_STATE_COMPRESSED_CLEAR) {
> > > /* This slice doesn't have any fast-cleared bits. */
> > > @@ -214,29 +224,8 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > > }
> > >
> > > intel_miptree_set_depth_clear_value(brw, mt, clear_value);
> > > - same_clear_value = false;
> > > - }
> > > -
> > > - bool need_clear = false;
> > > - for (unsigned a = 0; a < num_layers; a++) {
> > > - enum isl_aux_state aux_state =
> > > - intel_miptree_get_aux_state(mt, depth_irb->mt_level,
> > > - depth_irb->mt_layer + a);
> > > -
> > > - if (aux_state != ISL_AUX_STATE_CLEAR) {
> > > - need_clear = true;
> > > - break;
> > > - }
> > > - }
> > > -
> > > - if (!need_clear) {
> > > - if (!same_clear_value) {
> > > - /* BLORP updates the indirect clear color buffer when
> > > performing a
> > > - * fast clear. Since we are skipping the fast clear here, we
> > > need to
> > > - * do the update ourselves.
> > > - */
> > > + if (!blorp_will_update_indirect_color)
> > > intel_miptree_update_indirect_color(brw, mt);
> > > - }
> > > }
> > >
> > > for (unsigned a = 0; a < num_layers; a++) {
> > > --
> > > 2.16.2
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > 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
