On Fri, May 19, 2017 at 4:51 PM, Nanley Chery <nanleych...@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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev