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.
I'd also throw the routines under lib/
> @@ -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?
> +
> + 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?
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
