On Tue, Jun 6, 2017 at 12:54 PM, Chad Versace <chadvers...@chromium.org>
wrote:
> There's a patch on your branch I didn't see on mesa-dev.
> Subject: i965: Be a bit more conservative about certain resolves
> It has my r-b.
>
> I have comments on this patch...
>
> On Fri 26 May 2017, Jason Ekstrand wrote:
> > We have two different bits of resolve code for render targets: one in
> > brw_draw where it's always been and one in brw_context to deal with sRGB
> > on gen9. Let's pull them together.
> > ---
> > src/mesa/drivers/dri/i965/brw_context.c | 47
> ++++++++++++++++++++-------------
> > src/mesa/drivers/dri/i965/brw_draw.c | 34 ------------------------
> > 2 files changed, 29 insertions(+), 52 deletions(-)
>
>
>
> > + /* For layered rendering non-compressed fast cleared buffers need
> to be
> > + * resolved. Surface state can carry only one fast color clear
> value
> > + * while each layer may have its own fast clear color value. For
> > + * compressed buffers color value is available in the color
> buffer.
> > + */
> > + if (irb->layer_count > 1 &&
> > + !(irb->mt->aux_disable & INTEL_AUX_DISABLE_CCS) &&
> > + !intel_miptree_is_lossless_compressed(brw, mt)) {
>
> This condition smells bad. It smells like a shot in the dark. It smells
> like a haphazard guess. "We haven't permanently disabled CCS for this
> miptree. And it lacks CCS_E. So, well, it probably has CCS_D, I guess.".
>
> I would much rather see the condition with something more certain.
> Something like:
>
> if (irb->layer_count > 1 &&
> intel_miptree_has_css_d_in_layer_range(brw, mt, irb->mt_level,
> irb->mt_layer, irb->layer_count))
>
> Anway, this patch is a good cleanup, and functional changes like I'm
> requesting don't belong in a refactoring patch like this one.
>
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.
> Reviewed-by: Chad Versace <chadvers...@chromium.org>
>
> > + assert(brw->gen >= 8);
> > +
> > + intel_miptree_resolve_color(brw, mt, irb->mt_level, 1,
> > + irb->mt_layer, irb->layer_count,
> 0);
> > + }
> > }
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
