[PATCH] drm/i915: Report the actual swizzling back to userspace

[PATCH] drm/i915: Report the actual swizzling back to userspace

[Chris Wilson]

Userspace cares about whether or not swizzling depends on the page
address for its direct access into bound objects. Extend the get_tiling
ioctl to report the physical swizzling value in addition to the logical
swizzling value so that userspace can accurately determine when it is
possible for manual detiling.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Akash Goel <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_tiling.c | 1 +
 include/uapi/drm/i915_drm.h            | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 3c073fb58730..b53ec941f17e 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -446,6 +446,7 @@ i915_gem_get_tiling(struct drm_device *dev, void *data,
 	}
 
 	/* Hide bit 17 from the user -- see comment in i915_gem_set_tiling */
+	args->phys_swizzle_mode = args->swizzle_mode;
 	if (args->swizzle_mode == I915_BIT_6_SWIZZLE_9_17)
 		args->swizzle_mode = I915_BIT_6_SWIZZLE_9;
 	if (args->swizzle_mode == I915_BIT_6_SWIZZLE_9_10_17)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7615ae68e015..5ff86692e03e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -934,6 +934,12 @@ struct drm_i915_gem_get_tiling {
 	 * mmap mapping.
 	 */
 	__u32 swizzle_mode;
+
+	/**
+	 * Returned address bit 6 swizzling required for CPU access through
+	 * mmap mapping whilst bound.
+	 */
+	__u32 phys_swizzle_mode;
 };
 
 struct drm_i915_gem_get_aperture {
-- 
2.1.1

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Daniel Vetter]

On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> Userspace cares about whether or not swizzling depends on the page
> address for its direct access into bound objects. Extend the get_tiling
> ioctl to report the physical swizzling value in addition to the logical
> swizzling value so that userspace can accurately determine when it is
> possible for manual detiling.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

I wonder whether we should care really, since on gen5+ we know that they
agree. Pimp some igt (by skipping if needed) to convince me?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_tiling.c | 1 +
>  include/uapi/drm/i915_drm.h            | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 3c073fb58730..b53ec941f17e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -446,6 +446,7 @@ i915_gem_get_tiling(struct drm_device *dev, void *data,
>  	}
>  
>  	/* Hide bit 17 from the user -- see comment in i915_gem_set_tiling */
> +	args->phys_swizzle_mode = args->swizzle_mode;
>  	if (args->swizzle_mode == I915_BIT_6_SWIZZLE_9_17)
>  		args->swizzle_mode = I915_BIT_6_SWIZZLE_9;
>  	if (args->swizzle_mode == I915_BIT_6_SWIZZLE_9_10_17)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 7615ae68e015..5ff86692e03e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -934,6 +934,12 @@ struct drm_i915_gem_get_tiling {
>  	 * mmap mapping.
>  	 */
>  	__u32 swizzle_mode;
> +
> +	/**
> +	 * Returned address bit 6 swizzling required for CPU access through
> +	 * mmap mapping whilst bound.
> +	 */
> +	__u32 phys_swizzle_mode;
>  };
>  
>  struct drm_i915_gem_get_aperture {
> -- 
> 2.1.1
> 

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

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Chris Wilson]

On Fri, Oct 24, 2014 at 04:39:29PM +0200, Daniel Vetter wrote:
> On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> > Userspace cares about whether or not swizzling depends on the page
> > address for its direct access into bound objects. Extend the get_tiling
> > ioctl to report the physical swizzling value in addition to the logical
> > swizzling value so that userspace can accurately determine when it is
> > possible for manual detiling.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Akash Goel <akash.goel@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I wonder whether we should care really, since on gen5+ we know that they
> agree. Pimp some igt (by skipping if needed) to convince me?

It's for gen2-4... Trivial little change to enable more sharing of code
paths and speedups.

What type of igt do you want? I personally have never seen a bit17
swizzling machine, but we know that some do exist due to the bug
reports.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Daniel Vetter]

On Fri, Oct 24, 2014 at 04:16:14PM +0100, Chris Wilson wrote:
> On Fri, Oct 24, 2014 at 04:39:29PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> > > Userspace cares about whether or not swizzling depends on the page
> > > address for its direct access into bound objects. Extend the get_tiling
> > > ioctl to report the physical swizzling value in addition to the logical
> > > swizzling value so that userspace can accurately determine when it is
> > > possible for manual detiling.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Akash Goel <akash.goel@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I wonder whether we should care really, since on gen5+ we know that they
> > agree. Pimp some igt (by skipping if needed) to convince me?
> 
> It's for gen2-4... Trivial little change to enable more sharing of code
> paths and speedups.
> 
> What type of igt do you want? I personally have never seen a bit17
> swizzling machine, but we know that some do exist due to the bug
> reports.

I've thought of adding the relevant new cases to gem_tiled_pread.c. But
that's for the pread stuff and not cpu mmaps, but copypaste to
gem_tiled_cpu_mmap.c can fix that. And the desktop i915g I have here does
bit17 - without any L-shaped craziness even afaik!
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Chris Wilson]

On Mon, Oct 27, 2014 at 10:08:29AM +0100, Daniel Vetter wrote:
> On Fri, Oct 24, 2014 at 04:16:14PM +0100, Chris Wilson wrote:
> > On Fri, Oct 24, 2014 at 04:39:29PM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> > > > Userspace cares about whether or not swizzling depends on the page
> > > > address for its direct access into bound objects. Extend the get_tiling
> > > > ioctl to report the physical swizzling value in addition to the logical
> > > > swizzling value so that userspace can accurately determine when it is
> > > > possible for manual detiling.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Akash Goel <akash.goel@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > I wonder whether we should care really, since on gen5+ we know that they
> > > agree. Pimp some igt (by skipping if needed) to convince me?
> > 
> > It's for gen2-4... Trivial little change to enable more sharing of code
> > paths and speedups.
> > 
> > What type of igt do you want? I personally have never seen a bit17
> > swizzling machine, but we know that some do exist due to the bug
> > reports.
> 
> I've thought of adding the relevant new cases to gem_tiled_pread.c. But
> that's for the pread stuff and not cpu mmaps, but copypaste to
> gem_tiled_cpu_mmap.c can fix that. And the desktop i915g I have here does
> bit17 - without any L-shaped craziness even afaik!

But what do you actually want to verify? That if we write through a
linear view onto a tiled surface using the swizzle mode indicated by the
ioctl that it is in order when read back through a fence register? And
in particular for v2 that we don't do anything when the swizzle
indicates bit17 is involved.

A get_tiling ioctl test needs to exercise that it reports swizzling
correctly, right?
-Chris

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

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Daniel Vetter]

On Thu, Oct 30, 2014 at 10:40:53AM +0000, Chris Wilson wrote:
> On Mon, Oct 27, 2014 at 10:08:29AM +0100, Daniel Vetter wrote:
> > On Fri, Oct 24, 2014 at 04:16:14PM +0100, Chris Wilson wrote:
> > > On Fri, Oct 24, 2014 at 04:39:29PM +0200, Daniel Vetter wrote:
> > > > On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> > > > > Userspace cares about whether or not swizzling depends on the page
> > > > > address for its direct access into bound objects. Extend the get_tiling
> > > > > ioctl to report the physical swizzling value in addition to the logical
> > > > > swizzling value so that userspace can accurately determine when it is
> > > > > possible for manual detiling.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Akash Goel <akash.goel@intel.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > 
> > > > I wonder whether we should care really, since on gen5+ we know that they
> > > > agree. Pimp some igt (by skipping if needed) to convince me?
> > > 
> > > It's for gen2-4... Trivial little change to enable more sharing of code
> > > paths and speedups.
> > > 
> > > What type of igt do you want? I personally have never seen a bit17
> > > swizzling machine, but we know that some do exist due to the bug
> > > reports.
> > 
> > I've thought of adding the relevant new cases to gem_tiled_pread.c. But
> > that's for the pread stuff and not cpu mmaps, but copypaste to
> > gem_tiled_cpu_mmap.c can fix that. And the desktop i915g I have here does
> > bit17 - without any L-shaped craziness even afaik!
> 
> But what do you actually want to verify? That if we write through a
> linear view onto a tiled surface using the swizzle mode indicated by the
> ioctl that it is in order when read back through a fence register? And
> in particular for v2 that we don't do anything when the swizzle
> indicates bit17 is involved.
> 
> A get_tiling ioctl test needs to exercise that it reports swizzling
> correctly, right?

Yeah, if we copypaste gem_tiled_pread.c and use linear cpu mmaps instead
of pread then we should pretty much just get all this. And if you get
bit17 swizzling then we'd just skip this new testcase.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Chris Wilson]

On Mon, Nov 03, 2014 at 12:41:37PM +0100, Daniel Vetter wrote:
> On Thu, Oct 30, 2014 at 10:40:53AM +0000, Chris Wilson wrote:
> > On Mon, Oct 27, 2014 at 10:08:29AM +0100, Daniel Vetter wrote:
> > > On Fri, Oct 24, 2014 at 04:16:14PM +0100, Chris Wilson wrote:
> > > > On Fri, Oct 24, 2014 at 04:39:29PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> > > > > > Userspace cares about whether or not swizzling depends on the page
> > > > > > address for its direct access into bound objects. Extend the get_tiling
> > > > > > ioctl to report the physical swizzling value in addition to the logical
> > > > > > swizzling value so that userspace can accurately determine when it is
> > > > > > possible for manual detiling.
> > > > > > 
> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Akash Goel <akash.goel@intel.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > 
> > > > > I wonder whether we should care really, since on gen5+ we know that they
> > > > > agree. Pimp some igt (by skipping if needed) to convince me?
> > > > 
> > > > It's for gen2-4... Trivial little change to enable more sharing of code
> > > > paths and speedups.
> > > > 
> > > > What type of igt do you want? I personally have never seen a bit17
> > > > swizzling machine, but we know that some do exist due to the bug
> > > > reports.
> > > 
> > > I've thought of adding the relevant new cases to gem_tiled_pread.c. But
> > > that's for the pread stuff and not cpu mmaps, but copypaste to
> > > gem_tiled_cpu_mmap.c can fix that. And the desktop i915g I have here does
> > > bit17 - without any L-shaped craziness even afaik!
> > 
> > But what do you actually want to verify? That if we write through a
> > linear view onto a tiled surface using the swizzle mode indicated by the
> > ioctl that it is in order when read back through a fence register? And
> > in particular for v2 that we don't do anything when the swizzle
> > indicates bit17 is involved.
> > 
> > A get_tiling ioctl test needs to exercise that it reports swizzling
> > correctly, right?
> 
> Yeah, if we copypaste gem_tiled_pread.c and use linear cpu mmaps instead
> of pread then we should pretty much just get all this. And if you get
> bit17 swizzling then we'd just skip this new testcase.

Oh, gem_tiled_pread. That completely flew over my head.

gem_tiled_wc done. Though, the next job would be to unify the
gem_tiled_pread* and have wc equivalents for all.
-Chris

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

[PATCH] igt/gem_tiled_wc: Exercise wc mmaps with swizzling

[Chris Wilson]

This exercises both the wc mmappings and the extended get_tiling ioctl.
Userspace cannot handle bit17 swizzling through wc mmaps (because bit17
requires swizzling based on the actual physical address of the page -
which is unknown to userspace) and so we need an extended get_tiling
ioctl to report the actual as well as the logical swizzling on an
object. We then check that the contents of the object are tiled and
swizzled correctly when viewed through a wc mmap.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/gem_tiled_wc.c   | 241 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+)
 create mode 100644 tests/gem_tiled_wc.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 6c485f0..7023ede 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -100,6 +100,7 @@ gem_tiled_partial_pwrite_pread
 gem_tiled_pread
 gem_tiled_pread_pwrite
 gem_tiled_swapping
+gem_tiled_wc
 gem_tiling_max_stride
 gem_unfence_active_buffers
 gem_unref_active_buffers
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 6e5458a..d4a203c 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -133,6 +133,7 @@ TESTS_progs = \
 	gem_tiled_pread \
 	gem_tiled_pread_pwrite \
 	gem_tiled_swapping \
+	gem_tiled_wc \
 	gem_tiling_max_stride \
 	gem_unfence_active_buffers \
 	gem_unref_active_buffers \
diff --git a/tests/gem_tiled_wc.c b/tests/gem_tiled_wc.c
new file mode 100644
index 0000000..ad09bc9
--- /dev/null
+++ b/tests/gem_tiled_wc.c
@@ -0,0 +1,241 @@
+/*
+ * Copyright © 2009 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Eric Anholt <eric@anholt.net>
+ *
+ */
+
+/** @file gem_tiled_pread.c
+ *
+ * This is a test of pread's behavior on tiled objects with respect to the
+ * reported swizzling value.
+ *
+ * The goal is to exercise the slow_bit17_copy path for reading on bit17
+ * machines, but will also be useful for catching swizzling value bugs on
+ * other systems.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/ioctl.h>
+#include "drm.h"
+#include "ioctl_wrappers.h"
+#include "drmtest.h"
+#include "intel_io.h"
+#include "intel_chipset.h"
+
+#define WIDTH 512
+#define HEIGHT 512
+#define SIZE (WIDTH*HEIGHT*sizeof(uint32_t))
+
+#define PAGE_SIZE 4096
+
+static int tile_width;
+static int tile_height;
+static int tile_size;
+
+static uint32_t
+create_bo(int fd)
+{
+	uint32_t handle;
+	uint32_t *data;
+	int i;
+
+	handle = gem_create(fd, SizE);
+	gem_set_tiling(fd, handle, I915_TILING_X, WIDTH * sizeof(uint32_t));
+
+	/* Fill the BO with dwords starting at start_val */
+	data = gem_mmap__wc(fd, handle, SIZE, PROT_READ | PROT_WRITE);
+	for (i = 0; i < WIDTH*HEIGHT; i++)
+		data[i] = i;
+	munmap(data, SIZE);
+
+	return handle;
+}
+
+static int
+swizzle_bit(int bit, int offset)
+{
+	return (offset & (1 << bit)) >> (bit - 6);
+}
+
+/* Translate from a swizzled offset in the tiled buffer to the corresponding
+ * value from the original linear buffer.
+ */
+static uint32_t
+calculate_expected(int offset)
+{
+	int tile_off = offset & (tile_size - 1);
+	int tile_base = offset & -tile_size;
+	int tile_index = tile_base / tile_size;
+	int tiles_per_row = 4*WIDTH / tile_width;
+
+	/* base x,y values from the tile (page) index. */
+	int base_y = tile_index / tiles_per_row * tile_height;
+	int base_x = tile_index % tiles_per_row * (tile_width/4);
+
+	/* x, y offsets within the tile */
+	int tile_y = tile_off / tile_width;
+	int tile_x = (tile_off % tile_width) / 4;
+
+	igt_debug("%3d, %3d, %3d,%3d\n", base_x, base_y, tile_x, tile_y);
+	return (base_y + tile_y) * WIDTH + base_x + tile_x;
+}
+
+static void
+get_tiling(int fd, uint32_t handle, uint32_t *tiling, uint32_t *swizzle)
+{
+	struct drm_i915_gem_get_tiling2 {
+		uint32_t handle;
+		uint32_t tiling_mode;
+		uint32_t swizzle_mode;
+		uint32_t phys_swizzle_mode;
+	} arg;
+#define DRM_IOCTL_I915_GEM_GET_TILING2	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_GET_TILING, struct drm_i915_gem_get_tiling2)
+
+	memset(&arg, 0, sizeof(arg));
+	arg.handle = handle;
+
+	do_or_die(drmIoctl(fd, DRM_IOCTL_I915_GEM_GET_TILING2, &arg));
+	igt_requiee(arg.phys_swizzle_mode == arg.swizzle_mode);
+
+	*tiling = arg.tiling_mode;
+	*swizzle = arg.swizzle_mode;
+}
+
+igt_simple_main
+{
+	int fd;
+	int i, iter = 100;
+	uint32_t tiling, swizzle;
+	uint32_t handle;
+
+	fd = drm_open_any();
+
+	igt_require_mmap_wc(fd);
+	get_tiling(fd, handle, &tiling, &swizzle);
+
+	if (IS_GEN2(intel_get_drm_devid(fd))) {
+		tile_height = 16;
+		tile_width = 128;
+		tile_size = 2048;
+	} else {
+		tile_height = 8;
+		tile_width = 512;
+		tile_size = PAGE_SIZE;
+	}
+
+	handle = create_bo(fd);
+
+	/* Read a bunch of random subsets of the data and check that they come
+	 * out right.
+	 */
+	for (i = 0; i < iter; i++) {
+		int size = WIDTH * HEIGHT * 4;
+		int offset = (random() % size) & ~3;
+		int len = (random() % size) & ~3;
+		int first_page, last_page;
+		uint32_t *linear;
+		int j;
+
+		if (len == 0)
+			len = 4;
+
+		if (offset + len > size)
+			len = size - offset;
+
+		if (i == 0) {
+			offset = 0;
+			len = size;
+		}
+
+		first_page = offset & ~4095;
+		last_page = (offset + len + 4095) & ~4095;
+		offset -= first_page;
+
+		linear = gem_mmap__wc(fd, handle, first_page, last_page - first_page, PROT_READ);
+		igt_assert(linear);
+
+		/* Translate from offsets in the read buffer to the swizzled
+		 * address that it corresponds to.  This is the opposite of
+		 * what Mesa does (calculate offset to be read given the linear
+		 * offset it's looking for).
+		 */
+		for (j = offset; j < offset + len; j += 4) {
+			uint32_t expected_val, found_val;
+			int swizzled_offset;
+			const char *swizzle_str;
+
+			switch (swizzle) {
+			case I915_BIT_6_SWIZZLE_NONE:
+				swizzled_offset = j;
+				swizzle_str = "none";
+				break;
+			case I915_BIT_6_SWIZZLE_9:
+				swizzled_offset = j ^
+					swizzle_bit(9, j);
+				swizzle_str = "bit9";
+				break;
+			case I915_BIT_6_SWIZZLE_9_10:
+				swizzled_offset = j ^
+					swizzle_bit(9, j) ^
+					swizzle_bit(10, j);
+				swizzle_str = "bit9^10";
+				break;
+			case I915_BIT_6_SWIZZLE_9_11:
+				swizzled_offset = j ^
+					swizzle_bit(9, j) ^
+					swizzle_bit(11, j);
+				swizzle_str = "bit9^11";
+				break;
+			case I915_BIT_6_SWIZZLE_9_10_11:
+				swizzled_offset = j ^
+					swizzle_bit(9, j) ^
+					swizzle_bit(10, j) ^
+					swizzle_bit(11, j);
+				swizzle_str = "bit9^10^11";
+				break;
+			default:
+				igt_assert_f(0, "Bad swizzle bits; %d\n",
+					     swizzle);
+			}
+			expected_val = calculate_expected(swizzled_offset);
+			found_val = linear[(j - offset) / 4];
+			igt_assert_f(expected_val == found_val,
+				     "Bad read [%d]: %d instead of %d at 0x%08x "
+				     "for read from 0x%08x to 0x%08x, swizzle=%s\n",
+				     i, found_val, expected_val, j,
+				     offset, offset + len,
+				     swizzle_str);
+			munmap(linear, last_page - first_page);
+		}
+	}
+
+	close(fd);
+}
-- 
2.1.1

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

Re: [PATCH] drm/i915: Report the actual swizzling back to userspace

[Daniel Vetter]

On Tue, Nov 04, 2014 at 07:29:25AM +0000, Chris Wilson wrote:
> On Mon, Nov 03, 2014 at 12:41:37PM +0100, Daniel Vetter wrote:
> > On Thu, Oct 30, 2014 at 10:40:53AM +0000, Chris Wilson wrote:
> > > On Mon, Oct 27, 2014 at 10:08:29AM +0100, Daniel Vetter wrote:
> > > > On Fri, Oct 24, 2014 at 04:16:14PM +0100, Chris Wilson wrote:
> > > > > On Fri, Oct 24, 2014 at 04:39:29PM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Oct 24, 2014 at 12:11:11PM +0100, Chris Wilson wrote:
> > > > > > > Userspace cares about whether or not swizzling depends on the page
> > > > > > > address for its direct access into bound objects. Extend the get_tiling
> > > > > > > ioctl to report the physical swizzling value in addition to the logical
> > > > > > > swizzling value so that userspace can accurately determine when it is
> > > > > > > possible for manual detiling.
> > > > > > > 
> > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > Cc: Akash Goel <akash.goel@intel.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > 
> > > > > > I wonder whether we should care really, since on gen5+ we know that they
> > > > > > agree. Pimp some igt (by skipping if needed) to convince me?
> > > > > 
> > > > > It's for gen2-4... Trivial little change to enable more sharing of code
> > > > > paths and speedups.
> > > > > 
> > > > > What type of igt do you want? I personally have never seen a bit17
> > > > > swizzling machine, but we know that some do exist due to the bug
> > > > > reports.
> > > > 
> > > > I've thought of adding the relevant new cases to gem_tiled_pread.c. But
> > > > that's for the pread stuff and not cpu mmaps, but copypaste to
> > > > gem_tiled_cpu_mmap.c can fix that. And the desktop i915g I have here does
> > > > bit17 - without any L-shaped craziness even afaik!
> > > 
> > > But what do you actually want to verify? That if we write through a
> > > linear view onto a tiled surface using the swizzle mode indicated by the
> > > ioctl that it is in order when read back through a fence register? And
> > > in particular for v2 that we don't do anything when the swizzle
> > > indicates bit17 is involved.
> > > 
> > > A get_tiling ioctl test needs to exercise that it reports swizzling
> > > correctly, right?
> > 
> > Yeah, if we copypaste gem_tiled_pread.c and use linear cpu mmaps instead
> > of pread then we should pretty much just get all this. And if you get
> > bit17 swizzling then we'd just skip this new testcase.
> 
> Oh, gem_tiled_pread. That completely flew over my head.
> 
> gem_tiled_wc done. Though, the next job would be to unify the
> gem_tiled_pread* and have wc equivalents for all.

Ok, merged the kernel patch with the Testcase: line added, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] igt/gem_tiled_wc: Exercise wc mmaps with swizzling

[Daniel Vetter]

On Tue, Nov 04, 2014 at 07:35:24AM +0000, Chris Wilson wrote:
> This exercises both the wc mmappings and the extended get_tiling ioctl.
> Userspace cannot handle bit17 swizzling through wc mmaps (because bit17
> requires swizzling based on the actual physical address of the page -
> which is unknown to userspace) and so we need an extended get_tiling
> ioctl to report the actual as well as the logical swizzling on an
> object. We then check that the contents of the object are tiled and
> swizzled correctly when viewed through a wc mmap.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/.gitignore       |   1 +
>  tests/Makefile.sources |   1 +
>  tests/gem_tiled_wc.c   | 241 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 243 insertions(+)
>  create mode 100644 tests/gem_tiled_wc.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 6c485f0..7023ede 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -100,6 +100,7 @@ gem_tiled_partial_pwrite_pread
>  gem_tiled_pread
>  gem_tiled_pread_pwrite
>  gem_tiled_swapping
> +gem_tiled_wc
>  gem_tiling_max_stride
>  gem_unfence_active_buffers
>  gem_unref_active_buffers
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 6e5458a..d4a203c 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -133,6 +133,7 @@ TESTS_progs = \
>  	gem_tiled_pread \
>  	gem_tiled_pread_pwrite \
>  	gem_tiled_swapping \
> +	gem_tiled_wc \
>  	gem_tiling_max_stride \
>  	gem_unfence_active_buffers \
>  	gem_unref_active_buffers \
> diff --git a/tests/gem_tiled_wc.c b/tests/gem_tiled_wc.c
> new file mode 100644
> index 0000000..ad09bc9
> --- /dev/null
> +++ b/tests/gem_tiled_wc.c
> @@ -0,0 +1,241 @@
> +/*
> + * Copyright © 2009 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Eric Anholt <eric@anholt.net>
> + *
> + */
> +
> +/** @file gem_tiled_pread.c
> + *
> + * This is a test of pread's behavior on tiled objects with respect to the
> + * reported swizzling value.
> + *
> + * The goal is to exercise the slow_bit17_copy path for reading on bit17
> + * machines, but will also be useful for catching swizzling value bugs on
> + * other systems.
> + */

I guess this should be update, other Thomas Wood will put a bogus
description in ;-)

> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/time.h>
> +#include <sys/ioctl.h>
> +#include "drm.h"
> +#include "ioctl_wrappers.h"
> +#include "drmtest.h"
> +#include "intel_io.h"
> +#include "intel_chipset.h"
> +
> +#define WIDTH 512
> +#define HEIGHT 512
> +#define SIZE (WIDTH*HEIGHT*sizeof(uint32_t))
> +
> +#define PAGE_SIZE 4096
> +
> +static int tile_width;
> +static int tile_height;
> +static int tile_size;
> +
> +static uint32_t
> +create_bo(int fd)
> +{
> +	uint32_t handle;
> +	uint32_t *data;
> +	int i;
> +
> +	handle = gem_create(fd, SizE);
> +	gem_set_tiling(fd, handle, I915_TILING_X, WIDTH * sizeof(uint32_t));
> +
> +	/* Fill the BO with dwords starting at start_val */
> +	data = gem_mmap__wc(fd, handle, SIZE, PROT_READ | PROT_WRITE);
> +	for (i = 0; i < WIDTH*HEIGHT; i++)
> +		data[i] = i;
> +	munmap(data, SIZE);
> +
> +	return handle;
> +}
> +
> +static int
> +swizzle_bit(int bit, int offset)
> +{
> +	return (offset & (1 << bit)) >> (bit - 6);
> +}
> +
> +/* Translate from a swizzled offset in the tiled buffer to the corresponding
> + * value from the original linear buffer.
> + */
> +static uint32_t
> +calculate_expected(int offset)
> +{
> +	int tile_off = offset & (tile_size - 1);
> +	int tile_base = offset & -tile_size;
> +	int tile_index = tile_base / tile_size;
> +	int tiles_per_row = 4*WIDTH / tile_width;
> +
> +	/* base x,y values from the tile (page) index. */
> +	int base_y = tile_index / tiles_per_row * tile_height;
> +	int base_x = tile_index % tiles_per_row * (tile_width/4);
> +
> +	/* x, y offsets within the tile */
> +	int tile_y = tile_off / tile_width;
> +	int tile_x = (tile_off % tile_width) / 4;
> +
> +	igt_debug("%3d, %3d, %3d,%3d\n", base_x, base_y, tile_x, tile_y);
> +	return (base_y + tile_y) * WIDTH + base_x + tile_x;
> +}
> +
> +static void
> +get_tiling(int fd, uint32_t handle, uint32_t *tiling, uint32_t *swizzle)
> +{
> +	struct drm_i915_gem_get_tiling2 {
> +		uint32_t handle;
> +		uint32_t tiling_mode;
> +		uint32_t swizzle_mode;
> +		uint32_t phys_swizzle_mode;
> +	} arg;
> +#define DRM_IOCTL_I915_GEM_GET_TILING2	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_GET_TILING, struct drm_i915_gem_get_tiling2)
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.handle = handle;
> +
> +	do_or_die(drmIoctl(fd, DRM_IOCTL_I915_GEM_GET_TILING2, &arg));
> +	igt_requiee(arg.phys_swizzle_mode == arg.swizzle_mode);
> +
> +	*tiling = arg.tiling_mode;
> +	*swizzle = arg.swizzle_mode;
> +}
> +
> +igt_simple_main
> +{
> +	int fd;
> +	int i, iter = 100;
> +	uint32_t tiling, swizzle;
> +	uint32_t handle;
> +
> +	fd = drm_open_any();
> +
> +	igt_require_mmap_wc(fd);
> +	get_tiling(fd, handle, &tiling, &swizzle);
> +
> +	if (IS_GEN2(intel_get_drm_devid(fd))) {
> +		tile_height = 16;
> +		tile_width = 128;
> +		tile_size = 2048;
> +	} else {
> +		tile_height = 8;
> +		tile_width = 512;
> +		tile_size = PAGE_SIZE;
> +	}
> +
> +	handle = create_bo(fd);
> +
> +	/* Read a bunch of random subsets of the data and check that they come
> +	 * out right.
> +	 */
> +	for (i = 0; i < iter; i++) {
> +		int size = WIDTH * HEIGHT * 4;
> +		int offset = (random() % size) & ~3;
> +		int len = (random() % size) & ~3;
> +		int first_page, last_page;
> +		uint32_t *linear;
> +		int j;
> +
> +		if (len == 0)
> +			len = 4;
> +
> +		if (offset + len > size)
> +			len = size - offset;
> +
> +		if (i == 0) {
> +			offset = 0;
> +			len = size;
> +		}
> +
> +		first_page = offset & ~4095;
> +		last_page = (offset + len + 4095) & ~4095;
> +		offset -= first_page;
> +
> +		linear = gem_mmap__wc(fd, handle, first_page, last_page - first_page, PROT_READ);
> +		igt_assert(linear);
> +
> +		/* Translate from offsets in the read buffer to the swizzled
> +		 * address that it corresponds to.  This is the opposite of
> +		 * what Mesa does (calculate offset to be read given the linear
> +		 * offset it's looking for).
> +		 */
> +		for (j = offset; j < offset + len; j += 4) {
> +			uint32_t expected_val, found_val;
> +			int swizzled_offset;
> +			const char *swizzle_str;
> +
> +			switch (swizzle) {
> +			case I915_BIT_6_SWIZZLE_NONE:
> +				swizzled_offset = j;
> +				swizzle_str = "none";
> +				break;
> +			case I915_BIT_6_SWIZZLE_9:
> +				swizzled_offset = j ^
> +					swizzle_bit(9, j);
> +				swizzle_str = "bit9";
> +				break;
> +			case I915_BIT_6_SWIZZLE_9_10:
> +				swizzled_offset = j ^
> +					swizzle_bit(9, j) ^
> +					swizzle_bit(10, j);
> +				swizzle_str = "bit9^10";
> +				break;
> +			case I915_BIT_6_SWIZZLE_9_11:
> +				swizzled_offset = j ^
> +					swizzle_bit(9, j) ^
> +					swizzle_bit(11, j);
> +				swizzle_str = "bit9^11";
> +				break;
> +			case I915_BIT_6_SWIZZLE_9_10_11:
> +				swizzled_offset = j ^
> +					swizzle_bit(9, j) ^
> +					swizzle_bit(10, j) ^
> +					swizzle_bit(11, j);
> +				swizzle_str = "bit9^10^11";
> +				break;

I think an explicit list of all the bit17 swizzle modes we know about with
an igt_skip("bit17 swizzling") should be here. Otherwise the test will
fail. Otherwise lgtm.
-Daniel

> +			default:
> +				igt_assert_f(0, "Bad swizzle bits; %d\n",
> +					     swizzle);
> +			}
> +			expected_val = calculate_expected(swizzled_offset);
> +			found_val = linear[(j - offset) / 4];
> +			igt_assert_f(expected_val == found_val,
> +				     "Bad read [%d]: %d instead of %d at 0x%08x "
> +				     "for read from 0x%08x to 0x%08x, swizzle=%s\n",
> +				     i, found_val, expected_val, j,
> +				     offset, offset + len,
> +				     swizzle_str);
> +			munmap(linear, last_page - first_page);
> +		}
> +	}
> +
> +	close(fd);
> +}
> -- 
> 2.1.1
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] igt/gem_tiled_wc: Exercise wc mmaps with swizzling

[Chris Wilson]

On Tue, Nov 04, 2014 at 10:43:23AM +0100, Daniel Vetter wrote:
> > +			switch (swizzle) {
> > +			case I915_BIT_6_SWIZZLE_NONE:
> > +				swizzled_offset = j;
> > +				swizzle_str = "none";
> > +				break;
> > +			case I915_BIT_6_SWIZZLE_9:
> > +				swizzled_offset = j ^
> > +					swizzle_bit(9, j);
> > +				swizzle_str = "bit9";
> > +				break;
> > +			case I915_BIT_6_SWIZZLE_9_10:
> > +				swizzled_offset = j ^
> > +					swizzle_bit(9, j) ^
> > +					swizzle_bit(10, j);
> > +				swizzle_str = "bit9^10";
> > +				break;
> > +			case I915_BIT_6_SWIZZLE_9_11:
> > +				swizzled_offset = j ^
> > +					swizzle_bit(9, j) ^
> > +					swizzle_bit(11, j);
> > +				swizzle_str = "bit9^11";
> > +				break;
> > +			case I915_BIT_6_SWIZZLE_9_10_11:
> > +				swizzled_offset = j ^
> > +					swizzle_bit(9, j) ^
> > +					swizzle_bit(10, j) ^
> > +					swizzle_bit(11, j);
> > +				swizzle_str = "bit9^10^11";
> > +				break;
> 
> I think an explicit list of all the bit17 swizzle modes we know about with
> an igt_skip("bit17 swizzling") should be here. Otherwise the test will
> fail. Otherwise lgtm.

bit17 swizzling will trigger an igt_require() earlier on in get_tiling.
But yeah, we should skip an unknown swizzle rather than fail.

For Thomas,

/** @file gem_tiled_wc.c
 *
 * This is a test of write-combining mmap's behavior on tiled objects
 * with respect to the reported swizzling value.
 *
 * The goal is to exercise the complications that arise when using a linear
 * view of a tiled object that is subject to hardware swizzling. This is
 * useful to check that we are presenting the correct view of the object
 * to userspace, and that userspace has to respect the swizzle.
 */
-Chris

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