<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 6, 2017 at 12:54 PM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There's a patch on your branch I didn't see on mesa-dev.<br>
   Subject: i965: Be a bit more conservative about certain resolves<br>
It has my r-b.<br>
<br>
I have comments on this patch...<br>
<span class=""><br>
On Fri 26 May 2017, Jason Ekstrand wrote:<br>
> We have two different bits of resolve code for render targets: one in<br>
> brw_draw where it's always been and one in brw_context to deal with sRGB<br>
> on gen9.  Let's pull them together.<br>
> ---<br>
>  src/mesa/drivers/dri/i965/brw_<wbr>context.c | 47 ++++++++++++++++++++----------<wbr>---<br>
>  src/mesa/drivers/dri/i965/brw_<wbr>draw.c    | 34 ------------------------<br>
>  2 files changed, 29 insertions(+), 52 deletions(-)<br>
<br>
<br>
<br>
</span><span class="">> +      /* For layered rendering non-compressed fast cleared buffers need to be<br>
> +       * resolved. Surface state can carry only one fast color clear value<br>
> +       * while each layer may have its own fast clear color value. For<br>
> +       * compressed buffers color value is available in the color buffer.<br>
> +       */<br>
> +      if (irb->layer_count > 1 &&<br>
> +          !(irb->mt->aux_disable & INTEL_AUX_DISABLE_CCS) &&<br>
> +          !intel_miptree_is_lossless_<wbr>compressed(brw, mt)) {<br>
<br>
</span>This condition smells bad. It smells like a shot in the dark. It smells<br>
like a haphazard guess. "We haven't permanently disabled CCS for this<br>
miptree. And it lacks CCS_E. So, well, it probably has CCS_D, I guess.".<br>
<br>
I would much rather see the condition with something more certain.<br>
Something like:<br>
<span class=""><br>
    if (irb->layer_count > 1 &&<br>
</span>        intel_miptree_has_css_d_in_<wbr>layer_range(brw, mt, irb->mt_level, irb->mt_layer, irb->layer_count))<br>
<br>
Anway, this patch is a good cleanup, and functional changes like I'm<br>
requesting don't belong in a refactoring patch like this one.<br></blockquote><div><br></div><div>Yes, I'd like to get that cleaned up.  I think the right thing to do is actually to check for whether or not mt->format is_ccs_e_compatible with the actual format that will be used for rendering.  If it is, then we can just fix up the clear color.  If not, we need a full resolve.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Reviewed-by: Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>><br>
<div class="HOEnZb"><div class="h5"><br>
> +         assert(brw->gen >= 8);<br>
> +<br>
> +         intel_miptree_resolve_color(<wbr>brw, mt, irb->mt_level, 1,<br>
> +                                     irb->mt_layer, irb->layer_count, 0);<br>
> +      }<br>
>     }<br>
</div></div></blockquote></div><br></div></div>