On Apr 12, 2016 8:46 AM, "Marek Olšák" <mar...@gmail.com> wrote:
>
> On Tue, Apr 12, 2016 at 1:53 AM, Marek Olšák <mar...@gmail.com> wrote:
> > On Mon, Apr 11, 2016 at 7:37 PM, Nicolai Hähnle <nhaeh...@gmail.com>
wrote:
> >> On 10.04.2016 17:34, Marek Olšák wrote:
> >>>
> >>> From: Marek Olšák <marek.ol...@amd.com>
> >>>
> >>> 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 <g...@chown.ath.cx>.
> >>> ---
> >>> 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
