On Sun, Jan 22, 2017 at 10:42:17PM -0800, Kenneth Graunke wrote:
> From: Jason Ekstrand <ja...@jlekstrand.net>
>
> (Patch co-authored by Jason and Ken.)
>
> We scaled the guardband based on the viewport size, but failed to
> take into account the translation portion of the viewport transform.
>
> This meant the guardband was always centered around the origin.
> We want it to be centered around the screen-space drawing area,
> which is the intersection of the viewport and the render target.
>
> At best, getting this wrong would reduce the guardband's effectiveness
> in some cases. At worst, it might break things - objects outside of the
> guardband are trivially rejected, so getting the guardband in the wrong
> place and leaving guardband clipping enabled could cause problems.
>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
> ---
> src/mesa/drivers/dri/i965/brw_state.h | 5 +
> src/mesa/drivers/dri/i965/gen6_viewport_state.c | 129
> ++++++++++++++++++++----
> src/mesa/drivers/dri/i965/gen7_viewport_state.c | 39 +++----
> src/mesa/drivers/dri/i965/gen8_viewport_state.c | 48 ++-------
> 4 files changed, 139 insertions(+), 82 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_state.h
> b/src/mesa/drivers/dri/i965/brw_state.h
> index ec6006c3fc6..36307c7ef90 100644
> --- a/src/mesa/drivers/dri/i965/brw_state.h
> +++ b/src/mesa/drivers/dri/i965/brw_state.h
> @@ -454,6 +454,11 @@ use_state_point_size(const struct brw_context *brw)
> (brw->vue_map_geom_out.slots_valid & VARYING_BIT_PSIZ) == 0;
> }
>
> +void brw_calculate_guardband_size(const struct gen_device_info *devinfo,
> + uint32_t fb_width, uint32_t fb_height,
> + float m00, float m11, float m30, float m31,
> + float *xmin, float *xmax,
> + float *ymin, float *ymax);
>
> #ifdef __cplusplus
> }
> diff --git a/src/mesa/drivers/dri/i965/gen6_viewport_state.c
> b/src/mesa/drivers/dri/i965/gen6_viewport_state.c
> index 2e08f1a1290..3968456413e 100644
> --- a/src/mesa/drivers/dri/i965/gen6_viewport_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_viewport_state.c
> @@ -33,18 +33,118 @@
> #include "main/framebuffer.h"
> #include "main/viewport.h"
>
> +void
> +brw_calculate_guardband_size(const struct gen_device_info *devinfo,
> + uint32_t fb_width, uint32_t fb_height,
> + float m00, float m11, float m30, float m31,
> + float *xmin, float *xmax,
> + float *ymin, float *ymax)
> +{
> + /* According to the "Vertex X,Y Clamping and Quantization" section of the
> + * Strips and Fans documentation:
> + *
> + * "The vertex X and Y screen-space coordinates are also /clamped/ to the
> + * fixed-point "guardband" range supported by the rasterization hardware"
> + *
> + * and
> + *
> + * "In almost all circumstances, if an object???s vertices are actually
> + * modified by this clamping (i.e., had X or Y coordinates outside of
> + * the guardband extent the rendered object will not match the intended
> + * result. Therefore software should take steps to ensure that this does
> + * not happen - e.g., by clipping objects such that they do not exceed
> + * these limits after the Drawing Rectangle is applied."
> + *
> + * I believe the fundamental restriction is that the rasterizer (in
> + * the SF/WM stages) have a limit on the number of pixels that can be
> + * rasterized. We need to ensure any coordinates beyond the rasterizer
> + * limit are handled by the clipper. So effectively that limit becomes
> + * the clipper's guardband size.
> + *
> + * It goes on to say:
> + *
> + * "In addition, in order to be correctly rendered, objects must have a
> + * screenspace bounding box not exceeding 8K in the X or Y direction.
> + * This additional restriction must also be comprehended by software,
> + * i.e., enforced by use of clipping."
> + *
> + * This makes no sense. Gen7+ hardware supports 16K render targets,
> + * and you definitely need to be able to draw polygons that fill the
> + * surface. Our assumption is that the rasterizer was limited to 8K
> + * on Sandybridge, which only supports 8K surfaces, and it was actually
> + * increased to 16K on Ivybridge and later.
> + *
> + * So, limit the guardband to 16K on Gen7+ and 8K on Sandybridge.
> + *
> + * The SF_CLIP_VIEWPORT documentation also restricts these fields
> + * (related to WaGuardbandSize, which seems to have something to do
> + * with fixed point math precision).
> + *
> + * For XMin/YMin, it says:
> + * "Minimum allowed value for this field is -16384."
> + *
> + * For XMax/YMax, it says:
> + * "Maximum allowed value for this field is 16383."
> + *
> + * I'm choosing to interpret this as a screenspace limit rather than
> + * a limit on the resulting NDC coordinates. It's not entirely clear.
> + */
> + const float gb_size = devinfo->gen >= 7 ? 16384.0f : 8192.0f;
> +
> + if (m00 != 0 && m11 != 0) {
> + /* First, we compute the screen-space render area */
> + const float ss_ra_xmin = MIN3( 0, m30 + m00, m30 - m00);
> + const float ss_ra_xmax = MAX3( fb_width, m30 + m00, m30 - m00);
> + const float ss_ra_ymin = MIN3( 0, m31 + m11, m31 - m11);
> + const float ss_ra_ymax = MAX3(fb_height, m31 + m11, m31 - m11);
> +
> + /* We want the guardband to be centered on that */
> + const float ss_gb_xmin = (ss_ra_xmin + ss_ra_xmax) / 2 - gb_size;
> + const float ss_gb_xmax = (ss_ra_xmin + ss_ra_xmax) / 2 + gb_size;
> + const float ss_gb_ymin = (ss_ra_ymin + ss_ra_ymax) / 2 - gb_size;
> + const float ss_gb_ymax = (ss_ra_ymin + ss_ra_ymax) / 2 + gb_size;
I need to start asking questions: Here we seem to apply the 16k (or gen6 8k)
in both directions: [center - 16k, center + 16k]. So the actual box size is
32k? Is this intention?
> +
> + /* Now we need it in native device coordinates */
> + const float ndc_gb_xmin = (ss_gb_xmin - m30) / m00;
> + const float ndc_gb_xmax = (ss_gb_xmax - m30) / m00;
> + const float ndc_gb_ymin = (ss_gb_ymin - m31) / m11;
> + const float ndc_gb_ymax = (ss_gb_ymax - m31) / m11;
Here we seem to get 2x size as well, m00 is half width and m11 half height.
Is this the intention?
> +
> + /* Thanks to Y-flipping and ORIGIN_UPPER_LEFT, the Y coordinates may be
> + * flipped upside-down. X should be fine though.
> + */
> + assert(ndc_gb_xmin <= ndc_gb_xmax);
> + *xmin = ndc_gb_xmin;
> + *xmax = ndc_gb_xmax;
> + *ymin = MIN2(ndc_gb_ymin, ndc_gb_ymax);
> + *ymax = MIN2(MAX2(ndc_gb_ymin, ndc_gb_ymax), 16383);
> + } else {
> + /* The viewport scales to 0, so nothing will be rendered. */
> + *xmin = 0.0f;
> + *xmax = 0.0f;
> + *ymin = 0.0f;
> + *ymax = 0.0f;
> + }
> +}
> +
> static void
> gen6_upload_sf_and_clip_viewports(struct brw_context *brw)
> {
> struct gl_context *ctx = &brw->ctx;
> + const struct gen_device_info *devinfo = &brw->screen->devinfo;
> struct gen6_sf_viewport *sfv;
> struct brw_clipper_viewport *clv;
> GLfloat y_scale, y_bias;
> - const bool render_to_fbo = _mesa_is_user_fbo(ctx->DrawBuffer);
>
> /* BRW_NEW_VIEWPORT_COUNT */
> const unsigned viewport_count = brw->clip.viewport_count;
>
> + /* _NEW_BUFFERS */
> + struct gl_framebuffer *fb = ctx->DrawBuffer;
> + const bool render_to_fbo = _mesa_is_user_fbo(fb);
> + const uint32_t fb_width = _mesa_geometric_width(ctx->DrawBuffer);
> + const uint32_t fb_height = _mesa_geometric_height(ctx->DrawBuffer);
> +
> sfv = brw_state_batch(brw, AUB_TRACE_SF_VP_STATE,
> sizeof(*sfv) * viewport_count,
> 32, &brw->sf.vp_offset);
> @@ -54,13 +154,12 @@ gen6_upload_sf_and_clip_viewports(struct brw_context
> *brw)
> sizeof(*clv) * viewport_count,
> 32, &brw->clip.vp_offset);
>
> - /* _NEW_BUFFERS */
> if (render_to_fbo) {
> y_scale = 1.0;
> y_bias = 0.0;
> } else {
> y_scale = -1.0;
> - y_bias = (float)_mesa_geometric_height(ctx->DrawBuffer);
> + y_bias = (float)fb_height;
> }
>
> for (unsigned i = 0; i < viewport_count; i++) {
> @@ -75,25 +174,11 @@ gen6_upload_sf_and_clip_viewports(struct brw_context
> *brw)
> sfv[i].m31 = translate[1] * y_scale + y_bias;
> sfv[i].m32 = translate[2];
>
> - /* According to the "Vertex X,Y Clamping and Quantization" section of
> the
> - * Strips and Fans documentation, objects must not have a screen-space
> - * extents of over 8192 pixels, or they may be mis-rasterized. The
> maximum
> - * screen space coordinates of a small object may larger, but we have
> no
> - * way to enforce the object size other than through clipping.
> - *
> - * If you're surprised that we set clip to -gbx to +gbx and it seems
> like
> - * we'll end up with 16384 wide, note that for a 8192-wide render
> target,
> - * we'll end up with a normal (-1, 1) clip volume that just covers the
> - * drawable.
> - */
> - const float maximum_post_clamp_delta = 8192;
> - float gbx = maximum_post_clamp_delta / ctx->ViewportArray[i].Width;
> - float gby = maximum_post_clamp_delta / ctx->ViewportArray[i].Height;
> -
> - clv[i].xmin = -gbx;
> - clv[i].xmax = gbx;
> - clv[i].ymin = -gby;
> - clv[i].ymax = gby;
> + brw_calculate_guardband_size(devinfo, fb_width, fb_height,
> + sfv[i].m00, sfv[i].m11,
> + sfv[i].m30, sfv[i].m31,
> + &clv[i].xmin, &clv[i].xmax,
> + &clv[i].ymin, &clv[i].ymax);
> }
>
> brw->ctx.NewDriverState |= BRW_NEW_SF_VP | BRW_NEW_CLIP_VP;
> diff --git a/src/mesa/drivers/dri/i965/gen7_viewport_state.c
> b/src/mesa/drivers/dri/i965/gen7_viewport_state.c
> index c447331a2e5..000f238f3fe 100644
> --- a/src/mesa/drivers/dri/i965/gen7_viewport_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_viewport_state.c
> @@ -33,13 +33,19 @@ static void
> gen7_upload_sf_clip_viewport(struct brw_context *brw)
> {
> struct gl_context *ctx = &brw->ctx;
> + const struct gen_device_info *devinfo = &brw->screen->devinfo;
> GLfloat y_scale, y_bias;
> - const bool render_to_fbo = _mesa_is_user_fbo(ctx->DrawBuffer);
> struct gen7_sf_clip_viewport *vp;
>
> /* BRW_NEW_VIEWPORT_COUNT */
> const unsigned viewport_count = brw->clip.viewport_count;
>
> + /* _NEW_BUFFERS */
> + struct gl_framebuffer *fb = ctx->DrawBuffer;
> + const bool render_to_fbo = _mesa_is_user_fbo(fb);
> + const uint32_t fb_width = _mesa_geometric_width(ctx->DrawBuffer);
> + const uint32_t fb_height = _mesa_geometric_height(ctx->DrawBuffer);
> +
> vp = brw_state_batch(brw, AUB_TRACE_SF_VP_STATE,
> sizeof(*vp) * viewport_count, 64,
> &brw->sf.vp_offset);
> @@ -52,34 +58,13 @@ gen7_upload_sf_clip_viewport(struct brw_context *brw)
> y_bias = 0.0;
> } else {
> y_scale = -1.0;
> - y_bias = (float)_mesa_geometric_height(ctx->DrawBuffer);
> + y_bias = (float)fb_height;
> }
>
> for (unsigned i = 0; i < viewport_count; i++) {
> float scale[3], translate[3];
> _mesa_get_viewport_xform(ctx, i, scale, translate);
>
> - /* According to the "Vertex X,Y Clamping and Quantization" section of
> - * the Strips and Fans documentation, objects must not have a
> - * screen-space extents of over 8192 pixels, or they may be
> - * mis-rasterized. The maximum screen space coordinates of a small
> - * object may larger, but we have no way to enforce the object size
> - * other than through clipping.
> - *
> - * If you're surprised that we set clip to -gbx to +gbx and it seems
> - * like we'll end up with 16384 wide, note that for a 8192-wide render
> - * target, we'll end up with a normal (-1, 1) clip volume that just
> - * covers the drawable.
> - */
> - const float maximum_guardband_extent = 8192;
> - const float gbx = maximum_guardband_extent /
> ctx->ViewportArray[i].Width;
> - const float gby = maximum_guardband_extent /
> ctx->ViewportArray[i].Height;
> -
> - vp[i].guardband.xmin = -gbx;
> - vp[i].guardband.xmax = gbx;
> - vp[i].guardband.ymin = -gby;
> - vp[i].guardband.ymax = gby;
> -
> /* _NEW_VIEWPORT */
> vp[i].viewport.m00 = scale[0];
> vp[i].viewport.m11 = scale[1] * y_scale;
> @@ -87,6 +72,14 @@ gen7_upload_sf_clip_viewport(struct brw_context *brw)
> vp[i].viewport.m30 = translate[0];
> vp[i].viewport.m31 = translate[1] * y_scale + y_bias;
> vp[i].viewport.m32 = translate[2];
> +
> + brw_calculate_guardband_size(devinfo, fb_width, fb_height,
> + vp[i].viewport.m00, vp[i].viewport.m11,
> + vp[i].viewport.m30, vp[i].viewport.m31,
> + &vp[i].guardband.xmin,
> + &vp[i].guardband.xmax,
> + &vp[i].guardband.ymin,
> + &vp[i].guardband.ymax);
> }
>
> BEGIN_BATCH(2);
> diff --git a/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> index 84000e3a7e2..101ad2b110e 100644
> --- a/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_viewport_state.c
> @@ -33,13 +33,18 @@ static void
> gen8_upload_sf_clip_viewport(struct brw_context *brw)
> {
> struct gl_context *ctx = &brw->ctx;
> + const struct gen_device_info *devinfo = &brw->screen->devinfo;
> float y_scale, y_bias;
> - const float fb_height = (float)_mesa_geometric_height(ctx->DrawBuffer);
> - const bool render_to_fbo = _mesa_is_user_fbo(ctx->DrawBuffer);
>
> /* BRW_NEW_VIEWPORT_COUNT */
> const unsigned viewport_count = brw->clip.viewport_count;
>
> + /* _NEW_BUFFERS */
> + struct gl_framebuffer *fb = ctx->DrawBuffer;
> + const bool render_to_fbo = _mesa_is_user_fbo(fb);
> + const uint32_t fb_width = _mesa_geometric_width(ctx->DrawBuffer);
> + const uint32_t fb_height = _mesa_geometric_height(ctx->DrawBuffer);
> +
> float *vp = brw_state_batch(brw, AUB_TRACE_SF_VP_STATE,
> 16 * 4 * viewport_count,
> 64, &brw->sf.vp_offset);
> @@ -52,7 +57,7 @@ gen8_upload_sf_clip_viewport(struct brw_context *brw)
> y_bias = 0;
> } else {
> y_scale = -1.0;
> - y_bias = fb_height;
> + y_bias = (float)fb_height;
> }
>
> for (unsigned i = 0; i < viewport_count; i++) {
> @@ -71,40 +76,9 @@ gen8_upload_sf_clip_viewport(struct brw_context *brw)
> vp[6] = 0;
> vp[7] = 0;
>
> - /* According to the "Vertex X,Y Clamping and Quantization" section of
> the
> - * Strips and Fans documentation, objects must not have a screen-space
> - * extents of over 8192 pixels, or they may be mis-rasterized. The
> - * maximum screen space coordinates of a small object may larger, but
> we
> - * have no way to enforce the object size other than through clipping.
> - *
> - * The goal is to create the maximum sized guardband (8K x 8K) with the
> - * viewport rectangle in the center of the guardband. This looks weird
> - * because the hardware wants coordinates that are scaled to the
> viewport
> - * in NDC. In other words, an 8K x 8K viewport would have [-1,1] for X
> and Y.
> - * A 4K viewport would be [-2,2], 2K := [-4,4] etc.
> - *
> - * --------------------------------
> - * |Guardband |
> - * | |
> - * | ------------ |
> - * | |viewport | |
> - * | | | |
> - * | | | |
> - * | |__________| |
> - * | |
> - * | |
> - * |______________________________|
> - *
> - */
> - const float maximum_guardband_extent = 8192;
> - float gbx = maximum_guardband_extent / ctx->ViewportArray[i].Width;
> - float gby = maximum_guardband_extent / ctx->ViewportArray[i].Height;
> -
> - /* _NEW_VIEWPORT: Guardband Clipping */
> - vp[8] = -gbx; /* x-min */
> - vp[9] = gbx; /* x-max */
> - vp[10] = -gby; /* y-min */
> - vp[11] = gby; /* y-max */
> + brw_calculate_guardband_size(devinfo, fb_width, fb_height,
> + vp[0], vp[1], vp[3], vp[4],
> + &vp[8], &vp[9], &vp[10], &vp[11]);
>
> /* _NEW_VIEWPORT | _NEW_BUFFERS: Screen Space Viewport
> * The hardware will take the intersection of the drawing rectangle,
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
