[Mesa-dev] [PATCH] i965: Always scissor on Gen7/7.5 instead of disabling guardband.

[Kenneth Graunke]

On Thursday, January 12, 2017 8:46:43 AM PST Chris Wilson wrote:
> On Thu, Jan 12, 2017 at 12:19:41AM -0800, Kenneth Graunke wrote:
> > Previously we disabled the guardband when the viewport was smaller than
> > the framebuffer on Gen7/7.5, to prevent portions of primitives from
> > being draw outside of the viewport.  On Gen8+, we relied on the viewport
> > extents test to effectively scissor this away for us.
> > 
> > We can simply always enable scissoring instead.  We already include the
> > viewport in the scissor rectangle, so this will effectively do the
> > viewport extents test for us.  (The only difference is that the scissor
> > rectangle doesn't support sub-pixel values.  I think that's okay.)
> > 
> > Scissoring ought to be cheap (more or less free?), and enabling the
> > guardband reduces the cost of clipping.  It also allows us to drop
> > _NEW_SCISSOR from this atom (which isn't a huge deal - we already
> > listen to a ton of other bits).
> > 
> > This also fixes misrendering in Blender, where the "floor plane" grid
> > lines started rendering at wrong angles after I disabled XY clipping
> > of line primitives.  Enabling the guardband seems to solve the issue.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99339
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/gen6_clip_state.c | 26 --------------------------
> >  src/mesa/drivers/dri/i965/gen6_sf_state.c   | 15 +++------------
> >  src/mesa/drivers/dri/i965/gen7_sf_state.c   | 12 ++----------
> >  3 files changed, 5 insertions(+), 48 deletions(-)
> > 
> > I have not benchmarked this yet.  I should.
> > 
> > diff --git a/src/mesa/drivers/dri/i965/gen6_clip_state.c b/src/mesa/drivers/dri/i965/gen6_clip_state.c
> > index 0b3c7f16f18..8e893f5668f 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_clip_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_clip_state.c
> > @@ -203,32 +203,6 @@ upload_clip_state(struct brw_context *brw)
> >        }
> >     }
> >  
> > -   /* 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.
> > -    */
> > -   if (brw->gen < 8) {
> 
> Would not
> 
> 	if (brw->gen < 8 &&
> 	    !(brw_is_drawing_points(brw) || brw_is_drawing_lines(brw)))
> 
> by itself fix the misrendering as a first step without any concern for
> wider impact?
> -Chris

Yes, it would.  I wrote that patch first, but then thought this was a
simpler end result, if it isn't too expensive.

I suppose we could do both, and revert the bigger hammer if it hurts
things...

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170112/107cfe4f/attachment.sig>