[Mesa-dev] [PATCH 4/7] [v2] i965/meta/gen9: Individually fast clear color attachments

[Neil Roberts]

Hi,

You don't seem to have included any of the suggestions I made in my
review. Was this deliberate? If not, the main points were:

• You don't need to call brw_fast_clear_init or use_rectlist in the
  function because these are already called before entering it.

• I don't think it's worth creating a framebuffer. Instead you can just
  call _mesa_meta_drawbuffers_from_bitfield(1 << index) in the loop.
  Modifying the draw buffers state should be ok because it's saved in
  the meta state and it's already done for the Gen8 code path.

I went ahead and tried the changes in a patch here:

https://github.com/bpeel/mesa/commit/b2aa8f2d90572392030e5177952bf

It doesn't cause any Jenkins regressions. Feel free to squash it into
the patch if you want, or of course if you prefer to keep your patch as
it is it's up to you.

Regards,
- Neil

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
>
> v2: Doing the framebuffer binding only once (Chad)
> Directly use the renderbuffers from the mt (Chad)
>
> Cc: Chad Versace <chad.versace at linux.intel.com>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 94 +++++++++++++++++++++----
>  1 file changed, 81 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 eac92d4..97444d7 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> @@ -428,6 +428,71 @@ 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;
> +   const GLuint old_fb = ctx->DrawBuffer->Name;
> +   GLuint fbo;
> +
> +   _mesa_GenFramebuffers(1, &fbo);
> +   _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo);
> +   _mesa_DrawBuffer(GL_COLOR_ATTACHMENT0);
> +
> +   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);
> +
> +   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 ((fast_clear_buffers & (1 << index)) == 0)
> +         continue;
> +
> +
> +      _mesa_framebuffer_renderbuffer(ctx, ctx->DrawBuffer,
> +                                     GL_COLOR_ATTACHMENT0, rb,
> +                                     "meta fast clear (per-attachment)");
> +
> +      brw_draw_rectlist(ctx, &fast_clear_rect, MAX2(1, fb->MaxNumLayers));
> +
> +      /* 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;
> +   }
> +
> +   set_fast_clear_op(brw, 0);
> +   use_rectlist(brw, false);
> +
> +   _mesa_DeleteFramebuffers(1, &fbo);
> +   _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)
> @@ -603,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) {
> @@ -617,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.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev