[PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

Start converting over from the byte count to its semantic macro, either
we want to allocate the size of a physical page in main memory or we
want the size of a virtual page in the GTT. 4096 could mean either, but
PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
code comprehension and future changes. In the future, we may want to use
variable GTT page sizes and so have the challenge of knowing which
hardcoded values were used to represent a physical page vs the virtual
page.

v2: Look for a few more 4096s to convert, discover IS_ALIGNED().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c              |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c      |  7 ++++---
 drivers/gpu/drm/i915/i915_gem_evict.c        |  4 ++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  5 ++---
 drivers/gpu/drm/i915/i915_gem_fence_reg.c    | 12 ++++++------
 drivers/gpu/drm/i915/i915_gem_gtt.c          | 10 +++++-----
 drivers/gpu/drm/i915/i915_gem_gtt.h          |  5 ++++-
 drivers/gpu/drm/i915/i915_gem_render_state.c |  4 ++--
 drivers/gpu/drm/i915/i915_gem_stolen.c       |  7 ++++---
 drivers/gpu/drm/i915/i915_gem_tiling.c       |  8 ++++----
 drivers/gpu/drm/i915/i915_vma.c              | 17 +++++++++++------
 drivers/gpu/drm/i915/intel_lrc.c             |  5 +++--
 drivers/gpu/drm/i915/intel_lrc.h             |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c      | 10 +++++-----
 14 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91d726f8bdfa..27d6e19ebff6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3483,7 +3483,7 @@ i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
 		return;
 
 	if (--vma->obj->pin_display == 0)
-		vma->display_alignment = 4096;
+		vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
 
 	/* Bump the LRU to try and avoid premature eviction whilst flipping  */
 	if (!i915_vma_is_active(vma))
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 40a6939e3956..ed31133b3ce3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,7 +97,7 @@
  * part. It should be safe to decrease this, but it's more future proof as is.
  */
 #define GEN6_CONTEXT_ALIGN (64<<10)
-#define GEN7_CONTEXT_ALIGN 4096
+#define GEN7_CONTEXT_ALIGN I915_GTT_MIN_ALIGNMENT
 
 static size_t get_context_alignment(struct drm_i915_private *dev_priv)
 {
@@ -341,7 +341,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	if (HAS_GUC(dev_priv) && i915.enable_guc_loading)
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
-		ctx->ggtt_offset_bias = 4096;
+		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
 
 	return ctx;
 
@@ -456,7 +456,8 @@ int i915_gem_context_init(struct drm_i915_private *dev_priv)
 		dev_priv->hw_context_size = 0;
 	} else if (HAS_HW_CONTEXTS(dev_priv)) {
 		dev_priv->hw_context_size =
-			round_up(get_context_size(dev_priv), 4096);
+			round_up(get_context_size(dev_priv),
+				 I915_GTT_PAGE_SIZE);
 		if (dev_priv->hw_context_size > (1<<20)) {
 			DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
 					 dev_priv->hw_context_size);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 026ebc5a452a..6a5415e31acf 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -264,9 +264,9 @@ int i915_gem_evict_for_vma(struct i915_vma *target, unsigned int flags)
 	if (check_color) {
 		/* Expand search to cover neighbouring guard pages (or lack!) */
 		if (start > target->vm->start)
-			start -= 4096;
+			start -= I915_GTT_PAGE_SIZE;
 		if (end < target->vm->start + target->vm->total)
-			end += 4096;
+			end += I915_GTT_PAGE_SIZE;
 	}
 
 	drm_mm_for_each_node_in_range(node, &target->vm->mm, start, end) {
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a5fe299da1d3..259fe4aa8d41 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -438,7 +438,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
 			memset(&cache->node, 0, sizeof(cache->node));
 			ret = drm_mm_insert_node_in_range_generic
 				(&ggtt->base.mm, &cache->node,
-				 4096, 0, I915_COLOR_UNEVICTABLE,
+				 PAGE_SIZE, 0, I915_COLOR_UNEVICTABLE,
 				 0, ggtt->mappable_end,
 				 DRM_MM_SEARCH_DEFAULT,
 				 DRM_MM_CREATE_DEFAULT);
@@ -851,8 +851,7 @@ eb_vma_misplaced(struct i915_vma *vma)
 	WARN_ON(entry->flags & __EXEC_OBJECT_NEEDS_MAP &&
 		!i915_vma_is_ggtt(vma));
 
-	if (entry->alignment &&
-	    vma->node.start & (entry->alignment - 1))
+	if (entry->alignment && !IS_ALIGNED(vma->node.start, entry->alignment))
 		return true;
 
 	if (vma->node.size < entry->pad_to_size)
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index 9e65696a960c..cd93468c6db2 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
 		unsigned int stride = i915_gem_object_get_stride(vma->obj);
 
 		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
-		GEM_BUG_ON(vma->node.start & 4095);
-		GEM_BUG_ON(vma->fence_size & 4095);
-		GEM_BUG_ON(stride & 127);
+		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
+		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));
+		GEM_BUG_ON(!IS_ALIGNED(stride, 128));
 
-		val = (vma->node.start + vma->fence_size - 4096) << 32;
+		val = (vma->node.start + vma->fence_size - I915_GTT_PAGE_SIZE) << 32;
 		val |= vma->node.start;
 		val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
 		if (i915_gem_object_get_tiling(vma->obj) == I915_TILING_Y)
@@ -127,7 +127,7 @@ static void i915_write_fence_reg(struct drm_i915_fence_reg *fence,
 		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
 		GEM_BUG_ON(vma->node.start & ~I915_FENCE_START_MASK);
 		GEM_BUG_ON(!is_power_of_2(vma->fence_size));
-		GEM_BUG_ON(vma->node.start & (vma->fence_size - 1));
+		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, vma->fence_size));
 
 		if (is_y_tiled && HAS_128_BYTE_Y_TILING(fence->i915))
 			stride /= 128;
@@ -166,7 +166,7 @@ static void i830_write_fence_reg(struct drm_i915_fence_reg *fence,
 		GEM_BUG_ON(vma->node.start & ~I830_FENCE_START_MASK);
 		GEM_BUG_ON(!is_power_of_2(vma->fence_size));
 		GEM_BUG_ON(!is_power_of_2(stride / 128));
-		GEM_BUG_ON(vma->node.start & (vma->fence_size - 1));
+		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, vma->fence_size));
 
 		val = vma->node.start;
 		if (i915_gem_object_get_tiling(vma->obj) == I915_TILING_Y)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2d7ab1d35e47..8aca11f5f446 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -329,7 +329,7 @@ static int __setup_page_dma(struct drm_i915_private *dev_priv,
 		return -ENOMEM;
 
 	p->daddr = dma_map_page(kdev,
-				p->page, 0, 4096, PCI_DMA_BIDIRECTIONAL);
+				p->page, 0, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
 
 	if (dma_mapping_error(kdev, p->daddr)) {
 		__free_page(p->page);
@@ -353,7 +353,7 @@ static void cleanup_page_dma(struct drm_i915_private *dev_priv,
 	if (WARN_ON(!p->page))
 		return;
 
-	dma_unmap_page(&pdev->dev, p->daddr, 4096, PCI_DMA_BIDIRECTIONAL);
+	dma_unmap_page(&pdev->dev, p->daddr, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
 	__free_page(p->page);
 	memset(p, 0, sizeof(*p));
 }
@@ -2711,11 +2711,11 @@ static void i915_gtt_color_adjust(const struct drm_mm_node *node,
 				  u64 *end)
 {
 	if (node->color != color)
-		*start += 4096;
+		*start += I915_GTT_PAGE_SIZE;
 
 	node = list_next_entry(node, node_list);
 	if (node->allocated && node->color != color)
-		*end -= 4096;
+		*end -= I915_GTT_PAGE_SIZE;
 }
 
 int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
@@ -2742,7 +2742,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	/* Reserve a mappable slot for our lockless error capture */
 	ret = drm_mm_insert_node_in_range_generic(&ggtt->base.mm,
 						  &ggtt->error_capture,
-						  4096, 0,
+						  PAGE_SIZE, 0,
 						  I915_COLOR_UNEVICTABLE,
 						  0, ggtt->mappable_end,
 						  0, 0);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9e91d7e6149c..ca470103c2d9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -40,6 +40,9 @@
 #include "i915_gem_timeline.h"
 #include "i915_gem_request.h"
 
+#define I915_GTT_PAGE_SIZE 4096
+#define I915_GTT_MIN_ALIGNMENT I915_GTT_PAGE_SIZE
+
 #define I915_FENCE_REG_NONE -1
 #define I915_MAX_NUM_FENCES 32
 /* 32 fences + sign bit for FENCE_REG_NONE */
@@ -543,6 +546,6 @@ void i915_gem_gtt_finish_pages(struct drm_i915_gem_object *obj,
 #define PIN_HIGH		BIT(9)
 #define PIN_OFFSET_BIAS		BIT(10)
 #define PIN_OFFSET_FIXED	BIT(11)
-#define PIN_OFFSET_MASK		(~4095)
+#define PIN_OFFSET_MASK		(-I915_GTT_PAGE_SIZE)
 
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 5af19b0bf713..63ae7e813335 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -187,14 +187,14 @@ int i915_gem_render_state_init(struct intel_engine_cs *engine)
 	if (!rodata)
 		return 0;
 
-	if (rodata->batch_items * 4 > 4096)
+	if (rodata->batch_items * 4 > PAGE_SIZE)
 		return -EINVAL;
 
 	so = kmalloc(sizeof(*so), GFP_KERNEL);
 	if (!so)
 		return -ENOMEM;
 
-	obj = i915_gem_object_create_internal(engine->i915, 4096);
+	obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
 	if (IS_ERR(obj)) {
 		ret = PTR_ERR(obj);
 		goto err_free;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index f1a1d33febcd..985302448879 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -485,7 +485,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 	stolen_usable_start = 0;
 	/* WaSkipStolenMemoryFirstPage:bdw+ */
 	if (INTEL_GEN(dev_priv) >= 8)
-		stolen_usable_start = 4096;
+		stolen_usable_start = PAGE_SIZE;
 
 	ggtt->stolen_usable_size =
 		ggtt->stolen_size - reserved_total - stolen_usable_start;
@@ -647,8 +647,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *dev_priv
 			stolen_offset, gtt_offset, size);
 
 	/* KISS and expect everything to be page-aligned */
-	if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
-	    WARN_ON(stolen_offset & 4095))
+	if (WARN_ON(size == 0) ||
+	    WARN_ON(!IS_ALIGNED(size, I915_GTT_PAGE_SIZE)) ||
+	    WARN_ON(!IS_ALIGNED(stolen_offset, I915_GTT_MIN_ALIGNMENT)))
 		return NULL;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 4f83e331f598..28568f6fa834 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -82,7 +82,7 @@ u32 i915_gem_fence_size(struct drm_i915_private *i915,
 
 	if (INTEL_GEN(i915) >= 4) {
 		stride *= i915_gem_tile_height(tiling);
-		GEM_BUG_ON(stride & 4095);
+		GEM_BUG_ON(!IS_ALIGNED(stride, I915_GTT_PAGE_SIZE));
 		return roundup(size, stride);
 	}
 
@@ -118,7 +118,7 @@ u32 i915_gem_fence_alignment(struct drm_i915_private *i915, u32 size,
 	 * if a fence register is needed for the object.
 	 */
 	if (INTEL_GEN(i915) >= 4 || tiling == I915_TILING_NONE)
-		return 4096;
+		return I915_GTT_MIN_ALIGNMENT;
 
 	/*
 	 * Previous chips need to be aligned to the size of the smallest
@@ -170,7 +170,7 @@ i915_tiling_ok(struct drm_i915_gem_object *obj,
 	else
 		tile_width = 512;
 
-	if (stride & (tile_width - 1))
+	if (!IS_ALIGNED(stride, tile_width))
 		return false;
 
 	/* 965+ just needs multiples of tile width */
@@ -195,7 +195,7 @@ static bool i915_vma_fence_prepare(struct i915_vma *vma,
 		return false;
 
 	alignment = i915_gem_fence_alignment(i915, vma->size, tiling_mode, stride);
-	if (vma->node.start & (alignment - 1))
+	if (!IS_ALIGNED(vma->node.start, alignment))
 		return false;
 
 	return true;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index f137475fab51..c3073a9cfaed 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -91,7 +91,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
 	vma->vm = vm;
 	vma->obj = obj;
 	vma->size = obj->base.size;
-	vma->display_alignment = 4096;
+	vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
 
 	if (view) {
 		vma->ggtt_view = *view;
@@ -115,7 +115,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
 		vma->fence_size = i915_gem_fence_size(vm->i915, vma->size,
 						      i915_gem_object_get_tiling(obj),
 						      i915_gem_object_get_stride(obj));
-		GEM_BUG_ON(vma->fence_size & 4095);
+		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_MIN_ALIGNMENT));
 
 		vma->fence_alignment = i915_gem_fence_alignment(vm->i915, vma->size,
 								i915_gem_object_get_tiling(obj),
@@ -270,7 +270,8 @@ i915_vma_misplaced(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	if (vma->node.size < size)
 		return true;
 
-	if (alignment && vma->node.start & (alignment - 1))
+	GEM_BUG_ON(alignment && !is_power_of_2(alignment));
+	if (alignment && !IS_ALIGNED(vma->node.start, alignment))
 		return true;
 
 	if (flags & PIN_MAPPABLE && !i915_vma_is_map_and_fenceable(vma))
@@ -302,7 +303,7 @@ void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
 		return;
 
 	fenceable = (vma->node.size >= vma->fence_size &&
-		     (vma->node.start & (vma->fence_alignment - 1)) == 0);
+		     IS_ALIGNED(vma->node.start, vma->fence_alignment));
 
 	mappable = vma->node.start + vma->fence_size <= i915_vm_to_ggtt(vma->vm)->mappable_end;
 
@@ -380,6 +381,10 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 				  alignment, vma->fence_alignment);
 	}
 
+	GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_PAGE_SIZE));
+	GEM_BUG_ON(!IS_ALIGNED(alignment, I915_GTT_MIN_ALIGNMENT));
+	GEM_BUG_ON(!is_power_of_2(alignment));
+
 	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
 
 	end = vma->vm->total;
@@ -406,7 +411,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 
 	if (flags & PIN_OFFSET_FIXED) {
 		u64 offset = flags & PIN_OFFSET_MASK;
-		if (offset & (alignment - 1) ||
+		if (!IS_ALIGNED(offset, alignment) ||
 		    range_overflows(offset, size, end)) {
 			ret = -EINVAL;
 			goto err_unpin;
@@ -440,7 +445,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 		 * with zero alignment, so where possible use the optimal
 		 * path.
 		 */
-		if (alignment <= 4096)
+		if (alignment <= I915_GTT_MIN_ALIGNMENT)
 			alignment = 0;
 
 search_free:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6db246ad2f13..81665a9eb43f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1927,7 +1927,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
 	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
 
-	ret = intel_engine_create_scratch(engine, 4096);
+	ret = intel_engine_create_scratch(engine, PAGE_SIZE);
 	if (ret)
 		return ret;
 
@@ -2209,7 +2209,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 
 	WARN_ON(ce->state);
 
-	context_size = round_up(intel_lr_context_size(engine), 4096);
+	context_size = round_up(intel_lr_context_size(engine),
+				I915_GTT_PAGE_SIZE);
 
 	/* One extra page as the sharing data between driver and GuC */
 	context_size += PAGE_SIZE * LRC_PPHWSP_PN;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 01ba36ea125e..0c852c024227 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -26,7 +26,7 @@
 
 #include "intel_ringbuffer.h"
 
-#define GEN8_LR_CONTEXT_ALIGN 4096
+#define GEN8_LR_CONTEXT_ALIGN I915_GTT_MIN_ALIGNMENT
 
 /* Execlists regs */
 #define RING_ELSP(engine)			_MMIO((engine)->mmio_base + 0x230)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0971ac396b60..ab83fc22d207 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1736,7 +1736,7 @@ static int init_status_page(struct intel_engine_cs *engine)
 	void *vaddr;
 	int ret;
 
-	obj = i915_gem_object_create_internal(engine->i915, 4096);
+	obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
 	if (IS_ERR(obj)) {
 		DRM_ERROR("Failed to allocate status page\n");
 		return PTR_ERR(obj);
@@ -1777,7 +1777,7 @@ static int init_status_page(struct intel_engine_cs *engine)
 
 	engine->status_page.vma = vma;
 	engine->status_page.ggtt_offset = i915_ggtt_offset(vma);
-	engine->status_page.page_addr = memset(vaddr, 0, 4096);
+	engine->status_page.page_addr = memset(vaddr, 0, PAGE_SIZE);
 
 	DRM_DEBUG_DRIVER("%s hws offset: 0x%08x\n",
 			 engine->name, i915_ggtt_offset(vma));
@@ -2049,7 +2049,7 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
 	}
 
 	/* Ring wraparound at offset 0 sometimes hangs. No idea why. */
-	ret = intel_ring_pin(ring, 4096);
+	ret = intel_ring_pin(ring, I915_GTT_PAGE_SIZE);
 	if (ret) {
 		intel_ring_free(ring);
 		goto error;
@@ -2466,7 +2466,7 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
 	if (INTEL_GEN(dev_priv) >= 8 && !dev_priv->semaphore) {
 		struct i915_vma *vma;
 
-		obj = i915_gem_object_create(dev_priv, 4096);
+		obj = i915_gem_object_create(dev_priv, PAGE_SIZE);
 		if (IS_ERR(obj))
 			goto err;
 
@@ -2683,7 +2683,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 		return ret;
 
 	if (INTEL_GEN(dev_priv) >= 6) {
-		ret = intel_engine_create_scratch(engine, 4096);
+		ret = intel_engine_create_scratch(engine, PAGE_SIZE);
 		if (ret)
 			return ret;
 	} else if (HAS_BROKEN_CS_TLB(dev_priv)) {
-- 
2.11.0

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

Re: [PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Joonas Lahtinen]

On ti, 2017-01-10 at 12:15 +0000, Chris Wilson wrote:
> Start converting over from the byte count to its semantic macro, either
> we want to allocate the size of a physical page in main memory or we
> want the size of a virtual page in the GTT. 4096 could mean either, but
> PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> code comprehension and future changes. In the future, we may want to use
> variable GTT page sizes and so have the challenge of knowing which
> hardcoded values were used to represent a physical page vs the virtual
> page.
> 
> v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> 
> @@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
> >  		unsigned int stride = i915_gem_object_get_stride(vma->obj);
>  
>  		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
> -		GEM_BUG_ON(vma->node.start & 4095);
> -		GEM_BUG_ON(vma->fence_size & 4095);
> -		GEM_BUG_ON(stride & 127);
> +		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
> +		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));

GTT_MIN_ALIGN would make more sense here? I don't think this test
should change if page size increases.

> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -40,6 +40,9 @@
>  #include "i915_gem_timeline.h"
>  #include "i915_gem_request.h"
>  
> +#define I915_GTT_PAGE_SIZE 4096

Could be 4096UL (to mimic PAGE_SIZE) as discussed in IRC.

> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -187,14 +187,14 @@ int i915_gem_render_state_init(struct intel_engine_cs *engine)
>  	if (!rodata)
>  		return 0;
>  
> -	if (rodata->batch_items * 4 > 4096)
> +	if (rodata->batch_items * 4 > PAGE_SIZE)
>  		return -EINVAL;

Umm, how is not render state tied to GT page size?

> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -485,7 +485,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  	stolen_usable_start = 0;
>  	/* WaSkipStolenMemoryFirstPage:bdw+ */
>  	if (INTEL_GEN(dev_priv) >= 8)
> -		stolen_usable_start = 4096;
> +		stolen_usable_start = PAGE_SIZE;

This is again borderline. Maybe we should have I915_GTT_MIN_PAGE_SIZE
and use it for MIN_SIZE and MIN_ALIGNMENT? It would also be self-
documenting not to necessarily be the actual page size for the object
in question?

> @@ -82,7 +82,7 @@ u32 i915_gem_fence_size(struct drm_i915_private *i915,
>  
>  	if (INTEL_GEN(i915) >= 4) {
>  		stride *= i915_gem_tile_height(tiling);
> -		GEM_BUG_ON(stride & 4095);
> +		GEM_BUG_ON(!IS_ALIGNED(stride, I915_GTT_PAGE_SIZE));

MIN_ALIGN would read better here.

> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1927,7 +1927,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
>  	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
>  	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
>  
> -	ret = intel_engine_create_scratch(engine, 4096);
> +	ret = intel_engine_create_scratch(engine, PAGE_SIZE);

GTT_PAGE?

The ones after this point seem to follow the logic of getting accessed
from CPU and are thus PAGE_SIZE. So maybe max(PAGE_SIZE,
GTT_MIN_PAGE_SIZE) as unified_page_size :P Others could comment here,
too.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

On Tue, Jan 10, 2017 at 03:55:35PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-01-10 at 12:15 +0000, Chris Wilson wrote:
> > Start converting over from the byte count to its semantic macro, either
> > we want to allocate the size of a physical page in main memory or we
> > want the size of a virtual page in the GTT. 4096 could mean either, but
> > PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> > code comprehension and future changes. In the future, we may want to use
> > variable GTT page sizes and so have the challenge of knowing which
> > hardcoded values were used to represent a physical page vs the virtual
> > page.
> > 
> > v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > 
> > @@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
> > >  		unsigned int stride = i915_gem_object_get_stride(vma->obj);
> >  
> >  		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
> > -		GEM_BUG_ON(vma->node.start & 4095);
> > -		GEM_BUG_ON(vma->fence_size & 4095);
> > -		GEM_BUG_ON(stride & 127);
> > +		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
> > +		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));
> 
> GTT_MIN_ALIGN would make more sense here? I don't think this test
> should change if page size increases.

Yes, MIN_ALIGN makes more sense here.
 
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -40,6 +40,9 @@
> >  #include "i915_gem_timeline.h"
> >  #include "i915_gem_request.h"
> >  
> > +#define I915_GTT_PAGE_SIZE 4096
> 
> Could be 4096UL (to mimic PAGE_SIZE) as discussed in IRC.
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> > @@ -187,14 +187,14 @@ int i915_gem_render_state_init(struct intel_engine_cs *engine)
> >  	if (!rodata)
> >  		return 0;
> >  
> > -	if (rodata->batch_items * 4 > 4096)
> > +	if (rodata->batch_items * 4 > PAGE_SIZE)
> >  		return -EINVAL;
> 
> Umm, how is not render state tied to GT page size?

We only allocated a physical page for the render state, not a virtual page.

> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -485,7 +485,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
> >  	stolen_usable_start = 0;
> >  	/* WaSkipStolenMemoryFirstPage:bdw+ */
> >  	if (INTEL_GEN(dev_priv) >= 8)
> > -		stolen_usable_start = 4096;
> > +		stolen_usable_start = PAGE_SIZE;
> 
> This is again borderline. Maybe we should have I915_GTT_MIN_PAGE_SIZE
> and use it for MIN_SIZE and MIN_ALIGNMENT? It would also be self-
> documenting not to necessarily be the actual page size for the object
> in question?

Probably best left as 4096 as I have no idea what the hw is actually
doing.

> > @@ -82,7 +82,7 @@ u32 i915_gem_fence_size(struct drm_i915_private *i915,
> >  
> >  	if (INTEL_GEN(i915) >= 4) {
> >  		stride *= i915_gem_tile_height(tiling);
> > -		GEM_BUG_ON(stride & 4095);
> > +		GEM_BUG_ON(!IS_ALIGNED(stride, I915_GTT_PAGE_SIZE));
> 
> MIN_ALIGN would read better here.
> 
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1927,7 +1927,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
> >  	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
> >  	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
> >  
> > -	ret = intel_engine_create_scratch(engine, 4096);
> > +	ret = intel_engine_create_scratch(engine, PAGE_SIZE);
> 
> GTT_PAGE?

We allocate 1 physical page for scratch.
 
> The ones after this point seem to follow the logic of getting accessed
> from CPU and are thus PAGE_SIZE. So maybe max(PAGE_SIZE,
> GTT_MIN_PAGE_SIZE) as unified_page_size :P Others could comment here,
> too.

Right, it the object allocation ones that are the most contentious - are
we only allocating a physical page (such as for scratch / hws) or do we
want to occupy a full GTT_PAGE. A bit like asking if we want to allocate
a 2+GiB physical page where only 4096 bytes would do. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

On Tue, Jan 10, 2017 at 03:55:35PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-01-10 at 12:15 +0000, Chris Wilson wrote:
> > Start converting over from the byte count to its semantic macro, either
> > we want to allocate the size of a physical page in main memory or we
> > want the size of a virtual page in the GTT. 4096 could mean either, but
> > PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> > code comprehension and future changes. In the future, we may want to use
> > variable GTT page sizes and so have the challenge of knowing which
> > hardcoded values were used to represent a physical page vs the virtual
> > page.
> > 
> > v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > 
> > @@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
> > >  		unsigned int stride = i915_gem_object_get_stride(vma->obj);
> >  
> >  		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
> > -		GEM_BUG_ON(vma->node.start & 4095);
> > -		GEM_BUG_ON(vma->fence_size & 4095);
> > -		GEM_BUG_ON(stride & 127);
> > +		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
> > +		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));
> 
> GTT_MIN_ALIGN would make more sense here? I don't think this test
> should change if page size increases.

On the other hand, the 4096 here is FENCE_PAGE units. So perhaps better
as


diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index cd93468c6db2..d3bcacea63de 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -60,10 +60,14 @@
 static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
                                 struct i915_vma *vma)
 {
+#define I965_FENCE_PAGE 4096UL
        i915_reg_t fence_reg_lo, fence_reg_hi;
        int fence_pitch_shift;
        u64 val;
 
+       /* Make sure that we are consistent with i915_gem_fence_alignment() */
+       BUILD_BUG_ON(!IS_ALIGNED(I915_GTT_MIN_ALIGNMENT, I965_FENCE_PAGE));
+
        if (INTEL_INFO(fence->i915)->gen >= 6) {
                fence_reg_lo = FENCE_REG_GEN6_LO(fence->id);
                fence_reg_hi = FENCE_REG_GEN6_HI(fence->id);
@@ -80,11 +84,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
                unsigned int stride = i915_gem_object_get_stride(vma->obj);
 
                GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
-               GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
-               GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));
+               GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I965_FENCE_PAGE));
+               GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I965_FENCE_PAGE));
                GEM_BUG_ON(!IS_ALIGNED(stride, 128));
 
-               val = (vma->node.start + vma->fence_size - I915_GTT_PAGE_SIZE) << 32;
+               val = (vma->node.start + vma->fence_size - I965_FENCE_PAGE) << 32;
                val |= vma->node.start;
                val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
                if (i915_gem_object_get_tiling(vma->obj) == I915_TILING_Y)
@@ -111,6 +115,7 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
                I915_WRITE(fence_reg_lo, lower_32_bits(val));
                POSTING_READ(fence_reg_lo);
        }
+#undef I965_FENCE_PAGE
 }


-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

On Tue, Jan 10, 2017 at 03:55:35PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-01-10 at 12:15 +0000, Chris Wilson wrote:
> > Start converting over from the byte count to its semantic macro, either
> > we want to allocate the size of a physical page in main memory or we
> > want the size of a virtual page in the GTT. 4096 could mean either, but
> > PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> > code comprehension and future changes. In the future, we may want to use
> > variable GTT page sizes and so have the challenge of knowing which
> > hardcoded values were used to represent a physical page vs the virtual
> > page.
> > 
> > v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > 
> > @@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
> > >  		unsigned int stride = i915_gem_object_get_stride(vma->obj);
> >  
> >  		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
> > -		GEM_BUG_ON(vma->node.start & 4095);
> > -		GEM_BUG_ON(vma->fence_size & 4095);
> > -		GEM_BUG_ON(stride & 127);
> > +		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
> > +		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));
> 
> GTT_MIN_ALIGN would make more sense here? I don't think this test
> should change if page size increases.
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -40,6 +40,9 @@
> >  #include "i915_gem_timeline.h"
> >  #include "i915_gem_request.h"
> >  
> > +#define I915_GTT_PAGE_SIZE 4096
> 
> Could be 4096UL (to mimic PAGE_SIZE) as discussed in IRC.
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> > @@ -187,14 +187,14 @@ int i915_gem_render_state_init(struct intel_engine_cs *engine)
> >  	if (!rodata)
> >  		return 0;
> >  
> > -	if (rodata->batch_items * 4 > 4096)
> > +	if (rodata->batch_items * 4 > PAGE_SIZE)
> >  		return -EINVAL;
> 
> Umm, how is not render state tied to GT page size?
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -485,7 +485,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
> >  	stolen_usable_start = 0;
> >  	/* WaSkipStolenMemoryFirstPage:bdw+ */
> >  	if (INTEL_GEN(dev_priv) >= 8)
> > -		stolen_usable_start = 4096;
> > +		stolen_usable_start = PAGE_SIZE;
> 
> This is again borderline. Maybe we should have I915_GTT_MIN_PAGE_SIZE
> and use it for MIN_SIZE and MIN_ALIGNMENT? It would also be self-
> documenting not to necessarily be the actual page size for the object
> in question?
> 
> > @@ -82,7 +82,7 @@ u32 i915_gem_fence_size(struct drm_i915_private *i915,
> >  
> >  	if (INTEL_GEN(i915) >= 4) {
> >  		stride *= i915_gem_tile_height(tiling);
> > -		GEM_BUG_ON(stride & 4095);
> > +		GEM_BUG_ON(!IS_ALIGNED(stride, I915_GTT_PAGE_SIZE));
> 
> MIN_ALIGN would read better here.

Now using I965_FENCE_PAGE (a new 4096ul from i915_gem_fence_reg.h).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

Start converting over from the byte count to its semantic macro, either
we want to allocate the size of a physical page in main memory or we
want the size of a virtual page in the GTT. 4096 could mean either, but
PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
code comprehension and future changes. In the future, we may want to use
variable GTT page sizes and so have the challenge of knowing which
hardcoded values were used to represent a physical page vs the virtual
page.

v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
v3: 4096ul paranoia, make fence alignment a distinct value of 4096, keep
bdw stolen w/a as 4096 until we know better.
v4: Add asserts that i915_vma_insert() start/end are aligned to GTT page
sizes.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c              |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c      |  7 ++++---
 drivers/gpu/drm/i915/i915_gem_evict.c        |  4 ++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  5 ++---
 drivers/gpu/drm/i915/i915_gem_fence_reg.c    | 12 ++++++------
 drivers/gpu/drm/i915/i915_gem_fence_reg.h    |  2 ++
 drivers/gpu/drm/i915/i915_gem_gtt.c          | 10 +++++-----
 drivers/gpu/drm/i915/i915_gem_gtt.h          |  5 ++++-
 drivers/gpu/drm/i915/i915_gem_render_state.c |  4 ++--
 drivers/gpu/drm/i915/i915_gem_stolen.c       |  5 +++--
 drivers/gpu/drm/i915/i915_gem_tiling.c       | 13 ++++++++-----
 drivers/gpu/drm/i915/i915_vma.c              | 21 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_lrc.c             |  5 +++--
 drivers/gpu/drm/i915/intel_lrc.h             |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c      | 10 +++++-----
 15 files changed, 62 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91d726f8bdfa..27d6e19ebff6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3483,7 +3483,7 @@ i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
 		return;
 
 	if (--vma->obj->pin_display == 0)
-		vma->display_alignment = 4096;
+		vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
 
 	/* Bump the LRU to try and avoid premature eviction whilst flipping  */
 	if (!i915_vma_is_active(vma))
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 40a6939e3956..ed31133b3ce3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,7 +97,7 @@
  * part. It should be safe to decrease this, but it's more future proof as is.
  */
 #define GEN6_CONTEXT_ALIGN (64<<10)
-#define GEN7_CONTEXT_ALIGN 4096
+#define GEN7_CONTEXT_ALIGN I915_GTT_MIN_ALIGNMENT
 
 static size_t get_context_alignment(struct drm_i915_private *dev_priv)
 {
@@ -341,7 +341,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	if (HAS_GUC(dev_priv) && i915.enable_guc_loading)
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
-		ctx->ggtt_offset_bias = 4096;
+		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
 
 	return ctx;
 
@@ -456,7 +456,8 @@ int i915_gem_context_init(struct drm_i915_private *dev_priv)
 		dev_priv->hw_context_size = 0;
 	} else if (HAS_HW_CONTEXTS(dev_priv)) {
 		dev_priv->hw_context_size =
-			round_up(get_context_size(dev_priv), 4096);
+			round_up(get_context_size(dev_priv),
+				 I915_GTT_PAGE_SIZE);
 		if (dev_priv->hw_context_size > (1<<20)) {
 			DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
 					 dev_priv->hw_context_size);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 026ebc5a452a..6a5415e31acf 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -264,9 +264,9 @@ int i915_gem_evict_for_vma(struct i915_vma *target, unsigned int flags)
 	if (check_color) {
 		/* Expand search to cover neighbouring guard pages (or lack!) */
 		if (start > target->vm->start)
-			start -= 4096;
+			start -= I915_GTT_PAGE_SIZE;
 		if (end < target->vm->start + target->vm->total)
-			end += 4096;
+			end += I915_GTT_PAGE_SIZE;
 	}
 
 	drm_mm_for_each_node_in_range(node, &target->vm->mm, start, end) {
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a5fe299da1d3..259fe4aa8d41 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -438,7 +438,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
 			memset(&cache->node, 0, sizeof(cache->node));
 			ret = drm_mm_insert_node_in_range_generic
 				(&ggtt->base.mm, &cache->node,
-				 4096, 0, I915_COLOR_UNEVICTABLE,
+				 PAGE_SIZE, 0, I915_COLOR_UNEVICTABLE,
 				 0, ggtt->mappable_end,
 				 DRM_MM_SEARCH_DEFAULT,
 				 DRM_MM_CREATE_DEFAULT);
@@ -851,8 +851,7 @@ eb_vma_misplaced(struct i915_vma *vma)
 	WARN_ON(entry->flags & __EXEC_OBJECT_NEEDS_MAP &&
 		!i915_vma_is_ggtt(vma));
 
-	if (entry->alignment &&
-	    vma->node.start & (entry->alignment - 1))
+	if (entry->alignment && !IS_ALIGNED(vma->node.start, entry->alignment))
 		return true;
 
 	if (vma->node.size < entry->pad_to_size)
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index 9e65696a960c..fadbe8f4c745 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
 		unsigned int stride = i915_gem_object_get_stride(vma->obj);
 
 		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
-		GEM_BUG_ON(vma->node.start & 4095);
-		GEM_BUG_ON(vma->fence_size & 4095);
-		GEM_BUG_ON(stride & 127);
+		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I965_FENCE_PAGE));
+		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I965_FENCE_PAGE));
+		GEM_BUG_ON(!IS_ALIGNED(stride, 128));
 
-		val = (vma->node.start + vma->fence_size - 4096) << 32;
+		val = (vma->node.start + vma->fence_size - I965_FENCE_PAGE) << 32;
 		val |= vma->node.start;
 		val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
 		if (i915_gem_object_get_tiling(vma->obj) == I915_TILING_Y)
@@ -127,7 +127,7 @@ static void i915_write_fence_reg(struct drm_i915_fence_reg *fence,
 		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
 		GEM_BUG_ON(vma->node.start & ~I915_FENCE_START_MASK);
 		GEM_BUG_ON(!is_power_of_2(vma->fence_size));
-		GEM_BUG_ON(vma->node.start & (vma->fence_size - 1));
+		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, vma->fence_size));
 
 		if (is_y_tiled && HAS_128_BYTE_Y_TILING(fence->i915))
 			stride /= 128;
@@ -166,7 +166,7 @@ static void i830_write_fence_reg(struct drm_i915_fence_reg *fence,
 		GEM_BUG_ON(vma->node.start & ~I830_FENCE_START_MASK);
 		GEM_BUG_ON(!is_power_of_2(vma->fence_size));
 		GEM_BUG_ON(!is_power_of_2(stride / 128));
-		GEM_BUG_ON(vma->node.start & (vma->fence_size - 1));
+		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, vma->fence_size));
 
 		val = vma->node.start;
 		if (i915_gem_object_get_tiling(vma->obj) == I915_TILING_Y)
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.h b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
index 22c4a2d01adf..8a608ae8ac1b 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.h
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
@@ -47,5 +47,7 @@ struct drm_i915_fence_reg {
 	bool dirty;
 };
 
+#define I965_FENCE_PAGE 4096UL
+
 #endif
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2d7ab1d35e47..8aca11f5f446 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -329,7 +329,7 @@ static int __setup_page_dma(struct drm_i915_private *dev_priv,
 		return -ENOMEM;
 
 	p->daddr = dma_map_page(kdev,
-				p->page, 0, 4096, PCI_DMA_BIDIRECTIONAL);
+				p->page, 0, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
 
 	if (dma_mapping_error(kdev, p->daddr)) {
 		__free_page(p->page);
@@ -353,7 +353,7 @@ static void cleanup_page_dma(struct drm_i915_private *dev_priv,
 	if (WARN_ON(!p->page))
 		return;
 
-	dma_unmap_page(&pdev->dev, p->daddr, 4096, PCI_DMA_BIDIRECTIONAL);
+	dma_unmap_page(&pdev->dev, p->daddr, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
 	__free_page(p->page);
 	memset(p, 0, sizeof(*p));
 }
@@ -2711,11 +2711,11 @@ static void i915_gtt_color_adjust(const struct drm_mm_node *node,
 				  u64 *end)
 {
 	if (node->color != color)
-		*start += 4096;
+		*start += I915_GTT_PAGE_SIZE;
 
 	node = list_next_entry(node, node_list);
 	if (node->allocated && node->color != color)
-		*end -= 4096;
+		*end -= I915_GTT_PAGE_SIZE;
 }
 
 int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
@@ -2742,7 +2742,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	/* Reserve a mappable slot for our lockless error capture */
 	ret = drm_mm_insert_node_in_range_generic(&ggtt->base.mm,
 						  &ggtt->error_capture,
-						  4096, 0,
+						  PAGE_SIZE, 0,
 						  I915_COLOR_UNEVICTABLE,
 						  0, ggtt->mappable_end,
 						  0, 0);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9e91d7e6149c..34a4fd560fa2 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -40,6 +40,9 @@
 #include "i915_gem_timeline.h"
 #include "i915_gem_request.h"
 
+#define I915_GTT_PAGE_SIZE 4096UL
+#define I915_GTT_MIN_ALIGNMENT I915_GTT_PAGE_SIZE
+
 #define I915_FENCE_REG_NONE -1
 #define I915_MAX_NUM_FENCES 32
 /* 32 fences + sign bit for FENCE_REG_NONE */
@@ -543,6 +546,6 @@ void i915_gem_gtt_finish_pages(struct drm_i915_gem_object *obj,
 #define PIN_HIGH		BIT(9)
 #define PIN_OFFSET_BIAS		BIT(10)
 #define PIN_OFFSET_FIXED	BIT(11)
-#define PIN_OFFSET_MASK		(~4095)
+#define PIN_OFFSET_MASK		(-I915_GTT_PAGE_SIZE)
 
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 5af19b0bf713..63ae7e813335 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -187,14 +187,14 @@ int i915_gem_render_state_init(struct intel_engine_cs *engine)
 	if (!rodata)
 		return 0;
 
-	if (rodata->batch_items * 4 > 4096)
+	if (rodata->batch_items * 4 > PAGE_SIZE)
 		return -EINVAL;
 
 	so = kmalloc(sizeof(*so), GFP_KERNEL);
 	if (!so)
 		return -ENOMEM;
 
-	obj = i915_gem_object_create_internal(engine->i915, 4096);
+	obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
 	if (IS_ERR(obj)) {
 		ret = PTR_ERR(obj);
 		goto err_free;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index f1a1d33febcd..e5be8e04bf3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -647,8 +647,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *dev_priv
 			stolen_offset, gtt_offset, size);
 
 	/* KISS and expect everything to be page-aligned */
-	if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
-	    WARN_ON(stolen_offset & 4095))
+	if (WARN_ON(size == 0) ||
+	    WARN_ON(!IS_ALIGNED(size, I915_GTT_PAGE_SIZE)) ||
+	    WARN_ON(!IS_ALIGNED(stolen_offset, I915_GTT_MIN_ALIGNMENT)))
 		return NULL;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 4f83e331f598..b1361cfd4c5c 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -82,7 +82,7 @@ u32 i915_gem_fence_size(struct drm_i915_private *i915,
 
 	if (INTEL_GEN(i915) >= 4) {
 		stride *= i915_gem_tile_height(tiling);
-		GEM_BUG_ON(stride & 4095);
+		GEM_BUG_ON(!IS_ALIGNED(stride, I965_FENCE_PAGE));
 		return roundup(size, stride);
 	}
 
@@ -117,8 +117,11 @@ u32 i915_gem_fence_alignment(struct drm_i915_private *i915, u32 size,
 	 * Minimum alignment is 4k (GTT page size), but might be greater
 	 * if a fence register is needed for the object.
 	 */
-	if (INTEL_GEN(i915) >= 4 || tiling == I915_TILING_NONE)
-		return 4096;
+	if (tiling == I915_TILING_NONE)
+		return I915_GTT_MIN_ALIGNMENT;
+
+	if (INTEL_GEN(i915) >= 4)
+		return I965_FENCE_PAGE;
 
 	/*
 	 * Previous chips need to be aligned to the size of the smallest
@@ -170,7 +173,7 @@ i915_tiling_ok(struct drm_i915_gem_object *obj,
 	else
 		tile_width = 512;
 
-	if (stride & (tile_width - 1))
+	if (!IS_ALIGNED(stride, tile_width))
 		return false;
 
 	/* 965+ just needs multiples of tile width */
@@ -195,7 +198,7 @@ static bool i915_vma_fence_prepare(struct i915_vma *vma,
 		return false;
 
 	alignment = i915_gem_fence_alignment(i915, vma->size, tiling_mode, stride);
-	if (vma->node.start & (alignment - 1))
+	if (!IS_ALIGNED(vma->node.start, alignment))
 		return false;
 
 	return true;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index f137475fab51..490914f89663 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -91,7 +91,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
 	vma->vm = vm;
 	vma->obj = obj;
 	vma->size = obj->base.size;
-	vma->display_alignment = 4096;
+	vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
 
 	if (view) {
 		vma->ggtt_view = *view;
@@ -115,7 +115,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
 		vma->fence_size = i915_gem_fence_size(vm->i915, vma->size,
 						      i915_gem_object_get_tiling(obj),
 						      i915_gem_object_get_stride(obj));
-		GEM_BUG_ON(vma->fence_size & 4095);
+		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_MIN_ALIGNMENT));
 
 		vma->fence_alignment = i915_gem_fence_alignment(vm->i915, vma->size,
 								i915_gem_object_get_tiling(obj),
@@ -270,7 +270,8 @@ i915_vma_misplaced(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	if (vma->node.size < size)
 		return true;
 
-	if (alignment && vma->node.start & (alignment - 1))
+	GEM_BUG_ON(alignment && !is_power_of_2(alignment));
+	if (alignment && !IS_ALIGNED(vma->node.start, alignment))
 		return true;
 
 	if (flags & PIN_MAPPABLE && !i915_vma_is_map_and_fenceable(vma))
@@ -302,7 +303,7 @@ void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
 		return;
 
 	fenceable = (vma->node.size >= vma->fence_size &&
-		     (vma->node.start & (vma->fence_alignment - 1)) == 0);
+		     IS_ALIGNED(vma->node.start, vma->fence_alignment));
 
 	mappable = vma->node.start + vma->fence_size <= i915_vm_to_ggtt(vma->vm)->mappable_end;
 
@@ -380,13 +381,19 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 				  alignment, vma->fence_alignment);
 	}
 
+	GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_PAGE_SIZE));
+	GEM_BUG_ON(!IS_ALIGNED(alignment, I915_GTT_MIN_ALIGNMENT));
+	GEM_BUG_ON(!is_power_of_2(alignment));
+
 	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
+	GEM_BUG_ON(!IS_ALIGNED(start, I915_GTT_PAGE_SIZE));
 
 	end = vma->vm->total;
 	if (flags & PIN_MAPPABLE)
 		end = min_t(u64, end, dev_priv->ggtt.mappable_end);
 	if (flags & PIN_ZONE_4G)
-		end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE);
+		end = min_t(u64, end, (1ULL << 32) - I915_GTT_PAGE_SIZE);
+	GEM_BUG_ON(!IS_ALIGNED(end, I915_GTT_PAGE_SIZE));
 
 	/* If binding the object/GGTT view requires more space than the entire
 	 * aperture has, reject it early before evicting everything in a vain
@@ -406,7 +413,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 
 	if (flags & PIN_OFFSET_FIXED) {
 		u64 offset = flags & PIN_OFFSET_MASK;
-		if (offset & (alignment - 1) ||
+		if (!IS_ALIGNED(offset, alignment) ||
 		    range_overflows(offset, size, end)) {
 			ret = -EINVAL;
 			goto err_unpin;
@@ -440,7 +447,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 		 * with zero alignment, so where possible use the optimal
 		 * path.
 		 */
-		if (alignment <= 4096)
+		if (alignment <= I915_GTT_MIN_ALIGNMENT)
 			alignment = 0;
 
 search_free:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6db246ad2f13..81665a9eb43f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1927,7 +1927,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
 	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
 
-	ret = intel_engine_create_scratch(engine, 4096);
+	ret = intel_engine_create_scratch(engine, PAGE_SIZE);
 	if (ret)
 		return ret;
 
@@ -2209,7 +2209,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 
 	WARN_ON(ce->state);
 
-	context_size = round_up(intel_lr_context_size(engine), 4096);
+	context_size = round_up(intel_lr_context_size(engine),
+				I915_GTT_PAGE_SIZE);
 
 	/* One extra page as the sharing data between driver and GuC */
 	context_size += PAGE_SIZE * LRC_PPHWSP_PN;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 01ba36ea125e..0c852c024227 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -26,7 +26,7 @@
 
 #include "intel_ringbuffer.h"
 
-#define GEN8_LR_CONTEXT_ALIGN 4096
+#define GEN8_LR_CONTEXT_ALIGN I915_GTT_MIN_ALIGNMENT
 
 /* Execlists regs */
 #define RING_ELSP(engine)			_MMIO((engine)->mmio_base + 0x230)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0971ac396b60..ab83fc22d207 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1736,7 +1736,7 @@ static int init_status_page(struct intel_engine_cs *engine)
 	void *vaddr;
 	int ret;
 
-	obj = i915_gem_object_create_internal(engine->i915, 4096);
+	obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
 	if (IS_ERR(obj)) {
 		DRM_ERROR("Failed to allocate status page\n");
 		return PTR_ERR(obj);
@@ -1777,7 +1777,7 @@ static int init_status_page(struct intel_engine_cs *engine)
 
 	engine->status_page.vma = vma;
 	engine->status_page.ggtt_offset = i915_ggtt_offset(vma);
-	engine->status_page.page_addr = memset(vaddr, 0, 4096);
+	engine->status_page.page_addr = memset(vaddr, 0, PAGE_SIZE);
 
 	DRM_DEBUG_DRIVER("%s hws offset: 0x%08x\n",
 			 engine->name, i915_ggtt_offset(vma));
@@ -2049,7 +2049,7 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
 	}
 
 	/* Ring wraparound at offset 0 sometimes hangs. No idea why. */
-	ret = intel_ring_pin(ring, 4096);
+	ret = intel_ring_pin(ring, I915_GTT_PAGE_SIZE);
 	if (ret) {
 		intel_ring_free(ring);
 		goto error;
@@ -2466,7 +2466,7 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
 	if (INTEL_GEN(dev_priv) >= 8 && !dev_priv->semaphore) {
 		struct i915_vma *vma;
 
-		obj = i915_gem_object_create(dev_priv, 4096);
+		obj = i915_gem_object_create(dev_priv, PAGE_SIZE);
 		if (IS_ERR(obj))
 			goto err;
 
@@ -2683,7 +2683,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 		return ret;
 
 	if (INTEL_GEN(dev_priv) >= 6) {
-		ret = intel_engine_create_scratch(engine, 4096);
+		ret = intel_engine_create_scratch(engine, PAGE_SIZE);
 		if (ret)
 			return ret;
 	} else if (HAS_BROKEN_CS_TLB(dev_priv)) {
-- 
2.11.0

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

Re: [PATCH v4] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Joonas Lahtinen]

On ti, 2017-01-10 at 14:47 +0000, Chris Wilson wrote:
> Start converting over from the byte count to its semantic macro, either
> we want to allocate the size of a physical page in main memory or we
> want the size of a virtual page in the GTT. 4096 could mean either, but
> PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> code comprehension and future changes. In the future, we may want to use
> variable GTT page sizes and so have the challenge of knowing which
> hardcoded values were used to represent a physical page vs the virtual
> page.
> 
> v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> v3: 4096ul paranoia, make fence alignment a distinct value of 4096, keep
> bdw stolen w/a as 4096 until we know better.
> v4: Add asserts that i915_vma_insert() start/end are aligned to GTT page
> sizes.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
> @@ -47,5 +47,7 @@ struct drm_i915_fence_reg {
> >  	bool dirty;
>  };
>  
> +#define I965_FENCE_PAGE 4096UL

I'm pretty sure I'd look for this from the very top of the file before
structs. Any specific reason not there?

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

On Tue, Jan 10, 2017 at 06:17:42PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-01-10 at 14:47 +0000, Chris Wilson wrote:
> > Start converting over from the byte count to its semantic macro, either
> > we want to allocate the size of a physical page in main memory or we
> > want the size of a virtual page in the GTT. 4096 could mean either, but
> > PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> > code comprehension and future changes. In the future, we may want to use
> > variable GTT page sizes and so have the challenge of knowing which
> > hardcoded values were used to represent a physical page vs the virtual
> > page.
> > 
> > v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> > v3: 4096ul paranoia, make fence alignment a distinct value of 4096, keep
> > bdw stolen w/a as 4096 until we know better.
> > v4: Add asserts that i915_vma_insert() start/end are aligned to GTT page
> > sizes.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
> > @@ -47,5 +47,7 @@ struct drm_i915_fence_reg {
> > >  	bool dirty;
> >  };
> >  
> > +#define I965_FENCE_PAGE 4096UL
> 
> I'm pretty sure I'd look for this from the very top of the file before
> structs. Any specific reason not there?

No good reason, mostly because that's where vim opened.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE (rev3)
URL   : https://patchwork.freedesktop.org/series/17761/
State : warning

== Summary ==

Series 17761v3 drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE
https://patchwork.freedesktop.org/api/1.0/series/17761/revisions/3/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:214  dwarn:1   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

44b1ad71b67ecfae6ba9b816c6f3a6ebe1fd182e drm-tip: 2017y-01m-10d-16h-32m-29s UTC integration manifest
a226fa5 drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3468/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

On Tue, Jan 10, 2017 at 06:17:42PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-01-10 at 14:47 +0000, Chris Wilson wrote:
> > Start converting over from the byte count to its semantic macro, either
> > we want to allocate the size of a physical page in main memory or we
> > want the size of a virtual page in the GTT. 4096 could mean either, but
> > PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> > code comprehension and future changes. In the future, we may want to use
> > variable GTT page sizes and so have the challenge of knowing which
> > hardcoded values were used to represent a physical page vs the virtual
> > page.
> > 
> > v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> > v3: 4096ul paranoia, make fence alignment a distinct value of 4096, keep
> > bdw stolen w/a as 4096 until we know better.
> > v4: Add asserts that i915_vma_insert() start/end are aligned to GTT page
> > sizes.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
> > @@ -47,5 +47,7 @@ struct drm_i915_fence_reg {
> > >  	bool dirty;
> >  };
> >  
> > +#define I965_FENCE_PAGE 4096UL
> 
> I'm pretty sure I'd look for this from the very top of the file before
> structs. Any specific reason not there?
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Bumped the macro and pushed. Even if this patch is not perfect, I think
it is a good step forward into sorting through the 4096.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx