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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-02 14:46:10)
> 
> 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 at chris-wilson.co.uk>
> >>>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>>>> Cc: <stable at 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.

The case we want to handle is avail_in != 0. We cannot move on until
avail_in is 0.

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

Read the paragraph again, specifically when avail_out == 0 you should
allocate more space and rerun the compressor. If avail_out != 0 and
avail_in != 0 we want to SYNC in order to consume the remaining
avail_in.
-Chris