Re: [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  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)
> > 

Re: [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  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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Nanley Chery]

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.

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

-Nanley

> const bool hiz_enabled = ISL_AUX_USAGE_HIZ ==
> -- 
> 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