[Mesa-dev] [PATCH] i965: Stop lying about cpp and height of a stencil buffer.

[Chad Versace]

One comment.

On 04/09/2012 09:43 AM, Paul Berry wrote:
> When using a separate stencil buffer, i965 requires that the pitch of
> the buffer (in the 3DSTATE_STENCIL_BUFFER command) be specified as 2x
> the actual pitch.
> 
> Previously this was accomplished by doubling the "cpp" and "pitch"
> values stored in the intel_region data structure, and halving the
> height.  However, this was confusing, and it led to a subtle (but
> benign) bug: since a stencil buffer is W-tiled, its true height must
> be aligned to a multiple of 64; we were accidentally aligning its faux
> height to a multiple of 64, causing memory to be wasted.
> 
> Note that for window system stencil buffers, the X server also doubles
> the cpp and pitch values.  To facilitate fixing this X server bug in
> the future, we fix the cpp and pitch values we receive from the X
> server only if cpp has the "incorrect" value of 2.
> ---
>  src/mesa/drivers/dri/i965/brw_misc_state.c     |   11 ++++---
>  src/mesa/drivers/dri/i965/gen7_misc_state.c    |   16 ++++++++++-
>  src/mesa/drivers/dri/intel/intel_context.c     |   35 +++++++++++++++++------
>  src/mesa/drivers/dri/intel/intel_mipmap_tree.c |   24 ++--------------
>  src/mesa/drivers/dri/intel/intel_screen.c      |   24 ++++++++++------
>  5 files changed, 65 insertions(+), 45 deletions(-)
> 

[snip]

> --- a/src/mesa/drivers/dri/intel/intel_context.c
> +++ b/src/mesa/drivers/dri/intel/intel_context.c
> @@ -1251,17 +1251,34 @@ intel_process_dri2_buffer_with_separate_stencil(struct intel_context *intel,
>  
>     int buffer_width;
>     int buffer_height;
> +   int buffer_cpp = buffer->cpp;
> +   int buffer_pitch = buffer->pitch;
>     if (buffer->attachment == __DRI_BUFFER_STENCIL) {
> -      /* The stencil buffer has quirky pitch requirements.  From Section
> -       * 2.11.5.6.2.1 3DSTATE_STENCIL_BUFFER, field "Surface Pitch":
> -       *    The pitch must be set to 2x the value computed based on width, as
> -       *    the stencil buffer is stored with two rows interleaved.
> +      /* Stencil buffers use W tiling, a tiling format that the DRM functions
> +       * don't properly account for.  Therefore, when we allocate a stencil
> +       * buffer that is private to Mesa (see intel_miptree_create), we round
> +       * the height and width up to the next multiple of the tile size (64x64)
> +       * and then ask DRM to allocate an untiled buffer.  Consequently, the
> +       * height and the width stored in the stencil buffer's region structure
> +       * are always multiples of 64, even if the stencil buffer itself is
> +       * smaller.
>         *
> -       * To satisfy the pitch requirement, the X driver allocated the region
> -       * with the following dimensions.
> +       * To avoid inconsistencies between how we represent private buffers and
> +       * buffers shared with the window system, round up the height and width
> +       * for window system buffers too.
>         */
>         buffer_width = ALIGN(drawable->w, 64);
> -       buffer_height = ALIGN(ALIGN(drawable->h, 2) / 2, 64);
> +       buffer_height = ALIGN(drawable->h, 64);
> +
> +       /* As of 4/6/2012, the X server lies and sends cpp and pitch values

The Xserver doesn't do this. Intel's Xorg driver, aka xf86-video-intel, does. I would
instead say "Beginning in xf86-video-intel 2.16, the ddx driver lies...".
(DDX means "device-dependent X").

> +        * that are two times too large for stencil buffers.  Hopefully this
> +        * will be fixed someday, but for now detect the bug by checking if cpp
> +        * is 2, and fixing cpp and pitch if it is.
> +        */
> +       if (buffer_cpp == 2) {
> +          buffer_cpp = 1;
> +          buffer_pitch /= 2;
> +       }
>     } else {
>         buffer_width = drawable->w;
>         buffer_height = drawable->h;

Other than the Xserver/ddx confusion,
Reviewed-by: Chad Versace <chad.versace at linux.intel.com>