[igt-dev] [PATCH i-g-t] tests/i915: Add test for unaligned detiler fences

[igt-dev] [PATCH i-g-t] tests/i915: Add test for unaligned detiler fences

[Imre Deak]

Add a test to check the detiling on a buffer with a size that isn't
aligned to the detiler fence stride we add for the buffer. While writes
beyond the last full tile row may target an address beyond the buffer's
size, we still expect that such writes will not lead to a corruption
on memory pages that are not owned by the process performing the write.

The reason for this test: on TGL such writes lead to random memory
corruption in memory not belonging to the process. This should be
prevented ensuring that we keep pages that user space can access
(padding the object size to be tile-row size aligned) reserved in the
GTT address space until userspace can write through the fenced region.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 tests/Makefile.sources                |   3 +
 tests/i915/gem_unaligned_fence.c      | 108 ++++++++++++++++++++++++++
 tests/intel-ci/fast-feedback.testlist |   1 +
 tests/meson.build                     |   1 +
 4 files changed, 113 insertions(+)
 create mode 100644 tests/i915/gem_unaligned_fence.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 806eb02d..dce1d197 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -460,6 +460,9 @@ gem_tiled_wc_SOURCES = i915/gem_tiled_wc.c
 TESTS_progs += gem_tiling_max_stride
 gem_tiling_max_stride_SOURCES = i915/gem_tiling_max_stride.c
 
+TESTS_progs += gem_unaligned_fence
+gem_unaligned_fence_SOURCES = i915/gem_unaligned_fence.c
+
 TESTS_progs += gem_unfence_active_buffers
 gem_unfence_active_buffers_SOURCES = i915/gem_unfence_active_buffers.c
 
diff --git a/tests/i915/gem_unaligned_fence.c b/tests/i915/gem_unaligned_fence.c
new file mode 100644
index 00000000..ac49fd80
--- /dev/null
+++ b/tests/i915/gem_unaligned_fence.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright © 2020 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 "igt.h"
+
+#define FILL_PAT64	0xeeeeeeeeeeeeeeee
+
+static bool check_buf(int fd, uint32_t handle, uint32_t size)
+{
+	uint64_t *ptr;
+	size_t i;
+
+	gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, 0);
+
+	ptr = gem_mmap__gtt(fd, handle, size, PROT_READ);
+
+	for (i = 0; i < size / sizeof(*ptr); i++)
+		if (ptr[i] != FILL_PAT64)
+			break;
+	munmap(ptr, size);
+
+	return i * sizeof(*ptr) == size;
+}
+
+static void memset64(uint64_t *s, uint64_t c, size_t n)
+{
+	for (int i = 0; i < n; i++)
+		s[i] = c;
+}
+
+static void fill_buf(int fd, uint32_t handle, size_t size)
+{
+	void *ptr;
+
+	gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+
+	ptr = gem_mmap__gtt(fd, handle, size, PROT_WRITE);
+	igt_assert(ptr);
+
+	memset64(ptr, FILL_PAT64, size / sizeof(uint64_t));
+
+	munmap(ptr, size);
+}
+
+static void test_stride(int fd, int stride, int tile_rows)
+{
+	uint32_t handle;
+	int tile_height;
+	int tile_row_size;
+	size_t size;
+
+	igt_info("Testing stride %d rows %d\n", stride, tile_rows);
+
+	tile_height = 32;
+	tile_row_size = stride * tile_height;
+	size = tile_row_size * tile_rows + tile_row_size / 2;
+
+	handle = gem_create(fd, size);
+
+	if (__gem_set_tiling(fd, handle, I915_TILING_Y, stride) != 0)
+		return;
+
+	fill_buf(fd, handle, size);
+
+	igt_assert(check_buf(fd, handle, size));
+
+	gem_close(fd, handle);
+}
+
+static void do_test_all_strides(int fd)
+{
+	int tile_rows;
+	int stride;
+
+	for (tile_rows = 1; tile_rows < 10; tile_rows++)
+		for (stride = 512; stride < 8196; stride += 512)
+			test_stride(fd, stride, tile_rows);
+}
+
+igt_main
+{
+	int fd;
+
+	igt_fixture
+		fd = drm_open_driver_render(DRIVER_INTEL);
+
+	igt_subtest("basic")
+		do_test_all_strides(fd);
+}
diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
index 8081446e..44460cd2 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -70,6 +70,7 @@ igt@gem_sync@basic-store-each
 igt@gem_tiled_blits@basic
 igt@gem_tiled_fence_blits@basic
 igt@gem_tiled_pread_basic
+igt@gem_unaligned_fence@basic
 igt@gem_wait@basic-busy-all
 igt@gem_wait@basic-wait-all
 igt@gem_wait@basic-await-all
diff --git a/tests/meson.build b/tests/meson.build
index 570de545..19cdce53 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -219,6 +219,7 @@ i915_progs = [
 	'gem_tiled_wb',
 	'gem_tiled_wc',
 	'gem_tiling_max_stride',
+	'gem_unaligned_fence',
 	'gem_unfence_active_buffers',
 	'gem_unref_active_buffers',
 	'gem_userptr_blits',
-- 
2.23.1

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Add test for unaligned detiler fences

[Chris Wilson]

Quoting Imre Deak (2020-01-13 17:21:20)
> Add a test to check the detiling on a buffer with a size that isn't
> aligned to the detiler fence stride we add for the buffer. While writes
> beyond the last full tile row may target an address beyond the buffer's
> size, we still expect that such writes will not lead to a corruption
> on memory pages that are not owned by the process performing the write.
> 
> The reason for this test: on TGL such writes lead to random memory
> corruption in memory not belonging to the process. This should be
> prevented ensuring that we keep pages that user space can access
> (padding the object size to be tile-row size aligned) reserved in the
> GTT address space until userspace can write through the fenced region.

This is going through a different IP block and driver paths than the
original bug. When we are using a fence, we create a vma that is
sufficient to include the last tile row. When we create a vma for the
display, we don't ask for that...

--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -344,19 +344,24 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	 * put it anyway and hope that userspace can cope (but always first
 	 * try to preserve the existing ABI).
 	 */
-	vma = ERR_PTR(-ENOSPC);
-	if ((flags & PIN_MAPPABLE) == 0 &&
-	    (!view || view->type == I915_GGTT_VIEW_NORMAL))
-		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment,
-					       flags |
-					       PIN_MAPPABLE |
-					       PIN_NONBLOCK);
-	if (IS_ERR(vma))
-		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
+	vma = i915_vma_instance(obj, &i915->ggtt.vm, view);
 	if (IS_ERR(vma))
 		return vma;

-	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
+	alignment = max(vma->display_alignment, alignment);
+
+	ret = -ENOSPC;
+	if ((flags & PIN_MAPPABLE) == 0 &&
+	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
+		ret = i915_vma_pin(vma,
+				   alignment, vma->fence_size,
+				   flags | PIN_MAPPABLE | PIN_NONBLOCK);
+	if (ret == -ENOSPC)
+		ret = i915_vma_pin(vma, alignment, vma->fence_size, flags);
+	if (ret)
+		return ERR_PTR(ret);
+
+	vma->display_alignment = alignment;

 	i915_gem_object_flush_if_display(obj);

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 17d7c525ea5c..57df22b809fa 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -619,7 +619,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	GEM_BUG_ON(drm_mm_node_allocated(&vma->node));

 	size = max(size, vma->size);
-	alignment = max(alignment, vma->display_alignment);
+	alignment = max_t(typeof(alignment), alignment, vma->display_alignment);
 	if (flags & PIN_MAPPABLE) {
 		size = max_t(typeof(size), size, vma->fence_size);
 		alignment = max_t(typeof(alignment),
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
index e0942efd5236..2cf642ecbd7d 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i915_vma_types.h
@@ -182,7 +182,6 @@ struct i915_vma {
 	struct i915_fence_reg *fence;

 	u64 size;
-	u64 display_alignment;
 	struct i915_page_sizes page_sizes;

 	/* mmap-offset associated with fencing for this vma */
@@ -190,6 +189,7 @@ struct i915_vma {

 	u32 fence_size;
 	u32 fence_alignment;
+	u32 display_alignment;

 	/**
 	 * Count of the number of times this vma has been opened by different

The partial fencing interaction with HW and peeking at the pages behind
should be covered by selftests -- as we want to peek at physical pages.
Mostly done already.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/i915: Add test for unaligned detiler fences

[Imre Deak]

On Mon, Jan 13, 2020 at 05:50:09PM +0000, Chris Wilson wrote:
> Quoting Imre Deak (2020-01-13 17:21:20)
> > Add a test to check the detiling on a buffer with a size that isn't
> > aligned to the detiler fence stride we add for the buffer. While writes
> > beyond the last full tile row may target an address beyond the buffer's
> > size, we still expect that such writes will not lead to a corruption
> > on memory pages that are not owned by the process performing the write.
> > 
> > The reason for this test: on TGL such writes lead to random memory
> > corruption in memory not belonging to the process. This should be
> > prevented ensuring that we keep pages that user space can access
> > (padding the object size to be tile-row size aligned) reserved in the
> > GTT address space until userspace can write through the fenced region.
> 
> This is going through a different IP block and driver paths than the
> original bug. When we are using a fence, we create a vma that is
> sufficient to include the last tile row. When we create a vma for the
> display, we don't ask for that...

Yes, then there must be some other issue too, since this test still
shows the corruption, even though it's only using a gem buffer not a
framebuffer.

> 
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -344,19 +344,24 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	 * put it anyway and hope that userspace can cope (but always first
>  	 * try to preserve the existing ABI).
>  	 */
> -	vma = ERR_PTR(-ENOSPC);
> -	if ((flags & PIN_MAPPABLE) == 0 &&
> -	    (!view || view->type == I915_GGTT_VIEW_NORMAL))
> -		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment,
> -					       flags |
> -					       PIN_MAPPABLE |
> -					       PIN_NONBLOCK);
> -	if (IS_ERR(vma))
> -		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
> +	vma = i915_vma_instance(obj, &i915->ggtt.vm, view);
>  	if (IS_ERR(vma))
>  		return vma;
> 
> -	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
> +	alignment = max(vma->display_alignment, alignment);
> +
> +	ret = -ENOSPC;
> +	if ((flags & PIN_MAPPABLE) == 0 &&
> +	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> +		ret = i915_vma_pin(vma,
> +				   alignment, vma->fence_size,
> +				   flags | PIN_MAPPABLE | PIN_NONBLOCK);
> +	if (ret == -ENOSPC)
> +		ret = i915_vma_pin(vma, alignment, vma->fence_size, flags);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	vma->display_alignment = alignment;
> 
>  	i915_gem_object_flush_if_display(obj);
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 17d7c525ea5c..57df22b809fa 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -619,7 +619,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  	GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
> 
>  	size = max(size, vma->size);
> -	alignment = max(alignment, vma->display_alignment);
> +	alignment = max_t(typeof(alignment), alignment, vma->display_alignment);
>  	if (flags & PIN_MAPPABLE) {
>  		size = max_t(typeof(size), size, vma->fence_size);
>  		alignment = max_t(typeof(alignment),
> diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
> index e0942efd5236..2cf642ecbd7d 100644
> --- a/drivers/gpu/drm/i915/i915_vma_types.h
> +++ b/drivers/gpu/drm/i915/i915_vma_types.h
> @@ -182,7 +182,6 @@ struct i915_vma {
>  	struct i915_fence_reg *fence;
> 
>  	u64 size;
> -	u64 display_alignment;
>  	struct i915_page_sizes page_sizes;
> 
>  	/* mmap-offset associated with fencing for this vma */
> @@ -190,6 +189,7 @@ struct i915_vma {
> 
>  	u32 fence_size;
>  	u32 fence_alignment;
> +	u32 display_alignment;
> 
>  	/**
>  	 * Count of the number of times this vma has been opened by different
> 
> The partial fencing interaction with HW and peeking at the pages behind
> should be covered by selftests -- as we want to peek at physical pages.
> Mostly done already.
> -Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev