On Apr 12, 2016 8:46 AM, "Marek Olšák" <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: > > On Tue, Apr 12, 2016 at 1:53 AM, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: > > On Mon, Apr 11, 2016 at 7:37 PM, Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>> wrote: > >> On 10.04.2016 17:34, Marek Olšák wrote: > >>> > >>> From: Marek Olšák <<a href="mailto:marek.olsak@amd.com">marek.olsak@amd.com</a>> > >>> > >>> 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 <<a href="mailto:greg@chown.ath.cx">greg@chown.ath.cx</a>>. > >>> --- > >>> 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 > You might also consider using pipe_debug_message to report such invalid usage. That way you avoid the abort but still let the user know what's going on. -ilia