[Intel-gfx] [PATCH 33/33] drm/i915: Compress GPU objects in error state

[Chris Wilson]

On Wed, Aug 10, 2016 at 01:32:29PM +0300, Joonas Lahtinen wrote:
> On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> > @@ -309,12 +310,30 @@ void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
> >  	va_end(args);
> >  }
> >  
> > +static bool
> > +ascii85_encode(u32 in, char *out)
> 
> 
> base64 is more de facto and I bet userland "expects" it too.

No. It expects a standard zlib compressed ascii85 stream.

> > @@ -326,13 +345,23 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
> >  			   lower_32_bits(obj->gtt_offset));
> >  	}
> >  
> > -	for (page = offset = 0; page < obj->page_count; page++) {
> > -		for (elt = 0; elt < PAGE_SIZE/4; elt++) {
> > -			err_printf(m, "%08x :  %08x\n", offset,
> > -				   obj->pages[page][elt]);
> > -			offset += 4;
> > +	err_puts(m, ":"); /* indicate compressed data */
> 
> I'd also keep the the uncompressed option, because somebody might be
> trying to make a micro-kernel without extra algorithms options. A
> config setting could be justified.
> 
> > +	for (page = 0; page < obj->page_count; page++) {
> > +		int i, len;
> > +
> > +		len = PAGE_SIZE;
> > +		if (page == obj->page_count - 1)
> > +			len -= obj->unused;
> > +		len = (len + 3) / 4;
> > +
> > +		for (i = 0; i < len; i++) {
> > +			if (ascii85_encode(obj->pages[page][i], out))
> > +				err_puts(m, out);
> > +			else
> > +				err_puts(m, "z");
> 
> I think the encode function could take ranges, you do not very often
> care about encoding/decoding single character.

> >  		}
> >  	}
> > +	err_puts(m, "\n");
> >  }
> >  
> >  int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> > @@ -593,17 +622,37 @@ static void i915_error_state_free(struct kref *error_ref)
> >  	kfree(error);
> >  }
> >  
> > -static int compress_page(void *src, struct drm_i915_error_object *dst)
> > +static int compress_page(struct z_stream_s *zstream,
> > +			 void *src,
> > +			 struct drm_i915_error_object *dst)
> >  {
> > -	unsigned long page;
> > +	zstream->next_in = src;
> > +	zstream->avail_in = PAGE_SIZE;
> >  
> > -	page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> > -	if (!page)
> > -		return -ENOMEM;
> > +	do {
> > +		if (zstream->avail_out == 0) {
> > +			unsigned long page;
> > +
> > +			page = __get_free_page(GFP_ATOMIC | __GFP_NOWARN);
> > +			if (!page)
> > +				return -ENOMEM;
> > +
> > +			dst->pages[dst->page_count++] = (void *)page;
> 
> Why is not dst->pages of different type?

You want dst->pages[] as an array of unsigned long?

> 
> > +
> > +			zstream->next_out = (void *)page;
> > +			zstream->avail_out = PAGE_SIZE;
> > +		}
> >  
> > -	dst->pages[dst->page_count++] = (void *)page;
> > +		if (zlib_deflate(zstream, Z_SYNC_FLUSH) != Z_OK)
> > +			return -EIO;
> > +
> > +#if 0
> > +		/* XXX fallback to uncompressed if we increases size? */
> > +		if (zstream->total_out > zstream->total_in)
> > +			return -E2BIG;
> > +#endif
> 
> Not something we would merge. FIXME: or TODO: comment should be enough,
> or make it DRM_INFO and we can act if we get reports?
> 
> > +	} while (zstream->avail_in);
> >  
> > -	memcpy((void *)page, src, PAGE_SIZE);
> 
> // The function name has been so descriptive previously :P
> 
> > @@ -622,6 +672,7 @@ i915_error_object_create(struct drm_i915_private *i915,
> >  		return NULL;
> >  
> >  	num_pages = min_t(u64, vma->size, vma->obj->base.size) >> PAGE_SHIFT;
> > +	num_pages = DIV_ROUND_UP(10 * num_pages, 8); /* worstcase zlib growth */
> 
> This kind of calculation could be made into a zlib function?
> 
> > @@ -629,6 +680,18 @@ i915_error_object_create(struct drm_i915_private *i915,
> >  
> >  	dst->gtt_offset = vma->node.start;
> >  	dst->page_count = 0;
> > +	dst->unused = 0;
> > +
> > +	memset(&zstream, 0, sizeof(zstream));
> > +	zstream.workspace = kmalloc(zlib_deflate_workspacesize(MAX_WBITS,
> > +							       MAX_MEM_LEVEL),
> > +				    GFP_ATOMIC | __GFP_NOWARN);
> 
> Wouldn't look better with an intermediate variable?

No.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre