[PATCH v2] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[PATCH v2] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Chris Wilson]

Since the hardware sometimes mysteriously totally flummoxes the 64bit
read of a 64bit register when read using a single instruction, split the
read into two instructions. Since the read here is of automatically
incrementing timestamp counters, we also have to be very careful in
order to make sure that it does not increment between the two
instructions.

However, since userspace tried to workaround this issue and so enshrined
this ABI for a broken hardware read and in the process neglected that
the read only fails in some environments, we have to introduce a new
uABI flag for userspace to request the 2x32 bit accurate read of the
timestamp.

Reported-by: Karol Herbst <freedesktop@karolherbst.de>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2c477663d378..925b02d1125f 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1310,10 +1310,12 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_reg_read *reg = data;
 	struct register_whitelist const *entry = whitelist;
+	unsigned size;
+	u64 offset;
 	int i, ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
-		if (entry->offset == reg->offset &&
+		if (entry->offset == (reg->offset & -(1 << entry->size)) &&
 		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
 			break;
 	}
@@ -1321,23 +1323,33 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 	if (i == ARRAY_SIZE(whitelist))
 		return -EINVAL;
 
+	/* We use the low bits to encode extra flags as the register should
+	 * be naturally aligned (and that aren't limit the available flags
+	 * for that register).
+	 */
+	offset = entry->offset;
+	size = entry->size;
+	size |= reg->offset ^ offset;
+
 	intel_runtime_pm_get(dev_priv);
 
-	switch (entry->size) {
+	switch (size) {
+	case 8 | 1:
+		reg->val = I915_READ64_2x32(offset, offset+4);
+		break;
 	case 8:
-		reg->val = I915_READ64(reg->offset);
+		reg->val = I915_READ64(offset);
 		break;
 	case 4:
-		reg->val = I915_READ(reg->offset);
+		reg->val = I915_READ(offset);
 		break;
 	case 2:
-		reg->val = I915_READ16(reg->offset);
+		reg->val = I915_READ16(offset);
 		break;
 	case 1:
-		reg->val = I915_READ8(reg->offset);
+		reg->val = I915_READ8(offset);
 		break;
 	default:
-		MISSING_CASE(entry->size);
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.1.4

[PATCH] tests/gem_reg_read: Extend and check for valid 36b counter

[Michał Winiarski]

When reading the timestamp register with single 64b read, we're observing
invalid values on x86_64:

[f = valid counter value | X = garbage]

i386:   0x0000000fffffffff
x86_64: 0xffffffffXXXXXXXX

Test checks if the counter is moving and increasing.
Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
shifting the value on x86_64 if we can't.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 tests/gem_reg_read.c | 123 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 103 insertions(+), 20 deletions(-)

diff --git a/tests/gem_reg_read.c b/tests/gem_reg_read.c
index d3e68d9..ba12fb1 100644
--- a/tests/gem_reg_read.c
+++ b/tests/gem_reg_read.c
@@ -28,10 +28,14 @@
 #include <stdio.h>
 #include <string.h>
 #include <errno.h>
+#include <sys/utsname.h>
 
 #include "ioctl_wrappers.h"
 #include "drmtest.h"
 
+static bool is_x86_64;
+static bool has_proper_timestamp;
+
 struct local_drm_i915_reg_read {
 	__u64 offset;
 	__u64 val; /* Return value */
@@ -39,39 +43,118 @@ struct local_drm_i915_reg_read {
 
 #define REG_READ_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x31, struct local_drm_i915_reg_read)
 
-static uint64_t timer_query(int fd)
+#define RENDER_RING_TIMESTAMP 0x2358
+
+static int read_register(int fd, uint64_t offset, uint64_t * val)
 {
+	int ret;
 	struct local_drm_i915_reg_read reg_read;
+	reg_read.offset = offset;
 
-	reg_read.offset = 0x2358;
-	igt_fail_on_f(drmIoctl(fd, REG_READ_IOCTL, &reg_read),
-		      "positive test case failed: ");
+	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
 
-	return reg_read.val;
+	*val = reg_read.val;
+
+	return ret;
 }
 
-igt_simple_main
+static bool check_kernel_x86_64(void)
 {
-	struct local_drm_i915_reg_read reg_read;
-	int fd, ret;
+	int ret;
+	struct utsname uts;
 
-	fd = drm_open_any();
+	ret = uname(&uts);
+	igt_assert(ret == 0);
 
-	reg_read.offset = 0x2358;
-	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
-	igt_assert(ret == 0 || errno == EINVAL);
-	igt_require(ret == 0);
+	if (!strcmp(uts.machine, "x86_64"))
+		return true;
+
+	return false;
+}
+
+static bool check_timestamp(int fd)
+{
+	int ret;
+	uint64_t val;
+
+	ret = read_register(fd, RENDER_RING_TIMESTAMP | 1, &val);
+	if (ret != 0 && errno == EINVAL)
+		return false;
+
+	return true;
+}
+
+static uint64_t timer_query(int fd, uint64_t * val)
+{
+	uint64_t offset;
+	int ret;
+
+	offset = RENDER_RING_TIMESTAMP;
+	if (has_proper_timestamp)
+		offset |= 1;
+
+	ret = read_register(fd, offset, val);
+
+	if (is_x86_64 && !has_proper_timestamp)
+		*val >>= 32;
 
-	reg_read.val = timer_query(fd);
+	return ret;
+}
+
+static void test_timestamp_moving(int fd)
+{
+	uint64_t first_val, second_val;
+
+	igt_fail_on(timer_query(fd, &first_val) != 0);
 	sleep(1);
-	/* Check that timer is moving and isn't busted. */
-	igt_assert(timer_query(fd) != reg_read.val);
+	igt_fail_on(timer_query(fd, &second_val) != 0);
+	igt_assert(second_val != first_val);
+}
 
-	/* bad reg */
-	reg_read.offset = 0x12345678;
-	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
+static void test_timestamp_monotonic(int fd)
+{
+	uint64_t first_val, second_val;
+	bool retry = true;
 
-	igt_assert(ret != 0 && errno == EINVAL);
+	igt_fail_on(timer_query(fd, &first_val) != 0);
 
+retry:
+	sleep(1);
+	igt_fail_on(timer_query(fd, &second_val) != 0);
+	if (second_val <= first_val && retry) {
+		retry = false;
+		first_val = second_val;
+		goto retry;
+	}
+
+	igt_assert(second_val > first_val);
+}
+
+igt_main
+{
+	uint64_t val = 0;
+	int fd = -1;
+
+	fd = drm_open_any();
+	is_x86_64 = check_kernel_x86_64();
+	has_proper_timestamp = check_timestamp(fd);
 	close(fd);
+
+	igt_fixture {
+		fd = drm_open_any();
+	}
+
+	igt_subtest("bad-register")
+		igt_assert(read_register(fd, 0x12345678, &val) == -1 &&
+				errno == EINVAL);
+
+	igt_subtest("timestamp-moving")
+		test_timestamp_moving(fd);
+
+	igt_subtest("timestamp-monotonic")
+		test_timestamp_monotonic(fd);
+
+	igt_fixture {
+		close(fd);
+	}
 }
-- 
2.4.3

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

Re: [PATCH] tests/gem_reg_read: Extend and check for valid 36b counter

[Chris Wilson]

On Thu, Jul 16, 2015 at 11:06:59AM +0200, Michał Winiarski wrote:
> When reading the timestamp register with single 64b read, we're observing
> invalid values on x86_64:
> 
> [f = valid counter value | X = garbage]
> 
> i386:   0x0000000fffffffff
> x86_64: 0xffffffffXXXXXXXX
> 
> Test checks if the counter is moving and increasing.
> Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
> shifting the value on x86_64 if we can't.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---

> +static bool check_timestamp(int fd)
> +{
> +	int ret;
> +	uint64_t val;
> +
> +	ret = read_register(fd, RENDER_RING_TIMESTAMP | 1, &val);
> +	if (ret != 0 && errno == EINVAL)
> +		return false;

I would have said return ret == 0; would be safer rather than only
returning false if it matched the expected error.

> +	return true;
> +}
> +
> +static uint64_t timer_query(int fd, uint64_t * val)
> +{
> +	uint64_t offset;
> +	int ret;
> +
> +	offset = RENDER_RING_TIMESTAMP;
> +	if (has_proper_timestamp)
> +		offset |= 1;
> +
> +	ret = read_register(fd, offset, val);
> +

/*
 * When reading the timestamp register with single 64b read, we're observing
 * invalid values on x86_64:
 *
 *	[f = valid counter value | X = garbage]
 *
 *	i386:   0x0000000fffffffff
 *	x86_64: 0xffffffffXXXXXXXX
 *
 * In the absence of a corrected register read ioctl, attempt
 * to fix up the return value to be vaguely useful.
 */

> +	if (is_x86_64 && !has_proper_timestamp)
> +		*val >>= 32;
>  
> -	reg_read.val = timer_query(fd);
> +	return ret;
> +}
> +
> +static void test_timestamp_monotonic(int fd)
> +{
> +	uint64_t first_val, second_val;
> +	bool retry = true;
>  
> -	igt_assert(ret != 0 && errno == EINVAL);
> +	igt_fail_on(timer_query(fd, &first_val) != 0);
>  
> +retry:
> +	sleep(1);
> +	igt_fail_on(timer_query(fd, &second_val) != 0);
> +	if (second_val <= first_val && retry) {
> +		retry = false;
> +		first_val = second_val;
> +		goto retry;
> +	}

Hmm, I expected a few more iterations for a monotonic test.

Something like:
	start = gettime();
	first_val = timer_query(fd);
	do {
		second_val = timer_query(fd);
		igt_assert_gte(second_val, first_val);
		first_val = second_val;
		
		elapsed = getreltime(&start);
	} while (elapsed < 5);
> +
> +	igt_assert(second_val > first_val);
> +}
> +
> +igt_main
> +{
> +	uint64_t val = 0;
> +	int fd = -1;
> +
> +	fd = drm_open_any();
> +	is_x86_64 = check_kernel_x86_64();
> +	has_proper_timestamp = check_timestamp(fd);
>  	close(fd);

Just do this in the fixture I guess.

> +
> +	igt_fixture {
> +		fd = drm_open_any();
> +	}
-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 v2] tests/gem_reg_read: Extend and check for valid 36b counter

[Michał Winiarski]

When reading the timestamp register with single 64b read, we are observing
invalid values on x86_64:

    [f = valid counter value | X = garbage]

    i386:   0x0000000fffffffff
    x86_64: 0xffffffffXXXXXXXX

Test checks if the counter is moving and increasing.
Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
shifting the value on x86_64 if we can't.

v2: More iterations of monotonic test, comments, minor fixups (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 tests/gem_reg_read.c | 137 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 116 insertions(+), 21 deletions(-)

diff --git a/tests/gem_reg_read.c b/tests/gem_reg_read.c
index d3e68d9..b512866 100644
--- a/tests/gem_reg_read.c
+++ b/tests/gem_reg_read.c
@@ -28,10 +28,15 @@
 #include <stdio.h>
 #include <string.h>
 #include <errno.h>
+#include <sys/utsname.h>
+#include <time.h>
 
 #include "ioctl_wrappers.h"
 #include "drmtest.h"
 
+static bool is_x86_64;
+static bool has_proper_timestamp;
+
 struct local_drm_i915_reg_read {
 	__u64 offset;
 	__u64 val; /* Return value */
@@ -39,39 +44,129 @@ struct local_drm_i915_reg_read {
 
 #define REG_READ_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x31, struct local_drm_i915_reg_read)
 
-static uint64_t timer_query(int fd)
+#define RENDER_RING_TIMESTAMP 0x2358
+
+static int read_register(int fd, uint64_t offset, uint64_t * val)
 {
+	int ret;
 	struct local_drm_i915_reg_read reg_read;
+	reg_read.offset = offset;
 
-	reg_read.offset = 0x2358;
-	igt_fail_on_f(drmIoctl(fd, REG_READ_IOCTL, &reg_read),
-		      "positive test case failed: ");
+	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
 
-	return reg_read.val;
+	*val = reg_read.val;
+
+	return ret;
 }
 
-igt_simple_main
+static bool check_kernel_x86_64(void)
 {
-	struct local_drm_i915_reg_read reg_read;
-	int fd, ret;
+	int ret;
+	struct utsname uts;
 
-	fd = drm_open_any();
+	ret = uname(&uts);
+	igt_assert(ret == 0);
 
-	reg_read.offset = 0x2358;
-	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
-	igt_assert(ret == 0 || errno == EINVAL);
-	igt_require(ret == 0);
+	if (!strcmp(uts.machine, "x86_64"))
+		return true;
+
+	return false;
+}
+
+static bool check_timestamp(int fd)
+{
+	int ret;
+	uint64_t val;
 
-	reg_read.val = timer_query(fd);
+	ret = read_register(fd, RENDER_RING_TIMESTAMP | 1, &val);
+
+	return ret == 0;
+}
+
+static uint64_t timer_query(int fd, uint64_t * val)
+{
+	uint64_t offset;
+	int ret;
+
+	offset = RENDER_RING_TIMESTAMP;
+	if (has_proper_timestamp)
+		offset |= 1;
+
+	ret = read_register(fd, offset, val);
+
+/*
+ * When reading the timestamp register with single 64b read, we are observing
+ * invalid values on x86_64:
+ *
+ *      [f = valid counter value | X = garbage]
+ *
+ *      i386:   0x0000000fffffffff
+ *      x86_64: 0xffffffffXXXXXXXX
+ *
+ * In the absence of a corrected register read ioctl, attempt
+ * to fix up the return value to be vaguely useful.
+ */
+
+	if (is_x86_64 && !has_proper_timestamp)
+		*val >>= 32;
+
+	return ret;
+}
+
+static void test_timestamp_moving(int fd)
+{
+	uint64_t first_val, second_val;
+
+	igt_fail_on(timer_query(fd, &first_val) != 0);
 	sleep(1);
-	/* Check that timer is moving and isn't busted. */
-	igt_assert(timer_query(fd) != reg_read.val);
+	igt_fail_on(timer_query(fd, &second_val) != 0);
+	igt_assert(second_val != first_val);
+}
 
-	/* bad reg */
-	reg_read.offset = 0x12345678;
-	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
+static void test_timestamp_monotonic(int fd)
+{
+	uint64_t first_val, second_val;
+	time_t start;
+	bool retry = true;
+
+	igt_fail_on(timer_query(fd, &first_val) != 0);
+	time(&start);
+	do {
+retry:
+		igt_fail_on(timer_query(fd, &second_val) != 0);
+		if (second_val < first_val && retry) {
+		/* We may hit timestamp overflow once */
+			retry = false;
+			first_val = second_val;
+			goto retry;
+		}
+		igt_assert(second_val >= first_val);
+	} while(difftime(time(NULL), start) < 5);
+
+}
+
+igt_main
+{
+	uint64_t val = 0;
+	int fd = -1;
+
+	igt_fixture {
+		fd = drm_open_any();
+		is_x86_64 = check_kernel_x86_64();
+		has_proper_timestamp = check_timestamp(fd);
+	}
+
+	igt_subtest("bad-register")
+		igt_assert(read_register(fd, 0x12345678, &val) == -1 &&
+				errno == EINVAL);
+
+	igt_subtest("timestamp-moving")
+		test_timestamp_moving(fd);
 
-	igt_assert(ret != 0 && errno == EINVAL);
+	igt_subtest("timestamp-monotonic")
+		test_timestamp_monotonic(fd);
 
-	close(fd);
+	igt_fixture {
+		close(fd);
+	}
 }
-- 
2.4.3

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

Re: [PATCH v2] tests/gem_reg_read: Extend and check for valid 36b counter

[Chris Wilson]

On Thu, Jul 16, 2015 at 12:37:55PM +0200, Michał Winiarski wrote:
> When reading the timestamp register with single 64b read, we are observing
> invalid values on x86_64:
> 
>     [f = valid counter value | X = garbage]
> 
>     i386:   0x0000000fffffffff
>     x86_64: 0xffffffffXXXXXXXX
> 
> Test checks if the counter is moving and increasing.
> Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
> shifting the value on x86_64 if we can't.
> 
> v2: More iterations of monotonic test, comments, minor fixups (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Down to being really pinicky now,

> +static int read_register(int fd, uint64_t offset, uint64_t * val)
>  {
> +	int ret;
>  	struct local_drm_i915_reg_read reg_read;
> +	reg_read.offset = offset;
>  
> -	reg_read.offset = 0x2358;
> -	igt_fail_on_f(drmIoctl(fd, REG_READ_IOCTL, &reg_read),
> -		      "positive test case failed: ");
> +	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);

A nice trick to use here is

ret = 0;
if (drmIoctl())
	ret = -errno;

Then later bad-reg becomes igt_assert_eq(read_register(BAD), -EINVAL);

> +static uint64_t timer_query(int fd, uint64_t * val)

Return is just int

> +	igt_subtest("bad-register")
> +		igt_assert(read_register(fd, 0x12345678, &val) == -1 &&
> +				errno == EINVAL);
> +
> +	igt_subtest("timestamp-moving")
> +		test_timestamp_moving(fd);
>  
> -	igt_assert(ret != 0 && errno == EINVAL);
> +	igt_subtest("timestamp-monotonic")
> +		test_timestamp_monotonic(fd);

Both of these subtests should start with
	igt_skip(timer_query() != 0)
so that we correctly disable the test for all kernels/machines that don't
support the register read. (Rather than reporting the tests as fail.)
-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 v3] tests/gem_reg_read: Extend and check for valid 36b counter

[Michał Winiarski]

When reading the timestamp register with single 64b read, we are observing
invalid values on x86_64:

    [f = valid counter value | X = garbage]

    i386:   0x0000000fffffffff
    x86_64: 0xffffffffXXXXXXXX

Test checks if the counter is moving and increasing.
Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
shifting the value on x86_64 if we can't.

v2: More iterations of monotonic test, comments, minor fixups (Chris)
v3: Skip tests if reg_read is not supported

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 tests/gem_reg_read.c | 141 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 120 insertions(+), 21 deletions(-)

diff --git a/tests/gem_reg_read.c b/tests/gem_reg_read.c
index d3e68d9..28ecdf5 100644
--- a/tests/gem_reg_read.c
+++ b/tests/gem_reg_read.c
@@ -28,10 +28,15 @@
 #include <stdio.h>
 #include <string.h>
 #include <errno.h>
+#include <sys/utsname.h>
+#include <time.h>
 
 #include "ioctl_wrappers.h"
 #include "drmtest.h"
 
+static bool is_x86_64;
+static bool has_proper_timestamp;
+
 struct local_drm_i915_reg_read {
 	__u64 offset;
 	__u64 val; /* Return value */
@@ -39,39 +44,133 @@ struct local_drm_i915_reg_read {
 
 #define REG_READ_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x31, struct local_drm_i915_reg_read)
 
-static uint64_t timer_query(int fd)
+#define RENDER_RING_TIMESTAMP 0x2358
+
+static int read_register(int fd, uint64_t offset, uint64_t * val)
 {
+	int ret = 0;
 	struct local_drm_i915_reg_read reg_read;
+	reg_read.offset = offset;
+
+	if (drmIoctl(fd, REG_READ_IOCTL, &reg_read))
+		ret = -errno;
 
-	reg_read.offset = 0x2358;
-	igt_fail_on_f(drmIoctl(fd, REG_READ_IOCTL, &reg_read),
-		      "positive test case failed: ");
+	*val = reg_read.val;
 
-	return reg_read.val;
+	return ret;
 }
 
-igt_simple_main
+static bool check_kernel_x86_64(void)
 {
-	struct local_drm_i915_reg_read reg_read;
-	int fd, ret;
+	int ret;
+	struct utsname uts;
+
+	ret = uname(&uts);
+	igt_assert(ret == 0);
+
+	if (!strcmp(uts.machine, "x86_64"))
+		return true;
+
+	return false;
+}
+
+static bool check_timestamp(int fd)
+{
+	int ret;
+	uint64_t val;
+
+	ret = read_register(fd, RENDER_RING_TIMESTAMP | 1, &val);
+
+	return ret == 0;
+}
+
+static int timer_query(int fd, uint64_t * val)
+{
+	uint64_t offset;
+	int ret;
+
+	offset = RENDER_RING_TIMESTAMP;
+	if (has_proper_timestamp)
+		offset |= 1;
+
+	ret = read_register(fd, offset, val);
+
+/*
+ * When reading the timestamp register with single 64b read, we are observing
+ * invalid values on x86_64:
+ *
+ *      [f = valid counter value | X = garbage]
+ *
+ *      i386:   0x0000000fffffffff
+ *      x86_64: 0xffffffffXXXXXXXX
+ *
+ * In the absence of a corrected register read ioctl, attempt
+ * to fix up the return value to be vaguely useful.
+ */
 
-	fd = drm_open_any();
+	if (is_x86_64 && !has_proper_timestamp)
+		*val >>= 32;
 
-	reg_read.offset = 0x2358;
-	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
-	igt_assert(ret == 0 || errno == EINVAL);
-	igt_require(ret == 0);
+	return ret;
+}
+
+static void test_timestamp_moving(int fd)
+{
+	uint64_t first_val, second_val;
 
-	reg_read.val = timer_query(fd);
+	igt_fail_on(timer_query(fd, &first_val) != 0);
 	sleep(1);
-	/* Check that timer is moving and isn't busted. */
-	igt_assert(timer_query(fd) != reg_read.val);
+	igt_fail_on(timer_query(fd, &second_val) != 0);
+	igt_assert(second_val != first_val);
+}
+
+static void test_timestamp_monotonic(int fd)
+{
+	uint64_t first_val, second_val;
+	time_t start;
+	bool retry = true;
+
+	igt_fail_on(timer_query(fd, &first_val) != 0);
+	time(&start);
+	do {
+retry:
+		igt_fail_on(timer_query(fd, &second_val) != 0);
+		if (second_val < first_val && retry) {
+		/* We may hit timestamp overflow once */
+			retry = false;
+			first_val = second_val;
+			goto retry;
+		}
+		igt_assert(second_val >= first_val);
+	} while(difftime(time(NULL), start) < 5);
+
+}
+
+igt_main
+{
+	uint64_t val = 0;
+	int fd = -1;
+
+	igt_fixture {
+		fd = drm_open_any();
+		is_x86_64 = check_kernel_x86_64();
+		has_proper_timestamp = check_timestamp(fd);
+	}
+
+	igt_subtest("bad-register")
+		igt_assert_eq(read_register(fd, 0x12345678, &val), -EINVAL);
 
-	/* bad reg */
-	reg_read.offset = 0x12345678;
-	ret = drmIoctl(fd, REG_READ_IOCTL, &reg_read);
+	igt_subtest("timestamp-moving") {
+		igt_skip_on(timer_query(fd, &val) != 0);
+		test_timestamp_moving(fd);
+	}
 
-	igt_assert(ret != 0 && errno == EINVAL);
+	igt_subtest("timestamp-monotonic") {
+		igt_skip_on(timer_query(fd, &val) != 0);
+		test_timestamp_monotonic(fd);
+	}
 
-	close(fd);
+	igt_fixture {
+		close(fd);
+	}
 }
-- 
2.4.3

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

Re: [PATCH v3] tests/gem_reg_read: Extend and check for valid 36b counter

[Chris Wilson]

On Thu, Jul 16, 2015 at 01:19:09PM +0200, Michał Winiarski wrote:
> When reading the timestamp register with single 64b read, we are observing
> invalid values on x86_64:
> 
>     [f = valid counter value | X = garbage]
> 
>     i386:   0x0000000fffffffff
>     x86_64: 0xffffffffXXXXXXXX
> 
> Test checks if the counter is moving and increasing.
> Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
> shifting the value on x86_64 if we can't.
> 
> v2: More iterations of monotonic test, comments, minor fixups (Chris)
> v3: Skip tests if reg_read is not supported
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Lgtm,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Chris Wilson]

Since the hardware sometimes mysteriously totally flummoxes the 64bit
read of a 64bit register when read using a single instruction, split the
read into two instructions. Since the read here is of automatically
incrementing timestamp counters, we also have to be very careful in
order to make sure that it does not increment between the two
instructions.

However, since userspace tried to workaround this issue and so enshrined
this ABI for a broken hardware read and in the process neglected that
the read only fails in some environments, we have to introduce a new
uABI flag for userspace to request the 2x32 bit accurate read of the
timestamp.

v2: Fix alignment check and include details of the workaround for
userspace.

Reported-by: Karol Herbst <freedesktop@karolherbst.de>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
Testcase: igt/gem_reg_read
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++-------
 include/uapi/drm/i915_drm.h         |  8 ++++++++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2c477663d378..eb244b57b3fd 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1310,10 +1310,12 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_reg_read *reg = data;
 	struct register_whitelist const *entry = whitelist;
+	unsigned size;
+	u64 offset;
 	int i, ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
-		if (entry->offset == reg->offset &&
+		if (entry->offset == (reg->offset & -entry->size) &&
 		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
 			break;
 	}
@@ -1321,23 +1323,33 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 	if (i == ARRAY_SIZE(whitelist))
 		return -EINVAL;
 
+	/* We use the low bits to encode extra flags as the register should
+	 * be naturally aligned (and those that are not so aligned merely
+	 * limit the available flags for that register).
+	 */
+	offset = entry->offset;
+	size = entry->size;
+	size |= reg->offset ^ offset;
+
 	intel_runtime_pm_get(dev_priv);
 
-	switch (entry->size) {
+	switch (size) {
+	case 8 | 1:
+		reg->val = I915_READ64_2x32(offset, offset+4);
+		break;
 	case 8:
-		reg->val = I915_READ64(reg->offset);
+		reg->val = I915_READ64(offset);
 		break;
 	case 4:
-		reg->val = I915_READ(reg->offset);
+		reg->val = I915_READ(offset);
 		break;
 	case 2:
-		reg->val = I915_READ16(reg->offset);
+		reg->val = I915_READ16(offset);
 		break;
 	case 1:
-		reg->val = I915_READ8(reg->offset);
+		reg->val = I915_READ8(offset);
 		break;
 	default:
-		MISSING_CASE(entry->size);
 		ret = -EINVAL;
 		goto out;
 	}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index b0f82ddab987..83f60f01dca2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1087,6 +1087,14 @@ struct drm_i915_reg_read {
 	__u64 offset;
 	__u64 val; /* Return value */
 };
+/* Known registers:
+ *
+ * Render engine timestamp - 0x2358 + 64bit - gen7+
+ * - Note this register returns an invalid value if using the default
+ *   single instruction 8byte read, in order to workaround that use
+ *   offset (0x2538 | 1) instead.
+ *
+ */
 
 struct drm_i915_reset_stats {
 	__u32 ctx_id;
-- 
2.1.4

Re: [PATCH v3] tests/gem_reg_read: Extend and check for valid 36b counter

[Damien Lespiau]

On Thu, Jul 16, 2015 at 12:24:19PM +0100, Chris Wilson wrote:
> On Thu, Jul 16, 2015 at 01:19:09PM +0200, Michał Winiarski wrote:
> > When reading the timestamp register with single 64b read, we are observing
> > invalid values on x86_64:
> > 
> >     [f = valid counter value | X = garbage]
> > 
> >     i386:   0x0000000fffffffff
> >     x86_64: 0xffffffffXXXXXXXX
> > 
> > Test checks if the counter is moving and increasing.
> > Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
> > shifting the value on x86_64 if we can't.
> > 
> > v2: More iterations of monotonic test, comments, minor fixups (Chris)
> > v3: Skip tests if reg_read is not supported
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> Lgtm,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed! thanks for the patch and review.

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

Re: [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Michał Winiarski]

On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> Since the hardware sometimes mysteriously totally flummoxes the 64bit
> read of a 64bit register when read using a single instruction, split the
> read into two instructions. Since the read here is of automatically
> incrementing timestamp counters, we also have to be very careful in
> order to make sure that it does not increment between the two
> instructions.
> 
> However, since userspace tried to workaround this issue and so enshrined
> this ABI for a broken hardware read and in the process neglected that
> the read only fails in some environments, we have to introduce a new
> uABI flag for userspace to request the 2x32 bit accurate read of the
> timestamp.
> 
> v2: Fix alignment check and include details of the workaround for
> userspace.
> 
> Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> Testcase: igt/gem_reg_read
Tested-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++-------
>  include/uapi/drm/i915_drm.h         |  8 ++++++++
>  2 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2c477663d378..eb244b57b3fd 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1310,10 +1310,12 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_reg_read *reg = data;
>  	struct register_whitelist const *entry = whitelist;
> +	unsigned size;
> +	u64 offset;
>  	int i, ret = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
> -		if (entry->offset == reg->offset &&
> +		if (entry->offset == (reg->offset & -entry->size) &&
>  		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
>  			break;
>  	}
> @@ -1321,23 +1323,33 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  	if (i == ARRAY_SIZE(whitelist))
>  		return -EINVAL;
>  
> +	/* We use the low bits to encode extra flags as the register should
> +	 * be naturally aligned (and those that are not so aligned merely
> +	 * limit the available flags for that register).
> +	 */
> +	offset = entry->offset;
> +	size = entry->size;
> +	size |= reg->offset ^ offset;
> +
>  	intel_runtime_pm_get(dev_priv);
>  
> -	switch (entry->size) {
> +	switch (size) {
> +	case 8 | 1:
> +		reg->val = I915_READ64_2x32(offset, offset+4);
> +		break;
>  	case 8:
> -		reg->val = I915_READ64(reg->offset);
> +		reg->val = I915_READ64(offset);
>  		break;
>  	case 4:
> -		reg->val = I915_READ(reg->offset);
> +		reg->val = I915_READ(offset);
>  		break;
>  	case 2:
> -		reg->val = I915_READ16(reg->offset);
> +		reg->val = I915_READ16(offset);
>  		break;
>  	case 1:
> -		reg->val = I915_READ8(reg->offset);
> +		reg->val = I915_READ8(offset);
>  		break;
>  	default:
> -		MISSING_CASE(entry->size);
>  		ret = -EINVAL;
>  		goto out;
>  	}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index b0f82ddab987..83f60f01dca2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1087,6 +1087,14 @@ struct drm_i915_reg_read {
>  	__u64 offset;
>  	__u64 val; /* Return value */
>  };
> +/* Known registers:
> + *
> + * Render engine timestamp - 0x2358 + 64bit - gen7+
> + * - Note this register returns an invalid value if using the default
> + *   single instruction 8byte read, in order to workaround that use
> + *   offset (0x2538 | 1) instead.
> + *
> + */
>  
>  struct drm_i915_reset_stats {
>  	__u32 ctx_id;
> -- 
> 2.1.4
> 

Re: [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Michał Winiarski]

On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> Since the hardware sometimes mysteriously totally flummoxes the 64bit
> read of a 64bit register when read using a single instruction, split the
> read into two instructions. Since the read here is of automatically
> incrementing timestamp counters, we also have to be very careful in
> order to make sure that it does not increment between the two
> instructions.
> 
> However, since userspace tried to workaround this issue and so enshrined
> this ABI for a broken hardware read and in the process neglected that
> the read only fails in some environments, we have to introduce a new
> uABI flag for userspace to request the 2x32 bit accurate read of the
> timestamp.
> 
> v2: Fix alignment check and include details of the workaround for
> userspace.
> 
> Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> Testcase: igt/gem_reg_read
Tested-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++-------
>  include/uapi/drm/i915_drm.h         |  8 ++++++++
>  2 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2c477663d378..eb244b57b3fd 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1310,10 +1310,12 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_reg_read *reg = data;
>  	struct register_whitelist const *entry = whitelist;
> +	unsigned size;
> +	u64 offset;
>  	int i, ret = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
> -		if (entry->offset == reg->offset &&
> +		if (entry->offset == (reg->offset & -entry->size) &&
>  		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
>  			break;
>  	}
> @@ -1321,23 +1323,33 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  	if (i == ARRAY_SIZE(whitelist))
>  		return -EINVAL;
>  
> +	/* We use the low bits to encode extra flags as the register should
> +	 * be naturally aligned (and those that are not so aligned merely
> +	 * limit the available flags for that register).
> +	 */
> +	offset = entry->offset;
> +	size = entry->size;
> +	size |= reg->offset ^ offset;
> +
>  	intel_runtime_pm_get(dev_priv);
>  
> -	switch (entry->size) {
> +	switch (size) {
> +	case 8 | 1:
> +		reg->val = I915_READ64_2x32(offset, offset+4);
> +		break;
>  	case 8:
> -		reg->val = I915_READ64(reg->offset);
> +		reg->val = I915_READ64(offset);
>  		break;
>  	case 4:
> -		reg->val = I915_READ(reg->offset);
> +		reg->val = I915_READ(offset);
>  		break;
>  	case 2:
> -		reg->val = I915_READ16(reg->offset);
> +		reg->val = I915_READ16(offset);
>  		break;
>  	case 1:
> -		reg->val = I915_READ8(reg->offset);
> +		reg->val = I915_READ8(offset);
>  		break;
>  	default:
> -		MISSING_CASE(entry->size);
>  		ret = -EINVAL;
>  		goto out;
>  	}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index b0f82ddab987..83f60f01dca2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1087,6 +1087,14 @@ struct drm_i915_reg_read {
>  	__u64 offset;
>  	__u64 val; /* Return value */
>  };
> +/* Known registers:
> + *
> + * Render engine timestamp - 0x2358 + 64bit - gen7+
> + * - Note this register returns an invalid value if using the default
> + *   single instruction 8byte read, in order to workaround that use
> + *   offset (0x2538 | 1) instead.
> + *
> + */
>  
>  struct drm_i915_reset_stats {
>  	__u32 ctx_id;
> -- 
> 2.1.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6813
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                 -1              285/285              284/285
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] tests/gem_reg_read: Extend and check for valid 36b counter

[Daniel Vetter]

On Thu, Jul 16, 2015 at 2:04 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Thu, Jul 16, 2015 at 12:24:19PM +0100, Chris Wilson wrote:
>> On Thu, Jul 16, 2015 at 01:19:09PM +0200, Michał Winiarski wrote:
>> > When reading the timestamp register with single 64b read, we are observing
>> > invalid values on x86_64:
>> >
>> >     [f = valid counter value | X = garbage]
>> >
>> >     i386:   0x0000000fffffffff
>> >     x86_64: 0xffffffffXXXXXXXX
>> >
>> > Test checks if the counter is moving and increasing.
>> > Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
>> > shifting the value on x86_64 if we can't.
>> >
>> > v2: More iterations of monotonic test, comments, minor fixups (Chris)
>> > v3: Skip tests if reg_read is not supported
>> >
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>
>> Lgtm,
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Pushed! thanks for the patch and review.

This is a testcase for new abi and the kernel side hasn't landed yet.
Intentional breach of procedures?
-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: [Intel-gfx] [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Daniel Vetter]

On Fri, Jul 17, 2015 at 05:10:25PM +0200, Michał Winiarski wrote:
> On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> > Since the hardware sometimes mysteriously totally flummoxes the 64bit
> > read of a 64bit register when read using a single instruction, split the
> > read into two instructions. Since the read here is of automatically
> > incrementing timestamp counters, we also have to be very careful in
> > order to make sure that it does not increment between the two
> > instructions.
> > 
> > However, since userspace tried to workaround this issue and so enshrined
> > this ABI for a broken hardware read and in the process neglected that
> > the read only fails in some environments, we have to introduce a new
> > uABI flag for userspace to request the 2x32 bit accurate read of the
> > timestamp.
> > 
> > v2: Fix alignment check and include details of the workaround for
> > userspace.
> > 
> > Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> > Testcase: igt/gem_reg_read
> Tested-by: Michał Winiarski <michal.winiarski@intel.com>

Where are the mesa/beignet/libva patches for this?
-Daniel

> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++-------
> >  include/uapi/drm/i915_drm.h         |  8 ++++++++
> >  2 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 2c477663d378..eb244b57b3fd 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1310,10 +1310,12 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_reg_read *reg = data;
> >  	struct register_whitelist const *entry = whitelist;
> > +	unsigned size;
> > +	u64 offset;
> >  	int i, ret = 0;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
> > -		if (entry->offset == reg->offset &&
> > +		if (entry->offset == (reg->offset & -entry->size) &&
> >  		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
> >  			break;
> >  	}
> > @@ -1321,23 +1323,33 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >  	if (i == ARRAY_SIZE(whitelist))
> >  		return -EINVAL;
> >  
> > +	/* We use the low bits to encode extra flags as the register should
> > +	 * be naturally aligned (and those that are not so aligned merely
> > +	 * limit the available flags for that register).
> > +	 */
> > +	offset = entry->offset;
> > +	size = entry->size;
> > +	size |= reg->offset ^ offset;
> > +
> >  	intel_runtime_pm_get(dev_priv);
> >  
> > -	switch (entry->size) {
> > +	switch (size) {
> > +	case 8 | 1:
> > +		reg->val = I915_READ64_2x32(offset, offset+4);
> > +		break;
> >  	case 8:
> > -		reg->val = I915_READ64(reg->offset);
> > +		reg->val = I915_READ64(offset);
> >  		break;
> >  	case 4:
> > -		reg->val = I915_READ(reg->offset);
> > +		reg->val = I915_READ(offset);
> >  		break;
> >  	case 2:
> > -		reg->val = I915_READ16(reg->offset);
> > +		reg->val = I915_READ16(offset);
> >  		break;
> >  	case 1:
> > -		reg->val = I915_READ8(reg->offset);
> > +		reg->val = I915_READ8(offset);
> >  		break;
> >  	default:
> > -		MISSING_CASE(entry->size);
> >  		ret = -EINVAL;
> >  		goto out;
> >  	}
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index b0f82ddab987..83f60f01dca2 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1087,6 +1087,14 @@ struct drm_i915_reg_read {
> >  	__u64 offset;
> >  	__u64 val; /* Return value */
> >  };
> > +/* Known registers:
> > + *
> > + * Render engine timestamp - 0x2358 + 64bit - gen7+
> > + * - Note this register returns an invalid value if using the default
> > + *   single instruction 8byte read, in order to workaround that use
> > + *   offset (0x2538 | 1) instead.
> > + *
> > + */
> >  
> >  struct drm_i915_reset_stats {
> >  	__u32 ctx_id;
> > -- 
> > 2.1.4
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3] tests/gem_reg_read: Extend and check for valid 36b counter

[Damien Lespiau]

On Tue, Jul 21, 2015 at 08:48:05AM +0200, Daniel Vetter wrote:
> On Thu, Jul 16, 2015 at 2:04 PM, Damien Lespiau
> <damien.lespiau@intel.com> wrote:
> > On Thu, Jul 16, 2015 at 12:24:19PM +0100, Chris Wilson wrote:
> >> On Thu, Jul 16, 2015 at 01:19:09PM +0200, Michał Winiarski wrote:
> >> > When reading the timestamp register with single 64b read, we are observing
> >> > invalid values on x86_64:
> >> >
> >> >     [f = valid counter value | X = garbage]
> >> >
> >> >     i386:   0x0000000fffffffff
> >> >     x86_64: 0xffffffffXXXXXXXX
> >> >
> >> > Test checks if the counter is moving and increasing.
> >> > Add a check to see if we can use (reg | 1) flag to get a proper 36b timestamp,
> >> > shifting the value on x86_64 if we can't.
> >> >
> >> > v2: More iterations of monotonic test, comments, minor fixups (Chris)
> >> > v3: Skip tests if reg_read is not supported
> >> >
> >> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> >>
> >> Lgtm,
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > Pushed! thanks for the patch and review.
> 
> This is a testcase for new abi and the kernel side hasn't landed yet.
> Intentional breach of procedures?

Nop, was just overlooked.

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

Re: [Intel-gfx] [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Chris Wilson]

On Tue, Jul 21, 2015 at 08:49:31AM +0200, Daniel Vetter wrote:
> On Fri, Jul 17, 2015 at 05:10:25PM +0200, Michał Winiarski wrote:
> > On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> > > Since the hardware sometimes mysteriously totally flummoxes the 64bit
> > > read of a 64bit register when read using a single instruction, split the
> > > read into two instructions. Since the read here is of automatically
> > > incrementing timestamp counters, we also have to be very careful in
> > > order to make sure that it does not increment between the two
> > > instructions.
> > > 
> > > However, since userspace tried to workaround this issue and so enshrined
> > > this ABI for a broken hardware read and in the process neglected that
> > > the read only fails in some environments, we have to introduce a new
> > > uABI flag for userspace to request the 2x32 bit accurate read of the
> > > timestamp.
> > > 
> > > v2: Fix alignment check and include details of the workaround for
> > > userspace.
> > > 
> > > Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> > > Testcase: igt/gem_reg_read
> > Tested-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> Where are the mesa/beignet/libva patches for this?

Trivial. Absolutely trivial. Just waiting for the kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Daniel Vetter]

On Tue, Jul 21, 2015 at 10:45:45AM +0100, Chris Wilson wrote:
> On Tue, Jul 21, 2015 at 08:49:31AM +0200, Daniel Vetter wrote:
> > On Fri, Jul 17, 2015 at 05:10:25PM +0200, Michał Winiarski wrote:
> > > On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> > > > Since the hardware sometimes mysteriously totally flummoxes the 64bit
> > > > read of a 64bit register when read using a single instruction, split the
> > > > read into two instructions. Since the read here is of automatically
> > > > incrementing timestamp counters, we also have to be very careful in
> > > > order to make sure that it does not increment between the two
> > > > instructions.
> > > > 
> > > > However, since userspace tried to workaround this issue and so enshrined
> > > > this ABI for a broken hardware read and in the process neglected that
> > > > the read only fails in some environments, we have to introduce a new
> > > > uABI flag for userspace to request the 2x32 bit accurate read of the
> > > > timestamp.
> > > > 
> > > > v2: Fix alignment check and include details of the workaround for
> > > > userspace.
> > > > 
> > > > Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> > > > Testcase: igt/gem_reg_read
> > > Tested-by: Michał Winiarski <michal.winiarski@intel.com>
> > 
> > Where are the mesa/beignet/libva patches for this?
> 
> Trivial. Absolutely trivial. Just waiting for the kernel.

Well still not how it should be done, so I guess you owe me them all ;-)

Anyway, applied to -fixes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Daniel Vetter]

On Tue, Jul 21, 2015 at 10:45:45AM +0100, Chris Wilson wrote:
> On Tue, Jul 21, 2015 at 08:49:31AM +0200, Daniel Vetter wrote:
> > On Fri, Jul 17, 2015 at 05:10:25PM +0200, Michał Winiarski wrote:
> > > On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> > > > Since the hardware sometimes mysteriously totally flummoxes the 64bit
> > > > read of a 64bit register when read using a single instruction, split the
> > > > read into two instructions. Since the read here is of automatically
> > > > incrementing timestamp counters, we also have to be very careful in
> > > > order to make sure that it does not increment between the two
> > > > instructions.
> > > > 
> > > > However, since userspace tried to workaround this issue and so enshrined
> > > > this ABI for a broken hardware read and in the process neglected that
> > > > the read only fails in some environments, we have to introduce a new
> > > > uABI flag for userspace to request the 2x32 bit accurate read of the
> > > > timestamp.
> > > > 
> > > > v2: Fix alignment check and include details of the workaround for
> > > > userspace.
> > > > 
> > > > Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> > > > Testcase: igt/gem_reg_read
> > > Tested-by: Michał Winiarski <michal.winiarski@intel.com>
> > 
> > Where are the mesa/beignet/libva patches for this?
> 
> Trivial. Absolutely trivial. Just waiting for the kernel.

Well still not how it should be done, so I guess you owe me them all ;-)

Anyway, applied to -fixes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx