[Mesa-dev] [PATCH 13/30] i965: Combine render target resolve code

[Jason Ekstrand]

On Tue, Jun 6, 2017 at 12:54 PM, Chad Versace <chadversary at 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 <chadversary at chromium.org>
>
> > +         assert(brw->gen >= 8);
> > +
> > +         intel_miptree_resolve_color(brw, mt, irb->mt_level, 1,
> > +                                     irb->mt_layer, irb->layer_count,
> 0);
> > +      }
> >     }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170606/7f7291e2/attachment-0001.html>