[Mesa-dev] [PATCH 1/2] i965/skl: Enable fast clears for SKL

Kenneth Graunke kenneth at whitecape.org
Tue Mar 10 21:19:30 PDT 2015


On Thursday, February 26, 2015 03:42:52 PM Ben Widawsky wrote:
> From: Kristian Høgsberg <krh at bitplanet.net>
> 
> v2 by Ben:
> Rebase with conflict resolution
> Add the SKL scaling factors

I'd probably keep those as a separate patch, but commit that before this
one.

> Squashed in i965/skl: Dont zero surf[12]. The patch can't work properly without
> that, so there's no point in a separate patch.
> 
> Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_clear.c           |  2 +-
>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 16 +++++++++++----
>  src/mesa/drivers/dri/i965/gen8_surface_state.c  | 26 ++++++++++++++++++++-----
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h   |  1 +
>  4 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_clear.c b/src/mesa/drivers/dri/i965/brw_clear.c
> index 1231420..a67b8a0 100644
> --- a/src/mesa/drivers/dri/i965/brw_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_clear.c
> @@ -242,7 +242,7 @@ brw_clear(struct gl_context *ctx, GLbitfield mask)
>     }
>  
>     /* Clear color buffers with fast clear or at least rep16 writes. */
> -   if (brw->gen >= 6 && brw->gen < 9 && (mask & BUFFER_BITS_COLOR)) {
> +   if (brw->gen >= 6 && (mask & BUFFER_BITS_COLOR)) {
>        if (brw_meta_fast_clear(brw, fb, mask, partial_clear)) {
>           debug_mask("blorp color", mask & BUFFER_BITS_COLOR);
>           mask &= ~BUFFER_BITS_COLOR;
> 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 c8f2a14..c4d4b68 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> @@ -336,12 +336,17 @@ get_buffer_rect(struct brw_context *brw, struct gl_framebuffer *fb,
>   */
>  static bool
>  is_color_fast_clear_compatible(struct brw_context *brw,
> -                               mesa_format format,
> +                               struct intel_mipmap_tree *mt,
>                                 const union gl_color_union *color)
>  {
> +   mesa_format format = _mesa_get_render_format(&brw->ctx, mt->format);
> +
>     if (_mesa_is_format_integer_color(format))
>        return false;
>  

Perhaps mention:

/* WaZeroOneClearValuesMSAA: "If Number of Multisamples is not
 * NUMSAMPLES_1, only 0/1 values allowed."
 */

I sort of expected num_samples == 0, but checking the MSAA layout seems
fine too (and avoids the whole num_samples == 0? 1? WAT? problem).

> +   if (brw->gen >= 9 && mt->msaa_layout == INTEL_MSAA_LAYOUT_NONE)
> +      return true;
> +
>     for (int i = 0; i < 4; i++) {
>        if (color->f[i] != 0.0 && color->f[i] != 1.0 &&
>            _mesa_format_has_color_component(format, i)) {
> @@ -409,7 +414,6 @@ brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>                      GLbitfield buffers, bool partial_clear)
>  {
>     struct gl_context *ctx = &brw->ctx;
> -   mesa_format format;
>     enum { FAST_CLEAR, REP_CLEAR, PLAIN_CLEAR } clear_type;
>     GLbitfield plain_clear_buffers, meta_save, rep_clear_buffers, fast_clear_buffers;
>     struct rect fast_clear_rect, clear_rect;
> @@ -455,8 +459,7 @@ brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>        /* Fast clear is only supported for colors where all components are
>         * either 0 or 1.
>         */
> -      format = _mesa_get_render_format(ctx, irb->mt->format);
> -      if (!is_color_fast_clear_compatible(brw, format, &ctx->Color.ClearColor))
> +      if (!is_color_fast_clear_compatible(brw, irb->mt, &ctx->Color.ClearColor))
>           clear_type = REP_CLEAR;
>  
>        /* From the SNB PRM (Vol4_Part1):
> @@ -492,6 +495,7 @@ brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>  
>        switch (clear_type) {
>        case FAST_CLEAR:
> +         irb->mt->fast_clear_color = ctx->Color.ClearColor;
>           irb->mt->fast_clear_color_value =
>              compute_fast_clear_color_bits(&ctx->Color.ClearColor);
>           irb->need_downsample = true;
> @@ -689,6 +693,10 @@ brw_meta_resolve_color(struct brw_context *brw,
>  
>     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_RESOLVE_ENABLE);
>  
>     mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> index d6b870e..48b2da9 100644
> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> @@ -146,6 +146,22 @@ gen8_emit_buffer_surface_state(struct brw_context *brw,
>  }
>  
>  static void
> +set_fast_clear_color(struct brw_context *brw, struct intel_mipmap_tree *mt, uint32_t *surf)
> +{
> +   if (brw->gen >= 9) {
> +      /* SKL+ can set a wider range of fast clear colors in certain cases, so
> +       * we have to program the colors as four dwords.
> +       */
> +      surf[12] = mt->fast_clear_color.ui[0];
> +      surf[13] = mt->fast_clear_color.ui[1];
> +      surf[14] = mt->fast_clear_color.ui[2];
> +      surf[15] = mt->fast_clear_color.ui[3];
> +   } else {
> +      surf[7] = mt->fast_clear_color_value;
> +   }
> +}
> +
> +static void
>  gen8_update_texture_surface(struct gl_context *ctx,
>                              unsigned unit,
>                              uint32_t *surf_offset,
> @@ -243,7 +259,7 @@ gen8_update_texture_surface(struct gl_context *ctx,
>        (firstImage->_BaseFormat == GL_DEPTH_COMPONENT ||
>         firstImage->_BaseFormat == GL_DEPTH_STENCIL);
>  
> -   surf[7] = mt->fast_clear_color_value;
> +   set_fast_clear_color(brw, mt, surf);
>  
>     const int swizzle =
>        unlikely(alpha_depth) ? SWIZZLE_XYZW : brw_get_texture_swizzle(ctx, tObj);
> @@ -264,7 +280,6 @@ gen8_update_texture_surface(struct gl_context *ctx,
>        surf[10] = 0;
>        surf[11] = 0;
>     }
> -   surf[12] = 0;
>  
>     /* Emit relocation to surface contents */
>     drm_intel_bo_emit_reloc(brw->batch.bo,
> @@ -421,13 +436,15 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
>        surf[6] = 0;
>     }
>  
> -   surf[7] = mt->fast_clear_color_value |
> -             SET_FIELD(HSW_SCS_RED,   GEN7_SURFACE_SCS_R) |
> +   surf[7] = SET_FIELD(HSW_SCS_RED,   GEN7_SURFACE_SCS_R) |
>               SET_FIELD(HSW_SCS_GREEN, GEN7_SURFACE_SCS_G) |
>               SET_FIELD(HSW_SCS_BLUE,  GEN7_SURFACE_SCS_B) |
>               SET_FIELD(HSW_SCS_ALPHA, GEN7_SURFACE_SCS_A);
>  
>     assert(mt->offset % mt->cpp == 0);
> +
> +   set_fast_clear_color(brw, mt, surf);

set_fast_clear_color does "surf[7] = ..." which will clobber the SCS
values set here.  You might make both use |= instead.  surf[7] should
already be memset to 0 by allocate_surface_state().

Have you tested this on both Skylake and Broadwell?

While reviewing, I also came across the text "If programmed to non 0/1 values,
SW must ensure a render target partial resolve pass before binding a cleared
RT to texture." which I believe is called WaZeroOneClearValuesAtSampler.

I think we're OK on that, since it looks like intel_update_state calls
intel_miptree_resolve_color for all textures.  Assuming the "render
target partial resolve" is what that's doing...

With the surf[7] thing fixed, this is
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +
>     *((uint64_t *) &surf[8]) = mt->bo->offset64 + mt->offset; /* reloc */
>  
>     if (aux_mt) {
> @@ -440,7 +457,6 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
>        surf[10] = 0;
>        surf[11] = 0;
>     }
> -   surf[12] = 0;
>  
>     drm_intel_bo_emit_reloc(brw->batch.bo,
>                             brw->wm.base.surf_offset[surf_index] + 8 * 4,
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index ee9cf1e..5e9d006 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -468,6 +468,7 @@ struct intel_mipmap_tree
>      * OR into dword 7 of SURFACE_STATE.
>      */
>     uint32_t fast_clear_color_value;
> +   union gl_color_union fast_clear_color;
>  
>     /* These are also refcounted:
>      */
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150310/d9bcb1b8/attachment.sig>


More information about the mesa-dev mailing list