[Mesa-dev] [PATCH 05/10] i965/meta/gen9: Individually fast clear color attachments

[Neil Roberts]

Ben Widawsky <benjamin.widawsky at intel.com> writes:

> The impetus for this patch comes from a seemingly benign statement within the
> spec (quoted within the patch). For me, this patch was at some point critical
> for getting stable piglit results (though this did not seem to be the case on a
> branch Chad was working on).
>
> It is very important for clearing multiple color buffer attachments and can be
> observed in the following piglit tests:
> spec/arb_framebuffer_object/fbo-drawbuffers-none glclear
> spec/ext_framebuffer_multisample/blit-multiple-render-targets 0
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 97 +++++++++++++++++++++----
>  1 file changed, 84 insertions(+), 13 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> index 7bf52f0..9e6711e 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> @@ -427,6 +427,74 @@ use_rectlist(struct brw_context *brw, bool enable)
>     brw->ctx.NewDriverState |= BRW_NEW_FRAGMENT_PROGRAM;
>  }
>  
> +/**
> + * Individually fast clear each color buffer attachment. On previous gens this
> + * isn't required. The motivation for this comes from one line (which seems to
> + * be specific to SKL+). The list item is in section titled _MCS Buffer for
> + * Render Target(s)_
> + *
> + *   "Since only one RT is bound with a clear pass, only one RT can be cleared
> + *   at a time. To clear multiple RTs, multiple clear passes are required."

This sentence also appears in the HSW PRM so it seems a bit odd if it's
only causing problems on SKL. I guess if we get Piglit regressions
without it then it makes sense to have the patch. It might be worth just
double checking whether this patch is completely necessary. The wording
in the commit message seems a little unsure.

> + *
> + * The code follows the same idea as the resolve code which creates a fake FBO
> + * to avoid interfering with too much of the GL state.
> + */

What state are you trying to avoid interfering with? Maybe the resolve
code only creates an FBO because it needs to work on an abritrary
intel_mipmap_tree which might not already have an FBO associated with
it. It seems like it should be enough just to call
_mesa_meta_drawbuffers_from_bitfield(1 << index) instead of creating an
FBO. If you did that you could also make the loop a bit nicer by just
looping over the bits set in fast_clear_buffers with ffs. The
glDrawBuffers state is already saved as part of the meta save state and
the existing code for gen<9 also modifies it.

> +static void
> +fast_clear_attachments(struct brw_context *brw,
> +                       struct gl_framebuffer *fb,
> +                       uint32_t fast_clear_buffers,
> +                       struct rect fast_clear_rect)
> +{
> +   assert(brw->gen >= 9);
> +   struct gl_context *ctx = &brw->ctx;
> +
> +   GLuint old_fb = ctx->DrawBuffer->Name;
> +
> +   for (unsigned buf = 0; buf < fb->_NumColorDrawBuffers; buf++) {
> +      struct gl_renderbuffer *rb = fb->_ColorDrawBuffers[buf];
> +      struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> +      GLuint fbo, rbo;
> +      int index = fb->_ColorDrawBufferIndexes[buf];
> +
> +      if (!((1 << index) & fast_clear_buffers))
> +         continue;
> +
> +      _mesa_GenFramebuffers(1, &fbo);
> +      rbo = brw_get_rb_for_slice(brw, irb->mt, 0, 0, false);
> +
> +      _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo);
> +      _mesa_FramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER,
> +                                    GL_COLOR_ATTACHMENT0,
> +                                    GL_RENDERBUFFER, rbo);
> +      _mesa_DrawBuffer(GL_COLOR_ATTACHMENT0);
> +
> +      brw_fast_clear_init(brw);

Is is really necessary to call this for every buffer? It looks like it's
called once before fast_clear_attachments is called so maybe it doesn't
need to be in this function at all?

> +
> +      use_rectlist(brw, true);

Same for this.

> +
> +      brw_bind_rep_write_shader(brw, (float *) fast_clear_color);
> +
> +      /* SKL+ also has a resolve mode for compressed render targets and thus more
> +       * bits to let us select the type of resolve.  For fast clear resolves, it
> +       * turns out we can use the same value as pre-SKL though.
> +       */
> +      set_fast_clear_op(brw, GEN7_PS_RENDER_TARGET_FAST_CLEAR_ENABLE);
> +      brw_draw_rectlist(ctx, &fast_clear_rect, MAX2(1, fb->MaxNumLayers));
> +      set_fast_clear_op(brw, 0);
> +      use_rectlist(brw, false);

And this.

> +
> +      _mesa_DeleteRenderbuffers(1, &rbo);
> +      _mesa_DeleteFramebuffers(1, &fbo);
> +
> +      /* Now set the mcs we cleared to INTEL_FAST_CLEAR_STATE_CLEAR so we'll
> +       * resolve them eventually.
> +       */
> +      irb->mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_CLEAR;
> +   }
> +
> +   _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, old_fb);
> +}
> +
>  bool
>  brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>                      GLbitfield buffers, bool partial_clear)
> @@ -600,12 +668,27 @@ brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>     use_rectlist(brw, true);
>  
>     layers = MAX2(1, fb->MaxNumLayers);
> -   if (fast_clear_buffers) {
> +
> +   if (brw->gen >= 9 && fast_clear_buffers) {
> +      fast_clear_attachments(brw, fb, fast_clear_buffers, fast_clear_rect);
> +   } else if (fast_clear_buffers) {

This seems a bit clunky. Maybe it would be nicer like this:

  if (fast_clear_buffers) {
     if (brw->gen >= 9) {
        fast_clear_attachments(...);
     } else {
        /* stuff for gen < 9 */
     }
  }

But if you'd rather keep it the way it is too avoid indenting too much
then I won't complain.

>        _mesa_meta_drawbuffers_from_bitfield(fast_clear_buffers);
>        brw_bind_rep_write_shader(brw, (float *) fast_clear_color);
>        set_fast_clear_op(brw, GEN7_PS_RENDER_TARGET_FAST_CLEAR_ENABLE);
>        brw_draw_rectlist(ctx, &fast_clear_rect, layers);
>        set_fast_clear_op(brw, 0);
> +
> +      /* Now set the mcs we cleared to INTEL_FAST_CLEAR_STATE_CLEAR so we'll
> +       * resolve them eventually.
> +       */
> +      for (unsigned buf = 0; buf < fb->_NumColorDrawBuffers; buf++) {
> +         struct gl_renderbuffer *rb = fb->_ColorDrawBuffers[buf];
> +         struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> +         int index = fb->_ColorDrawBufferIndexes[buf];
> +
> +         if ((1 << index) & fast_clear_buffers)
> +            irb->mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_CLEAR;
> +      }
>     }
>  
>     if (rep_clear_buffers) {
> @@ -614,18 +697,6 @@ brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>        brw_draw_rectlist(ctx, &clear_rect, layers);
>     }
>  
> -   /* Now set the mts we cleared to INTEL_FAST_CLEAR_STATE_CLEAR so we'll
> -    * resolve them eventually.
> -    */
> -   for (unsigned buf = 0; buf < fb->_NumColorDrawBuffers; buf++) {
> -      struct gl_renderbuffer *rb = fb->_ColorDrawBuffers[buf];
> -      struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> -      int index = fb->_ColorDrawBufferIndexes[buf];
> -
> -      if ((1 << index) & fast_clear_buffers)
> -         irb->mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_CLEAR;
> -   }
> -
>   bail_to_meta:
>     /* Dirty _NEW_BUFFERS so we reemit SURFACE_STATE which sets the fast clear
>      * color before resolve and sets irb->mt->fast_clear_state to UNRESOLVED if
> -- 
> 2.6.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev