[PATCH 0/5] drm/i915: fix gtt space allocated for tiled objects

[PATCH 0/5] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

At the moment it's possible for user space to allocate a tiled buffer
with a size not aligned to its tile row size and corrupt a buffer that
follows this in the gtt space. More details on this in the last patch.

I'll also send an i-g-t test case for this.

Tested on Gen5,6.

Imre Deak (5):
  drm/i915: merge get_gtt_alignment/get_unfenced_gtt_alignment()
  drm/i915: merge {i965,sandybridge}_write_fence_reg()
  drm/i915: use gtt_get_size() instead of open coding it
  drm/i915: factor out i915_gem_get_tile_width()
  drm/i915: fix gtt space allocated for tiled objects

 drivers/gpu/drm/i915/i915_drv.h        |   12 ++-
 drivers/gpu/drm/i915/i915_gem.c        |  144 +++++++++++++++-----------------
 drivers/gpu/drm/i915/i915_gem_tiling.c |   34 +++-----
 3 files changed, 89 insertions(+), 101 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] drm/i915: merge get_gtt_alignment/get_unfenced_gtt_alignment()

[Imre Deak]

The two functions are rather similar, so merge them.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    5 ++--
 drivers/gpu/drm/i915/i915_gem.c        |   44 +++++---------------------------
 drivers/gpu/drm/i915/i915_gem_tiling.c |    6 ++---
 3 files changed, 12 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b724688..9b70728 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1577,9 +1577,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
-i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
-				    uint32_t size,
-				    int tiling_mode);
+i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
+			    int tiling_mode, bool fenced);
 
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 18fa2af..240f798 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1491,16 +1491,15 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
  * Return the required GTT alignment for an object, taking into account
  * potential fence register mapping.
  */
-static uint32_t
-i915_gem_get_gtt_alignment(struct drm_device *dev,
-			   uint32_t size,
-			   int tiling_mode)
+uint32_t
+i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
+			   int tiling_mode, bool fenced)
 {
 	/*
 	 * Minimum alignment is 4k (GTT page size), but might be greater
 	 * if a fence register is needed for the object.
 	 */
-	if (INTEL_INFO(dev)->gen >= 4 ||
+	if (INTEL_INFO(dev)->gen >= 4 || (!fenced && IS_G33(dev)) ||
 	    tiling_mode == I915_TILING_NONE)
 		return 4096;
 
@@ -1511,35 +1510,6 @@ i915_gem_get_gtt_alignment(struct drm_device *dev,
 	return i915_gem_get_gtt_size(dev, size, tiling_mode);
 }
 
-/**
- * i915_gem_get_unfenced_gtt_alignment - return required GTT alignment for an
- *					 unfenced object
- * @dev: the device
- * @size: size of the object
- * @tiling_mode: tiling mode of the object
- *
- * Return the required GTT alignment for an object, only taking into account
- * unfenced tiled surface requirements.
- */
-uint32_t
-i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
-				    uint32_t size,
-				    int tiling_mode)
-{
-	/*
-	 * Minimum alignment is 4k (GTT page size) for sane hw.
-	 */
-	if (INTEL_INFO(dev)->gen >= 4 || IS_G33(dev) ||
-	    tiling_mode == I915_TILING_NONE)
-		return 4096;
-
-	/* Previous hardware however needs to be aligned to a power-of-two
-	 * tile height. The simplest method for determining this is to reuse
-	 * the power-of-tile object size.
-	 */
-	return i915_gem_get_gtt_size(dev, size, tiling_mode);
-}
-
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
@@ -2966,11 +2936,11 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 					   obj->tiling_mode);
 	fence_alignment = i915_gem_get_gtt_alignment(dev,
 						     obj->base.size,
-						     obj->tiling_mode);
+						     obj->tiling_mode, true);
 	unfenced_alignment =
-		i915_gem_get_unfenced_gtt_alignment(dev,
+		i915_gem_get_gtt_alignment(dev,
 						    obj->base.size,
-						    obj->tiling_mode);
+						    obj->tiling_mode, false);
 
 	if (alignment == 0)
 		alignment = map_and_fenceable ? fence_alignment :
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index c77e40a..02bb2bc 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -375,9 +375,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		/* Rebind if we need a change of alignment */
 		if (!obj->map_and_fenceable) {
 			u32 unfenced_alignment =
-				i915_gem_get_unfenced_gtt_alignment(dev,
-								    obj->base.size,
-								    args->tiling_mode);
+				i915_gem_get_gtt_alignment(dev, obj->base.size,
+							    args->tiling_mode,
+							    false);
 			if (obj->gtt_offset & (unfenced_alignment - 1))
 				ret = i915_gem_object_unbind(obj);
 		}
-- 
1.7.9.5

[PATCH 2/5] drm/i915: merge {i965, sandybridge}_write_fence_reg()

[Imre Deak]

The two functions are rather similar, so merge them.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   44 +++++++++++++--------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 240f798..b74e72f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2537,52 +2537,38 @@ int i915_gpu_idle(struct drm_device *dev)
 	return 0;
 }
 
-static void sandybridge_write_fence_reg(struct drm_device *dev, int reg,
-					struct drm_i915_gem_object *obj)
-{
-	drm_i915_private_t *dev_priv = dev->dev_private;
-	uint64_t val;
-
-	if (obj) {
-		u32 size = obj->gtt_space->size;
-
-		val = (uint64_t)((obj->gtt_offset + size - 4096) &
-				 0xfffff000) << 32;
-		val |= obj->gtt_offset & 0xfffff000;
-		val |= (uint64_t)((obj->stride / 128) - 1) <<
-			SANDYBRIDGE_FENCE_PITCH_SHIFT;
-
-		if (obj->tiling_mode == I915_TILING_Y)
-			val |= 1 << I965_FENCE_TILING_Y_SHIFT;
-		val |= I965_FENCE_REG_VALID;
-	} else
-		val = 0;
-
-	I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + reg * 8, val);
-	POSTING_READ(FENCE_REG_SANDYBRIDGE_0 + reg * 8);
-}
-
 static void i965_write_fence_reg(struct drm_device *dev, int reg,
 				 struct drm_i915_gem_object *obj)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	int fence_reg;
+	int fence_pitch_shift;
 	uint64_t val;
 
+	if (INTEL_INFO(dev)->gen >= 6) {
+		fence_reg = FENCE_REG_SANDYBRIDGE_0;
+		fence_pitch_shift = SANDYBRIDGE_FENCE_PITCH_SHIFT;
+	} else {
+		fence_reg = FENCE_REG_965_0;
+		fence_pitch_shift = I965_FENCE_PITCH_SHIFT;
+	}
+
 	if (obj) {
 		u32 size = obj->gtt_space->size;
 
 		val = (uint64_t)((obj->gtt_offset + size - 4096) &
 				 0xfffff000) << 32;
 		val |= obj->gtt_offset & 0xfffff000;
-		val |= ((obj->stride / 128) - 1) << I965_FENCE_PITCH_SHIFT;
+		val |= (uint64_t)((obj->stride / 128) - 1) << fence_pitch_shift;
 		if (obj->tiling_mode == I915_TILING_Y)
 			val |= 1 << I965_FENCE_TILING_Y_SHIFT;
 		val |= I965_FENCE_REG_VALID;
 	} else
 		val = 0;
 
-	I915_WRITE64(FENCE_REG_965_0 + reg * 8, val);
-	POSTING_READ(FENCE_REG_965_0 + reg * 8);
+	fence_reg += reg * 8;
+	I915_WRITE64(fence_reg, val);
+	POSTING_READ(fence_reg);
 }
 
 static void i915_write_fence_reg(struct drm_device *dev, int reg,
@@ -2666,7 +2652,7 @@ static void i915_gem_write_fence(struct drm_device *dev, int reg,
 {
 	switch (INTEL_INFO(dev)->gen) {
 	case 7:
-	case 6: sandybridge_write_fence_reg(dev, reg, obj); break;
+	case 6:
 	case 5:
 	case 4: i965_write_fence_reg(dev, reg, obj); break;
 	case 3: i915_write_fence_reg(dev, reg, obj); break;
-- 
1.7.9.5

[PATCH 3/5] drm/i915: use gtt_get_size() instead of open coding it

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    2 ++
 drivers/gpu/drm/i915/i915_gem.c        |    2 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c |   13 +------------
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b70728..91838b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1577,6 +1577,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
+i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
+uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b74e72f..77d4de9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1463,7 +1463,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	obj->fault_mappable = false;
 }
 
-static uint32_t
+uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
 	uint32_t gtt_size;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 02bb2bc..896ef41 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -273,18 +273,7 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 			return false;
 	}
 
-	/*
-	 * Previous chips need to be aligned to the size of the smallest
-	 * fence register that can contain the object.
-	 */
-	if (INTEL_INFO(obj->base.dev)->gen == 3)
-		size = 1024*1024;
-	else
-		size = 512*1024;
-
-	while (size < obj->base.size)
-		size <<= 1;
-
+	size = i915_gem_get_gtt_size(obj->base.dev, obj->base.size, tiling_mode);
 	if (obj->gtt_space->size != size)
 		return false;
 
-- 
1.7.9.5

[PATCH 4/5] drm/i915: factor out i915_gem_get_tile_width()

[Imre Deak]

This will be needed by the upcoming patch.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    1 +
 drivers/gpu/drm/i915/i915_gem.c        |   12 ++++++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |    8 ++------
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 91838b8..a20edca 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1581,6 +1581,7 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
+int i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode);
 
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 77d4de9..a878b9f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1463,6 +1463,18 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	obj->fault_mappable = false;
 }
 
+int
+i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode)
+{
+	BUG_ON(tiling_mode != I915_TILING_Y && tiling_mode != I915_TILING_X);
+
+	if (IS_GEN2(dev) ||
+	    (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
+		return 128;
+	else
+		return 512;
+}
+
 uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 896ef41..7b0a3b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -211,12 +211,6 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 	if (tiling_mode == I915_TILING_NONE)
 		return true;
 
-	if (IS_GEN2(dev) ||
-	    (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
-		tile_width = 128;
-	else
-		tile_width = 512;
-
 	/* check maximum stride & object size */
 	if (INTEL_INFO(dev)->gen >= 4) {
 		/* i965 stores the end address of the gtt mapping in the fence
@@ -236,6 +230,8 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 		}
 	}
 
+	tile_width = i915_gem_get_tile_width(dev, tiling_mode);
+
 	/* 965+ just needs multiples of tile width */
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (stride & (tile_width - 1))
-- 
1.7.9.5

[PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

The gtt space needed for tiled objects might be bigger than the linear
size programmed into the correpsonding fence register. For example for
the following buffer on a Gen5+ HW:

- allocation size: 4096 bytes
- tiling mode: X tiled
- stride: 1536

we need (1536 / 512) * 4096 bytes of gtt space to cover all the pixels
in the buffer, but at the moment we allocate only 4096. This means that
any buffer following this tiled buffer in the gtt space will be
corrupted if pixels belonging to the 2nd and 3rd tiles are written.

Fix this by rounding up the size of the allocated gtt space to the next
tile row address. The page frames beyond the allocation size will be
backed by the single gtt scratch page used already elsewhere for similar
padding.

Note that this is more of a security/robustness problem and not fixing any
reported issue that I know of. This is because applications will normally
access only the part of the buffer that is tile row size aligned.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    6 ++++-
 drivers/gpu/drm/i915/i915_gem.c        |   42 ++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_gem_tiling.c |    9 ++++---
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a20edca..3a755e4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1577,7 +1577,11 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
-i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
+i915_gem_get_gtt_linear_size(struct drm_device *dev, uint32_t size,
+			     int tiling_mode);
+uint32_t
+i915_gem_get_gtt_physical_size(struct drm_device *dev, uint32_t size,
+			       int tiling_mode, int stride);
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a878b9f..548ee8b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1476,7 +1476,8 @@ i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode)
 }
 
 uint32_t
-i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
+i915_gem_get_gtt_linear_size(struct drm_device *dev, uint32_t size,
+			     int tiling_mode)
 {
 	uint32_t gtt_size;
 
@@ -1496,6 +1497,26 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 	return gtt_size;
 }
 
+uint32_t
+i915_gem_get_gtt_physical_size(struct drm_device *dev, uint32_t size,
+			       int tiling_mode, int stride)
+{
+	uint32_t linear_size;
+	int tile_y_num;
+	int tile_row_size;
+
+	if (tiling_mode == I915_TILING_NONE)
+		return size;
+
+	linear_size = i915_gem_get_gtt_linear_size(dev, size, tiling_mode);
+	tile_y_num = DIV_ROUND_UP(stride,
+				  i915_gem_get_tile_width(dev, tiling_mode));
+	tile_row_size = tile_y_num * PAGE_SIZE;
+	size = roundup(size, tile_row_size);
+
+	return max(size, linear_size);
+}
+
 /**
  * i915_gem_get_gtt_alignment - return required GTT alignment for an object
  * @obj: object to check
@@ -1519,7 +1540,7 @@ i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 	 * Previous chips need to be aligned to the size of the smallest
 	 * fence register that can contain the object.
 	 */
-	return i915_gem_get_gtt_size(dev, size, tiling_mode);
+	return i915_gem_get_gtt_linear_size(dev, size, tiling_mode);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -2566,8 +2587,10 @@ static void i965_write_fence_reg(struct drm_device *dev, int reg,
 	}
 
 	if (obj) {
-		u32 size = obj->gtt_space->size;
+		u32 size;
 
+		size = i915_gem_get_gtt_linear_size(dev, obj->base.size,
+						    obj->tiling_mode);
 		val = (uint64_t)((obj->gtt_offset + size - 4096) &
 				 0xfffff000) << 32;
 		val |= obj->gtt_offset & 0xfffff000;
@@ -2590,10 +2613,12 @@ static void i915_write_fence_reg(struct drm_device *dev, int reg,
 	u32 val;
 
 	if (obj) {
-		u32 size = obj->gtt_space->size;
+		u32 size;
 		int pitch_val;
 		int tile_width;
 
+		size = i915_gem_get_gtt_linear_size(dev, obj->base.size,
+						    obj->tiling_mode);
 		WARN((obj->gtt_offset & ~I915_FENCE_START_MASK) ||
 		     (size & -size) != size ||
 		     (obj->gtt_offset & (size - 1)),
@@ -2634,9 +2659,11 @@ static void i830_write_fence_reg(struct drm_device *dev, int reg,
 	uint32_t val;
 
 	if (obj) {
-		u32 size = obj->gtt_space->size;
+		u32 size;
 		uint32_t pitch_val;
 
+		size = i915_gem_get_gtt_linear_size(dev, obj->base.size,
+						    obj->tiling_mode);
 		WARN((obj->gtt_offset & ~I830_FENCE_START_MASK) ||
 		     (size & -size) != size ||
 		     (obj->gtt_offset & (size - 1)),
@@ -2929,9 +2956,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 		return -EINVAL;
 	}
 
-	fence_size = i915_gem_get_gtt_size(dev,
-					   obj->base.size,
-					   obj->tiling_mode);
+	fence_size = i915_gem_get_gtt_physical_size(dev, obj->base.size,
+						obj->tiling_mode, obj->stride);
 	fence_alignment = i915_gem_get_gtt_alignment(dev,
 						     obj->base.size,
 						     obj->tiling_mode, true);
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 7b0a3b3..e738fcb 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -251,7 +251,8 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 
 /* Is the current GTT allocation valid for the change in tiling? */
 static bool
-i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
+i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode,
+			 int stride)
 {
 	u32 size;
 
@@ -269,7 +270,8 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 			return false;
 	}
 
-	size = i915_gem_get_gtt_size(obj->base.dev, obj->base.size, tiling_mode);
+	size = i915_gem_get_gtt_physical_size(obj->base.dev, obj->base.size,
+					      tiling_mode, stride);
 	if (obj->gtt_space->size != size)
 		return false;
 
@@ -355,7 +357,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		obj->map_and_fenceable =
 			obj->gtt_space == NULL ||
 			(obj->gtt_offset + obj->base.size <= dev_priv->mm.gtt_mappable_end &&
-			 i915_gem_object_fence_ok(obj, args->tiling_mode));
+			 i915_gem_object_fence_ok(obj, args->tiling_mode,
+						  args->stride));
 
 		/* Rebind if we need a change of alignment */
 		if (!obj->map_and_fenceable) {
-- 
1.7.9.5

Re: [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Chris Wilson]

On Fri,  4 Jan 2013 18:42:00 +0200, Imre Deak <imre.deak@intel.com> wrote:
> The gtt space needed for tiled objects might be bigger than the linear
> size programmed into the correpsonding fence register. For example for
> the following buffer on a Gen5+ HW:
> 
> - allocation size: 4096 bytes
> - tiling mode: X tiled
> - stride: 1536
> 
> we need (1536 / 512) * 4096 bytes of gtt space to cover all the pixels
> in the buffer, but at the moment we allocate only 4096. This means that
> any buffer following this tiled buffer in the gtt space will be
> corrupted if pixels belonging to the 2nd and 3rd tiles are written.
> 
> Fix this by rounding up the size of the allocated gtt space to the next
> tile row address. The page frames beyond the allocation size will be
> backed by the single gtt scratch page used already elsewhere for similar
> padding.
> 
> Note that this is more of a security/robustness problem and not fixing any
> reported issue that I know of. This is because applications will normally
> access only the part of the buffer that is tile row size aligned.

There should not be any reported issues because all userspace already
allocates up to the end of tile-row and stride should be enforced to be
a multiple of tile-width. So the use of DIV_ROUND_UP implies a
programming error that should have been reported back to userspace
earlier. We can extend that by checking to make sure userspace has
allocated a valid buffer, that is, it has allocated sufficient pages for
the sampler access into the tiled buffer (or reject the set-tiling).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

On Fri, 2013-01-04 at 17:07 +0000, Chris Wilson wrote:
> On Fri,  4 Jan 2013 18:42:00 +0200, Imre Deak <imre.deak@intel.com> wrote:
> > The gtt space needed for tiled objects might be bigger than the linear
> > size programmed into the correpsonding fence register. For example for
> > the following buffer on a Gen5+ HW:
> > 
> > - allocation size: 4096 bytes
> > - tiling mode: X tiled
> > - stride: 1536
> > 
> > we need (1536 / 512) * 4096 bytes of gtt space to cover all the pixels
> > in the buffer, but at the moment we allocate only 4096. This means that
> > any buffer following this tiled buffer in the gtt space will be
> > corrupted if pixels belonging to the 2nd and 3rd tiles are written.
> > 
> > Fix this by rounding up the size of the allocated gtt space to the next
> > tile row address. The page frames beyond the allocation size will be
> > backed by the single gtt scratch page used already elsewhere for similar
> > padding.
> > 
> > Note that this is more of a security/robustness problem and not fixing any
> > reported issue that I know of. This is because applications will normally
> > access only the part of the buffer that is tile row size aligned.
> 
> There should not be any reported issues because all userspace already
> allocates up to the end of tile-row and stride should be enforced to be
> a multiple of tile-width. So the use of DIV_ROUND_UP implies a
> programming error that should have been reported back to userspace
> earlier. We can extend that by checking to make sure userspace has
> allocated a valid buffer, that is, it has allocated sufficient pages for
> the sampler access into the tiled buffer (or reject the set-tiling).
> -Chris

Ok, I tested this with older UXA that still allocated non-aligned
buffers. If that's not the case any more then rejecting set-tiling if
it's called on a non tile-row size aligned buffer would work too.

--Imre

Re: [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Chris Wilson]

On Fri, 04 Jan 2013 19:23:42 +0200, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, 2013-01-04 at 17:07 +0000, Chris Wilson wrote:
> > On Fri,  4 Jan 2013 18:42:00 +0200, Imre Deak <imre.deak@intel.com> wrote:
> > > The gtt space needed for tiled objects might be bigger than the linear
> > > size programmed into the correpsonding fence register. For example for
> > > the following buffer on a Gen5+ HW:
> > > 
> > > - allocation size: 4096 bytes
> > > - tiling mode: X tiled
> > > - stride: 1536
> > > 
> > > we need (1536 / 512) * 4096 bytes of gtt space to cover all the pixels
> > > in the buffer, but at the moment we allocate only 4096. This means that
> > > any buffer following this tiled buffer in the gtt space will be
> > > corrupted if pixels belonging to the 2nd and 3rd tiles are written.
> > > 
> > > Fix this by rounding up the size of the allocated gtt space to the next
> > > tile row address. The page frames beyond the allocation size will be
> > > backed by the single gtt scratch page used already elsewhere for similar
> > > padding.
> > > 
> > > Note that this is more of a security/robustness problem and not fixing any
> > > reported issue that I know of. This is because applications will normally
> > > access only the part of the buffer that is tile row size aligned.
> > 
> > There should not be any reported issues because all userspace already
> > allocates up to the end of tile-row and stride should be enforced to be
> > a multiple of tile-width. So the use of DIV_ROUND_UP implies a
> > programming error that should have been reported back to userspace
> > earlier. We can extend that by checking to make sure userspace has
> > allocated a valid buffer, that is, it has allocated sufficient pages for
> > the sampler access into the tiled buffer (or reject the set-tiling).
> > -Chris
> 
> Ok, I tested this with older UXA that still allocated non-aligned
> buffers. If that's not the case any more then rejecting set-tiling if
> it's called on a non tile-row size aligned buffer would work too.

Meep. A long time ago we got the calcuations wrong (slightly less for
gen2), but it was and still is a userspace bug with the potential of
handing the GPU.

A multiple of tile_height tall and a mutiple of tile_width across should
always be an exact number of pages (and an exact multiple of tile-row
pages). And we should have been obeying that since the introduction of
set-tiling (ignoring the aforementioned bugs) - so I'd really like to
see any evidence of userspace getting that wrong.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

[-- Attachment #1: Type: text/plain, Size: 3646 bytes --]

On Fri, 2013-01-04 at 17:47 +0000, Chris Wilson wrote:
> On Fri, 04 Jan 2013 19:23:42 +0200, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2013-01-04 at 17:07 +0000, Chris Wilson wrote:
> > > On Fri,  4 Jan 2013 18:42:00 +0200, Imre Deak <imre.deak@intel.com> wrote:
> > > > The gtt space needed for tiled objects might be bigger than the linear
> > > > size programmed into the correpsonding fence register. For example for
> > > > the following buffer on a Gen5+ HW:
> > > > 
> > > > - allocation size: 4096 bytes
> > > > - tiling mode: X tiled
> > > > - stride: 1536
> > > > 
> > > > we need (1536 / 512) * 4096 bytes of gtt space to cover all the pixels
> > > > in the buffer, but at the moment we allocate only 4096. This means that
> > > > any buffer following this tiled buffer in the gtt space will be
> > > > corrupted if pixels belonging to the 2nd and 3rd tiles are written.
> > > > 
> > > > Fix this by rounding up the size of the allocated gtt space to the next
> > > > tile row address. The page frames beyond the allocation size will be
> > > > backed by the single gtt scratch page used already elsewhere for similar
> > > > padding.
> > > > 
> > > > Note that this is more of a security/robustness problem and not fixing any
> > > > reported issue that I know of. This is because applications will normally
> > > > access only the part of the buffer that is tile row size aligned.
> > > 
> > > There should not be any reported issues because all userspace already
> > > allocates up to the end of tile-row and stride should be enforced to be
> > > a multiple of tile-width. So the use of DIV_ROUND_UP implies a
> > > programming error that should have been reported back to userspace
> > > earlier. We can extend that by checking to make sure userspace has
> > > allocated a valid buffer, that is, it has allocated sufficient pages for
> > > the sampler access into the tiled buffer (or reject the set-tiling).
> > > -Chris
> > 
> > Ok, I tested this with older UXA that still allocated non-aligned
> > buffers. If that's not the case any more then rejecting set-tiling if
> > it's called on a non tile-row size aligned buffer would work too.
> 
> Meep. A long time ago we got the calcuations wrong (slightly less for
> gen2), but it was and still is a userspace bug with the potential of
> handing the GPU.
> 
> A multiple of tile_height tall and a mutiple of tile_width across should
> always be an exact number of pages (and an exact multiple of tile-row
> pages). And we should have been obeying that since the introduction of
> set-tiling (ignoring the aforementioned bugs) - so I'd really like to
> see any evidence of userspace getting that wrong.

On Ubuntu 12.10, with xf86-video-intel 2.20.9 I get the attached log
with the following patch applied:

diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 7b0a3b3..846e96f 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -236,9 +236,17 @@ i915_tiling_ok(struct drm_device *dev, int stride,
int size, int tiling_mode)
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (stride & (tile_width - 1))
 			return false;
-		return true;
 	}
 
+	if (size % (stride / tile_width * PAGE_SIZE))
+		printk("unaligned tiling: comm %.*s size %u mode %c stride %d
size-mod-tilerow-size %zd\n",
+			(int)sizeof(current->comm), current->comm,
+			size, tiling_mode == I915_TILING_X ? 'X' : 'Y',
+			stride, size % (stride / tile_width * PAGE_SIZE));
+
+	if (INTEL_INFO(dev)->gen >= 4)
+		return true;
+
 	/* Pre-965 needs power of two tile widths */
 	if (stride < tile_width)
 		return false;



[-- Attachment #2: log --]
[-- Type: text/plain, Size: 4271 bytes --]

dmesg | grep unaligned | cut -c 16- | sort | uniq -c
      2 unaligned tiling: comm compiz size 10485760 mode X stride 6656 size-mod-tilerow-size 49152
      2 unaligned tiling: comm compiz size 10485760 mode Y stride 6400 size-mod-tilerow-size 40960
      1 unaligned tiling: comm compiz size 1572864 mode Y stride 2944 size-mod-tilerow-size 65536
      1 unaligned tiling: comm compiz size 163840 mode X stride 6144 size-mod-tilerow-size 16384
      1 unaligned tiling: comm compiz size 163840 mode Y stride 1536 size-mod-tilerow-size 16384
      2 unaligned tiling: comm compiz size 229376 mode Y stride 1152 size-mod-tilerow-size 8192
      1 unaligned tiling: comm compiz size 2621440 mode Y stride 4736 size-mod-tilerow-size 45056
      1 unaligned tiling: comm compiz size 262144 mode X stride 5120 size-mod-tilerow-size 16384
      2 unaligned tiling: comm compiz size 327680 mode X stride 6144 size-mod-tilerow-size 32768
      6 unaligned tiling: comm compiz size 327680 mode X stride 6656 size-mod-tilerow-size 8192
      1 unaligned tiling: comm compiz size 327680 mode Y stride 3072 size-mod-tilerow-size 32768
      1 unaligned tiling: comm compiz size 327680 mode Y stride 4736 size-mod-tilerow-size 24576
      1 unaligned tiling: comm compiz size 3670016 mode X stride 3072 size-mod-tilerow-size 8192
      1 unaligned tiling: comm compiz size 458752 mode Y stride 6400 size-mod-tilerow-size 49152
      1 unaligned tiling: comm compiz size 524288 mode Y stride 384 size-mod-tilerow-size 8192
      1 unaligned tiling: comm compiz size 6291456 mode X stride 6656 size-mod-tilerow-size 8192
      2 unaligned tiling: comm compiz size 6291456 mode Y stride 6400 size-mod-tilerow-size 147456
      6 unaligned tiling: comm compiz size 65536 mode Y stride 640 size-mod-tilerow-size 4096
      1 unaligned tiling: comm unity_support_t size 6291456 mode Y stride 6400 size-mod-tilerow-size 147456
     69 unaligned tiling: comm Xorg size 114688 mode X stride 2560 size-mod-tilerow-size 12288
    152 unaligned tiling: comm Xorg size 1310720 mode X stride 1536 size-mod-tilerow-size 8192
      3 unaligned tiling: comm Xorg size 131072 mode X stride 2560 size-mod-tilerow-size 8192
    317 unaligned tiling: comm Xorg size 131072 mode X stride 3072 size-mod-tilerow-size 8192
      4 unaligned tiling: comm Xorg size 163840 mode X stride 1536 size-mod-tilerow-size 4096
     87 unaligned tiling: comm Xorg size 163840 mode X stride 3072 size-mod-tilerow-size 16384
   1104 unaligned tiling: comm Xorg size 163840 mode X stride 3584 size-mod-tilerow-size 20480
      1 unaligned tiling: comm Xorg size 163840 mode X stride 4608 size-mod-tilerow-size 16384
      1 unaligned tiling: comm Xorg size 163840 mode X stride 6656 size-mod-tilerow-size 4096
      2 unaligned tiling: comm Xorg size 196608 mode X stride 2560 size-mod-tilerow-size 12288
     12 unaligned tiling: comm Xorg size 196608 mode X stride 3584 size-mod-tilerow-size 24576
     20 unaligned tiling: comm Xorg size 196608 mode X stride 4608 size-mod-tilerow-size 12288
      2 unaligned tiling: comm Xorg size 2097152 mode X stride 2560 size-mod-tilerow-size 8192
     10 unaligned tiling: comm Xorg size 229376 mode X stride 3072 size-mod-tilerow-size 8192
    193 unaligned tiling: comm Xorg size 2621440 mode X stride 4608 size-mod-tilerow-size 4096
      4 unaligned tiling: comm Xorg size 262144 mode X stride 1536 size-mod-tilerow-size 4096
     16 unaligned tiling: comm Xorg size 262144 mode X stride 4608 size-mod-tilerow-size 4096
      1 unaligned tiling: comm Xorg size 3145728 mode X stride 5632 size-mod-tilerow-size 36864
      3 unaligned tiling: comm Xorg size 327680 mode X stride 1536 size-mod-tilerow-size 8192
      2 unaligned tiling: comm Xorg size 393216 mode X stride 4608 size-mod-tilerow-size 24576
      3 unaligned tiling: comm Xorg size 40960 mode X stride 1536 size-mod-tilerow-size 4096
      1 unaligned tiling: comm Xorg size 5242880 mode X stride 5632 size-mod-tilerow-size 16384
    116 unaligned tiling: comm Xorg size 6291456 mode X stride 6656 size-mod-tilerow-size 8192
    280 unaligned tiling: comm Xorg size 65536 mode X stride 1536 size-mod-tilerow-size 4096
      6 unaligned tiling: comm Xorg size 65536 mode X stride 2560 size-mod-tilerow-size 4096

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Chris Wilson]

On Fri, 04 Jan 2013 21:18:53 +0200, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, 2013-01-04 at 17:47 +0000, Chris Wilson wrote:
> > Meep. A long time ago we got the calcuations wrong (slightly less for
> > gen2), but it was and still is a userspace bug with the potential of
> > handing the GPU.
> > 
> > A multiple of tile_height tall and a mutiple of tile_width across should
> > always be an exact number of pages (and an exact multiple of tile-row
> > pages). And we should have been obeying that since the introduction of
> > set-tiling (ignoring the aforementioned bugs) - so I'd really like to
> > see any evidence of userspace getting that wrong.
> 
> On Ubuntu 12.10, with xf86-video-intel 2.20.9 I get the attached log
> with the following patch applied:
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
> b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 7b0a3b3..846e96f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -236,9 +236,17 @@ i915_tiling_ok(struct drm_device *dev, int stride,
> int size, int tiling_mode)
>  	if (INTEL_INFO(dev)->gen >= 4) {
>  		if (stride & (tile_width - 1))
>  			return false;
> -		return true;
>  	}
>  
> +	if (size % (stride / tile_width * PAGE_SIZE))
> +		printk("unaligned tiling: comm %.*s size %u mode %c stride %d
> size-mod-tilerow-size %zd\n",
> +			(int)sizeof(current->comm), current->comm,
> +			size, tiling_mode == I915_TILING_X ? 'X' : 'Y',
> +			stride, size % (stride / tile_width * PAGE_SIZE));
> +
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		return true;
> +
>  	/* Pre-965 needs power of two tile widths */
>  	if (stride < tile_width)
>  		return false;
> 
> 
> dmesg | grep unaligned | cut -c 16- | sort | uniq -c
>       2 unaligned tiling: comm compiz size 10485760 mode X stride 6656 size-mod-tilerow-size 49152
>       2 unaligned tiling: comm compiz size 10485760 mode Y stride 6400 size-mod-tilerow-size 40960
>       1 unaligned tiling: comm compiz size 1572864 mode Y stride 2944 size-mod-tilerow-size 65536

Hmm, I also forgot to mention we then pluck a bo out of cache returning
a potentially larger than required size. However, looks like I need to
double check userspace to make sure we are overallocating tile rows.

So we have a choice of overallocating the GTT for fenced regions, or
rounding the fence region to a tile row which preserves the existing
functional userspace behaviour. The right answer is indeed to modify the
behaviour to overallocate physical space so the fence region doesn't
overlap another bo. Next up you need to review what happens after a
change of tiling and whether we rebind before the next fenced GTT
access.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

On Fri, 2013-01-04 at 20:32 +0000, Chris Wilson wrote:
> On Fri, 04 Jan 2013 21:18:53 +0200, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2013-01-04 at 17:47 +0000, Chris Wilson wrote:
> > > Meep. A long time ago we got the calcuations wrong (slightly less for
> > > gen2), but it was and still is a userspace bug with the potential of
> > > handing the GPU.
> > > 
> > > A multiple of tile_height tall and a mutiple of tile_width across should
> > > always be an exact number of pages (and an exact multiple of tile-row
> > > pages). And we should have been obeying that since the introduction of
> > > set-tiling (ignoring the aforementioned bugs) - so I'd really like to
> > > see any evidence of userspace getting that wrong.
> > 
> > On Ubuntu 12.10, with xf86-video-intel 2.20.9 I get the attached log
> > with the following patch applied:
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index 7b0a3b3..846e96f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -236,9 +236,17 @@ i915_tiling_ok(struct drm_device *dev, int stride,
> > int size, int tiling_mode)
> >  	if (INTEL_INFO(dev)->gen >= 4) {
> >  		if (stride & (tile_width - 1))
> >  			return false;
> > -		return true;
> >  	}
> >  
> > +	if (size % (stride / tile_width * PAGE_SIZE))
> > +		printk("unaligned tiling: comm %.*s size %u mode %c stride %d
> > size-mod-tilerow-size %zd\n",
> > +			(int)sizeof(current->comm), current->comm,
> > +			size, tiling_mode == I915_TILING_X ? 'X' : 'Y',
> > +			stride, size % (stride / tile_width * PAGE_SIZE));
> > +
> > +	if (INTEL_INFO(dev)->gen >= 4)
> > +		return true;
> > +
> >  	/* Pre-965 needs power of two tile widths */
> >  	if (stride < tile_width)
> >  		return false;
> > 
> > 
> > dmesg | grep unaligned | cut -c 16- | sort | uniq -c
> >       2 unaligned tiling: comm compiz size 10485760 mode X stride 6656 size-mod-tilerow-size 49152
> >       2 unaligned tiling: comm compiz size 10485760 mode Y stride 6400 size-mod-tilerow-size 40960
> >       1 unaligned tiling: comm compiz size 1572864 mode Y stride 2944 size-mod-tilerow-size 65536
> 
> Hmm, I also forgot to mention we then pluck a bo out of cache returning
> a potentially larger than required size. However, looks like I need to
> double check userspace to make sure we are overallocating tile rows.

Ok, I checked now the result is somewhat similar with 2.20.17.

> So we have a choice of overallocating the GTT for fenced regions, or
> rounding the fence region to a tile row which preserves the existing
> functional userspace behaviour.

Do you mean rounding the size down to the closest tile row address (and
program that to the fence reg)? I had such a version, but I thought it
got too complicated due to the old HW's power-of-two alignment
requirement. Here we would still need to over allocate if the
power-of-two size happens to be non tile-row size aligned.. But I guess
it's still doable if we want to save some gtt space.

> The right answer is indeed to modify the
> behaviour to overallocate physical space so the fence region doesn't
> overlap another bo. Next up you need to review what happens after a
> change of tiling and whether we rebind before the next fenced GTT
> access.

Ah right, atm only an incorrect alignment will cause an unbind, but we
would also need to check the new gtt size if we chose to over allocate.

--Imre

[PATCH v2 0/7] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

At the moment it's possible for user space to allocate a tiled buffer
with a size not aligned to its tile row size and corrupt a buffer that
follows this in the gtt space. More details on this in the last patch.

I'll also send a v2 i-g-t test case for this.

Tested on Gen5,6.

In v2:
- Instead of always overallocating for unaligned objects, limit the
  linear size - and overallocate only when necessary for POT fence
  sizes. Discussed this with Chris Wilson.
- Force a rebind if the GTT size changes after a tiling change. (Chris
  Wilson)
- Reject tiling if the object size is smaller than the tile row size.

Imre Deak (7):
  drm/i915: merge get_gtt_alignment/get_unfenced_gtt_alignment()
  drm/i915: merge {i965,sandybridge}_write_fence_reg()
  drm/i915: use gtt_get_size() instead of open coding it
  drm/i915: factor out i915_gem_get_tile_width()
  drm/i915: reject tiling for objects smaller than their tile row size
  drm/i915: check tile object alignment explicitly
  drm/i915: fix gtt space allocated for tiled objects

 drivers/gpu/drm/i915/i915_drv.h        |   13 ++-
 drivers/gpu/drm/i915/i915_gem.c        |  183 ++++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_gem_tiling.c |   48 ++++-----
 3 files changed, 138 insertions(+), 106 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/7] drm/i915: merge get_gtt_alignment/get_unfenced_gtt_alignment()

[Imre Deak]

The two functions are rather similar, so merge them.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    5 ++--
 drivers/gpu/drm/i915/i915_gem.c        |   44 +++++---------------------------
 drivers/gpu/drm/i915/i915_gem_tiling.c |    6 ++---
 3 files changed, 12 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b1b1b73..154323a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1566,9 +1566,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
-i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
-				    uint32_t size,
-				    int tiling_mode);
+i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
+			    int tiling_mode, bool fenced);
 
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9dbc284..d47c143 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1463,16 +1463,15 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
  * Return the required GTT alignment for an object, taking into account
  * potential fence register mapping.
  */
-static uint32_t
-i915_gem_get_gtt_alignment(struct drm_device *dev,
-			   uint32_t size,
-			   int tiling_mode)
+uint32_t
+i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
+			   int tiling_mode, bool fenced)
 {
 	/*
 	 * Minimum alignment is 4k (GTT page size), but might be greater
 	 * if a fence register is needed for the object.
 	 */
-	if (INTEL_INFO(dev)->gen >= 4 ||
+	if (INTEL_INFO(dev)->gen >= 4 || (!fenced && IS_G33(dev)) ||
 	    tiling_mode == I915_TILING_NONE)
 		return 4096;
 
@@ -1483,35 +1482,6 @@ i915_gem_get_gtt_alignment(struct drm_device *dev,
 	return i915_gem_get_gtt_size(dev, size, tiling_mode);
 }
 
-/**
- * i915_gem_get_unfenced_gtt_alignment - return required GTT alignment for an
- *					 unfenced object
- * @dev: the device
- * @size: size of the object
- * @tiling_mode: tiling mode of the object
- *
- * Return the required GTT alignment for an object, only taking into account
- * unfenced tiled surface requirements.
- */
-uint32_t
-i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
-				    uint32_t size,
-				    int tiling_mode)
-{
-	/*
-	 * Minimum alignment is 4k (GTT page size) for sane hw.
-	 */
-	if (INTEL_INFO(dev)->gen >= 4 || IS_G33(dev) ||
-	    tiling_mode == I915_TILING_NONE)
-		return 4096;
-
-	/* Previous hardware however needs to be aligned to a power-of-two
-	 * tile height. The simplest method for determining this is to reuse
-	 * the power-of-tile object size.
-	 */
-	return i915_gem_get_gtt_size(dev, size, tiling_mode);
-}
-
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
@@ -2934,11 +2904,11 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 					   obj->tiling_mode);
 	fence_alignment = i915_gem_get_gtt_alignment(dev,
 						     obj->base.size,
-						     obj->tiling_mode);
+						     obj->tiling_mode, true);
 	unfenced_alignment =
-		i915_gem_get_unfenced_gtt_alignment(dev,
+		i915_gem_get_gtt_alignment(dev,
 						    obj->base.size,
-						    obj->tiling_mode);
+						    obj->tiling_mode, false);
 
 	if (alignment == 0)
 		alignment = map_and_fenceable ? fence_alignment :
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 65f1d4f..cb71ded 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -374,9 +374,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		/* Rebind if we need a change of alignment */
 		if (!obj->map_and_fenceable) {
 			u32 unfenced_alignment =
-				i915_gem_get_unfenced_gtt_alignment(dev,
-								    obj->base.size,
-								    args->tiling_mode);
+				i915_gem_get_gtt_alignment(dev, obj->base.size,
+							    args->tiling_mode,
+							    false);
 			if (obj->gtt_offset & (unfenced_alignment - 1))
 				ret = i915_gem_object_unbind(obj);
 		}
-- 
1.7.10.4

[PATCH v2 2/7] drm/i915: merge {i965, sandybridge}_write_fence_reg()

[Imre Deak]

The two functions are rather similar, so merge them.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   44 +++++++++++++--------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d47c143..5e00dc1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2507,52 +2507,38 @@ int i915_gpu_idle(struct drm_device *dev)
 	return 0;
 }
 
-static void sandybridge_write_fence_reg(struct drm_device *dev, int reg,
-					struct drm_i915_gem_object *obj)
-{
-	drm_i915_private_t *dev_priv = dev->dev_private;
-	uint64_t val;
-
-	if (obj) {
-		u32 size = obj->gtt_space->size;
-
-		val = (uint64_t)((obj->gtt_offset + size - 4096) &
-				 0xfffff000) << 32;
-		val |= obj->gtt_offset & 0xfffff000;
-		val |= (uint64_t)((obj->stride / 128) - 1) <<
-			SANDYBRIDGE_FENCE_PITCH_SHIFT;
-
-		if (obj->tiling_mode == I915_TILING_Y)
-			val |= 1 << I965_FENCE_TILING_Y_SHIFT;
-		val |= I965_FENCE_REG_VALID;
-	} else
-		val = 0;
-
-	I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + reg * 8, val);
-	POSTING_READ(FENCE_REG_SANDYBRIDGE_0 + reg * 8);
-}
-
 static void i965_write_fence_reg(struct drm_device *dev, int reg,
 				 struct drm_i915_gem_object *obj)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	int fence_reg;
+	int fence_pitch_shift;
 	uint64_t val;
 
+	if (INTEL_INFO(dev)->gen >= 6) {
+		fence_reg = FENCE_REG_SANDYBRIDGE_0;
+		fence_pitch_shift = SANDYBRIDGE_FENCE_PITCH_SHIFT;
+	} else {
+		fence_reg = FENCE_REG_965_0;
+		fence_pitch_shift = I965_FENCE_PITCH_SHIFT;
+	}
+
 	if (obj) {
 		u32 size = obj->gtt_space->size;
 
 		val = (uint64_t)((obj->gtt_offset + size - 4096) &
 				 0xfffff000) << 32;
 		val |= obj->gtt_offset & 0xfffff000;
-		val |= ((obj->stride / 128) - 1) << I965_FENCE_PITCH_SHIFT;
+		val |= (uint64_t)((obj->stride / 128) - 1) << fence_pitch_shift;
 		if (obj->tiling_mode == I915_TILING_Y)
 			val |= 1 << I965_FENCE_TILING_Y_SHIFT;
 		val |= I965_FENCE_REG_VALID;
 	} else
 		val = 0;
 
-	I915_WRITE64(FENCE_REG_965_0 + reg * 8, val);
-	POSTING_READ(FENCE_REG_965_0 + reg * 8);
+	fence_reg += reg * 8;
+	I915_WRITE64(fence_reg, val);
+	POSTING_READ(fence_reg);
 }
 
 static void i915_write_fence_reg(struct drm_device *dev, int reg,
@@ -2636,7 +2622,7 @@ static void i915_gem_write_fence(struct drm_device *dev, int reg,
 {
 	switch (INTEL_INFO(dev)->gen) {
 	case 7:
-	case 6: sandybridge_write_fence_reg(dev, reg, obj); break;
+	case 6:
 	case 5:
 	case 4: i965_write_fence_reg(dev, reg, obj); break;
 	case 3: i915_write_fence_reg(dev, reg, obj); break;
-- 
1.7.10.4

[PATCH v2 3/7] drm/i915: use gtt_get_size() instead of open coding it

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    2 ++
 drivers/gpu/drm/i915/i915_gem.c        |    2 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c |   13 +------------
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 154323a..3b73615 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1566,6 +1566,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
+i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
+uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5e00dc1..aa6653d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1435,7 +1435,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	obj->fault_mappable = false;
 }
 
-static uint32_t
+uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
 	uint32_t gtt_size;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index cb71ded..e76f0d8 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -272,18 +272,7 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 			return false;
 	}
 
-	/*
-	 * Previous chips need to be aligned to the size of the smallest
-	 * fence register that can contain the object.
-	 */
-	if (INTEL_INFO(obj->base.dev)->gen == 3)
-		size = 1024*1024;
-	else
-		size = 512*1024;
-
-	while (size < obj->base.size)
-		size <<= 1;
-
+	size = i915_gem_get_gtt_size(obj->base.dev, obj->base.size, tiling_mode);
 	if (obj->gtt_space->size != size)
 		return false;
 
-- 
1.7.10.4

Re: [PATCH v2 3/7] drm/i915: use gtt_get_size() instead of open coding it

[Daniel Vetter]

On Mon, Jan 07, 2013 at 09:47:35PM +0200, Imre Deak wrote:
> Signed-off-by: Imre Deak <imre.deak@intel.com>

I've applied patches 1-3 from this series, since they look like nice
cleanups. Like discussed on irc, I'm not sold on the later ones since I
don't see a clear upside ...
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |    2 ++
>  drivers/gpu/drm/i915/i915_gem.c        |    2 +-
>  drivers/gpu/drm/i915/i915_gem_tiling.c |   13 +------------
>  3 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 154323a..3b73615 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1566,6 +1566,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
>  void i915_gem_release(struct drm_device *dev, struct drm_file *file);
>  
>  uint32_t
> +i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
> +uint32_t
>  i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
>  			    int tiling_mode, bool fenced);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5e00dc1..aa6653d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1435,7 +1435,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>  	obj->fault_mappable = false;
>  }
>  
> -static uint32_t
> +uint32_t
>  i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
>  {
>  	uint32_t gtt_size;
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index cb71ded..e76f0d8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -272,18 +272,7 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
>  			return false;
>  	}
>  
> -	/*
> -	 * Previous chips need to be aligned to the size of the smallest
> -	 * fence register that can contain the object.
> -	 */
> -	if (INTEL_INFO(obj->base.dev)->gen == 3)
> -		size = 1024*1024;
> -	else
> -		size = 512*1024;
> -
> -	while (size < obj->base.size)
> -		size <<= 1;
> -
> +	size = i915_gem_get_gtt_size(obj->base.dev, obj->base.size, tiling_mode);
>  	if (obj->gtt_space->size != size)
>  		return false;
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 3/7] drm/i915: use gtt_get_size() instead of open coding it

