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

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

2018-02-12 Thread Jason Ekstrand

On Mon, Feb 12, 2018 at 5:11 PM, Nanley Chery  wrote:

> 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-sta...@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?
>

It's a GCC extension (also supported by clang), but yes.


> > +
> > +#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?
>

Yes it should.  Fixed locally.


> > +  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?
>

Yes it should.  Fixed locally.  This caused a lot of fails.  I don't know
how I didn't catch it. :(

Do you want a v2?

--Jason


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

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

2018-02-12 Thread 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-sta...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-02-12 Thread Jason Ekstrand

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-sta...@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 = {};
+
+#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,
+  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);
 
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information