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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx