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

Tvrtko Ursulin
2018-10-02T13:46:12Z

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