On Wed, Jan 24, 2018 at 05:00:47PM -0800, Jason Ekstrand wrote:
> On Wed, Jan 24, 2018 at 4:39 PM, Nanley Chery <nanleych...@gmail.com> wrote:
>
> > On Wed, Jan 24, 2018 at 04:15:33PM -0800, Jason Ekstrand wrote:
> > > On Wed, Jan 24, 2018 at 4:04 PM, Nanley Chery <nanleych...@gmail.com>
> > wrote:
> > >
> > > > On Fri, Jan 19, 2018 at 03:47:30PM -0800, Jason Ekstrand wrote:
> > > > > The current strategy we use for managing resolves has an issues
> > where we
> > > > > track clear colors and the need for resolves per-LOD but we still
> > allow
> > > > > resolves of only a subset of the slices in any given LOD and doing so
> > > > > sets the "needs resolve" flag for that LOD to false while leaving the
> > > > > remaining layers unresolved. This patch is only the first step and
> > does
> > > > > not, by itself fix anything. However, it's fairly self-contained and
> > > > > splitting it out means any performance regressions should bisect to
> > this
> > > > > nice obvious commit rather than to the giant "rework aux tracking"
> > > > > commit.
> > > > >
> > > > > Nanley and I did some testing and none of the applications we tested
> > > > > even tried to fast-clear anything other than the first slice of an
> > > > > image. The applications tested were:
> > > > >
> > > >
> > > > Thanks for documenting the tested apps! We could be a tad more precise
> > > > by noting that we didn't check if an app cleared a subset of its
> > > > layers.. though even if they did, we wouldn't try to optimize for it so
> > > > it doesn't really matter.
> > > >
> > >
> > > Sure, I can make the comment a bit more descriptive:
> > >
> > > Nanley and I did some testing and none of the applications we tested
> > > even tried to fast-clear anything other than the first slice of an
> > > image. The test was done by adding a printf right before we call
> > > blorp_fast_clear if we were every going to touch any slice other than
> > > the first with a fast-clear. Due to the way the original code was
> > > structured, this would not have included applications which only
> > cleared
> > > a subset of layers. The applications tested were:
> > >
> > >
> > >
> >
> > That works.
> >
> > > > > * All Sascha Willems demos
> > > > > * Aztec Ruins
> > > > > * Dota 2
> > > > > * The Talos Principle
> > > > > * Mad Max
> > > > >
> > > >
> > > > I'm also seeing the same behavior in the following apps:
> > > > * Warhammer 40,000: Dawn of War III
> > > > * Serious Sam Fusion 2017: BFE
> > > >
> > > > Feel free to add them on.
> > > >
> > >
> > > Will do. Thanks!
> > >
> > >
> > > > > While not the full list of shipping applications, it's a pretty good
> > > > > spread and covers most of the engines we've seen running on our
> > driver.
> > > > > If this is ever shown to be a performance problem in the future, we
> > can
> > > > > reconsider our strategy.
> > > > > ---
> > > > > src/intel/vulkan/genX_cmd_buffer.c | 34
> > +++++++++++++++++-------------
> > > > ----
> > > > > 1 file changed, 17 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > > index ad7b9fc..0f56719 100644
> > > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > > @@ -309,23 +309,6 @@ color_attachment_compute_aux_usage(struct
> > > > anv_device * device,
> > > > > if (GEN_GEN <= 8 && !att_state->clear_color_is_zero_one)
> > > > > att_state->fast_clear = false;
> > > > >
> > > > > - /* We allow fast clears when all aux layers of the miplevel
> > are
> > > > targeted.
> > > > > - * See add_fast_clear_state_buffer() for more information.
> > Also,
> > > > because
> > > > > - * we only either do a fast clear or a normal clear and not
> > both,
> > > > this
> > > > > - * complies with the gen7 restriction of not fast-clearing
> > > > multiple
> > > > > - * layers.
> > > > > - */
> > > > > - if (cmd_state->framebuffer->layers !=
> > > > > - anv_image_aux_layers(iview->image,
> > VK_IMAGE_ASPECT_COLOR_BIT,
> > > > > - iview->planes[0].isl.base_level)) {
> > > > > - att_state->fast_clear = false;
> > > > > - if (GEN_GEN == 7) {
> > > > > - anv_perf_warn(device->instance, iview->image,
> > > > > - "Not fast-clearing the first layer in "
> > > > > - "a multi-layer fast clear.");
> > > > > - }
> > > > > - }
> > > > > -
> > > > > /* We only allow fast clears in the GENERAL layout if the
> > > > auxiliary
> > > > > * buffer is always enabled and the fast-clear value is all
> > 0's.
> > > > See
> > > > > * add_fast_clear_state_buffer() for more information.
> > > > > @@ -337,6 +320,23 @@ color_attachment_compute_aux_usage(struct
> > > > anv_device * device,
> > > > > att_state->fast_clear = false;
> > > > > }
> > > > >
> > > > > + /* We only allow fast clears to the first slice of an image
> > > > (level 0,
> > > > > + * layer 0) and only for the entire slice. This guarantees us
> > > > that, at
> > > > > + * any given time, there is only one clear color on any given
> > > > image at
> > > > > + * any given time. At the time of our testing (Jan 17, 2018),
> > > > there
> > > > > + * were known applications which would benefit from
> > fast-clearing
> > > > more
> > > > ^
> > > > no
> > > >
> > >
> > > Yup. Fixed locally.
> > >
> > >
> > > > > + * than just the first slice.
> > > > > + */
> > > > > + if (att_state->fast_clear &&
> > > > > + (iview->planes[0].isl.base_level > 0 ||
> > > > > + iview->image->type == VK_IMAGE_TYPE_3D ||
> > > > > + iview->image->array_size > 0)) {
> > > >
> > > > These conditions seem too restrictive. Instead of restricting
> > > > fast-clears to the first slice of an image, we're restricting
> > > > fast-clears to images with only one layer. Also, the imageview could
> > > > point to the first slice even if the base image is 3D.
> > > >
> > >
> > > Yes, it is. We need to restrict it this far due to issues with resolve
> > > tracking. The last patch in the series lifts the restriction back to the
> > > point we originally discussed of clearing the first slice of 3D and array
> > > textures.
> > >
> > >
> >
> > Right, I must've skimmed the commit title earlier.
> > Changing: iview->image->type == VK_IMAGE_TYPE_3D
> > to: iview->image->extent.depth > 1
> > is more what we want right?
> >
>
> I think that would work, yes. I can make the change if you prefer. I
> hightly doubt there are a lot of depth-1 3D images though. :) I don't
> really care as it's going away later anyway.
>
>
Yeah, they probably are small in number, but I do like that new diff
would do exactly what the commit message says. With that change,
this patch is
Reviewed-by: Nanley Chery <nanley.g.ch...@intel.com>
> > > > > + anv_perf_warn(device->instance, iview->image,
> > > > > + "Rendering to a multi-LOD or multi-layer
> > > > framebuffer "
> > > > > + "with LOAD_OP_CLEAR. Not fast-clearing");
> > > > > + att_state->fast_clear = false;
> > > > > + }
> > > > > +
> > > > > if (att_state->fast_clear) {
> > > > > memcpy(fast_clear_color->u32, att_state->clear_value.color.
> > > > uint32,
> > > > > sizeof(fast_clear_color->u32));
> > > > > --
> > > > > 2.5.0.400.gff86faf
> > > > >
> > > > > _______________________________________________
> > > > > 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