[Imre Deak]

On Mon, 2013-01-14 at 17:14 +0100, Daniel Vetter wrote:
> On Mon, Jan 07, 2013 at 09:47:35PM +0200, Imre Deak wrote:
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> I've applied patches 1-3 from this series, since they look like nice
> cleanups. Like discussed on irc, I'm not sold on the later ones since I
> don't see a clear upside ...

I understood there are other ways to corrupt buffers than what this
patch would fix, so I agree that it's not a security fix. I would still
argue that eliminating this particular way makes things more robust
against obscure application bugs.

--Imre

> -Daniel
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h        |    2 ++
> >  drivers/gpu/drm/i915/i915_gem.c        |    2 +-
> >  drivers/gpu/drm/i915/i915_gem_tiling.c |   13 +------------
> >  3 files changed, 4 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 154323a..3b73615 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1566,6 +1566,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
> >  void i915_gem_release(struct drm_device *dev, struct drm_file *file);
> >  
> >  uint32_t
> > +i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
> > +uint32_t
> >  i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
> >  			    int tiling_mode, bool fenced);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 5e00dc1..aa6653d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1435,7 +1435,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> >  	obj->fault_mappable = false;
> >  }
> >  
> > -static uint32_t
> > +uint32_t
> >  i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
> >  {
> >  	uint32_t gtt_size;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index cb71ded..e76f0d8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -272,18 +272,7 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
> >  			return false;
> >  	}
> >  
> > -	/*
> > -	 * Previous chips need to be aligned to the size of the smallest
> > -	 * fence register that can contain the object.
> > -	 */
> > -	if (INTEL_INFO(obj->base.dev)->gen == 3)
> > -		size = 1024*1024;
> > -	else
> > -		size = 512*1024;
> > -
> > -	while (size < obj->base.size)
> > -		size <<= 1;
> > -
> > +	size = i915_gem_get_gtt_size(obj->base.dev, obj->base.size, tiling_mode);
> >  	if (obj->gtt_space->size != size)
> >  		return false;
> >  
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[PATCH v2 4/7] drm/i915: factor out i915_gem_get_tile_width()

