[Mesa-dev] [PATCH 16/22] anv/cmd_buffer: Add transition_color_buffer()

[Nanley Chery]

On Wed, May 10, 2017 at 12:38:50PM -0700, Jason Ekstrand wrote:
> On Wed, May 10, 2017 at 11:30 AM, Nanley Chery <nanleychery at gmail.com>
> wrote:
> 
> > On Wed, May 03, 2017 at 02:24:19PM -0700, Jason Ekstrand wrote:
> > > On Thu, Apr 27, 2017 at 11:32 AM, Nanley Chery <nanleychery at gmail.com>
> > > wrote:
> > >
> > > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > > ---
> > > >  src/intel/vulkan/genX_cmd_buffer.c | 93
> > ++++++++++++++++++++++++++++++
> > > > ++++++++
> > > >  1 file changed, 93 insertions(+)
> > > >
> > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > index d5cc358aec..1ae0c3256e 100644
> > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > @@ -454,6 +454,99 @@ genX(transfer_clear_value)(struct anv_cmd_buffer
> > *
> > > > const cmd_buffer,
> > > >     }
> > > >  }
> > > >
> > > > +/* Transitions a color buffer from one layout to another. */
> > > > +static void
> > > > +transition_color_buffer(struct anv_cmd_buffer * const cmd_buffer,
> > > > +                        const struct anv_image * const image,
> > > > +                        const uint32_t base_level, uint32_t
> > level_count,
> > > > +                        const uint32_t base_layer, uint32_t
> > layer_count,
> > > > +                        const VkImageLayout initial_layout,
> > > > +                        const VkImageLayout final_layout)
> > > > +{
> > > > +   assert(cmd_buffer && image);
> > > > +
> > > > +   /* This must be a color image. */
> > > > +   assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > > > +
> > > > +   /* Only color buffers with CCS need resolving.  */
> > > > +   if (image->aux_surface.isl.size == 0 || image->samples > 1)
> > > > +      return;
> > > > +
> > > > +   /* Don't transition this subresource range if it lacks auxiliary
> > data.
> > > > */
> > > > +   if (base_level >= anv_color_aux_levels(image) ||
> > > > +       base_layer >= anv_color_aux_layers(image, base_level))
> > > > +      return;
> > > > +
> > > > +   /* The undefined layout indicates that the user doesn't care about
> > the
> > > > +    * data that's currently in the buffer. The pre-initialized layout
> > is
> > > > +    * equivalent to the undefined layout for optimally-tiled images.
> > > > +    *
> > > > +    * We can only skip the resolve for CCS_E images in this layout
> > > > because it
> > > > +    * is enabled outside of render passes. This allows previously
> > > > fast-cleared
> > > > +    * and undefined buffers to be defined with transfer operations.
> > > > +    */
> > > > +   const bool is_ccs_e = image->aux_usage == ISL_AUX_USAGE_CCS_E;
> > > > +   const bool undef_layout = initial_layout ==
> > VK_IMAGE_LAYOUT_UNDEFINED
> > > > ||
> > > > +                             initial_layout == VK_IMAGE_LAYOUT_
> > > > PREINITIALIZED;
> > > > +   if (is_ccs_e && undef_layout)
> > > > +      return;
> > > > +
> > > > +   /* A resolve isn't necessary when transitioning from a layout that
> > > > doesn't
> > > > +    * have fast-clear data or to a layout that will be aware of the
> > > > fast-clear
> > > > +    * value.
> > > > +    */
> > > > +   const bool maybe_fast_cleared = undef_layout || initial_layout ==
> > > > +                                   VK_IMAGE_LAYOUT_COLOR_
> > > > ATTACHMENT_OPTIMAL;
> > > > +   if (!maybe_fast_cleared || final_layout ==
> > > > +       VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL)
> > > > +      return;
> > > > +
> > > > +   /* Determine the optimal resolve operation. For now we only need to
> > > > resolve
> > > > +    * the clear color.
> > > > +    */
> > > > +   const enum blorp_fast_clear_op op = is_ccs_e ?
> > > > +                                       BLORP_FAST_CLEAR_OP_RESOLVE_
> > PARTIAL
> > > > :
> > > > +                                       BLORP_FAST_CLEAR_OP_RESOLVE_
> > FULL;
> > > > +
> > > > +   /* The actual range that will be transitioned is limited by the
> > number
> > > > of
> > > > +    * subresources that have auxiliary data.
> > > > +    */
> > > > +   level_count = MIN2(level_count, anv_color_aux_levels(image));
> > > > +
> > > > +   /* A write cache flush with an end-of-pipe sync is required between
> > > > +    * rendering, clearing, and resolving operations. Perform a flush
> > of
> > > > the
> > > > +    * write cache before and after the resolve operation to meet this
> > > > +    * requirement.
> > > > +    */
> > > >
> > >
> > > anv_blorp.c has a more detailed comment about this.  I think I'd rather
> > we
> > > copy it here so that it doesn't get lost when we delete
> > > ccs_resolve_attachments.
> > >
> > > Also, I have no idea whether we need to do it per-fast-clear-op or once
> > > before we do fast-clear ops and once afterwards.  I think this is
> > probably
> > > fine.
> > >
> > >
> >
> > Contrary to the comment in anv_blorp, I found documentation that
> > provides us more information as to what's needed. How about I rewrite my
> > comment to this instead?
> >
> >    From the Sky Lake PRM Vol. 7, "MCS Buffer for Render Target(s)":
> >
> >       Any transition from any value in {Clear, Render, Resolve} to a
> >       different value in {Clear, Render, Resolve} requires end of pipe
> >       synchronization.
> >
> >    Perform a flush of the write cache before and after the resolve
> >    operation to meet this requirement.
> >
> 
> That PRM citation is fine and maybe better than the one I used (though they
> say about the same thing).  The part I was hoping to preserve is
> 
>     * This comment is a bit cryptic and doesn't really tell you what's going
>     * or what's really needed.  It appears that fast clear ops are not
>     * properly synchronized with other drawing.  We need to use a
> PIPE_CONTROL
>     * to ensure that the contents of the previous draw hit the render target
>     * before we resolve and then use a second PIPE_CONTROL after the resolve
>     * to ensure that it is completed before any additional drawing occurs.
> 
> Yes, the PRM text tells you that an end of pipe sync is needed, but it
> doesn't tell you where.  It's empirical evidence from hunting down bugs
> that tells us we need it on both sides.
> 

How does the PRM text not tell you where the end of pipe sync is needed?
It states that we must perform the sync between any value in {Clear,
Render, Resolve} to a different value in {Clear, Render, Resolve}. This
means that we're left with two options:

1. Track every clear, render, resolve operation and perform the sync
   in between as described (should be easy in GL).
2. Track none of the operations and perform the sync around 2 of the
   three operations (only feasible option in Vulkan).