<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Feb 12, 2018 at 5:11 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">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: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a> > --- > 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 = {}; </div></div>Is this initializer valid? </blockquote><div> </div><div>It's a GCC extension (also supported by clang), but yes. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + > +#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? </blockquote><div> </div><div>Yes it should. Fixed locally. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + 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? </blockquote><div> Yes it should. Fixed locally. This caused a lot of fails. I don't know how I didn't catch it. :( </div><div>Do you want a v2? </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> -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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>