[PATCH 1/4] drm/i915: Replace some open-coded i915_map_coherent_type()

[PATCH 1/4] drm/i915: Replace some open-coded i915_map_coherent_type()

[Chris Wilson]

A few callsites where deciding on using WC or WB maps based on
HAS_LLC(), so replace them with the equivalent helper function
i915_map_coherent_type().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c          | 3 +--
 drivers/gpu/drm/i915/selftests/intel_hangcheck.c | 4 ++--
 drivers/gpu/drm/i915/selftests/intel_lrc.c       | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c092d5099ebf..b8a7a014d46d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1023,8 +1023,7 @@ i915_emit_bb_start(struct i915_request *rq,
 int intel_ring_pin(struct intel_ring *ring)
 {
 	struct i915_vma *vma = ring->vma;
-	enum i915_map_type map =
-		HAS_LLC(vma->vm->i915) ? I915_MAP_WB : I915_MAP_WC;
+	enum i915_map_type map = i915_coherent_map_type(vma->vm->i915);
 	unsigned int flags;
 	void *addr;
 	int ret;
diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
index db378226ac10..51d0e2bed9e1 100644
--- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
@@ -76,7 +76,7 @@ static int hang_init(struct hang *h, struct drm_i915_private *i915)
 	h->seqno = memset(vaddr, 0xff, PAGE_SIZE);
 
 	vaddr = i915_gem_object_pin_map(h->obj,
-					HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC);
+					i915_coherent_map_type(i915));
 	if (IS_ERR(vaddr)) {
 		err = PTR_ERR(vaddr);
 		goto err_unpin_hws;
@@ -234,7 +234,7 @@ hang_create_request(struct hang *h, struct intel_engine_cs *engine)
 			return ERR_CAST(obj);
 
 		vaddr = i915_gem_object_pin_map(obj,
-						HAS_LLC(h->i915) ? I915_MAP_WB : I915_MAP_WC);
+						i915_coherent_map_type(h->i915));
 		if (IS_ERR(vaddr)) {
 			i915_gem_object_put(obj);
 			return ERR_CAST(vaddr);
diff --git a/drivers/gpu/drm/i915/selftests/intel_lrc.c b/drivers/gpu/drm/i915/selftests/intel_lrc.c
index c8b7f03c35bd..9f241d1c72db 100644
--- a/drivers/gpu/drm/i915/selftests/intel_lrc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_lrc.c
@@ -49,7 +49,7 @@ static int spinner_init(struct spinner *spin, struct drm_i915_private *i915)
 	}
 	spin->seqno = memset(vaddr, 0xff, PAGE_SIZE);
 
-	mode = HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
+	mode = i915_coherent_map_type(i915);
 	vaddr = i915_gem_object_pin_map(spin->obj, mode);
 	if (IS_ERR(vaddr)) {
 		err = PTR_ERR(vaddr);
-- 
2.19.0

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

[PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

The final call to zlib_deflate(Z_FINISH) may require more output
space to be allocated and so needs to re-invoked. Failure to do so in
the current code leads to incomplete zlib streams (albeit intact due to
the use of Z_SYNC_FLUSH) resulting in the occasional short object
capture.

Testcase: igt/i915-error-capture.js
Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 3d5554f14dfd..ed8c16cbfaa4 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
 			 struct drm_i915_error_object *dst)
 {
 	struct z_stream_s *zstream = &c->zstream;
+	int flush = Z_NO_FLUSH;
 
 	zstream->next_in = src;
 	if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
@@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
 			zstream->avail_out = PAGE_SIZE;
 		}
 
-		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
+		if (zlib_deflate(zstream, flush) != Z_OK)
 			return -EIO;
+
+		if (zstream->avail_out)
+			flush = Z_SYNC_FLUSH;
 	} while (zstream->avail_in);
 
 	/* Fallback to uncompressed if we increase size? */
@@ -268,19 +272,43 @@ static int compress_page(struct compress *c,
 	return 0;
 }
 
-static void compress_fini(struct compress *c,
+static int compress_flush(struct compress *c,
 			  struct drm_i915_error_object *dst)
 {
 	struct z_stream_s *zstream = &c->zstream;
+	unsigned long page;
 
-	if (dst) {
-		zlib_deflate(zstream, Z_FINISH);
-		dst->unused = zstream->avail_out;
-	}
+	do {
+		switch (zlib_deflate(zstream, Z_FINISH)) {
+		case Z_OK: /* more space requested */
+			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
+			if (!page)
+				return -ENOMEM;
+
+			dst->pages[dst->page_count++] = (void *)page;
+			zstream->next_out = (void *)page;
+			zstream->avail_out = PAGE_SIZE;
+			break;
+		case Z_STREAM_END:
+			goto end;
+		default: /* any error */
+			return -EIO;
+		}
+	} while (1);
+
+end:
+	memset(zstream->next_out, 0, zstream->avail_out);
+	dst->unused = zstream->avail_out;
+	return 0;
+}
+
+static void compress_fini(struct compress *c,
+			  struct drm_i915_error_object *dst)
+{
+	struct z_stream_s *zstream = &c->zstream;
 
 	zlib_deflateEnd(zstream);
 	kfree(zstream->workspace);
-
 	if (c->tmp)
 		free_page((unsigned long)c->tmp);
 }
@@ -319,6 +347,12 @@ static int compress_page(struct compress *c,
 	return 0;
 }
 
+static int compress_flush(struct compress *c,
+			  struct drm_i915_error_object *dst)
+{
+	return 0;
+}
+
 static void compress_fini(struct compress *c,
 			  struct drm_i915_error_object *dst)
 {
@@ -951,15 +985,15 @@ i915_error_object_create(struct drm_i915_private *i915,
 		if (ret)
 			goto unwind;
 	}
-	goto out;
 
+	if (compress_flush(&compress, dst)) {
 unwind:
-	while (dst->page_count--)
-		free_page((unsigned long)dst->pages[dst->page_count]);
-	kfree(dst);
-	dst = NULL;
+		while (dst->page_count--)
+			free_page((unsigned long)dst->pages[dst->page_count]);
+		kfree(dst);
+		dst = NULL;
+	}
 
-out:
 	compress_fini(&compress, dst);
 	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
 	return dst;
-- 
2.19.0

[PATCH 3/4] drm/i915: Clear the error PTE just once on finish

[Chris Wilson]

We do not need to continually clear our dedicated PTE for error capture
as it will be updated and invalidated to the next object. Only at the
end do we wish to be sure that the PTE doesn't point back to any buffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index ed8c16cbfaa4..fed9574be5b6 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -995,7 +995,6 @@ i915_error_object_create(struct drm_i915_private *i915,
 	}
 
 	compress_fini(&compress, dst);
-	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
 	return dst;
 }
 
@@ -1781,6 +1780,14 @@ static unsigned long capture_find_epoch(const struct i915_gpu_state *error)
 	return epoch;
 }
 
+static void capture_finish(struct i915_gpu_state *error)
+{
+	struct i915_ggtt *ggtt = &error->i915->ggtt;
+	const u64 slot = ggtt->error_capture.start;
+
+	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
+}
+
 static int capture(void *data)
 {
 	struct i915_gpu_state *error = data;
@@ -1805,6 +1812,7 @@ static int capture(void *data)
 
 	error->epoch = capture_find_epoch(error);
 
+	capture_finish(error);
 	return 0;
 }
 
-- 
2.19.0

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

[PATCH 4/4] drm/i915: Cache the error string

[Chris Wilson]

Currently, we convert the error state into a string every time we read
from sysfs (and sysfs reads in page size (4KiB) chunks). We do try to
window the string and only capture the portion that is being read, but
that means that we must always convert up to the window to find the
start. For a very large error state bordering on EXEC_OBJECT_CAPTURE
abuse, this is noticeable as it degrades to O(N^2)!

As we do not have a convenient hook for sysfs open(), and we would like
to keep the lazy conversion into a string, do the conversion of the
whole string on the first read and keep the string until the error state
is freed.

v2: Don't double advance simple_read_from_buffer
v3: Due to extreme pain of lack of vrealloc, use a scatterlist
v4: Keep the forward iterator loosely cached

Reported-by: Jason Ekstrand <jason@jlekstrand.net>
Testcase: igt/gem_exec_capture/many*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c   |  32 +-
 drivers/gpu/drm/i915/i915_gpu_error.c | 402 +++++++++++++++-----------
 drivers/gpu/drm/i915/i915_gpu_error.h |  28 +-
 drivers/gpu/drm/i915/i915_sysfs.c     |  27 +-
 4 files changed, 273 insertions(+), 216 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b4744a68cd88..2ac75bc10afa 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -943,30 +943,28 @@ static int i915_gem_fence_regs_info(struct seq_file *m, void *data)
 static ssize_t gpu_state_read(struct file *file, char __user *ubuf,
 			      size_t count, loff_t *pos)
 {
-	struct i915_gpu_state *error = file->private_data;
-	struct drm_i915_error_state_buf str;
+	struct i915_gpu_state *error;
 	ssize_t ret;
-	loff_t tmp;
+	void *buf;
 
+	error = file->private_data;
 	if (!error)
 		return 0;
 
-	ret = i915_error_state_buf_init(&str, error->i915, count, *pos);
-	if (ret)
-		return ret;
-
-	ret = i915_error_state_to_str(&str, error);
-	if (ret)
-		goto out;
+	/* Bounce buffer required because of kernfs __user API convenience. */
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
-	tmp = 0;
-	ret = simple_read_from_buffer(ubuf, count, &tmp, str.buf, str.bytes);
-	if (ret < 0)
-		goto out;
+	ret = i915_gpu_state_copy_to_buffer(error, buf, *pos, count);
+	if (ret > 0) {
+		if (!copy_to_user(ubuf, buf, ret))
+			*pos += ret;
+		else
+			ret = -EFAULT;
+	}
 
-	*pos = str.start + ret;
-out:
-	i915_error_state_buf_release(&str);
+	kfree(buf);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index fed9574be5b6..259f0f98a012 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -28,6 +28,8 @@
  */
 
 #include <generated/utsrelease.h>
+#include <linux/nmi.h>
+#include <linux/scatterlist.h>
 #include <linux/stop_machine.h>
 #include <linux/zlib.h>
 #include <drm/drm_print.h>
@@ -77,112 +79,110 @@ static const char *purgeable_flag(int purgeable)
 	return purgeable ? " purgeable" : "";
 }
 
-static bool __i915_error_ok(struct drm_i915_error_state_buf *e)
+static void __sg_set_buf(struct scatterlist *sg,
+			 void *addr, unsigned int len, loff_t it)
 {
-
-	if (!e->err && WARN(e->bytes > (e->size - 1), "overflow")) {
-		e->err = -ENOSPC;
-		return false;
-	}
-
-	if (e->bytes == e->size - 1 || e->err)
-		return false;
-
-	return true;
+	sg->page_link = (unsigned long)virt_to_page(addr);
+	sg->offset = offset_in_page(addr);
+	sg->length = len;
+	sg->dma_address = it;
 }
 