[Imre Deak]

This will be needed by the upcoming patch.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    1 +
 drivers/gpu/drm/i915/i915_gem.c        |   12 ++++++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |    8 ++------
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3b73615..c863b0f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1570,6 +1570,7 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
+int i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode);
 
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index aa6653d..d029e9e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1435,6 +1435,18 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	obj->fault_mappable = false;
 }
 
+int
+i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode)
+{
+	BUG_ON(tiling_mode != I915_TILING_Y && tiling_mode != I915_TILING_X);
+
+	if (IS_GEN2(dev) ||
+	    (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
+		return 128;
+	else
+		return 512;
+}
+
 uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index e76f0d8..e2f2a71 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -210,12 +210,6 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 	if (tiling_mode == I915_TILING_NONE)
 		return true;
 
-	if (IS_GEN2(dev) ||
-	    (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
-		tile_width = 128;
-	else
-		tile_width = 512;
-
 	/* check maximum stride & object size */
 	if (INTEL_INFO(dev)->gen >= 4) {
 		/* i965 stores the end address of the gtt mapping in the fence
@@ -235,6 +229,8 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 		}
 	}
 
+	tile_width = i915_gem_get_tile_width(dev, tiling_mode);
+
 	/* 965+ just needs multiples of tile width */
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (stride & (tile_width - 1))
-- 
1.7.10.4

[PATCH v2 5/7] drm/i915: reject tiling for objects smaller than their tile row size

[Imre Deak]

For these objects there isn't enough backing storage even for a single
linear pixel line, so asking tiling for them is clearly a programming error.

i915_gem_get_tile_row_size() will be used by a later patch, so export it.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    4 ++++
 drivers/gpu/drm/i915/i915_gem.c        |    6 ++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |    3 +++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c863b0f..e67332f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1570,6 +1570,10 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
+
+size_t
+i915_gem_get_tile_row_size(struct drm_device *dev, int tiling_mode, int stride);
+
 int i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode);
 
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d029e9e..e51a4cd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1447,6 +1447,12 @@ i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode)
 		return 512;
 }
 
+size_t
+i915_gem_get_tile_row_size(struct drm_device *dev, int tiling_mode, int stride)
+{
+	return stride / i915_gem_get_tile_width(dev, tiling_mode) * PAGE_SIZE;
+}
+
 uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index e2f2a71..1a03e41 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -229,6 +229,9 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 		}
 	}
 
+	if (size < i915_gem_get_tile_row_size(dev, tiling_mode, stride))
+		return false;
+
 	tile_width = i915_gem_get_tile_width(dev, tiling_mode);
 
 	/* 965+ just needs multiples of tile width */
-- 
1.7.10.4

[PATCH v3 5/7] drm/i915: reject tiling for objects smaller than their tile row size

[Imre Deak]

For these objects there isn't enough backing storage even for a single
linear pixel line, so asking tiling for them is clearly a programming error.

i915_gem_get_tile_row_size() will be used by a later patch, so export it.

In v3:
- don't use PAGE_SIZE for the tile size as this is only coincidental and
  for Gen2 not even true (Chris Wilson)
- use the correct tile size of 2048 bytes for Gen2 (Chris Wilson, Daniel
  Vetter)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    4 ++++
 drivers/gpu/drm/i915/i915_gem.c        |    8 ++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |    3 +++
 3 files changed, 15 insertions(+)

[ Sending v3 only for this patch, as the rest of the patchset is
  unchanged. ]

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c863b0f..e67332f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1570,6 +1570,10 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 			    int tiling_mode, bool fenced);
+
+size_t
+i915_gem_get_tile_row_size(struct drm_device *dev, int tiling_mode, int stride);
+
 int i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode);
 
 int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d029e9e..dd185b4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1447,6 +1447,14 @@ i915_gem_get_tile_width(struct drm_device *dev, int tiling_mode)
 		return 512;
 }
 
+size_t
+i915_gem_get_tile_row_size(struct drm_device *dev, int tiling_mode, int stride)
+{
+	size_t tile_size = IS_GEN2(dev) ? 2048 : 4096;
+
+	return stride / i915_gem_get_tile_width(dev, tiling_mode) * tile_size;
+}
+
 uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index e2f2a71..1a03e41 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -229,6 +229,9 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 		}
 	}
 
+	if (size < i915_gem_get_tile_row_size(dev, tiling_mode, stride))
+		return false;
+
 	tile_width = i915_gem_get_tile_width(dev, tiling_mode);
 
 	/* 965+ just needs multiples of tile width */
-- 
1.7.10.4

[PATCH v2 6/7] drm/i915: check tile object alignment explicitly

[Imre Deak]

So far we used the object size for alignment check, since this matched
the actual alignment requirement. An upcoming patch will introduce
linear and physical fenced sizes making this connection less clear, so
use instead the get-alignment function explicitly.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_tiling.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 1a03e41..6d69d06 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -256,6 +256,7 @@ static bool
 i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 {
 	u32 size;
+	u32 align;
 
 	if (tiling_mode == I915_TILING_NONE)
 		return true;
@@ -275,7 +276,10 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 	if (obj->gtt_space->size != size)
 		return false;
 
-	if (obj->gtt_offset & (size - 1))
+	align = i915_gem_get_gtt_alignment(obj->base.dev, obj->base.size,
+					   tiling_mode, true);
+
+	if (obj->gtt_offset & (align - 1))
 		return false;
 
 	return true;
-- 
1.7.10.4

[PATCH v2 7/7] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

The GTT space needed for tiled objects might be bigger than the linear
size programmed into the correpsonding fence register. For example for
the following buffer on a Gen5+ HW:

- allocation size: 4 * 4096 bytes
- tiling mode: X tiled
- stride: 1536

This needs 6 * 4096 bytes of GTT space to cover all the pixels in the
buffer, but at the moment we allocate only 4 * 4096. This means that any
buffer following this tiled buffer in the GTT space will be corrupted if
pixels belonging to the last two tiles are written.

Fix this by limiting the linear size to the object's last full tile row
offset (3 * 4096 bytes in the above example). Beyond this offset we
don't have enough backing storage to hold even a single linear pixel
line, so tiling wouldn't work properly anyway. For old HW with
power-of-two fence regions we might have to also overallocate GTT space
in case the POT offset happens to be not aligned to the tile row size.
The page frames for the overallocated area will be backed by the single
GTT scratch page used already for padding objects with a POT fence
region.

Note that this is more of a security/robustness problem and not fixing any
reported issue that I know of. This is because applications will normally
access only the part of the buffer that is tile row size aligned.

In v2:
- Instead of always overallocating for unaligned objects, limit the
  linear size - and overallocate only when necessary for POT fence
  sizes. Discussed this with Chris Wilson.
- Force buffer-rebind if the GTT size changes after a tiling change.
  (Chris Wilson)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |    5 +-
 drivers/gpu/drm/i915/i915_gem.c        |   85 ++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_gem_tiling.c |   18 ++++---
 3 files changed, 84 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e67332f..22ade39 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1566,10 +1566,11 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
-i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
+i915_gem_get_gtt_physical_size(struct drm_device *dev, uint32_t size,
+			       int tiling_mode, int stride);
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
-			    int tiling_mode, bool fenced);
+			    int tiling_mode, int stride, bool fenced);
 
 size_t
 i915_gem_get_tile_row_size(struct drm_device *dev, int tiling_mode, int stride);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e51a4cd..22a2108 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1453,16 +1453,39 @@ i915_gem_get_tile_row_size(struct drm_device *dev, int tiling_mode, int stride)
 	return stride / i915_gem_get_tile_width(dev, tiling_mode) * PAGE_SIZE;
 }
 
