On Tue, Apr 12, 2016 at 1:53 AM, Marek Olšák <[email protected]> wrote: > On Mon, Apr 11, 2016 at 7:37 PM, Nicolai Hähnle <[email protected]> wrote: >> On 10.04.2016 17:34, Marek Olšák wrote: >>> >>> From: Marek Olšák <[email protected]> >>> >>> Guard band clipping speeds up rasterization for primitives that are >>> partially off-screen. This change in particular results in small >>> framerate improvements in a wide range of games. >>> >>> Started by Grigori Goronzy <[email protected]>. >>> --- >>> src/gallium/drivers/radeonsi/si_state.c | 73 >>> +++++++++++++++++++++++++++++++-- >>> 1 file changed, 69 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/gallium/drivers/radeonsi/si_state.c >>> b/src/gallium/drivers/radeonsi/si_state.c >>> index b5db456..bc04b46 100644 >>> --- a/src/gallium/drivers/radeonsi/si_state.c >>> +++ b/src/gallium/drivers/radeonsi/si_state.c >>> @@ -887,6 +887,15 @@ static void si_clip_scissor(struct pipe_scissor_state >>> *out, >>> out->maxy = MIN2(out->maxy, clip->maxy); >>> } >>> >>> +static void si_scissor_make_union(struct si_signed_scissor *out, >>> + struct si_signed_scissor *in) >>> +{ >>> + out->minx = MIN2(out->minx, in->minx); >>> + out->miny = MIN2(out->miny, in->miny); >>> + out->maxx = MAX2(out->maxx, in->maxx); >>> + out->maxy = MAX2(out->maxy, in->maxy); >>> +} >>> + >>> static void si_emit_one_scissor(struct radeon_winsys_cs *cs, >>> struct si_signed_scissor *vp_scissor, >>> struct pipe_scissor_state *scissor) >>> @@ -908,12 +917,64 @@ static void si_emit_one_scissor(struct >>> radeon_winsys_cs *cs, >>> S_028254_BR_Y(final.maxy)); >>> } >>> >>> +/* the range is [-MAX, MAX] */ >>> +#define SI_MAX_VIEWPORT_RANGE 32768 >>> + >>> +static void si_emit_guardband(struct si_context *sctx, >>> + struct si_signed_scissor *vp_as_scissor) >>> +{ >>> + struct radeon_winsys_cs *cs = sctx->b.gfx.cs; >>> + struct pipe_viewport_state vp; >>> + float left, top, right, bottom, max_range, guardband_x, >>> guardband_y; >>> + >>> + /* Reconstruct the viewport transformation from the scissor. */ >>> + vp.translate[0] = (vp_as_scissor->minx + vp_as_scissor->maxx) / >>> 2.0; >>> + vp.translate[1] = (vp_as_scissor->miny + vp_as_scissor->maxy) / >>> 2.0; >>> + vp.scale[0] = vp_as_scissor->maxx - vp.translate[0]; >>> + vp.scale[1] = vp_as_scissor->maxy - vp.translate[1]; >>> + >>> + /* Treat a 0x0 viewport as 1x1 to prevent division by zero. */ >>> + if (vp_as_scissor->minx == vp_as_scissor->maxx) >>> + vp.scale[0] = 0.5; >>> + if (vp_as_scissor->miny == vp_as_scissor->maxy) >>> + vp.scale[1] = 0.5; >>> + >>> + /* Find the biggest guard band that is inside the supported >>> viewport >>> + * range. The guard band is specified as a horizontal and vertical >>> + * distance from (0,0) in clip space. >>> + * >>> + * This is done by applying the inverse viewport transformation >>> + * on the viewport limits to get those limits in clip space. >>> + * >>> + * Use a limit one pixel smaller to allow for some precision >>> error. >>> + */ >>> + max_range = SI_MAX_VIEWPORT_RANGE - 1; >>> + left = (-max_range - vp.translate[0]) / vp.scale[0]; >>> + right = ( max_range - vp.translate[0]) / vp.scale[0]; >>> + top = (-max_range - vp.translate[1]) / vp.scale[1]; >>> + bottom = ( max_range - vp.translate[1]) / vp.scale[1]; >>> + >>> + assert(left < 0 && top < 0 && right > 0 && bottom > 0); >> >> >> The viewport transform is supplied directly by the user, isn't it? So if >> they provide a crazy transform, this assert might trigger. On the other >> hand, for reasonable viewport transforms, it should even be the case that >> left <= -1.0, right >= 1.0, and so on, right? I guess what I'm saying is >> that the spirit of the assert is good and could probably be even >> strengthened, but in reality it probably needs to go away. >> >> In a similar spirit, perhaps guardband_x/y should be bounded to be at least >> 1.0? > > glViewport is clamped to ctx->Const.ViewportBounds.Min/Max. This > assures that all guard bands are >= 1. However, if we get invalid > coordinates from other sources, the guard band must prevent generating
That should be "if we get invalid viewport transform from other sources". I think the assertion should stay and be strengthened to "x >= 1", because: - we don't know how the discard guard band interacts with the clip guard band - we should keep the assertion to prevent and detect possible corruption caused by invalid viewport transformation Does this sound good? Marek _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
