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

[Chad Versace]

On Tue 13 Oct 2015, Ben Widawsky wrote:
> 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."
> + *
> + * 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.
> + */
> +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);

Hard-coding level=0 layer=0 here makes me uncomfortable. We'll have to
fix it later when implementing fast clears for level > 0 and layer
0 on gen >= 8.

Any, there's no need to create a new renderbuffer. You should instead
extract the existing gl_renderbuffer of gl_texture from
gl_framebuffer::Attachments[buf]. If you do that, then there's an added
bonus: the hard-coded level=0 layer=0 disappear.

> +
> +      _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo);
> +      _mesa_FramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER,
> +                                    GL_COLOR_ATTACHMENT0,
> +                                    GL_RENDERBUFFER, rbo);
> +      _mesa_DrawBuffer(GL_COLOR_ATTACHMENT0);

There's no need to create a new framebuffer on each iteration of the
loop. You should create one framebuffer, before the loop, then reuse it
for each iteration.

Well... if this function were using real GL, not metainsanity, then
there would be no need to create a new renderbuffer and framebuffer for
each loop iteration. Meta, due to some insane reason, may require you to
create a new framebuffer and renderbuffer, but I doubt it.

> +
> +      brw_fast_clear_init(brw);
> +
> +      use_rectlist(brw, true);
> +
> +      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);
> +
> +      _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) {
>        _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