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

[Kenneth Graunke]

On Monday, August 04, 2014 12:24:03 PM Ben Widawsky wrote:
> 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

                                         ^^^ is sent to?

> 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

                                       is changed or changes ^^^

> 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

"screen dimensions" sounds like 1920x1080 (etc) to me.  How about "drawable dimensions"?

> +    * 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.
> +    */

The comment seems a bit choppy to me.  How about something like:

/* If the viewport dimensions are smaller than the drawable dimensions,
 * we have to disable guardband clipping prior to Gen8.  We always program
 * the guardband to a fixed size, which is almost always larger than the
 * viewport.  Any geometry which intersects the viewport but lies within
 * the guardband would bypass the 3D clipping stage, so it wouldn't be
 * clipped to the viewport.  Rendering would happen beyond the viewport,
 * but still inside the drawable.
 *
 * Gen8+ introduces a viewport extents test which restricts rendering to
 * the viewport, so we can ignore this restriction.
 */

Regardless of whether you take my suggestion, this is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +   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;
>        }
>     }
>  
> 
-------------- 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/20140809/ea7b7a21/attachment.sig>