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 <nanleych...@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 <nanleych...@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
>
Ok, I guess I see how that looks closer in some way.
> > 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.
>
Yeah, that's starting to sound like a good idea. We could easily add a
blorp_batch_flag for "don't auto-update the clear color" and use it from
GL. Or we can just move the clear color updating back into Vulkan.
> 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@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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
