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 <kenn...@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; > } > } > >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev