[Mesa-dev] [PATCH] i965/clear: Quantize the depth clear value based on the format

[Kenneth Graunke]

On Saturday, August 19, 2017 8:56:06 PM PDT Jason Ekstrand wrote:
> In f9fd976e8adba733b08d we changed the clear value to be stored as an
> isl_color_value.  This had the side-effect same clear value check is now
> happening directly between the f32[0] field of the isl_color_value and
> ctx->Depth.Clear.  This isn't what we want for two reasons.  One is that
> the comparison happens in floating point even for Z16 and Z24 formats.
> Worse than that, ctx->Depth.Clear is a double so, even for 32-bit float
> formats, we were comparing as doubles and not floats.  This means that
> the test basically always fails for anything other than 0.0f and 1.0f.
> This caused a slight performance regression in Lightsmark 2008 because
> it was using a depth clear value of 0.999 which can't be stored in a
> 32-bit float so we were doing unneeded resolves.
> 
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Bugzilla: https://bugs.freedesktop.org/101678
> ---
>  src/mesa/drivers/dri/i965/brw_clear.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_clear.c b/src/mesa/drivers/dri/i965/brw_clear.c
> index 5eb2423..b7bf03d 100644
> --- a/src/mesa/drivers/dri/i965/brw_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_clear.c
> @@ -159,12 +159,22 @@ brw_fast_clear_depth(struct gl_context *ctx)
>        break;
>     }
>  
> +   /* Quantize the clear value to what can be stored in the actual depth
> +    * buffer.  This makes the following check more accurate because it now
> +    * checks if the actual depth bits will match.  It also prevents us from
> +    * getting a too-accurate depth value during depth testing or when sampling
> +    * with HiZ enabled.
> +    */
> +   float clear_value =
> +      mt->format == MESA_FORMAT_Z_FLOAT32 ? ctx->Depth.Clear :
> +      (unsigned)(ctx->Depth.Clear * fb->_DepthMax) / (float)fb->_DepthMax;
> +
>     const uint32_t num_layers = depth_att->Layered ? depth_irb->layer_count : 1;
>  
>     /* If we're clearing to a new clear value, then we need to resolve any clear
>      * flags out of the HiZ buffer into the real depth buffer.
>      */
> -   if (mt->fast_clear_color.f32[0] != ctx->Depth.Clear) {
> +   if (mt->fast_clear_color.f32[0] != clear_value) {
>        for (uint32_t level = mt->first_level; level <= mt->last_level; level++) {
>           if (!intel_miptree_level_has_hiz(mt, level))
>              continue;
> @@ -201,7 +211,7 @@ brw_fast_clear_depth(struct gl_context *ctx)
>           }
>        }
>  
> -      mt->fast_clear_color.f32[0] = ctx->Depth.Clear;
> +      mt->fast_clear_color.f32[0] = clear_value;
>     }
>  
>     bool need_clear = false;
> 

This second version of the patch is:

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

(The one from 11 minutes earlier is bogus.)
-------------- 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/20170821/cc15ccbb/attachment.sig>