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