Re: [PATCH v9 2/2] drm/xe/guc: Compress GuC log and CTB dump with zstd

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 15, 2026 at 10:48:40AM -0700, Alan Previn wrote:
> From: Zhanjun Dong <zhanjun.dong@intel.com>
> 
> With default settings, GuC log is 8MB in size and
> dumping the log is time-consuming. In debugging scenarios,
> dump-to-dmesg suffers truncation loss because CI's size limit is 2MB.
> Additionally, if also forwarded to serial console, it takes several
> minutes. In use-case of continuous error+log collection over extended
> period, we pressure CI's allocated storage. Thus, compress the
> GuC log data dump to address these issues and multiply any benefits
> from a future upgrade in CI's size limits.
> 
> Replace the raw ascii85 dumps of GuC log snapshots with a
> zstd-compressed ascii85 stream and reuse the same helper for CTB
> snapshot output.
> 
> Use zstd streaming with the default compression level of 3 and an
> estimated source size of 0 so the compression workspace stays bounded
> even for multi-megabyte logs.
> 
> Add a new Kconfig option DRM_XE_COMPRESS_DUMP to control whether
> GuC log and CTB dumps are compressed with ZSTD. This allows users to
> choose between reduced dump sizes and lower memory usage.
> 
> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Acked-by: José Roberto de Souza <jose.souza@intel.com>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Since no one had the willigness to actually review this yet, I asked AI some help

> ---
>  drivers/gpu/drm/xe/Kconfig          |  15 +++
>  drivers/gpu/drm/xe/xe_devcoredump.c |   4 +-
>  drivers/gpu/drm/xe/xe_guc_ct.c      |  19 ++--
>  drivers/gpu/drm/xe/xe_guc_ct.h      |   4 +-
>  drivers/gpu/drm/xe/xe_guc_log.c     | 161 +++++++++++++++++++++++++---
>  drivers/gpu/drm/xe/xe_guc_log.h     |   9 +-
>  6 files changed, 183 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> index 4d7dcaff2b91..18a682a44b31 100644
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -33,6 +33,7 @@ config DRM_XE
>  	select ACPI_WMI if X86 && ACPI
>  	select SYNC_FILE
>  	select CRC32
> +	select ZSTD_COMPRESS if DRM_XE_COMPRESS_DUMP

There's a circular/reverse dependency here.

Please move the select ZSTD_COMPRESS into DRM_XE_COMPRESS_DUMP
like DRM_XE_GPUSVM selects DRM_GPUSVM...

>  	select SND_HDA_I915 if SND_HDA_CORE
>  	select CEC_CORE if CEC_NOTIFIER
>  	select VMAP_PFN
> @@ -101,6 +102,20 @@ config DRM_XE_PAGEMAP
>  
>  	  If in doubt say "Y".
>  
> +config DRM_XE_COMPRESS_DUMP
> +	bool "Enable ZSTD compression for error dumps"
> +	depends on DRM_XE
> +	default y
> +	help
> +	  Enable this option to compress GuC log and CTB dumps with ZSTD
> +	  compression. This can significantly reduce the size of error dumps
> +	  but requires additional memory for compression workspace.
> +
> +	  Disable this option if you want to save memory or prefer uncompressed
> +	  error dumps for debugging.
> +
> +	  If in doubt say "Y".
> +
>  config DRM_XE_FORCE_PROBE
>  	string "Force probe xe for selected Intel hardware IDs"
>  	depends on DRM_XE
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 5f2b90b18f97..27473c25aad2 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -115,9 +115,9 @@ static ssize_t __xe_devcoredump_read(char *buffer, ssize_t count,
>  	drm_printf(&p, "\tTile: %d\n", ss->gt->tile->id);
>  
>  	drm_puts(&p, "\n**** GuC Log ****\n");
> -	xe_guc_log_snapshot_print(ss->guc.log, &p);
> +	xe_guc_log_snapshot_print(ss->guc.log, &p, ss->gt);
>  	drm_puts(&p, "\n**** GuC CT ****\n");
> -	xe_guc_ct_snapshot_print(ss->guc.ct, &p);
> +	xe_guc_ct_snapshot_print(ss->guc.ct, &p, ss->gt);
>  
>  	drm_puts(&p, "\n**** Contexts ****\n");
>  	xe_guc_exec_queue_snapshot_print(ss->ge, &p);
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 5c4733da385c..10a8cf327036 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -2186,11 +2186,12 @@ struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct)
>   * xe_guc_ct_snapshot_print - Print out a given GuC CT snapshot.
>   * @snapshot: GuC CT snapshot object.
>   * @p: drm_printer where it will be printed out.
> + * @gt: GT structure for error reporting
>   *
>   * This function prints out a given GuC CT snapshot object.
>   */
>  void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
> -			      struct drm_printer *p)
> +			      struct drm_printer *p, struct xe_gt *gt)
>  {
>  	if (!snapshot)
>  		return;
> @@ -2204,11 +2205,10 @@ void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
>  		drm_printf(p, "\tg2h outstanding: %d\n",
>  			   snapshot->g2h_outstanding);
>  
> -		if (snapshot->ctb) {
> -			drm_printf(p, "[CTB].length: 0x%zx\n", snapshot->ctb_size);
> -			xe_print_blob_ascii85(p, "[CTB].data", '\n',
> -					      snapshot->ctb, 0, snapshot->ctb_size);
> -		}
> +		if (snapshot->ctb)
> +			xe_guc_print_blob(gt, p, "[CTB]", "[CTB].data",
> +					  &snapshot->ctb, 1,
> +					  snapshot->ctb_size, snapshot->ctb_size);
>  	} else {
>  		drm_puts(p, "CT disabled\n");
>  	}
> @@ -2242,9 +2242,10 @@ void xe_guc_ct_snapshot_free(struct xe_guc_ct_snapshot *snapshot)
>  void xe_guc_ct_print(struct xe_guc_ct *ct, struct drm_printer *p, bool want_ctb)
>  {
>  	struct xe_guc_ct_snapshot *snapshot;
> +	struct xe_gt *gt = ct_to_gt(ct);
>  
>  	snapshot = guc_ct_snapshot_capture(ct, false, want_ctb);
> -	xe_guc_ct_snapshot_print(snapshot, p);
> +	xe_guc_ct_snapshot_print(snapshot, p, gt);
>  	xe_guc_ct_snapshot_free(snapshot);
>  }
>  
> @@ -2342,10 +2343,10 @@ static void ct_dead_print(struct xe_dead_ct *dead)
>  	drm_printf(&lp, "\tTile: %d\n", gt->tile->id);
>  
>  	drm_puts(&lp, "**** GuC Log ****\n");
> -	xe_guc_log_snapshot_print(dead->snapshot_log, &lp);
> +	xe_guc_log_snapshot_print(dead->snapshot_log, &lp, gt);
>  
>  	drm_puts(&lp, "**** GuC CT ****\n");
> -	xe_guc_ct_snapshot_print(dead->snapshot_ct, &lp);
> +	xe_guc_ct_snapshot_print(dead->snapshot_ct, &lp, gt);
>  
>  	drm_puts(&lp, "Done.\n");
>  }
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> index 3ddc665ab84a..f950d68a060a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> @@ -12,6 +12,7 @@
>  
>  struct drm_printer;
>  struct xe_device;
> +struct xe_gt;
>  
>  int xe_guc_ct_init_noalloc(struct xe_guc_ct *ct);
>  int xe_guc_ct_init(struct xe_guc_ct *ct);
> @@ -26,7 +27,8 @@ void xe_guc_ct_flush_and_stop(struct xe_guc_ct *ct);
>  void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
>  
>  struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct);
> -void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot, struct drm_printer *p);
> +void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot, struct drm_printer *p,
> +			      struct xe_gt *gt);
>  void xe_guc_ct_snapshot_free(struct xe_guc_ct_snapshot *snapshot);
>  void xe_guc_ct_print(struct xe_guc_ct *ct, struct drm_printer *p, bool want_ctb);
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
> index 538d4df0f7aa..aaeb0f76af30 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.c
> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
> @@ -6,6 +6,9 @@
>  #include "xe_guc_log.h"
>  
>  #include <linux/fault-inject.h>
> +#if IS_ENABLED(CONFIG_DRM_XE_COMPRESS_DUMP)
> +#include <linux/zstd.h>
> +#endif
>  
>  #include <linux/utsname.h>
>  #include <drm/drm_managed.h>
> @@ -225,16 +228,148 @@ struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct xe_guc_log *log,
>  	return snapshot;
>  }
>  
> +#if IS_ENABLED(CONFIG_DRM_XE_COMPRESS_DUMP)
> +static void xe_guc_print_blob_compressed(struct xe_gt *gt, struct drm_printer *p,
> +					 const char *length_label, const char *data_label,
> +					 void **chunks, int num_chunks,
> +					 size_t chunk_size, size_t total_size)
> +{
> +	zstd_parameters params;
> +	zstd_cstream *cstream;
> +	zstd_out_buffer outbuf;
> +	size_t wksp_size, dst_size;
> +	size_t remain, ret;
> +	void *wksp = NULL;
> +	char *dst = NULL;
> +	int i;
> +
> +	/*
> +	 * Pass 0 as estimated_src_size to use zstd level-3 defaults
> +	 * (windowLog=18, 256KB window). Passing the full total_size would
> +	 * cause zstd to inflate windowLog proportionally (e.g. windowLog=24
> +	 * for a 19MB log), making the workspace tens of MB.  With 0, the
> +	 * workspace stays ~1.5MB, well within kmalloc limits.
> +	 */
> +	params = zstd_get_params(3, 0);
> +
> +	wksp_size = zstd_cstream_workspace_bound(&params.cParams);
> +	wksp = kvzalloc(wksp_size, GFP_KERNEL);
> +	if (!wksp)
> +		goto err;
> +
> +	dst_size = zstd_compress_bound(total_size);
> +	dst = kvzalloc(dst_size, GFP_KERNEL);
> +	if (!dst)
> +		goto err;
> +
> +	cstream = zstd_init_cstream(&params, 0, wksp, wksp_size);
> +	if (!cstream) {
> +		xe_gt_err(gt, "failed to init zstd cstream\n");
> +		goto err;
> +	}
> +
> +	outbuf.dst = dst;
> +	outbuf.size = dst_size;
> +	outbuf.pos = 0;
> +
> +	remain = total_size;
> +	for (i = 0; i < num_chunks && remain; i++) {
> +		size_t size = min(chunk_size, remain);
> +		zstd_in_buffer inbuf = { .src = chunks[i], .size = size, .pos = 0 };
> +
> +		while (inbuf.pos < inbuf.size) {

LLM:
unbounded loop - the Sashiko finding is real
"
include/linux/zstd.h:504 - "Guaranteed to make some forward progress if `input`
and `output` are not empty." Once outbuf.pos == outbuf.size, forward progress is
explicitly NOT guaranteed and this spins forever. zstd_compress_bound() should
prevent output exhaustion here (single frame, no intermediate flushes), so it is
unreachable in practice - but a hard lockup in an error-dump path is bad enough
that a `if (outbuf.pos == outbuf.size) goto err;` guard is worth the one line.
"

> +			ret = zstd_compress_stream(cstream, &outbuf, &inbuf);
> +			if (ZSTD_isError(ret)) {
> +				xe_gt_err(gt, "zstd_compress_stream failed: %s\n",
> +					  ZSTD_getErrorName(ret));
> +				goto err;
> +			}
> +		}
> +		remain -= size;
> +	}
> +
> +	do {
> +		ret = zstd_end_stream(cstream, &outbuf);
> +		if (ZSTD_isError(ret)) {
> +			xe_gt_err(gt, "zstd_end_stream failed: %s\n",
> +				  ZSTD_getErrorName(ret));
> +			goto err;
> +		}
> +	} while (ret > 0 && outbuf.pos < outbuf.size);

LLM:
"
include/linux/zstd.h:535 - "zstd_end_stream() must be called until it returns
0, meaning all the data has been flushed and the frame epilogue has been
written." The outbuf.pos < outbuf.size term lets the loop exit with ret > 0,
emitting a frame with no epilogue, and the code then prints "Algo: ZSTD" as if
it succeeded. Either loop until 0, or treat "output full before 0" as an error.
"

> +
> +	drm_printf(p, "%s.length: 0x%zx -> 0x%zx Algo: ZSTD\n",
> +		   length_label, total_size, outbuf.pos);
> +	xe_print_blob_ascii85(p, data_label, '\n', dst, 0,
> +			      DIV_ROUND_UP(outbuf.pos, sizeof(u32)) * sizeof(u32));

LLM:
"DIV_ROUND_UP(outbuf.pos, sizeof(u32)) * sizeof(u32) can read up to 3 bytes past
dst if outbuf.pos == dst_size and dst_size is not 4-aligned. In practice it is
unreachable: snapshot->size is xe_bo_size() (page-aligned) and ctb_size is a sum
of bo sizes, so compress_bound(size) = size + size/256 stays 4-aligned. But
don't rely on that - kvzalloc(round_up(dst_size, sizeof(u32))) settles it
permanently."

> +	goto out;
> +
> +err:
> +	drm_printf(p, "%s.length: 0x%zx Algo: ERROR (compression failed)\n",
> +		   length_label, total_size);

__xe_devcoredump_read() is called twice, once to get the size and the second time
to print the output in a given buffer.
If the compression fails in the first pass, the size will be different and not
enough.

And worse part it seems that in this case of failure we now lose the guc log.
So in the actual/real second pass we will not get the right log anyway.

The AI suggestion is to cache the compressed guc log. But of course, if we
cache we need to be careful to ensure it is accurate/valid.

> +out:
> +	kvfree(dst);
> +	kvfree(wksp);
> +}
> +#endif
> +
>  /**
> - * xe_guc_log_snapshot_print - dump a previously saved copy of the GuC log to some useful location
> - * @snapshot: a snapshot of the GuC log
> - * @p: the printer object to output to
> + * xe_guc_print_blob - Print binary data (compressed or uncompressed) as ascii85
> + * @gt: GT structure for error reporting
> + * @p: drm_printer for output
> + * @length_label: label prefix for the length/metadata line (e.g. "[LOG]" or "[CTB]")
> + * @data_label: label for the ascii85 blob (e.g. "[LOG].data" or "[CTB].data")
> + * @chunks: array of pointers to source data chunks
> + * @num_chunks: number of elements in @chunks
> + * @chunk_size: nominal size of each chunk; the last chunk may hold fewer valid bytes
> + * @total_size: total uncompressed byte count across all chunks
> + *
> + * If CONFIG_DRM_XE_COMPRESS_DUMP is enabled, compresses @total_size bytes using
> + * zstd level-3 streaming compression and prints the result via @p using ascii85
> + * encoding. Otherwise, directly prints the uncompressed data.
> + * On failure the function returns without printing the data blob.
>   */
> -void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p)
> +void xe_guc_print_blob(struct xe_gt *gt __maybe_unused, struct drm_printer *p,
> +		       const char *length_label, const char *data_label,
> +		       void **chunks, int num_chunks,
> +		       size_t chunk_size, size_t total_size)
>  {
> +#if IS_ENABLED(CONFIG_DRM_XE_COMPRESS_DUMP)
> +	xe_guc_print_blob_compressed(gt, p, length_label, data_label,
> +				     chunks, num_chunks, chunk_size, total_size);
> +#else
>  	size_t remain;
>  	int i;
>  
> +	drm_printf(p, "%s.length: 0x%zx Algo: RAW\n", length_label, total_size);
> +
> +	/*
> +	 * Print each chunk directly without merging into a single buffer.
> +	 * xe_print_blob_ascii85() supports multiple calls - use suffix=0
> +	 * for all but the last chunk to omit the newline between chunks.
> +	 */
> +	remain = total_size;
> +	for (i = 0; i < num_chunks && remain; i++) {
> +		size_t size = min(chunk_size, remain);
> +		const char *prefix = i ? NULL : data_label;
> +		/* Add suffix only on the last chunk */
> +		char suffix = (remain == size) ? '\n' : 0;
> +
> +		xe_print_blob_ascii85(p, prefix, suffix, chunks[i], 0,
> +				      DIV_ROUND_UP(size, sizeof(u32)) * sizeof(u32));
> +		remain -= size;
> +	}
> +#endif
> +}
> +
> +/**
> + * xe_guc_log_snapshot_print - dump a previously saved copy of the GuC log to some useful location
> + * @snapshot: a snapshot of the GuC log
> + * @p: the printer object to output to
> + * @gt: GT structure of the GuC log
> + */
> +void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p,
> +			       struct xe_gt *gt)
> +{
>  	if (!snapshot) {
>  		drm_printf(p, "GuC log snapshot not allocated!\n");
>  		return;
> @@ -248,16 +383,9 @@ void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_
>  	drm_printf(p, "GuC timestamp: 0x%08llX [%llu]\n", snapshot->stamp, snapshot->stamp);
>  	drm_printf(p, "Log level: %u\n", snapshot->level);
>  
> -	drm_printf(p, "[LOG].length: 0x%zx\n", snapshot->size);
> -	remain = snapshot->size;
> -	for (i = 0; i < snapshot->num_chunks; i++) {
> -		size_t size = min(GUC_LOG_CHUNK_SIZE, remain);
> -		const char *prefix = i ? NULL : "[LOG].data";
> -		char suffix = i == snapshot->num_chunks - 1 ? '\n' : 0;
> -
> -		xe_print_blob_ascii85(p, prefix, suffix, snapshot->copy[i], 0, size);
> -		remain -= size;
> -	}
> +	xe_guc_print_blob(gt, p, "[LOG]", "[LOG].data",
> +			  snapshot->copy, snapshot->num_chunks,
> +			  GUC_LOG_CHUNK_SIZE, snapshot->size);
>  }
>  
>  static inline void lfd_output_binary(struct drm_printer *p, char *buf, int buf_size)
> @@ -598,12 +726,13 @@ void xe_guc_log_print_dmesg(struct xe_guc_log *log)
>  void xe_guc_log_print(struct xe_guc_log *log, struct drm_printer *p)
>  {
>  	struct xe_guc_log_snapshot *snapshot;
> +	struct xe_gt *gt = log_to_gt(log);
>  
>  	drm_printf(p, "**** GuC Log ****\n");
>  
>  	snapshot = xe_guc_log_snapshot_capture(log, false);
> -	drm_printf(p, "CS reference clock: %u\n", log_to_gt(log)->info.reference_clock);
> -	xe_guc_log_snapshot_print(snapshot, p);
> +	drm_printf(p, "CS reference clock: %u\n", gt->info.reference_clock);
> +	xe_guc_log_snapshot_print(snapshot, p, gt);
>  	xe_guc_log_snapshot_free(snapshot);
>  }
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.h b/drivers/gpu/drm/xe/xe_guc_log.h
> index 4649a260755e..3e97647ee8b5 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.h
> +++ b/drivers/gpu/drm/xe/xe_guc_log.h
> @@ -11,6 +11,7 @@
>  
>  struct drm_printer;
>  struct xe_device;
> +struct xe_gt;
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DEBUG_GUC)
>  #define XE_GUC_LOG_EVENT_DATA_BUFFER_SIZE	SZ_16M
> @@ -59,7 +60,13 @@ void xe_guc_log_print(struct xe_guc_log *log, struct drm_printer *p);
>  void xe_guc_log_print_lfd(struct xe_guc_log *log, struct drm_printer *p);
>  void xe_guc_log_print_dmesg(struct xe_guc_log *log);
>  struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct xe_guc_log *log, bool atomic);
> -void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p);
> +
> +void xe_guc_print_blob(struct xe_gt *gt, struct drm_printer *p, const char *length_label,
> +		       const char *data_label, void **chunks, int num_chunks,
> +		       size_t chunk_size, size_t total_size);
> +
> +void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p,
> +			       struct xe_gt *gt);
>  void xe_guc_log_snapshot_free(struct xe_guc_log_snapshot *snapshot);
>  
>  static inline u32
> -- 
> 2.43.0
>