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

[Jason Ekstrand]

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.


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

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170519/2d597d37/attachment.html>