[Mesa-dev] [PATCH] i965/gen5: Fix rendering of depth buffers without stencil

[Kenneth Graunke]

On 01/17/2012 04:03 PM, Chad Versace wrote:
> Fixes the following OGLConform tests on gen5:
>      depth-stencil(misc.state_on.depth_int)
>      fbo_db_ARBfp(basic.OnlyDepthBuffDrawBufferRender)
>
> The problem was that, if the depth buffer's Mesa format was X8_Z24, then
> we emitted the hardware format D24_UNORM_X8. But, on gen5, D24_UNORM_S8
> must be emitted.
>
> This bug was introduced by:
>      commit d84a180417d1eabd680554970f1eaaa93abcd41e
>      Author: Eric Anholt<eric at anholt.net>
>      i965: Base HW depth format setup based on MESA_FORMAT, not bpp.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43408
> Reported-by: xunx.fang at intel.com
> Note: This is a candidate for the 8.0 branch.
> Signed-off-by: Chad Versace<chad.versace at linux.intel.com>
> ---
>   src/mesa/drivers/dri/i965/brw_misc_state.c |   16 +++++++++++++---
>   1 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
> index b6bca4b..852c770 100644
> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> @@ -223,10 +223,20 @@ brw_depthbuffer_format(struct brw_context *brw)
>      case MESA_FORMAT_Z32_FLOAT:
>         return BRW_DEPTHFORMAT_D32_FLOAT;
>      case MESA_FORMAT_X8_Z24:
> -      if (intel->gen>= 5)
> -	 return BRW_DEPTHFORMAT_D24_UNORM_X8_UINT;
> -      else /* Gen4 doesn't support X8; use S8 instead. */
> +      if (brw->intel.gen<  6) {

Please just use intel->gen here (like the existing code).  I'd also be 
inclined to format this as:

       if (intel->gen >= 6)
          return BRW_DEPTHFORMAT_D24_UNORM_X8_UINT;

       /* comment...
        */
       return BRW_DEPTHFORMAT_D24_UNORM_S8_UINT;

so the forward looking code is at the top.  But that's bike-shedding, 
and I don't really care.  You can also include "From the Ironlake PRM, 
Volume 2, Part 1, page 326:" if you wish.

Assuming you make the brw.intel->gen => intel->gen change:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks for fixing this.  I totally missed that text.

> +	 /* Use D24_UNORM_S8, not D24_UNORM_X8.
> +	  *
> +	  * D24_UNORM_X8 was not introduced until Gen5 (See field
> +	  * 3DSTATE_DEPTH_BUFFER.Surface_Format in the PRM).
> +	  *
> +	  * However, on Gen5, D24_UNORM_X8 may be used only if separate
> +	  * stencil is enabled, and we never enable it.  (See field
> +	  * 3DSTATE_DEPTH_BUFFER.Separate_Stencil_Buffer_Enable),
> +	  */
>   	 return BRW_DEPTHFORMAT_D24_UNORM_S8_UINT;
> +      } else {
> +	 return BRW_DEPTHFORMAT_D24_UNORM_X8_UINT;
> +      }
>      case MESA_FORMAT_S8_Z24:
>         return BRW_DEPTHFORMAT_D24_UNORM_S8_UINT;
>      case MESA_FORMAT_Z32_FLOAT_X24S8: