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

Tvrtko Ursulin
2018-10-02T12:20:08Z

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