<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd"> <html><head><meta name="qrichtext" content="1" /><style type="text/css"> p, li { white-space: pre-wrap; } </style></head><body style=" font-family:'Monospace'; font-size:9pt; font-weight:400; font-style:normal;"> Hi Matt, On Monday, July 13, 2015 16:22:03 Matt Turner wrote: > ARB_viewport_array specifies that DEPTH_RANGE consists of double- > precision parameters (corresponding commit d4dc35987), and a preparatory > commit (6340e609a) added _mesa_get_viewport_xform() which returned > double-precision scale[3] and translate[3] vectors, even though X, Y, > Width, and Height were still floats. > > All users of _mesa_get_viewport_xform() immediately convert the double > scale and translation vectors into floats (which were floats originally, > but were converted to doubles in _mesa_get_viewport_xform(), sigh). > > i965 at least cannot consume doubles (see SF_CLIP_VIEWPORT). If we want > to pass doubles to hardware, we should have a different function that > does that. The aim was to route the precision provided with the depth range one stage further down to the hardware. At the current time we do not support a single piece of hardware that takes doubles at that point, right? There is a comment inline below. I am not sure if I am in the position to review, but at least you can get my Acked-by: Mathias Froehlich <Mathias.Froehlich@web.de> Mathias > --- > src/mesa/drivers/dri/i915/i915_state.c | 2 +- > src/mesa/drivers/dri/i965/brw_sf_state.c | 2 +- > src/mesa/drivers/dri/i965/gen6_viewport_state.c | 2 +- > src/mesa/drivers/dri/i965/gen7_viewport_state.c | 2 +- > src/mesa/drivers/dri/i965/gen8_viewport_state.c | 2 +- > src/mesa/drivers/dri/r200/r200_state.c | 2 +- > src/mesa/drivers/dri/radeon/radeon_state.c | 2 +- > src/mesa/main/viewport.c | 14 +++++++------- > src/mesa/main/viewport.h | 2 +- > src/mesa/math/m_matrix.c | 4 ++-- > src/mesa/math/m_matrix.h | 4 ++-- > src/mesa/state_tracker/st_atom_viewport.c | 2 +- > src/mesa/tnl/t_context.c | 2 +- > src/mesa/tnl/t_rasterpos.c | 2 +- > 14 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/src/mesa/drivers/dri/i915/i915_state.c b/src/mesa/drivers/dri/i915/i915_state.c > index 5f10b84..4c83073 100644 > --- a/src/mesa/drivers/dri/i915/i915_state.c > +++ b/src/mesa/drivers/dri/i915/i915_state.c > @@ -402,7 +402,7 @@ void > intelCalcViewport(struct gl_context * ctx) > { > struct intel_context *intel = intel_context(ctx); > - double scale[3], translate[3]; > + float scale[3], translate[3]; > > _mesa_get_viewport_xform(ctx, 0, scale, translate); > > diff --git a/src/mesa/drivers/dri/i965/brw_sf_state.c b/src/mesa/drivers/dri/i965/brw_sf_state.c > index 5d98922..3be6e4a 100644 > --- a/src/mesa/drivers/dri/i965/brw_sf_state.c > +++ b/src/mesa/drivers/dri/i965/brw_sf_state.c > @@ -45,7 +45,7 @@ static void upload_sf_vp(struct brw_context *brw) > struct gl_context *ctx = &brw->ctx; > struct brw_sf_viewport *sfv; > GLfloat y_scale, y_bias; > - double scale[3], translate[3]; > + float scale[3], translate[3]; > const bool render_to_fbo = _mesa_is_user_fbo(ctx->DrawBuffer); > > sfv = brw_state_batch(brw, AUB_TRACE_SF_VP_STATE, > diff --git a/src/mesa/drivers/dri/i965/gen6_viewport_state.c b/src/mesa/drivers/dri/i965/gen6_viewport_state.c > index 7c8d884..11b9a36 100644 > --- a/src/mesa/drivers/dri/i965/gen6_viewport_state.c > +++ b/src/mesa/drivers/dri/i965/gen6_viewport_state.c > @@ -101,7 +101,7 @@ gen6_upload_sf_vp(struct brw_context *brw) > } > > for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) { > - double scale[3], translate[3]; > + float scale[3], translate[3]; > > /* _NEW_VIEWPORT */ > _mesa_get_viewport_xform(ctx, i, scale, translate); > diff --git a/src/mesa/drivers/dri/i965/gen7_viewport_state.c b/src/mesa/drivers/dri/i965/gen7_viewport_state.c > index b655205..c75dc99 100644 > --- a/src/mesa/drivers/dri/i965/gen7_viewport_state.c > +++ b/src/mesa/drivers/dri/i965/gen7_viewport_state.c > @@ -53,7 +53,7 @@ gen7_upload_sf_clip_viewport(struct brw_context *brw) > } > > for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) { > - double scale[3], translate[3]; > + float scale[3], translate[3]; > _mesa_get_viewport_xform(ctx, i, scale, translate); > > /* According to the "Vertex X,Y Clamping and Quantization" section of > diff --git a/src/mesa/drivers/dri/i965/gen8_viewport_state.c b/src/mesa/drivers/dri/i965/gen8_viewport_state.c > index 2d8eeb1..2692ad5 100644 > --- a/src/mesa/drivers/dri/i965/gen8_viewport_state.c > +++ b/src/mesa/drivers/dri/i965/gen8_viewport_state.c > @@ -53,7 +53,7 @@ gen8_upload_sf_clip_viewport(struct brw_context *brw) > } > > for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) { > - double scale[3], translate[3]; > + float scale[3], translate[3]; > _mesa_get_viewport_xform(ctx, i, scale, translate); > > /* _NEW_VIEWPORT: Viewport Matrix Elements */ > diff --git a/src/mesa/drivers/dri/r200/r200_state.c b/src/mesa/drivers/dri/r200/r200_state.c > index 6fe70b5..68485dd 100644 > --- a/src/mesa/drivers/dri/r200/r200_state.c > +++ b/src/mesa/drivers/dri/r200/r200_state.c > @@ -1546,7 +1546,7 @@ void r200UpdateWindow( struct gl_context *ctx ) > GLfloat xoffset = 0; > GLfloat yoffset = dPriv ? (GLfloat) dPriv->h : 0; > const GLboolean render_to_fbo = (ctx->DrawBuffer ? _mesa_is_user_fbo(ctx->DrawBuffer) : 0); > - double scale[3], translate[3]; > + float scale[3], translate[3]; > GLfloat y_scale, y_bias; > > if (render_to_fbo) { > diff --git a/src/mesa/drivers/dri/radeon/radeon_state.c b/src/mesa/drivers/dri/radeon/radeon_state.c > index cba3d9c..2c0926a 100644 > --- a/src/mesa/drivers/dri/radeon/radeon_state.c > +++ b/src/mesa/drivers/dri/radeon/radeon_state.c > @@ -1354,7 +1354,7 @@ void radeonUpdateWindow( struct gl_context *ctx ) > GLfloat xoffset = 0.0; > GLfloat yoffset = dPriv ? (GLfloat) dPriv->h : 0; > const GLboolean render_to_fbo = (ctx->DrawBuffer ? _mesa_is_user_fbo(ctx->DrawBuffer) : 0); > - double scale[3], translate[3]; > + float scale[3], translate[3]; > GLfloat y_scale, y_bias; > > if (render_to_fbo) { > diff --git a/src/mesa/main/viewport.c b/src/mesa/main/viewport.c > index b270630..d7849e0 100644 > --- a/src/mesa/main/viewport.c > +++ b/src/mesa/main/viewport.c > @@ -443,12 +443,12 @@ _mesa_ClipControl(GLenum origin, GLenum depth) > */ > void > _mesa_get_viewport_xform(struct gl_context *ctx, unsigned i, > - double scale[3], double translate[3]) > + float scale[3], float translate[3]) > { > - double x = ctx->ViewportArray[i].X; > - double y = ctx->ViewportArray[i].Y; > - double half_width = 0.5*ctx->ViewportArray[i].Width; > - double half_height = 0.5*ctx->ViewportArray[i].Height; > + float x = ctx->ViewportArray[i].X; > + float y = ctx->ViewportArray[i].Y; > + float half_width = 0.5f * ctx->ViewportArray[i].Width; > + float half_height = 0.5f * ctx->ViewportArray[i].Height; > double n = ctx->ViewportArray[i].Near; > double f = ctx->ViewportArray[i].Far; May be you can place a comment here that n and f are doubles in the viewport definition and should stay doubles here. The point in that is that we have then more headroom for cancelation problems that can happen while summing these two up below. The major use case here that is checked with the clip-control-depth-precision test should even work with floats through the whole stack as all the numbers in question for that use case are exactly representable even with float precision. But there may be use cases, I don't know of, which rely on these operations happening with higher than float precision. Just to do a double check: Can you confirm that the clip-control-depth-precision piglit test still passes? > @@ -462,8 +462,8 @@ _mesa_get_viewport_xform(struct gl_context *ctx, unsigned i, > translate[1] = half_height + y; > } > if (ctx->Transform.ClipDepthMode == GL_NEGATIVE_ONE_TO_ONE) { > - scale[2] = 0.5*(f - n); > - translate[2] = 0.5*(n + f); > + scale[2] = 0.5f * (f - n); > + translate[2] = 0.5f * (n + f); Since n and f are doubles the 0.5f will promote to double again for this operation. > } else { > scale[2] = f - n; > translate[2] = n; > diff --git a/src/mesa/main/viewport.h b/src/mesa/main/viewport.h > index 899dc2d..b0675db 100644 > --- a/src/mesa/main/viewport.h > +++ b/src/mesa/main/viewport.h > @@ -73,6 +73,6 @@ _mesa_ClipControl(GLenum origin, GLenum depth); > > extern void > _mesa_get_viewport_xform(struct gl_context *ctx, unsigned i, > - double scale[3], double translate[3]); > + float scale[3], float translate[3]); > > #endif > diff --git a/src/mesa/math/m_matrix.c b/src/mesa/math/m_matrix.c > index ecf564c..6a42c6c 100644 > --- a/src/mesa/math/m_matrix.c > +++ b/src/mesa/math/m_matrix.c > @@ -1111,8 +1111,8 @@ _math_matrix_translate( GLmatrix *mat, GLfloat x, GLfloat y, GLfloat z ) > * Transforms Normalized Device Coords to window/Z values. > */ > void > -_math_matrix_viewport(GLmatrix *m, const double scale[3], > - const double translate[3], double depthMax) > +_math_matrix_viewport(GLmatrix *m, const float scale[3], > + const float translate[3], double depthMax) > { > m->m[MAT_SX] = scale[0]; > m->m[MAT_TX] = translate[0]; > diff --git a/src/mesa/math/m_matrix.h b/src/mesa/math/m_matrix.h > index 778d716..c34d9e3 100644 > --- a/src/mesa/math/m_matrix.h > +++ b/src/mesa/math/m_matrix.h > @@ -122,8 +122,8 @@ _math_matrix_frustum( GLmatrix *mat, > GLfloat nearval, GLfloat farval ); > > extern void > -_math_matrix_viewport( GLmatrix *m, const double scale[3], > - const double translate[3], double depthMax ); > +_math_matrix_viewport( GLmatrix *m, const float scale[3], > + const float translate[3], double depthMax ); > > extern void > _math_matrix_set_identity( GLmatrix *dest ); > diff --git a/src/mesa/state_tracker/st_atom_viewport.c b/src/mesa/state_tracker/st_atom_viewport.c > index 2f62590..9a692ce 100644 > --- a/src/mesa/state_tracker/st_atom_viewport.c > +++ b/src/mesa/state_tracker/st_atom_viewport.c > @@ -64,7 +64,7 @@ update_viewport( struct st_context *st ) > */ > for (i = 0; i < ctx->Const.MaxViewports; i++) > { > - double scale[3], translate[3]; > + float scale[3], translate[3]; > _mesa_get_viewport_xform(ctx, i, scale, translate); > > st->state.viewport[i].scale[0] = scale[0]; > diff --git a/src/mesa/tnl/t_context.c b/src/mesa/tnl/t_context.c > index bc77ba8..b5c0b3e 100644 > --- a/src/mesa/tnl/t_context.c > +++ b/src/mesa/tnl/t_context.c > @@ -190,7 +190,7 @@ _tnl_InvalidateState( struct gl_context *ctx, GLuint new_state ) > } > > if (new_state & (_NEW_VIEWPORT | _NEW_BUFFERS)) { > - double scale[3], translate[3]; > + float scale[3], translate[3]; > _mesa_get_viewport_xform(ctx, 0, scale, translate); > _math_matrix_viewport(&tnl->_WindowMap, scale, translate, > ctx->DrawBuffer->_DepthMaxF); > diff --git a/src/mesa/tnl/t_rasterpos.c b/src/mesa/tnl/t_rasterpos.c > index d4b45ba..7ef50ea 100644 > --- a/src/mesa/tnl/t_rasterpos.c > +++ b/src/mesa/tnl/t_rasterpos.c > @@ -378,7 +378,7 @@ _tnl_RasterPos(struct gl_context *ctx, const GLfloat vObj[4]) > GLfloat eye[4], clip[4], ndc[3], d; > GLfloat *norm, eyenorm[3]; > GLfloat *objnorm = ctx->Current.Attrib[VERT_ATTRIB_NORMAL]; > - double scale[3], translate[3]; > + float scale[3], translate[3]; > > /* apply modelview matrix: eye = MV * obj */ > TRANSFORM_POINT( eye, ctx->ModelviewMatrixStack.Top->m, vObj ); > </body></html>