[Mesa-dev] [PATCH] i965: Disable guardband clipping in the smaller-than-viewport case.

[Pohjolainen, Topi]

On Tue, Aug 26, 2014 at 04:39:55PM -0700, Kenneth Graunke wrote:
> Apparently guardband clipping doesn't work like we thought: objects
> entirely outside fthe guardband are trivially rejected, regardless of
> their relation to the viewport.  Normally, the guardband is larger than
> the viewport, so this is not a problem.  However, when the viewport is
> larger than the guardband, this means that we would discard primitives
> which were wholly outside of the guardband, but still visible.
> 
> We always program the guardband to 8K x 8K to enforce the restriction
> that the screenspace bounding box of a single triangle must be no more
> than 8K x 8K.  So, if the viewport is larger than that, we need to
> disable guardband clipping.
> 
> Fixes ES3 conformance tests:
> - framebuffer_blit_functionality_negative_height_blit
> - framebuffer_blit_functionality_negative_width_blit
> - framebuffer_blit_functionality_negative_dimensions_blit
> - framebuffer_blit_functionality_magnifying_blit
> - framebuffer_blit_functionality_multisampled_to_singlesampled_blit
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
> Cc: Ian Romanick <idr at freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/gen6_clip_state.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen6_clip_state.c b/src/mesa/drivers/dri/i965/gen6_clip_state.c
> index e8c1b91..524219f 100644
> --- a/src/mesa/drivers/dri/i965/gen6_clip_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_clip_state.c
> @@ -98,6 +98,35 @@ upload_clip_state(struct brw_context *brw)
>  
>     dw2 |= GEN6_CLIP_GB_TEST;
>  
> +   /* We need to disable guardband clipping if the guardband (which we always
> +    * program to the maximum screen-space bounding box of 8K x 8K) will be
> +    * smaller than the viewport.
> +    *
> +    * Closely examining the clip determination formulas in the documentation
> +    * reveals that objects will be discarded entirely if they're outside the
> +    * (small) guardband, even if they're within the (large) viewport:
> +    *
> +    *     TR = TR_GB || TR_VPXY || TR_VPZ || TR_UC || TR_NEGW
> +    *     TA   = !TR && TA_GB && TA_VPZ && TA_NEGW
> +    *     MC = !(TA || TR)
> +    *
> +    * Disabling guardband clipping removes the TR_GB condition, which means
> +    * they'll be considered MC ("Must Clip") unless they're rejected for

Maybe add TA (Trivial Accept) and TR (Trivial Reject) somewhere in the
documentation. I didn't know these before and it would have been easier to
decode the formulas if I had.
Otherwise makes sense to me and many thanks for the explanation:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> +    * some other reason.
> +    *
> +    * Note that there is no TA_VPXY condition.  If there were, objects entirely
> +    * inside a 16384x16384 viewport would be trivially accepted, breaking the
> +    * "objects must have a screenspace bounding box not exceeding 8K in the X
> +    * or Y direction" restriction.  Instead, they're clipped.
> +    */
> +   for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) {
> +      if (ctx->ViewportArray[i].Width > 8192 ||
> +          ctx->ViewportArray[i].Height > 8192) {
> +         dw2 &= ~GEN6_CLIP_GB_TEST;
> +         break;
> +      }
> +   }
> +
>     /* 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
> -- 
> 2.0.0
>