[Mesa-dev] [PATCH 09/29] anv/cmd_buffer: Generalize transition_color_buffer

[Nanley Chery]

On Mon, Nov 27, 2017 at 07:05:59PM -0800, Jason Ekstrand wrote:
> This moves it to being based on layout_to_aux_usage instead of being
> hard-coded based on bits of a priori knowledge of how transitions
> interact with layouts.  This conceptually simplifies things because
> we're now using layout_to_aux_usage and layout_supports_fast_clear to
> make resolve decisions so changes to those functions will do what one
> expects.
> 
> This fixes a potential bug with window system integration on gen9+ where
> we wouldn't do a resolve when transitioning to the PRESENT_SRC layout
> because we just assume that everything that handles CCS_E can handle it
> all the time.  When handing a CCS_E image off to the window system, we
> may need to do a full resolve if the window system does not support the
> CCS_E modifier.  The only reason why this hasn't been a problem yet is
> because we don't support modifiers in Vulkan WSI and so we always get X
> tiling which implies no CCS on gen9+.
> ---
>  src/intel/vulkan/genX_cmd_buffer.c | 53 +++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> index be717eb..65cc85d 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -593,6 +593,7 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
>                          VkImageLayout initial_layout,
>                          VkImageLayout final_layout)
>  {
> +   const struct gen_device_info *devinfo = &cmd_buffer->device->info;
>     /* Validate the inputs. */
>     assert(cmd_buffer);
>     assert(image && image->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV);
> @@ -733,17 +734,48 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
>                                   VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL,
>                                   final_layout);
>        }
> -   } else if (initial_layout != VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL) {
> -      /* Resolves are only necessary if the subresource may contain blocks
> -       * fast-cleared to values unsupported in other layouts. This only occurs
> -       * if the initial layout is COLOR_ATTACHMENT_OPTIMAL.
> -       */
> -      return;
> -   } else if (image->samples > 1) {
> -      /* MCS buffers don't need resolving. */
>        return;
>     }
>  
> +   /* If initial aux usage is NONE, there is nothing to resolve */
> +   enum isl_aux_usage initial_aux_usage =
> +      anv_layout_to_aux_usage(devinfo, image, aspect, initial_layout);
> +   if (initial_aux_usage == ISL_AUX_USAGE_NONE)
> +      return;
> +
> +   enum isl_aux_op resolve_op = ISL_AUX_OP_NONE;
> +
> +   /* If the initial layout supports fast clear but the final one does not,
> +    * then we need at least a partial resolve.
> +    */
> +   if (anv_layout_supports_fast_clear(devinfo, image, aspect, initial_layout) &&
> +       !anv_layout_supports_fast_clear(devinfo, image, aspect, final_layout))
> +      resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;

I find this hunk more readable, when resolve_op is modified less. How
about the following?
      if (anv_layout_supports_fast_clear(devinfo, image, aspect, initial_layout) &&
          !anv_layout_supports_fast_clear(devinfo, image, aspect, final_layout)) {
	 /* CCS_D only supports full resolves and BLORP will assert on
	  * us if we try to do a partial resolve on a CCS_D surface.
          */
         if (initial_aux_usage == ISL_AUX_USAGE_CCS_D) {
            resolve_op = ISL_AUX_OP_FULL_RESOLVE;
	 } else {
            resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;
	 }
      }

It may even be more readable to have most of these changes in a function
called something like color_transition_to_aux_op(). We shouldn't even
need a resolve_op variable in that case. Just a thought.

> +
> +   enum isl_aux_usage final_aux_usage =

With all the equality comparisons going on with initial_aux_usage and
final_aux_usage it would be helpful to have them const.

> +      anv_layout_to_aux_usage(devinfo, image, aspect, final_layout);
> +   if (initial_aux_usage == ISL_AUX_USAGE_CCS_E &&
> +       final_aux_usage != ISL_AUX_USAGE_CCS_E)
> +      resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> +
> +   /* CCS_D only supports full resolves and BLORP will assert on us if we try
> +    * to do a partial resolve on a CCS_D surface.
> +    */
> +   if (resolve_op == ISL_AUX_OP_PARTIAL_RESOLVE &&
> +       initial_aux_usage == ISL_AUX_USAGE_CCS_D)
> +      resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> +
> +   if (resolve_op == ISL_AUX_OP_NONE)
> +      return;
> +
> +   /* Even though the above code can theoretically handle multiple resolve
> +    * types such as CCS_D -> CCS_E, the predication code below can't.  We only

I may be missing something obvious, but what kind of resolve is needed
for CCS_D -> CCS_E? Also, why can't the predication code handle multiple
resolve types?

> +    * really handle a couple of cases.
> +    */
> +   assert(initial_aux_usage == ISL_AUX_USAGE_NONE ||
> +          final_aux_usage == ISL_AUX_USAGE_NONE ||
> +          initial_aux_usage == final_aux_usage);
> +
>     /* Perform a resolve to synchronize data between the main and aux buffer.
>      * Before we begin, we must satisfy the cache flushing requirement specified
>      * in the Sky Lake PRM Vol. 7, "MCS Buffer for Render Target(s)":
> @@ -774,10 +806,7 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
>        genX(load_needs_resolve_predicate)(cmd_buffer, image, aspect, level);
>  
>        anv_image_ccs_op(cmd_buffer, image, aspect, level,
> -                       base_layer, layer_count,
> -                       image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E ?
> -                       ISL_AUX_OP_PARTIAL_RESOLVE : ISL_AUX_OP_FULL_RESOLVE,
> -                       true);
> +                       base_layer, layer_count, resolve_op, true);
>  

This patch doesn't seem to fix the bug mentioned in the commit message.
I think you also need to disable predication if we're CCS_E and doing a
FULL_RESOLVE. At this point, the full resolve will only happen if the
image has been fast-cleared with a non-zero clear color.

-Nanley

>        genX(set_image_needs_resolve)(cmd_buffer, image, aspect, level, false);
>     }
> -- 
> 2.5.0.400.gff86faf
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev