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 <[email protected]>
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.
_______________________________________________
mesa-dev mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
