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

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

Thanks for review, please see my comments inline below.

Regards,
Zhanjun Dong

On 2026-07-21 12:29 p.m., Julia Filipchuk wrote:
> Patch looks generally good.
> 
> Please check deallocation in xe_guc_print_blob_compressed().
See my comments in that area.

> 
> Please remove added build files.
>>   kernel/build64-debug/Makefile             |   3 +
>>   kernel/build64-debug/scripts/basic/fixdep | Bin 0 -> 16936 bytes
> 
Oops, to be removed.>
> 
> On 7/16/2026 8:51 AM, Dong, Zhanjun wrote:
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>> index 538d4df0f7aa..d0325c4f7b86 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> @@ -225,16 +228,149 @@ 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) {
>> +			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);
>> +
>> +	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));
>> +	goto out;
>> +
>> +err:
>> +	drm_printf(p, "%s.length: 0x%zx Algo: ERROR (compression failed)\n",
>> +		   length_label, total_size);
>> +out:
>> +	kvfree(dst);> +	kvfree(wksp);
> 
> Possible free of unallocated pointer? I don't think kvfree checks for null.
> 
> Maybe add checks before free. Or implement full error unroll via gotos.
> 
> 	if (dst)
> 		kvfree(dst);
> 	if (wksp)
> 		kvfree(wksp);
> 
kvfree calls vfree and kfree, both can handel null.
Double check with Xe module, no "null check" for kvfree, so will keep as 
is.>> +}
>> +#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
>> +	/* Compression disabled, output uncompressed data directly */
> 
> Minor nit. Above comment line seems unneeded with funciton description already
> covering the topic.
Sure, to be removed.

>>   	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
>> +}
> 
> 
> 
> 
>> @@ -248,16 +384,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;
>> -	}
> 
> Variable remain may now unused with this block removed.
After patch, variable remain not exist in this function>
>> +	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)
> 
> 
> 
>