-static bool __i915_error_seek(struct drm_i915_error_state_buf *e,
-			      unsigned len)
+static bool __i915_error_grow(struct drm_i915_error_state_buf *e, size_t len)
 {
-	if (e->pos + len <= e->start) {
-		e->pos += len;
+	if (!len)
 		return false;
-	}
 
-	/* First vsnprintf needs to fit in its entirety for memmove */
-	if (len >= e->size) {
-		e->err = -EIO;
-		return false;
-	}
+	if (e->bytes + len + 1 > e->size) {
+		if (e->bytes) {
+			__sg_set_buf(e->cur++, e->buf, e->bytes, e->iter);
+			e->iter += e->bytes;
+			e->buf = NULL;
+			e->bytes = 0;
+		}
 
-	return true;
-}
+		if (e->cur == e->end) {
+			struct scatterlist *sgl;
 
-static void __i915_error_advance(struct drm_i915_error_state_buf *e,
-				 unsigned len)
-{
-	/* If this is first printf in this window, adjust it so that
-	 * start position matches start of the buffer
-	 */
+			sgl = (typeof(sgl))__get_free_page(GFP_KERNEL);
+			if (!sgl) {
+				e->err = -ENOMEM;
+				return false;
+			}
 
-	if (e->pos < e->start) {
-		const size_t off = e->start - e->pos;
+			if (e->cur) {
+				e->cur->offset = 0;
+				e->cur->length = 0;
+				e->cur->page_link =
+					(unsigned long)sgl | SG_CHAIN;
+			} else {
+				e->sgl = sgl;
+			}
 
-		/* Should not happen but be paranoid */
-		if (off > len || e->bytes) {
-			e->err = -EIO;
-			return;
+			e->cur = sgl;
+			e->end = sgl + SG_MAX_SINGLE_ALLOC - 1;
 		}
 
-		memmove(e->buf, e->buf + off, len - off);
-		e->bytes = len - off;
-		e->pos = e->start;
-		return;
+		e->size = ALIGN(len + 1, SZ_64K);
+		e->buf = kmalloc(e->size,
+				 GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+		if (!e->buf) {
+			e->size = PAGE_ALIGN(len + 1);
+			e->buf = kmalloc(e->size, GFP_KERNEL);
+		}
+		if (!e->buf) {
+			e->err = -ENOMEM;
+			return false;
+		}
 	}
 
-	e->bytes += len;
-	e->pos += len;
+	return true;
 }
 
 __printf(2, 0)
 static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
-			       const char *f, va_list args)
+			       const char *fmt, va_list args)
 {
-	unsigned len;
+	va_list ap;
+	int len;
 
-	if (!__i915_error_ok(e))
+	if (e->err)
 		return;
 
-	/* Seek the first printf which is hits start position */
-	if (e->pos < e->start) {
-		va_list tmp;
-
-		va_copy(tmp, args);
-		len = vsnprintf(NULL, 0, f, tmp);
-		va_end(tmp);
-
-		if (!__i915_error_seek(e, len))
-			return;
+	va_copy(ap, args);
+	len = vsnprintf(NULL, 0, fmt, ap);
+	va_end(ap);
+	if (len <= 0) {
+		e->err = len;
+		return;
 	}
 
-	len = vsnprintf(e->buf + e->bytes, e->size - e->bytes, f, args);
-	if (len >= e->size - e->bytes)
-		len = e->size - e->bytes - 1;
+	if (!__i915_error_grow(e, len))
+		return;
 
-	__i915_error_advance(e, len);
+	GEM_BUG_ON(e->bytes >= e->size);
+	len = vscnprintf(e->buf + e->bytes, e->size - e->bytes, fmt, args);
+	if (len < 0) {
+		e->err = len;
+		return;
+	}
+	e->bytes += len;
 }
 
-static void i915_error_puts(struct drm_i915_error_state_buf *e,
-			    const char *str)
+static void i915_error_puts(struct drm_i915_error_state_buf *e, const char *str)
 {
 	unsigned len;
 
-	if (!__i915_error_ok(e))
+	if (e->err || !str)
 		return;
 
 	len = strlen(str);
+	if (!__i915_error_grow(e, len))
+		return;
 
-	/* Seek the first printf which is hits start position */
-	if (e->pos < e->start) {
-		if (!__i915_error_seek(e, len))
-			return;
-	}
-
-	if (len >= e->size - e->bytes)
-		len = e->size - e->bytes - 1;
+	GEM_BUG_ON(e->bytes + len > e->size);
 	memcpy(e->buf + e->bytes, str, len);
-
-	__i915_error_advance(e, len);
+	e->bytes += len;
 }
 
 #define err_printf(e, ...) i915_error_printf(e, __VA_ARGS__)
@@ -263,6 +263,8 @@ static int compress_page(struct compress *c,
 
 		if (zstream->avail_out)
 			flush = Z_SYNC_FLUSH;
+
+		touch_nmi_watchdog();
 	} while (zstream->avail_in);
 
 	/* Fallback to uncompressed if we increase size? */
@@ -631,33 +633,50 @@ static void err_print_uc(struct drm_i915_error_state_buf *m,
 	print_error_obj(m, NULL, "GuC log buffer", error_uc->guc_log);
 }
 
-int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
-			    const struct i915_gpu_state *error)
+static void err_free_sgl(struct scatterlist *sgl)
 {
-	struct drm_i915_private *dev_priv = m->i915;
+	while (sgl) {
+		struct scatterlist *sg;
+
+		for (sg = sgl; !sg_is_chain(sg); sg++) {
+			kfree(sg_virt(sg));
+			if (sg_is_last(sg))
+				break;
+		}
+
+		sg = sg_is_last(sg) ? NULL : sg_chain_ptr(sg);
+		free_page((unsigned long)sgl);
+		sgl = sg;
+	}
+}
+
+static int err_print_to_sgl(struct i915_gpu_state *error)
+{
+	struct drm_i915_error_state_buf m;
 	struct drm_i915_error_object *obj;
 	struct timespec64 ts;
 	int i, j;
 
-	if (!error) {
-		err_printf(m, "No error state collected\n");
+	if (READ_ONCE(error->sgl))
 		return 0;
-	}
+
+	memset(&m, 0, sizeof(m));
+	m.i915 = error->i915;
 
 	if (*error->error_msg)
-		err_printf(m, "%s\n", error->error_msg);
-	err_printf(m, "Kernel: " UTS_RELEASE "\n");
+		err_printf(&m, "%s\n", error->error_msg);
+	err_printf(&m, "Kernel: " UTS_RELEASE "\n");
 	ts = ktime_to_timespec64(error->time);
-	err_printf(m, "Time: %lld s %ld us\n",
+	err_printf(&m, "Time: %lld s %ld us\n",
 		   (s64)ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC);
 	ts = ktime_to_timespec64(error->boottime);
-	err_printf(m, "Boottime: %lld s %ld us\n",
+	err_printf(&m, "Boottime: %lld s %ld us\n",
 		   (s64)ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC);
 	ts = ktime_to_timespec64(error->uptime);
-	err_printf(m, "Uptime: %lld s %ld us\n",
+	err_printf(&m, "Uptime: %lld s %ld us\n",
 		   (s64)ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC);
-	err_printf(m, "Epoch: %lu jiffies (%u HZ)\n", error->epoch, HZ);
-	err_printf(m, "Capture: %lu jiffies; %d ms ago, %d ms after epoch\n",
+	err_printf(&m, "Epoch: %lu jiffies (%u HZ)\n", error->epoch, HZ);
+	err_printf(&m, "Capture: %lu jiffies; %d ms ago, %d ms after epoch\n",
 		   error->capture,
 		   jiffies_to_msecs(jiffies - error->capture),
 		   jiffies_to_msecs(error->capture - error->epoch));
@@ -665,63 +684,63 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	for (i = 0; i < ARRAY_SIZE(error->engine); i++) {
 		if (error->engine[i].hangcheck_stalled &&
 		    error->engine[i].context.pid) {
-			err_printf(m, "Active process (on ring %s): %s [%d], score %d%s\n",
-				   engine_name(m->i915, i),
+			err_printf(&m, "Active process (on ring %s): %s [%d], score %d%s\n",
+				   engine_name(m.i915, i),
 				   error->engine[i].context.comm,
 				   error->engine[i].context.pid,
 				   error->engine[i].context.ban_score,
 				   bannable(&error->engine[i].context));
 		}
 	}
-	err_printf(m, "Reset count: %u\n", error->reset_count);
-	err_printf(m, "Suspend count: %u\n", error->suspend_count);
-	err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform));
-	err_print_pciid(m, error->i915);
+	err_printf(&m, "Reset count: %u\n", error->reset_count);
+	err_printf(&m, "Suspend count: %u\n", error->suspend_count);
+	err_printf(&m, "Platform: %s\n", intel_platform_name(error->device_info.platform));
+	err_print_pciid(&m, m.i915);
 
-	err_printf(m, "IOMMU enabled?: %d\n", error->iommu);
+	err_printf(&m, "IOMMU enabled?: %d\n", error->iommu);
 
-	if (HAS_CSR(dev_priv)) {
-		struct intel_csr *csr = &dev_priv->csr;
+	if (HAS_CSR(m.i915)) {
+		struct intel_csr *csr = &m.i915->csr;
 
-		err_printf(m, "DMC loaded: %s\n",
+		err_printf(&m, "DMC loaded: %s\n",
 			   yesno(csr->dmc_payload != NULL));
-		err_printf(m, "DMC fw version: %d.%d\n",
+		err_printf(&m, "DMC fw version: %d.%d\n",
 			   CSR_VERSION_MAJOR(csr->version),
 			   CSR_VERSION_MINOR(csr->version));
 	}
 
-	err_printf(m, "GT awake: %s\n", yesno(error->awake));
-	err_printf(m, "RPM wakelock: %s\n", yesno(error->wakelock));
-	err_printf(m, "PM suspended: %s\n", yesno(error->suspended));
-	err_printf(m, "EIR: 0x%08x\n", error->eir);
-	err_printf(m, "IER: 0x%08x\n", error->ier);
+	err_printf(&m, "GT awake: %s\n", yesno(error->awake));
+	err_printf(&m, "RPM wakelock: %s\n", yesno(error->wakelock));
+	err_printf(&m, "PM suspended: %s\n", yesno(error->suspended));
+	err_printf(&m, "EIR: 0x%08x\n", error->eir);
+	err_printf(&m, "IER: 0x%08x\n", error->ier);
 	for (i = 0; i < error->ngtier; i++)
-		err_printf(m, "GTIER[%d]: 0x%08x\n", i, error->gtier[i]);
-	err_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
-	err_printf(m, "FORCEWAKE: 0x%08x\n", error->forcewake);
-	err_printf(m, "DERRMR: 0x%08x\n", error->derrmr);
-	err_printf(m, "CCID: 0x%08x\n", error->ccid);
-	err_printf(m, "Missed interrupts: 0x%08lx\n", dev_priv->gpu_error.missed_irq_rings);
+		err_printf(&m, "GTIER[%d]: 0x%08x\n", i, error->gtier[i]);
+	err_printf(&m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
+	err_printf(&m, "FORCEWAKE: 0x%08x\n", error->forcewake);
+	err_printf(&m, "DERRMR: 0x%08x\n", error->derrmr);
+	err_printf(&m, "CCID: 0x%08x\n", error->ccid);
+	err_printf(&m, "Missed interrupts: 0x%08lx\n", m.i915->gpu_error.missed_irq_rings);
 
 	for (i = 0; i < error->nfence; i++)
-		err_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
+		err_printf(&m, "  fence[%d] = %08llx\n", i, error->fence[i]);
 
-	if (INTEL_GEN(dev_priv) >= 6) {
-		err_printf(m, "ERROR: 0x%08x\n", error->error);
+	if (INTEL_GEN(m.i915) >= 6) {
+		err_printf(&m, "ERROR: 0x%08x\n", error->error);
 
-		if (INTEL_GEN(dev_priv) >= 8)
-			err_printf(m, "FAULT_TLB_DATA: 0x%08x 0x%08x\n",
+		if (INTEL_GEN(m.i915) >= 8)
+			err_printf(&m, "FAULT_TLB_DATA: 0x%08x 0x%08x\n",
 				   error->fault_data1, error->fault_data0);
 
-		err_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
+		err_printf(&m, "DONE_REG: 0x%08x\n", error->done_reg);
 	}
 
-	if (IS_GEN7(dev_priv))
-		err_printf(m, "ERR_INT: 0x%08x\n", error->err_int);
+	if (IS_GEN7(m.i915))
+		err_printf(&m, "ERR_INT: 0x%08x\n", error->err_int);
 
 	for (i = 0; i < ARRAY_SIZE(error->engine); i++) {
 		if (error->engine[i].engine_id != -1)
-			error_print_engine(m, &error->engine[i], error->epoch);
+			error_print_engine(&m, &error->engine[i], error->epoch);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(error->active_vm); i++) {
@@ -738,16 +757,16 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 
 			len += scnprintf(buf + len, sizeof(buf), "%s%s",
 					 first ? "" : ", ",
-					 dev_priv->engine[j]->name);
+					 m.i915->engine[j]->name);
 			first = 0;
 		}
 		scnprintf(buf + len, sizeof(buf), ")");
-		print_error_buffers(m, buf,
+		print_error_buffers(&m, buf,
 				    error->active_bo[i],
 				    error->active_bo_count[i]);
 	}
 
-	print_error_buffers(m, "Pinned (global)",
+	print_error_buffers(&m, "Pinned (global)",
 			    error->pinned_bo,
 			    error->pinned_bo_count);
 
@@ -756,115 +775,163 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 
 		obj = ee->batchbuffer;
 		if (obj) {
-			err_puts(m, dev_priv->engine[i]->name);
+			err_puts(&m, m.i915->engine[i]->name);
 			if (ee->context.pid)
-				err_printf(m, " (submitted by %s [%d], ctx %d [%d], score %d%s)",
+				err_printf(&m, " (submitted by %s [%d], ctx %d [%d], score %d%s)",
 					   ee->context.comm,
 					   ee->context.pid,
 					   ee->context.handle,
 					   ee->context.hw_id,
 					   ee->context.ban_score,
 					   bannable(&ee->context));
-			err_printf(m, " --- gtt_offset = 0x%08x %08x\n",
+			err_printf(&m, " --- gtt_offset = 0x%08x %08x\n",
 				   upper_32_bits(obj->gtt_offset),
 				   lower_32_bits(obj->gtt_offset));
-			print_error_obj(m, dev_priv->engine[i], NULL, obj);
+			print_error_obj(&m, m.i915->engine[i], NULL, obj);
 		}
 
 		for (j = 0; j < ee->user_bo_count; j++)
-			print_error_obj(m, dev_priv->engine[i],
+			print_error_obj(&m, m.i915->engine[i],
 					"user", ee->user_bo[j]);
 
 		if (ee->num_requests) {
-			err_printf(m, "%s --- %d requests\n",
-				   dev_priv->engine[i]->name,
+			err_printf(&m, "%s --- %d requests\n",
+				   m.i915->engine[i]->name,
 				   ee->num_requests);
 			for (j = 0; j < ee->num_requests; j++)
-				error_print_request(m, " ",
+				error_print_request(&m, " ",
 						    &ee->requests[j],
 						    error->epoch);
 		}
 
 		if (IS_ERR(ee->waiters)) {
-			err_printf(m, "%s --- ? waiters [unable to acquire spinlock]\n",
-				   dev_priv->engine[i]->name);
+			err_printf(&m, "%s --- ? waiters [unable to acquire spinlock]\n",
+				   m.i915->engine[i]->name);
 		} else if (ee->num_waiters) {
-			err_printf(m, "%s --- %d waiters\n",
-				   dev_priv->engine[i]->name,
+			err_printf(&m, "%s --- %d waiters\n",
+				   m.i915->engine[i]->name,
 				   ee->num_waiters);
 			for (j = 0; j < ee->num_waiters; j++) {
-				err_printf(m, " seqno 0x%08x for %s [%d]\n",
+				err_printf(&m, " seqno 0x%08x for %s [%d]\n",
 					   ee->waiters[j].seqno,
 					   ee->waiters[j].comm,
 					   ee->waiters[j].pid);
 			}
 		}
 
-		print_error_obj(m, dev_priv->engine[i],
+		print_error_obj(&m, m.i915->engine[i],
 				"ringbuffer", ee->ringbuffer);
 
-		print_error_obj(m, dev_priv->engine[i],
+		print_error_obj(&m, m.i915->engine[i],
 				"HW Status", ee->hws_page);
 
-		print_error_obj(m, dev_priv->engine[i],
+		print_error_obj(&m, m.i915->engine[i],
 				"HW context", ee->ctx);
 
-		print_error_obj(m, dev_priv->engine[i],
+		print_error_obj(&m, m.i915->engine[i],
 				"WA context", ee->wa_ctx);
 
-		print_error_obj(m, dev_priv->engine[i],
+		print_error_obj(&m, m.i915->engine[i],
 				"WA batchbuffer", ee->wa_batchbuffer);
 
-		print_error_obj(m, dev_priv->engine[i],
+		print_error_obj(&m, m.i915->engine[i],
 				"NULL context", ee->default_state);
 	}
 
 	if (error->overlay)
-		intel_overlay_print_error_state(m, error->overlay);
+		intel_overlay_print_error_state(&m, error->overlay);
 
 	if (error->display)
-		intel_display_print_error_state(m, error->display);
+		intel_display_print_error_state(&m, error->display);
 
-	err_print_capabilities(m, &error->device_info, &error->driver_caps);
-	err_print_params(m, &error->params);
-	err_print_uc(m, &error->uc);
+	err_print_capabilities(&m, &error->device_info, &error->driver_caps);
+	err_print_params(&m, &error->params);
+	err_print_uc(&m, &error->uc);
 
-	if (m->bytes == 0 && m->err)
-		return m->err;
+	if (m.buf) {
+		__sg_set_buf(m.cur++, m.buf, m.bytes, m.iter);
+		m.bytes = 0;
+		m.buf = NULL;
+	}
+	if (m.cur) {
+		GEM_BUG_ON(m.end < m.cur);
+		sg_mark_end(m.cur - 1);
+	}
+	GEM_BUG_ON(m.sgl && !m.cur);
+
+	if (m.err) {
+		err_free_sgl(m.sgl);
+		return m.err;
+	}
+
+	if (cmpxchg(&error->sgl, NULL, m.sgl))
+		err_free_sgl(m.sgl);
 
 	return 0;
 }
 
-int i915_error_state_buf_init(struct drm_i915_error_state_buf *ebuf,
-			      struct drm_i915_private *i915,
-			      size_t count, loff_t pos)
+ssize_t i915_gpu_state_copy_to_buffer(struct i915_gpu_state *error,
+				      char *buf, loff_t off, size_t rem)
 {
-	memset(ebuf, 0, sizeof(*ebuf));
-	ebuf->i915 = i915;
+	struct scatterlist *sg;
+	size_t count;
+	loff_t pos;
+	int err;
 
-	/* We need to have enough room to store any i915_error_state printf
-	 * so that we can move it to start position.
-	 */
-	ebuf->size = count + 1 > PAGE_SIZE ? count + 1 : PAGE_SIZE;
-	ebuf->buf = kmalloc(ebuf->size,
-				GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
+	if (!error || !rem)
+		return 0;
 
-	if (ebuf->buf == NULL) {
-		ebuf->size = PAGE_SIZE;
-		ebuf->buf = kmalloc(ebuf->size, GFP_KERNEL);
-	}
+	err = err_print_to_sgl(error);
+	if (err)
+		return err;
 
-	if (ebuf->buf == NULL) {
-		ebuf->size = 128;
-		ebuf->buf = kmalloc(ebuf->size, GFP_KERNEL);
-	}
+	sg = READ_ONCE(error->fit);
+	if (!sg || off < sg->dma_address)
+		sg = error->sgl;
+	if (!sg)
+		return 0;
 
-	if (ebuf->buf == NULL)
-		return -ENOMEM;
+	pos = sg->dma_address;
+	count = 0;
+	do {
+		size_t len, start;
 
-	ebuf->start = pos;
+		if (sg_is_chain(sg)) {
+			sg = sg_chain_ptr(sg);
+			GEM_BUG_ON(sg_is_chain(sg));
+		}
 
-	return 0;
+		len = sg->length;
+		if (pos + len <= off) {
+			pos += len;
+			continue;
+		}
+
+		start = sg->offset;
+		if (pos < off) {
+			GEM_BUG_ON(off - pos > len);
+			len -= off - pos;
+			start += off - pos;
+			pos = off;
+		}
+
+		len = min(len, rem);
+		GEM_BUG_ON(!len || len > sg->length);
+
+		memcpy(buf, page_address(sg_page(sg)) + start, len);
+
+		count += len;
+		pos += len;
+
+		buf += len;
+		rem -= len;
+		if (!rem) {
+			WRITE_ONCE(error->fit, sg);
+			break;
+		}
+	} while (!sg_is_last(sg++));
+
+	return count;
 }
 
 static void i915_error_object_free(struct drm_i915_error_object *obj)
@@ -937,6 +1004,7 @@ void __i915_gpu_state_free(struct kref *error_ref)
 	cleanup_params(error);
 	cleanup_uc_state(error);
 
+	err_free_sgl(error->sgl);
 	kfree(error);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
index f893a4e8b783..1c1bc0c23468 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.h
+++ b/drivers/gpu/drm/i915/i915_gpu_error.h
@@ -191,6 +191,8 @@ struct i915_gpu_state {
 	} *active_bo[I915_NUM_ENGINES], *pinned_bo;
 	u32 active_bo_count[I915_NUM_ENGINES], pinned_bo_count;
 	struct i915_address_space *active_vm[I915_NUM_ENGINES];
+
+	struct scatterlist *sgl, *fit;
 };
 
 struct i915_gpu_error {
@@ -297,29 +299,20 @@ struct i915_gpu_error {
 
 struct drm_i915_error_state_buf {
 	struct drm_i915_private *i915;
-	unsigned int bytes;
-	unsigned int size;
+	struct scatterlist *sgl, *cur, *end;
+
+	char *buf;
+	size_t bytes;
+	size_t size;
+	loff_t iter;
+
 	int err;
-	u8 *buf;
-	loff_t start;
-	loff_t pos;
 };
 
 #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
 
 __printf(2, 3)
 void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...);
-int i915_error_state_to_str(struct drm_i915_error_state_buf *estr,
-			    const struct i915_gpu_state *gpu);
-int i915_error_state_buf_init(struct drm_i915_error_state_buf *eb,
-			      struct drm_i915_private *i915,
-			      size_t count, loff_t pos);
-
-static inline void
-i915_error_state_buf_release(struct drm_i915_error_state_buf *eb)
-{
-	kfree(eb->buf);
-}
 
 struct i915_gpu_state *i915_capture_gpu_state(struct drm_i915_private *i915);
 void i915_capture_error_state(struct drm_i915_private *dev_priv,
@@ -333,6 +326,9 @@ i915_gpu_state_get(struct i915_gpu_state *gpu)
 	return gpu;
 }
 
+ssize_t i915_gpu_state_copy_to_buffer(struct i915_gpu_state *error,
+				      char *buf, loff_t offset, size_t count);
+
 void __i915_gpu_state_free(struct kref *kref);
 static inline void i915_gpu_state_put(struct i915_gpu_state *gpu)
 {
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index e5e6f6bb2b05..ae63a7d0f51d 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -516,26 +516,21 @@ static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
 {
 
 	struct device *kdev = kobj_to_dev(kobj);
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct drm_i915_error_state_buf error_str;
+	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
 	struct i915_gpu_state *gpu;
 	ssize_t ret;
 
-	ret = i915_error_state_buf_init(&error_str, dev_priv, count, off);
-	if (ret)
-		return ret;
-
-	gpu = i915_first_error_state(dev_priv);
-	ret = i915_error_state_to_str(&error_str, gpu);
-	if (ret)
-		goto out;
-
-	ret = count < error_str.bytes ? count : error_str.bytes;
-	memcpy(buf, error_str.buf, ret);
+	gpu = i915_first_error_state(i915);
+	if (gpu) {
+		ret = i915_gpu_state_copy_to_buffer(gpu, buf, off, count);
+		i915_gpu_state_put(gpu);
+	} else {
+		const char *str = "No error state collected\n";
+		size_t len = strlen(str);
 
-out:
-	i915_gpu_state_put(gpu);
-	i915_error_state_buf_release(&error_str);
+		ret = min_t(size_t, count, len - off);
+		memcpy(buf, str + off, ret);
+	}
 
 	return ret;
 }
-- 
2.19.0

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

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type()

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type()
URL   : https://patchwork.freedesktop.org/series/50408/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Replace some open-coded i915_map_coherent_type()
Okay!

Commit: drm/i915: Handle incomplete Z_FINISH for compressed error states
Okay!

Commit: drm/i915: Clear the error PTE just once on finish
Okay!

Commit: drm/i915: Cache the error string
+drivers/gpu/drm/i915/i915_gpu_error.c:918:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gpu_error.c:918:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_sysfs.c:531:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_sysfs.c:531:23: warning: expression using sizeof(void)

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

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type()

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type()
URL   : https://patchwork.freedesktop.org/series/50408/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4911 -> Patchwork_10313 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10313 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10313, 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/50408/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@vgem_basic@debugfs:
      fi-ilk-650:         PASS -> INCOMPLETE

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_getparams_basic@basic-subslice-total:
      fi-snb-2520m:       NOTRUN -> DMESG-WARN (fdo#103713) +10

    igt@gem_exec_suspend@basic-s3:
      fi-skl-caroline:    PASS -> INCOMPLETE (fdo#107773, fdo#107556, fdo#104108)

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

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773


== Participating hosts (49 -> 44) ==

  Additional (3): fi-gdg-551 fi-snb-2520m fi-pnv-d510 
  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus fi-snb-2600 


== Build changes ==

    * Linux: CI_DRM_4911 -> Patchwork_10313

  CI_DRM_4911: e46b4809753a2e3d3d73c5a9e028cd030bea60e5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4659: 7f41adfbfd17027b71c332d6ae997f1364f73731 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10313: 8e58a32e738615317e38a9e9440b52499a101598 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8e58a32e7386 drm/i915: Cache the error string
ff536a417a34 drm/i915: Clear the error PTE just once on finish
b90981da8f54 drm/i915: Handle incomplete Z_FINISH for compressed error states
094552a0cfc5 drm/i915: Replace some open-coded i915_map_coherent_type()

== Logs ==

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

Re: [PATCH 1/4] drm/i915: Replace some open-coded i915_map_coherent_type()

[Tvrtko Ursulin]

On 01/10/2018 20:44, Chris Wilson wrote:
> A few callsites where deciding on using WC or WB maps based on
> HAS_LLC(), so replace them with the equivalent helper function
> i915_map_coherent_type().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c          | 3 +--
>   drivers/gpu/drm/i915/selftests/intel_hangcheck.c | 4 ++--
>   drivers/gpu/drm/i915/selftests/intel_lrc.c       | 2 +-
>   3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c092d5099ebf..b8a7a014d46d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1023,8 +1023,7 @@ i915_emit_bb_start(struct i915_request *rq,
>   int intel_ring_pin(struct intel_ring *ring)
>   {
>   	struct i915_vma *vma = ring->vma;
> -	enum i915_map_type map =
> -		HAS_LLC(vma->vm->i915) ? I915_MAP_WB : I915_MAP_WC;
> +	enum i915_map_type map = i915_coherent_map_type(vma->vm->i915);
>   	unsigned int flags;
>   	void *addr;
>   	int ret;
> diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> index db378226ac10..51d0e2bed9e1 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> @@ -76,7 +76,7 @@ static int hang_init(struct hang *h, struct drm_i915_private *i915)
>   	h->seqno = memset(vaddr, 0xff, PAGE_SIZE);
>   
>   	vaddr = i915_gem_object_pin_map(h->obj,
> -					HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC);
> +					i915_coherent_map_type(i915));
>   	if (IS_ERR(vaddr)) {
>   		err = PTR_ERR(vaddr);
>   		goto err_unpin_hws;
> @@ -234,7 +234,7 @@ hang_create_request(struct hang *h, struct intel_engine_cs *engine)
>   			return ERR_CAST(obj);
>   
>   		vaddr = i915_gem_object_pin_map(obj,
> -						HAS_LLC(h->i915) ? I915_MAP_WB : I915_MAP_WC);
> +						i915_coherent_map_type(h->i915));
>   		if (IS_ERR(vaddr)) {
>   			i915_gem_object_put(obj);
>   			return ERR_CAST(vaddr);
> diff --git a/drivers/gpu/drm/i915/selftests/intel_lrc.c b/drivers/gpu/drm/i915/selftests/intel_lrc.c
> index c8b7f03c35bd..9f241d1c72db 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_lrc.c
> @@ -49,7 +49,7 @@ static int spinner_init(struct spinner *spin, struct drm_i915_private *i915)
>   	}
>   	spin->seqno = memset(vaddr, 0xff, PAGE_SIZE);
>   
> -	mode = HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
> +	mode = i915_coherent_map_type(i915);
>   	vaddr = i915_gem_object_pin_map(spin->obj, mode);
>   	if (IS_ERR(vaddr)) {
>   		err = PTR_ERR(vaddr);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Tvrtko Ursulin]

On 01/10/2018 20:44, Chris Wilson wrote:
> The final call to zlib_deflate(Z_FINISH) may require more output
> space to be allocated and so needs to re-invoked. Failure to do so in
> the current code leads to incomplete zlib streams (albeit intact due to
> the use of Z_SYNC_FLUSH) resulting in the occasional short object
> capture.
> 
> Testcase: igt/i915-error-capture.js
> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
>   drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
>   1 file changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 3d5554f14dfd..ed8c16cbfaa4 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
>   			 struct drm_i915_error_object *dst)
>   {
>   	struct z_stream_s *zstream = &c->zstream;
> +	int flush = Z_NO_FLUSH;
>   
>   	zstream->next_in = src;
>   	if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
> @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
>   			zstream->avail_out = PAGE_SIZE;
>   		}

Looks like the block above, not shown in this diff, could use a check 
and abort if the dst->page_count overgrows the pessimistic allocation of 
the array.

>   
> -		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> +		if (zlib_deflate(zstream, flush) != Z_OK)

So this (not always asking for a flush) actually not only fixes the 
flush at the end but improves the API usage and potentially compression 
ratio, correct?

>   			return -EIO;
> +
> +		if (zstream->avail_out)
> +			flush = Z_SYNC_FLUSH;

Hm.. but why this? It will flush only occasionally, when one input page 
did not fit in the available output - but that will depend on 
compressibility so I don't think it has a fixed period. It is not for 
instance for every 4k of compressed output, if that was maybe the goal.

>   	} while (zstream->avail_in);
>   
>   	/* Fallback to uncompressed if we increase size? */
> @@ -268,19 +272,43 @@ static int compress_page(struct compress *c,
>   	return 0;
>   }
>   
> -static void compress_fini(struct compress *c,
> +static int compress_flush(struct compress *c,
>   			  struct drm_i915_error_object *dst)
>   {
>   	struct z_stream_s *zstream = &c->zstream;
> +	unsigned long page;
>   
> -	if (dst) {
> -		zlib_deflate(zstream, Z_FINISH);
> -		dst->unused = zstream->avail_out;
> -	}
> +	do {
> +		switch (zlib_deflate(zstream, Z_FINISH)) {
> +		case Z_OK: /* more space requested */
> +			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> +			if (!page)
> +				return -ENOMEM;
> +
> +			dst->pages[dst->page_count++] = (void *)page;

I'd put in a check for pages array exhaustion here as well. Or even 
better, compress_page and compress_flush could share this whole block.

> +			zstream->next_out = (void *)page;
> +			zstream->avail_out = PAGE_SIZE;
> +			break;
> +		case Z_STREAM_END:
> +			goto end;
> +		default: /* any error */
> +			return -EIO;
> +		}
> +	} while (1);
> +
> +end:
> +	memset(zstream->next_out, 0, zstream->avail_out);
> +	dst->unused = zstream->avail_out;
> +	return 0;
> +}
> +
> +static void compress_fini(struct compress *c,
> +			  struct drm_i915_error_object *dst)
> +{
> +	struct z_stream_s *zstream = &c->zstream;
>   
>   	zlib_deflateEnd(zstream);
>   	kfree(zstream->workspace);
> -
>   	if (c->tmp)
>   		free_page((unsigned long)c->tmp);
>   }
> @@ -319,6 +347,12 @@ static int compress_page(struct compress *c,
>   	return 0;
>   }
>   
> +static int compress_flush(struct compress *c,
> +			  struct drm_i915_error_object *dst)
> +{
> +	return 0;
> +}
> +
>   static void compress_fini(struct compress *c,
>   			  struct drm_i915_error_object *dst)
>   {
> @@ -951,15 +985,15 @@ i915_error_object_create(struct drm_i915_private *i915,
>   		if (ret)
>   			goto unwind;
>   	}
> -	goto out;
>   
> +	if (compress_flush(&compress, dst)) {
>   unwind:

A bit nasty, jump in conditional block. Could set a boolean and break 
from the above loop. Like "if (failed || compress_flush(...))".

> -	while (dst->page_count--)
> -		free_page((unsigned long)dst->pages[dst->page_count]);
> -	kfree(dst);
> -	dst = NULL;
> +		while (dst->page_count--)
> +			free_page((unsigned long)dst->pages[dst->page_count]);
> +		kfree(dst);
> +		dst = NULL;
> +	}
>   
> -out:
>   	compress_fini(&compress, dst);
>   	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
>   	return dst;
> 

Regards,

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

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-02 13:20:05)
> 
> On 01/10/2018 20:44, Chris Wilson wrote:
> > The final call to zlib_deflate(Z_FINISH) may require more output
> > space to be allocated and so needs to re-invoked. Failure to do so in
> > the current code leads to incomplete zlib streams (albeit intact due to
> > the use of Z_SYNC_FLUSH) resulting in the occasional short object
> > capture.
> > 
> > Testcase: igt/i915-error-capture.js
> > Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.10+
> > ---
> >   drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
> >   1 file changed, 47 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 3d5554f14dfd..ed8c16cbfaa4 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
> >                        struct drm_i915_error_object *dst)
> >   {
> >       struct z_stream_s *zstream = &c->zstream;
> > +     int flush = Z_NO_FLUSH;
> >   
> >       zstream->next_in = src;
> >       if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
> > @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
> >                       zstream->avail_out = PAGE_SIZE;
> >               }
> 
> Looks like the block above, not shown in this diff, could use a check 
> and abort if the dst->page_count overgrows the pessimistic allocation of 
> the array.
> 
> >   
> > -             if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> > +             if (zlib_deflate(zstream, flush) != Z_OK)
> 
> So this (not always asking for a flush) actually not only fixes the 
> flush at the end but improves the API usage and potentially compression 
> ratio, correct?

Yes.
 
> >                       return -EIO;
> > +
> > +             if (zstream->avail_out)
> > +                     flush = Z_SYNC_FLUSH;
> 
> Hm.. but why this? It will flush only occasionally, when one input page 
> did not fit in the available output - but that will depend on 
> compressibility so I don't think it has a fixed period. It is not for 
> instance for every 4k of compressed output, if that was maybe the goal.

My thinking is that if the zlib_deflate() wants to defer to fill its
window (or whatever) but we need to cross a page boundary, we have to
push everything from the current page before we change the PTE.

> >       } while (zstream->avail_in);
> >   
> >       /* Fallback to uncompressed if we increase size? */
> > @@ -268,19 +272,43 @@ static int compress_page(struct compress *c,
> >       return 0;
> >   }
> >   
> > -static void compress_fini(struct compress *c,
> > +static int compress_flush(struct compress *c,
> >                         struct drm_i915_error_object *dst)
> >   {
> >       struct z_stream_s *zstream = &c->zstream;
> > +     unsigned long page;
> >   
> > -     if (dst) {
> > -             zlib_deflate(zstream, Z_FINISH);
> > -             dst->unused = zstream->avail_out;
> > -     }
> > +     do {
> > +             switch (zlib_deflate(zstream, Z_FINISH)) {
> > +             case Z_OK: /* more space requested */
> > +                     page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> > +                     if (!page)
> > +                             return -ENOMEM;
> > +
> > +                     dst->pages[dst->page_count++] = (void *)page;
> 
> I'd put in a check for pages array exhaustion here as well. Or even 
> better, compress_page and compress_flush could share this whole block.
> 
> > +                     zstream->next_out = (void *)page;
> > +                     zstream->avail_out = PAGE_SIZE;
> > +                     break;
> > +             case Z_STREAM_END:
> > +                     goto end;
> > +             default: /* any error */
> > +                     return -EIO;
> > +             }
> > +     } while (1);
> > +
> > +end:
> > +     memset(zstream->next_out, 0, zstream->avail_out);
> > +     dst->unused = zstream->avail_out;
> > +     return 0;
> > +}
> > +
> > +static void compress_fini(struct compress *c,
> > +                       struct drm_i915_error_object *dst)
> > +{
> > +     struct z_stream_s *zstream = &c->zstream;
> >   
> >       zlib_deflateEnd(zstream);
> >       kfree(zstream->workspace);
> > -
> >       if (c->tmp)
> >               free_page((unsigned long)c->tmp);
> >   }
> > @@ -319,6 +347,12 @@ static int compress_page(struct compress *c,
> >       return 0;
> >   }
> >   
> > +static int compress_flush(struct compress *c,
> > +                       struct drm_i915_error_object *dst)
> > +{
> > +     return 0;
> > +}
> > +
> >   static void compress_fini(struct compress *c,
> >                         struct drm_i915_error_object *dst)
> >   {
> > @@ -951,15 +985,15 @@ i915_error_object_create(struct drm_i915_private *i915,
> >               if (ret)
> >                       goto unwind;
> >       }
> > -     goto out;
> >   
> > +     if (compress_flush(&compress, dst)) {
> >   unwind:
> 
> A bit nasty, jump in conditional block. Could set a boolean and break 
> from the above loop. Like "if (failed || compress_flush(...))".

Or just jmp here for simplicity :-p
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Clear the error PTE just once on finish

[Tvrtko Ursulin]

On 01/10/2018 20:44, Chris Wilson wrote:
> We do not need to continually clear our dedicated PTE for error capture
> as it will be updated and invalidated to the next object. Only at the
> end do we wish to be sure that the PTE doesn't point back to any buffer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gpu_error.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index ed8c16cbfaa4..fed9574be5b6 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -995,7 +995,6 @@ i915_error_object_create(struct drm_i915_private *i915,
>   	}
>   
>   	compress_fini(&compress, dst);
> -	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
>   	return dst;
>   }
>   
> @@ -1781,6 +1780,14 @@ static unsigned long capture_find_epoch(const struct i915_gpu_state *error)
>   	return epoch;
>   }
>   
> +static void capture_finish(struct i915_gpu_state *error)
> +{
> +	struct i915_ggtt *ggtt = &error->i915->ggtt;
> +	const u64 slot = ggtt->error_capture.start;
> +
> +	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
> +}
> +
>   static int capture(void *data)
>   {
>   	struct i915_gpu_state *error = data;
> @@ -1805,6 +1812,7 @@ static int capture(void *data)
>   
>   	error->epoch = capture_find_epoch(error);
>   
> +	capture_finish(error);
>   	return 0;
>   }
>   
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

[PATCH v2] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

The final call to zlib_deflate(Z_FINISH) may require more output
space to be allocated and so needs to re-invoked. Failure to do so in
the current code leads to incomplete zlib streams (albeit intact due to
the use of Z_SYNC_FLUSH) resulting in the occasional short object
capture.

v2: Check against overrunning our pre-allocated page array

Testcase: igt/i915-error-capture.js
Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.10+
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 92 +++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gpu_error.h |  1 +
 2 files changed, 68 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 3d5554f14dfd..1035298e9a82 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -232,11 +232,26 @@ static bool compress_init(struct compress *c)
 	return true;
 }
 
+static void *compress_next_page(struct drm_i915_error_object *dst)
+{
+	unsigned long page;
+
+	if (dst->page_count >= dst->num_pages)
+		return ERR_PTR(-ENOSPC);
+
+	page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
+	if (!page)
+		return ERR_PTR(-ENOMEM);
+
+	return dst->pages[dst->page_count++] = (void *)page;
+}
+
 static int compress_page(struct compress *c,
 			 void *src,
 			 struct drm_i915_error_object *dst)
 {
 	struct z_stream_s *zstream = &c->zstream;
+	int flush = Z_NO_FLUSH;
 
 	zstream->next_in = src;
 	if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
@@ -245,20 +260,18 @@ static int compress_page(struct compress *c,
 
 	do {
 		if (zstream->avail_out == 0) {
-			unsigned long page;
-
-			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
-			if (!page)
-				return -ENOMEM;
+			zstream->next_out = compress_next_page(dst);
+			if (IS_ERR(zstream->next_out))
+				return PTR_ERR(zstream->next_out);
 
-			dst->pages[dst->page_count++] = (void *)page;
-
-			zstream->next_out = (void *)page;
 			zstream->avail_out = PAGE_SIZE;
 		}
 
-		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
+		if (zlib_deflate(zstream, flush) != Z_OK)
 			return -EIO;
+
+		if (zstream->avail_out)
+			flush = Z_SYNC_FLUSH;
 	} while (zstream->avail_in);
 
 	/* Fallback to uncompressed if we increase size? */
@@ -268,19 +281,42 @@ static int compress_page(struct compress *c,
 	return 0;
 }
 
-static void compress_fini(struct compress *c,
+static int compress_flush(struct compress *c,
 			  struct drm_i915_error_object *dst)
 {
 	struct z_stream_s *zstream = &c->zstream;
 
-	if (dst) {
-		zlib_deflate(zstream, Z_FINISH);
-		dst->unused = zstream->avail_out;
-	}
+	do {
+		switch (zlib_deflate(zstream, Z_FINISH)) {
+		case Z_OK: /* more space requested */
+			zstream->next_out = compress_next_page(dst);
+			if (IS_ERR(zstream->next_out))
+				return PTR_ERR(zstream->next_out);
+
+			zstream->avail_out = PAGE_SIZE;
+			break;
+
+		case Z_STREAM_END:
+			goto end;
+
+		default: /* any error */
+			return -EIO;
+		}
+	} while (1);
+
+end:
+	memset(zstream->next_out, 0, zstream->avail_out);
+	dst->unused = zstream->avail_out;
+	return 0;
+}
+
+static void compress_fini(struct compress *c,
+			  struct drm_i915_error_object *dst)
+{
+	struct z_stream_s *zstream = &c->zstream;
 
 	zlib_deflateEnd(zstream);
 	kfree(zstream->workspace);
-
 	if (c->tmp)
 		free_page((unsigned long)c->tmp);
 }
@@ -319,6 +355,12 @@ static int compress_page(struct compress *c,
 	return 0;
 }
 
+static int compress_flush(struct compress *c,
+			  struct drm_i915_error_object *dst)
+{
+	return 0;
+}
+
 static void compress_fini(struct compress *c,
 			  struct drm_i915_error_object *dst)
 {
@@ -917,6 +959,7 @@ i915_error_object_create(struct drm_i915_private *i915,
 	unsigned long num_pages;
 	struct sgt_iter iter;
 	dma_addr_t dma;
+	int ret;
 
 	if (!vma)
 		return NULL;
@@ -930,6 +973,7 @@ i915_error_object_create(struct drm_i915_private *i915,
 
 	dst->gtt_offset = vma->node.start;
 	dst->gtt_size = vma->node.size;
+	dst->num_pages = num_pages;
 	dst->page_count = 0;
 	dst->unused = 0;
 
@@ -938,28 +982,26 @@ i915_error_object_create(struct drm_i915_private *i915,
 		return NULL;
 	}
 
+	ret = -EINVAL;
 	for_each_sgt_dma(dma, iter, vma->pages) {
 		void __iomem *s;
-		int ret;
 
 		ggtt->vm.insert_page(&ggtt->vm, dma, slot, I915_CACHE_NONE, 0);
 
 		s = io_mapping_map_atomic_wc(&ggtt->iomap, slot);
 		ret = compress_page(&compress, (void  __force *)s, dst);
 		io_mapping_unmap_atomic(s);
-
 		if (ret)
-			goto unwind;
+			break;
 	}
-	goto out;
 
-unwind:
-	while (dst->page_count--)
-		free_page((unsigned long)dst->pages[dst->page_count]);
-	kfree(dst);
-	dst = NULL;
+	if (ret || compress_flush(&compress, dst)) {
+		while (dst->page_count--)
+			free_page((unsigned long)dst->pages[dst->page_count]);
+		kfree(dst);
+		dst = NULL;
+	}
 
-out:
 	compress_fini(&compress, dst);
 	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
 	return dst;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
index f893a4e8b783..8710fb18ed74 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.h
+++ b/drivers/gpu/drm/i915/i915_gpu_error.h
@@ -135,6 +135,7 @@ struct i915_gpu_state {
 		struct drm_i915_error_object {
 			u64 gtt_offset;
 			u64 gtt_size;
+			int num_pages;
 			int page_count;
 			int unused;
 			u32 *pages[0];
-- 
2.19.0

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Tvrtko Ursulin]

On 02/10/2018 13:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-02 13:20:05)
>>
>> On 01/10/2018 20:44, Chris Wilson wrote:
>>> The final call to zlib_deflate(Z_FINISH) may require more output
>>> space to be allocated and so needs to re-invoked. Failure to do so in
>>> the current code leads to incomplete zlib streams (albeit intact due to
>>> the use of Z_SYNC_FLUSH) resulting in the occasional short object
>>> capture.
>>>
>>> Testcase: igt/i915-error-capture.js
>>> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: <stable@vger.kernel.org> # v4.10+
>>> ---
>>>    drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
>>>    1 file changed, 47 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>>> index 3d5554f14dfd..ed8c16cbfaa4 100644
>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>> @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
>>>                         struct drm_i915_error_object *dst)
>>>    {
>>>        struct z_stream_s *zstream = &c->zstream;
>>> +     int flush = Z_NO_FLUSH;
>>>    
>>>        zstream->next_in = src;
>>>        if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
>>> @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
>>>                        zstream->avail_out = PAGE_SIZE;
>>>                }
>>
>> Looks like the block above, not shown in this diff, could use a check
>> and abort if the dst->page_count overgrows the pessimistic allocation of
>> the array.
>>
>>>    
>>> -             if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
>>> +             if (zlib_deflate(zstream, flush) != Z_OK)
>>
>> So this (not always asking for a flush) actually not only fixes the
>> flush at the end but improves the API usage and potentially compression
>> ratio, correct?
> 
> Yes.
>   
>>>                        return -EIO;
>>> +
>>> +             if (zstream->avail_out)
>>> +                     flush = Z_SYNC_FLUSH;
>>
>> Hm.. but why this? It will flush only occasionally, when one input page
>> did not fit in the available output - but that will depend on
>> compressibility so I don't think it has a fixed period. It is not for
>> instance for every 4k of compressed output, if that was maybe the goal.
> 
> My thinking is that if the zlib_deflate() wants to defer to fill its
> window (or whatever) but we need to cross a page boundary, we have to
> push everything from the current page before we change the PTE.

Hm but I am not sure this is achieving that, or that it is needed. I 
read the docs many times and I am not sure how it is supposed to work.

First thing I am not sure of is whether we need to ask for any flushes, 
or it would be enough to loop until avail_in > 0 || avail_out == 0. 
Since docs say that if avail_out == 0, zlib_deflate needs to be called 
again in the same flush mode.

Current code fails to ensure this. It would happen on the next call to 
compress_page, but a) I think that is unclean and b) flush mode will not 
match.

So I think it would be interesting to try:

do {
	get_page_if_no_space;
	zlib_deflate(no_flush) == Z_OK or -EIO;
while (avail_in || !avail_out);

This would make sure whole input page is output before going to next page.

But my confidence level this is correct is not super high..

Regards,

Tvrtko

> 
>>>        } while (zstream->avail_in);
>>>    
>>>        /* Fallback to uncompressed if we increase size? */
>>> @@ -268,19 +272,43 @@ static int compress_page(struct compress *c,
>>>        return 0;
>>>    }
>>>    
>>> -static void compress_fini(struct compress *c,
>>> +static int compress_flush(struct compress *c,
>>>                          struct drm_i915_error_object *dst)
>>>    {
>>>        struct z_stream_s *zstream = &c->zstream;
>>> +     unsigned long page;
>>>    
>>> -     if (dst) {
>>> -             zlib_deflate(zstream, Z_FINISH);
>>> -             dst->unused = zstream->avail_out;
>>> -     }
>>> +     do {
>>> +             switch (zlib_deflate(zstream, Z_FINISH)) {
>>> +             case Z_OK: /* more space requested */
>>> +                     page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
>>> +                     if (!page)
>>> +                             return -ENOMEM;
>>> +
>>> +                     dst->pages[dst->page_count++] = (void *)page;
>>
>> I'd put in a check for pages array exhaustion here as well. Or even
>> better, compress_page and compress_flush could share this whole block.
>>
>>> +                     zstream->next_out = (void *)page;
>>> +                     zstream->avail_out = PAGE_SIZE;
>>> +                     break;
>>> +             case Z_STREAM_END:
>>> +                     goto end;
>>> +             default: /* any error */
>>> +                     return -EIO;
>>> +             }
>>> +     } while (1);
>>> +
>>> +end:
>>> +     memset(zstream->next_out, 0, zstream->avail_out);
>>> +     dst->unused = zstream->avail_out;
>>> +     return 0;
>>> +}
>>> +
>>> +static void compress_fini(struct compress *c,
>>> +                       struct drm_i915_error_object *dst)
>>> +{
>>> +     struct z_stream_s *zstream = &c->zstream;
>>>    
>>>        zlib_deflateEnd(zstream);
>>>        kfree(zstream->workspace);
>>> -
>>>        if (c->tmp)
>>>                free_page((unsigned long)c->tmp);
>>>    }
>>> @@ -319,6 +347,12 @@ static int compress_page(struct compress *c,
>>>        return 0;
>>>    }
>>>    
>>> +static int compress_flush(struct compress *c,
>>> +                       struct drm_i915_error_object *dst)
>>> +{
>>> +     return 0;
>>> +}
>>> +
>>>    static void compress_fini(struct compress *c,
>>>                          struct drm_i915_error_object *dst)
>>>    {
>>> @@ -951,15 +985,15 @@ i915_error_object_create(struct drm_i915_private *i915,
>>>                if (ret)
>>>                        goto unwind;
>>>        }
>>> -     goto out;
>>>    
>>> +     if (compress_flush(&compress, dst)) {
>>>    unwind:
>>
>> A bit nasty, jump in conditional block. Could set a boolean and break
>> from the above loop. Like "if (failed || compress_flush(...))".
> 
> Or just jmp here for simplicity :-p
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-02 14:13:14)
> 
> On 02/10/2018 13:24, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-10-02 13:20:05)
> >>
> >> On 01/10/2018 20:44, Chris Wilson wrote:
> >>> The final call to zlib_deflate(Z_FINISH) may require more output
> >>> space to be allocated and so needs to re-invoked. Failure to do so in
> >>> the current code leads to incomplete zlib streams (albeit intact due to
> >>> the use of Z_SYNC_FLUSH) resulting in the occasional short object
> >>> capture.
> >>>
> >>> Testcase: igt/i915-error-capture.js
> >>> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>> Cc: <stable@vger.kernel.org> # v4.10+
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
> >>>    1 file changed, 47 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>> index 3d5554f14dfd..ed8c16cbfaa4 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>> @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
> >>>                         struct drm_i915_error_object *dst)
> >>>    {
> >>>        struct z_stream_s *zstream = &c->zstream;
> >>> +     int flush = Z_NO_FLUSH;
> >>>    
> >>>        zstream->next_in = src;
> >>>        if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
> >>> @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
> >>>                        zstream->avail_out = PAGE_SIZE;
> >>>                }
> >>
> >> Looks like the block above, not shown in this diff, could use a check
> >> and abort if the dst->page_count overgrows the pessimistic allocation of
> >> the array.
> >>
> >>>    
> >>> -             if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> >>> +             if (zlib_deflate(zstream, flush) != Z_OK)
> >>
> >> So this (not always asking for a flush) actually not only fixes the
> >> flush at the end but improves the API usage and potentially compression
> >> ratio, correct?
> > 
> > Yes.
> >   
> >>>                        return -EIO;
> >>> +
> >>> +             if (zstream->avail_out)
> >>> +                     flush = Z_SYNC_FLUSH;
> >>
> >> Hm.. but why this? It will flush only occasionally, when one input page
> >> did not fit in the available output - but that will depend on
> >> compressibility so I don't think it has a fixed period. It is not for
> >> instance for every 4k of compressed output, if that was maybe the goal.
> > 
> > My thinking is that if the zlib_deflate() wants to defer to fill its
> > window (or whatever) but we need to cross a page boundary, we have to
> > push everything from the current page before we change the PTE.
> 
> Hm but I am not sure this is achieving that, or that it is needed. I 
> read the docs many times and I am not sure how it is supposed to work.
> 
> First thing I am not sure of is whether we need to ask for any flushes, 
> or it would be enough to loop until avail_in > 0 || avail_out == 0. 
> Since docs say that if avail_out == 0, zlib_deflate needs to be called 
> again in the same flush mode.

Iirc, Z_SYNC_FLUSH talks about terminating the compression block earlier
in order to consume next_avail, without the SYNC repeating a call is
allowed to not progress.

> Current code fails to ensure this. It would happen on the next call to 
> compress_page, but a) I think that is unclean and b) flush mode will not 
> match.

Fails to ensure what exactly? I think it is obeying the rules on flush:

  If the parameter flush is set to Z_SYNC_FLUSH, all pending output is
  flushed to the output buffer and the output is aligned on a byte boundary, so
  that the decompressor can get all input data available so far. (In particular
  avail_in is zero after the call if enough output space has been provided
  before the call.)  Flushing may degrade compression for some compression
  algorithms and so it should be used only when necessary.

  ...

  If deflate returns with avail_out == 0, this function must be called again
  with the same value of the flush parameter and more output space (updated
  avail_out), until the flush is complete (deflate returns with non-zero
  avail_out).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)
URL   : https://patchwork.freedesktop.org/series/50408/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Handle incomplete Z_FINISH for compressed error states
Okay!

Commit: drm/i915: Clear the error PTE just once on finish
Okay!

Commit: drm/i915: Cache the error string
+drivers/gpu/drm/i915/i915_gpu_error.c:926:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gpu_error.c:926:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_sysfs.c:531:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_sysfs.c:531:23: warning: expression using sizeof(void)

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

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Tvrtko Ursulin]

On 02/10/2018 14:22, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-02 14:13:14)
>>
>> On 02/10/2018 13:24, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-10-02 13:20:05)
>>>>
>>>> On 01/10/2018 20:44, Chris Wilson wrote:
>>>>> The final call to zlib_deflate(Z_FINISH) may require more output
>>>>> space to be allocated and so needs to re-invoked. Failure to do so in
>>>>> the current code leads to incomplete zlib streams (albeit intact due to
>>>>> the use of Z_SYNC_FLUSH) resulting in the occasional short object
>>>>> capture.
>>>>>
>>>>> Testcase: igt/i915-error-capture.js
>>>>> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>> Cc: <stable@vger.kernel.org> # v4.10+
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
>>>>>     1 file changed, 47 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>> index 3d5554f14dfd..ed8c16cbfaa4 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>> @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
>>>>>                          struct drm_i915_error_object *dst)
>>>>>     {
>>>>>         struct z_stream_s *zstream = &c->zstream;
>>>>> +     int flush = Z_NO_FLUSH;
>>>>>     
>>>>>         zstream->next_in = src;
>>>>>         if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
>>>>> @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
>>>>>                         zstream->avail_out = PAGE_SIZE;
>>>>>                 }
>>>>
>>>> Looks like the block above, not shown in this diff, could use a check
>>>> and abort if the dst->page_count overgrows the pessimistic allocation of
>>>> the array.
>>>>
>>>>>     
>>>>> -             if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
>>>>> +             if (zlib_deflate(zstream, flush) != Z_OK)
>>>>
>>>> So this (not always asking for a flush) actually not only fixes the
>>>> flush at the end but improves the API usage and potentially compression
>>>> ratio, correct?
>>>
>>> Yes.
>>>    
>>>>>                         return -EIO;
>>>>> +
>>>>> +             if (zstream->avail_out)
>>>>> +                     flush = Z_SYNC_FLUSH;
>>>>
>>>> Hm.. but why this? It will flush only occasionally, when one input page
>>>> did not fit in the available output - but that will depend on
>>>> compressibility so I don't think it has a fixed period. It is not for
>>>> instance for every 4k of compressed output, if that was maybe the goal.
>>>
>>> My thinking is that if the zlib_deflate() wants to defer to fill its
>>> window (or whatever) but we need to cross a page boundary, we have to
>>> push everything from the current page before we change the PTE.
>>
>> Hm but I am not sure this is achieving that, or that it is needed. I
>> read the docs many times and I am not sure how it is supposed to work.
>>
>> First thing I am not sure of is whether we need to ask for any flushes,
>> or it would be enough to loop until avail_in > 0 || avail_out == 0.
>> Since docs say that if avail_out == 0, zlib_deflate needs to be called
>> again in the same flush mode.
> 
> Iirc, Z_SYNC_FLUSH talks about terminating the compression block earlier
> in order to consume next_avail, without the SYNC repeating a call is
> allowed to not progress.
> 
>> Current code fails to ensure this. It would happen on the next call to
>> compress_page, but a) I think that is unclean and b) flush mode will not
>> match.
> 
> Fails to ensure what exactly? I think it is obeying the rules on flush:
> 
>    If the parameter flush is set to Z_SYNC_FLUSH, all pending output is
>    flushed to the output buffer and the output is aligned on a byte boundary, so
>    that the decompressor can get all input data available so far. (In particular
>    avail_in is zero after the call if enough output space has been provided
>    before the call.)  Flushing may degrade compression for some compression
>    algorithms and so it should be used only when necessary.

I am not sure we need to flush - I don't see that it says in the docs 
that we have to. AFAIU when avail_in drops to zero we are allowed to 
move on. Anything outstanding in the compressor would be then flushed 
either on processing the next page, or on stream close.

>    ...
> 
>    If deflate returns with avail_out == 0, this function must be called again
>    with the same value of the flush parameter and more output space (updated
>    avail_out), until the flush is complete (deflate returns with non-zero
>    avail_out).

Yes, but the patch does not do that. You enable the flush mode when 
avail_out is non zero, but then two options are possible. Either 
avail_in is zero, in which case nothing happens, or it is not. In the 
latter case there is another flushing call to zlib_deflate. If avail_out 
is zero after that call, and avail_in is also zero, the documentation 
paragraph above says you have to call zlib_deflate in the same flush 
mode again.

Regards,

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)
URL   : https://patchwork.freedesktop.org/series/50408/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4915 -> Patchwork_10324 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10324 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10324, 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/50408/revisions/2/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@drv_selftest@live_guc:
      fi-glk-j4005:       SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-bdw-samus:       NOTRUN -> INCOMPLETE (fdo#107773)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         DMESG-FAIL (fdo#107164) -> PASS

    igt@drv_selftest@live_execlists:
      fi-glk-j4005:       INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@gem_exec_suspend@basic-s3:
      fi-bdw-samus:       INCOMPLETE (fdo#107773) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-skl-guc:         FAIL (fdo#103191) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (46 -> 43) ==

  Missing    (3): fi-bsw-cyan fi-byt-squawks fi-icl-u2 


== Build changes ==

    * Linux: CI_DRM_4915 -> Patchwork_10324

  CI_DRM_4915: 26e7a7d954a9c28b97af8ca7813f430fd9117232 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4660: d0975646c50568e66e65b44b81d28232d059b94e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10324: b693af3ecd245d4c8abc93a21863a3d42398907c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b693af3ecd24 drm/i915: Cache the error string
9bc195de53fa drm/i915: Clear the error PTE just once on finish
0b1ffb63f630 drm/i915: Handle incomplete Z_FINISH for compressed error states

== Logs ==

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

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-02 14:46:10)
> 
> On 02/10/2018 14:22, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-10-02 14:13:14)
> >>
> >> On 02/10/2018 13:24, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-10-02 13:20:05)
> >>>>
> >>>> On 01/10/2018 20:44, Chris Wilson wrote:
> >>>>> The final call to zlib_deflate(Z_FINISH) may require more output
> >>>>> space to be allocated and so needs to re-invoked. Failure to do so in
> >>>>> the current code leads to incomplete zlib streams (albeit intact due to
> >>>>> the use of Z_SYNC_FLUSH) resulting in the occasional short object
> >>>>> capture.
> >>>>>
> >>>>> Testcase: igt/i915-error-capture.js
> >>>>> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>>>> Cc: <stable@vger.kernel.org> # v4.10+
> >>>>> ---
> >>>>>     drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
> >>>>>     1 file changed, 47 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>> index 3d5554f14dfd..ed8c16cbfaa4 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>> @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
> >>>>>                          struct drm_i915_error_object *dst)
> >>>>>     {
> >>>>>         struct z_stream_s *zstream = &c->zstream;
> >>>>> +     int flush = Z_NO_FLUSH;
> >>>>>     
> >>>>>         zstream->next_in = src;
> >>>>>         if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
> >>>>> @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
> >>>>>                         zstream->avail_out = PAGE_SIZE;
> >>>>>                 }
> >>>>
> >>>> Looks like the block above, not shown in this diff, could use a check
> >>>> and abort if the dst->page_count overgrows the pessimistic allocation of
> >>>> the array.
> >>>>
> >>>>>     
> >>>>> -             if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> >>>>> +             if (zlib_deflate(zstream, flush) != Z_OK)
> >>>>
> >>>> So this (not always asking for a flush) actually not only fixes the
> >>>> flush at the end but improves the API usage and potentially compression
> >>>> ratio, correct?
> >>>
> >>> Yes.
> >>>    
> >>>>>                         return -EIO;
> >>>>> +
> >>>>> +             if (zstream->avail_out)
> >>>>> +                     flush = Z_SYNC_FLUSH;
> >>>>
> >>>> Hm.. but why this? It will flush only occasionally, when one input page
> >>>> did not fit in the available output - but that will depend on
> >>>> compressibility so I don't think it has a fixed period. It is not for
> >>>> instance for every 4k of compressed output, if that was maybe the goal.
> >>>
> >>> My thinking is that if the zlib_deflate() wants to defer to fill its
> >>> window (or whatever) but we need to cross a page boundary, we have to
> >>> push everything from the current page before we change the PTE.
> >>
> >> Hm but I am not sure this is achieving that, or that it is needed. I
> >> read the docs many times and I am not sure how it is supposed to work.
> >>
> >> First thing I am not sure of is whether we need to ask for any flushes,
> >> or it would be enough to loop until avail_in > 0 || avail_out == 0.
> >> Since docs say that if avail_out == 0, zlib_deflate needs to be called
> >> again in the same flush mode.
> > 
> > Iirc, Z_SYNC_FLUSH talks about terminating the compression block earlier
> > in order to consume next_avail, without the SYNC repeating a call is
> > allowed to not progress.
> > 
> >> Current code fails to ensure this. It would happen on the next call to
> >> compress_page, but a) I think that is unclean and b) flush mode will not
> >> match.
> > 
> > Fails to ensure what exactly? I think it is obeying the rules on flush:
> > 
> >    If the parameter flush is set to Z_SYNC_FLUSH, all pending output is
> >    flushed to the output buffer and the output is aligned on a byte boundary, so
> >    that the decompressor can get all input data available so far. (In particular
> >    avail_in is zero after the call if enough output space has been provided
> >    before the call.)  Flushing may degrade compression for some compression
> >    algorithms and so it should be used only when necessary.
> 
> I am not sure we need to flush - I don't see that it says in the docs 
> that we have to. AFAIU when avail_in drops to zero we are allowed to 
> move on. Anything outstanding in the compressor would be then flushed 
> either on processing the next page, or on stream close.

The case we want to handle is avail_in != 0. We cannot move on until
avail_in is 0.

> >    ...
> > 
> >    If deflate returns with avail_out == 0, this function must be called again
> >    with the same value of the flush parameter and more output space (updated
> >    avail_out), until the flush is complete (deflate returns with non-zero
> >    avail_out).
> 
> Yes, but the patch does not do that. You enable the flush mode when 
> avail_out is non zero, but then two options are possible. Either 
> avail_in is zero, in which case nothing happens, or it is not. In the 
> latter case there is another flushing call to zlib_deflate. If avail_out 
> is zero after that call, and avail_in is also zero, the documentation 
> paragraph above says you have to call zlib_deflate in the same flush 
> mode again.

Read the paragraph again, specifically when avail_out == 0 you should
allocate more space and rerun the compressor. If avail_out != 0 and
avail_in != 0 we want to SYNC in order to consume the remaining
avail_in.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Tvrtko Ursulin]

On 02/10/2018 14:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-02 14:46:10)
>>
>> On 02/10/2018 14:22, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-10-02 14:13:14)
>>>>
>>>> On 02/10/2018 13:24, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2018-10-02 13:20:05)
>>>>>>
>>>>>> On 01/10/2018 20:44, Chris Wilson wrote:
>>>>>>> The final call to zlib_deflate(Z_FINISH) may require more output
>>>>>>> space to be allocated and so needs to re-invoked. Failure to do so in
>>>>>>> the current code leads to incomplete zlib streams (albeit intact due to
>>>>>>> the use of Z_SYNC_FLUSH) resulting in the occasional short object
>>>>>>> capture.
>>>>>>>
>>>>>>> Testcase: igt/i915-error-capture.js
>>>>>>> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>>>> Cc: <stable@vger.kernel.org> # v4.10+
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++------
>>>>>>>      1 file changed, 47 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>>>> index 3d5554f14dfd..ed8c16cbfaa4 100644
>>>>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>>>> @@ -237,6 +237,7 @@ static int compress_page(struct compress *c,
>>>>>>>                           struct drm_i915_error_object *dst)
>>>>>>>      {
>>>>>>>          struct z_stream_s *zstream = &c->zstream;
>>>>>>> +     int flush = Z_NO_FLUSH;
>>>>>>>      
>>>>>>>          zstream->next_in = src;
>>>>>>>          if (c->tmp && i915_memcpy_from_wc(c->tmp, src, PAGE_SIZE))
>>>>>>> @@ -257,8 +258,11 @@ static int compress_page(struct compress *c,
>>>>>>>                          zstream->avail_out = PAGE_SIZE;
>>>>>>>                  }
>>>>>>
>>>>>> Looks like the block above, not shown in this diff, could use a check
>>>>>> and abort if the dst->page_count overgrows the pessimistic allocation of
>>>>>> the array.
>>>>>>
>>>>>>>      
>>>>>>> -             if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
>>>>>>> +             if (zlib_deflate(zstream, flush) != Z_OK)
>>>>>>
>>>>>> So this (not always asking for a flush) actually not only fixes the
>>>>>> flush at the end but improves the API usage and potentially compression
>>>>>> ratio, correct?
>>>>>
>>>>> Yes.
>>>>>     
>>>>>>>                          return -EIO;
>>>>>>> +
>>>>>>> +             if (zstream->avail_out)
>>>>>>> +                     flush = Z_SYNC_FLUSH;
>>>>>>
>>>>>> Hm.. but why this? It will flush only occasionally, when one input page
>>>>>> did not fit in the available output - but that will depend on
>>>>>> compressibility so I don't think it has a fixed period. It is not for
>>>>>> instance for every 4k of compressed output, if that was maybe the goal.
>>>>>
>>>>> My thinking is that if the zlib_deflate() wants to defer to fill its
>>>>> window (or whatever) but we need to cross a page boundary, we have to
>>>>> push everything from the current page before we change the PTE.
>>>>
>>>> Hm but I am not sure this is achieving that, or that it is needed. I
>>>> read the docs many times and I am not sure how it is supposed to work.
>>>>
>>>> First thing I am not sure of is whether we need to ask for any flushes,
>>>> or it would be enough to loop until avail_in > 0 || avail_out == 0.
>>>> Since docs say that if avail_out == 0, zlib_deflate needs to be called
>>>> again in the same flush mode.
>>>
>>> Iirc, Z_SYNC_FLUSH talks about terminating the compression block earlier
>>> in order to consume next_avail, without the SYNC repeating a call is
>>> allowed to not progress.
>>>
>>>> Current code fails to ensure this. It would happen on the next call to
>>>> compress_page, but a) I think that is unclean and b) flush mode will not
>>>> match.
>>>
>>> Fails to ensure what exactly? I think it is obeying the rules on flush:
>>>
>>>     If the parameter flush is set to Z_SYNC_FLUSH, all pending output is
>>>     flushed to the output buffer and the output is aligned on a byte boundary, so
>>>     that the decompressor can get all input data available so far. (In particular
>>>     avail_in is zero after the call if enough output space has been provided
>>>     before the call.)  Flushing may degrade compression for some compression
>>>     algorithms and so it should be used only when necessary.
>>
>> I am not sure we need to flush - I don't see that it says in the docs
>> that we have to. AFAIU when avail_in drops to zero we are allowed to
>> move on. Anything outstanding in the compressor would be then flushed
>> either on processing the next page, or on stream close.
> 
> The case we want to handle is avail_in != 0. We cannot move on until
> avail_in is 0.

That's what I said! :) But I don't see in the docs that is says we have 
to flush after avail_in is zero, nor when avail_out is zero.

> 
>>>     ...
>>>
>>>     If deflate returns with avail_out == 0, this function must be called again
>>>     with the same value of the flush parameter and more output space (updated
>>>     avail_out), until the flush is complete (deflate returns with non-zero
>>>     avail_out).
>>
>> Yes, but the patch does not do that. You enable the flush mode when
>> avail_out is non zero, but then two options are possible. Either
>> avail_in is zero, in which case nothing happens, or it is not. In the
>> latter case there is another flushing call to zlib_deflate. If avail_out
>> is zero after that call, and avail_in is also zero, the documentation
>> paragraph above says you have to call zlib_deflate in the same flush
>> mode again.
> 
> Read the paragraph again, specifically when avail_out == 0 you should
> allocate more space and rerun the compressor. If avail_out != 0 and
> avail_in != 0 we want to SYNC in order to consume the remaining
> avail_in.

Sorry I don't see where it says that you have to SYNC_FLUSH in any 
circumstance apart from influencing the stream composition either for 
error recovery or interactive applications?

Furthermore, as I wrote above, when you start a flush in this patch you 
don't ensure the next zlib_deflate call is with the same flush mode 
(since it can be only on the next call to compress_page).

Z_OK && avail_out == 0 it says you have to run it again, so that too 
would be more intuitive being done in scope of a single compress_page 
call. (It doesn't say avail_in == 0 && avail_out == 0 is not possible.)

Regards,

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

✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)
URL   : https://patchwork.freedesktop.org/series/50408/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4915_full -> Patchwork_10324_full =

== Summary - FAILURE ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-xtiled:
      shard-skl:          PASS -> FAIL +1

    igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
      shard-skl:          NOTRUN -> FAIL

    
    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@gem_workarounds@suspend-resume-context:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
      shard-kbl:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_chv_cursor_fail@pipe-a-256x256-left-edge:
      shard-skl:          PASS -> FAIL (fdo#104671)

    igt@kms_cursor_crc@cursor-256x256-random:
      shard-skl:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-256x85-sliding:
      shard-glk:          PASS -> FAIL (fdo#103232) +3

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +5

    igt@kms_draw_crc@draw-method-xrgb2101010-mmap-cpu-xtiled:
      shard-skl:          PASS -> FAIL (fdo#103184)

    igt@kms_draw_crc@draw-method-xrgb8888-blt-ytiled:
      shard-glk:          PASS -> FAIL (fdo#107589)

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-kbl:          PASS -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
      shard-apl:          PASS -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
      shard-glk:          PASS -> FAIL (fdo#103167) +3

    igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt:
      shard-skl:          PASS -> FAIL (fdo#105682)

    igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-pwrite:
      shard-skl:          PASS -> FAIL (fdo#103167) +2

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane@pixel-format-pipe-b-planes:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#106885)

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

    igt@kms_setmode@basic:
      shard-snb:          NOTRUN -> FAIL (fdo#99912)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@kms_ccs@pipe-b-missing-ccs-buffer:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +14

    igt@kms_chv_cursor_fail@pipe-a-256x256-top-edge:
      shard-skl:          FAIL (fdo#104671) -> PASS

    igt@kms_color@pipe-b-ctm-max:
      shard-apl:          FAIL -> PASS

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          FAIL (fdo#103232) -> PASS
      shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS

    igt@kms_cursor_crc@cursor-64x21-onscreen:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
      shard-glk:          FAIL (fdo#103167) -> PASS +2

    igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite:
      shard-skl:          FAIL (fdo#103167) -> PASS +2

    igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-msflip-blt:
      shard-skl:          FAIL (fdo#105682) -> PASS

    igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-render:
      shard-skl:          FAIL (fdo#103167) -> SKIP

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      shard-skl:          FAIL (fdo#103191) -> PASS

    igt@kms_plane@plane-panning-top-left-pipe-a-planes:
      shard-skl:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
      shard-glk:          FAIL -> PASS

    igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
      shard-skl:          FAIL -> PASS

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

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

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

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
  fdo#107589 https://bugs.freedesktop.org/show_bug.cgi?id=107589
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4915 -> Patchwork_10324

  CI_DRM_4915: 26e7a7d954a9c28b97af8ca7813f430fd9117232 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4660: d0975646c50568e66e65b44b81d28232d059b94e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10324: b693af3ecd245d4c8abc93a21863a3d42398907c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

[PATCH v3] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

The final call to zlib_deflate(Z_FINISH) may require more output
space to be allocated and so needs to re-invoked. Failure to do so in
the current code leads to incomplete zlib streams (albeit intact due to
the use of Z_SYNC_FLUSH) resulting in the occasional short object
capture.

v2: Check against overrunning our pre-allocated page array
v3: Drop Z_SYNC_FLUSH entirely

Testcase: igt/i915-error-capture.js
Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.10+
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 88 +++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gpu_error.h |  1 +
 2 files changed, 64 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 3d5554f14dfd..705ff122100f 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -232,6 +232,20 @@ static bool compress_init(struct compress *c)
 	return true;
 }
 
+static void *compress_next_page(struct drm_i915_error_object *dst)
+{
+	unsigned long page;
+
+	if (dst->page_count >= dst->num_pages)
+		return ERR_PTR(-ENOSPC);
+
+	page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
+	if (!page)
+		return ERR_PTR(-ENOMEM);
+
+	return dst->pages[dst->page_count++] = (void *)page;
+}
+
 static int compress_page(struct compress *c,
 			 void *src,
 			 struct drm_i915_error_object *dst)
@@ -245,19 +259,14 @@ static int compress_page(struct compress *c,
 
 	do {
 		if (zstream->avail_out == 0) {
-			unsigned long page;
-
-			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
-			if (!page)
-				return -ENOMEM;
+			zstream->next_out = compress_next_page(dst);
+			if (IS_ERR(zstream->next_out))
+				return PTR_ERR(zstream->next_out);
 
-			dst->pages[dst->page_count++] = (void *)page;
-
-			zstream->next_out = (void *)page;
 			zstream->avail_out = PAGE_SIZE;
 		}
 
-		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
+		if (zlib_deflate(zstream, Z_NO_FLUSH) != Z_OK)
 			return -EIO;
 	} while (zstream->avail_in);
 
@@ -268,19 +277,42 @@ static int compress_page(struct compress *c,
 	return 0;
 }
 
-static void compress_fini(struct compress *c,
+static int compress_flush(struct compress *c,
 			  struct drm_i915_error_object *dst)
 {
 	struct z_stream_s *zstream = &c->zstream;
 
-	if (dst) {
-		zlib_deflate(zstream, Z_FINISH);
-		dst->unused = zstream->avail_out;
-	}
+	do {
+		switch (zlib_deflate(zstream, Z_FINISH)) {
+		case Z_OK: /* more space requested */
+			zstream->next_out = compress_next_page(dst);
+			if (IS_ERR(zstream->next_out))
+				return PTR_ERR(zstream->next_out);
+
+			zstream->avail_out = PAGE_SIZE;
+			break;
+
+		case Z_STREAM_END:
+			goto end;
+
+		default: /* any error */
+			return -EIO;
+		}
+	} while (1);
+
+end:
+	memset(zstream->next_out, 0, zstream->avail_out);
+	dst->unused = zstream->avail_out;
+	return 0;
+}
+
+static void compress_fini(struct compress *c,
+			  struct drm_i915_error_object *dst)
+{
+	struct z_stream_s *zstream = &c->zstream;
 
 	zlib_deflateEnd(zstream);
 	kfree(zstream->workspace);
-
 	if (c->tmp)
 		free_page((unsigned long)c->tmp);
 }
@@ -319,6 +351,12 @@ static int compress_page(struct compress *c,
 	return 0;
 }
 
+static int compress_flush(struct compress *c,
+			  struct drm_i915_error_object *dst)
+{
+	return 0;
+}
+
 static void compress_fini(struct compress *c,
 			  struct drm_i915_error_object *dst)
 {
@@ -917,6 +955,7 @@ i915_error_object_create(struct drm_i915_private *i915,
 	unsigned long num_pages;
 	struct sgt_iter iter;
 	dma_addr_t dma;
+	int ret;
 
 	if (!vma)
 		return NULL;
@@ -930,6 +969,7 @@ i915_error_object_create(struct drm_i915_private *i915,
 
 	dst->gtt_offset = vma->node.start;
 	dst->gtt_size = vma->node.size;
+	dst->num_pages = num_pages;
 	dst->page_count = 0;
 	dst->unused = 0;
 
@@ -938,28 +978,26 @@ i915_error_object_create(struct drm_i915_private *i915,
 		return NULL;
 	}
 
+	ret = -EINVAL;
 	for_each_sgt_dma(dma, iter, vma->pages) {
 		void __iomem *s;
-		int ret;
 
 		ggtt->vm.insert_page(&ggtt->vm, dma, slot, I915_CACHE_NONE, 0);
 
 		s = io_mapping_map_atomic_wc(&ggtt->iomap, slot);
 		ret = compress_page(&compress, (void  __force *)s, dst);
 		io_mapping_unmap_atomic(s);
-
 		if (ret)
-			goto unwind;
+			break;
 	}
-	goto out;
 
-unwind:
-	while (dst->page_count--)
-		free_page((unsigned long)dst->pages[dst->page_count]);
-	kfree(dst);
-	dst = NULL;
+	if (ret || compress_flush(&compress, dst)) {
+		while (dst->page_count--)
+			free_page((unsigned long)dst->pages[dst->page_count]);
+		kfree(dst);
+		dst = NULL;
+	}
 
-out:
 	compress_fini(&compress, dst);
 	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
 	return dst;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
index f893a4e8b783..8710fb18ed74 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.h
+++ b/drivers/gpu/drm/i915/i915_gpu_error.h
@@ -135,6 +135,7 @@ struct i915_gpu_state {
 		struct drm_i915_error_object {
 			u64 gtt_offset;
 			u64 gtt_size;
+			int num_pages;
 			int page_count;
 			int unused;
 			u32 *pages[0];
-- 
2.19.0

Re: [Intel-gfx] [PATCH v3] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Tvrtko Ursulin]

On 03/10/2018 09:24, Chris Wilson wrote:
> The final call to zlib_deflate(Z_FINISH) may require more output
> space to be allocated and so needs to re-invoked. Failure to do so in
> the current code leads to incomplete zlib streams (albeit intact due to
> the use of Z_SYNC_FLUSH) resulting in the occasional short object
> capture.
> 
> v2: Check against overrunning our pre-allocated page array
> v3: Drop Z_SYNC_FLUSH entirely
> 
> Testcase: igt/i915-error-capture.js
> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gpu_error.c | 88 +++++++++++++++++++--------
>   drivers/gpu/drm/i915/i915_gpu_error.h |  1 +
>   2 files changed, 64 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 3d5554f14dfd..705ff122100f 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -232,6 +232,20 @@ static bool compress_init(struct compress *c)
>   	return true;
>   }
>   
> +static void *compress_next_page(struct drm_i915_error_object *dst)
> +{
> +	unsigned long page;
> +
> +	if (dst->page_count >= dst->num_pages)
> +		return ERR_PTR(-ENOSPC);
> +
> +	page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> +	if (!page)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return dst->pages[dst->page_count++] = (void *)page;
> +}
> +
>   static int compress_page(struct compress *c,
>   			 void *src,
>   			 struct drm_i915_error_object *dst)
> @@ -245,19 +259,14 @@ static int compress_page(struct compress *c,
>   
>   	do {
>   		if (zstream->avail_out == 0) {
> -			unsigned long page;
> -
> -			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> -			if (!page)
> -				return -ENOMEM;
> +			zstream->next_out = compress_next_page(dst);
> +			if (IS_ERR(zstream->next_out))
> +				return PTR_ERR(zstream->next_out);
>   
> -			dst->pages[dst->page_count++] = (void *)page;
> -
> -			zstream->next_out = (void *)page;
>   			zstream->avail_out = PAGE_SIZE;
>   		}
>   
> -		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> +		if (zlib_deflate(zstream, Z_NO_FLUSH) != Z_OK)
>   			return -EIO;
>   	} while (zstream->avail_in);
>   
> @@ -268,19 +277,42 @@ static int compress_page(struct compress *c,
>   	return 0;
>   }
>   
> -static void compress_fini(struct compress *c,
> +static int compress_flush(struct compress *c,
>   			  struct drm_i915_error_object *dst)
>   {
>   	struct z_stream_s *zstream = &c->zstream;
>   
> -	if (dst) {
> -		zlib_deflate(zstream, Z_FINISH);
> -		dst->unused = zstream->avail_out;
> -	}
> +	do {
> +		switch (zlib_deflate(zstream, Z_FINISH)) {
> +		case Z_OK: /* more space requested */
> +			zstream->next_out = compress_next_page(dst);
> +			if (IS_ERR(zstream->next_out))
> +				return PTR_ERR(zstream->next_out);
> +
> +			zstream->avail_out = PAGE_SIZE;
> +			break;
> +
> +		case Z_STREAM_END:
> +			goto end;
> +
> +		default: /* any error */
> +			return -EIO;
> +		}
> +	} while (1);
> +
> +end:
> +	memset(zstream->next_out, 0, zstream->avail_out);
> +	dst->unused = zstream->avail_out;
> +	return 0;
> +}
> +
> +static void compress_fini(struct compress *c,
> +			  struct drm_i915_error_object *dst)
> +{
> +	struct z_stream_s *zstream = &c->zstream;
>   
>   	zlib_deflateEnd(zstream);
>   	kfree(zstream->workspace);
> -
>   	if (c->tmp)
>   		free_page((unsigned long)c->tmp);
>   }
> @@ -319,6 +351,12 @@ static int compress_page(struct compress *c,
>   	return 0;
>   }
>   
> +static int compress_flush(struct compress *c,
> +			  struct drm_i915_error_object *dst)
> +{
> +	return 0;
> +}
> +
>   static void compress_fini(struct compress *c,
>   			  struct drm_i915_error_object *dst)
>   {
> @@ -917,6 +955,7 @@ i915_error_object_create(struct drm_i915_private *i915,
>   	unsigned long num_pages;
>   	struct sgt_iter iter;
>   	dma_addr_t dma;
> +	int ret;
>   
>   	if (!vma)
>   		return NULL;
> @@ -930,6 +969,7 @@ i915_error_object_create(struct drm_i915_private *i915,
>   
>   	dst->gtt_offset = vma->node.start;
>   	dst->gtt_size = vma->node.size;
> +	dst->num_pages = num_pages;
>   	dst->page_count = 0;
>   	dst->unused = 0;
>   
> @@ -938,28 +978,26 @@ i915_error_object_create(struct drm_i915_private *i915,
>   		return NULL;
>   	}
>   
> +	ret = -EINVAL;
>   	for_each_sgt_dma(dma, iter, vma->pages)  >   		void __iomem *s;
> -		int ret;
>   
>   		ggtt->vm.insert_page(&ggtt->vm, dma, slot, I915_CACHE_NONE, 0);
>   
>   		s = io_mapping_map_atomic_wc(&ggtt->iomap, slot);
>   		ret = compress_page(&compress, (void  __force *)s, dst);
>   		io_mapping_unmap_atomic(s);
> -
>   		if (ret)
> -			goto unwind;
> +			break;
>   	}
> -	goto out;
>   
> -unwind:
> -	while (dst->page_count--)
> -		free_page((unsigned long)dst->pages[dst->page_count]);
> -	kfree(dst);
> -	dst = NULL;
> +	if (ret || compress_flush(&compress, dst)) {
> +		while (dst->page_count--)
> +			free_page((unsigned long)dst->pages[dst->page_count]);
> +		kfree(dst);
> +		dst = NULL;
> +	}
>   
> -out:
>   	compress_fini(&compress, dst);
>   	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
>   	return dst;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
> index f893a4e8b783..8710fb18ed74 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.h
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.h
> @@ -135,6 +135,7 @@ struct i915_gpu_state {
>   		struct drm_i915_error_object {
>   			u64 gtt_offset;
>   			u64 gtt_size;
> +			int num_pages;
>   			int page_count;
>   			int unused;
>   			u32 *pages[0];
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [PATCH v3] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Tvrtko Ursulin]

On 03/10/2018 09:24, Chris Wilson wrote:
> The final call to zlib_deflate(Z_FINISH) may require more output
> space to be allocated and so needs to re-invoked. Failure to do so in
> the current code leads to incomplete zlib streams (albeit intact due to
> the use of Z_SYNC_FLUSH) resulting in the occasional short object
> capture.
> 
> v2: Check against overrunning our pre-allocated page array
> v3: Drop Z_SYNC_FLUSH entirely
> 
> Testcase: igt/i915-error-capture.js
> Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gpu_error.c | 88 +++++++++++++++++++--------
>   drivers/gpu/drm/i915/i915_gpu_error.h |  1 +
>   2 files changed, 64 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 3d5554f14dfd..705ff122100f 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -232,6 +232,20 @@ static bool compress_init(struct compress *c)
>   	return true;
>   }
>   
> +static void *compress_next_page(struct drm_i915_error_object *dst)
> +{
> +	unsigned long page;
> +
> +	if (dst->page_count >= dst->num_pages)
> +		return ERR_PTR(-ENOSPC);
> +
> +	page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> +	if (!page)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return dst->pages[dst->page_count++] = (void *)page;
> +}
> +
>   static int compress_page(struct compress *c,
>   			 void *src,
>   			 struct drm_i915_error_object *dst)
> @@ -245,19 +259,14 @@ static int compress_page(struct compress *c,
>   
>   	do {
>   		if (zstream->avail_out == 0) {
> -			unsigned long page;
> -
> -			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> -			if (!page)
> -				return -ENOMEM;
> +			zstream->next_out = compress_next_page(dst);
> +			if (IS_ERR(zstream->next_out))
> +				return PTR_ERR(zstream->next_out);
>   
> -			dst->pages[dst->page_count++] = (void *)page;
> -
> -			zstream->next_out = (void *)page;
>   			zstream->avail_out = PAGE_SIZE;
>   		}
>   
> -		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> +		if (zlib_deflate(zstream, Z_NO_FLUSH) != Z_OK)
>   			return -EIO;
>   	} while (zstream->avail_in);
>   
> @@ -268,19 +277,42 @@ static int compress_page(struct compress *c,
>   	return 0;
>   }
>   
> -static void compress_fini(struct compress *c,
> +static int compress_flush(struct compress *c,
>   			  struct drm_i915_error_object *dst)
>   {
>   	struct z_stream_s *zstream = &c->zstream;
>   
> -	if (dst) {
> -		zlib_deflate(zstream, Z_FINISH);
> -		dst->unused = zstream->avail_out;
> -	}
> +	do {
> +		switch (zlib_deflate(zstream, Z_FINISH)) {
> +		case Z_OK: /* more space requested */
> +			zstream->next_out = compress_next_page(dst);
> +			if (IS_ERR(zstream->next_out))
> +				return PTR_ERR(zstream->next_out);
> +
> +			zstream->avail_out = PAGE_SIZE;
> +			break;
> +
> +		case Z_STREAM_END:
> +			goto end;
> +
> +		default: /* any error */
> +			return -EIO;
> +		}
> +	} while (1);
> +
> +end:
> +	memset(zstream->next_out, 0, zstream->avail_out);
> +	dst->unused = zstream->avail_out;
> +	return 0;
> +}
> +
> +static void compress_fini(struct compress *c,
> +			  struct drm_i915_error_object *dst)
> +{
> +	struct z_stream_s *zstream = &c->zstream;
>   
>   	zlib_deflateEnd(zstream);
>   	kfree(zstream->workspace);
> -
>   	if (c->tmp)
>   		free_page((unsigned long)c->tmp);
>   }
> @@ -319,6 +351,12 @@ static int compress_page(struct compress *c,
>   	return 0;
>   }
>   
> +static int compress_flush(struct compress *c,
> +			  struct drm_i915_error_object *dst)
> +{
> +	return 0;
> +}
> +
>   static void compress_fini(struct compress *c,
>   			  struct drm_i915_error_object *dst)
>   {
> @@ -917,6 +955,7 @@ i915_error_object_create(struct drm_i915_private *i915,
>   	unsigned long num_pages;
>   	struct sgt_iter iter;
>   	dma_addr_t dma;
> +	int ret;
>   
>   	if (!vma)
>   		return NULL;
> @@ -930,6 +969,7 @@ i915_error_object_create(struct drm_i915_private *i915,
>   
>   	dst->gtt_offset = vma->node.start;
>   	dst->gtt_size = vma->node.size;
> +	dst->num_pages = num_pages;
>   	dst->page_count = 0;
>   	dst->unused = 0;
>   
> @@ -938,28 +978,26 @@ i915_error_object_create(struct drm_i915_private *i915,
>   		return NULL;
>   	}
>   
> +	ret = -EINVAL;
>   	for_each_sgt_dma(dma, iter, vma->pages)  >   		void __iomem *s;
> -		int ret;
>   
>   		ggtt->vm.insert_page(&ggtt->vm, dma, slot, I915_CACHE_NONE, 0);
>   
>   		s = io_mapping_map_atomic_wc(&ggtt->iomap, slot);
>   		ret = compress_page(&compress, (void  __force *)s, dst);
>   		io_mapping_unmap_atomic(s);
> -
>   		if (ret)
> -			goto unwind;
> +			break;
>   	}
> -	goto out;
>   
> -unwind:
> -	while (dst->page_count--)
> -		free_page((unsigned long)dst->pages[dst->page_count]);
> -	kfree(dst);
> -	dst = NULL;
> +	if (ret || compress_flush(&compress, dst)) {
> +		while (dst->page_count--)
> +			free_page((unsigned long)dst->pages[dst->page_count]);
> +		kfree(dst);
> +		dst = NULL;
> +	}
>   
> -out:
>   	compress_fini(&compress, dst);
>   	ggtt->vm.clear_range(&ggtt->vm, slot, PAGE_SIZE);
>   	return dst;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
> index f893a4e8b783..8710fb18ed74 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.h
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.h
> @@ -135,6 +135,7 @@ struct i915_gpu_state {
>   		struct drm_i915_error_object {
>   			u64 gtt_offset;
>   			u64 gtt_size;
> +			int num_pages;
>   			int page_count;
>   			int unused;
>   			u32 *pages[0];
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev3)
URL   : https://patchwork.freedesktop.org/series/50408/
State : failure

== Summary ==

Applying: drm/i915: Replace some open-coded i915_map_coherent_type()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_ringbuffer.c
M	drivers/gpu/drm/i915/selftests/intel_hangcheck.c
M	drivers/gpu/drm/i915/selftests/intel_lrc.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915: Handle incomplete Z_FINISH for compressed error states
Applying: drm/i915: Clear the error PTE just once on finish
Applying: drm/i915: Cache the error string
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_debugfs.c
M	drivers/gpu/drm/i915/i915_gpu_error.c
M	drivers/gpu/drm/i915/i915_gpu_error.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_gpu_error.h
Auto-merging drivers/gpu/drm/i915/i915_gpu_error.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_gpu_error.c
Auto-merging drivers/gpu/drm/i915/i915_debugfs.c
error: Failed to merge in the changes.
Patch failed at 0004 drm/i915: Cache the error string
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: ✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)

[Martin Peres]

On 03/10/2018 05:47, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/4] drm/i915: Replace some open-coded i915_map_coherent_type() (rev2)
> URL   : https://patchwork.freedesktop.org/series/50408/
> State : failure
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4915_full -> Patchwork_10324_full =
> 
> == Summary - FAILURE ==
> 
>   Serious unknown changes coming with Patchwork_10324_full absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_10324_full, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   
> 
> == Possible new issues ==
> 
>   Here are the unknown changes that may have been introduced in Patchwork_10324_full:
> 
>   === IGT changes ===
> 
>     ==== Possible regressions ====
> 
>     igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-xtiled:
>       shard-skl:          PASS -> FAIL +1
> 
>     igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
>       shard-skl:          NOTRUN -> FAIL

Known issue: https://bugs.freedesktop.org/show_bug.cgi?id=108145

Martin
> 
>     
>     ==== Warnings ====
> 
>     igt@pm_rc6_residency@rc6-accuracy:
>       shard-snb:          SKIP -> PASS
> 
>     
> == Known issues ==
> 
>   Here are the changes found in Patchwork_10324_full that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@gem_exec_suspend@basic-s3:
>       shard-snb:          PASS -> INCOMPLETE (fdo#105411)
> 
>     igt@gem_workarounds@suspend-resume-context:
>       shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)
> 
>     igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
>       shard-kbl:          PASS -> DMESG-WARN (fdo#107956)
> 
>     igt@kms_chv_cursor_fail@pipe-a-256x256-left-edge:
>       shard-skl:          PASS -> FAIL (fdo#104671)
> 
>     igt@kms_cursor_crc@cursor-256x256-random:
>       shard-skl:          NOTRUN -> FAIL (fdo#103232)
> 
>     igt@kms_cursor_crc@cursor-256x85-sliding:
>       shard-glk:          PASS -> FAIL (fdo#103232) +3
> 
>     igt@kms_cursor_crc@cursor-64x21-random:
>       shard-apl:          PASS -> FAIL (fdo#103232) +5
> 
>     igt@kms_draw_crc@draw-method-xrgb2101010-mmap-cpu-xtiled:
>       shard-skl:          PASS -> FAIL (fdo#103184)
> 
>     igt@kms_draw_crc@draw-method-xrgb8888-blt-ytiled:
>       shard-glk:          PASS -> FAIL (fdo#107589)
> 
>     igt@kms_flip@plain-flip-fb-recreate-interruptible:
>       shard-kbl:          PASS -> FAIL (fdo#100368)
> 
>     igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
>       shard-apl:          PASS -> FAIL (fdo#103167) +2
> 
>     igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
>       shard-glk:          PASS -> FAIL (fdo#103167) +3
> 
>     igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt:
>       shard-skl:          PASS -> FAIL (fdo#105682)
> 
>     igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-pwrite:
>       shard-skl:          PASS -> FAIL (fdo#103167) +2
> 
>     igt@kms_plane@pixel-format-pipe-a-planes:
>       shard-apl:          PASS -> FAIL (fdo#103166)
> 
>     igt@kms_plane@pixel-format-pipe-b-planes:
>       shard-skl:          NOTRUN -> DMESG-WARN (fdo#106885)
> 
>     igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
>       shard-glk:          PASS -> FAIL (fdo#103166) +1
> 
>     igt@kms_setmode@basic:
>       shard-snb:          NOTRUN -> FAIL (fdo#99912)
> 
>     igt@perf@polling:
>       shard-hsw:          PASS -> FAIL (fdo#102252)
> 
>     
>     ==== Possible fixes ====
> 
>     igt@kms_ccs@pipe-b-missing-ccs-buffer:
>       shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +14
> 
>     igt@kms_chv_cursor_fail@pipe-a-256x256-top-edge:
>       shard-skl:          FAIL (fdo#104671) -> PASS
> 
>     igt@kms_color@pipe-b-ctm-max:
>       shard-apl:          FAIL -> PASS
> 
>     igt@kms_cursor_crc@cursor-128x128-suspend:
>       shard-glk:          FAIL (fdo#103232) -> PASS
>       shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS
> 
>     igt@kms_cursor_crc@cursor-64x21-onscreen:
>       shard-apl:          FAIL (fdo#103232) -> PASS
> 
>     igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
>       shard-glk:          FAIL (fdo#105363) -> PASS
> 
>     igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
>       shard-apl:          FAIL (fdo#103167) -> PASS +1
> 
>     igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
>       shard-glk:          FAIL (fdo#103167) -> PASS +2
> 
>     igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite:
>       shard-skl:          FAIL (fdo#103167) -> PASS +2
> 
>     igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-msflip-blt:
>       shard-skl:          FAIL (fdo#105682) -> PASS
> 
>     igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-render:
>       shard-skl:          FAIL (fdo#103167) -> SKIP
> 
>     igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
>       shard-skl:          FAIL (fdo#103191) -> PASS
> 
>     igt@kms_plane@plane-panning-top-left-pipe-a-planes:
>       shard-skl:          FAIL (fdo#103166) -> PASS
> 
>     igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
>       shard-glk:          FAIL -> PASS
> 
>     igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
>       shard-skl:          FAIL -> PASS
> 
>     igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
>       shard-apl:          FAIL (fdo#103166) -> PASS +1
> 
>     igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
>       shard-glk:          FAIL (fdo#103166) -> PASS
> 
>     igt@kms_setmode@basic:
>       shard-apl:          FAIL (fdo#99912) -> PASS
>       shard-hsw:          FAIL (fdo#99912) -> PASS
> 
>     
>   fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
>   fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
>   fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
>   fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
>   fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
>   fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
>   fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
>   fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
>   fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
>   fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
>   fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
>   fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
>   fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
>   fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
>   fdo#107589 https://bugs.freedesktop.org/show_bug.cgi?id=107589
>   fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
>   fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
>   fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
> 
> 
> == Participating hosts (6 -> 6) ==
> 
>   No changes in participating hosts
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_4915 -> Patchwork_10324
> 
>   CI_DRM_4915: 26e7a7d954a9c28b97af8ca7813f430fd9117232 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4660: d0975646c50568e66e65b44b81d28232d059b94e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_10324: b693af3ecd245d4c8abc93a21863a3d42398907c @ git://anongit.freedesktop.org/gfx-ci/linux
>   piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10324/shards.html
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Handle incomplete Z_FINISH for compressed error states

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-03 10:04:04)
> 
> On 03/10/2018 09:24, Chris Wilson wrote:
> > The final call to zlib_deflate(Z_FINISH) may require more output
> > space to be allocated and so needs to re-invoked. Failure to do so in
> > the current code leads to incomplete zlib streams (albeit intact due to
> > the use of Z_SYNC_FLUSH) resulting in the occasional short object
> > capture.
> > 
> > v2: Check against overrunning our pre-allocated page array
> > v3: Drop Z_SYNC_FLUSH entirely
> > 
> > Testcase: igt/i915-error-capture.js
> > Fixes: 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.10+
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Ta, thanks for the review. gem_exec_capture with several million
objects/pages was happy so pushed.

Now, we just need the O(N^2) fix so that gem_exec_capture runs in a few
minutes and not a few _decades_.
-Chris