diff mbox

[2/2] dri: Add support for DRI2BufferStencil and DRI2BufferHiz

Message ID 1307232391-26973-3-git-send-email-chad@chad-versace.us
State New, archived
Headers show

Commit Message

Chad Versace June 5, 2011, 12:06 a.m. UTC
Before this commit, if a client were to request a stencil or hiz buffer, then
I830DRI2CreateBuffer() allocated and returned an X-tiled buffer by
accident. (DRI2BufferStencil and DRI2BufferHiz were unintentionally caught
by the default case of a switch statement.)

Now, I830DRI2CreateBuffer() correctly returns a Y-tiled buffer and handles
the stencil buffer as a special case due its quirky pitch requirements.

This shouldn't break older Mesa versions, because they never query (via
DRI2GetBuffersWithFormat) for the drawable's DRI2BufferStencil.

Signed-off-by: Chad Versace <chad@chad-versace.us>
---
 src/intel_dri.c |   30 ++++++++++++++++++++++++++----
 1 files changed, 26 insertions(+), 4 deletions(-)

Comments

Kenneth Graunke June 5, 2011, 8:56 a.m. UTC | #1
On 06/04/2011 05:06 PM, Chad Versace wrote:
> Before this commit, if a client were to request a stencil or hiz buffer, then
> I830DRI2CreateBuffer() allocated and returned an X-tiled buffer by
> accident. (DRI2BufferStencil and DRI2BufferHiz were unintentionally caught
> by the default case of a switch statement.)
>
> Now, I830DRI2CreateBuffer() correctly returns a Y-tiled buffer and handles
> the stencil buffer as a special case due its quirky pitch requirements.
>
> This shouldn't break older Mesa versions, because they never query (via
> DRI2GetBuffersWithFormat) for the drawable's DRI2BufferStencil.
>
> Signed-off-by: Chad Versace<chad@chad-versace.us>
> ---
>   src/intel_dri.c |   30 ++++++++++++++++++++++++++----
>   1 files changed, 26 insertions(+), 4 deletions(-)

This looks good to me, though I'm not too familiar with the DDX code.

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Chris Wilson June 5, 2011, 9:48 a.m. UTC | #2
On Sat,  4 Jun 2011 17:06:31 -0700, Chad Versace <chad@chad-versace.us> wrote:
> Before this commit, if a client were to request a stencil or hiz buffer, then
> I830DRI2CreateBuffer() allocated and returned an X-tiled buffer by
> accident. (DRI2BufferStencil and DRI2BufferHiz were unintentionally caught
> by the default case of a switch statement.)
> 
> Now, I830DRI2CreateBuffer() correctly returns a Y-tiled buffer and handles
> the stencil buffer as a special case due its quirky pitch requirements.

Yeah, this really highlights how broken the ddx is by conflating Pixmaps
and RenderBuffer attachments.

However as a short-term feature addition, it looks fine.
-Chris
Eric Anholt June 5, 2011, 5:10 p.m. UTC | #3
On Sat,  4 Jun 2011 17:06:31 -0700, Chad Versace <chad@chad-versace.us> wrote:
> Before this commit, if a client were to request a stencil or hiz buffer, then
> I830DRI2CreateBuffer() allocated and returned an X-tiled buffer by
> accident. (DRI2BufferStencil and DRI2BufferHiz were unintentionally caught
> by the default case of a switch statement.)
> 
> Now, I830DRI2CreateBuffer() correctly returns a Y-tiled buffer and handles
> the stencil buffer as a special case due its quirky pitch requirements.
> 
> This shouldn't break older Mesa versions, because they never query (via
> DRI2GetBuffersWithFormat) for the drawable's DRI2BufferStencil.
> 
> Signed-off-by: Chad Versace <chad@chad-versace.us>

This should make configure.ac depend on the bumped version of
dri2proto.h, so people get an explanation instead of just build failure.

Also, I'm surprised to see you didn't fix the "we'll allocate a buffer
for *anything*" behavior that made the compat path in Mesa strange this
time around.
Chad Versace June 6, 2011, 3:38 a.m. UTC | #4
On Sun, 05 Jun 2011 10:10:39 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Sat,  4 Jun 2011 17:06:31 -0700, Chad Versace <chad@chad-versace.us> wrote:
> > Before this commit, if a client were to request a stencil or hiz buffer, then
> > I830DRI2CreateBuffer() allocated and returned an X-tiled buffer by
> > accident. (DRI2BufferStencil and DRI2BufferHiz were unintentionally caught
> > by the default case of a switch statement.)
> > 
> > Now, I830DRI2CreateBuffer() correctly returns a Y-tiled buffer and handles
> > the stencil buffer as a special case due its quirky pitch requirements.
> > 
> > This shouldn't break older Mesa versions, because they never query (via
> > DRI2GetBuffersWithFormat) for the drawable's DRI2BufferStencil.
> > 
> > Signed-off-by: Chad Versace <chad@chad-versace.us>
> 
> This should make configure.ac depend on the bumped version of
> dri2proto.h, so people get an explanation instead of just build failure.
> 
> Also, I'm surprised to see you didn't fix the "we'll allocate a buffer
> for *anything*" behavior that made the compat path in Mesa strange this
> time around.

Eric, I've fixed the patch series according to your comments and resubmitted.

I don't know why I didn't fix the "we'll allocate anything you ask for"
problem; guess I was just too busy digging in other things.
diff mbox

Patch

diff --git a/src/intel_dri.c b/src/intel_dri.c
index 48d0f56..4bcec45 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -329,11 +329,16 @@  I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int attachment,
 		pixmap = get_front_buffer(drawable);
 	if (pixmap == NULL) {
 		unsigned int hint = INTEL_CREATE_PIXMAP_DRI2;
+		int pixmap_width = drawable->width;
+		int pixmap_height = drawable->height;
+		int pixmap_cpp = (format != 0) ? format : drawable->depth;
 
 		if (intel->tiling & INTEL_TILING_3D) {
 			switch (attachment) {
 			case DRI2BufferDepth:
 			case DRI2BufferDepthStencil:
+			case DRI2BufferStencil:
+			case DRI2BufferHiz:
 				if (SUPPORTS_YTILING(intel)) {
 					hint |= INTEL_CREATE_PIXMAP_TILING_Y;
 					break;
@@ -344,11 +349,28 @@  I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int attachment,
 			}
 		}
 
+		/*
+		 * The stencil buffer has quirky pitch requirements.  From Vol
+		 * 2a, 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.
+		 * To accomplish this, we resort to the nasty hack of doubling
+		 * the drm region's cpp and halving its height.
+		 *
+		 * If we neglect to double the pitch, then
+		 * drm_intel_gem_bo_map_gtt() maps the memory incorrectly.
+		 */
+		if (attachment == DRI2BufferStencil) {
+			pixmap_height /= 2;
+			pixmap_cpp *= 2;
+		}
+
 		pixmap = screen->CreatePixmap(screen,
-					      drawable->width,
-					      drawable->height,
-					      (format != 0) ? format :
-							      drawable->depth,
+					      pixmap_width,
+					      pixmap_height,
+					      pixmap_cpp,
 					      hint);
 		if (pixmap == NULL || intel_get_pixmap_bo(pixmap) == NULL) {
 			if (pixmap)