[Mesa-dev] [PATCH 3/4] i965/guardband: Enable for all viewport dimensions (GEN8+)

[Ben Widawsky]

The goal of guardband clipping is to try to avoid 3d clipping because it
is an expensive operation. When guardband clipping is disabled, all
geometry that intersects the viewport is to the FF 3d clipper. Objects
which are entirely enclosed within the viewport are said to be
"trivially accepted" while those entirely outside of the viewport are,
"trivially rejected".

When guardband clipping is turned on the above behavior is change such
that if the geometry is within the guardband, and intersects the
viewport, it skips the 3d clipper. Prior to GEN8, this was problematic
if the viewport was smaller than the screen as it could allow for
rendering to occur outside of the viewport. That could be mitigated if
the programmer specified a scissor region which was less than or equal
to the viewport - but this is not required for correctness in OpenGL. In
theory you could be clever with the guardband so as not to invoke this
problem. We do not do this, and have no data that suggests we should
bother (nor the converse data).

With viewport extents in place on GEN8, it should be safe to turn on
guardband clipping for all cases

While here, add a comment to the code which confused me thoroughly.
---
 src/mesa/drivers/dri/i965/gen6_clip_state.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/gen6_clip_state.c b/src/mesa/drivers/dri/i965/gen6_clip_state.c
index 52027e0..42e78a1 100644
--- a/src/mesa/drivers/dri/i965/gen6_clip_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_clip_state.c
@@ -97,17 +97,26 @@ upload_clip_state(struct brw_context *brw)
            GEN6_USER_CLIP_CLIP_DISTANCES_SHIFT);
 
    dw2 |= GEN6_CLIP_GB_TEST;
-   for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) {
-      if (ctx->ViewportArray[i].X != 0 ||
-          ctx->ViewportArray[i].Y != 0 ||
-          ctx->ViewportArray[i].Width != (float) fb->Width ||
-          ctx->ViewportArray[i].Height != (float) fb->Height) {
-         dw2 &= ~GEN6_CLIP_GB_TEST;
-         if (brw->gen >= 8) {
-            perf_debug("Disabling GB clipping due to lack of Gen8 viewport "
-                       "clipping setup code.  This should be fixed.\n");
+   /* If the viewport dimensions < screen dimensions we have to disable
+    * guardband clipping. This is because as the code works today, the
+    * guardband rectangle is set to a fixed size. If that size is larger than
+    * the viewport (likely) Guardband clipping can potentially make geometry
+    * bypass the 3d clipping phase (geometry which intersects the viewport).
+    * This will possibly cause rendering to occur outside of the viewport, but
+    * inside the screen.
+    *
+    * On GEN8 we get a useful viewport extents test which allows us to
+    * ignore this entirely.
+    */
+   if (brw->gen < 8) {
+      for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) {
+         if (ctx->ViewportArray[i].X != 0 ||
+             ctx->ViewportArray[i].Y != 0 ||
+             ctx->ViewportArray[i].Width != (float) fb->Width ||
+             ctx->ViewportArray[i].Height != (float) fb->Height) {
+            dw2 &= ~GEN6_CLIP_GB_TEST;
+            break;
          }
-         break;
       }
    }
 
-- 
2.0.3