[Mesa-dev] [PATCH 00/12] anv: Implement HiZ for simple render passes

[Nanley Chery]

On Fri, Sep 02, 2016 at 05:27:11PM -0700, Jason Ekstrand wrote:
> On Wed, Aug 31, 2016 at 8:29 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > This series enables Hierarchical depth buffer rendering and fast depth
> > clears
> > for render passes with a single subpass running on platforms BDW+.
> > Platforms
> > pre-BDW can test this feature with an environment variable. The FPS of some
> > demos are roughly estimated to increase by as much as ~50% on a SKL GT2.
> >
> > This feature was partially implemented by Chad and Jason. Where applicable,
> > I've tried to accurately note the modifications I've made to their patches
> > without being too verbose. I've also tried to maintain the authorship of
> > their
> > patches when the core of their work remained.
> >
> > The only patch which wasn't retained due to the core of the work being lost
> > was a patch to create a HiZ surface. This was replaced with my patch to
> > update
> > an existing function which does so. This diverged enough for me to feel at
> > risk of misrepresenting the original author's work.
> >
> > Any suggestions with respect to my annotating method, notices of
> > incorrectly
> > attributed credit, or general comments are welcome.
> >
> 
> Feel free to take more credit. :)  Chad and I wrote sketchy, untested,
> skeleton patches.  You were the one who got it working!
> 
> Patches 1, 3-7, and 10-12 have a few comments here and there.  Assuming
> those comments are addressed, those patches are
> 
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> 

I've made more updates to patch 10 and 12 than your comments so
I'll wait for you to take a look at the V2 before applying your Rb.

> We talked about 2 offline and I sent my little 6-patch series that makes
> the original plan work.
> 
> On patch 9, I gave a bunch of comments but one thing was clear: We need
> tests.  In the interest of merging patches, I think I'd recommend that we
> disable HiZ for mipmapped surfaces (we can just not allocate the surface)
> and don't do fast-clears for anything other than full-RT clears.  That
> seems like the shortest path to getting the patches merged quickly with
> some guarantee of correctness.
> 

I had a local patch to disable gen8 multisampled and BDW+ mipmapped HiZ in
patch 9, but I prefer your plan of not allocating the surface at all.
Partial clears are currently tested by the CTS.

> For partial clears and mipmapped HiZ, I think we need more tests.  There
> may be CTS tests for partial depth clears (In particular, the subpass
> tests) but I'm not sure.  I'll leave it up to you as to whether you'd
> rather write CTS tests or crucible tests.  Crucible may be easier, but the
> CTS needs those tests too, so maybe we should be good citizens and put them
> there?
> 
> 

Yes, we do need more tests. The CTS is steadily increasing its test
coverage so I'm thinking of revisiting those cases once a test
exists for it. If I do write a test, it'd likely be a crucible one.

Nanley

> >
> > Chad Versace (4):
> >   anv: Add anv_image::hiz_surface
> >   anv: Add func anv_image_has_hiz()
> >   anv: Allocate hiz surface
> >   genX/cmd_buffer: Enable rendering to HiZ
> >
> > Jason Ekstrand (3):
> >   anv: Move BindImageMemory to anv_image.c
> >   anv/image: Memset hiz surfaces to 0 when binding memory
> >   anv/cmd_buffer: Add code for performing HZ operations
> >
> > Nanley Chery (5):
> >   isl: Correct a comment in the isl_format enum
> >   isl: Update isl_surf_get_hiz_surf()
> >   isl: Make MSAA pixel scaling function public
> >   genX/cmd_buffer: Enable fast depth clears
> >   anv/TODO: Update the HiZ task
> >
> >  src/intel/isl/isl.c                |  41 ++++++++++--
> >  src/intel/isl/isl.h                |   6 +-
> >  src/intel/vulkan/TODO              |   2 +-
> >  src/intel/vulkan/anv_device.c      |  20 ------
> >  src/intel/vulkan/anv_genX.h        |   3 +
> >  src/intel/vulkan/anv_image.c       |  67 ++++++++++++++++++-
> >  src/intel/vulkan/anv_pass.c        |  11 +++
> >  src/intel/vulkan/anv_private.h     |  18 +++++
> >  src/intel/vulkan/gen7_cmd_buffer.c |   5 ++
> >  src/intel/vulkan/gen8_cmd_buffer.c | 134 ++++++++++++++++++++++++++++++
> > +++++++
> >  src/intel/vulkan/genX_cmd_buffer.c |  45 +++++++++++--
> >  11 files changed, 313 insertions(+), 39 deletions(-)
> >
> > --
> > 2.9.3
> >
> >