[PATCH] drm/i915: Support 64b relocations

[PATCH] drm/i915: Support 64b relocations

[Ben Widawsky]

All the rest of the code to enable this is in my branch. Without my
branch, hitting > 32b offsets is impossible. The code has always
"supported" 64b, but it's never actually been run of tested. This change
doesn't actually fix anything. [1] I am not sure why X won't work yet. I
do not get hangs or obvious errors.

There are 3 fixes grouped together here. First is to remove the
hardcoded 0 for the upper dword of the relocation. The next fix is to
use a 64b value for target_offset. The final fix is to not directly
apply target_offset to reloc->delta. reloc->delta is part of ABI, and so
we cannot change it. As it stands, 32b is enough to represent everything
we're interested in representing anyway. The main problem is, we cannot
add greater than 32b values to it directly.

[1] Almost all of intel-gpu-tools is not yet ready to test 64b
relocations. There are a few places that expect 32b values for offsets
and these all won't work.

Cc: Rafael Barbalho <rafael.barbalho@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0d806fc..6ffecd2 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -262,10 +262,12 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 
 static int
 relocate_entry_cpu(struct drm_i915_gem_object *obj,
-		   struct drm_i915_gem_relocation_entry *reloc)
+		   struct drm_i915_gem_relocation_entry *reloc,
+		   uint64_t target_offset)
 {
 	struct drm_device *dev = obj->base.dev;
 	uint32_t page_offset = offset_in_page(reloc->offset);
+	uint64_t delta = reloc->delta + target_offset;
 	char *vaddr;
 	int ret;
 
@@ -275,7 +277,7 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj,
 
 	vaddr = kmap_atomic(i915_gem_object_get_page(obj,
 				reloc->offset >> PAGE_SHIFT));
-	*(uint32_t *)(vaddr + page_offset) = reloc->delta;
+	*(uint32_t *)(vaddr + page_offset) = lower_32_bits(delta);
 
 	if (INTEL_INFO(dev)->gen >= 8) {
 		page_offset = offset_in_page(page_offset + sizeof(uint32_t));
@@ -286,7 +288,7 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj,
 			    (reloc->offset + sizeof(uint32_t)) >> PAGE_SHIFT));
 		}
 
-		*(uint32_t *)(vaddr + page_offset) = 0;
+		*(uint32_t *)(vaddr + page_offset) = upper_32_bits(delta);
 	}
 
 	kunmap_atomic(vaddr);
@@ -296,10 +298,12 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj,
 
 static int
 relocate_entry_gtt(struct drm_i915_gem_object *obj,
-		   struct drm_i915_gem_relocation_entry *reloc)
+		   struct drm_i915_gem_relocation_entry *reloc,
+		   uint64_t target_offset)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint64_t delta = reloc->delta + target_offset;
 	uint32_t __iomem *reloc_entry;
 	void __iomem *reloc_page;
 	int ret;
@@ -318,7 +322,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
 			reloc->offset & PAGE_MASK);
 	reloc_entry = (uint32_t __iomem *)
 		(reloc_page + offset_in_page(reloc->offset));
-	iowrite32(reloc->delta, reloc_entry);
+	iowrite32(lower_32_bits(delta), reloc_entry);
 
 	if (INTEL_INFO(dev)->gen >= 8) {
 		reloc_entry += 1;
@@ -331,7 +335,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
 			reloc_entry = reloc_page;
 		}
 
-		iowrite32(0, reloc_entry);
+		iowrite32(upper_32_bits(delta), reloc_entry);
 	}
 
 	io_mapping_unmap_atomic(reloc_page);
@@ -348,7 +352,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	struct drm_gem_object *target_obj;
 	struct drm_i915_gem_object *target_i915_obj;
 	struct i915_vma *target_vma;
-	uint32_t target_offset;
+	uint64_t target_offset;
 	int ret;
 
 	/* we've already hold a reference to all valid objects */
@@ -427,11 +431,10 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	if (obj->active && in_atomic())
 		return -EFAULT;
 
-	reloc->delta += target_offset;
 	if (use_cpu_reloc(obj))
-		ret = relocate_entry_cpu(obj, reloc);
+		ret = relocate_entry_cpu(obj, reloc, target_offset);
 	else
-		ret = relocate_entry_gtt(obj, reloc);
+		ret = relocate_entry_gtt(obj, reloc, target_offset);
 
 	if (ret)
 		return ret;
-- 
1.9.2

[PATCH] drm/i915: Expand error state's address width to 64b

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  4 ++--
 drivers/gpu/drm/i915/i915_gpu_error.c | 16 +++++++++-------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 539f16db..cdde849 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -366,7 +366,7 @@ struct drm_i915_error_state {
 
 		struct drm_i915_error_object {
 			int page_count;
-			u32 gtt_offset;
+			u64 gtt_offset;
 			u32 *pages[0];
 		} *ringbuffer, *batchbuffer, *wa_batchbuffer, *ctx, *hws_page;
 
@@ -391,7 +391,7 @@ struct drm_i915_error_state {
 		u32 size;
 		u32 name;
 		u32 rseqno, wseqno;
-		u32 gtt_offset;
+		u64 gtt_offset;
 		u32 read_domains;
 		u32 write_domain;
 		s32 fence_reg:I915_MAX_NUM_FENCE_BITS;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 481a7d1..a5cd3b0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -194,7 +194,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
 	err_printf(m, "%s [%d]:\n", name, count);
 
 	while (count--) {
-		err_printf(m, "  %08x %8u %02x %02x %x %x",
+		err_printf(m, "  %16llx %8u %02x %02x %x %x",
 			   err->gtt_offset,
 			   err->size,
 			   err->read_domains,
@@ -401,7 +401,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 				err_printf(m, " (submitted by %s [%d])",
 					   error->ring[i].comm,
 					   error->ring[i].pid);
-			err_printf(m, " --- gtt_offset = 0x%08x\n",
+			err_printf(m, " --- gtt_offset = 0x%16llx\n",
 				   obj->gtt_offset);
 			print_error_obj(m, obj);
 		}
@@ -409,7 +409,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		obj = error->ring[i].wa_batchbuffer;
 		if (obj) {
 			err_printf(m, "%s (w/a) --- gtt_offset = 0x%08x\n",
-				   dev_priv->ring[i].name, obj->gtt_offset);
+				   dev_priv->ring[i].name,
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 
@@ -428,14 +429,14 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		if ((obj = error->ring[i].ringbuffer)) {
 			err_printf(m, "%s --- ringbuffer = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 
 		if ((obj = error->ring[i].hws_page)) {
 			err_printf(m, "%s --- HW Status = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			offset = 0;
 			for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
 				err_printf(m, "[%04x] %08x %08x %08x %08x\n",
@@ -451,14 +452,15 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		if ((obj = error->ring[i].ctx)) {
 			err_printf(m, "%s --- HW Context = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 	}
 
 	obj = error->semaphore_obj;
 	if (obj) {
-		err_printf(m, "Semaphore page = 0x%08x\n", obj->gtt_offset);
+		err_printf(m, "Semaphore page = 0x%08x\n",
+			   lower_32_bits(obj->gtt_offset));
 		for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
 			err_printf(m, "[%04x] %08x %08x %08x %08x\n",
 				   elt * 4,
-- 
1.9.2

[PATCH] [v2] drm/i915: Expand error state's address width to 64b

[Ben Widawsky]

v2: 0 pad the new 8B fields or else intel_error_decode has a hard time.
Note, regardless we need an igt update.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  4 ++--
 drivers/gpu/drm/i915/i915_gpu_error.c | 16 +++++++++-------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 539f16db..cdde849 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -366,7 +366,7 @@ struct drm_i915_error_state {
 
 		struct drm_i915_error_object {
 			int page_count;
-			u32 gtt_offset;
+			u64 gtt_offset;
 			u32 *pages[0];
 		} *ringbuffer, *batchbuffer, *wa_batchbuffer, *ctx, *hws_page;
 
@@ -391,7 +391,7 @@ struct drm_i915_error_state {
 		u32 size;
 		u32 name;
 		u32 rseqno, wseqno;
-		u32 gtt_offset;
+		u64 gtt_offset;
 		u32 read_domains;
 		u32 write_domain;
 		s32 fence_reg:I915_MAX_NUM_FENCE_BITS;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 481a7d1..881ad8f 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -194,7 +194,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
 	err_printf(m, "%s [%d]:\n", name, count);
 
 	while (count--) {
-		err_printf(m, "  %08x %8u %02x %02x %x %x",
+		err_printf(m, "  %016llx %8u %02x %02x %x %x",
 			   err->gtt_offset,
 			   err->size,
 			   err->read_domains,
@@ -401,7 +401,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 				err_printf(m, " (submitted by %s [%d])",
 					   error->ring[i].comm,
 					   error->ring[i].pid);
-			err_printf(m, " --- gtt_offset = 0x%08x\n",
+			err_printf(m, " --- gtt_offset = 0x%016llx\n",
 				   obj->gtt_offset);
 			print_error_obj(m, obj);
 		}
@@ -409,7 +409,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		obj = error->ring[i].wa_batchbuffer;
 		if (obj) {
 			err_printf(m, "%s (w/a) --- gtt_offset = 0x%08x\n",
-				   dev_priv->ring[i].name, obj->gtt_offset);
+				   dev_priv->ring[i].name,
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 
@@ -428,14 +429,14 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		if ((obj = error->ring[i].ringbuffer)) {
 			err_printf(m, "%s --- ringbuffer = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 
 		if ((obj = error->ring[i].hws_page)) {
 			err_printf(m, "%s --- HW Status = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			offset = 0;
 			for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
 				err_printf(m, "[%04x] %08x %08x %08x %08x\n",
@@ -451,14 +452,15 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		if ((obj = error->ring[i].ctx)) {
 			err_printf(m, "%s --- HW Context = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 	}
 
 	obj = error->semaphore_obj;
 	if (obj) {
-		err_printf(m, "Semaphore page = 0x%08x\n", obj->gtt_offset);
+		err_printf(m, "Semaphore page = 0x%08x\n",
+			   lower_32_bits(obj->gtt_offset));
 		for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
 			err_printf(m, "[%04x] %08x %08x %08x %08x\n",
 				   elt * 4,
-- 
1.9.2

[PATCH] intel_error_decode: use 64b gtt_offset

[Ben Widawsky]

See the relevant kernel patch for the details. I guess this breaks
support for older error state, I am not actually sure. Without
versioning our error state though, I cannot think of a better way.
Suggestions are welcome.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tools/intel_error_decode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/intel_error_decode.c b/tools/intel_error_decode.c
index 1eeff07..d0028a1 100644
--- a/tools/intel_error_decode.c
+++ b/tools/intel_error_decode.c
@@ -311,17 +311,17 @@ print_fence(unsigned int devid, uint64_t fence)
 uint32_t head[MAX_RINGS];
 int head_ndx = 0;
 int num_rings = 0;
-static void print_batch(int is_batch, const char *ring_name, uint32_t gtt_offset)
+static void print_batch(int is_batch, const char *ring_name, uint64_t gtt_offset)
 {
 	const char *buffer_type[2] = {  "ringbuffer", "batchbuffer" };
 	if (is_batch || !num_rings)
-		printf("%s (%s) at 0x%08x\n", buffer_type[is_batch], ring_name, gtt_offset);
+		printf("%s (%s) at 0x%016lx\n", buffer_type[is_batch], ring_name, gtt_offset);
 	else
-		printf("%s (%s) at 0x%08x; HEAD points to: 0x%08x\n", buffer_type[is_batch], ring_name, gtt_offset, head[head_ndx++ % num_rings] + gtt_offset);
+		printf("%s (%s) at 0x%016lx; HEAD points to: 0x%016lx\n", buffer_type[is_batch], ring_name, gtt_offset, head[head_ndx++ % num_rings] + gtt_offset);
 }
 
 static void decode(struct drm_intel_decode *ctx, bool is_batch,
-		   const char *ring_name, uint32_t gtt_offset, uint32_t *data,
+		   const char *ring_name, uint64_t gtt_offset, uint32_t *data,
 		   int *count)
 {
 	if (!*count)
@@ -344,7 +344,7 @@ read_data_file(FILE *file)
 	char *line = NULL;
 	size_t line_size;
 	uint32_t offset, value, ring_length = 0;
-	uint32_t gtt_offset = 0, new_gtt_offset;
+	uint64_t gtt_offset = 0, new_gtt_offset;
 	char *ring_name = NULL;
 	int is_batch = 1;
 
@@ -361,7 +361,7 @@ read_data_file(FILE *file)
 			if (num_rings == -1)
 				num_rings = head_ndx;
 
-			matched = sscanf(dashes, "--- gtt_offset = 0x%08x\n",
+			matched = sscanf(dashes, "--- gtt_offset = 0x%016lx\n",
 					&new_gtt_offset);
 			if (matched == 1) {
 				decode(decode_ctx, is_batch, ring_name,
@@ -373,7 +373,7 @@ read_data_file(FILE *file)
 				continue;
 			}
 
-			matched = sscanf(dashes, "--- ringbuffer = 0x%08x\n",
+			matched = sscanf(dashes, "--- ringbuffer = 0x%08lx\n",
 					&new_gtt_offset);
 			if (matched == 1) {
 				decode(decode_ctx, is_batch, ring_name,
-- 
1.9.2

Re: [PATCH] intel_error_decode: use 64b gtt_offset

[Daniel Vetter]

On Mon, Apr 28, 2014 at 06:45:50PM -0700, Ben Widawsky wrote:
> See the relevant kernel patch for the details. I guess this breaks
> support for older error state, I am not actually sure. Without
> versioning our error state though, I cannot think of a better way.
> Suggestions are welcome.

Just drop the length qualifier and let scanf it the full number?
-Daniel

> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  tools/intel_error_decode.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/intel_error_decode.c b/tools/intel_error_decode.c
> index 1eeff07..d0028a1 100644
> --- a/tools/intel_error_decode.c
> +++ b/tools/intel_error_decode.c
> @@ -311,17 +311,17 @@ print_fence(unsigned int devid, uint64_t fence)
>  uint32_t head[MAX_RINGS];
>  int head_ndx = 0;
>  int num_rings = 0;
> -static void print_batch(int is_batch, const char *ring_name, uint32_t gtt_offset)
> +static void print_batch(int is_batch, const char *ring_name, uint64_t gtt_offset)
>  {
>  	const char *buffer_type[2] = {  "ringbuffer", "batchbuffer" };
>  	if (is_batch || !num_rings)
> -		printf("%s (%s) at 0x%08x\n", buffer_type[is_batch], ring_name, gtt_offset);
> +		printf("%s (%s) at 0x%016lx\n", buffer_type[is_batch], ring_name, gtt_offset);
>  	else
> -		printf("%s (%s) at 0x%08x; HEAD points to: 0x%08x\n", buffer_type[is_batch], ring_name, gtt_offset, head[head_ndx++ % num_rings] + gtt_offset);
> +		printf("%s (%s) at 0x%016lx; HEAD points to: 0x%016lx\n", buffer_type[is_batch], ring_name, gtt_offset, head[head_ndx++ % num_rings] + gtt_offset);
>  }
>  
>  static void decode(struct drm_intel_decode *ctx, bool is_batch,
> -		   const char *ring_name, uint32_t gtt_offset, uint32_t *data,
> +		   const char *ring_name, uint64_t gtt_offset, uint32_t *data,
>  		   int *count)
>  {
>  	if (!*count)
> @@ -344,7 +344,7 @@ read_data_file(FILE *file)
>  	char *line = NULL;
>  	size_t line_size;
>  	uint32_t offset, value, ring_length = 0;
> -	uint32_t gtt_offset = 0, new_gtt_offset;
> +	uint64_t gtt_offset = 0, new_gtt_offset;
>  	char *ring_name = NULL;
>  	int is_batch = 1;
>  
> @@ -361,7 +361,7 @@ read_data_file(FILE *file)
>  			if (num_rings == -1)
>  				num_rings = head_ndx;
>  
> -			matched = sscanf(dashes, "--- gtt_offset = 0x%08x\n",
> +			matched = sscanf(dashes, "--- gtt_offset = 0x%016lx\n",
>  					&new_gtt_offset);
>  			if (matched == 1) {
>  				decode(decode_ctx, is_batch, ring_name,
> @@ -373,7 +373,7 @@ read_data_file(FILE *file)
>  				continue;
>  			}
>  
> -			matched = sscanf(dashes, "--- ringbuffer = 0x%08x\n",
> +			matched = sscanf(dashes, "--- ringbuffer = 0x%08lx\n",
>  					&new_gtt_offset);
>  			if (matched == 1) {
>  				decode(decode_ctx, is_batch, ring_name,
> -- 
> 1.9.2
> 
> _______________________________________________
> 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] intel_error_decode: use 64b gtt_offset

[Daniel Vetter]

On Tue, Apr 29, 2014 at 10:52:44AM +0200, Daniel Vetter wrote:
> On Mon, Apr 28, 2014 at 06:45:50PM -0700, Ben Widawsky wrote:
> > See the relevant kernel patch for the details. I guess this breaks
> > support for older error state, I am not actually sure. Without
> > versioning our error state though, I cannot think of a better way.
> > Suggestions are welcome.
> 
> Just drop the length qualifier and let scanf it the full number?

Also, you know the drill: Testcase, please. A copy of drv_hangman to also
feed the captured error state into intel_error_decode and check that it
doesn't fall overr (exitcode != 0 and nothing on stderr). Maybe call it
drv_error_decode or something like that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] intel_error_decode: use 64b gtt_offset

[Chris Wilson]

On Tue, Apr 29, 2014 at 11:01:10AM +0200, Daniel Vetter wrote:
> On Tue, Apr 29, 2014 at 10:52:44AM +0200, Daniel Vetter wrote:
> > On Mon, Apr 28, 2014 at 06:45:50PM -0700, Ben Widawsky wrote:
> > > See the relevant kernel patch for the details. I guess this breaks
> > > support for older error state, I am not actually sure. Without
> > > versioning our error state though, I cannot think of a better way.
> > > Suggestions are welcome.
> > 
> > Just drop the length qualifier and let scanf it the full number?
> 
> Also, you know the drill: Testcase, please. A copy of drv_hangman to also
> feed the captured error state into intel_error_decode and check that it
> doesn't fall overr (exitcode != 0 and nothing on stderr). Maybe call it
> drv_error_decode or something like that.

Actually, I would have hoped you asked for uniformity in presenting and
parsing 64bit values :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] intel_error_decode: use 64b gtt_offset

[Daniel Vetter]

On Tue, Apr 29, 2014 at 12:48 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Apr 29, 2014 at 11:01:10AM +0200, Daniel Vetter wrote:
>> On Tue, Apr 29, 2014 at 10:52:44AM +0200, Daniel Vetter wrote:
>> > On Mon, Apr 28, 2014 at 06:45:50PM -0700, Ben Widawsky wrote:
>> > > See the relevant kernel patch for the details. I guess this breaks
>> > > support for older error state, I am not actually sure. Without
>> > > versioning our error state though, I cannot think of a better way.
>> > > Suggestions are welcome.
>> >
>> > Just drop the length qualifier and let scanf it the full number?
>>
>> Also, you know the drill: Testcase, please. A copy of drv_hangman to also
>> feed the captured error state into intel_error_decode and check that it
>> doesn't fall overr (exitcode != 0 and nothing on stderr). Maybe call it
>> drv_error_decode or something like that.
>
> Actually, I would have hoped you asked for uniformity in presenting and
> parsing 64bit values :)

That approaches a turing test, so a bit out of scope for igt ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] intel_error_decode: use 64b gtt_offset

[Ben Widawsky]

On Tue, Apr 29, 2014 at 11:01:10AM +0200, Daniel Vetter wrote:
> On Tue, Apr 29, 2014 at 10:52:44AM +0200, Daniel Vetter wrote:
> > On Mon, Apr 28, 2014 at 06:45:50PM -0700, Ben Widawsky wrote:
> > > See the relevant kernel patch for the details. I guess this breaks
> > > support for older error state, I am not actually sure. Without
> > > versioning our error state though, I cannot think of a better way.
> > > Suggestions are welcome.
> > 
> > Just drop the length qualifier and let scanf it the full number?
> 
> Also, you know the drill: Testcase, please. A copy of drv_hangman to also
> feed the captured error state into intel_error_decode and check that it
> doesn't fall overr (exitcode != 0 and nothing on stderr). Maybe call it
> drv_error_decode or something like that.
> -Daniel

You're not going to get it from me. If you don't take the patch, that's
fine by me.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] [v2] drm/i915: Expand error state's address width to 64b

[Chris Wilson]

On Mon, Apr 28, 2014 at 06:43:04PM -0700, Ben Widawsky wrote:
> v2: 0 pad the new 8B fields or else intel_error_decode has a hard time.
> Note, regardless we need an igt update.

We should decide whether to use 0000000100000000 or 00000001 00000000
and be consistent.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] [v3] drm/i915: Expand error state's address width to 64b

[Ben Widawsky]

v2: 0 pad the new 8B fields or else intel_error_decode has a hard time.
Note, regardless we need an igt update.

v3: Make reloc_offset 64b also.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  4 ++--
 drivers/gpu/drm/i915/i915_gpu_error.c | 18 ++++++++++--------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 539f16db..cdde849 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -366,7 +366,7 @@ struct drm_i915_error_state {
 
 		struct drm_i915_error_object {
 			int page_count;
-			u32 gtt_offset;
+			u64 gtt_offset;
 			u32 *pages[0];
 		} *ringbuffer, *batchbuffer, *wa_batchbuffer, *ctx, *hws_page;
 
@@ -391,7 +391,7 @@ struct drm_i915_error_state {
 		u32 size;
 		u32 name;
 		u32 rseqno, wseqno;
-		u32 gtt_offset;
+		u64 gtt_offset;
 		u32 read_domains;
 		u32 write_domain;
 		s32 fence_reg:I915_MAX_NUM_FENCE_BITS;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 481a7d1..1e6ac12 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -194,7 +194,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
 	err_printf(m, "%s [%d]:\n", name, count);
 
 	while (count--) {
-		err_printf(m, "  %08x %8u %02x %02x %x %x",
+		err_printf(m, "  %016llx %8u %02x %02x %x %x",
 			   err->gtt_offset,
 			   err->size,
 			   err->read_domains,
@@ -401,7 +401,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 				err_printf(m, " (submitted by %s [%d])",
 					   error->ring[i].comm,
 					   error->ring[i].pid);
-			err_printf(m, " --- gtt_offset = 0x%08x\n",
+			err_printf(m, " --- gtt_offset = 0x%016llx\n",
 				   obj->gtt_offset);
 			print_error_obj(m, obj);
 		}
@@ -409,7 +409,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		obj = error->ring[i].wa_batchbuffer;
 		if (obj) {
 			err_printf(m, "%s (w/a) --- gtt_offset = 0x%08x\n",
-				   dev_priv->ring[i].name, obj->gtt_offset);
+				   dev_priv->ring[i].name,
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 
@@ -428,14 +429,14 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		if ((obj = error->ring[i].ringbuffer)) {
 			err_printf(m, "%s --- ringbuffer = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 
 		if ((obj = error->ring[i].hws_page)) {
 			err_printf(m, "%s --- HW Status = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			offset = 0;
 			for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
 				err_printf(m, "[%04x] %08x %08x %08x %08x\n",
@@ -451,14 +452,15 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		if ((obj = error->ring[i].ctx)) {
 			err_printf(m, "%s --- HW Context = 0x%08x\n",
 				   dev_priv->ring[i].name,
-				   obj->gtt_offset);
+				   lower_32_bits(obj->gtt_offset));
 			print_error_obj(m, obj);
 		}
 	}
 
 	obj = error->semaphore_obj;
 	if (obj) {
-		err_printf(m, "Semaphore page = 0x%08x\n", obj->gtt_offset);
+		err_printf(m, "Semaphore page = 0x%08x\n",
+			   lower_32_bits(obj->gtt_offset));
 		for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
 			err_printf(m, "[%04x] %08x %08x %08x %08x\n",
 				   elt * 4,
@@ -554,7 +556,7 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
 {
 	struct drm_i915_error_object *dst;
 	int i;
-	u32 reloc_offset;
+	u64 reloc_offset;
 
 	if (src == NULL || src->pages == NULL)
 		return NULL;
-- 
1.9.2

[PATCH] drm/i915: Support 64b execbuf

[Ben Widawsky]

Previously, our code only had a 32b offset value for where the
batchbuffer starts. With full PPGTT, and 64b canonical GPU address
space, that is an insufficient value. The code to expand is pretty
straight forward, and only one platform needs to do anything with the
extra bits.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6ffecd2..f5f0b92 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1017,7 +1017,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct i915_hw_context *ctx;
 	struct i915_address_space *vm;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
-	u32 exec_start = args->batch_start_offset, exec_len;
+	u64 exec_start = args->batch_start_offset, exec_len;
 	u32 mask, flags;
 	int ret, mode, i;
 	bool need_relocs;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a42942f..bbe989f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1324,7 +1324,7 @@ gen8_ring_put_irq(struct intel_ring_buffer *ring)
 
 static int
 i965_dispatch_execbuffer(struct intel_ring_buffer *ring,
-			 u32 offset, u32 length,
+			 u64 offset, u32 length,
 			 unsigned flags)
 {
 	int ret;
@@ -1347,7 +1347,7 @@ i965_dispatch_execbuffer(struct intel_ring_buffer *ring,
 #define I830_BATCH_LIMIT (256*1024)
 static int
 i830_dispatch_execbuffer(struct intel_ring_buffer *ring,
-				u32 offset, u32 len,
+				u64 offset, u32 len,
 				unsigned flags)
 {
 	int ret;
@@ -1398,7 +1398,7 @@ i830_dispatch_execbuffer(struct intel_ring_buffer *ring,
 
 static int
 i915_dispatch_execbuffer(struct intel_ring_buffer *ring,
-			 u32 offset, u32 len,
+			 u64 offset, u32 len,
 			 unsigned flags)
 {
 	int ret;
@@ -1943,7 +1943,7 @@ static int gen6_bsd_ring_flush(struct intel_ring_buffer *ring,
 
 static int
 gen8_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
-			      u32 offset, u32 len,
+			      u64 offset, u32 len,
 			      unsigned flags)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
@@ -1957,8 +1957,8 @@ gen8_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 
 	/* FIXME(BDW): Address space and security selectors. */
 	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
-	intel_ring_emit(ring, offset);
-	intel_ring_emit(ring, 0);
+	intel_ring_emit(ring, lower_32_bits(offset));
+	intel_ring_emit(ring, upper_32_bits(offset));
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 
@@ -1967,7 +1967,7 @@ gen8_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 
 static int
 hsw_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
-			      u32 offset, u32 len,
+			      u64 offset, u32 len,
 			      unsigned flags)
 {
 	int ret;
@@ -1988,7 +1988,7 @@ hsw_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 
 static int
 gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
-			      u32 offset, u32 len,
+			      u64 offset, u32 len,
 			      unsigned flags)
 {
 	int ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index dbdce5f..cb55cff 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -135,7 +135,7 @@ struct  intel_ring_buffer {
 	void		(*set_seqno)(struct intel_ring_buffer *ring,
 				     u32 seqno);
 	int		(*dispatch_execbuffer)(struct intel_ring_buffer *ring,
-					       u32 offset, u32 length,
+					       u64 offset, u32 length,
 					       unsigned flags);
 #define I915_DISPATCH_SECURE 0x1
 #define I915_DISPATCH_PINNED 0x2
-- 
1.9.2

Re: [PATCH] drm/i915: Support 64b execbuf

[Chris Wilson]

On Mon, Apr 28, 2014 at 07:29:25PM -0700, Ben Widawsky wrote:
> Previously, our code only had a 32b offset value for where the
> batchbuffer starts. With full PPGTT, and 64b canonical GPU address
> space, that is an insufficient value. The code to expand is pretty
> straight forward, and only one platform needs to do anything with the
> extra bits.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Support 64b execbuf

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Thursday, May 01, 2014 9:13 AM
> To: Widawsky, Benjamin
> Cc: Intel GFX
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Support 64b execbuf
> 
> On Mon, Apr 28, 2014 at 07:29:25PM -0700, Ben Widawsky wrote:
> > Previously, our code only had a 32b offset value for where the
> > batchbuffer starts. With full PPGTT, and 64b canonical GPU address
> > space, that is an insufficient value. The code to expand is pretty
> > straight forward, and only one platform needs to do anything with the
> > extra bits.
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>

Thanks,
Rafael

Re: [PATCH] drm/i915: Support 64b execbuf

[Daniel Vetter]

On Thu, May 01, 2014 at 10:18:52AM +0000, Barbalho, Rafael wrote:
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Chris Wilson
> > Sent: Thursday, May 01, 2014 9:13 AM
> > To: Widawsky, Benjamin
> > Cc: Intel GFX
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Support 64b execbuf
> > 
> > On Mon, Apr 28, 2014 at 07:29:25PM -0700, Ben Widawsky wrote:
> > > Previously, our code only had a 32b offset value for where the
> > > batchbuffer starts. With full PPGTT, and 64b canonical GPU address
> > > space, that is an insufficient value. The code to expand is pretty
> > > straight forward, and only one platform needs to do anything with the
> > > extra bits.
> > >
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
> Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>

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

Re: [PATCH] drm/i915: Support 64b relocations

[Chris Wilson]

On Mon, Apr 28, 2014 at 05:18:28PM -0700, Ben Widawsky wrote:
> All the rest of the code to enable this is in my branch. Without my
> branch, hitting > 32b offsets is impossible. The code has always
> "supported" 64b, but it's never actually been run of tested. This change
> doesn't actually fix anything. [1] I am not sure why X won't work yet. I
> do not get hangs or obvious errors.
> 
> There are 3 fixes grouped together here. First is to remove the
> hardcoded 0 for the upper dword of the relocation. The next fix is to
> use a 64b value for target_offset. The final fix is to not directly
> apply target_offset to reloc->delta. reloc->delta is part of ABI, and so
> we cannot change it. As it stands, 32b is enough to represent everything
> we're interested in representing anyway. The main problem is, we cannot
> add greater than 32b values to it directly.
> 
> [1] Almost all of intel-gpu-tools is not yet ready to test 64b
> relocations. There are a few places that expect 32b values for offsets
> and these all won't work.
> 
> Cc: Rafael Barbalho <rafael.barbalho@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Seriously, we did this? I am ashamed. I was annoyed by the original
assertion that no userspace was ready in the first place, and to see
that the code was a complete farce anyway...

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0d806fc..6ffecd2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -262,10 +262,12 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
>  
>  static int
>  relocate_entry_cpu(struct drm_i915_gem_object *obj,
> -		   struct drm_i915_gem_relocation_entry *reloc)
> +		   struct drm_i915_gem_relocation_entry *reloc,
> +		   uint64_t target_offset)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	uint32_t page_offset = offset_in_page(reloc->offset);
> +	uint64_t delta = reloc->delta + target_offset;

I would not have called the final value delta, but target_offset.
I was going to quible over the use of a local variable instead of
reloc->delta, but you successfully argued in my head that your way was
less obtuse.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Support 64b relocations

[Daniel Vetter]

On Thu, May 01, 2014 at 09:04:50AM +0100, Chris Wilson wrote:
> On Mon, Apr 28, 2014 at 05:18:28PM -0700, Ben Widawsky wrote:
> > All the rest of the code to enable this is in my branch. Without my
> > branch, hitting > 32b offsets is impossible. The code has always
> > "supported" 64b, but it's never actually been run of tested. This change
> > doesn't actually fix anything. [1] I am not sure why X won't work yet. I
> > do not get hangs or obvious errors.
> > 
> > There are 3 fixes grouped together here. First is to remove the
> > hardcoded 0 for the upper dword of the relocation. The next fix is to
> > use a 64b value for target_offset. The final fix is to not directly
> > apply target_offset to reloc->delta. reloc->delta is part of ABI, and so
> > we cannot change it. As it stands, 32b is enough to represent everything
> > we're interested in representing anyway. The main problem is, we cannot
> > add greater than 32b values to it directly.

Imo if you have a target_offset > 32b in a valid use-case we can bother to
look at this. But not before, since I expect that hw advances will make
this obsolete anyway.

> > [1] Almost all of intel-gpu-tools is not yet ready to test 64b
> > relocations. There are a few places that expect 32b values for offsets
> > and these all won't work.
> > 
> > Cc: Rafael Barbalho <rafael.barbalho@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Seriously, we did this? I am ashamed. I was annoyed by the original
> assertion that no userspace was ready in the first place, and to see
> that the code was a complete farce anyway...

Well my idea was that we try to prep userspace to avoid a needless abi
rev, but it was always clear to me that the kernel side (and igt) is
hopelessly broken for 64b relocs.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

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