Align tiled pixmap height so we don't address beyond the end of our buffers.
diff mbox

Message ID 1249694345-9228-1-git-send-email-eric@anholt.net
State Not Applicable
Headers show

Commit Message

Eric Anholt Aug. 8, 2009, 1:19 a.m. UTC
---
 src/i830.h        |    2 ++
 src/i830_uxa.c    |   11 +++++++++--
 src/i965_render.c |    2 --
 src/i965_video.c  |    3 ---
 4 files changed, 11 insertions(+), 7 deletions(-)

Comments

Ian Romanick Aug. 12, 2009, 9:55 p.m. UTC | #1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Eric Anholt wrote:

> diff --git a/src/i830_uxa.c b/src/i830_uxa.c
> index 3a476a7..1087128 100644
> --- a/src/i830_uxa.c
> +++ b/src/i830_uxa.c
> @@ -615,6 +615,13 @@ i830_uxa_create_pixmap (ScreenPtr screen, int w, int h, int depth, unsigned usag
>  	if (tiling == I915_TILING_NONE) {
>  	    size = stride * h;
>  	} else {
> +	    int aligned_h = h;
> +	    if (tiling == I915_TILING_X)
> +		aligned_h = ALIGN(h, 8);
> +	    else
> +		aligned_h = ALIGN(h, 16);
> +	    assert(aligned_h >= h);
> +

Looking at the other patch, should the else case be 'else if (tiling ==
I915_TILING_Y)'?

>  	    stride = i830_get_fence_pitch(i830, stride, tiling);
>  	    /* Round the object up to the size of the fence it will live in
>  	     * if necessary.  We could potentially make the kernel allocate
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqDOloACgkQX1gOwKyEAw8T7wCeLC4PkjNrqTjGX8kK9E16oria
sGIAn0C8lm9B6Zjo9LMjmGY8oQ7GkJSF
=Qa2G
-----END PGP SIGNATURE-----
Chris Wilson Aug. 12, 2009, 10:13 p.m. UTC | #2
On Wed, 2009-08-12 at 14:55 -0700, Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Eric Anholt wrote:
> 
> > diff --git a/src/i830_uxa.c b/src/i830_uxa.c
> > index 3a476a7..1087128 100644
> > --- a/src/i830_uxa.c
> > +++ b/src/i830_uxa.c
> > @@ -615,6 +615,13 @@ i830_uxa_create_pixmap (ScreenPtr screen, int w, int h, int depth, unsigned usag
> >  	if (tiling == I915_TILING_NONE) {
> >  	    size = stride * h;
> >  	} else {
> > +	    int aligned_h = h;
> > +	    if (tiling == I915_TILING_X)
> > +		aligned_h = ALIGN(h, 8);
> > +	    else
> > +		aligned_h = ALIGN(h, 16);
> > +	    assert(aligned_h >= h);
> > +
> 
> Looking at the other patch, should the else case be 'else if (tiling ==
> I915_TILING_Y)'?

Depends. Is there a secret undocumented tiling mode? ;-)

In seriousness though, according to the documentation [Vol 1a, p135]
even the NONE case should be ALIGN(h, 2).
-ickle
Eric Anholt Aug. 13, 2009, 2:01 a.m. UTC | #3
On Wed, 2009-08-12 at 23:13 +0100, Chris Wilson wrote:
> On Wed, 2009-08-12 at 14:55 -0700, Ian Romanick wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > Eric Anholt wrote:
> > 
> > > diff --git a/src/i830_uxa.c b/src/i830_uxa.c
> > > index 3a476a7..1087128 100644
> > > --- a/src/i830_uxa.c
> > > +++ b/src/i830_uxa.c
> > > @@ -615,6 +615,13 @@ i830_uxa_create_pixmap (ScreenPtr screen, int w, int h, int depth, unsigned usag
> > >  	if (tiling == I915_TILING_NONE) {
> > >  	    size = stride * h;
> > >  	} else {
> > > +	    int aligned_h = h;
> > > +	    if (tiling == I915_TILING_X)
> > > +		aligned_h = ALIGN(h, 8);
> > > +	    else
> > > +		aligned_h = ALIGN(h, 16);
> > > +	    assert(aligned_h >= h);
> > > +
> > 
> > Looking at the other patch, should the else case be 'else if (tiling ==
> > I915_TILING_Y)'?
> 
> Depends. Is there a secret undocumented tiling mode? ;-)
> 
> In seriousness though, according to the documentation [Vol 1a, p135]
> even the NONE case should be ALIGN(h, 2).

Good point.  That's actually the part of the docs I was looking at that
sparked me thinking to produce that bugfix!

Patch
diff mbox

diff --git a/src/i830.h b/src/i830.h
index 58afe76..b46eff1 100644
--- a/src/i830.h
+++ b/src/i830.h
@@ -619,6 +619,8 @@  typedef struct _I830Rec {
 #define I830PTR(p) ((I830Ptr)((p)->driverPrivate))
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))
+#define ALIGN(i,m)	(((i) + (m) - 1) & ~((m) - 1))
+#define MIN(a,b)	((a) < (b) ? (a) : (b))
 
 #define I830_SELECT_FRONT	0
 #define I830_SELECT_BACK	1
diff --git a/src/i830_uxa.c b/src/i830_uxa.c
index 3a476a7..1087128 100644
--- a/src/i830_uxa.c
+++ b/src/i830_uxa.c
@@ -615,6 +615,13 @@  i830_uxa_create_pixmap (ScreenPtr screen, int w, int h, int depth, unsigned usag
 	if (tiling == I915_TILING_NONE) {
 	    size = stride * h;
 	} else {
+	    int aligned_h = h;
+	    if (tiling == I915_TILING_X)
+		aligned_h = ALIGN(h, 8);
+	    else
+		aligned_h = ALIGN(h, 16);
+	    assert(aligned_h >= h);
+
 	    stride = i830_get_fence_pitch(i830, stride, tiling);
 	    /* Round the object up to the size of the fence it will live in
 	     * if necessary.  We could potentially make the kernel allocate
@@ -622,8 +629,8 @@  i830_uxa_create_pixmap (ScreenPtr screen, int w, int h, int depth, unsigned usag
 	     * but this is easier and also keeps us out of trouble (as much)
 	     * with drm_intel_bufmgr_check_aperture().
 	     */
-	    size = i830_get_fence_size(i830, stride * h);
-	    assert(size >= stride * h);
+	    size = i830_get_fence_size(i830, stride * aligned_h);
+	    assert(size >= stride * aligned_h);
 	}
 
 	/* Fail very large allocations on 32-bit systems.  Large BOs will
diff --git a/src/i965_render.c b/src/i965_render.c
index eeb23e1..1a8075b 100644
--- a/src/i965_render.c
+++ b/src/i965_render.c
@@ -251,8 +251,6 @@  i965_check_composite(int op, PicturePtr pSrcPicture, PicturePtr pMaskPicture,
 
 }
 
-#define ALIGN(i,m)    (((i) + (m) - 1) & ~((m) - 1))
-#define MIN(a,b) ((a) < (b) ? (a) : (b))
 #define BRW_GRF_BLOCKS(nreg)    ((nreg + 15) / 16 - 1)
 
 /* Set up a default static partitioning of the URB, which is supposed to
diff --git a/src/i965_video.c b/src/i965_video.c
index 805b33f..46a461f 100644
--- a/src/i965_video.c
+++ b/src/i965_video.c
@@ -131,9 +131,6 @@  static const uint32_t ps_kernel_planar_static_gen5[][4] = {
 #include "exa_wm_write.g4b.gen5"
 };
 
-#define ALIGN(i,m)    (((i) + (m) - 1) & ~((m) - 1))
-#define MIN(a,b) ((a) < (b) ? (a) : (b))
-
 static uint32_t float_to_uint (float f) {
     union {uint32_t i; float f;} x;
     x.f = f;