On Fri, Jul 28, 2017 at 05:33:53PM +0100, Lionel Landwerlin wrote:
> On 28/07/17 17:21, Nanley Chery wrote:
> > On Fri, Jul 28, 2017 at 09:28:45AM +0100, Lionel Landwerlin wrote:
> > > On 19/07/17 22:21, Nanley Chery wrote:
> > > > Cc: <mesa-sta...@lists.freedesktop.org>
> > > > Suggested-by: Jason Ekstrand <ja...@jlekstrand.net>
> > > > Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com>
> > > > ---
> > > > src/intel/vulkan/anv_blorp.c | 8 ++++----
> > > > src/intel/vulkan/anv_private.h | 8 ++++----
> > > > src/intel/vulkan/genX_cmd_buffer.c | 25 +++++++++++++++----------
> > > > 3 files changed, 23 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > > > index 459d57ec57..d6cbc1c0cd 100644
> > > > --- a/src/intel/vulkan/anv_blorp.c
> > > > +++ b/src/intel/vulkan/anv_blorp.c
> > > > @@ -1434,10 +1434,10 @@ void anv_CmdResolveImage(
> > > > }
> > > > void
> > > > -anv_image_ccs_clear(struct anv_cmd_buffer *cmd_buffer,
> > > > - const struct anv_image *image,
> > > > - const uint32_t base_level, const uint32_t
> > > > level_count,
> > > > - const uint32_t base_layer, uint32_t layer_count)
> > > > +anv_image_fast_clear(struct anv_cmd_buffer *cmd_buffer,
> > > > + const struct anv_image *image,
> > > > + const uint32_t base_level, const uint32_t
> > > > level_count,
> > > > + const uint32_t base_layer, uint32_t layer_count)
> > > > {
> > > > assert(image->type == VK_IMAGE_TYPE_3D || image->extent.depth ==
> > > > 1);
> > > > diff --git a/src/intel/vulkan/anv_private.h
> > > > b/src/intel/vulkan/anv_private.h
> > > > index 4dce360c76..9a5d2d6fa4 100644
> > > > --- a/src/intel/vulkan/anv_private.h
> > > > +++ b/src/intel/vulkan/anv_private.h
> > > > @@ -2108,10 +2108,10 @@ anv_ccs_resolve(struct anv_cmd_buffer * const
> > > > cmd_buffer,
> > > > const enum blorp_fast_clear_op op);
> > > > void
> > > > -anv_image_ccs_clear(struct anv_cmd_buffer *cmd_buffer,
> > > > - const struct anv_image *image,
> > > > - const uint32_t base_level, const uint32_t
> > > > level_count,
> > > > - const uint32_t base_layer, uint32_t layer_count);
> > > > +anv_image_fast_clear(struct anv_cmd_buffer *cmd_buffer,
> > > > + const struct anv_image *image,
> > > > + const uint32_t base_level, const uint32_t
> > > > level_count,
> > > > + const uint32_t base_layer, uint32_t layer_count);
> > > > enum isl_aux_usage
> > > > anv_layout_to_aux_usage(const struct gen_device_info * const devinfo,
> > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > index 9b3bb10164..81972821d1 100644
> > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > @@ -392,7 +392,9 @@ transition_color_buffer(struct anv_cmd_buffer
> > > > *cmd_buffer,
> > > > VkImageLayout initial_layout,
> > > > VkImageLayout final_layout)
> > > > {
> > > > - if (image->aux_usage != ISL_AUX_USAGE_CCS_E)
> > > > + assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > > > +
> > > > + if (image->aux_usage == ISL_AUX_USAGE_NONE)
> > > > return;
> > > This bit definitely makes sense to me.
> > > I've been hitting it on newer work where the anv_image_fast_clear() would
> > > assert because there is no auxiliary surface.
> > >
> > > I'm not familiar with the CCS stuff enough to review the part below though
> > > :(
> > >
> > No problem, and please don't feel obligated to.
> >
> > > Acked-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>
> > >
> > Thanks for the Ack, though, this series has already been pushed
> > upstream. The assert you mentioned, are you hitting that on current
> > master?
>
> Yes, though I have other changes on top, so it might be my fault.
> That's funny you mentioned this is already upstream, I can't see the
> if (image->aux_usage == NONE) return;
> in the transition_color_buffer function on master.
> Has that changed?
>
Yes. Later on in this series, that part was replaced with:
/* No work is necessary if the layout stays the same or if this subresource
* range lacks auxiliary data.
*/
if (initial_layout == final_layout ||
base_layer >= anv_image_aux_layers(image, base_level))
return;
> >
> > Regards,
> > Nanley
> >
> > > > if (initial_layout != VK_IMAGE_LAYOUT_UNDEFINED &&
> > > > @@ -405,15 +407,18 @@ transition_color_buffer(struct anv_cmd_buffer
> > > > *cmd_buffer,
> > > > layer_count = anv_minify(image->extent.depth, base_level);
> > > > }
> > > > -#if GEN_GEN >= 9
> > > > - /* We're transitioning from an undefined layout so it doesn't
> > > > really matter
> > > > - * what data ends up in the color buffer. We do, however, need to
> > > > ensure
> > > > - * that the CCS has valid data in it. One easy way to do that is to
> > > > - * fast-clear the specified range.
> > > > - */
> > > > - anv_image_ccs_clear(cmd_buffer, image, base_level, level_count,
> > > > - base_layer, layer_count);
> > > > -#endif
> > > > + if (image->aux_usage == ISL_AUX_USAGE_CCS_E ||
> > > > + image->samples == 2 || image->samples == 8) {
> > > > + /* We're transitioning from an undefined layout so it doesn't
> > > > really
> > > > + * matter what data ends up in the color buffer. We do, however,
> > > > need to
> > > > + * ensure that the auxiliary surface is not in an undefined
> > > > state. This
> > > > + * state is possible for CCS buffers SKL+ and MCS buffers with
> > > > certain
> > > > + * sample counts. One easy way to get to a valid state is to
> > > > fast-clear
> > > > + * the specified range.
> > > > + */
> > > > + anv_image_fast_clear(cmd_buffer, image, base_level, level_count,
> > > > + base_layer, layer_count);
> > > > + }
> > > > }
> > > > /**
> > >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
