diff mbox

[v4] drm/i915: Avoid writing relocs with addresses in non-canonical form

Message ID 1450468863-9268-1-git-send-email-michal.winiarski@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michał Winiarski Dec. 18, 2015, 8:01 p.m. UTC
According to bspec, some parts of HW require the addresses to be in
a canonical form, where bits [63:48] == [47]. Let's convert addresses to
canonical form prior to relocating and return converted offsets to
userspace. We also need to make sure that userspace is using addresses
in canonical form in case of softpin.

v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville)
v3: Rebase on top of softpin, fix a hole in relocate_entry,
    s/expect/require (Chris)
v4: Handle softpin in validate_exec_list (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 45 ++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

Comments

Chris Wilson Dec. 18, 2015, 8:26 p.m. UTC | #1
On Fri, Dec 18, 2015 at 09:01:03PM +0100, Micha? Winiarski wrote:
> According to bspec, some parts of HW require the addresses to be in
> a canonical form, where bits [63:48] == [47]. Let's convert addresses to
> canonical form prior to relocating and return converted offsets to
> userspace. We also need to make sure that userspace is using addresses
> in canonical form in case of softpin.
> 
> v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville)
> v3: Rebase on top of softpin, fix a hole in relocate_entry,
>     s/expect/require (Chris)
> v4: Handle softpin in validate_exec_list (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com>

> @@ -625,8 +645,9 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>  			entry->flags |= __EXEC_OBJECT_HAS_FENCE;
>  	}
>  
> -	if (entry->offset != vma->node.start) {
> -		entry->offset = vma->node.start;
> +	offset = gen8_canonical_addr(vma->node.start);
> +	if (entry->offset != offset) {

entry->offset is now in normal form.

> +		entry->offset = offset;
>  		*need_reloc = true;
>  	}

But we need to convert back to canonical form again on the userspace
boundary (copy_to_user).
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..87d9887 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,25 @@  static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* Used to convert any address to canonical form.
+ * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
+ * MI_LOAD_REGISTER_MEM and others, see Broadwell PRM Vol2a) require the
+ * addresses to be in a canonical form:
+ * "GraphicsAddress[63:48] are ignored by the HW and assumed to be in correct
+ * canonical form [63:48] == [47]."
+ */
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return ((int64_t)address << 16) >> 16;
+}
+
+static inline uint64_t
+relocation_target(struct drm_i915_gem_relocation_entry *reloc,
+		  uint64_t target_offset)
+{
+	return gen8_canonical_addr((int)reloc->delta + target_offset);
+}
+
 static int
 relocate_entry_cpu(struct drm_i915_gem_object *obj,
 		   struct drm_i915_gem_relocation_entry *reloc,
@@ -256,7 +275,7 @@  relocate_entry_cpu(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	uint32_t page_offset = offset_in_page(reloc->offset);
-	uint64_t delta = reloc->delta + target_offset;
+	uint64_t delta = relocation_target(reloc, target_offset);
 	char *vaddr;
 	int ret;
 
@@ -292,7 +311,7 @@  relocate_entry_gtt(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint64_t delta = reloc->delta + target_offset;
+	uint64_t delta = relocation_target(reloc, target_offset);
 	uint64_t offset;
 	void __iomem *reloc_page;
 	int ret;
@@ -347,7 +366,7 @@  relocate_entry_clflush(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	uint32_t page_offset = offset_in_page(reloc->offset);
-	uint64_t delta = (int)reloc->delta + target_offset;
+	uint64_t delta = relocation_target(reloc, target_offset);
 	char *vaddr;
 	int ret;
 
@@ -395,7 +414,7 @@  i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	target_i915_obj = target_vma->obj;
 	target_obj = &target_vma->obj->base;
 
-	target_offset = target_vma->node.start;
+	target_offset = gen8_canonical_addr(target_vma->node.start);
 
 	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
 	 * pipe_control writes because the gpu doesn't properly redirect them
@@ -583,6 +602,7 @@  i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	struct drm_i915_gem_object *obj = vma->obj;
 	struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
 	uint64_t flags;
+	uint64_t offset;
 	int ret;
 
 	flags = PIN_USER;
@@ -625,8 +645,9 @@  i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 			entry->flags |= __EXEC_OBJECT_HAS_FENCE;
 	}
 
-	if (entry->offset != vma->node.start) {
-		entry->offset = vma->node.start;
+	offset = gen8_canonical_addr(vma->node.start);
+	if (entry->offset != offset) {
+		entry->offset = offset;
 		*need_reloc = true;
 	}
 
@@ -994,6 +1015,18 @@  validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), since
+		 * userspace has to use canonical format, we need to reject all
+		 * non-canonical addresses.
+		 */
+		if (exec[i].offset != gen8_canonical_addr(exec[i].offset))
+			return -EINVAL;
+
+		/* On the other hand, from drm_mm perspective address space is
+		 * continuous, so we're converting to non-canonical form
+		 */
+		exec[i].offset &= (1ULL << 48) - 1;
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;