[Mesa-stable] [Mesa-dev] [PATCH 2/2] anv: Be more careful about fast-clear colors

[Nanley Chery]

On Mon, Feb 12, 2018 at 04:35:20PM -0800, Jason Ekstrand wrote:
> Previously, we just used all the channels regardless of the format.
> This is less than ideal because some channels may have undefined values
> and this should be ok from the client's perspective.  Even though the
> driver should do the correct thing regardless of what is in the
> undefined value, it makes things less deterministic.  In particular, the
> driver may choose to fast-clear or not based on undefined values.  This
> level of nondeterminism is bad.
> 
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/intel/vulkan/genX_cmd_buffer.c | 47 ++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> index 99854eb..a574024 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -202,24 +202,6 @@ add_image_view_relocs(struct anv_cmd_buffer *cmd_buffer,
>     }
>  }
>  
> -static bool
> -color_is_zero_one(VkClearColorValue value, enum isl_format format)
> -{
> -   if (isl_format_has_int_channel(format)) {
> -      for (unsigned i = 0; i < 4; i++) {
> -         if (value.int32[i] != 0 && value.int32[i] != 1)
> -            return false;
> -      }
> -   } else {
> -      for (unsigned i = 0; i < 4; i++) {
> -         if (value.float32[i] != 0.0f && value.float32[i] != 1.0f)
> -            return false;
> -      }
> -   }
> -
> -   return true;
> -}
> -
>  static void
>  color_attachment_compute_aux_usage(struct anv_device * device,
>                                     struct anv_cmd_state * cmd_state,
> @@ -294,13 +276,26 @@ color_attachment_compute_aux_usage(struct anv_device * device,
>  
>     assert(iview->image->planes[0].aux_surface.isl.usage & ISL_SURF_USAGE_CCS_BIT);
>  
> +   const struct isl_format_layout *view_fmtl =
> +      isl_format_get_layout(iview->planes[0].isl.format);
> +   union isl_color_value clear_color = {};

Is this initializer valid?

> +
> +#define COPY_CLEAR_COLOR_CHANNEL(c, i) \
> +   if (view_fmtl->channels.c.bits) \
> +      clear_color.u32[i] = att_state->clear_value.color.uint32[i]
> +
> +   COPY_CLEAR_COLOR_CHANNEL(r, 0);
> +   COPY_CLEAR_COLOR_CHANNEL(g, 1);
> +   COPY_CLEAR_COLOR_CHANNEL(b, 2);
> +   COPY_CLEAR_COLOR_CHANNEL(a, 3);
> +
> +#undef COPY_CLEAR_COLOR_CHANNEL
> +
>     att_state->clear_color_is_zero_one =
> -      color_is_zero_one(att_state->clear_value.color, iview->planes[0].isl.format);
> +      isl_color_value_is_zero_one(*fast_clear_color,

Should this be clear_color?

> +                                  iview->planes[0].isl.format);
>     att_state->clear_color_is_zero =
> -      att_state->clear_value.color.uint32[0] == 0 &&
> -      att_state->clear_value.color.uint32[1] == 0 &&
> -      att_state->clear_value.color.uint32[2] == 0 &&
> -      att_state->clear_value.color.uint32[3] == 0;
> +      isl_color_value_is_zero(*fast_clear_color, iview->planes[0].isl.format);
>  

Should this be clear_color?


-Nanley

>     if (att_state->pending_clear_aspects == VK_IMAGE_ASPECT_COLOR_BIT) {
>        /* Start by getting the fast clear type.  We use the first subpass
> @@ -358,10 +353,8 @@ color_attachment_compute_aux_usage(struct anv_device * device,
>                         "LOAD_OP_CLEAR.  Only fast-clearing the first slice");
>        }
>  
> -      if (att_state->fast_clear) {
> -         memcpy(fast_clear_color->u32, att_state->clear_value.color.uint32,
> -                sizeof(fast_clear_color->u32));
> -      }
> +      if (att_state->fast_clear)
> +         *fast_clear_color = clear_color;
>     } else {
>        att_state->fast_clear = false;
>     }
> -- 
> 2.5.0.400.gff86faf
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev