Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Nanley Chery]

On Thu, Apr 26, 2018 at 11:27:00AM -0700, Jason Ekstrand wrote:
> On Thu, Apr 26, 2018 at 11:19 AM, Rafael Antognolli <
> rafael.antogno...@intel.com> wrote:
> 
> > On Thu, Apr 26, 2018 at 10:41:37AM -0700, Nanley Chery wrote:
> > > On Wed, Apr 25, 2018 at 08:53:36PM -0400, Jason Ekstrand wrote:
> > > >
> > > >
> > > > On April 25, 2018 20:25:16 Nanley Chery  wrote:
> > > >
> > > > On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
> > > > On Tue, Apr 24, 2018 at 5:48 PM, Nanley Chery 
> > wrote:
> > > >
> > >
> > > Your email client dropped the quotes :/ Thankfully, gmail can pick out
> > > the diff.
> > >
> > > > 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 =
> > > > >Attachment[BUFFER_DEPTH];
> > > > const struct gen_device_info *devinfo = >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.
> > > > */
> > > > 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;
> > > >
> > > > This boolean is rather awkward.
> > > >
> > > > Why's that?
> > > >
> > > > It does have a clear meaning and it does what it says it does.
> > However,
> > > > it's not that obvious of a thing to work with compared to "did we do a
> > > > clear?"
> > > >
> > > >
> > > > +
> > > > 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;
> > > >
> > > > Putting this here separates the detection of whether or not we are
> > doing a
> > > > fast clear (and therefore don't need to set the clear color) even
> > further
> > > > from where we do the clear and use this value than it was previously.
> > > >
> > > >
> > >
> > > Sure.
> > >
> > > > +
> > > > /* 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;
> > > > -  }

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Jason Ekstrand]

On Thu, Apr 26, 2018 at 11:19 AM, Rafael Antognolli <
rafael.antogno...@intel.com> wrote:

> On Thu, Apr 26, 2018 at 10:41:37AM -0700, Nanley Chery wrote:
> > On Wed, Apr 25, 2018 at 08:53:36PM -0400, Jason Ekstrand wrote:
> > >
> > >
> > > On April 25, 2018 20:25:16 Nanley Chery  wrote:
> > >
> > > On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
> > > On Tue, Apr 24, 2018 at 5:48 PM, Nanley Chery 
> wrote:
> > >
> >
> > Your email client dropped the quotes :/ Thankfully, gmail can pick out
> > the diff.
> >
> > > 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 =
> > > >Attachment[BUFFER_DEPTH];
> > > const struct gen_device_info *devinfo = >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.
> > > */
> > > 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;
> > >
> > > This boolean is rather awkward.
> > >
> > > Why's that?
> > >
> > > It does have a clear meaning and it does what it says it does.
> However,
> > > it's not that obvious of a thing to work with compared to "did we do a
> > > clear?"
> > >
> > >
> > > +
> > > 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;
> > >
> > > Putting this here separates the detection of whether or not we are
> doing a
> > > fast clear (and therefore don't need to set the clear color) even
> further
> > > from where we do the clear and use this value than it was previously.
> > >
> > >
> >
> > Sure.
> >
> > > +
> > > /* 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.
> > > - 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Rafael Antognolli]

On Thu, Apr 26, 2018 at 10:41:37AM -0700, Nanley Chery wrote:
> On Wed, Apr 25, 2018 at 08:53:36PM -0400, Jason Ekstrand wrote:
> > 
> > 
> > On April 25, 2018 20:25:16 Nanley Chery  wrote:
> > 
> > On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
> > On Tue, Apr 24, 2018 at 5:48 PM, Nanley Chery  wrote:
> > 
> 
> Your email client dropped the quotes :/ Thankfully, gmail can pick out
> the diff.
> 
> > 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 =
> > >Attachment[BUFFER_DEPTH];
> > const struct gen_device_info *devinfo = >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.
> > */
> > 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;
> > 
> > This boolean is rather awkward.
> > 
> > Why's that?
> > 
> > It does have a clear meaning and it does what it says it does.  However,
> > it's not that obvious of a thing to work with compared to "did we do a
> > clear?"
> > 
> > 
> > +
> > 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;
> > 
> > Putting this here separates the detection of whether or not we are doing a
> > fast clear (and therefore don't need to set the clear color) even further
> > from where we do the clear and use this value than it was previously.
> > 
> > 
> 
> Sure.
> 
> > +
> > /* 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);
> > -  }
> > 
> > I think we can do this even better.  We could do
> > 
> > bool blorp_updated_indirect_clear_color = false;
> > 
> > and then set it to true if we call intel_hiz_exec below.  Then, after the
> > loop below we would 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Nanley Chery]

+ Rafael

On Thu, Apr 26, 2018 at 10:41:37AM -0700, Nanley Chery wrote:
> On Wed, Apr 25, 2018 at 08:53:36PM -0400, Jason Ekstrand wrote:
> > 
> > 
> > On April 25, 2018 20:25:16 Nanley Chery  wrote:
> > 
> > On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
> > On Tue, Apr 24, 2018 at 5:48 PM, Nanley Chery  wrote:
> > 
> 
> Your email client dropped the quotes :/ Thankfully, gmail can pick out
> the diff.
> 
> > 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 =
> > >Attachment[BUFFER_DEPTH];
> > const struct gen_device_info *devinfo = >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.
> > */
> > 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;
> > 
> > This boolean is rather awkward.
> > 
> > Why's that?
> > 
> > It does have a clear meaning and it does what it says it does.  However,
> > it's not that obvious of a thing to work with compared to "did we do a
> > clear?"
> > 
> > 
> > +
> > 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;
> > 
> > Putting this here separates the detection of whether or not we are doing a
> > fast clear (and therefore don't need to set the clear color) even further
> > from where we do the clear and use this value than it was previously.
> > 
> > 
> 
> Sure.
> 
> > +
> > /* 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);
> > -  }
> > 
> > I think we can do this even better.  We could do
> > 
> > bool blorp_updated_indirect_clear_color = false;
> > 
> > and then set it to true if we call intel_hiz_exec below.  Then, after the
> > loop 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Nanley Chery]

On Wed, Apr 25, 2018 at 08:53:36PM -0400, Jason Ekstrand wrote:
> 
> 
> On April 25, 2018 20:25:16 Nanley Chery  wrote:
> 
> On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
> On Tue, Apr 24, 2018 at 5:48 PM, Nanley Chery  wrote:
> 

Your email client dropped the quotes :/ Thankfully, gmail can pick out
the diff.

> 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 =
> >Attachment[BUFFER_DEPTH];
> const struct gen_device_info *devinfo = >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.
> */
> 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;
> 
> This boolean is rather awkward.
> 
> Why's that?
> 
> It does have a clear meaning and it does what it says it does.  However,
> it's not that obvious of a thing to work with compared to "did we do a
> clear?"
> 
> 
> +
> 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;
> 
> Putting this here separates the detection of whether or not we are doing a
> fast clear (and therefore don't need to set the clear color) even further
> from where we do the clear and use this value than it was previously.
> 
> 

Sure.

> +
> /* 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);
> -  }
> 
> I think we can do this even better.  We could do
> 
> bool blorp_updated_indirect_clear_color = false;
> 
> and then set it to true if we call intel_hiz_exec below.  Then, after the
> loop below we would do
> 
> if (!blorp_updated_indirect_clear_color)
> intel_miptree_update_indirect_color(brw, mt);
> 
> after we've done the clears.
> 
> I had something like that originally and I think that solution would
> have marginally better performance. I went with doing it this way
> because it allows us to:
> 
> * Do all the clear color updates 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Jason Ekstrand]

On April 25, 2018 20:25:16 Nanley Chery  wrote:

On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
On Tue, Apr 24, 2018 at 5:48 PM, 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 =
>Attachment[BUFFER_DEPTH];
const struct gen_device_info *devinfo = >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.
*/
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;

This boolean is rather awkward.

Why's that?

It does have a clear meaning and it does what it says it does.  However, 
it's not that obvious of a thing to work with compared to "did we do a clear?"



+
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;

Putting this here separates the detection of whether or not we are doing a 
fast clear (and therefore don't need to set the clear color) even further 
from where we do the clear and use this value than it was previously.



+
/* 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);
-  }

I think we can do this even better.  We could do

bool blorp_updated_indirect_clear_color = false;

and then set it to true if we call intel_hiz_exec below.  Then, after the
loop below we would do

if (!blorp_updated_indirect_clear_color)
intel_miptree_update_indirect_color(brw, mt);

after we've done the clears.

I had something like that originally and I think that solution would
have marginally better performance. I went with doing it this way
because it allows us to:

* Do all the clear color updates in one place.

That's sort-of true.  It puts all the clear color updated that happen in 
this function together.  But there is another update that BLORP is doing 
that, I would argue, it separates even further.



* Place blorp_will_update_indirect_color in a scope smaller
than the function.

True, but it's declaration, update, and use are much further apart in terms 
of logic and lines of code.  Also, it's much further away from 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Nanley Chery]

On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote:
> On Tue, Apr 24, 2018 at 5:48 PM, 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 =
> > >Attachment[BUFFER_DEPTH];
> > const struct gen_device_info *devinfo = >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.
> >  */
> > 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;
> >
> 
> This boolean is rather awkward.
> 
> 

Why's that?

> > +
> >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);
> > -  }
> >
> 
> I think we can do this even better.  We could do
> 
> bool blorp_updated_indirect_clear_color = false;
> 
> and then set it to true if we call intel_hiz_exec below.  Then, after the
> loop below we would do
> 
> if (!blorp_updated_indirect_clear_color)
>intel_miptree_update_indirect_color(brw, mt);
> 
> after we've done the clears.
> 
> 

I had something like that originally and I think that solution would
have marginally better performance. I went with doing it this way
because it allows us to:

* Do all the clear color updates in one place.
* Place blorp_will_update_indirect_color in a scope smaller
  than the function.
* Delete more code.

If we wait until the loop below to assign

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Jason Ekstrand]

On Tue, Apr 24, 2018 at 5:48 PM, 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 =
> >Attachment[BUFFER_DEPTH];
> const struct gen_device_info *devinfo = >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.
>  */
> 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;
>

This boolean is rather awkward.


> +
>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);
> -  }
>

I think we can do this even better.  We could do

bool blorp_updated_indirect_clear_color = false;

and then set it to true if we call intel_hiz_exec below.  Then, after the
loop below we would do

if (!blorp_updated_indirect_clear_color)
   intel_miptree_update_indirect_color(brw, mt);

after we've done the clears.


> }
>
> 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

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Rafael Antognolli]

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 = 
> > > >Attachment[BUFFER_DEPTH];
> > > const struct gen_device_info *devinfo = >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 
> > 
> 
> 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 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Nanley Chery]

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 = 
> > >Attachment[BUFFER_DEPTH];
> > const struct gen_device_info *devinfo = >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.
*/

> 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 
> 

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 

Re: [Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Rafael Antognolli]

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 = 
> >Attachment[BUFFER_DEPTH];
> const struct gen_device_info *devinfo = >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'm not sure if this actually helps or if it just makes the comment
unnecessarily complex.

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 

> 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 

[Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

[Nanley Chery]

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 = 
>Attachment[BUFFER_DEPTH];
const struct gen_device_info *devinfo = >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.
 */
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