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

[Nanley Chery]

On Fri, May 19, 2017 at 06:09:49PM -0700, Nanley Chery wrote:
> 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.
> 

I've had more time to think about this and I now agree with what you've
done here for the following reasons:
* ISL_AUX_USAGE_NONE is more than an convenient layout/usage. It is
  correct to say that the aux buffer cannot be used in the UNDEFINED
  layout.
* If we still want to maintain the behaviour of not suggesting a
  buffer for a layout that doesn't support device access we can explain
  returning ISL_AUX_USAGE_NONE for UNDEFINED with the fact that NONE
  doesn't suggest using the main buffer, but rather states a restriction
  on the aux buffer.
* As you mentioned, handling UNDEFINED in the mapping function as we do
  with this patch quite nicely gives us the behavior we want in the
  transitioning function.

With this change, I think we should also update some comments in the
layout_to_aux_usage function like:
* Dropping the NOTE at the top (it's not really saying anything useful).
* Moving & updating (or dropping) the comment that used to be above 
  VK_IMAGE_LAYOUT_UNDEFINED.

-Nanley

> > > >     /* 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.