All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t v5 0/4] Calculate softpin offsets from minimum GTT alignment
@ 2019-11-04 17:13 ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Some tests assume 4kB page size while using softpin.  That assumption
may be wrong on future GEM backends with possibly larger minimum page
sizes.  As a result, those tests may either fail on softpin at offsets
which are incorrectly aligned, may silently skip such incorrectly
aligned addresses assuming them occupied by other users, or may always
succeed when examining invalid use patterns.

Provide a helper function that detects minimum page size and returns
the size order.  Use it in test which perform softpin to calculate
offsets suitable for actually used backing store.

Changelog:
v2: Don't skip failing offsets only when on full PPGTT,
  - simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Drop patch 1/2 "Don't filter out addresses when on PPGTT" - I don't
    know how to detect if the kernel is interfering with the user's GTT,
  - introduce patch 1/4 "lib: Move redundant local helpers to lib/",
    subsequent patch will use the helper,
  - introduce patch 2/4 "lib: Add GEM minimum page size helper",
    subsequent patches will use the new helper (inspired by Chris),
  - in former patch 2/2, now 3/4, initialize page size order with an
    actual minimum returned by the new helper (inspired by Chris),
  - add a new fix for gem_ctx_shared test (patch 4/4).
v4: Rename the helper, use 'minimum GTT alignment' term across the
    changes (Chris),
  - update variable names accordingly,
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the helper code (Chris),
  - drop no longer required patch 1/4 "lib: Move redundant local
    helpers to lib/",
  - reintroduce former patch 1/2 as 1/4 "tests/gem_exec_reloc: Don't
    filter out invalid addresses" with the former not on full PPGTT
    requirement for skipping now replaced with error code checking.
v5: Put the helper code under lib/i915/, not in lib/ioctl_wrappers.c
    (Chris),
  - validate objects with an invalid reloc applied so execbuf requests
    called only for validation purposes are actually not emitted to GPU
    (Chris),
  - move object validation code to a separate helper,
  - promote the former patch 2/4 as 1/4 so the new helper is available
    for use by the former patch 1/4, now 2/4,
  - silently skip only those offsets which have been explicitly
    reported as overlapping with shared GTT reserved space, not simply
    all which raise failures other than -EINVAL (Chris),
  - as an implementation of moving the probe out of line so it's not
    easily confused with the central point of the test (Chris), use
    object validation library helper just introduced,
  - name the variable 'stride', not 'alignment', it better reflects
    its purpose (Chris).

Janusz Krzysztofik (4):
  lib/i915: Add minimum GTT alignment helper
  tests/gem_exec_reloc: Don't filter out invalid addresses
  tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
  tests/gem_ctx_shared: Align objects using minimum GTT alignment

 lib/Makefile.sources        |   2 +
 lib/i915/gem_gtt_topology.c | 118 ++++++++++++++++++++++++++++++++++++
 lib/i915/gem_gtt_topology.h |  36 +++++++++++
 lib/meson.build             |   1 +
 tests/i915/gem_ctx_shared.c |   7 ++-
 tests/i915/gem_exec_reloc.c |  31 ++++++----
 6 files changed, 180 insertions(+), 15 deletions(-)
 create mode 100644 lib/i915/gem_gtt_topology.c
 create mode 100644 lib/i915/gem_gtt_topology.h

-- 
2.21.0

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH i-g-t v5 0/4] Calculate softpin offsets from minimum GTT alignment
@ 2019-11-04 17:13 ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Some tests assume 4kB page size while using softpin.  That assumption
may be wrong on future GEM backends with possibly larger minimum page
sizes.  As a result, those tests may either fail on softpin at offsets
which are incorrectly aligned, may silently skip such incorrectly
aligned addresses assuming them occupied by other users, or may always
succeed when examining invalid use patterns.

Provide a helper function that detects minimum page size and returns
the size order.  Use it in test which perform softpin to calculate
offsets suitable for actually used backing store.

Changelog:
v2: Don't skip failing offsets only when on full PPGTT,
  - simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Drop patch 1/2 "Don't filter out addresses when on PPGTT" - I don't
    know how to detect if the kernel is interfering with the user's GTT,
  - introduce patch 1/4 "lib: Move redundant local helpers to lib/",
    subsequent patch will use the helper,
  - introduce patch 2/4 "lib: Add GEM minimum page size helper",
    subsequent patches will use the new helper (inspired by Chris),
  - in former patch 2/2, now 3/4, initialize page size order with an
    actual minimum returned by the new helper (inspired by Chris),
  - add a new fix for gem_ctx_shared test (patch 4/4).
v4: Rename the helper, use 'minimum GTT alignment' term across the
    changes (Chris),
  - update variable names accordingly,
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the helper code (Chris),
  - drop no longer required patch 1/4 "lib: Move redundant local
    helpers to lib/",
  - reintroduce former patch 1/2 as 1/4 "tests/gem_exec_reloc: Don't
    filter out invalid addresses" with the former not on full PPGTT
    requirement for skipping now replaced with error code checking.
v5: Put the helper code under lib/i915/, not in lib/ioctl_wrappers.c
    (Chris),
  - validate objects with an invalid reloc applied so execbuf requests
    called only for validation purposes are actually not emitted to GPU
    (Chris),
  - move object validation code to a separate helper,
  - promote the former patch 2/4 as 1/4 so the new helper is available
    for use by the former patch 1/4, now 2/4,
  - silently skip only those offsets which have been explicitly
    reported as overlapping with shared GTT reserved space, not simply
    all which raise failures other than -EINVAL (Chris),
  - as an implementation of moving the probe out of line so it's not
    easily confused with the central point of the test (Chris), use
    object validation library helper just introduced,
  - name the variable 'stride', not 'alignment', it better reflects
    its purpose (Chris).

Janusz Krzysztofik (4):
  lib/i915: Add minimum GTT alignment helper
  tests/gem_exec_reloc: Don't filter out invalid addresses
  tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
  tests/gem_ctx_shared: Align objects using minimum GTT alignment

 lib/Makefile.sources        |   2 +
 lib/i915/gem_gtt_topology.c | 118 ++++++++++++++++++++++++++++++++++++
 lib/i915/gem_gtt_topology.h |  36 +++++++++++
 lib/meson.build             |   1 +
 tests/i915/gem_ctx_shared.c |   7 ++-
 tests/i915/gem_exec_reloc.c |  31 ++++++----
 6 files changed, 180 insertions(+), 15 deletions(-)
 create mode 100644 lib/i915/gem_gtt_topology.c
 create mode 100644 lib/i915/gem_gtt_topology.h

-- 
2.21.0

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* [igt-dev] [PATCH i-g-t v5 0/4] Calculate softpin offsets from minimum GTT alignment
@ 2019-11-04 17:13 ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Some tests assume 4kB page size while using softpin.  That assumption
may be wrong on future GEM backends with possibly larger minimum page
sizes.  As a result, those tests may either fail on softpin at offsets
which are incorrectly aligned, may silently skip such incorrectly
aligned addresses assuming them occupied by other users, or may always
succeed when examining invalid use patterns.

Provide a helper function that detects minimum page size and returns
the size order.  Use it in test which perform softpin to calculate
offsets suitable for actually used backing store.

Changelog:
v2: Don't skip failing offsets only when on full PPGTT,
  - simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Drop patch 1/2 "Don't filter out addresses when on PPGTT" - I don't
    know how to detect if the kernel is interfering with the user's GTT,
  - introduce patch 1/4 "lib: Move redundant local helpers to lib/",
    subsequent patch will use the helper,
  - introduce patch 2/4 "lib: Add GEM minimum page size helper",
    subsequent patches will use the new helper (inspired by Chris),
  - in former patch 2/2, now 3/4, initialize page size order with an
    actual minimum returned by the new helper (inspired by Chris),
  - add a new fix for gem_ctx_shared test (patch 4/4).
v4: Rename the helper, use 'minimum GTT alignment' term across the
    changes (Chris),
  - update variable names accordingly,
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the helper code (Chris),
  - drop no longer required patch 1/4 "lib: Move redundant local
    helpers to lib/",
  - reintroduce former patch 1/2 as 1/4 "tests/gem_exec_reloc: Don't
    filter out invalid addresses" with the former not on full PPGTT
    requirement for skipping now replaced with error code checking.
v5: Put the helper code under lib/i915/, not in lib/ioctl_wrappers.c
    (Chris),
  - validate objects with an invalid reloc applied so execbuf requests
    called only for validation purposes are actually not emitted to GPU
    (Chris),
  - move object validation code to a separate helper,
  - promote the former patch 2/4 as 1/4 so the new helper is available
    for use by the former patch 1/4, now 2/4,
  - silently skip only those offsets which have been explicitly
    reported as overlapping with shared GTT reserved space, not simply
    all which raise failures other than -EINVAL (Chris),
  - as an implementation of moving the probe out of line so it's not
    easily confused with the central point of the test (Chris), use
    object validation library helper just introduced,
  - name the variable 'stride', not 'alignment', it better reflects
    its purpose (Chris).

Janusz Krzysztofik (4):
  lib/i915: Add minimum GTT alignment helper
  tests/gem_exec_reloc: Don't filter out invalid addresses
  tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
  tests/gem_ctx_shared: Align objects using minimum GTT alignment

 lib/Makefile.sources        |   2 +
 lib/i915/gem_gtt_topology.c | 118 ++++++++++++++++++++++++++++++++++++
 lib/i915/gem_gtt_topology.h |  36 +++++++++++
 lib/meson.build             |   1 +
 tests/i915/gem_ctx_shared.c |   7 ++-
 tests/i915/gem_exec_reloc.c |  31 ++++++----
 6 files changed, 180 insertions(+), 15 deletions(-)
 create mode 100644 lib/i915/gem_gtt_topology.c
 create mode 100644 lib/i915/gem_gtt_topology.h

-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Some tests assume 4kB offset alignment while using softpin.  That
assumption may be wrong on future GEM backends with possibly larger
minimum page sizes.  As a result, those tests may either fail on
softpin at offsets which are incorrectly aligned, may silently skip
such incorrectly aligned addresses assuming them occupied by other
users, or may always succeed when examining invalid use patterns.

Provide a helper function that detects minimum GTT alignment.  Tests
may use it to calculate softpin offsets valid on actually used backing
store.

Also expose a new object validation helper just created, it may be
useful for checking if a shared GTT address is not reserved, for
example.

v2: Rename the helper, use 'minimum GTT alignment' term across the
    change (Chris),
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users,
  - simplify the code (Chris).
v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
  - validate objects with an invalid reloc applied so execbuf requests
    called only for validation purposes are actually not emitted to GPU
    (Chris),
  - move object validation code to a separate helper.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
---
 lib/Makefile.sources        |   2 +
 lib/i915/gem_gtt_topology.c | 118 ++++++++++++++++++++++++++++++++++++
 lib/i915/gem_gtt_topology.h |  36 +++++++++++
 lib/meson.build             |   1 +
 4 files changed, 157 insertions(+)
 create mode 100644 lib/i915/gem_gtt_topology.c
 create mode 100644 lib/i915/gem_gtt_topology.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 34e0c012..975200fc 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -7,6 +7,8 @@ lib_source_list =	 	\
 	i915/gem_context.h	\
 	i915/gem_engine_topology.c	\
 	i915/gem_engine_topology.h	\
+	i915/gem_gtt_topology.c	\
+	i915/gem_gtt_topology.h	\
 	i915/gem_scheduler.c	\
 	i915/gem_scheduler.h	\
 	i915/gem_submission.c	\
diff --git a/lib/i915/gem_gtt_topology.c b/lib/i915/gem_gtt_topology.c
new file mode 100644
index 00000000..427f0c14
--- /dev/null
+++ b/lib/i915/gem_gtt_topology.c
@@ -0,0 +1,118 @@
+/*
+ * Copyright © 2019 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.
+ */
+
+#include <errno.h>
+#include <stdint.h>
+#include <string.h>
+
+#include "i915_drm.h"
+
+#include "igt_core.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_gtt_topology.h"
+
+/**
+ * gem_gtt_validate_object:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function verifies validity of GEM object attributes.  For example,
+ * it is useful for validation of the object softpin offset.
+ *
+ * Returns:
+ * 0 on success, negative error code returned by GEM_EXECBUF IOCTL on failure
+ */
+
+int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
+{
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	uintptr_t old_reloc;
+	int old_count, err;
+
+	memset(&reloc, 0, sizeof(reloc));
+	memset(&execbuf, 0, sizeof(execbuf));
+
+	/* use invalid reloc to save request emission */
+	old_reloc = obj->relocs_ptr;
+	old_count = obj->relocation_count;
+	obj->relocs_ptr = to_user_pointer(&reloc);
+	obj->relocation_count = 1;
+	gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
+
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = 1;
+
+	err = __gem_execbuf(fd, &execbuf);
+	if (err == -ENOENT) {
+		/* suppress -ENOENT we get for invalid reloc handle */
+		err = 0;
+	}
+
+	obj->relocs_ptr = old_reloc;
+	obj->relocation_count = old_count;
+
+	return err;
+}
+
+/**
+ * gem_gtt_min_alignment_order:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function detects the minimum possible alignment of a soft-pinned gem
+ * object allocated from a default backing store.  It is useful for calculating
+ * correctly aligned softpin offsets.
+ * Since size order to size conversion (size = 1 << order) is less trivial
+ * than the opposite, the function returns the alignment order as more handy.
+ *
+ * Returns:
+ * Size order of the minimum GTT alignment
+ */
+
+int gem_gtt_min_alignment_order(int fd)
+{
+	struct drm_i915_gem_exec_object2 obj;
+	int order;
+
+	/* no softpin => 4kB page size */
+	if (!gem_has_softpin(fd))
+		return 12;
+
+	memset(&obj, 0, sizeof(obj));
+
+	obj.handle = gem_create(fd, 4096);
+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+
+	for (order = 12; order < 64; order++) {
+		obj.offset = 1ull << order;
+		if (gem_gtt_validate_object(fd, &obj) != -EINVAL)
+			break;
+	}
+	igt_assert(obj.offset < gem_aperture_size(fd));
+
+	gem_close(fd, obj.handle);
+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
+	return order;
+}
diff --git a/lib/i915/gem_gtt_topology.h b/lib/i915/gem_gtt_topology.h
new file mode 100644
index 00000000..59161c8a
--- /dev/null
+++ b/lib/i915/gem_gtt_topology.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright © 2019 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>
+ *    Daniel Vetter <daniel.vetter@ffwll.ch>
+ *
+ */
+
+#ifndef GEM_GTT_TOPOLOGY_H
+#define GEM_GTT_TOPOLOGY_H
+
+int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj);
+int gem_gtt_min_alignment_order(int fd);
+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
+
+#endif /* GEM_GTT_TOPOLOGY_H */
diff --git a/lib/meson.build b/lib/meson.build
index fbc0c8d1..125de1a1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -2,6 +2,7 @@ lib_sources = [
 	'drmtest.c',
 	'i915/gem_context.c',
 	'i915/gem_engine_topology.c',
+	'i915/gem_gtt_topology.c',
 	'i915/gem_scheduler.c',
 	'i915/gem_submission.c',
 	'i915/gem_ring.c',
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Some tests assume 4kB offset alignment while using softpin.  That
assumption may be wrong on future GEM backends with possibly larger
minimum page sizes.  As a result, those tests may either fail on
softpin at offsets which are incorrectly aligned, may silently skip
such incorrectly aligned addresses assuming them occupied by other
users, or may always succeed when examining invalid use patterns.

Provide a helper function that detects minimum GTT alignment.  Tests
may use it to calculate softpin offsets valid on actually used backing
store.

Also expose a new object validation helper just created, it may be
useful for checking if a shared GTT address is not reserved, for
example.

v2: Rename the helper, use 'minimum GTT alignment' term across the
    change (Chris),
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users,
  - simplify the code (Chris).
v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
  - validate objects with an invalid reloc applied so execbuf requests
    called only for validation purposes are actually not emitted to GPU
    (Chris),
  - move object validation code to a separate helper.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
---
 lib/Makefile.sources        |   2 +
 lib/i915/gem_gtt_topology.c | 118 ++++++++++++++++++++++++++++++++++++
 lib/i915/gem_gtt_topology.h |  36 +++++++++++
 lib/meson.build             |   1 +
 4 files changed, 157 insertions(+)
 create mode 100644 lib/i915/gem_gtt_topology.c
 create mode 100644 lib/i915/gem_gtt_topology.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 34e0c012..975200fc 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -7,6 +7,8 @@ lib_source_list =	 	\
 	i915/gem_context.h	\
 	i915/gem_engine_topology.c	\
 	i915/gem_engine_topology.h	\
+	i915/gem_gtt_topology.c	\
+	i915/gem_gtt_topology.h	\
 	i915/gem_scheduler.c	\
 	i915/gem_scheduler.h	\
 	i915/gem_submission.c	\
diff --git a/lib/i915/gem_gtt_topology.c b/lib/i915/gem_gtt_topology.c
new file mode 100644
index 00000000..427f0c14
--- /dev/null
+++ b/lib/i915/gem_gtt_topology.c
@@ -0,0 +1,118 @@
+/*
+ * Copyright © 2019 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.
+ */
+
+#include <errno.h>
+#include <stdint.h>
+#include <string.h>
+
+#include "i915_drm.h"
+
+#include "igt_core.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_gtt_topology.h"
+
+/**
+ * gem_gtt_validate_object:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function verifies validity of GEM object attributes.  For example,
+ * it is useful for validation of the object softpin offset.
+ *
+ * Returns:
+ * 0 on success, negative error code returned by GEM_EXECBUF IOCTL on failure
+ */
+
+int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
+{
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	uintptr_t old_reloc;
+	int old_count, err;
+
+	memset(&reloc, 0, sizeof(reloc));
+	memset(&execbuf, 0, sizeof(execbuf));
+
+	/* use invalid reloc to save request emission */
+	old_reloc = obj->relocs_ptr;
+	old_count = obj->relocation_count;
+	obj->relocs_ptr = to_user_pointer(&reloc);
+	obj->relocation_count = 1;
+	gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
+
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = 1;
+
+	err = __gem_execbuf(fd, &execbuf);
+	if (err == -ENOENT) {
+		/* suppress -ENOENT we get for invalid reloc handle */
+		err = 0;
+	}
+
+	obj->relocs_ptr = old_reloc;
+	obj->relocation_count = old_count;
+
+	return err;
+}
+
+/**
+ * gem_gtt_min_alignment_order:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function detects the minimum possible alignment of a soft-pinned gem
+ * object allocated from a default backing store.  It is useful for calculating
+ * correctly aligned softpin offsets.
+ * Since size order to size conversion (size = 1 << order) is less trivial
+ * than the opposite, the function returns the alignment order as more handy.
+ *
+ * Returns:
+ * Size order of the minimum GTT alignment
+ */
+
+int gem_gtt_min_alignment_order(int fd)
+{
+	struct drm_i915_gem_exec_object2 obj;
+	int order;
+
+	/* no softpin => 4kB page size */
+	if (!gem_has_softpin(fd))
+		return 12;
+
+	memset(&obj, 0, sizeof(obj));
+
+	obj.handle = gem_create(fd, 4096);
+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+
+	for (order = 12; order < 64; order++) {
+		obj.offset = 1ull << order;
+		if (gem_gtt_validate_object(fd, &obj) != -EINVAL)
+			break;
+	}
+	igt_assert(obj.offset < gem_aperture_size(fd));
+
+	gem_close(fd, obj.handle);
+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
+	return order;
+}
diff --git a/lib/i915/gem_gtt_topology.h b/lib/i915/gem_gtt_topology.h
new file mode 100644
index 00000000..59161c8a
--- /dev/null
+++ b/lib/i915/gem_gtt_topology.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright © 2019 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>
+ *    Daniel Vetter <daniel.vetter@ffwll.ch>
+ *
+ */
+
+#ifndef GEM_GTT_TOPOLOGY_H
+#define GEM_GTT_TOPOLOGY_H
+
+int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj);
+int gem_gtt_min_alignment_order(int fd);
+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
+
+#endif /* GEM_GTT_TOPOLOGY_H */
diff --git a/lib/meson.build b/lib/meson.build
index fbc0c8d1..125de1a1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -2,6 +2,7 @@ lib_sources = [
 	'drmtest.c',
 	'i915/gem_context.c',
 	'i915/gem_engine_topology.c',
+	'i915/gem_gtt_topology.c',
 	'i915/gem_scheduler.c',
 	'i915/gem_submission.c',
 	'i915/gem_ring.c',
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Some tests assume 4kB offset alignment while using softpin.  That
assumption may be wrong on future GEM backends with possibly larger
minimum page sizes.  As a result, those tests may either fail on
softpin at offsets which are incorrectly aligned, may silently skip
such incorrectly aligned addresses assuming them occupied by other
users, or may always succeed when examining invalid use patterns.

Provide a helper function that detects minimum GTT alignment.  Tests
may use it to calculate softpin offsets valid on actually used backing
store.

Also expose a new object validation helper just created, it may be
useful for checking if a shared GTT address is not reserved, for
example.

v2: Rename the helper, use 'minimum GTT alignment' term across the
    change (Chris),
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users,
  - simplify the code (Chris).
v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
  - validate objects with an invalid reloc applied so execbuf requests
    called only for validation purposes are actually not emitted to GPU
    (Chris),
  - move object validation code to a separate helper.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
---
 lib/Makefile.sources        |   2 +
 lib/i915/gem_gtt_topology.c | 118 ++++++++++++++++++++++++++++++++++++
 lib/i915/gem_gtt_topology.h |  36 +++++++++++
 lib/meson.build             |   1 +
 4 files changed, 157 insertions(+)
 create mode 100644 lib/i915/gem_gtt_topology.c
 create mode 100644 lib/i915/gem_gtt_topology.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 34e0c012..975200fc 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -7,6 +7,8 @@ lib_source_list =	 	\
 	i915/gem_context.h	\
 	i915/gem_engine_topology.c	\
 	i915/gem_engine_topology.h	\
+	i915/gem_gtt_topology.c	\
+	i915/gem_gtt_topology.h	\
 	i915/gem_scheduler.c	\
 	i915/gem_scheduler.h	\
 	i915/gem_submission.c	\
diff --git a/lib/i915/gem_gtt_topology.c b/lib/i915/gem_gtt_topology.c
new file mode 100644
index 00000000..427f0c14
--- /dev/null
+++ b/lib/i915/gem_gtt_topology.c
@@ -0,0 +1,118 @@
+/*
+ * Copyright © 2019 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.
+ */
+
+#include <errno.h>
+#include <stdint.h>
+#include <string.h>
+
+#include "i915_drm.h"
+
+#include "igt_core.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_gtt_topology.h"
+
+/**
+ * gem_gtt_validate_object:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function verifies validity of GEM object attributes.  For example,
+ * it is useful for validation of the object softpin offset.
+ *
+ * Returns:
+ * 0 on success, negative error code returned by GEM_EXECBUF IOCTL on failure
+ */
+
+int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
+{
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	uintptr_t old_reloc;
+	int old_count, err;
+
+	memset(&reloc, 0, sizeof(reloc));
+	memset(&execbuf, 0, sizeof(execbuf));
+
+	/* use invalid reloc to save request emission */
+	old_reloc = obj->relocs_ptr;
+	old_count = obj->relocation_count;
+	obj->relocs_ptr = to_user_pointer(&reloc);
+	obj->relocation_count = 1;
+	gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
+
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = 1;
+
+	err = __gem_execbuf(fd, &execbuf);
+	if (err == -ENOENT) {
+		/* suppress -ENOENT we get for invalid reloc handle */
+		err = 0;
+	}
+
+	obj->relocs_ptr = old_reloc;
+	obj->relocation_count = old_count;
+
+	return err;
+}
+
+/**
+ * gem_gtt_min_alignment_order:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function detects the minimum possible alignment of a soft-pinned gem
+ * object allocated from a default backing store.  It is useful for calculating
+ * correctly aligned softpin offsets.
+ * Since size order to size conversion (size = 1 << order) is less trivial
+ * than the opposite, the function returns the alignment order as more handy.
+ *
+ * Returns:
+ * Size order of the minimum GTT alignment
+ */
+
+int gem_gtt_min_alignment_order(int fd)
+{
+	struct drm_i915_gem_exec_object2 obj;
+	int order;
+
+	/* no softpin => 4kB page size */
+	if (!gem_has_softpin(fd))
+		return 12;
+
+	memset(&obj, 0, sizeof(obj));
+
+	obj.handle = gem_create(fd, 4096);
+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+
+	for (order = 12; order < 64; order++) {
+		obj.offset = 1ull << order;
+		if (gem_gtt_validate_object(fd, &obj) != -EINVAL)
+			break;
+	}
+	igt_assert(obj.offset < gem_aperture_size(fd));
+
+	gem_close(fd, obj.handle);
+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
+	return order;
+}
diff --git a/lib/i915/gem_gtt_topology.h b/lib/i915/gem_gtt_topology.h
new file mode 100644
index 00000000..59161c8a
--- /dev/null
+++ b/lib/i915/gem_gtt_topology.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright © 2019 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>
+ *    Daniel Vetter <daniel.vetter@ffwll.ch>
+ *
+ */
+
+#ifndef GEM_GTT_TOPOLOGY_H
+#define GEM_GTT_TOPOLOGY_H
+
+int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj);
+int gem_gtt_min_alignment_order(int fd);
+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
+
+#endif /* GEM_GTT_TOPOLOGY_H */
diff --git a/lib/meson.build b/lib/meson.build
index fbc0c8d1..125de1a1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -2,6 +2,7 @@ lib_sources = [
 	'drmtest.c',
 	'i915/gem_context.c',
 	'i915/gem_engine_topology.c',
+	'i915/gem_gtt_topology.c',
 	'i915/gem_scheduler.c',
 	'i915/gem_submission.c',
 	'i915/gem_ring.c',
-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
addresses for !ppgtt") introduced filtering of addresses possibly
occupied by other users of shared GTT.  Unfortunately, that filtering
doesn't distinguish between actually occupied addresses and otherwise
invalid softpin offsets.  If incorrect GTT alignment will be assumed
when running on future backends with possibly larger minimum page
sizes, a half of calculated offsets to be tested will be incorrectly
detected as occupied by other users and silently skipped instead of
reported as a problem.  That will significantly distort the intended
test pattern.

Filter out failing addresses only if reported as occupied.

v2: Skip unavailable addresses only when not running on full PPGTT.
v3: Replace the not on full PPGTT requirement for skipping with error
    code checking.
v4: Silently skip only those offsets which have been explicitly
    reported as overlapping with shared GTT reserved space, not simply
    all which raise failures other than -EINVAL (Chris),
  - as an implementation of moving the probe out of line so it's not
    easily confused with the central point of the test (Chris), use
    object validation library helper just introduced.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index fdd9661d..e46a4df7 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -23,6 +23,7 @@
 
 #include "igt.h"
 #include "igt_dummyload.h"
+#include "i915/gem_gtt_topology.c"
 
 IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
 
@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
-	int count, n;
+	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
 		obj[n].offset = (1ull << (i + 12)) - 4096;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
-		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = gem_gtt_validate_object(fd, &obj[n]);
+		if (err) {
+			/* Iff using a shared GTT, some of it may be reserved */
+			igt_assert_eq(err, -ENOSPC);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
 		obj[n].offset = 1ull << (i + 12);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
-		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = gem_gtt_validate_object(fd, &obj[n]);
+		if (err) {
+			/* Iff using a shared GTT, some of it may be reserved */
+			igt_assert_eq(err, -ENOSPC);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
addresses for !ppgtt") introduced filtering of addresses possibly
occupied by other users of shared GTT.  Unfortunately, that filtering
doesn't distinguish between actually occupied addresses and otherwise
invalid softpin offsets.  If incorrect GTT alignment will be assumed
when running on future backends with possibly larger minimum page
sizes, a half of calculated offsets to be tested will be incorrectly
detected as occupied by other users and silently skipped instead of
reported as a problem.  That will significantly distort the intended
test pattern.

Filter out failing addresses only if reported as occupied.

v2: Skip unavailable addresses only when not running on full PPGTT.
v3: Replace the not on full PPGTT requirement for skipping with error
    code checking.
v4: Silently skip only those offsets which have been explicitly
    reported as overlapping with shared GTT reserved space, not simply
    all which raise failures other than -EINVAL (Chris),
  - as an implementation of moving the probe out of line so it's not
    easily confused with the central point of the test (Chris), use
    object validation library helper just introduced.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index fdd9661d..e46a4df7 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -23,6 +23,7 @@
 
 #include "igt.h"
 #include "igt_dummyload.h"
+#include "i915/gem_gtt_topology.c"
 
 IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
 
@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
-	int count, n;
+	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
 		obj[n].offset = (1ull << (i + 12)) - 4096;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
-		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = gem_gtt_validate_object(fd, &obj[n]);
+		if (err) {
+			/* Iff using a shared GTT, some of it may be reserved */
+			igt_assert_eq(err, -ENOSPC);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
 		obj[n].offset = 1ull << (i + 12);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
-		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = gem_gtt_validate_object(fd, &obj[n]);
+		if (err) {
+			/* Iff using a shared GTT, some of it may be reserved */
+			igt_assert_eq(err, -ENOSPC);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [igt-dev] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
addresses for !ppgtt") introduced filtering of addresses possibly
occupied by other users of shared GTT.  Unfortunately, that filtering
doesn't distinguish between actually occupied addresses and otherwise
invalid softpin offsets.  If incorrect GTT alignment will be assumed
when running on future backends with possibly larger minimum page
sizes, a half of calculated offsets to be tested will be incorrectly
detected as occupied by other users and silently skipped instead of
reported as a problem.  That will significantly distort the intended
test pattern.

Filter out failing addresses only if reported as occupied.

v2: Skip unavailable addresses only when not running on full PPGTT.
v3: Replace the not on full PPGTT requirement for skipping with error
    code checking.
v4: Silently skip only those offsets which have been explicitly
    reported as overlapping with shared GTT reserved space, not simply
    all which raise failures other than -EINVAL (Chris),
  - as an implementation of moving the probe out of line so it's not
    easily confused with the central point of the test (Chris), use
    object validation library helper just introduced.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index fdd9661d..e46a4df7 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -23,6 +23,7 @@
 
 #include "igt.h"
 #include "igt_dummyload.h"
+#include "i915/gem_gtt_topology.c"
 
 IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
 
@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
-	int count, n;
+	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
 		obj[n].offset = (1ull << (i + 12)) - 4096;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
-		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = gem_gtt_validate_object(fd, &obj[n]);
+		if (err) {
+			/* Iff using a shared GTT, some of it may be reserved */
+			igt_assert_eq(err, -ENOSPC);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
 		obj[n].offset = 1ull << (i + 12);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
-		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = gem_gtt_validate_object(fd, &obj[n]);
+		if (err) {
+			/* Iff using a shared GTT, some of it may be reserved */
+			igt_assert_eq(err, -ENOSPC);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

The basic-range subtest assumes 4kB alignment while calculating softpin
offsets.  On future backends with possibly larger minimum page sizes
the test will fail as a half of calculated offsets to be tested will be
incorrectly aligned.

Replace hardcoded constants corresponding to the assumed 4kB page size
with variables initialized with actual minimum GTT alignment size and
order.

v2: Simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Reword commit message after removal of patch "Don't filter out
    addresses on full PPGTT" from the series,
  - initialize page size order with an actual minimum returned by a new
    helper (inspired by Chris).
v4: Update the helper name, use the term 'minimum GTT alignment' across
    the change, adjust variable names,
  - refresh the commit message on top of the reintroduced patch that
    fixes invalid offsets incorrectly assumed as occupied.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index e46a4df7..a2a04343 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -521,14 +521,16 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
+	int alignment_order = gem_gtt_min_alignment_order(fd);
+	uint64_t alignment = 1ull << alignment_order;
 	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
-	for (count = 12; gtt_size >> (count + 1); count++)
+	for (count = alignment_order; gtt_size >> (count + 1); count++)
 		;
 
-	count -= 12;
+	count -= alignment_order;
 
 	memset(obj, 0, sizeof(obj));
 	memset(reloc, 0, sizeof(reloc));
@@ -537,7 +539,7 @@ static void basic_range(int fd, unsigned flags)
 	n = 0;
 	for (int i = 0; i <= count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = (1ull << (i + 12)) - 4096;
+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		err = gem_gtt_validate_object(fd, &obj[n]);
@@ -558,7 +560,7 @@ static void basic_range(int fd, unsigned flags)
 	}
 	for (int i = 1; i < count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = 1ull << (i + 12);
+		obj[n].offset = 1ull << (i + alignment_order);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		err = gem_gtt_validate_object(fd, &obj[n]);
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

The basic-range subtest assumes 4kB alignment while calculating softpin
offsets.  On future backends with possibly larger minimum page sizes
the test will fail as a half of calculated offsets to be tested will be
incorrectly aligned.

Replace hardcoded constants corresponding to the assumed 4kB page size
with variables initialized with actual minimum GTT alignment size and
order.

v2: Simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Reword commit message after removal of patch "Don't filter out
    addresses on full PPGTT" from the series,
  - initialize page size order with an actual minimum returned by a new
    helper (inspired by Chris).
v4: Update the helper name, use the term 'minimum GTT alignment' across
    the change, adjust variable names,
  - refresh the commit message on top of the reintroduced patch that
    fixes invalid offsets incorrectly assumed as occupied.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index e46a4df7..a2a04343 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -521,14 +521,16 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
+	int alignment_order = gem_gtt_min_alignment_order(fd);
+	uint64_t alignment = 1ull << alignment_order;
 	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
-	for (count = 12; gtt_size >> (count + 1); count++)
+	for (count = alignment_order; gtt_size >> (count + 1); count++)
 		;
 
-	count -= 12;
+	count -= alignment_order;
 
 	memset(obj, 0, sizeof(obj));
 	memset(reloc, 0, sizeof(reloc));
@@ -537,7 +539,7 @@ static void basic_range(int fd, unsigned flags)
 	n = 0;
 	for (int i = 0; i <= count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = (1ull << (i + 12)) - 4096;
+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		err = gem_gtt_validate_object(fd, &obj[n]);
@@ -558,7 +560,7 @@ static void basic_range(int fd, unsigned flags)
 	}
 	for (int i = 1; i < count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = 1ull << (i + 12);
+		obj[n].offset = 1ull << (i + alignment_order);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		err = gem_gtt_validate_object(fd, &obj[n]);
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [igt-dev] [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

The basic-range subtest assumes 4kB alignment while calculating softpin
offsets.  On future backends with possibly larger minimum page sizes
the test will fail as a half of calculated offsets to be tested will be
incorrectly aligned.

Replace hardcoded constants corresponding to the assumed 4kB page size
with variables initialized with actual minimum GTT alignment size and
order.

v2: Simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Reword commit message after removal of patch "Don't filter out
    addresses on full PPGTT" from the series,
  - initialize page size order with an actual minimum returned by a new
    helper (inspired by Chris).
v4: Update the helper name, use the term 'minimum GTT alignment' across
    the change, adjust variable names,
  - refresh the commit message on top of the reintroduced patch that
    fixes invalid offsets incorrectly assumed as occupied.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index e46a4df7..a2a04343 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -521,14 +521,16 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
+	int alignment_order = gem_gtt_min_alignment_order(fd);
+	uint64_t alignment = 1ull << alignment_order;
 	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
-	for (count = 12; gtt_size >> (count + 1); count++)
+	for (count = alignment_order; gtt_size >> (count + 1); count++)
 		;
 
-	count -= 12;
+	count -= alignment_order;
 
 	memset(obj, 0, sizeof(obj));
 	memset(reloc, 0, sizeof(reloc));
@@ -537,7 +539,7 @@ static void basic_range(int fd, unsigned flags)
 	n = 0;
 	for (int i = 0; i <= count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = (1ull << (i + 12)) - 4096;
+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		err = gem_gtt_validate_object(fd, &obj[n]);
@@ -558,7 +560,7 @@ static void basic_range(int fd, unsigned flags)
 	}
 	for (int i = 1; i < count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = 1ull << (i + 12);
+		obj[n].offset = 1ull << (i + alignment_order);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		err = gem_gtt_validate_object(fd, &obj[n]);
-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [PATCH i-g-t v5 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

exec-shared-gtt-* subtests use hardcoded values for object size and
softpin offset, based on 4kB GTT alignment assumption.  That may result
in those subtests failing when run on future backing stores with
possibly larger minimum page sizes.

Replace hardcoded constants with values calculated from minimum GTT
alignment of actual backing store the test is running on.

v2: Update helper name, use 'minimum GTT alignment' term across the
    change, adjust variable name.
v3: Name the variable 'stride', not 'alignment', it better reflects
    its purpose (Chris).

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_ctx_shared.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
index f7852482..cb726b4d 100644
--- a/tests/i915/gem_ctx_shared.c
+++ b/tests/i915/gem_ctx_shared.c
@@ -41,6 +41,7 @@
 #include "igt_rand.h"
 #include "igt_vgem.h"
 #include "sync_file.h"
+#include "i915/gem_gtt_topology.c"
 
 #define LO 0
 #define HI 1
@@ -195,6 +196,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	uint32_t scratch, *s;
 	uint32_t batch, cs[16];
 	uint64_t offset;
+	uint64_t stride;
 	int i;
 
 	gem_require_ring(i915, ring);
@@ -203,7 +205,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
 
 	/* Find a hole big enough for both objects later */
-	scratch = gem_create(i915, 16384);
+	stride = 2 * gem_gtt_min_alignment(i915);
+	scratch = gem_create(i915, 2 * stride);
 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
 	obj.handle = scratch;
 	gem_execbuf(i915, &execbuf);
@@ -246,7 +249,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	gem_write(i915, batch, 0, cs, sizeof(cs));
 
 	obj.handle = batch;
-	obj.offset += 8192; /* make sure we don't cause an eviction! */
+	obj.offset += stride; /* make sure we don't cause an eviction! */
 	execbuf.rsvd1 = clone;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH i-g-t v5 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment
@ 2019-11-04 17:13   ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-04 17:13 UTC (permalink / raw)
  To: igt-dev; +Cc: intel-gfx

exec-shared-gtt-* subtests use hardcoded values for object size and
softpin offset, based on 4kB GTT alignment assumption.  That may result
in those subtests failing when run on future backing stores with
possibly larger minimum page sizes.

Replace hardcoded constants with values calculated from minimum GTT
alignment of actual backing store the test is running on.

v2: Update helper name, use 'minimum GTT alignment' term across the
    change, adjust variable name.
v3: Name the variable 'stride', not 'alignment', it better reflects
    its purpose (Chris).

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_ctx_shared.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
index f7852482..cb726b4d 100644
--- a/tests/i915/gem_ctx_shared.c
+++ b/tests/i915/gem_ctx_shared.c
@@ -41,6 +41,7 @@
 #include "igt_rand.h"
 #include "igt_vgem.h"
 #include "sync_file.h"
+#include "i915/gem_gtt_topology.c"
 
 #define LO 0
 #define HI 1
@@ -195,6 +196,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	uint32_t scratch, *s;
 	uint32_t batch, cs[16];
 	uint64_t offset;
+	uint64_t stride;
 	int i;
 
 	gem_require_ring(i915, ring);
@@ -203,7 +205,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
 
 	/* Find a hole big enough for both objects later */
-	scratch = gem_create(i915, 16384);
+	stride = 2 * gem_gtt_min_alignment(i915);
+	scratch = gem_create(i915, 2 * stride);
 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
 	obj.handle = scratch;
 	gem_execbuf(i915, &execbuf);
@@ -246,7 +249,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	gem_write(i915, batch, 0, cs, sizeof(cs));
 
 	obj.handle = batch;
-	obj.offset += 8192; /* make sure we don't cause an eviction! */
+	obj.offset += stride; /* make sure we don't cause an eviction! */
 	execbuf.rsvd1 = clone;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
-- 
2.21.0

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for Calculate softpin offsets from minimum GTT alignment (rev2)
  2019-11-04 17:13 ` [Intel-gfx] " Janusz Krzysztofik
                   ` (5 preceding siblings ...)
  (?)
@ 2019-11-04 19:13 ` Patchwork
  -1 siblings, 0 replies; 35+ messages in thread
From: Patchwork @ 2019-11-04 19:13 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: igt-dev

== Series Details ==

Series: Calculate softpin offsets from minimum GTT alignment (rev2)
URL   : https://patchwork.freedesktop.org/series/68831/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7257 -> IGTPW_3647
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/index.html

Known issues
------------

  Here are the changes found in IGTPW_3647 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap_gtt@basic-write-read-distinct:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-icl-u3/igt@gem_mmap_gtt@basic-write-read-distinct.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-icl-u3/igt@gem_mmap_gtt@basic-write-read-distinct.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-peppy:       [PASS][3] -> [DMESG-FAIL][4] ([fdo#112147])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-hsw-peppy/igt@i915_selftest@live_blt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-hsw-peppy/igt@i915_selftest@live_blt.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][5] -> [FAIL][6] ([fdo#111045] / [fdo#111096])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@gem_linear_blits@basic:
    - {fi-icl-dsi}:       [DMESG-WARN][7] ([fdo#106107]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-icl-dsi/igt@gem_linear_blits@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-icl-dsi/igt@gem_linear_blits@basic.html

  * igt@gem_mmap_gtt@basic-small-bo-tiledy:
    - fi-icl-u3:          [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10] +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-icl-u3/igt@gem_mmap_gtt@basic-small-bo-tiledy.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-icl-u3/igt@gem_mmap_gtt@basic-small-bo-tiledy.html

  * igt@i915_selftest@live_blt:
    - fi-bsw-n3050:       [DMESG-FAIL][11] ([fdo#112176]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-bsw-n3050/igt@i915_selftest@live_blt.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-bsw-n3050/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-bsw-nick:        [INCOMPLETE][13] ([fdo# 111542]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-bsw-nick/igt@i915_selftest@live_gem_contexts.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-bsw-nick/igt@i915_selftest@live_gem_contexts.html

  * igt@i915_selftest@live_workarounds:
    - fi-icl-u2:          [INCOMPLETE][15] ([fdo#107713]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/fi-icl-u2/igt@i915_selftest@live_workarounds.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/fi-icl-u2/igt@i915_selftest@live_workarounds.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo# 111542]: https://bugs.freedesktop.org/show_bug.cgi?id= 111542
  [fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111154]: https://bugs.freedesktop.org/show_bug.cgi?id=111154
  [fdo#111880]: https://bugs.freedesktop.org/show_bug.cgi?id=111880
  [fdo#111998]: https://bugs.freedesktop.org/show_bug.cgi?id=111998
  [fdo#112147]: https://bugs.freedesktop.org/show_bug.cgi?id=112147
  [fdo#112176]: https://bugs.freedesktop.org/show_bug.cgi?id=112176
  [fdo#112205]: https://bugs.freedesktop.org/show_bug.cgi?id=112205


Participating hosts (54 -> 44)
------------------------------

  Missing    (10): fi-ilk-m540 fi-tgl-u2 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-ctg-p8600 fi-gdg-551 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5261 -> IGTPW_3647

  CI-20190529: 20190529
  CI_DRM_7257: 6eb50a2e5d10298715b69d987200db8f5a900b5d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3647: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/index.html
  IGT_5261: 6c3bae1455c373c49fe744ea037e33b11e8daf1e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-04 20:46     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:46 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:28PM +0100, Janusz Krzysztofik wrote:
>Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
>addresses for !ppgtt") introduced filtering of addresses possibly
>occupied by other users of shared GTT.  Unfortunately, that filtering
>doesn't distinguish between actually occupied addresses and otherwise
>invalid softpin offsets.  If incorrect GTT alignment will be assumed
>when running on future backends with possibly larger minimum page
>sizes, a half of calculated offsets to be tested will be incorrectly
>detected as occupied by other users and silently skipped instead of
>reported as a problem.  That will significantly distort the intended
>test pattern.
>
>Filter out failing addresses only if reported as occupied.
>
>v2: Skip unavailable addresses only when not running on full PPGTT.
>v3: Replace the not on full PPGTT requirement for skipping with error
>    code checking.
>v4: Silently skip only those offsets which have been explicitly
>    reported as overlapping with shared GTT reserved space, not simply
>    all which raise failures other than -EINVAL (Chris),
>  - as an implementation of moving the probe out of line so it's not
>    easily confused with the central point of the test (Chris), use
>    object validation library helper just introduced.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index fdd9661d..e46a4df7 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -23,6 +23,7 @@
>
> #include "igt.h"
> #include "igt_dummyload.h"
>+#include "i915/gem_gtt_topology.c"
>
> IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
>
>@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>-	int count, n;
>+	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = (1ull << (i + 12)) - 4096;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>-		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = gem_gtt_validate_object(fd, &obj[n]);

Would it be better to name this function as
gem_gtt_validate_object_offset? From the earlier code is it was easy to
see that the intention of the test was to use gem_execbuf to map the
object to the offset. From the new method name unless I check the code
it's not straight forward that we are just checking the offset.

>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = 1ull << (i + 12);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>-		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = gem_gtt_validate_object(fd, &obj[n]);
>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>-- 

Other than the above comments, looks good to me.
Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

Vanshi

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-04 20:46     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:46 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:28PM +0100, Janusz Krzysztofik wrote:
>Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
>addresses for !ppgtt") introduced filtering of addresses possibly
>occupied by other users of shared GTT.  Unfortunately, that filtering
>doesn't distinguish between actually occupied addresses and otherwise
>invalid softpin offsets.  If incorrect GTT alignment will be assumed
>when running on future backends with possibly larger minimum page
>sizes, a half of calculated offsets to be tested will be incorrectly
>detected as occupied by other users and silently skipped instead of
>reported as a problem.  That will significantly distort the intended
>test pattern.
>
>Filter out failing addresses only if reported as occupied.
>
>v2: Skip unavailable addresses only when not running on full PPGTT.
>v3: Replace the not on full PPGTT requirement for skipping with error
>    code checking.
>v4: Silently skip only those offsets which have been explicitly
>    reported as overlapping with shared GTT reserved space, not simply
>    all which raise failures other than -EINVAL (Chris),
>  - as an implementation of moving the probe out of line so it's not
>    easily confused with the central point of the test (Chris), use
>    object validation library helper just introduced.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index fdd9661d..e46a4df7 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -23,6 +23,7 @@
>
> #include "igt.h"
> #include "igt_dummyload.h"
>+#include "i915/gem_gtt_topology.c"
>
> IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
>
>@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>-	int count, n;
>+	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = (1ull << (i + 12)) - 4096;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>-		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = gem_gtt_validate_object(fd, &obj[n]);

Would it be better to name this function as
gem_gtt_validate_object_offset? From the earlier code is it was easy to
see that the intention of the test was to use gem_execbuf to map the
object to the offset. From the new method name unless I check the code
it's not straight forward that we are just checking the offset.

>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = 1ull << (i + 12);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>-		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = gem_gtt_validate_object(fd, &obj[n]);
>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>-- 

Other than the above comments, looks good to me.
Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

Vanshi

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-04 20:46     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:46 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:28PM +0100, Janusz Krzysztofik wrote:
>Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
>addresses for !ppgtt") introduced filtering of addresses possibly
>occupied by other users of shared GTT.  Unfortunately, that filtering
>doesn't distinguish between actually occupied addresses and otherwise
>invalid softpin offsets.  If incorrect GTT alignment will be assumed
>when running on future backends with possibly larger minimum page
>sizes, a half of calculated offsets to be tested will be incorrectly
>detected as occupied by other users and silently skipped instead of
>reported as a problem.  That will significantly distort the intended
>test pattern.
>
>Filter out failing addresses only if reported as occupied.
>
>v2: Skip unavailable addresses only when not running on full PPGTT.
>v3: Replace the not on full PPGTT requirement for skipping with error
>    code checking.
>v4: Silently skip only those offsets which have been explicitly
>    reported as overlapping with shared GTT reserved space, not simply
>    all which raise failures other than -EINVAL (Chris),
>  - as an implementation of moving the probe out of line so it's not
>    easily confused with the central point of the test (Chris), use
>    object validation library helper just introduced.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index fdd9661d..e46a4df7 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -23,6 +23,7 @@
>
> #include "igt.h"
> #include "igt_dummyload.h"
>+#include "i915/gem_gtt_topology.c"
>
> IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
>
>@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>-	int count, n;
>+	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = (1ull << (i + 12)) - 4096;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>-		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = gem_gtt_validate_object(fd, &obj[n]);

Would it be better to name this function as
gem_gtt_validate_object_offset? From the earlier code is it was easy to
see that the intention of the test was to use gem_execbuf to map the
object to the offset. From the new method name unless I check the code
it's not straight forward that we are just checking the offset.

>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = 1ull << (i + 12);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>-		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = gem_gtt_validate_object(fd, &obj[n]);
>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>-- 

Other than the above comments, looks good to me.
Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

Vanshi

>2.21.0
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
@ 2019-11-04 20:48     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:48 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:29PM +0100, Janusz Krzysztofik wrote:
>The basic-range subtest assumes 4kB alignment while calculating softpin
>offsets.  On future backends with possibly larger minimum page sizes
>the test will fail as a half of calculated offsets to be tested will be
>incorrectly aligned.
>
>Replace hardcoded constants corresponding to the assumed 4kB page size
>with variables initialized with actual minimum GTT alignment size and
>order.
>
>v2: Simplify the code by reversing the size->order conversion,
>  - drop irrelevant modifications of requested object sizes.
>v3: Reword commit message after removal of patch "Don't filter out
>    addresses on full PPGTT" from the series,
>  - initialize page size order with an actual minimum returned by a new
>    helper (inspired by Chris).
>v4: Update the helper name, use the term 'minimum GTT alignment' across
>    the change, adjust variable names,
>  - refresh the commit message on top of the reintroduced patch that
>    fixes invalid offsets incorrectly assumed as occupied.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>Cc: Stuart Summers <stuart.summers@intel.com>
>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index e46a4df7..a2a04343 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -521,14 +521,16 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>+	int alignment_order = gem_gtt_min_alignment_order(fd);
>+	uint64_t alignment = 1ull << alignment_order;
> 	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>-	for (count = 12; gtt_size >> (count + 1); count++)
>+	for (count = alignment_order; gtt_size >> (count + 1); count++)
> 		;
>
>-	count -= 12;
>+	count -= alignment_order;
>
> 	memset(obj, 0, sizeof(obj));
> 	memset(reloc, 0, sizeof(reloc));
>@@ -537,7 +539,7 @@ static void basic_range(int fd, unsigned flags)
> 	n = 0;
> 	for (int i = 0; i <= count; i++) {
> 		obj[n].handle = gem_create(fd, 4096);
>-		obj[n].offset = (1ull << (i + 12)) - 4096;
>+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 		err = gem_gtt_validate_object(fd, &obj[n]);
>@@ -558,7 +560,7 @@ static void basic_range(int fd, unsigned flags)
> 	}
> 	for (int i = 1; i < count; i++) {
> 		obj[n].handle = gem_create(fd, 4096);
>-		obj[n].offset = 1ull << (i + 12);
>+		obj[n].offset = 1ull << (i + alignment_order);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 		err = gem_gtt_validate_object(fd, &obj[n]);
>-- 
>2.21.0

Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
@ 2019-11-04 20:48     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:48 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:29PM +0100, Janusz Krzysztofik wrote:
>The basic-range subtest assumes 4kB alignment while calculating softpin
>offsets.  On future backends with possibly larger minimum page sizes
>the test will fail as a half of calculated offsets to be tested will be
>incorrectly aligned.
>
>Replace hardcoded constants corresponding to the assumed 4kB page size
>with variables initialized with actual minimum GTT alignment size and
>order.
>
>v2: Simplify the code by reversing the size->order conversion,
>  - drop irrelevant modifications of requested object sizes.
>v3: Reword commit message after removal of patch "Don't filter out
>    addresses on full PPGTT" from the series,
>  - initialize page size order with an actual minimum returned by a new
>    helper (inspired by Chris).
>v4: Update the helper name, use the term 'minimum GTT alignment' across
>    the change, adjust variable names,
>  - refresh the commit message on top of the reintroduced patch that
>    fixes invalid offsets incorrectly assumed as occupied.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>Cc: Stuart Summers <stuart.summers@intel.com>
>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index e46a4df7..a2a04343 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -521,14 +521,16 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>+	int alignment_order = gem_gtt_min_alignment_order(fd);
>+	uint64_t alignment = 1ull << alignment_order;
> 	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>-	for (count = 12; gtt_size >> (count + 1); count++)
>+	for (count = alignment_order; gtt_size >> (count + 1); count++)
> 		;
>
>-	count -= 12;
>+	count -= alignment_order;
>
> 	memset(obj, 0, sizeof(obj));
> 	memset(reloc, 0, sizeof(reloc));
>@@ -537,7 +539,7 @@ static void basic_range(int fd, unsigned flags)
> 	n = 0;
> 	for (int i = 0; i <= count; i++) {
> 		obj[n].handle = gem_create(fd, 4096);
>-		obj[n].offset = (1ull << (i + 12)) - 4096;
>+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 		err = gem_gtt_validate_object(fd, &obj[n]);
>@@ -558,7 +560,7 @@ static void basic_range(int fd, unsigned flags)
> 	}
> 	for (int i = 1; i < count; i++) {
> 		obj[n].handle = gem_create(fd, 4096);
>-		obj[n].offset = 1ull << (i + 12);
>+		obj[n].offset = 1ull << (i + alignment_order);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 		err = gem_gtt_validate_object(fd, &obj[n]);
>-- 
>2.21.0

Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
@ 2019-11-04 20:48     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:48 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:29PM +0100, Janusz Krzysztofik wrote:
>The basic-range subtest assumes 4kB alignment while calculating softpin
>offsets.  On future backends with possibly larger minimum page sizes
>the test will fail as a half of calculated offsets to be tested will be
>incorrectly aligned.
>
>Replace hardcoded constants corresponding to the assumed 4kB page size
>with variables initialized with actual minimum GTT alignment size and
>order.
>
>v2: Simplify the code by reversing the size->order conversion,
>  - drop irrelevant modifications of requested object sizes.
>v3: Reword commit message after removal of patch "Don't filter out
>    addresses on full PPGTT" from the series,
>  - initialize page size order with an actual minimum returned by a new
>    helper (inspired by Chris).
>v4: Update the helper name, use the term 'minimum GTT alignment' across
>    the change, adjust variable names,
>  - refresh the commit message on top of the reintroduced patch that
>    fixes invalid offsets incorrectly assumed as occupied.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>Cc: Stuart Summers <stuart.summers@intel.com>
>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index e46a4df7..a2a04343 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -521,14 +521,16 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>+	int alignment_order = gem_gtt_min_alignment_order(fd);
>+	uint64_t alignment = 1ull << alignment_order;
> 	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>-	for (count = 12; gtt_size >> (count + 1); count++)
>+	for (count = alignment_order; gtt_size >> (count + 1); count++)
> 		;
>
>-	count -= 12;
>+	count -= alignment_order;
>
> 	memset(obj, 0, sizeof(obj));
> 	memset(reloc, 0, sizeof(reloc));
>@@ -537,7 +539,7 @@ static void basic_range(int fd, unsigned flags)
> 	n = 0;
> 	for (int i = 0; i <= count; i++) {
> 		obj[n].handle = gem_create(fd, 4096);
>-		obj[n].offset = (1ull << (i + 12)) - 4096;
>+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 		err = gem_gtt_validate_object(fd, &obj[n]);
>@@ -558,7 +560,7 @@ static void basic_range(int fd, unsigned flags)
> 	}
> 	for (int i = 1; i < count; i++) {
> 		obj[n].handle = gem_create(fd, 4096);
>-		obj[n].offset = 1ull << (i + 12);
>+		obj[n].offset = 1ull << (i + alignment_order);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 		err = gem_gtt_validate_object(fd, &obj[n]);
>-- 
>2.21.0

Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [PATCH i-g-t v5 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment
@ 2019-11-04 20:49     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:49 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:30PM +0100, Janusz Krzysztofik wrote:
>exec-shared-gtt-* subtests use hardcoded values for object size and
>softpin offset, based on 4kB GTT alignment assumption.  That may result
>in those subtests failing when run on future backing stores with
>possibly larger minimum page sizes.
>
>Replace hardcoded constants with values calculated from minimum GTT
>alignment of actual backing store the test is running on.
>
>v2: Update helper name, use 'minimum GTT alignment' term across the
>    change, adjust variable name.
>v3: Name the variable 'stride', not 'alignment', it better reflects
>    its purpose (Chris).
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_ctx_shared.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>index f7852482..cb726b4d 100644
>--- a/tests/i915/gem_ctx_shared.c
>+++ b/tests/i915/gem_ctx_shared.c
>@@ -41,6 +41,7 @@
> #include "igt_rand.h"
> #include "igt_vgem.h"
> #include "sync_file.h"
>+#include "i915/gem_gtt_topology.c"
>
> #define LO 0
> #define HI 1
>@@ -195,6 +196,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	uint32_t scratch, *s;
> 	uint32_t batch, cs[16];
> 	uint64_t offset;
>+	uint64_t stride;
> 	int i;
>
> 	gem_require_ring(i915, ring);
>@@ -203,7 +205,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
>
> 	/* Find a hole big enough for both objects later */
>-	scratch = gem_create(i915, 16384);
>+	stride = 2 * gem_gtt_min_alignment(i915);
>+	scratch = gem_create(i915, 2 * stride);
> 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> 	obj.handle = scratch;
> 	gem_execbuf(i915, &execbuf);
>@@ -246,7 +249,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	gem_write(i915, batch, 0, cs, sizeof(cs));
>
> 	obj.handle = batch;
>-	obj.offset += 8192; /* make sure we don't cause an eviction! */
>+	obj.offset += stride; /* make sure we don't cause an eviction! */
> 	execbuf.rsvd1 = clone;
> 	if (gen > 3 && gen < 6)
> 		execbuf.flags |= I915_EXEC_SECURE;
>-- 
>2.21.0

Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t v5 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment
@ 2019-11-04 20:49     ` Vanshidhar Konda
  0 siblings, 0 replies; 35+ messages in thread
From: Vanshidhar Konda @ 2019-11-04 20:49 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: intel-gfx, igt-dev

On Mon, Nov 04, 2019 at 06:13:30PM +0100, Janusz Krzysztofik wrote:
>exec-shared-gtt-* subtests use hardcoded values for object size and
>softpin offset, based on 4kB GTT alignment assumption.  That may result
>in those subtests failing when run on future backing stores with
>possibly larger minimum page sizes.
>
>Replace hardcoded constants with values calculated from minimum GTT
>alignment of actual backing store the test is running on.
>
>v2: Update helper name, use 'minimum GTT alignment' term across the
>    change, adjust variable name.
>v3: Name the variable 'stride', not 'alignment', it better reflects
>    its purpose (Chris).
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_ctx_shared.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>index f7852482..cb726b4d 100644
>--- a/tests/i915/gem_ctx_shared.c
>+++ b/tests/i915/gem_ctx_shared.c
>@@ -41,6 +41,7 @@
> #include "igt_rand.h"
> #include "igt_vgem.h"
> #include "sync_file.h"
>+#include "i915/gem_gtt_topology.c"
>
> #define LO 0
> #define HI 1
>@@ -195,6 +196,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	uint32_t scratch, *s;
> 	uint32_t batch, cs[16];
> 	uint64_t offset;
>+	uint64_t stride;
> 	int i;
>
> 	gem_require_ring(i915, ring);
>@@ -203,7 +205,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
>
> 	/* Find a hole big enough for both objects later */
>-	scratch = gem_create(i915, 16384);
>+	stride = 2 * gem_gtt_min_alignment(i915);
>+	scratch = gem_create(i915, 2 * stride);
> 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> 	obj.handle = scratch;
> 	gem_execbuf(i915, &execbuf);
>@@ -246,7 +249,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	gem_write(i915, batch, 0, cs, sizeof(cs));
>
> 	obj.handle = batch;
>-	obj.offset += 8192; /* make sure we don't cause an eviction! */
>+	obj.offset += stride; /* make sure we don't cause an eviction! */
> 	execbuf.rsvd1 = clone;
> 	if (gen > 3 && gen < 6)
> 		execbuf.flags |= I915_EXEC_SECURE;
>-- 
>2.21.0

Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for Calculate softpin offsets from minimum GTT alignment (rev2)
  2019-11-04 17:13 ` [Intel-gfx] " Janusz Krzysztofik
                   ` (6 preceding siblings ...)
  (?)
@ 2019-11-05  4:23 ` Patchwork
  -1 siblings, 0 replies; 35+ messages in thread
From: Patchwork @ 2019-11-05  4:23 UTC (permalink / raw)
  To: Janusz Krzysztofik; +Cc: igt-dev

== Series Details ==

Series: Calculate softpin offsets from minimum GTT alignment (rev2)
URL   : https://patchwork.freedesktop.org/series/68831/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7257_full -> IGTPW_3647_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_3647_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@gem_ctx_persistence@smoketest}:
    - shard-glk:          [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-glk6/igt@gem_ctx_persistence@smoketest.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-glk3/igt@gem_ctx_persistence@smoketest.html
    - shard-iclb:         [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb3/igt@gem_ctx_persistence@smoketest.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb8/igt@gem_ctx_persistence@smoketest.html

  
Known issues
------------

  Here are the changes found in IGTPW_3647_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@vcs1-none:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#109276] / [fdo#112080]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb4/igt@gem_ctx_isolation@vcs1-none.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb7/igt@gem_ctx_isolation@vcs1-none.html

  * igt@gem_exec_parallel@vcs1-fds:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#112080]) +12 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb1/igt@gem_exec_parallel@vcs1-fds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb3/igt@gem_exec_parallel@vcs1-fds.html

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [PASS][9] -> [SKIP][10] ([fdo#112146]) +3 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb5/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb4/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@gem_persistent_relocs@forked-thrashing:
    - shard-snb:          [PASS][11] -> [FAIL][12] ([fdo#112037])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-snb1/igt@gem_persistent_relocs@forked-thrashing.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-snb6/igt@gem_persistent_relocs@forked-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy:
    - shard-snb:          [PASS][13] -> [DMESG-WARN][14] ([fdo#111870]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-snb2/igt@gem_userptr_blits@map-fixed-invalidate-busy.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-hsw:          [PASS][15] -> [DMESG-WARN][16] ([fdo#111870])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-hsw8/igt@gem_userptr_blits@sync-unmap-cycles.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-hsw1/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          [PASS][17] -> [SKIP][18] ([fdo#109271])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-snb4/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-snb1/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [PASS][19] -> [FAIL][20] ([fdo#105363])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-glk8/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-glk3/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [PASS][21] -> [DMESG-WARN][22] ([fdo#108566])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-apl4/igt@kms_flip@flip-vs-suspend-interruptible.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [PASS][23] -> [FAIL][24] ([fdo#103167]) +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
    - shard-kbl:          [PASS][25] -> [FAIL][26] ([fdo#103167])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
    - shard-apl:          [PASS][27] -> [FAIL][28] ([fdo#103167])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-apl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-apl6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [PASS][29] -> [SKIP][30] ([fdo#109642] / [fdo#111068])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb8/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [PASS][31] -> [SKIP][32] ([fdo#109441]) +2 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb7/igt@kms_psr@psr2_cursor_blt.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [PASS][33] -> [DMESG-WARN][34] ([fdo#108566]) +6 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [PASS][35] -> [SKIP][36] ([fdo#109276]) +17 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb4/igt@prime_vgem@fence-wait-bsd2.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb6/igt@prime_vgem@fence-wait-bsd2.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [DMESG-WARN][37] ([fdo#108566]) -> [PASS][38] +8 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl6/igt@gem_ctx_isolation@rcs0-s3.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl6/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_ctx_isolation@vcs1-dirty-create:
    - shard-iclb:         [SKIP][39] ([fdo#109276] / [fdo#112080]) -> [PASS][40] +3 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb3/igt@gem_ctx_isolation@vcs1-dirty-create.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb4/igt@gem_ctx_isolation@vcs1-dirty-create.html

  * igt@gem_ctx_isolation@vcs1-s3:
    - {shard-tglb}:       [INCOMPLETE][41] ([fdo#111832]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-tglb4/igt@gem_ctx_isolation@vcs1-s3.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-tglb7/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@gem_ctx_shared@q-smoketest-bsd1:
    - {shard-tglb}:       [INCOMPLETE][43] ([fdo#111735]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-tglb6/igt@gem_ctx_shared@q-smoketest-bsd1.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-tglb5/igt@gem_ctx_shared@q-smoketest-bsd1.html

  * igt@gem_ctx_switch@vcs1-heavy:
    - shard-iclb:         [SKIP][45] ([fdo#112080]) -> [PASS][46] +5 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb5/igt@gem_ctx_switch@vcs1-heavy.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb1/igt@gem_ctx_switch@vcs1-heavy.html

  * igt@gem_eio@suspend:
    - {shard-tglb}:       [INCOMPLETE][47] ([fdo#111850]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-tglb2/igt@gem_eio@suspend.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-tglb4/igt@gem_eio@suspend.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][49] ([fdo#110854]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb6/igt@gem_exec_balancer@smoke.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb2/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_schedule@preemptive-hang-bsd:
    - shard-iclb:         [SKIP][51] ([fdo#112146]) -> [PASS][52] +8 similar issues
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb2/igt@gem_exec_schedule@preemptive-hang-bsd.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb6/igt@gem_exec_schedule@preemptive-hang-bsd.html

  * igt@gem_persistent_relocs@forked-faulting-reloc-thrashing:
    - {shard-tglb}:       [INCOMPLETE][53] ([fdo#112068 ]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-tglb8/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-tglb2/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
    - shard-hsw:          [TIMEOUT][55] ([fdo#112068 ]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-hsw1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-hsw1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-thrashing:
    - shard-kbl:          [TIMEOUT][57] ([fdo#112068 ]) -> [PASS][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl3/igt@gem_persistent_relocs@forked-thrashing.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl2/igt@gem_persistent_relocs@forked-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
    - shard-hsw:          [DMESG-WARN][59] ([fdo#111870]) -> [PASS][60] +1 similar issue
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-hsw2/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-hsw2/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
    - shard-snb:          [DMESG-WARN][61] ([fdo#111870]) -> [PASS][62] +2 similar issues
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-snb2/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html

  * {igt@i915_pm_dc@dc5-dpms}:
    - shard-iclb:         [FAIL][63] ([fdo#111795 ]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb3/igt@i915_pm_dc@dc5-dpms.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb4/igt@i915_pm_dc@dc5-dpms.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - {shard-tglb}:       [INCOMPLETE][65] ([fdo#111832] / [fdo#111850]) -> [PASS][66] +1 similar issue
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-tglb3/igt@i915_suspend@fence-restore-tiled2untiled.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-tglb1/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [INCOMPLETE][67] ([fdo#103665]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl3/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl3/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-apl:          [DMESG-WARN][69] ([fdo#108566]) -> [PASS][70] +2 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-apl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-hsw:          [INCOMPLETE][71] ([fdo#103540]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-hsw5/igt@kms_flip@flip-vs-suspend.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-hsw2/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-blt:
    - shard-iclb:         [FAIL][73] ([fdo#103167]) -> [PASS][74] +8 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-blt.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-blt.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][75] ([fdo#109642] / [fdo#111068]) -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb4/igt@kms_psr2_su@page_flip.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_cursor_mmap_gtt:
    - shard-iclb:         [SKIP][77] ([fdo#109441]) -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb7/igt@kms_psr@psr2_cursor_mmap_gtt.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_gtt.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][79] ([fdo#99912]) -> [PASS][80]
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-apl7/igt@kms_setmode@basic.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-apl2/igt@kms_setmode@basic.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][81] ([fdo#109276]) -> [PASS][82] +12 similar issues
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb6/igt@prime_busy@hang-bsd2.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb1/igt@prime_busy@hang-bsd2.html

  * igt@prime_vgem@sync-bsd2:
    - {shard-tglb}:       [INCOMPLETE][83] -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-tglb6/igt@prime_vgem@sync-bsd2.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-tglb8/igt@prime_vgem@sync-bsd2.html

  
#### Warnings ####

  * igt@gem_mocs_settings@mocs-reset-bsd2:
    - shard-iclb:         [FAIL][85] ([fdo#111330]) -> [SKIP][86] ([fdo#109276]) +1 similar issue
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb2/igt@gem_mocs_settings@mocs-reset-bsd2.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb6/igt@gem_mocs_settings@mocs-reset-bsd2.html

  * igt@gem_mocs_settings@mocs-settings-bsd2:
    - shard-iclb:         [SKIP][87] ([fdo#109276]) -> [FAIL][88] ([fdo#111330]) +1 similar issue
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb3/igt@gem_mocs_settings@mocs-settings-bsd2.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb4/igt@gem_mocs_settings@mocs-settings-bsd2.html

  * igt@gem_softpin@noreloc-s3:
    - shard-kbl:          [DMESG-WARN][89] ([fdo#108566]) -> [DMESG-WARN][90] ([fdo#103313] / [fdo#108566])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl7/igt@gem_softpin@noreloc-s3.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl6/igt@gem_softpin@noreloc-s3.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
    - shard-snb:          [DMESG-WARN][91] ([fdo#111870]) -> [DMESG-WARN][92] ([fdo#110789] / [fdo#111870])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-snb4/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-snb6/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][93] ([fdo#108566]) -> [FAIL][94] ([fdo#103375])
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-kbl4/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [DMESG-WARN][95] ([fdo#107724]) -> [SKIP][96] ([fdo#109441])
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7257/shard-iclb2/igt@kms_psr@psr2_suspend.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/shard-iclb1/igt@kms_psr@psr2_suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103313]: https://bugs.freedesktop.org/show_bug.cgi?id=103313
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110789]: https://bugs.freedesktop.org/show_bug.cgi?id=110789
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#111593]: https://bugs.freedesktop.org/show_bug.cgi?id=111593
  [fdo#111646]: https://bugs.freedesktop.org/show_bug.cgi?id=111646
  [fdo#111671]: https://bugs.freedesktop.org/show_bug.cgi?id=111671
  [fdo#111703]: https://bugs.freedesktop.org/show_bug.cgi?id=111703
  [fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735
  [fdo#111795 ]: https://bugs.freedesktop.org/show_bug.cgi?id=111795 
  [fdo#111832]: https://bugs.freedesktop.org/show_bug.cgi?id=111832
  [fdo#111850]: https://bugs.freedesktop.org/show_bug.cgi?id=111850
  [fdo#111855]: https://bugs.freedesktop.org/show_bug.cgi?id=111855
  [fdo#111866]: https://bugs.freedesktop.org/show_bug.cgi?id=111866
  [fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870
  [fdo#111884]: https://bugs.freedesktop.org/show_bug.cg

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3647/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-05  9:14     ` Joonas Lahtinen
  0 siblings, 0 replies; 35+ messages in thread
From: Joonas Lahtinen @ 2019-11-05  9:14 UTC (permalink / raw)
  To: Janusz Krzysztofik, igt-dev; +Cc: intel-gfx

Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> Some tests assume 4kB offset alignment while using softpin.  That
> assumption may be wrong on future GEM backends with possibly larger
> minimum page sizes.  As a result, those tests may either fail on
> softpin at offsets which are incorrectly aligned, may silently skip
> such incorrectly aligned addresses assuming them occupied by other
> users, or may always succeed when examining invalid use patterns.
> 
> Provide a helper function that detects minimum GTT alignment.  Tests
> may use it to calculate softpin offsets valid on actually used backing
> store.
> 
> Also expose a new object validation helper just created, it may be
> useful for checking if a shared GTT address is not reserved, for
> example.
> 
> v2: Rename the helper, use 'minimum GTT alignment' term across the
>     change (Chris),
>   - use error numbers to distinguish between invalid offsets and
>     addresses occupied by other users,
>   - simplify the code (Chris).
> v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
>   - validate objects with an invalid reloc applied so execbuf requests
>     called only for validation purposes are actually not emitted to GPU
>     (Chris),
>   - move object validation code to a separate helper.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>

<SNIP>

> +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> +{
> +       struct drm_i915_gem_relocation_entry reloc;
> +       struct drm_i915_gem_execbuffer2 execbuf;
> +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> +       uintptr_t old_reloc;
> +       int old_count, err;
> +
> +       memset(&reloc, 0, sizeof(reloc));
> +       memset(&execbuf, 0, sizeof(execbuf));
> +
> +       /* use invalid reloc to save request emission */
> +       old_reloc = obj->relocs_ptr;
> +       old_count = obj->relocation_count;
> +       obj->relocs_ptr = to_user_pointer(&reloc);
> +       obj->relocation_count = 1;
> +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));

Don't use relocations for anything new. Also, don't depend on such
quirk behavior as to kernel validating parameters in certain order.

> +int gem_gtt_min_alignment_order(int fd)
> +{
> +       struct drm_i915_gem_exec_object2 obj;
> +       int order;
> +
> +       /* no softpin => 4kB page size */
> +       if (!gem_has_softpin(fd))
> +               return 12;
> +
> +       memset(&obj, 0, sizeof(obj));
> +
> +       obj.handle = gem_create(fd, 4096);
> +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;

Default will be system memory, so it's unclear what this would establish.

Just use PCI ID to detect this. It's not going to be dynamic within a
device. This just adds extra startup cost for detecting something that
is not dynamic.

> +/*
> + * Copyright © 2019 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>
> + *    Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> + */

Please don't blindly copy "Authors:" around in the code.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-05  9:14     ` Joonas Lahtinen
  0 siblings, 0 replies; 35+ messages in thread
From: Joonas Lahtinen @ 2019-11-05  9:14 UTC (permalink / raw)
  To: Janusz Krzysztofik, igt-dev; +Cc: intel-gfx

Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> Some tests assume 4kB offset alignment while using softpin.  That
> assumption may be wrong on future GEM backends with possibly larger
> minimum page sizes.  As a result, those tests may either fail on
> softpin at offsets which are incorrectly aligned, may silently skip
> such incorrectly aligned addresses assuming them occupied by other
> users, or may always succeed when examining invalid use patterns.
> 
> Provide a helper function that detects minimum GTT alignment.  Tests
> may use it to calculate softpin offsets valid on actually used backing
> store.
> 
> Also expose a new object validation helper just created, it may be
> useful for checking if a shared GTT address is not reserved, for
> example.
> 
> v2: Rename the helper, use 'minimum GTT alignment' term across the
>     change (Chris),
>   - use error numbers to distinguish between invalid offsets and
>     addresses occupied by other users,
>   - simplify the code (Chris).
> v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
>   - validate objects with an invalid reloc applied so execbuf requests
>     called only for validation purposes are actually not emitted to GPU
>     (Chris),
>   - move object validation code to a separate helper.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>

<SNIP>

> +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> +{
> +       struct drm_i915_gem_relocation_entry reloc;
> +       struct drm_i915_gem_execbuffer2 execbuf;
> +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> +       uintptr_t old_reloc;
> +       int old_count, err;
> +
> +       memset(&reloc, 0, sizeof(reloc));
> +       memset(&execbuf, 0, sizeof(execbuf));
> +
> +       /* use invalid reloc to save request emission */
> +       old_reloc = obj->relocs_ptr;
> +       old_count = obj->relocation_count;
> +       obj->relocs_ptr = to_user_pointer(&reloc);
> +       obj->relocation_count = 1;
> +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));

Don't use relocations for anything new. Also, don't depend on such
quirk behavior as to kernel validating parameters in certain order.

> +int gem_gtt_min_alignment_order(int fd)
> +{
> +       struct drm_i915_gem_exec_object2 obj;
> +       int order;
> +
> +       /* no softpin => 4kB page size */
> +       if (!gem_has_softpin(fd))
> +               return 12;
> +
> +       memset(&obj, 0, sizeof(obj));
> +
> +       obj.handle = gem_create(fd, 4096);
> +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;

Default will be system memory, so it's unclear what this would establish.

Just use PCI ID to detect this. It's not going to be dynamic within a
device. This just adds extra startup cost for detecting something that
is not dynamic.

> +/*
> + * Copyright © 2019 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>
> + *    Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> + */

Please don't blindly copy "Authors:" around in the code.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-05 15:49       ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-05 15:49 UTC (permalink / raw)
  To: Vanshidhar Konda; +Cc: intel-gfx, igt-dev

Hi,

On Monday, November 4, 2019 9:46:28 PM CET Vanshidhar Konda wrote:
> On Mon, Nov 04, 2019 at 06:13:28PM +0100, Janusz Krzysztofik wrote:
> >Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> >addresses for !ppgtt") introduced filtering of addresses possibly
> >occupied by other users of shared GTT.  Unfortunately, that filtering
> >doesn't distinguish between actually occupied addresses and otherwise
> >invalid softpin offsets.  If incorrect GTT alignment will be assumed
> >when running on future backends with possibly larger minimum page
> >sizes, a half of calculated offsets to be tested will be incorrectly
> >detected as occupied by other users and silently skipped instead of
> >reported as a problem.  That will significantly distort the intended
> >test pattern.
> >
> >Filter out failing addresses only if reported as occupied.
> >
> >v2: Skip unavailable addresses only when not running on full PPGTT.
> >v3: Replace the not on full PPGTT requirement for skipping with error
> >    code checking.
> >v4: Silently skip only those offsets which have been explicitly
> >    reported as overlapping with shared GTT reserved space, not simply
> >    all which raise failures other than -EINVAL (Chris),
> >  - as an implementation of moving the probe out of line so it's not
> >    easily confused with the central point of the test (Chris), use
> >    object validation library helper just introduced.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> >index fdd9661d..e46a4df7 100644
> >--- a/tests/i915/gem_exec_reloc.c
> >+++ b/tests/i915/gem_exec_reloc.c
> >@@ -23,6 +23,7 @@
> >
> > #include "igt.h"
> > #include "igt_dummyload.h"
> >+#include "i915/gem_gtt_topology.c"

Auto correction: s/.c/.h/

> >
> > IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
> >
> >@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
> > 	uint64_t gtt_size = gem_aperture_size(fd);
> > 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > 	igt_spin_t *spin = NULL;
> >-	int count, n;
> >+	int count, n, err;
> >
> > 	igt_require(gem_has_softpin(fd));
> >
> >@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> > 		obj[n].offset = (1ull << (i + 12)) - 4096;
> > 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> > 		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >-		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = gem_gtt_validate_object(fd, &obj[n]);
> 
> Would it be better to name this function as
> gem_gtt_validate_object_offset? From the earlier code is it was easy to
> see that the intention of the test was to use gem_execbuf to map the
> object to the offset. From the new method name unless I check the code
> it's not straight forward that we are just checking the offset.
> 
> >+		if (err) {
> >+			/* Iff using a shared GTT, some of it may 
be reserved */
> 
> Nit-pick: If, not Iff

Hmm, I've assumed Chris' English is better than mine and I'm using his 
spelling from one of his comments.  AFAICT, 'iff' is an abbreviation of 'if 
and only if' used in a math slang.

Thanks,
Janusz


> >+			igt_assert_eq(err, -ENOSPC);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long 
long)obj[n].offset);
> >@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> > 		obj[n].offset = 1ull << (i + 12);
> > 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> > 		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >-		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = gem_gtt_validate_object(fd, &obj[n]);
> >+		if (err) {
> >+			/* Iff using a shared GTT, some of it may 
be reserved */
> 
> Nit-pick: If, not Iff
> 
> >+			igt_assert_eq(err, -ENOSPC);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long 
long)obj[n].offset);
> 
> Other than the above comments, looks good to me.
> Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>
> 
> Vanshi
> 
> >2.21.0
> >
> 




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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-05 15:49       ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-05 15:49 UTC (permalink / raw)
  To: Vanshidhar Konda; +Cc: intel-gfx, igt-dev

Hi,

On Monday, November 4, 2019 9:46:28 PM CET Vanshidhar Konda wrote:
> On Mon, Nov 04, 2019 at 06:13:28PM +0100, Janusz Krzysztofik wrote:
> >Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> >addresses for !ppgtt") introduced filtering of addresses possibly
> >occupied by other users of shared GTT.  Unfortunately, that filtering
> >doesn't distinguish between actually occupied addresses and otherwise
> >invalid softpin offsets.  If incorrect GTT alignment will be assumed
> >when running on future backends with possibly larger minimum page
> >sizes, a half of calculated offsets to be tested will be incorrectly
> >detected as occupied by other users and silently skipped instead of
> >reported as a problem.  That will significantly distort the intended
> >test pattern.
> >
> >Filter out failing addresses only if reported as occupied.
> >
> >v2: Skip unavailable addresses only when not running on full PPGTT.
> >v3: Replace the not on full PPGTT requirement for skipping with error
> >    code checking.
> >v4: Silently skip only those offsets which have been explicitly
> >    reported as overlapping with shared GTT reserved space, not simply
> >    all which raise failures other than -EINVAL (Chris),
> >  - as an implementation of moving the probe out of line so it's not
> >    easily confused with the central point of the test (Chris), use
> >    object validation library helper just introduced.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> >index fdd9661d..e46a4df7 100644
> >--- a/tests/i915/gem_exec_reloc.c
> >+++ b/tests/i915/gem_exec_reloc.c
> >@@ -23,6 +23,7 @@
> >
> > #include "igt.h"
> > #include "igt_dummyload.h"
> >+#include "i915/gem_gtt_topology.c"

Auto correction: s/.c/.h/

> >
> > IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
> >
> >@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
> > 	uint64_t gtt_size = gem_aperture_size(fd);
> > 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > 	igt_spin_t *spin = NULL;
> >-	int count, n;
> >+	int count, n, err;
> >
> > 	igt_require(gem_has_softpin(fd));
> >
> >@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> > 		obj[n].offset = (1ull << (i + 12)) - 4096;
> > 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> > 		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >-		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = gem_gtt_validate_object(fd, &obj[n]);
> 
> Would it be better to name this function as
> gem_gtt_validate_object_offset? From the earlier code is it was easy to
> see that the intention of the test was to use gem_execbuf to map the
> object to the offset. From the new method name unless I check the code
> it's not straight forward that we are just checking the offset.
> 
> >+		if (err) {
> >+			/* Iff using a shared GTT, some of it may 
be reserved */
> 
> Nit-pick: If, not Iff

Hmm, I've assumed Chris' English is better than mine and I'm using his 
spelling from one of his comments.  AFAICT, 'iff' is an abbreviation of 'if 
and only if' used in a math slang.

Thanks,
Janusz


> >+			igt_assert_eq(err, -ENOSPC);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long 
long)obj[n].offset);
> >@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> > 		obj[n].offset = 1ull << (i + 12);
> > 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> > 		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >-		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = gem_gtt_validate_object(fd, &obj[n]);
> >+		if (err) {
> >+			/* Iff using a shared GTT, some of it may 
be reserved */
> 
> Nit-pick: If, not Iff
> 
> >+			igt_assert_eq(err, -ENOSPC);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long 
long)obj[n].offset);
> 
> Other than the above comments, looks good to me.
> Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>
> 
> Vanshi
> 
> >2.21.0
> >
> 




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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses
@ 2019-11-05 15:49       ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-05 15:49 UTC (permalink / raw)
  To: Vanshidhar Konda; +Cc: intel-gfx, igt-dev

Hi,

On Monday, November 4, 2019 9:46:28 PM CET Vanshidhar Konda wrote:
> On Mon, Nov 04, 2019 at 06:13:28PM +0100, Janusz Krzysztofik wrote:
> >Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> >addresses for !ppgtt") introduced filtering of addresses possibly
> >occupied by other users of shared GTT.  Unfortunately, that filtering
> >doesn't distinguish between actually occupied addresses and otherwise
> >invalid softpin offsets.  If incorrect GTT alignment will be assumed
> >when running on future backends with possibly larger minimum page
> >sizes, a half of calculated offsets to be tested will be incorrectly
> >detected as occupied by other users and silently skipped instead of
> >reported as a problem.  That will significantly distort the intended
> >test pattern.
> >
> >Filter out failing addresses only if reported as occupied.
> >
> >v2: Skip unavailable addresses only when not running on full PPGTT.
> >v3: Replace the not on full PPGTT requirement for skipping with error
> >    code checking.
> >v4: Silently skip only those offsets which have been explicitly
> >    reported as overlapping with shared GTT reserved space, not simply
> >    all which raise failures other than -EINVAL (Chris),
> >  - as an implementation of moving the probe out of line so it's not
> >    easily confused with the central point of the test (Chris), use
> >    object validation library helper just introduced.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > tests/i915/gem_exec_reloc.c | 21 ++++++++++++---------
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> >index fdd9661d..e46a4df7 100644
> >--- a/tests/i915/gem_exec_reloc.c
> >+++ b/tests/i915/gem_exec_reloc.c
> >@@ -23,6 +23,7 @@
> >
> > #include "igt.h"
> > #include "igt_dummyload.h"
> >+#include "i915/gem_gtt_topology.c"

Auto correction: s/.c/.h/

> >
> > IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
> >
> >@@ -520,7 +521,7 @@ static void basic_range(int fd, unsigned flags)
> > 	uint64_t gtt_size = gem_aperture_size(fd);
> > 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > 	igt_spin_t *spin = NULL;
> >-	int count, n;
> >+	int count, n, err;
> >
> > 	igt_require(gem_has_softpin(fd));
> >
> >@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> > 		obj[n].offset = (1ull << (i + 12)) - 4096;
> > 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> > 		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >-		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = gem_gtt_validate_object(fd, &obj[n]);
> 
> Would it be better to name this function as
> gem_gtt_validate_object_offset? From the earlier code is it was easy to
> see that the intention of the test was to use gem_execbuf to map the
> object to the offset. From the new method name unless I check the code
> it's not straight forward that we are just checking the offset.
> 
> >+		if (err) {
> >+			/* Iff using a shared GTT, some of it may 
be reserved */
> 
> Nit-pick: If, not Iff

Hmm, I've assumed Chris' English is better than mine and I'm using his 
spelling from one of his comments.  AFAICT, 'iff' is an abbreviation of 'if 
and only if' used in a math slang.

Thanks,
Janusz


> >+			igt_assert_eq(err, -ENOSPC);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long 
long)obj[n].offset);
> >@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> > 		obj[n].offset = 1ull << (i + 12);
> > 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> > 		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >-		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >-		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >-		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = gem_gtt_validate_object(fd, &obj[n]);
> >+		if (err) {
> >+			/* Iff using a shared GTT, some of it may 
be reserved */
> 
> Nit-pick: If, not Iff
> 
> >+			igt_assert_eq(err, -ENOSPC);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long 
long)obj[n].offset);
> 
> Other than the above comments, looks good to me.
> Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>
> 
> Vanshi
> 
> >2.21.0
> >
> 




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-06  8:18       ` Chris Wilson
  0 siblings, 0 replies; 35+ messages in thread
From: Chris Wilson @ 2019-11-06  8:18 UTC (permalink / raw)
  To: Janusz Krzysztofik, Joonas Lahtinen, igt-dev; +Cc: intel-gfx

Quoting Joonas Lahtinen (2019-11-05 09:14:20)
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.

Relocations cannot happen before reservation. Even if you were to do the
entire thing async, you still have to copy the relocation arrays from
userspace. (Note touching the relocation array would be a noticeable
performance regression.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-06  8:18       ` Chris Wilson
  0 siblings, 0 replies; 35+ messages in thread
From: Chris Wilson @ 2019-11-06  8:18 UTC (permalink / raw)
  To: Janusz Krzysztofik, Joonas Lahtinen, igt-dev; +Cc: intel-gfx

Quoting Joonas Lahtinen (2019-11-05 09:14:20)
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.

Relocations cannot happen before reservation. Even if you were to do the
entire thing async, you still have to copy the relocation arrays from
userspace. (Note touching the relocation array would be a noticeable
performance regression.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-06  8:18       ` Chris Wilson
  0 siblings, 0 replies; 35+ messages in thread
From: Chris Wilson @ 2019-11-06  8:18 UTC (permalink / raw)
  To: Janusz Krzysztofik, Joonas Lahtinen, igt-dev; +Cc: intel-gfx

Quoting Joonas Lahtinen (2019-11-05 09:14:20)
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.

Relocations cannot happen before reservation. Even if you were to do the
entire thing async, you still have to copy the relocation arrays from
userspace. (Note touching the relocation array would be a noticeable
performance regression.)
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-08 16:51       ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-08 16:51 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: igt-dev, intel-gfx

Hi Jonas,

On Tuesday, November 5, 2019 10:14:20 AM CET Joonas Lahtinen wrote:
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.
> 
> > +int gem_gtt_min_alignment_order(int fd)
> > +{
> > +       struct drm_i915_gem_exec_object2 obj;
> > +       int order;
> > +
> > +       /* no softpin => 4kB page size */
> > +       if (!gem_has_softpin(fd))
> > +               return 12;
> > +
> > +       memset(&obj, 0, sizeof(obj));
> > +
> > +       obj.handle = gem_create(fd, 4096);
> > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 
> Default will be system memory, so it's unclear what this would establish.
> 
> Just use PCI ID to detect this. 

I'm not sure what I'm supposed to detect if default will be system memory.  Do we expect 
different, device dependent minimum GTT alignments in system memory?

Thanks,
Janusz


> It's not going to be dynamic within a
> device. This just adds extra startup cost for detecting something that
> is not dynamic.
> 
> > +/*
> > + * Copyright © 2019 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>
> > + *    Daniel Vetter <daniel.vetter@ffwll.ch>
> > + *
> > + */
> 
> Please don't blindly copy "Authors:" around in the code.
> 
> Regards, Joonas
> 




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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-08 16:51       ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-08 16:51 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: igt-dev, intel-gfx

Hi Jonas,

On Tuesday, November 5, 2019 10:14:20 AM CET Joonas Lahtinen wrote:
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.
> 
> > +int gem_gtt_min_alignment_order(int fd)
> > +{
> > +       struct drm_i915_gem_exec_object2 obj;
> > +       int order;
> > +
> > +       /* no softpin => 4kB page size */
> > +       if (!gem_has_softpin(fd))
> > +               return 12;
> > +
> > +       memset(&obj, 0, sizeof(obj));
> > +
> > +       obj.handle = gem_create(fd, 4096);
> > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 
> Default will be system memory, so it's unclear what this would establish.
> 
> Just use PCI ID to detect this. 

I'm not sure what I'm supposed to detect if default will be system memory.  Do we expect 
different, device dependent minimum GTT alignments in system memory?

Thanks,
Janusz


> It's not going to be dynamic within a
> device. This just adds extra startup cost for detecting something that
> is not dynamic.
> 
> > +/*
> > + * Copyright © 2019 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>
> > + *    Daniel Vetter <daniel.vetter@ffwll.ch>
> > + *
> > + */
> 
> Please don't blindly copy "Authors:" around in the code.
> 
> Regards, Joonas
> 




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

^ permalink raw reply	[flat|nested] 35+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper
@ 2019-11-08 16:51       ` Janusz Krzysztofik
  0 siblings, 0 replies; 35+ messages in thread
From: Janusz Krzysztofik @ 2019-11-08 16:51 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: igt-dev, intel-gfx

Hi Jonas,

On Tuesday, November 5, 2019 10:14:20 AM CET Joonas Lahtinen wrote:
> Quoting Janusz Krzysztofik (2019-11-04 19:13:27)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users, or may always succeed when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid on actually used backing
> > store.
> > 
> > Also expose a new object validation helper just created, it may be
> > useful for checking if a shared GTT address is not reserved, for
> > example.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users,
> >   - simplify the code (Chris).
> > v3: Put the code under lib/i915/, not in lib/ioctl_wrappers.c (Chris),
> >   - validate objects with an invalid reloc applied so execbuf requests
> >     called only for validation purposes are actually not emitted to GPU
> >     (Chris),
> >   - move object validation code to a separate helper.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> 
> <SNIP>
> 
> > +int gem_gtt_validate_object(int fd, struct drm_i915_gem_exec_object2 *obj)
> > +{
> > +       struct drm_i915_gem_relocation_entry reloc;
> > +       struct drm_i915_gem_execbuffer2 execbuf;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       uintptr_t old_reloc;
> > +       int old_count, err;
> > +
> > +       memset(&reloc, 0, sizeof(reloc));
> > +       memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +       /* use invalid reloc to save request emission */
> > +       old_reloc = obj->relocs_ptr;
> > +       old_count = obj->relocation_count;
> > +       obj->relocs_ptr = to_user_pointer(&reloc);
> > +       obj->relocation_count = 1;
> > +       gem_write(fd, obj->handle, 0, &bbe, sizeof(bbe));
> 
> Don't use relocations for anything new. Also, don't depend on such
> quirk behavior as to kernel validating parameters in certain order.
> 
> > +int gem_gtt_min_alignment_order(int fd)
> > +{
> > +       struct drm_i915_gem_exec_object2 obj;
> > +       int order;
> > +
> > +       /* no softpin => 4kB page size */
> > +       if (!gem_has_softpin(fd))
> > +               return 12;
> > +
> > +       memset(&obj, 0, sizeof(obj));
> > +
> > +       obj.handle = gem_create(fd, 4096);
> > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> 
> Default will be system memory, so it's unclear what this would establish.
> 
> Just use PCI ID to detect this. 

I'm not sure what I'm supposed to detect if default will be system memory.  Do we expect 
different, device dependent minimum GTT alignments in system memory?

Thanks,
Janusz


> It's not going to be dynamic within a
> device. This just adds extra startup cost for detecting something that
> is not dynamic.
> 
> > +/*
> > + * Copyright © 2019 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>
> > + *    Daniel Vetter <daniel.vetter@ffwll.ch>
> > + *
> > + */
> 
> Please don't blindly copy "Authors:" around in the code.
> 
> Regards, Joonas
> 




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-11-08 16:51 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04 17:13 [PATCH i-g-t v5 0/4] Calculate softpin offsets from minimum GTT alignment Janusz Krzysztofik
2019-11-04 17:13 ` [igt-dev] " Janusz Krzysztofik
2019-11-04 17:13 ` [Intel-gfx] " Janusz Krzysztofik
2019-11-04 17:13 ` [PATCH i-g-t v5 1/4] lib/i915: Add minimum GTT alignment helper Janusz Krzysztofik
2019-11-04 17:13   ` [igt-dev] " Janusz Krzysztofik
2019-11-04 17:13   ` [Intel-gfx] " Janusz Krzysztofik
2019-11-05  9:14   ` [igt-dev] " Joonas Lahtinen
2019-11-05  9:14     ` [Intel-gfx] " Joonas Lahtinen
2019-11-06  8:18     ` Chris Wilson
2019-11-06  8:18       ` [igt-dev] [Intel-gfx] " Chris Wilson
2019-11-06  8:18       ` [Intel-gfx] [igt-dev] " Chris Wilson
2019-11-08 16:51     ` Janusz Krzysztofik
2019-11-08 16:51       ` Janusz Krzysztofik
2019-11-08 16:51       ` [Intel-gfx] " Janusz Krzysztofik
2019-11-04 17:13 ` [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses Janusz Krzysztofik
2019-11-04 17:13   ` [igt-dev] " Janusz Krzysztofik
2019-11-04 17:13   ` [Intel-gfx] " Janusz Krzysztofik
2019-11-04 20:46   ` Vanshidhar Konda
2019-11-04 20:46     ` [igt-dev] " Vanshidhar Konda
2019-11-04 20:46     ` [Intel-gfx] " Vanshidhar Konda
2019-11-05 15:49     ` Janusz Krzysztofik
2019-11-05 15:49       ` [igt-dev] " Janusz Krzysztofik
2019-11-05 15:49       ` [Intel-gfx] " Janusz Krzysztofik
2019-11-04 17:13 ` [PATCH i-g-t v5 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment Janusz Krzysztofik
2019-11-04 17:13   ` [igt-dev] " Janusz Krzysztofik
2019-11-04 17:13   ` [Intel-gfx] " Janusz Krzysztofik
2019-11-04 20:48   ` Vanshidhar Konda
2019-11-04 20:48     ` [igt-dev] " Vanshidhar Konda
2019-11-04 20:48     ` [Intel-gfx] " Vanshidhar Konda
2019-11-04 17:13 ` [PATCH i-g-t v5 4/4] tests/gem_ctx_shared: Align objects using " Janusz Krzysztofik
2019-11-04 17:13   ` [Intel-gfx] " Janusz Krzysztofik
2019-11-04 20:49   ` Vanshidhar Konda
2019-11-04 20:49     ` [Intel-gfx] " Vanshidhar Konda
2019-11-04 19:13 ` [igt-dev] ✓ Fi.CI.BAT: success for Calculate softpin offsets from minimum GTT alignment (rev2) Patchwork
2019-11-05  4:23 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.