Re: [Mesa-dev] [PATCH 1/2] i965: Add and use a helper to update the indirect miptree color
On Mon, Apr 23, 2018 at 11:27:16AM -0700, Jason Ekstrand wrote:
> There are two refactors going on here that are being conflated. One is
> what the commit message says where we add and use a helper.
>
> On Fri, Apr 20, 2018 at 3:12 PM, Rafael Antognolli <
> rafael.antogno...@intel.com> wrote:
>
> > On Wed, Apr 11, 2018 at 01:56:16PM -0700, Nanley Chery wrote:
> > > Split out this functionality to enable a fast-clear optimization for
> > > color miptrees in the next commit.
> > > ---
> > > src/mesa/drivers/dri/i965/brw_clear.c | 54
> > ---
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +++
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 7
> > > 3 files changed, 36 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
> > b/src/mesa/drivers/dri/i965/brw_clear.c
> > > index 3d540d6d905..1cdc2241eac 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;
> > > @@ -176,7 +175,8 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > > /* 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.
> > > */
> > > - if (mt->fast_clear_color.f32[0] != clear_value) {
> > > + const bool same_clear_value = mt->fast_clear_color.f32[0] ==
> > clear_value;
> > > + if (!same_clear_value) {
> > >for (uint32_t level = mt->first_level; level <= mt->last_level;
> > level++) {
> > > if (!intel_miptree_level_has_hiz(mt, level))
> > > continue;
> > > @@ -214,7 +214,6 @@ 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;
> > > @@ -225,56 +224,17 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > >
> > >if (aux_state != ISL_AUX_STATE_CLEAR) {
> > > need_clear = true;
> > > - break;
> > > - }
> > > - }
> > > -
> > > - if (!need_clear) {
> > > - /* If all of the layers we intend to clear are already in the
> > clear
> > > - * state then simply updating the miptree fast clear value is
> > sufficient
> > > - * to change their clear value.
> > > - */
> > > - if (devinfo->gen >= 10 && !same_clear_value) {
> > > - /* Before gen10, it was enough to just update the clear value
> > in the
> > > - * miptree. But on gen10+, we let blorp update the clear value
> > state
> > > - * buffer when doing a fast clear. Since we are skipping the
> > fast
> > > - * clear here, we need to update the clear color ourselves.
> > > - */
> > > - uint32_t clear_offset = mt->aux_buf->clear_color_offset;
> > > - union isl_color_value clear_color = { .f32 = { clear_value, }
> > };
> > > -
> > > - /* We can't update the clear color while the hardware is still
> > using
> > > - * the previous one for a resolve or sampling from it. So make
> > sure
> > > - * that there's no pending commands at this point.
> > > - */
> > > - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL);
> > > - for (int i = 0; i < 4; i++) {
> > > -brw_store_data_imm32(brw, mt->aux_buf->clear_color_bo,
> > > - clear_offset + i * 4,
> > clear_color.u32[i]);
> > > - }
> > > - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_
> > INVALIDATE);
> > > - }
> > > - return true;
> > > - }
> >
>
> The other is down here where you re-order setting the indirect clear color
> with respect to the HiZ op. I'd rather we split this patch into two
> different onces which do the two different refactors. I think the re-order
> is safe but I'd like to have something for it to bisect to if not. :-)
>
>
Yeah, this patch does do many things at once.. I'll split it out into
multiple patches.
-Nanley
> > > -
> > > - 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) {
> > > intel_hiz_exec(brw, mt, depth_irb->mt_level,
> > > depth_irb->mt_layer + a, 1,
> > > ISL_AUX_OP_FAST_CLEAR);
> > > +
Re: [Mesa-dev] [PATCH 1/2] i965: Add and use a helper to update the indirect miptree color
There are two refactors going on here that are being conflated. One is
what the commit message says where we add and use a helper.
On Fri, Apr 20, 2018 at 3:12 PM, Rafael Antognolli <
rafael.antogno...@intel.com> wrote:
> On Wed, Apr 11, 2018 at 01:56:16PM -0700, Nanley Chery wrote:
> > Split out this functionality to enable a fast-clear optimization for
> > color miptrees in the next commit.
> > ---
> > src/mesa/drivers/dri/i965/brw_clear.c | 54
> ---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +++
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 7
> > 3 files changed, 36 insertions(+), 47 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
> b/src/mesa/drivers/dri/i965/brw_clear.c
> > index 3d540d6d905..1cdc2241eac 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;
> > @@ -176,7 +175,8 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > /* 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.
> > */
> > - if (mt->fast_clear_color.f32[0] != clear_value) {
> > + const bool same_clear_value = mt->fast_clear_color.f32[0] ==
> clear_value;
> > + if (!same_clear_value) {
> >for (uint32_t level = mt->first_level; level <= mt->last_level;
> level++) {
> > if (!intel_miptree_level_has_hiz(mt, level))
> > continue;
> > @@ -214,7 +214,6 @@ 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;
> > @@ -225,56 +224,17 @@ brw_fast_clear_depth(struct gl_context *ctx)
> >
> >if (aux_state != ISL_AUX_STATE_CLEAR) {
> > need_clear = true;
> > - break;
> > - }
> > - }
> > -
> > - if (!need_clear) {
> > - /* If all of the layers we intend to clear are already in the
> clear
> > - * state then simply updating the miptree fast clear value is
> sufficient
> > - * to change their clear value.
> > - */
> > - if (devinfo->gen >= 10 && !same_clear_value) {
> > - /* Before gen10, it was enough to just update the clear value
> in the
> > - * miptree. But on gen10+, we let blorp update the clear value
> state
> > - * buffer when doing a fast clear. Since we are skipping the
> fast
> > - * clear here, we need to update the clear color ourselves.
> > - */
> > - uint32_t clear_offset = mt->aux_buf->clear_color_offset;
> > - union isl_color_value clear_color = { .f32 = { clear_value, }
> };
> > -
> > - /* We can't update the clear color while the hardware is still
> using
> > - * the previous one for a resolve or sampling from it. So make
> sure
> > - * that there's no pending commands at this point.
> > - */
> > - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL);
> > - for (int i = 0; i < 4; i++) {
> > -brw_store_data_imm32(brw, mt->aux_buf->clear_color_bo,
> > - clear_offset + i * 4,
> clear_color.u32[i]);
> > - }
> > - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_
> INVALIDATE);
> > - }
> > - return true;
> > - }
>
The other is down here where you re-order setting the indirect clear color
with respect to the HiZ op. I'd rather we split this patch into two
different onces which do the two different refactors. I think the re-order
is safe but I'd like to have something for it to bisect to if not. :-)
> > -
> > - 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) {
> > intel_hiz_exec(brw, mt, depth_irb->mt_level,
> > depth_irb->mt_layer + a, 1,
> > ISL_AUX_OP_FAST_CLEAR);
> > + intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level,
> > + depth_irb->mt_layer + a, 1,
> > + ISL_AUX_STATE_CLEAR);
> >}
> > }
> >
> > - /* Now, the HiZ buffer contains data that needs to be resolved to
> the depth
> > -* buffer.
> > -*/
> > - intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level,
> > -
Re: [Mesa-dev] [PATCH 1/2] i965: Add and use a helper to update the indirect miptree color
On Wed, Apr 11, 2018 at 01:56:16PM -0700, Nanley Chery wrote:
> Split out this functionality to enable a fast-clear optimization for
> color miptrees in the next commit.
> ---
> src/mesa/drivers/dri/i965/brw_clear.c | 54
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +++
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 7
> 3 files changed, 36 insertions(+), 47 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
> b/src/mesa/drivers/dri/i965/brw_clear.c
> index 3d540d6d905..1cdc2241eac 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;
> @@ -176,7 +175,8 @@ brw_fast_clear_depth(struct gl_context *ctx)
> /* 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.
> */
> - if (mt->fast_clear_color.f32[0] != clear_value) {
> + const bool same_clear_value = mt->fast_clear_color.f32[0] == clear_value;
> + if (!same_clear_value) {
>for (uint32_t level = mt->first_level; level <= mt->last_level;
> level++) {
> if (!intel_miptree_level_has_hiz(mt, level))
> continue;
> @@ -214,7 +214,6 @@ 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;
> @@ -225,56 +224,17 @@ brw_fast_clear_depth(struct gl_context *ctx)
>
>if (aux_state != ISL_AUX_STATE_CLEAR) {
> need_clear = true;
> - break;
> - }
> - }
> -
> - if (!need_clear) {
> - /* If all of the layers we intend to clear are already in the clear
> - * state then simply updating the miptree fast clear value is
> sufficient
> - * to change their clear value.
> - */
> - if (devinfo->gen >= 10 && !same_clear_value) {
> - /* Before gen10, it was enough to just update the clear value in the
> - * miptree. But on gen10+, we let blorp update the clear value state
> - * buffer when doing a fast clear. Since we are skipping the fast
> - * clear here, we need to update the clear color ourselves.
> - */
> - uint32_t clear_offset = mt->aux_buf->clear_color_offset;
> - union isl_color_value clear_color = { .f32 = { clear_value, } };
> -
> - /* We can't update the clear color while the hardware is still using
> - * the previous one for a resolve or sampling from it. So make sure
> - * that there's no pending commands at this point.
> - */
> - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL);
> - for (int i = 0; i < 4; i++) {
> -brw_store_data_imm32(brw, mt->aux_buf->clear_color_bo,
> - clear_offset + i * 4, clear_color.u32[i]);
> - }
> - brw_emit_pipe_control_flush(brw,
> PIPE_CONTROL_STATE_CACHE_INVALIDATE);
> - }
> - return true;
> - }
> -
> - 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) {
> intel_hiz_exec(brw, mt, depth_irb->mt_level,
> depth_irb->mt_layer + a, 1,
> ISL_AUX_OP_FAST_CLEAR);
> + intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level,
> + depth_irb->mt_layer + a, 1,
> + ISL_AUX_STATE_CLEAR);
>}
> }
>
> - /* Now, the HiZ buffer contains data that needs to be resolved to the
> depth
> -* buffer.
> -*/
> - intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level,
> - depth_irb->mt_layer, num_layers,
> - ISL_AUX_STATE_CLEAR);
> + if (!need_clear && !same_clear_value)
> + intel_miptree_update_indirect_color(brw, mt);
>
> return true;
> }
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 0b6a821d08c..23e73c5419c 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -3831,3 +3831,25 @@ intel_miptree_get_clear_color(const struct
> gen_device_info *devinfo,
>return mt->fast_clear_color;
> }
> }
> +
> +void
> +intel_miptree_update_indirect_color(struct brw_context
[Mesa-dev] [PATCH 1/2] i965: Add and use a helper to update the indirect miptree color
Split out this functionality to enable a fast-clear optimization for
color miptrees in the next commit.
---
src/mesa/drivers/dri/i965/brw_clear.c | 54 ---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +++
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 7
3 files changed, 36 insertions(+), 47 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
b/src/mesa/drivers/dri/i965/brw_clear.c
index 3d540d6d905..1cdc2241eac 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;
@@ -176,7 +175,8 @@ brw_fast_clear_depth(struct gl_context *ctx)
/* 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.
*/
- if (mt->fast_clear_color.f32[0] != clear_value) {
+ const bool same_clear_value = mt->fast_clear_color.f32[0] == clear_value;
+ if (!same_clear_value) {
for (uint32_t level = mt->first_level; level <= mt->last_level; level++)
{
if (!intel_miptree_level_has_hiz(mt, level))
continue;
@@ -214,7 +214,6 @@ 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;
@@ -225,56 +224,17 @@ brw_fast_clear_depth(struct gl_context *ctx)
if (aux_state != ISL_AUX_STATE_CLEAR) {
need_clear = true;
- break;
- }
- }
-
- if (!need_clear) {
- /* If all of the layers we intend to clear are already in the clear
- * state then simply updating the miptree fast clear value is sufficient
- * to change their clear value.
- */
- if (devinfo->gen >= 10 && !same_clear_value) {
- /* Before gen10, it was enough to just update the clear value in the
- * miptree. But on gen10+, we let blorp update the clear value state
- * buffer when doing a fast clear. Since we are skipping the fast
- * clear here, we need to update the clear color ourselves.
- */
- uint32_t clear_offset = mt->aux_buf->clear_color_offset;
- union isl_color_value clear_color = { .f32 = { clear_value, } };
-
- /* We can't update the clear color while the hardware is still using
- * the previous one for a resolve or sampling from it. So make sure
- * that there's no pending commands at this point.
- */
- brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL);
- for (int i = 0; i < 4; i++) {
-brw_store_data_imm32(brw, mt->aux_buf->clear_color_bo,
- clear_offset + i * 4, clear_color.u32[i]);
- }
- brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_INVALIDATE);
- }
- return true;
- }
-
- 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) {
intel_hiz_exec(brw, mt, depth_irb->mt_level,
depth_irb->mt_layer + a, 1,
ISL_AUX_OP_FAST_CLEAR);
+ intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level,
+ depth_irb->mt_layer + a, 1,
+ ISL_AUX_STATE_CLEAR);
}
}
- /* Now, the HiZ buffer contains data that needs to be resolved to the depth
-* buffer.
-*/
- intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level,
- depth_irb->mt_layer, num_layers,
- ISL_AUX_STATE_CLEAR);
+ if (!need_clear && !same_clear_value)
+ intel_miptree_update_indirect_color(brw, mt);
return true;
}
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 0b6a821d08c..23e73c5419c 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -3831,3 +3831,25 @@ intel_miptree_get_clear_color(const struct
gen_device_info *devinfo,
return mt->fast_clear_color;
}
}
+
+void
+intel_miptree_update_indirect_color(struct brw_context *brw,
+struct intel_mipmap_tree *mt)
+{
+ assert(mt->aux_buf);
+
+ if (mt->aux_buf->clear_color_bo == NULL)
+ return;
+
+ /* We can't update the clear color while the hardware is still using the
+* previous one for a resolve or sampling from it. Make sure
