[Mesa-dev] [PATCH 01/13] mesa: Use floats for viewport bounds.

Wed Jul 15 12:34:13 PDT 2015

On Wed, Jul 15, 2015 at 12:48 AM, Mathias Fröhlich
<Mathias.Froehlich at gmx.net> wrote:
>
>
> 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?

I don't know about NVIDIA or AMD hardware, but Intel hardware cannot
take double-precision viewport bounds, and pipe_viewport_state (what
the code in st_atom_viewport.c copies the outputs from
_mesa_get_viewport_xform() into) defines scale and translate as
floats. Certainly there's no point in the conversion round trip for
hardware like i915, radeon, and r200.

My suggestion, if we want to pipe double-precision viewport bounds
from GL into capable hardware is to add a new function.

> 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 at web.de>

Thanks!

> 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?

Indeed, it still passes (on all i965).

>> @@ -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.

Thanks, indeed you're right. Fixed.