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 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