<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 11, 2017 at 4:31 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jul 10, 2017 at 02:36:16PM -0700, Jason Ekstrand wrote: > On Wed, Jun 28, 2017 at 2:14 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > For readability, bring the assignment of CCS closer to the assignment of > > NONE and MCS. > > > > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > --- > > src/intel/vulkan/genX_cmd_buffer.c | 62 ++++++++++++++++++------------ > > -------- > > 1 file changed, 30 insertions(+), 32 deletions(-) > > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c > > b/src/intel/vulkan/genX_cmd_buffer.c > > index 49ad41edbd..1aa79c8e7b 100644 > > --- a/src/intel/vulkan/genX_cmd_buffer.c > > +++ b/src/intel/vulkan/genX_cmd_buffer.c > > @@ -253,6 +253,36 @@ color_attachment_compute_aux_usage(struct anv_device > > * device, > > att_state->input_aux_usage = ISL_AUX_USAGE_MCS; > > att_state->fast_clear = false; > > return; > > + } else if (iview->image->aux_usage == ISL_AUX_USAGE_CCS_E) { > > + att_state->aux_usage = ISL_AUX_USAGE_CCS_E; > > + att_state->input_aux_usage = ISL_AUX_USAGE_CCS_E; > > > > I'm not sure if this actually improves readability as-is. The no aux case > and MCS cases are early returns. Maybe what we want is something like this: > > if (aux_surface.isl.size == 0) { > /* set to none */ > return; > } > > switch (aux_usage) { > case MCS: > case CCS_E: > aux_state->aux_usage = iview->image->aux_usage; > aux_state->input_aux_usage = iview->image->aux_usage; > break; > > case NONE: > assert(samples == 1); > /* stuff below */ > break; > > default: > unreachable(); > } > > /* Now we determine whether or not we want to fast-clear */ > > if (samples > 1) { > perf_debug(); > fast_clear = false; > return; > } > > /* Other fast clear determination. */ > > Incidentally, it may be cleaner in the long run if we split this into two > functions: compute_ccs_usage and compute_mcs_usage. > > Just thoughts BTW. I'm not 100% sure how to make this the most readable. > > </div></div>I'm personally not finding the alternative block above more readable.</blockquote><div> </div><div>I'm not entirely convinced either. :-) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> To overcome this disagreement, I don't mind doing any of the following: 1. Omit the rationale in the commit message. 2. Drop the patch from this series. </blockquote><div> </div><div>I don't really care that much. I'd go for either 2. or just leave it as is. Either is fine with me. I think it'll be much more clear what to do once we have MCS fast clears in there. What I wrote above was me trying to imagine what it would look like with MCS fast clears and then delete the extra MCS bits. It'll work better once someone is writing code in a computer rather than my brain. :-) </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Thoughts? -Nanley <div class="HOEnZb"><div class="h5"> > > + } else { > > + att_state->aux_usage = ISL_AUX_USAGE_CCS_D; > > + /* From the Sky Lake PRM, RENDER_SURFACE_STATE:: > > AuxiliarySurfaceMode: > > + * > > + * "If Number of Multisamples is MULTISAMPLECOUNT_1, AUX_CCS_D > > + * setting is only allowed if Surface Format supported for Fast > > + * Clear. In addition, if the surface is bound to the sampling > > + * engine, Surface Format must be supported for Render Target > > + * Compression for surfaces bound to the sampling engine." > > + * > > + * In other words, we can only sample from a fast-cleared image if > > it > > + * also supports color compression. > > + */ > > + if (isl_format_supports_ccs_e(&device->info, iview->isl.format)) { > > + /* TODO: Consider using a heuristic to determine if temporarily > > enabling > > + * CCS_E for this image view would be beneficial. > > + * > > + * While fast-clear resolves and partial resolves are fairly > > cheap in the > > + * case where you render to most of the pixels, full resolves > > are not > > + * because they potentially involve reading and writing the > > entire > > + * framebuffer. If we can't texture with CCS_E, we should leave > > it off and > > + * limit ourselves to fast clears. > > + */ > > + att_state->input_aux_usage = ISL_AUX_USAGE_CCS_D; > > + } else { > > + att_state->input_aux_usage = ISL_AUX_USAGE_NONE; > > + } > > } > > > > assert(iview->image->aux_surface.isl.usage & ISL_SURF_USAGE_CCS_BIT); > > @@ -315,38 +345,6 @@ color_attachment_compute_aux_usage(struct anv_device > > * device, > > } else { > > att_state->fast_clear = false; > > } > > - > > - /** > > - * TODO: Consider using a heuristic to determine if temporarily > > enabling > > - * CCS_E for this image view would be beneficial. > > - * > > - * While fast-clear resolves and partial resolves are fairly cheap in > > the > > - * case where you render to most of the pixels, full resolves are not > > - * because they potentially involve reading and writing the entire > > - * framebuffer. If we can't texture with CCS_E, we should leave it > > off and > > - * limit ourselves to fast clears. > > - */ > > - if (iview->image->aux_usage == ISL_AUX_USAGE_CCS_E) { > > - att_state->aux_usage = ISL_AUX_USAGE_CCS_E; > > - att_state->input_aux_usage = ISL_AUX_USAGE_CCS_E; > > - } else { > > - att_state->aux_usage = ISL_AUX_USAGE_CCS_D; > > - /* From the Sky Lake PRM, RENDER_SURFACE_STATE:: > > AuxiliarySurfaceMode: > > - * > > - * "If Number of Multisamples is MULTISAMPLECOUNT_1, AUX_CCS_D > > - * setting is only allowed if Surface Format supported for Fast > > - * Clear. In addition, if the surface is bound to the sampling > > - * engine, Surface Format must be supported for Render Target > > - * Compression for surfaces bound to the sampling engine." > > - * > > - * In other words, we can only sample from a fast-cleared image if > > it > > - * also supports color compression. > > - */ > > - if (isl_format_supports_ccs_e(&device->info, iview->isl.format)) > > - att_state->input_aux_usage = ISL_AUX_USAGE_CCS_D; > > - else > > - att_state->input_aux_usage = ISL_AUX_USAGE_NONE; > > - } > > } > > > > static bool > > -- > > 2.13.1 > > > > _______________________________________________ > > 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> > > </div></div></blockquote></div> </div></div>