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 <ch...@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: <sta...@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
