On Nov 24, 2016 8:42 AM, "Ben Widawsky" <<a href="mailto:benjamin.widawsky@intel.com">benjamin.widawsky@intel.com</a>> wrote: > > On 16-11-23 12:04:02, Jason Ekstrand wrote: >> >> On Wed, Nov 23, 2016 at 10:16 AM, Pohjolainen, Topi < >> <a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote: >> >>> On Wed, Nov 23, 2016 at 09:05:39AM -0800, Jason Ekstrand wrote: >>> > On Wed, Nov 23, 2016 at 1:16 AM, Topi Pohjolainen >>> > <[1]<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote: >>> > >>> > Originally re-clears where skipped but when lossless compression >>> > was introduced the re-clears where errorneously enabled also for >>> > non-compressed fast clears. >>> > Signed-off-by: Topi Pohjolainen <[2]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> >>> > CC: Ben Widawsky <[3]<a href="mailto:benjamin.widawsky@intel.com">benjamin.widawsky@intel.com</a>> >>> > CC: Kenneth Graunke <[4]<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> >>> > CC: Harri Syrja <[5]<a href="mailto:harri.syrja@intel.com">harri.syrja@intel.com</a>> >>> > Cc: Chad Versace <[6]<a href="mailto:chad@kiwitree.net">chad@kiwitree.net</a>> >>> > --- >>> > src/mesa/drivers/dri/i965/brw_blorp.c | 19 ++++++++++++++++--- >>> > 1 file changed, 16 insertions(+), 3 deletions(-) >>> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c >>> > b/src/mesa/drivers/dri/i965/brw_blorp.c >>> > index 556f2c0..9a849f5 100644 >>> > --- a/src/mesa/drivers/dri/i965/brw_blorp.c >>> > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c >>> > @@ -825,10 +825,23 @@ do_single_blorp_clear(struct brw_context *brw, >>> > struct gl_framebuffer *fb, >>> > brw, &irb->mt->gen9_fast_clear_ >>> > color, >>> > &override_color); >>> > - /* If the buffer is already in INTEL_FAST_CLEAR_STATE_CLEAR, >>> > the clear >>> > - * is redundant and can be skipped. >>> > + /* If the buffer is already in INTEL_FAST_CLEAR_STATE_CLEAR, >>> > and the >>> > + * buffer isn't compressed, the clear is redundant and can be >>> > skipped. >>> > + * >>> > + * Without compression fast clear only operates on the mcs >>> > buffer >>> > + * recording if color values are cleared. The hardware, >>> > however, >>> > + * doesn't write the actual color value into the mcs or color >>> > + * buffer. Only by the time of render (inclucing color >>> > resolve) does the >>> > + * hardware read the _current_ color value in the surface >>> > state and >>> > + * write the actual pixel values in the color buffer >>> > accordingly. >>> > + * >>> > + * This seems to be the reason why sampler engine cannot >>> > handle >>> > + * non-compressed fast clear - it doesn't know how to read >>> > the color >>> > + * value from the surface state. With compression the color >>> > value is >>> > + * recorded in the color buffer (only not for every pixel) >>> > and therefore >>> > + * it is available without consulting the surface state. >>> > >>> > This doesn't jive with my understanding of fast clears on gen9. >>> > Everything I've seen so far indicates that the clear color in the >>> > surface state *does* matter. Otherwise, why would it be there? In >>> > particular, my understanding of the 2-bit CCS values is that 0 means >>> > resolved, 1 means compressed and 3 means clear where "clear" means "go >>> > look at the clear color". Have you done experiments that lead you to >>> > some other conclusion? >>> >>> Right, you are correct. This is actually a patch from really early days >>> when I >>> didn't know any better. We might want to drop this for now, there is the >>> 0/1 >>> color thing for sampler engine that we probably need to fix first anyway. >>> >> >> Let's drop it if we can. >> >> We should already have code for Broadwell and earlier that prevents >> fast-clears with non-0/1 clear colors. We could just also do that on Sky >> Lake and deal with partial resolves later. I doubt non-0/1 fast-clear is a >> big enough deal over color compression to slow down getting this landed. >> > > Numbers What do you mean? Of i recall correctly, we noticed zero speedups in any apps when we enabled non-0/1 fast clears in the first place. Also, I wasn't trying to say we shouldn't enable them or shouldn't get them working; my understanding is that they're enabled and broken today. I was saying that we shouldn't block landing this on coming up with a partial resolve story to fix non-0/1 clears with compression because that's really an unrelated task. We do need better resolves and hopefully we'll get that done soon but poor Topi has been sitting on this 3‰ perf improvement and trying to land it fitter months now. Sorry of that got a bit ranty. I hope it makes more sense. >> >>> What do you think? >>> >>> > >>> > */ >>> > - if (!color_updated && >>> > + if ((!color_updated || !is_lossless_compressed) && >>> > irb->mt->fast_clear_state == >>> > INTEL_FAST_CLEAR_STATE_CLEAR) >>> > return true; >>> > -- >>> > 2.5.5 >>> > _______________________________________________ >>> > mesa-dev mailing list >>> > [7]<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >>> > [8]<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >>> > >>> > References >>> > >>> > 1. mailto:<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a> >>> > 2. mailto:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> >>> > 3. mailto:<a href="mailto:benjamin.widawsky@intel.com">benjamin.widawsky@intel.com</a> >>> > 4. mailto:<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a> >>> > 5. mailto:<a href="mailto:harri.syrja@intel.com">harri.syrja@intel.com</a> >>> > 6. mailto:<a href="mailto:chad@kiwitree.net">chad@kiwitree.net</a> >>> > 7. mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >>> > 8. <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >>> > > -- > Ben Widawsky, Intel Open Source Technology Center