[Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value
Rafael Antognolli
rafael.antognolli at intel.com
Thu Apr 26 18:19:06 UTC 2018
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 <nanleychery at gmail.com> 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 <nanleychery at gmail.com> 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 =
> > &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.
> > */
> > 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 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.
> >
> >
>
> I don't see how it's being separated further. This is the flow of the
> current patch:
>
> * Update the inline miptree color
> * Update the indirect miptree color ourselves
> * Update the indirect miptree color through BLORP
> * Do the fast-clear operation through BLORP
>
> This is the flow of the suggested patch:
>
> * Update the inline miptree color
> * Update the indirect miptree color through BLORP
> * Do the fast-clear operation through BLORP
> * Update the indirect miptree color ourselves
>
> As we're discussing this, I'm starting to wonder if we should make it
> possible to opt-out of BLORP's update of the indirect color. In GL, we
> clear slice-by-slice to avoid fast-clearing something that's already
> cleared. Since we aren't clearing the whole range at once, BLORP will
> write the indirect clear color multiple times. We could avoid these
> duplicate writes by just having intel_miptree_set_depth_clear_value()
> and the like update the indirect color if the clear value has changed.
> That would also solve the issues we're discussing here. Thoughts?
If you want to opt out of BLORP's update of the indirect clear color,
you can just not set the clear color address during a fast clear. This
way, blorp will just stomp the clear state in the aux buffer, but not
update the color.
We still need to set the clear color address during a resolve, so BLORP
can use it, but it won't update the clear color state buffer.
> > * 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 it's real
> > meaning which is determined by the loop below. I dislike tightly coupled
> > lots that look almost nothing some and have to agree on a value. Before
> > this patch, we had two such loops but they were small and structured the
> > same so it was ready to verify that the generated value was correct. Now,
> > we're generating it in a very different looking loop so that's much harder
> > to verify.
> >
> >
>
> Okay, I can see the difficulty in that.
>
> > * Delete more code.
> >
> > I think what I suggested below is actually less code.
> >
> >
>
> Right. If also I modify how we set same_clear_value, I think it'll
> result in one less line.
>
> -Nanley
>
> > If we wait until the loop below to assign
> > blorp_updated_indirect_clear_color, we have to keep the same_clear_value
> > variable around and then do:
> >
> > Yes, we would. However, that's a very straightforward and obvious value to
> > keep around.
> >
> > --Jason
> >
> >
> > if (!blorp_updated_indirect_clear_color && !same_clear_value)
> > intel_miptree_update_indirect_color(brw, mt);
> >
> > -Nanley
> >
> > }
> >
> > for (unsigned a = 0; a < num_layers; a++) {
> > --
> > 2.16.2
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list