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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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