[Mesa-dev] [PATCH 04/16] anv: Handle transitioning depth from UNDEFINED to other layouts

[Nanley Chery]

On Fri, May 19, 2017 at 05:18:12PM -0700, Jason Ekstrand wrote:
> On Fri, May 19, 2017 at 4:51 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > On Thu, May 18, 2017 at 02:00:51PM -0700, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/vulkan/anv_image.c       | 12 ++++++++++--
> > >  src/intel/vulkan/genX_cmd_buffer.c |  9 +--------
> > >  2 files changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > > index d21e055..d65ef47 100644
> > > --- a/src/intel/vulkan/anv_image.c
> > > +++ b/src/intel/vulkan/anv_image.c
> > > @@ -488,12 +488,20 @@ anv_layout_to_aux_usage(const struct
> > gen_device_info * const devinfo,
> > >     /* According to the Vulkan Spec, the following layouts are valid
> > only as
> > >      * initial layouts in a layout transition and don't support device
> > access.
> > >      */
> > > -   case VK_IMAGE_LAYOUT_UNDEFINED:
> > > -   case VK_IMAGE_LAYOUT_PREINITIALIZED:
> > >     case VK_IMAGE_LAYOUT_RANGE_SIZE:
> > >     case VK_IMAGE_LAYOUT_MAX_ENUM:
> > >        unreachable("Invalid image layout for device access.");
> > >
> > > +   /* Undefined layouts
> > > +    *
> > > +    * The pre-initialized layout is equivalent to the undefined layout
> > for
> > > +    * optimally-tiled images.  We can only do color compression (CCS or
> > HiZ)
> > > +    * on tiled images.
> > > +    */
> > > +   case VK_IMAGE_LAYOUT_UNDEFINED:
> > > +   case VK_IMAGE_LAYOUT_PREINITIALIZED:
> > > +      return ISL_AUX_USAGE_NONE;
> > > +
> > >
> >
> > This function is defined to return the isl_aux_usage for "device access"
> > as described by the Vulkan spec. The user is not allowed to perform
> > device accesses in either of those layouts (11.4. Image Layouts). My
> > suggestion for dealing with this can be found below.
> >
> 
> I guess I don't really see why the "device access" distinction is useful.
> Perhaps you could explain?  Just plugging the two other layouts in seems to
> work fairly well.
> 
> 

This distinction has helped me to catch the UNDEFINED layout being used
in an unexpected place (to me at the time). genX_cmd_buffer.c:532.

At the time we were defining this function, we decided that the caller of
this function would be responsible for determining the best layout for
performing a layout transition. 

I suppose setting the usage to NONE isn't determining the optimal
layout, but rather a convenient layout. It's getting late, so I'll have
to give this more thought next week.

If we do decide to handle those enums here, I think we'll have to update
some comments in this function.

> > >     /* Transfer Layouts
> > >      *
> > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > > index 1f30a12..8d5f61b 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -355,15 +355,8 @@ transition_depth_buffer(struct anv_cmd_buffer
> > *cmd_buffer,
> > >      * The undefined layout indicates that the user doesn't care about
> > the data
> > >      * that's currently in the buffer. Therefore, a data-preserving
> > resolve
> > >      * operation is not needed.
> > > -    *
> > > -    * The pre-initialized layout is equivalent to the undefined layout
> > for
> > > -    * optimally-tiled images. Anv only exposes support for
> > optimally-tiled
> > > -    * depth buffers.
> > >      */
> > > -   if (image->aux_usage != ISL_AUX_USAGE_HIZ ||
> > > -       initial_layout == final_layout ||
> > > -       initial_layout == VK_IMAGE_LAYOUT_UNDEFINED ||
> > > -       initial_layout == VK_IMAGE_LAYOUT_PREINITIALIZED)
> > > +   if (image->aux_usage != ISL_AUX_USAGE_HIZ || initial_layout ==
> > final_layout)
> > >        return;
> > >
> >
> > I think we should keep this new condition and add another one below for
> > UNDEFINED and PREINITIALIZED in which we do the resolve early and
> > return.
> >
> > I don't mind adding a new condition because the original idea I had of
> > only comparing hiz_enabled and enable_hiz isn't optimal - for example,
> > we unnecessarily perform a resolve when going from a read-only
> > hiz-enabled layout to a hiz-disabled layout. (Thankfully, I haven't yet
> > found any apps that make such a transition.)
> >
> 
> I don't think that's unnecessary.  If the user goes
> 
> DEPTH_STENCIL_OPTIMAL
> SHADER_READ_ONLY_OPTIMAL
> TRANSFER_DST_OPTIMAL
> 
> we need to resolve somewhere in the chain.  The best I think we can hope
> for is another predicated resolve trick like you did for color buffers.
> 

That's true.