[Mesa-dev] [PATCH] i965: Clamp "Maximum VP Index" to 1 when gl_ViewportIndex isn't written.

[Kristian Høgsberg Kristensen]

Kenneth Graunke <kenneth at whitecape.org> writes:

> fs_visitor::emit_urb_writes skips writing the VUE header for shaders
> that don't write gl_PointSize, gl_Layer, or gl_ViewportIndex.  This
> leaves their values uninitialized.  Kristian's nearby comment says:
>
> "But often none of the special varyings that live there are written
>  and in that case we can skip writing to the vue header, provided the
>  corresponding state properly clamps the values further down the
>  pipeline."
>
> However, we were clamping gl_ViewportIndex to [0, 15], so we would end
> up using a random viewport.  To fix this, detect when the shader doesn't
> write gl_ViewportIndex, and clamp it to [0, 0].
>
> The vec4 backend always writes zeros to the VUE header, so it doesn't
> suffer from this problem.  With vec4-style HWord writes, we can write
> the header and position together in a single message.  In the FS world,
> we would need 4 extra MOVs of 0 and a longer message, or a separate
> OWord write.  It's likely cheaper to just clamp the value.
>
> Fixes DiRT Showdown and Bioshock Infinite, which only rendered half of
> the screen - the lower left of two triangles.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93054
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/gen6_clip_state.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> I've tried several times to write a Piglit test for this bug, but haven't
> managed to make a test that reproduces it.  Something about these games
> triggers it every time, but none of my minimal tests do the trick.
>
> diff --git a/src/mesa/drivers/dri/i965/gen6_clip_state.c b/src/mesa/drivers/dri/i965/gen6_clip_state.c
> index 5ba3a80..47324b7 100644
> --- a/src/mesa/drivers/dri/i965/gen6_clip_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_clip_state.c
> @@ -177,6 +177,10 @@ upload_clip_state(struct brw_context *brw)
>     if (!is_drawing_points(brw) && !is_drawing_lines(brw))
>        dw2 |= GEN6_CLIP_XY_TEST;
>  
> +   const int max_vp_index =
> +      (brw->vue_map_geom_out.slots_valid & VARYING_BIT_VIEWPORT) != 0 ?
> +      ctx->Const.MaxViewports : 1;

Add

   /* BRW_NEW_VUE_MAP_GEOM_OUT */

above this chunk?

With that,
Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>

> +
>     BEGIN_BATCH(4);
>     OUT_BATCH(_3DSTATE_CLIP << 16 | (4 - 2));
>     OUT_BATCH(dw1);
> @@ -186,7 +190,7 @@ upload_clip_state(struct brw_context *brw)
>     OUT_BATCH(U_FIXED(0.125, 3) << GEN6_CLIP_MIN_POINT_WIDTH_SHIFT |
>               U_FIXED(255.875, 3) << GEN6_CLIP_MAX_POINT_WIDTH_SHIFT |
>               (_mesa_geometric_layers(fb) > 0 ? 0 : GEN6_CLIP_FORCE_ZERO_RTAINDEX) |
> -             ((ctx->Const.MaxViewports - 1) & GEN6_CLIP_MAX_VP_INDEX_MASK));
> +             ((max_vp_index - 1) & GEN6_CLIP_MAX_VP_INDEX_MASK));
>     ADVANCE_BATCH();
>  }
>  
> @@ -201,7 +205,8 @@ const struct brw_tracked_state gen6_clip_state = {
>                 BRW_NEW_GEOMETRY_PROGRAM |
>                 BRW_NEW_META_IN_PROGRESS |
>                 BRW_NEW_PRIMITIVE |
> -               BRW_NEW_RASTERIZER_DISCARD,
> +               BRW_NEW_RASTERIZER_DISCARD |
> +               BRW_NEW_VUE_MAP_GEOM_OUT,
>     },
>     .emit = upload_clip_state,
>  };
> @@ -218,7 +223,8 @@ const struct brw_tracked_state gen7_clip_state = {
>                 BRW_NEW_GEOMETRY_PROGRAM |
>                 BRW_NEW_META_IN_PROGRESS |
>                 BRW_NEW_PRIMITIVE |
> -               BRW_NEW_RASTERIZER_DISCARD,
> +               BRW_NEW_RASTERIZER_DISCARD |
> +               BRW_NEW_VUE_MAP_GEOM_OUT,
>     },
>     .emit = upload_clip_state,
>  };
> -- 
> 2.8.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev