[igt-dev] [PATCH i-g-t] igt: Another combinatorial exercise for blits

[igt-dev] [PATCH i-g-t] igt: Another combinatorial exercise for blits

[Chris Wilson]

The aim of this test is to combine gem_linear_blits, gem_tiled_blits etc
into one test runner that covers investigation into HW alignment issues
as well as driver boundaries (relocs, access, thrashing).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
---
 tests/Makefile.sources |   1 +
 tests/gem_blits.c      | 753 +++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 3 files changed, 755 insertions(+)
 create mode 100644 tests/gem_blits.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c84933f1d..564545fb7 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -41,6 +41,7 @@ TESTS_progs = \
 	drv_suspend \
 	gem_bad_reloc \
 	gem_basic \
+	gem_blits \
 	gem_busy \
 	gem_caching \
 	gem_close \
diff --git a/tests/gem_blits.c b/tests/gem_blits.c
new file mode 100644
index 000000000..44da775a4
--- /dev/null
+++ b/tests/gem_blits.c
@@ -0,0 +1,753 @@
+/*
+ * Copyright © 2018 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"
+#include "igt_x86.h"
+
+#define MI_FLUSH_DW (0x26 << 23)
+
+#define BCS_SWCTRL 0x22200
+#define BCS_SRC_Y (1 << 0)
+#define BCS_DST_Y (1 << 1)
+
+struct device {
+	int fd;
+	int gen;
+	int pciid;
+	int llc;
+};
+
+struct buffer {
+	uint32_t handle;
+	uint16_t width;
+	uint16_t height;
+	uint16_t stride;
+	uint32_t size;
+	unsigned int tiling;
+	unsigned int caching;
+	uint64_t gtt_offset;
+	uint32_t model[] __attribute__((aligned(16)));
+};
+
+static int fls(uint64_t x)
+{
+	int t;
+
+	for (t = 0; x >> t; t++)
+		;
+
+	return t;
+}
+
+static unsigned int
+get_tiling_stride(const struct device *device,
+		  unsigned int width, unsigned int tiling)
+{
+	unsigned int stride = 4u * width;
+
+	if (tiling) {
+		if (device->gen < 3)
+			stride = ALIGN(stride, 128);
+		else if (device->gen < 4 || tiling == I915_TILING_X)
+			stride = ALIGN(stride, 512);
+		else
+			stride = ALIGN(stride, 128);
+		if (device->gen < 4)
+			stride = 1 << fls(stride - 1);
+	} else {
+		if (device->gen >= 8)
+			stride = ALIGN(stride, 64);
+	}
+
+	igt_assert(stride < UINT16_MAX && stride >= 4*width);
+	return stride;
+}
+
+static unsigned int
+get_tiling_height(const struct device *device,
+		  unsigned int height, unsigned int tiling)
+{
+	if (!tiling)
+		return height;
+
+	if (device->gen < 3)
+		return ALIGN(height, 16);
+	else if (device->gen < 4 || tiling == I915_TILING_X)
+		return ALIGN(height, 8);
+	else
+		return ALIGN(height, 32);
+}
+
+static struct buffer *buffer_create(const struct device *device,
+				    unsigned int width,
+				    unsigned int height)
+{
+	struct buffer *buffer;
+
+	igt_assert(width && height);
+
+	buffer = calloc(1, sizeof(*buffer) + 4u * width * height);
+	if (!buffer)
+		return NULL;
+
+	buffer->width = width;
+	buffer->height = height;
+
+	buffer->stride = get_tiling_stride(device, width, I915_TILING_NONE);
+	buffer->size = ALIGN(buffer->stride * height, 4096);
+	buffer->handle = gem_create(device->fd, buffer->size);
+	buffer->caching = device->llc;
+
+	for (int y = 0; y < height; y++) {
+		uint32_t *row = buffer->model + y * width;
+
+		for (int x = 0; x < width; x++)
+			row[x] = (y << 16 | x) ^ buffer->handle;
+
+		gem_write(device->fd,
+			  buffer->handle, 4u * y * width,
+			  row, 4u * width);
+	}
+
+	return buffer;
+}
+
+static void buffer_set_tiling(const struct device *device,
+			      struct buffer *buffer,
+			      unsigned int tiling)
+{
+	struct drm_i915_gem_exec_object2 obj[3];
+	struct drm_i915_gem_relocation_entry reloc[2];
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const bool has_64b_reloc = device->gen >= 8;
+	uint32_t stride, size, pitch;
+	uint32_t *batch;
+	int i;
+
+	if (buffer->tiling == tiling)
+		return;
+
+	stride = get_tiling_stride(device, buffer->width, tiling);
+	size = stride * get_tiling_height(device, buffer->height, tiling);
+	size = ALIGN(size, 4096);
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	if (device->gen >= 6)
+		execbuf.flags = I915_EXEC_BLT;
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = gem_create(device->fd, size);
+	if (tiling) {
+		obj[0].flags = EXEC_OBJECT_NEEDS_FENCE;
+		gem_set_tiling(device->fd, obj[0].handle, tiling, stride);
+	}
+
+	obj[1].handle = buffer->handle;
+	obj[1].offset = buffer->gtt_offset;
+	if (buffer->tiling)
+		obj[1].flags = EXEC_OBJECT_NEEDS_FENCE;
+
+	obj[2].handle = gem_create(device->fd, 4096);
+	obj[2].relocs_ptr = to_user_pointer(memset(reloc, 0, sizeof(reloc)));
+	obj[2].relocation_count = 2;
+	batch = gem_mmap__cpu(device->fd, obj[2].handle, 0, 4096, PROT_WRITE);
+
+	i = 0;
+
+	if ((tiling | buffer->tiling) >= I915_TILING_Y) {
+		unsigned int mask;
+
+		batch[i++] = MI_LOAD_REGISTER_IMM;
+		batch[i++] = BCS_SWCTRL;
+
+		mask = (BCS_SRC_Y | BCS_DST_Y) << 16;
+		if (buffer->tiling == I915_TILING_Y)
+			mask |= BCS_SRC_Y;
+		if (tiling == I915_TILING_Y)
+			mask |= BCS_DST_Y;
+		batch[i++] = mask;
+	}
+
+	batch[i] = (XY_SRC_COPY_BLT_CMD |
+		    XY_SRC_COPY_BLT_WRITE_ALPHA |
+		    XY_SRC_COPY_BLT_WRITE_RGB);
+	batch[i] |= 6 + 2 * has_64b_reloc;
+	if (device->gen >= 4 && buffer->tiling)
+		batch[i] |= XY_SRC_COPY_BLT_SRC_TILED;
+	if (device->gen >= 4 && tiling)
+		batch[i] |= XY_SRC_COPY_BLT_DST_TILED;
+	i++;
+
+	pitch = stride;
+	if (device->gen >= 4 && tiling)
+		pitch /= 4;
+	batch[i++] = 3 << 24 | 0xcc << 16 | pitch;
+	batch[i++] = 0;
+	batch[i++] = buffer->height << 16 | buffer->width;
+	reloc[0].target_handle = obj[0].handle;
+	reloc[0].presumed_offset = obj[0].offset;
+	reloc[0].offset = sizeof(*batch) * i;
+	reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
+	reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
+	batch[i++] = obj[0].offset;
+	if (has_64b_reloc)
+		batch[i++] = obj[0].offset >> 32;
+
+	batch[i++] = 0;
+	pitch = buffer->stride;
+	if (device->gen >= 4 && buffer->tiling)
+		pitch /= 4;
+	batch[i++] = pitch;
+	reloc[1].target_handle = obj[1].handle;
+	reloc[1].presumed_offset = obj[1].offset;
+	reloc[1].offset = sizeof(*batch) * i;
+	reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
+	batch[i++] = obj[1].offset;
+	if (has_64b_reloc)
+		batch[i++] = obj[1].offset >> 32;
+
+	if ((tiling | buffer->tiling) >= I915_TILING_Y) {
+		igt_assert(device->gen >= 6);
+		batch[i++] = MI_FLUSH_DW | 2;
+		batch[i++] = 0;
+		batch[i++] = 0;
+		batch[i++] = 0;
+
+		batch[i++] = MI_LOAD_REGISTER_IMM;
+		batch[i++] = BCS_SWCTRL;
+		batch[i++] = (BCS_SRC_Y | BCS_DST_Y) << 16;
+	}
+
+	batch[i++] = MI_BATCH_BUFFER_END;
+	munmap(batch, 4096);
+
+	gem_execbuf(device->fd, &execbuf);
+
+	gem_close(device->fd, obj[2].handle);
+	gem_close(device->fd, obj[1].handle);
+
+	buffer->gtt_offset = obj[0].offset;
+	buffer->handle = obj[0].handle;
+
+	buffer->tiling = tiling;
+	buffer->stride = stride;
+	buffer->size = size;
+}
+
+enum mode {
+	CPU,
+	PRW,
+	GTT,
+	WC,
+};
+
+static void blit_to_linear(const struct device *device,
+			   const struct buffer *buffer,
+			   void *linear)
+{
+	struct drm_i915_gem_exec_object2 obj[3];
+	struct drm_i915_gem_relocation_entry reloc[2];
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const bool has_64b_reloc = device->gen >= 8;
+	uint32_t *batch;
+	uint32_t pitch;
+	int i = 0;
+
+	igt_assert(buffer->tiling);
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	if (device->gen >= 6)
+		execbuf.flags = I915_EXEC_BLT;
+
+	memset(obj, 0, sizeof(obj));
+	gem_userptr(device->fd, linear, buffer->size, 0, 0, &obj[0].handle);
+	obj[1].handle = buffer->handle;
+	obj[1].offset = buffer->gtt_offset;
+	obj[1].flags = EXEC_OBJECT_NEEDS_FENCE;
+
+	memset(reloc, 0, sizeof(reloc));
+	obj[2].handle = gem_create(device->fd, 4096);
+	obj[2].relocs_ptr = to_user_pointer(reloc);
+	obj[2].relocation_count = ARRAY_SIZE(reloc);
+	batch = gem_mmap__cpu(device->fd, obj[2].handle, 0, 4096, PROT_WRITE);
+
+	if (buffer->tiling >= I915_TILING_Y) {
+		unsigned int mask;
+
+		batch[i++] = MI_LOAD_REGISTER_IMM;
+		batch[i++] = BCS_SWCTRL;
+
+		mask = (BCS_SRC_Y | BCS_DST_Y) << 16;
+		if (buffer->tiling == I915_TILING_Y)
+			mask |= BCS_SRC_Y;
+		batch[i++] = mask;
+	}
+
+	batch[i] = (XY_SRC_COPY_BLT_CMD |
+		    XY_SRC_COPY_BLT_WRITE_ALPHA |
+		    XY_SRC_COPY_BLT_WRITE_RGB);
+	if (device->gen >= 4 && buffer->tiling)
+		batch[i] |= XY_SRC_COPY_BLT_SRC_TILED;
+
+	batch[i++] |= 6 + 2 * has_64b_reloc;
+
+	batch[i++] = 3 << 24 | 0xcc << 16 | buffer->stride;
+	batch[i++] = 0;
+	batch[i++] = buffer->height << 16 | buffer->width;
+	reloc[0].target_handle = obj[0].handle;
+	reloc[0].presumed_offset = obj[0].offset;
+	reloc[0].offset = sizeof(*batch) * i;
+	reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
+	reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
+	batch[i++] = obj[0].offset;
+	if (has_64b_reloc)
+		batch[i++] = obj[0].offset >> 32;
+
+	batch[i++] = 0;
+	pitch = buffer->stride;
+	if (device->gen >= 4 && buffer->tiling)
+		pitch /= 4;
+	batch[i++] = pitch;
+	reloc[1].target_handle = obj[1].handle;
+	reloc[1].presumed_offset = obj[1].offset;
+	reloc[1].offset = sizeof(*batch) * i;
+	reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
+	batch[i++] = obj[1].offset;
+	if (has_64b_reloc)
+		batch[i++] = obj[1].offset >> 32;
+
+	if (buffer->tiling >= I915_TILING_Y) {
+		igt_assert(device->gen >= 6);
+		batch[i++] = MI_FLUSH_DW | 2;
+		batch[i++] = 0;
+		batch[i++] = 0;
+		batch[i++] = 0;
+
+		batch[i++] = MI_LOAD_REGISTER_IMM;
+		batch[i++] = BCS_SWCTRL;
+		batch[i++] = (BCS_SRC_Y | BCS_DST_Y) << 16;
+	}
+
+	batch[i++] = MI_BATCH_BUFFER_END;
+	munmap(batch, 4096);
+
+	gem_execbuf(device->fd, &execbuf);
+	gem_close(device->fd, obj[2].handle);
+
+	gem_sync(device->fd, obj[0].handle);
+	gem_close(device->fd, obj[0].handle);
+}
+
+static void *download(const struct device *device,
+		      const struct buffer *buffer,
+		      enum mode mode)
+{
+	void *linear, *src;
+
+	igt_assert(posix_memalign(&linear, 4096, buffer->size) == 0);
+
+	switch (mode) {
+	case CPU:
+		if (buffer->tiling) {
+			if (buffer->caching && !device->llc) {
+				mode = GTT;
+				break;
+			}
+			if (device->gen < 3) {
+				mode = GTT;
+				break;
+			}
+
+			blit_to_linear(device, buffer, linear);
+			return linear;
+		}
+		break;
+
+	case PRW:
+	case WC:
+		if (!buffer->tiling)
+			break;
+
+	default:
+		mode = GTT;
+		break;
+	}
+
+	switch (mode) {
+	case CPU:
+		src = gem_mmap__cpu(device->fd, buffer->handle,
+				    0, buffer->size,
+				    PROT_READ);
+
+		gem_set_domain(device->fd, buffer->handle,
+			       I915_GEM_DOMAIN_CPU, 0);
+		igt_memcpy_from_wc(linear, src, buffer->size);
+		munmap(src, buffer->size);
+		break;
+
+	case WC:
+		src = gem_mmap__wc(device->fd, buffer->handle,
+				   0, buffer->size,
+				   PROT_READ);
+
+		gem_set_domain(device->fd, buffer->handle,
+			       I915_GEM_DOMAIN_WC, 0);
+		igt_memcpy_from_wc(linear, src, buffer->size);
+		munmap(src, buffer->size);
+		break;
+
+	case GTT:
+		src = gem_mmap__gtt(device->fd, buffer->handle,
+				   buffer->size,
+				   PROT_READ);
+
+		gem_set_domain(device->fd, buffer->handle,
+			       I915_GEM_DOMAIN_GTT, 0);
+		igt_memcpy_from_wc(linear, src, buffer->size);
+		munmap(src, buffer->size);
+		break;
+
+	case PRW:
+		gem_read(device->fd, buffer->handle, 0, linear, buffer->size);
+		break;
+	}
+
+	return linear;
+}
+
+static bool buffer_check(const struct device *device,
+			 const struct buffer *buffer,
+			 enum mode mode)
+{
+	unsigned int num_errors = 0;
+	uint32_t *linear;
+
+	linear = download(device, buffer, mode);
+	igt_assert(linear);
+
+	for (int y = 0; y < buffer->height; y++) {
+		const uint32_t *model = buffer->model + y * buffer->width;
+		const uint32_t *row =
+			linear + y * buffer->stride / sizeof(uint32_t);
+
+		for (int x = 0; x < buffer->width; x++) {
+			if (row[x] != model[x] && num_errors++ < 5) {
+				igt_warn("buffer handle=%d mismatch at (%d, %d): expected %08x, found %08x\n",
+					 buffer->handle,
+					 x, y, model[x], row[x]);
+			}
+		}
+	}
+
+	free(linear);
+
+	return num_errors == 0;
+}
+
+static void buffer_free(const struct device *device, struct buffer *buffer)
+{
+	igt_assert(buffer_check(device, buffer, GTT));
+	gem_close(device->fd, buffer->handle);
+	free(buffer);
+}
+
+static void memcpy_blt(const void *src, void *dst,
+		       uint32_t src_stride, uint32_t dst_stride,
+		       uint16_t src_x, uint16_t src_y,
+		       uint16_t dst_x, uint16_t dst_y,
+		       uint16_t width, uint16_t height)
+{
+	const uint8_t *src_bytes;
+	uint8_t *dst_bytes;
+	int byte_width;
+
+	src_bytes = (const uint8_t *)src + src_stride * src_y + src_x * 4;
+	dst_bytes = (uint8_t *)dst + dst_stride * dst_y + dst_x * 4;
+
+	byte_width = width * 4;
+	if (byte_width == src_stride && byte_width == dst_stride) {
+		byte_width *= height;
+		height = 1;
+	}
+
+	switch (byte_width) {
+	case 4:
+		do {
+			*(uint32_t *)dst_bytes = *(const uint32_t *)src_bytes;
+			src_bytes += src_stride;
+			dst_bytes += dst_stride;
+		} while (--height);
+		break;
+
+	case 8:
+		do {
+			*(uint64_t *)dst_bytes = *(const uint64_t *)src_bytes;
+			src_bytes += src_stride;
+			dst_bytes += dst_stride;
+		} while (--height);
+		break;
+	case 16:
+		do {
+			((uint64_t *)dst_bytes)[0] = ((const uint64_t *)src_bytes)[0];
+			((uint64_t *)dst_bytes)[1] = ((const uint64_t *)src_bytes)[1];
+			src_bytes += src_stride;
+			dst_bytes += dst_stride;
+		} while (--height);
+		break;
+
+	default:
+		do {
+			memcpy(dst_bytes, src_bytes, byte_width);
+			src_bytes += src_stride;
+			dst_bytes += dst_stride;
+		} while (--height);
+		break;
+	}
+}
+
+static void
+blit(const struct device *device,
+     struct buffer *src, uint16_t src_x, uint16_t src_y,
+     struct buffer *dst, uint16_t dst_x, uint16_t dst_y,
+     uint16_t width, uint16_t height)
+
+{
+	struct drm_i915_gem_exec_object2 obj[3];
+	struct drm_i915_gem_relocation_entry reloc[2];
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const bool has_64b_reloc = device->gen >= 8;
+	uint32_t *batch;
+	uint32_t pitch;
+	int i = 0;
+
+	if (src_x < 0) {
+		width += src_x;
+		dst_x -= src_x;
+		src_x = 0;
+	}
+	if (src_y < 0) {
+		height += src_y;
+		dst_y  -= src_y;
+		src_y = 0;
+	}
+
+	if (dst_x < 0) {
+		width += dst_x;
+		src_x -= dst_x;
+		dst_x = 0;
+	}
+	if (dst_y < 0) {
+		height += dst_y;
+		src_y  -= dst_y;
+		dst_y = 0;
+	}
+
+	if (src_x + width > src->width)
+		width = src->width - src_x;
+	if (dst_x + width > dst->width)
+		width = dst->width - dst_x;
+
+	if (src_y + height > src->height)
+		height = src->height - src_y;
+	if (dst_y + height > dst->height)
+		height = dst->height - dst_y;
+
+	if (dst->caching) {
+		igt_assert(device->gen >= 3);
+		igt_assert(device->llc || !src->caching);
+	}
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	if (device->gen >= 6)
+		execbuf.flags = I915_EXEC_BLT;
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = dst->handle;
+	obj[0].offset = dst->gtt_offset;
+	if (dst->tiling)
+		obj[0].flags = EXEC_OBJECT_NEEDS_FENCE;
+
+	obj[1].handle = src->handle;
+	obj[1].offset = src->gtt_offset;
+	if (src->tiling)
+		obj[1].flags = EXEC_OBJECT_NEEDS_FENCE;
+
+	memset(reloc, 0, sizeof(reloc));
+	obj[2].handle = gem_create(device->fd, 4096);
+	obj[2].relocs_ptr = to_user_pointer(reloc);
+	obj[2].relocation_count = ARRAY_SIZE(reloc);
+	batch = gem_mmap__cpu(device->fd, obj[2].handle, 0, 4096, PROT_WRITE);
+
+	if ((src->tiling | dst->tiling) >= I915_TILING_Y) {
+		unsigned int mask;
+
+		batch[i++] = MI_LOAD_REGISTER_IMM;
+		batch[i++] = BCS_SWCTRL;
+
+		mask = (BCS_SRC_Y | BCS_DST_Y) << 16;
+		if (src->tiling == I915_TILING_Y)
+			mask |= BCS_SRC_Y;
+		if (dst->tiling == I915_TILING_Y)
+			mask |= BCS_DST_Y;
+		batch[i++] = mask;
+	}
+
+	batch[i] = (XY_SRC_COPY_BLT_CMD |
+		    XY_SRC_COPY_BLT_WRITE_ALPHA |
+		    XY_SRC_COPY_BLT_WRITE_RGB);
+	batch[i] |= 6 + 2 * has_64b_reloc;
+	if (device->gen >= 4 && src->tiling)
+		batch[i] |= XY_SRC_COPY_BLT_SRC_TILED;
+	if (device->gen >= 4 && dst->tiling)
+		batch[i] |= XY_SRC_COPY_BLT_DST_TILED;
+	i++;
+
+	pitch = dst->stride;
+	if (device->gen >= 4 && dst->tiling)
+		pitch /= 4;
+	batch[i++] = 3 << 24 | 0xcc << 16 | pitch;
+
+	batch[i++] = dst_y << 16 | dst_x;
+	batch[i++] = (height + dst_y) << 16 | (width + dst_x);
+	reloc[0].target_handle = obj[0].handle;
+	reloc[0].presumed_offset = obj[0].offset;
+	reloc[0].offset = sizeof(*batch) * i;
+	reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
+	reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
+	batch[i++] = obj[0].offset;
+	if (has_64b_reloc)
+		batch[i++] = obj[0].offset >> 32;
+
+	batch[i++] = src_y << 16 | src_x;
+	pitch = src->stride;
+	if (device->gen >= 4 && src->tiling)
+		pitch /= 4;
+	batch[i++] = pitch;
+	reloc[1].target_handle = obj[1].handle;
+	reloc[1].presumed_offset = obj[1].offset;
+	reloc[1].offset = sizeof(*batch) * i;
+	reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
+	batch[i++] = obj[1].offset;
+	if (has_64b_reloc)
+		batch[i++] = obj[1].offset >> 32;
+
+	if ((src->tiling | dst->tiling) >= I915_TILING_Y) {
+		igt_assert(device->gen >= 6);
+		batch[i++] = MI_FLUSH_DW | 2;
+		batch[i++] = 0;
+		batch[i++] = 0;
+		batch[i++] = 0;
+
+		batch[i++] = MI_LOAD_REGISTER_IMM;
+		batch[i++] = BCS_SWCTRL;
+		batch[i++] = (BCS_SRC_Y | BCS_DST_Y) << 16;
+	}
+
+	batch[i++] = MI_BATCH_BUFFER_END;
+	munmap(batch, 4096);
+
+	gem_execbuf(device->fd, &execbuf);
+	gem_close(device->fd, obj[2].handle);
+
+	dst->gtt_offset = obj[0].offset;
+	src->gtt_offset = obj[1].offset;
+
+	memcpy_blt(src->model, dst->model,
+		   4u * src->width, 4u * dst->width,
+		   src_x, src_y,
+		   dst_x, dst_y,
+		   width, height);
+}
+
+igt_main
+{
+	struct device device;
+
+	igt_fixture {
+		device.fd = drm_open_driver_render(DRIVER_INTEL);
+		igt_require_gem(device.fd);
+
+		device.pciid = intel_get_drm_devid(device.fd);
+		device.gen = intel_gen(device.pciid);
+		device.llc = gem_has_llc(device.fd);
+	}
+
+	igt_subtest("basic") {
+		struct buffer *src, *dst;
+		unsigned int x, y;
+
+		for (unsigned int height = 1; height <= 16; height <<= 1) {
+			for (unsigned int width = 1; width <= 64; width <<= 1) {
+				src = buffer_create(&device,
+						    width * 16, height * 4);
+				dst = buffer_create(&device,
+						    width * 16, height * 4);
+
+				y = 0;
+				for (unsigned int src_tiling = I915_TILING_NONE;
+				     src_tiling <= (device.gen >= 6 ? I915_TILING_Y : I915_TILING_X);
+				     src_tiling++) {
+					buffer_set_tiling(&device, src, src_tiling);
+
+					x = 0;
+					for (unsigned int dst_tiling = I915_TILING_NONE;
+					     dst_tiling <= (device.gen >= 6 ? I915_TILING_Y : I915_TILING_X);
+					     dst_tiling++) {
+						buffer_set_tiling(&device, dst, dst_tiling);
+
+						for (enum mode down = CPU; down <= WC; down++) {
+							igt_debug("Testing src_tiling=%d, dst_tiling=%d, down=%d at (%d, %d) x (%d, %d)\n",
+								  src_tiling,
+								  dst_tiling,
+								  down, x, y,
+								  width, height);
+
+							igt_assert(x + width <= dst->width);
+							igt_assert(y + height <= dst->height);
+
+							blit(&device,
+							     src, x, y,
+							     dst, x, y,
+							     width, height);
+							igt_assert(buffer_check(&device, dst, down));
+
+							x += width;
+						}
+					}
+
+					y += height;
+				}
+
+				buffer_free(&device, dst);
+				buffer_free(&device, src);
+			}
+		}
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 32c2156c6..becadfa8d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -18,6 +18,7 @@ test_progs = [
 	'drv_suspend',
 	'gem_bad_reloc',
 	'gem_basic',
+	'gem_blits',
 	'gem_busy',
 	'gem_caching',
 	'gem_close',
-- 
2.18.0

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

[igt-dev] ✓ Fi.CI.BAT: success for igt: Another combinatorial exercise for blits

[Patchwork]

== Series Details ==

Series: igt: Another combinatorial exercise for blits
URL   : https://patchwork.freedesktop.org/series/47249/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4547 -> IGTPW_1646 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47249/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-no-display:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#105395) +2

    igt@gem_exec_suspend@basic-s3:
      {fi-skl-caroline}:  NOTRUN -> INCOMPLETE (fdo#104108)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#105128, fdo#107139)

    igt@gem_sync@basic-each:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    {igt@kms_psr@cursor_plane_move}:
      fi-cnl-psr:         NOTRUN -> DMESG-FAIL (fdo#107372)

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#107372) +1

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@gem_exec_create@basic:
      fi-glk-j4005:       DMESG-WARN (fdo#106745) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

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

  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105395 https://bugs.freedesktop.org/show_bug.cgi?id=105395
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (48 -> 42) ==

  Additional (3): fi-byt-j1900 fi-skl-caroline fi-cnl-psr 
  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-cfl-guc fi-kbl-guc fi-ctg-p8600 fi-byt-clapper 


== Build changes ==

    * IGT: IGT_4575 -> IGTPW_1646

  CI_DRM_4547: 0a7ab192a697e951b2404f3c1ce42c5fa74f9ed1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1646: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1646/
  IGT_4575: fe908a01012c9daafafb3410b9407725ca9d4f21 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@gem_blits@basic

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for igt: Another combinatorial exercise for blits

[Patchwork]

== Series Details ==

Series: igt: Another combinatorial exercise for blits
URL   : https://patchwork.freedesktop.org/series/47249/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4575_full -> IGTPW_1646_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1646_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1646_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47249/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-bsd1:
      shard-kbl:          SKIP -> PASS +2

    igt@perf_pmu@rc6-runtime-pm:
      shard-hsw:          SKIP -> PASS

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_cursor_legacy@all-pipes-torture-bo:
      shard-apl:          PASS -> DMESG-WARN (fdo#107122)
      shard-snb:          PASS -> DMESG-WARN (fdo#107122)

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355)

    igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-gtt:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          PASS -> FAIL (fdo#103166)

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-snb:          PASS -> FAIL (fdo#103925)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    igt@kms_universal_plane@cursor-fb-leak-pipe-b:
      shard-apl:          PASS -> FAIL (fdo#107241)

    igt@kms_vblank@pipe-a-ts-continuation-suspend:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@pm_rpm@dpms-lpsp:
      shard-glk:          SKIP -> FAIL (fdo#106539)

    igt@pm_rpm@fences:
      shard-glk:          PASS -> FAIL (fdo#106539) +1

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-apl:          INCOMPLETE (fdo#106886, fdo#103927) -> PASS

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@pm_rpm@debugfs-forcewake-user:
      shard-hsw:          FAIL (fdo#106539) -> PASS +4

    igt@pm_rpm@drm-resources-equal:
      shard-kbl:          FAIL -> PASS
      shard-glk:          FAIL (fdo#106539) -> PASS
      shard-apl:          FAIL (fdo#106539) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106539 https://bugs.freedesktop.org/show_bug.cgi?id=106539
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107122 https://bugs.freedesktop.org/show_bug.cgi?id=107122
  fdo#107241 https://bugs.freedesktop.org/show_bug.cgi?id=107241
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4575 -> IGTPW_1646
    * Linux: CI_DRM_4541 -> CI_DRM_4547

  CI_DRM_4541: 3e18e4c6c008597f4ff952d7a3457bd310ce945c @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4547: 0a7ab192a697e951b2404f3c1ce42c5fa74f9ed1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1646: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1646/
  IGT_4575: fe908a01012c9daafafb3410b9407725ca9d4f21 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t] igt: Another combinatorial exercise for blits

[Katarzyna Dec]

On Wed, Jul 25, 2018 at 10:38:23PM +0100, Chris Wilson wrote:
> The aim of this test is to combine gem_linear_blits, gem_tiled_blits etc
> into one test runner that covers investigation into HW alignment issues
> as well as driver boundaries (relocs, access, thrashing).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> ---
>  tests/Makefile.sources |   1 +
>  tests/gem_blits.c      | 753 +++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build      |   1 +
>  3 files changed, 755 insertions(+)
>  create mode 100644 tests/gem_blits.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index c84933f1d..564545fb7 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -41,6 +41,7 @@ TESTS_progs = \
>  	drv_suspend \
>  	gem_bad_reloc \
>  	gem_basic \
> +	gem_blits \
>  	gem_busy \
>  	gem_caching \
>  	gem_close \
> diff --git a/tests/gem_blits.c b/tests/gem_blits.c
> new file mode 100644
> index 000000000..44da775a4
> --- /dev/null
> +++ b/tests/gem_blits.c
> @@ -0,0 +1,753 @@
> +/*
> + * Copyright © 2018 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"
> +#include "igt_x86.h"
> +
> +#define MI_FLUSH_DW (0x26 << 23)
> +
> +#define BCS_SWCTRL 0x22200
> +#define BCS_SRC_Y (1 << 0)
> +#define BCS_DST_Y (1 << 1)
> +
> +struct device {
> +	int fd;
> +	int gen;
> +	int pciid;
> +	int llc;
> +};
> +
> +struct buffer {
> +	uint32_t handle;
> +	uint16_t width;
> +	uint16_t height;
> +	uint16_t stride;
> +	uint32_t size;
> +	unsigned int tiling;
> +	unsigned int caching;
> +	uint64_t gtt_offset;
> +	uint32_t model[] __attribute__((aligned(16)));
> +};
> +
> +static int fls(uint64_t x)
> +{
> +	int t;
> +
> +	for (t = 0; x >> t; t++)
> +		;
> +
> +	return t;
> +}
> +
> +static unsigned int
> +get_tiling_stride(const struct device *device,
> +		  unsigned int width, unsigned int tiling)
> +{
> +	unsigned int stride = 4u * width;
> +
> +	if (tiling) {
> +		if (device->gen < 3)
> +			stride = ALIGN(stride, 128);
> +		else if (device->gen < 4 || tiling == I915_TILING_X)
> +			stride = ALIGN(stride, 512);
> +		else
> +			stride = ALIGN(stride, 128);
> +		if (device->gen < 4)
> +			stride = 1 << fls(stride - 1);
Shouldn't 'else' be last here ^? What about order of 'else if' and 'if'
at the end?
> +	} else {
> +		if (device->gen >= 8)
> +			stride = ALIGN(stride, 64);
> +	}
> +
> +	igt_assert(stride < UINT16_MAX && stride >= 4*width);
> +	return stride;
> +}
> +
> +static unsigned int
> +get_tiling_height(const struct device *device,
> +		  unsigned int height, unsigned int tiling)
> +{
> +	if (!tiling)
> +		return height;
> +
> +	if (device->gen < 3)
> +		return ALIGN(height, 16);
> +	else if (device->gen < 4 || tiling == I915_TILING_X)
> +		return ALIGN(height, 8);
> +	else
> +		return ALIGN(height, 32);
> +}
> +
> +static struct buffer *buffer_create(const struct device *device,
> +				    unsigned int width,
> +				    unsigned int height)
> +{
> +	struct buffer *buffer;
> +
> +	igt_assert(width && height);
> +
> +	buffer = calloc(1, sizeof(*buffer) + 4u * width * height);
> +	if (!buffer)
> +		return NULL;
> +
> +	buffer->width = width;
> +	buffer->height = height;
> +
> +	buffer->stride = get_tiling_stride(device, width, I915_TILING_NONE);
> +	buffer->size = ALIGN(buffer->stride * height, 4096);
> +	buffer->handle = gem_create(device->fd, buffer->size);
> +	buffer->caching = device->llc;
> +
> +	for (int y = 0; y < height; y++) {
> +		uint32_t *row = buffer->model + y * width;
> +
> +		for (int x = 0; x < width; x++)
> +			row[x] = (y << 16 | x) ^ buffer->handle;
> +
> +		gem_write(device->fd,
> +			  buffer->handle, 4u * y * width,
> +			  row, 4u * width);
> +	}
> +
> +	return buffer;
> +}
> +
> +static void buffer_set_tiling(const struct device *device,
> +			      struct buffer *buffer,
> +			      unsigned int tiling)
> +{
> +	struct drm_i915_gem_exec_object2 obj[3];
> +	struct drm_i915_gem_relocation_entry reloc[2];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	const bool has_64b_reloc = device->gen >= 8;
> +	uint32_t stride, size, pitch;
> +	uint32_t *batch;
> +	int i;
> +
> +	if (buffer->tiling == tiling)
> +		return;
> +
> +	stride = get_tiling_stride(device, buffer->width, tiling);
> +	size = stride * get_tiling_height(device, buffer->height, tiling);
> +	size = ALIGN(size, 4096);
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	if (device->gen >= 6)
> +		execbuf.flags = I915_EXEC_BLT;
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[0].handle = gem_create(device->fd, size);
> +	if (tiling) {
> +		obj[0].flags = EXEC_OBJECT_NEEDS_FENCE;
> +		gem_set_tiling(device->fd, obj[0].handle, tiling, stride);
> +	}
> +
> +	obj[1].handle = buffer->handle;
> +	obj[1].offset = buffer->gtt_offset;
> +	if (buffer->tiling)
> +		obj[1].flags = EXEC_OBJECT_NEEDS_FENCE;
> +
> +	obj[2].handle = gem_create(device->fd, 4096);
> +	obj[2].relocs_ptr = to_user_pointer(memset(reloc, 0, sizeof(reloc)));
> +	obj[2].relocation_count = 2;
> +	batch = gem_mmap__cpu(device->fd, obj[2].handle, 0, 4096, PROT_WRITE);
> +
> +	i = 0;
> +
> +	if ((tiling | buffer->tiling) >= I915_TILING_Y) {
> +		unsigned int mask;
> +
> +		batch[i++] = MI_LOAD_REGISTER_IMM;
> +		batch[i++] = BCS_SWCTRL;
> +

All batch creation staring from here ^ looks misterious. Maybe we can make
buffer_set_tiling and buffer_linear functions more clearer?
It is hard do read and understand.

> +		mask = (BCS_SRC_Y | BCS_DST_Y) << 16;
> +		if (buffer->tiling == I915_TILING_Y)
> +			mask |= BCS_SRC_Y;
> +		if (tiling == I915_TILING_Y)
> +			mask |= BCS_DST_Y;
> +		batch[i++] = mask;
> +	}
> +
> +	batch[i] = (XY_SRC_COPY_BLT_CMD |
> +		    XY_SRC_COPY_BLT_WRITE_ALPHA |
> +		    XY_SRC_COPY_BLT_WRITE_RGB);
> +	batch[i] |= 6 + 2 * has_64b_reloc;
> +	if (device->gen >= 4 && buffer->tiling)
> +		batch[i] |= XY_SRC_COPY_BLT_SRC_TILED;
> +	if (device->gen >= 4 && tiling)
> +		batch[i] |= XY_SRC_COPY_BLT_DST_TILED;
> +	i++;
> +
> +	pitch = stride;
> +	if (device->gen >= 4 && tiling)
> +		pitch /= 4;
> +	batch[i++] = 3 << 24 | 0xcc << 16 | pitch;
> +	batch[i++] = 0;
> +	batch[i++] = buffer->height << 16 | buffer->width;
> +	reloc[0].target_handle = obj[0].handle;
> +	reloc[0].presumed_offset = obj[0].offset;
> +	reloc[0].offset = sizeof(*batch) * i;
> +	reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> +	batch[i++] = obj[0].offset;
> +	if (has_64b_reloc)
> +		batch[i++] = obj[0].offset >> 32;
> +
> +	batch[i++] = 0;
> +	pitch = buffer->stride;
> +	if (device->gen >= 4 && buffer->tiling)
> +		pitch /= 4;
> +	batch[i++] = pitch;
> +	reloc[1].target_handle = obj[1].handle;
> +	reloc[1].presumed_offset = obj[1].offset;
> +	reloc[1].offset = sizeof(*batch) * i;
> +	reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
> +	batch[i++] = obj[1].offset;
> +	if (has_64b_reloc)
> +		batch[i++] = obj[1].offset >> 32;
> +
> +	if ((tiling | buffer->tiling) >= I915_TILING_Y) {
> +		igt_assert(device->gen >= 6);
> +		batch[i++] = MI_FLUSH_DW | 2;
> +		batch[i++] = 0;
> +		batch[i++] = 0;
> +		batch[i++] = 0;
> +
> +		batch[i++] = MI_LOAD_REGISTER_IMM;
> +		batch[i++] = BCS_SWCTRL;
> +		batch[i++] = (BCS_SRC_Y | BCS_DST_Y) << 16;
> +	}
> +
> +	batch[i++] = MI_BATCH_BUFFER_END;
> +	munmap(batch, 4096);
> +
> +	gem_execbuf(device->fd, &execbuf);
> +
> +	gem_close(device->fd, obj[2].handle);
> +	gem_close(device->fd, obj[1].handle);
> +
> +	buffer->gtt_offset = obj[0].offset;
> +	buffer->handle = obj[0].handle;
> +
> +	buffer->tiling = tiling;
> +	buffer->stride = stride;
> +	buffer->size = size;
> +}
> +
> +enum mode {
> +	CPU,
> +	PRW,
> +	GTT,
> +	WC,
> +};
> +
> +static void blit_to_linear(const struct device *device,
> +			   const struct buffer *buffer,
> +			   void *linear)
> +{
> +	struct drm_i915_gem_exec_object2 obj[3];
> +	struct drm_i915_gem_relocation_entry reloc[2];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	const bool has_64b_reloc = device->gen >= 8;
> +	uint32_t *batch;
> +	uint32_t pitch;
> +	int i = 0;
> +
> +	igt_assert(buffer->tiling);
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	if (device->gen >= 6)
> +		execbuf.flags = I915_EXEC_BLT;
> +
> +	memset(obj, 0, sizeof(obj));
> +	gem_userptr(device->fd, linear, buffer->size, 0, 0, &obj[0].handle);
> +	obj[1].handle = buffer->handle;
> +	obj[1].offset = buffer->gtt_offset;
> +	obj[1].flags = EXEC_OBJECT_NEEDS_FENCE;
> +
> +	memset(reloc, 0, sizeof(reloc));
> +	obj[2].handle = gem_create(device->fd, 4096);
> +	obj[2].relocs_ptr = to_user_pointer(reloc);
> +	obj[2].relocation_count = ARRAY_SIZE(reloc);
> +	batch = gem_mmap__cpu(device->fd, obj[2].handle, 0, 4096, PROT_WRITE);
> +
> +	if (buffer->tiling >= I915_TILING_Y) {
> +		unsigned int mask;
> +
> +		batch[i++] = MI_LOAD_REGISTER_IMM;
> +		batch[i++] = BCS_SWCTRL;
> +
> +		mask = (BCS_SRC_Y | BCS_DST_Y) << 16;
> +		if (buffer->tiling == I915_TILING_Y)
> +			mask |= BCS_SRC_Y;
> +		batch[i++] = mask;
> +	}
> +
> +	batch[i] = (XY_SRC_COPY_BLT_CMD |
> +		    XY_SRC_COPY_BLT_WRITE_ALPHA |
> +		    XY_SRC_COPY_BLT_WRITE_RGB);
> +	if (device->gen >= 4 && buffer->tiling)
> +		batch[i] |= XY_SRC_COPY_BLT_SRC_TILED;
> +
> +	batch[i++] |= 6 + 2 * has_64b_reloc;
> +
> +	batch[i++] = 3 << 24 | 0xcc << 16 | buffer->stride;
> +	batch[i++] = 0;
> +	batch[i++] = buffer->height << 16 | buffer->width;
> +	reloc[0].target_handle = obj[0].handle;
> +	reloc[0].presumed_offset = obj[0].offset;
> +	reloc[0].offset = sizeof(*batch) * i;
> +	reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> +	batch[i++] = obj[0].offset;
> +	if (has_64b_reloc)
> +		batch[i++] = obj[0].offset >> 32;
> +
> +	batch[i++] = 0;
> +	pitch = buffer->stride;
> +	if (device->gen >= 4 && buffer->tiling)
> +		pitch /= 4;
> +	batch[i++] = pitch;
> +	reloc[1].target_handle = obj[1].handle;
> +	reloc[1].presumed_offset = obj[1].offset;
> +	reloc[1].offset = sizeof(*batch) * i;
> +	reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
> +	batch[i++] = obj[1].offset;
> +	if (has_64b_reloc)
> +		batch[i++] = obj[1].offset >> 32;
> +
> +	if (buffer->tiling >= I915_TILING_Y) {
> +		igt_assert(device->gen >= 6);
> +		batch[i++] = MI_FLUSH_DW | 2;
> +		batch[i++] = 0;
> +		batch[i++] = 0;
> +		batch[i++] = 0;
> +
> +		batch[i++] = MI_LOAD_REGISTER_IMM;
> +		batch[i++] = BCS_SWCTRL;
> +		batch[i++] = (BCS_SRC_Y | BCS_DST_Y) << 16;
> +	}
> +
> +	batch[i++] = MI_BATCH_BUFFER_END;
> +	munmap(batch, 4096);
> +
> +	gem_execbuf(device->fd, &execbuf);
> +	gem_close(device->fd, obj[2].handle);
> +
> +	gem_sync(device->fd, obj[0].handle);
> +	gem_close(device->fd, obj[0].handle);
> +}
> +
> +static void *download(const struct device *device,
> +		      const struct buffer *buffer,
> +		      enum mode mode)
> +{
> +	void *linear, *src;
> +
> +	igt_assert(posix_memalign(&linear, 4096, buffer->size) == 0);
> +
> +	switch (mode) {
> +	case CPU:
> +		if (buffer->tiling) {
> +			if (buffer->caching && !device->llc) {
> +				mode = GTT;
> +				break;
> +			}
> +			if (device->gen < 3) {
> +				mode = GTT;
> +				break;
> +			}
Why do we need 2 if-s here ^? Both are setting the same mode.
> +
> +			blit_to_linear(device, buffer, linear);
> +			return linear;
> +		}
> +		break;
> +
> +	case PRW:
> +	case WC:
> +		if (!buffer->tiling)
> +			break;
> +
> +	default:
> +		mode = GTT;
> +		break;
> +	}
> +
> +	switch (mode) {
> +	case CPU:
> +		src = gem_mmap__cpu(device->fd, buffer->handle,
> +				    0, buffer->size,
> +				    PROT_READ);
> +
> +		gem_set_domain(device->fd, buffer->handle,
> +			       I915_GEM_DOMAIN_CPU, 0);
> +		igt_memcpy_from_wc(linear, src, buffer->size);
> +		munmap(src, buffer->size);
> +		break;
> +
> +	case WC:
> +		src = gem_mmap__wc(device->fd, buffer->handle,
> +				   0, buffer->size,
> +				   PROT_READ);
> +
> +		gem_set_domain(device->fd, buffer->handle,
> +			       I915_GEM_DOMAIN_WC, 0);
> +		igt_memcpy_from_wc(linear, src, buffer->size);
> +		munmap(src, buffer->size);
> +		break;
> +
> +	case GTT:
> +		src = gem_mmap__gtt(device->fd, buffer->handle,
> +				   buffer->size,
> +				   PROT_READ);
> +
> +		gem_set_domain(device->fd, buffer->handle,
> +			       I915_GEM_DOMAIN_GTT, 0);
> +		igt_memcpy_from_wc(linear, src, buffer->size);
> +		munmap(src, buffer->size);
> +		break;
> +
> +	case PRW:
> +		gem_read(device->fd, buffer->handle, 0, linear, buffer->size);
> +		break;
> +	}
> +
> +	return linear;
> +}
> +
> +static bool buffer_check(const struct device *device,
> +			 const struct buffer *buffer,
> +			 enum mode mode)
> +{
> +	unsigned int num_errors = 0;
> +	uint32_t *linear;
> +
> +	linear = download(device, buffer, mode);
> +	igt_assert(linear);
> +
> +	for (int y = 0; y < buffer->height; y++) {
> +		const uint32_t *model = buffer->model + y * buffer->width;
> +		const uint32_t *row =
> +			linear + y * buffer->stride / sizeof(uint32_t);
> +
> +		for (int x = 0; x < buffer->width; x++) {
> +			if (row[x] != model[x] && num_errors++ < 5) {
> +				igt_warn("buffer handle=%d mismatch at (%d, %d): expected %08x, found %08x\n",
> +					 buffer->handle,
> +					 x, y, model[x], row[x]);
> +			}
> +		}
> +	}
> +
> +	free(linear);
> +
> +	return num_errors == 0;
> +}
> +
> +static void buffer_free(const struct device *device, struct buffer *buffer)
> +{
> +	igt_assert(buffer_check(device, buffer, GTT));
> +	gem_close(device->fd, buffer->handle);
> +	free(buffer);
> +}
> +
> +static void memcpy_blt(const void *src, void *dst,
> +		       uint32_t src_stride, uint32_t dst_stride,
> +		       uint16_t src_x, uint16_t src_y,
> +		       uint16_t dst_x, uint16_t dst_y,
> +		       uint16_t width, uint16_t height)
> +{
> +	const uint8_t *src_bytes;
> +	uint8_t *dst_bytes;
> +	int byte_width;
> +
> +	src_bytes = (const uint8_t *)src + src_stride * src_y + src_x * 4;
> +	dst_bytes = (uint8_t *)dst + dst_stride * dst_y + dst_x * 4;
> +
> +	byte_width = width * 4;
> +	if (byte_width == src_stride && byte_width == dst_stride) {
> +		byte_width *= height;
> +		height = 1;
> +	}
> +
> +	switch (byte_width) {
> +	case 4:
> +		do {
> +			*(uint32_t *)dst_bytes = *(const uint32_t *)src_bytes;
> +			src_bytes += src_stride;
> +			dst_bytes += dst_stride;
> +		} while (--height);
> +		break;
> +
> +	case 8:
> +		do {
> +			*(uint64_t *)dst_bytes = *(const uint64_t *)src_bytes;
> +			src_bytes += src_stride;
> +			dst_bytes += dst_stride;
> +		} while (--height);
> +		break;
> +	case 16:
> +		do {
> +			((uint64_t *)dst_bytes)[0] = ((const uint64_t *)src_bytes)[0];
> +			((uint64_t *)dst_bytes)[1] = ((const uint64_t *)src_bytes)[1];
> +			src_bytes += src_stride;
> +			dst_bytes += dst_stride;
> +		} while (--height);
> +		break;
> +
> +	default:
> +		do {
> +			memcpy(dst_bytes, src_bytes, byte_width);
> +			src_bytes += src_stride;
> +			dst_bytes += dst_stride;
> +		} while (--height);
> +		break;
> +	}
> +}
> +
> +static void
> +blit(const struct device *device,
> +     struct buffer *src, uint16_t src_x, uint16_t src_y,
> +     struct buffer *dst, uint16_t dst_x, uint16_t dst_y,
> +     uint16_t width, uint16_t height)
> +
> +{
> +	struct drm_i915_gem_exec_object2 obj[3];
> +	struct drm_i915_gem_relocation_entry reloc[2];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	const bool has_64b_reloc = device->gen >= 8;
> +	uint32_t *batch;
> +	uint32_t pitch;
> +	int i = 0;
> +
> +	if (src_x < 0) {
> +		width += src_x;
> +		dst_x -= src_x;
> +		src_x = 0;
> +	}
> +	if (src_y < 0) {
> +		height += src_y;
> +		dst_y  -= src_y;
> +		src_y = 0;
> +	}
> +
> +	if (dst_x < 0) {
> +		width += dst_x;
> +		src_x -= dst_x;
> +		dst_x = 0;
> +	}
> +	if (dst_y < 0) {
> +		height += dst_y;
> +		src_y  -= dst_y;
> +		dst_y = 0;
> +	}
> +
> +	if (src_x + width > src->width)
> +		width = src->width - src_x;
> +	if (dst_x + width > dst->width)
> +		width = dst->width - dst_x;
> +
> +	if (src_y + height > src->height)
> +		height = src->height - src_y;
> +	if (dst_y + height > dst->height)
> +		height = dst->height - dst_y;
> +
> +	if (dst->caching) {
> +		igt_assert(device->gen >= 3);
> +		igt_assert(device->llc || !src->caching);
> +	}
> +
How did you calculated this values above ^ ?

> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	if (device->gen >= 6)
> +		execbuf.flags = I915_EXEC_BLT;
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[0].handle = dst->handle;
> +	obj[0].offset = dst->gtt_offset;
> +	if (dst->tiling)
> +		obj[0].flags = EXEC_OBJECT_NEEDS_FENCE;
> +
> +	obj[1].handle = src->handle;
> +	obj[1].offset = src->gtt_offset;
> +	if (src->tiling)
> +		obj[1].flags = EXEC_OBJECT_NEEDS_FENCE;
> +
> +	memset(reloc, 0, sizeof(reloc));
> +	obj[2].handle = gem_create(device->fd, 4096);
> +	obj[2].relocs_ptr = to_user_pointer(reloc);
> +	obj[2].relocation_count = ARRAY_SIZE(reloc);
> +	batch = gem_mmap__cpu(device->fd, obj[2].handle, 0, 4096, PROT_WRITE);
> +
> +	if ((src->tiling | dst->tiling) >= I915_TILING_Y) {
> +		unsigned int mask;
> +
> +		batch[i++] = MI_LOAD_REGISTER_IMM;
> +		batch[i++] = BCS_SWCTRL;
> +
> +		mask = (BCS_SRC_Y | BCS_DST_Y) << 16;
> +		if (src->tiling == I915_TILING_Y)
> +			mask |= BCS_SRC_Y;
> +		if (dst->tiling == I915_TILING_Y)
> +			mask |= BCS_DST_Y;
> +		batch[i++] = mask;
> +	}
> +
> +	batch[i] = (XY_SRC_COPY_BLT_CMD |
> +		    XY_SRC_COPY_BLT_WRITE_ALPHA |
> +		    XY_SRC_COPY_BLT_WRITE_RGB);
> +	batch[i] |= 6 + 2 * has_64b_reloc;
> +	if (device->gen >= 4 && src->tiling)
> +		batch[i] |= XY_SRC_COPY_BLT_SRC_TILED;
> +	if (device->gen >= 4 && dst->tiling)
> +		batch[i] |= XY_SRC_COPY_BLT_DST_TILED;
> +	i++;
> +
> +	pitch = dst->stride;
> +	if (device->gen >= 4 && dst->tiling)
> +		pitch /= 4;
> +	batch[i++] = 3 << 24 | 0xcc << 16 | pitch;
> +
> +	batch[i++] = dst_y << 16 | dst_x;
> +	batch[i++] = (height + dst_y) << 16 | (width + dst_x);
> +	reloc[0].target_handle = obj[0].handle;
> +	reloc[0].presumed_offset = obj[0].offset;
> +	reloc[0].offset = sizeof(*batch) * i;
> +	reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> +	batch[i++] = obj[0].offset;
> +	if (has_64b_reloc)
> +		batch[i++] = obj[0].offset >> 32;
> +
> +	batch[i++] = src_y << 16 | src_x;
> +	pitch = src->stride;
> +	if (device->gen >= 4 && src->tiling)
> +		pitch /= 4;
> +	batch[i++] = pitch;
> +	reloc[1].target_handle = obj[1].handle;
> +	reloc[1].presumed_offset = obj[1].offset;
> +	reloc[1].offset = sizeof(*batch) * i;
> +	reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
> +	batch[i++] = obj[1].offset;
> +	if (has_64b_reloc)
> +		batch[i++] = obj[1].offset >> 32;
> +
> +	if ((src->tiling | dst->tiling) >= I915_TILING_Y) {
> +		igt_assert(device->gen >= 6);
> +		batch[i++] = MI_FLUSH_DW | 2;
> +		batch[i++] = 0;
> +		batch[i++] = 0;
> +		batch[i++] = 0;
> +
> +		batch[i++] = MI_LOAD_REGISTER_IMM;
> +		batch[i++] = BCS_SWCTRL;
> +		batch[i++] = (BCS_SRC_Y | BCS_DST_Y) << 16;
> +	}
> +
> +	batch[i++] = MI_BATCH_BUFFER_END;
> +	munmap(batch, 4096);
> +
> +	gem_execbuf(device->fd, &execbuf);
> +	gem_close(device->fd, obj[2].handle);
> +
> +	dst->gtt_offset = obj[0].offset;
> +	src->gtt_offset = obj[1].offset;
> +
> +	memcpy_blt(src->model, dst->model,
> +		   4u * src->width, 4u * dst->width,
> +		   src_x, src_y,
> +		   dst_x, dst_y,
> +		   width, height);
> +}
> +
> +igt_main
> +{
> +	struct device device;
> +
> +	igt_fixture {
> +		device.fd = drm_open_driver_render(DRIVER_INTEL);
> +		igt_require_gem(device.fd);
> +
> +		device.pciid = intel_get_drm_devid(device.fd);
> +		device.gen = intel_gen(device.pciid);
> +		device.llc = gem_has_llc(device.fd);
> +	}
> +
> +	igt_subtest("basic") {
> +		struct buffer *src, *dst;
> +		unsigned int x, y;
> +
> +		for (unsigned int height = 1; height <= 16; height <<= 1) {
> +			for (unsigned int width = 1; width <= 64; width <<= 1) {
> +				src = buffer_create(&device,
> +						    width * 16, height * 4);
> +				dst = buffer_create(&device,
> +						    width * 16, height * 4);
> +
> +				y = 0;
> +				for (unsigned int src_tiling = I915_TILING_NONE;
> +				     src_tiling <= (device.gen >= 6 ? I915_TILING_Y : I915_TILING_X);
> +				     src_tiling++) {
> +					buffer_set_tiling(&device, src, src_tiling);
> +
> +					x = 0;
> +					for (unsigned int dst_tiling = I915_TILING_NONE;
> +					     dst_tiling <= (device.gen >= 6 ? I915_TILING_Y : I915_TILING_X);
> +					     dst_tiling++) {
> +						buffer_set_tiling(&device, dst, dst_tiling);
> +
> +						for (enum mode down = CPU; down <= WC; down++) {
> +							igt_debug("Testing src_tiling=%d, dst_tiling=%d, down=%d at (%d, %d) x (%d, %d)\n",
> +								  src_tiling,
> +								  dst_tiling,
> +								  down, x, y,
> +								  width, height);
> +
> +							igt_assert(x + width <= dst->width);
> +							igt_assert(y + height <= dst->height);
> +
> +							blit(&device,
> +							     src, x, y,
> +							     dst, x, y,
> +							     width, height);
> +							igt_assert(buffer_check(&device, dst, down));
> +
> +							x += width;
> +						}
> +					}
> +
> +					y += height;
> +				}
> +
> +				buffer_free(&device, dst);
> +				buffer_free(&device, src);
> +			}
> +		}
> +	}
> +}

Whole test with all helper functions seems to be not clear enough. 
I would prefer to have more comments of what is going on in the code,
especially in creating batches.

General question - does this test gives additional coverge or will be
a substitue for some existing?

Kasia :)

> diff --git a/tests/meson.build b/tests/meson.build
> index 32c2156c6..becadfa8d 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -18,6 +18,7 @@ test_progs = [
>  	'drv_suspend',
>  	'gem_bad_reloc',
>  	'gem_basic',
> +	'gem_blits',
>  	'gem_busy',
>  	'gem_caching',
>  	'gem_close',
> -- 
> 2.18.0
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] igt: Another combinatorial exercise for blits

[Chris Wilson]

Quoting Katarzyna Dec (2018-08-01 15:47:39)
> On Wed, Jul 25, 2018 at 10:38:23PM +0100, Chris Wilson wrote:
> > +static unsigned int
> > +get_tiling_stride(const struct device *device,
> > +               unsigned int width, unsigned int tiling)
> > +{
> > +     unsigned int stride = 4u * width;
> > +
> > +     if (tiling) {
> > +             if (device->gen < 3)
> > +                     stride = ALIGN(stride, 128);
> > +             else if (device->gen < 4 || tiling == I915_TILING_X)
> > +                     stride = ALIGN(stride, 512);
> > +             else
> > +                     stride = ALIGN(stride, 128);
> > +             if (device->gen < 4)
> > +                     stride = 1 << fls(stride - 1);
> Shouldn't 'else' be last here ^? What about order of 'else if' and 'if'
> at the end?

First align to the stride, then round up to a pot for old gen. It can be
done either away around; the rule is just it has to be both a multiple
of tile_width and a power of two.

> > +     } else {
> > +             if (device->gen >= 8)
> > +                     stride = ALIGN(stride, 64);

This is one that is more dubious as it papering over a hw bump that is a
bit more subtle. After all part of the reason for this test is to detect
errors like that.

> > +     if ((tiling | buffer->tiling) >= I915_TILING_Y) {
> > +             unsigned int mask;
> > +
> > +             batch[i++] = MI_LOAD_REGISTER_IMM;
> > +             batch[i++] = BCS_SWCTRL;
> > +
> 
> All batch creation staring from here ^ looks misterious. Maybe we can make
> buffer_set_tiling and buffer_linear functions more clearer?
> It is hard do read and understand.

About the only thing I might touch is the lri. But I am not that
convinced it's worth it in the grand scheme.

> > +static void *download(const struct device *device,
> > +                   const struct buffer *buffer,
> > +                   enum mode mode)
> > +{
> > +     void *linear, *src;
> > +
> > +     igt_assert(posix_memalign(&linear, 4096, buffer->size) == 0);
> > +
> > +     switch (mode) {
> > +     case CPU:
> > +             if (buffer->tiling) {
> > +                     if (buffer->caching && !device->llc) {
> > +                             mode = GTT;
> > +                             break;
> > +                     }
> > +                     if (device->gen < 3) {
> > +                             mode = GTT;
> > +                             break;
> > +                     }
> Why do we need 2 if-s here ^? Both are setting the same mode.

Semantics, they are for different reasons, but related reasons.
Not allowed to copy between snooped surfaces; and gen2 doesn't allow
copy to snoop, just copy from snoop.

> > +     if (dst->caching) {
> > +             igt_assert(device->gen >= 3);
> > +             igt_assert(device->llc || !src->caching);
> > +     }
> > +
> How did you calculated this values above ^ ?

^^

> Whole test with all helper functions seems to be not clear enough. 
> I would prefer to have more comments of what is going on in the code,
> especially in creating batches.

There isn't much going on, no tricks, just straight forward XY_SRC_COPY.
 
> General question - does this test gives additional coverge or will be
> a substitue for some existing?

Both more and less at the moment. There's better testing for the copy
and access patterns, but less around the limits of the GTT. But atm we
only do the sanitychecks in CI, so from that perspective this is much
more complete testing of things like pixel/cacheline/page access between
tiling modes.

The challenge I have in mind is how to let it explore by itself. A small
bytecode to try out different combinations of state is one way, but that
only covers the primitives we have modelled and therein lies the rub.
How to go completely crazy yet still detect error (other than oopses).
-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] igt: Another combinatorial exercise for blits

[Katarzyna Dec]

On Wed, Aug 01, 2018 at 04:10:07PM +0100, Chris Wilson wrote:
> Quoting Katarzyna Dec (2018-08-01 15:47:39)
> > On Wed, Jul 25, 2018 at 10:38:23PM +0100, Chris Wilson wrote:
> > > +static unsigned int
> > > +get_tiling_stride(const struct device *device,
> > > +               unsigned int width, unsigned int tiling)
> > > +{
> > > +     unsigned int stride = 4u * width;
> > > +
> > > +     if (tiling) {
                    /* align to the stride */
> > > +             if (device->gen < 3)
> > > +                     stride = ALIGN(stride, 128);
> > > +             else if (device->gen < 4 || tiling == I915_TILING_X)
> > > +                     stride = ALIGN(stride, 512);
> > > +             else
> > > +                     stride = ALIGN(stride, 128);
                    /* round up to a pot for old gen */
> > > +             if (device->gen < 4)
> > > +                     stride = 1 << fls(stride - 1);
> > Shouldn't 'else' be last here ^? What about order of 'else if' and 'if'
> > at the end?
> 
> First align to the stride, then round up to a pot for old gen. It can be
> done either away around; the rule is just it has to be both a multiple
> of tile_width and a power of two.
>
Now I understand. Maybe it would be worth to add some comment about that
in the code? It would be much more clear for everybody.
> > > +     } else {
> > > +             if (device->gen >= 8)
> > > +                     stride = ALIGN(stride, 64);
> 
> This is one that is more dubious as it papering over a hw bump that is a
> bit more subtle. After all part of the reason for this test is to detect
> errors like that.
> 
> > > +     if ((tiling | buffer->tiling) >= I915_TILING_Y) {
> > > +             unsigned int mask;
> > > +
> > > +             batch[i++] = MI_LOAD_REGISTER_IMM;
> > > +             batch[i++] = BCS_SWCTRL;
> > > +
> > 
> > All batch creation staring from here ^ looks misterious. Maybe we can make
> > buffer_set_tiling and buffer_linear functions more clearer?
> > It is hard do read and understand.
> 
> About the only thing I might touch is the lri. But I am not that
> convinced it's worth it in the grand scheme.
>
Some documentation is better then no documentation. I know that code should be
it's own judge, but you need to have deep understanding in driver code to get
what is going on with creating these batches.
And by docs I do not mean every super formatted detail, just small comments
inline comments.

> > > +static void *download(const struct device *device,
> > > +                   const struct buffer *buffer,
> > > +                   enum mode mode)
> > > +{
> > > +     void *linear, *src;
> > > +
> > > +     igt_assert(posix_memalign(&linear, 4096, buffer->size) == 0);
> > > +
> > > +     switch (mode) {
> > > +     case CPU:
> > > +             if (buffer->tiling) {
> > > +                     if (buffer->caching && !device->llc) {
> > > +                             mode = GTT;
> > > +                             break;
> > > +                     }
> > > +                     if (device->gen < 3) {
> > > +                             mode = GTT;
> > > +                             break;
> > > +                     }
> > Why do we need 2 if-s here ^? Both are setting the same mode.
> 
> Semantics, they are for different reasons, but related reasons.
> Not allowed to copy between snooped surfaces; and gen2 doesn't allow
> copy to snoop, just copy from snoop.
Same as above, it would be nice to heve even one line comment here.
> 
> > > +     if (dst->caching) {
> > > +             igt_assert(device->gen >= 3);
> > > +             igt_assert(device->llc || !src->caching);
> > > +     }
> > > +
> > How did you calculated this values above ^ ?
> 
> ^^
> 
> > Whole test with all helper functions seems to be not clear enough. 
> > I would prefer to have more comments of what is going on in the code,
> > especially in creating batches.
> 
> There isn't much going on, no tricks, just straight forward XY_SRC_COPY.
>
It would be nice if you introduce some one-line comments in few places. Maybe
code is simple, but it doesn't look like that.

Kasia :)
> > General question - does this test gives additional coverge or will be
> > a substitue for some existing?
> 
> Both more and less at the moment. There's better testing for the copy
> and access patterns, but less around the limits of the GTT. But atm we
> only do the sanitychecks in CI, so from that perspective this is much
> more complete testing of things like pixel/cacheline/page access between
> tiling modes.
> 
> The challenge I have in mind is how to let it explore by itself. A small
> bytecode to try out different combinations of state is one way, but that
> only covers the primitives we have modelled and therein lies the rub.
> How to go completely crazy yet still detect error (other than oopses).
> -Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev