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

[Kenneth Graunke]

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.

Paul,

This seems like a nice clean-up.  I'm glad to see this.

I reviewed the code and it seems to make sense, but I'd really want to 
see Chad or Eric's review on it before it gets pushed.

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

One small comment below:

> diff --git a/src/mesa/drivers/dri/intel/intel_context.c b/src/mesa/drivers/dri/intel/intel_context.c
> index 16a9887..fc3258b 100644
> --- 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
> +        * 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;

I was going to suggest saying "xserver 1.12 lies and sends..." rather 
than using a date...but...I don't think it's the X server that's really 
at fault.  Isn't it xf86-video-intel (sometimes called the DDX)?

So, maybe "As of 4/6/2012, the DDX lies and sends cpp and pitch values"?

Sadly, I'm not sure how to fix that without breaking new DDX + old Mesa.