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

[Chris Wilson]

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.

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