[Mesa-dev] [PATCH 2/4] i965: Use a better guardband calculation.
Kenneth Graunke
kenneth at whitecape.org
Thu Jan 26 08:51:07 UTC 2017
On Thursday, January 26, 2017 10:18:32 AM PST Pohjolainen, Topi wrote:
> On Sun, Jan 22, 2017 at 10:42:17PM -0800, Kenneth Graunke wrote:
> > From: Jason Ekstrand <jason at 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 <kenneth at 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?
Yes - I think it's right, though I've been confused by this many times.
Let's say we have a 16384x16384 render target, and we've set the
viewport in the common manner - centered in the middle, and covering
the entire render target.
The viewport always has NDC coordinates of [-1.0, 1.0] - which is 2.0
units wide. Here's a picture:
(0, 0) (16384, 0)
<-1.0, -1.0> <1.0, -1.0>
+-------------------+
| ^ |
| | |
| 8192 | |
| v |
| X |
| |
| |
| |
| |
+-------------------+
(0, 16384) (16384, 16384)
< 1.0, -1.0> <1.0, 1.0 >
Starting with 32768 should give us something 2.0 units wide, fitting
the [-1.0, 1.0] range.
Note that we also did this before - we were trying to limit the
guardband to 8192 pixels, but we programmed it to [-8192, 8192]
before scaling.
Maybe I need to try and preserve Ben's picture and comments (deleted
below) somehow...
> > +
> > + /* 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,
>
-------------- 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/20170126/2f3722c0/attachment-0001.sig>
More information about the mesa-dev
mailing list