-uint32_t
-i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
+static uint32_t
+i915_gem_get_gtt_linear_size(struct drm_device *dev, uint32_t size,
+			     int tiling_mode, int stride)
 {
 	uint32_t gtt_size;
 
-	if (INTEL_INFO(dev)->gen >= 4 ||
-	    tiling_mode == I915_TILING_NONE)
+	if (tiling_mode == I915_TILING_NONE)
 		return size;
 
-	/* Previous chips need a power-of-two fence region when tiling */
+	/*
+	 * Applications can set tiling for objects with sizes not aligned to
+	 * the object's tile row size. This is a valid use case for example if
+	 * the application wants to reuse the same object with different tile
+	 * settings. In this case we have to limit the linear range
+	 * (programmed to the fence register) to the last full tile row
+	 * offset, otherwise users of this object would have access to the
+	 * object following it in the GTT space by reading/writing to the
+	 * unaligned part. For the unaligned part we don't have backing storage
+	 * even for a single linear pixel line, so tiling wouldn't work
+	 * properly anyway.
+	 */
+	size = rounddown(size, i915_gem_get_tile_row_size(dev, tiling_mode,
+				stride));
+	if (INTEL_INFO(dev)->gen >= 4)
+		return size;
+
+	/*
+	 * Previous chips need a power-of-two fence region when tiling. This
+	 * might set an unaligned object's linear size between its last full
+	 * tile row offset and the next tile row offset, which is a problem as
+	 * pointed out above. For these objects we'll overallocate GTT space
+	 * up to the next tile row offset, see i915_gem_get_gtt_physical_size().
+	 */
 	if (INTEL_INFO(dev)->gen == 3)
 		gtt_size = 1024*1024;
 	else
@@ -1474,6 +1497,28 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 	return gtt_size;
 }
 
+uint32_t
+i915_gem_get_gtt_physical_size(struct drm_device *dev, uint32_t size,
+			       int tiling_mode, int stride)
+{
+	uint32_t linear_size;
+
+	if (INTEL_INFO(dev)->gen >= 4 || tiling_mode == I915_TILING_NONE)
+		return size;
+
+	linear_size = i915_gem_get_gtt_linear_size(dev, size, tiling_mode,
+						  stride);
+	/*
+	 * Overallocate in case linear_size is greater than the object's last
+	 * full tile row offset and smaller than the tile row offset after
+	 * that.
+	 */
+	size = roundup(size,
+		       i915_gem_get_tile_row_size(dev, tiling_mode, stride));
+
+	return max(size, linear_size);
+}
+
 /**
  * i915_gem_get_gtt_alignment - return required GTT alignment for an object
  * @obj: object to check
@@ -1483,7 +1528,7 @@ i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
  */
 uint32_t
 i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
-			   int tiling_mode, bool fenced)
+			   int tiling_mode, int stride, bool fenced)
 {
 	/*
 	 * Minimum alignment is 4k (GTT page size), but might be greater
@@ -1497,7 +1542,7 @@ i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
 	 * Previous chips need to be aligned to the size of the smallest
 	 * fence register that can contain the object.
 	 */
-	return i915_gem_get_gtt_size(dev, size, tiling_mode);
+	return i915_gem_get_gtt_linear_size(dev, size, tiling_mode, stride);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -2542,8 +2587,11 @@ static void i965_write_fence_reg(struct drm_device *dev, int reg,
 	}
 
 	if (obj) {
-		u32 size = obj->gtt_space->size;
+		u32 size;
 
+		size = i915_gem_get_gtt_linear_size(dev, obj->base.size,
+						    obj->tiling_mode,
+						    obj->stride);
 		val = (uint64_t)((obj->gtt_offset + size - 4096) &
 				 0xfffff000) << 32;
 		val |= obj->gtt_offset & 0xfffff000;
@@ -2566,10 +2614,13 @@ static void i915_write_fence_reg(struct drm_device *dev, int reg,
 	u32 val;
 
 	if (obj) {
-		u32 size = obj->gtt_space->size;
+		u32 size;
 		int pitch_val;
 		int tile_width;
 
+		size = i915_gem_get_gtt_linear_size(dev, obj->base.size,
+						    obj->tiling_mode,
+						    obj->stride);
 		WARN((obj->gtt_offset & ~I915_FENCE_START_MASK) ||
 		     (size & -size) != size ||
 		     (obj->gtt_offset & (size - 1)),
@@ -2610,9 +2661,12 @@ static void i830_write_fence_reg(struct drm_device *dev, int reg,
 	uint32_t val;
 
 	if (obj) {
-		u32 size = obj->gtt_space->size;
+		u32 size;
 		uint32_t pitch_val;
 
+		size = i915_gem_get_gtt_linear_size(dev, obj->base.size,
+						    obj->tiling_mode,
+						    obj->stride);
 		WARN((obj->gtt_offset & ~I830_FENCE_START_MASK) ||
 		     (size & -size) != size ||
 		     (obj->gtt_offset & (size - 1)),
@@ -2903,16 +2957,17 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 		return -EINVAL;
 	}
 
-	fence_size = i915_gem_get_gtt_size(dev,
-					   obj->base.size,
-					   obj->tiling_mode);
+	fence_size = i915_gem_get_gtt_physical_size(dev, obj->base.size,
+						obj->tiling_mode, obj->stride);
 	fence_alignment = i915_gem_get_gtt_alignment(dev,
 						     obj->base.size,
-						     obj->tiling_mode, true);
+						     obj->tiling_mode,
+						     obj->stride, true);
 	unfenced_alignment =
 		i915_gem_get_gtt_alignment(dev,
 						    obj->base.size,
-						    obj->tiling_mode, false);
+						    obj->tiling_mode,
+						    obj->stride, false);
 
 	if (alignment == 0)
 		alignment = map_and_fenceable ? fence_alignment :
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 6d69d06..1af6f60 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -253,7 +253,8 @@ i915_tiling_ok(struct drm_device *dev, int stride, int size, int tiling_mode)
 
 /* Is the current GTT allocation valid for the change in tiling? */
 static bool
-i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
+i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode,
+			 int stride)
 {
 	u32 size;
 	u32 align;
@@ -261,6 +262,11 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 	if (tiling_mode == I915_TILING_NONE)
 		return true;
 
+	size = i915_gem_get_gtt_physical_size(obj->base.dev, obj->base.size,
+					      tiling_mode, stride);
+	if (obj->gtt_space->size != size)
+		return false;
+
 	if (INTEL_INFO(obj->base.dev)->gen >= 4)
 		return true;
 
@@ -272,12 +278,8 @@ i915_gem_object_fence_ok(struct drm_i915_gem_object *obj, int tiling_mode)
 			return false;
 	}
 
-	size = i915_gem_get_gtt_size(obj->base.dev, obj->base.size, tiling_mode);
-	if (obj->gtt_space->size != size)
-		return false;
-
 	align = i915_gem_get_gtt_alignment(obj->base.dev, obj->base.size,
-					   tiling_mode, true);
+					   tiling_mode, stride, true);
 
 	if (obj->gtt_offset & (align - 1))
 		return false;
@@ -361,13 +363,15 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		obj->map_and_fenceable =
 			obj->gtt_space == NULL ||
 			(obj->gtt_offset + obj->base.size <= dev_priv->mm.gtt_mappable_end &&
-			 i915_gem_object_fence_ok(obj, args->tiling_mode));
+			 i915_gem_object_fence_ok(obj, args->tiling_mode,
+						  args->stride));
 
 		/* Rebind if we need a change of alignment */
 		if (!obj->map_and_fenceable) {
 			u32 unfenced_alignment =
 				i915_gem_get_gtt_alignment(dev, obj->base.size,
 							    args->tiling_mode,
+							    args->stride,
 							    false);
 			if (obj->gtt_offset & (unfenced_alignment - 1))
 				ret = i915_gem_object_unbind(obj);
-- 
1.7.10.4

Re: [PATCH v2 7/7] drm/i915: fix gtt space allocated for tiled objects

[Chris Wilson]

On Mon,  7 Jan 2013 21:47:39 +0200, Imre Deak <imre.deak@intel.com> wrote:
> @@ -361,13 +363,15 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  		obj->map_and_fenceable =
>  			obj->gtt_space == NULL ||
>  			(obj->gtt_offset + obj->base.size <= dev_priv->mm.gtt_mappable_end &&
> -			 i915_gem_object_fence_ok(obj, args->tiling_mode));
> +			 i915_gem_object_fence_ok(obj, args->tiling_mode,
> +						  args->stride));

This is a big problem as this will now cause stalls where the code
expects none.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre