[Mesa-dev] [PATCH 19/30] i965: Move color rendering to the new resolve functions

[Chad Versace]

On Wed 31 May 2017, Pohjolainen, Topi wrote:
> On Tue, May 30, 2017 at 05:59:51PM -0700, Jason Ekstrand wrote:
> > On Fri, May 26, 2017 at 4:30 PM, Jason Ekstrand <jason at jlekstrand.net>
> > wrote:
> > 
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_context.c       | 35 +++----------------
> > >  src/mesa/drivers/dri/i965/brw_draw.c          |  4 +--
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 48
> > > +++++++++++++++++++++++++++
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  9 +++++
> > >  4 files changed, 63 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > > b/src/mesa/drivers/dri/i965/brw_context.c
> > > index 07ddaf0..48e8b6c 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > > @@ -301,38 +301,11 @@ intel_update_state(struct gl_context * ctx, GLuint
> > > new_state)
> > >        if (irb == NULL || irb->mt == NULL)
> > >           continue;
> > >
> > > -      struct intel_mipmap_tree *mt = irb->mt;
> > > +      intel_miptree_prepare_render(brw, irb->mt, irb->mt_level,
> > > +                                   irb->mt_layer, irb->layer_count,
> > > +                                   ctx->Color.sRGBEnabled);
> > >
> > > -      /* If FRAMEBUFFER_SRGB is used on Gen9+ then we need to resolve any
> > > of
> > > -       * the single-sampled color renderbuffers because the CCS buffer
> > > isn't
> > > -       * supported for SRGB formats. This only matters if
> > > FRAMEBUFFER_SRGB is
> > > -       * enabled because otherwise the surface state will be programmed
> > > with
> > > -       * the linear equivalent format anyway.
> > > -       */
> > > -      if (brw->gen >= 9 && ctx->Color.sRGBEnabled && mt->num_samples <= 1
> > > &&
> > > -          _mesa_get_srgb_format_linear(mt->format) != mt->format) {
> > > -
> > > -         /* Lossless compression is not supported for SRGB formats, it
> > > -          * should be impossible to get here with such surfaces.
> > > -          */
> > > -         assert(!intel_miptree_is_lossless_compressed(brw, mt));
> > > -         intel_miptree_all_slices_resolve_color(brw, mt, 0);
> > > -         brw_render_cache_set_check_flush(brw, mt->bo);
> > > -      }
> > > -
> > > -      /* 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)) {
> > > -         assert(brw->gen >= 8);
> > > -
> > > -         intel_miptree_resolve_color(brw, mt, irb->mt_level, 1,
> > > -                                     irb->mt_layer, irb->layer_count, 0);
> > > -      }
> > > +      brw_render_cache_set_check_flush(brw, irb->mt->bo);
> > >
> > 
> > This render_cache_set_check_flush is unneeded and is actually the cause of
> > most of the performance regressions in this series.  Making it
> > unconditional meant we flushed the render cache on every draw call.  It's a
> > bit surprising that doing so didn't hurt things any worse than it did.  It
> > was originally put in to satisfy the requirements about flushing around
> > resolves.  Now that we do that directly in brw_blorp_resolve_color, we
> > don't need it at all much less unconditionally.  I've removed this line
> > locally.
> 
> Makes sense, I remember fighting against unconditional flushing as well. This
> series makes a big difference in how easy is to keep track of aux state and
> flushing more precisely.

Please say in the commit message that the patch removes an instance of
brw_render_cache_set_check_flush(). The commit message in your
i965-resolve-rework-v3 branch implies that it's merely a refactoring
patch.

With the expanded commit message,
Reviewed-by: Chad Versace <chadversary at chromium.org>