Re: [PATCH v6 2/9] drm/xe: Add ASCII85 dump helper function

[John Harrison <john.c.harrison@intel.com>]

On 8/30/2024 14:56, Matthew Brost wrote:
> On Thu, Aug 29, 2024 at 11:23:03PM -0700, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> There is a need to include the GuC log and other large binary objects
>> in core dumps and via dmesg. So add a helper for dumping to a printer
>> function via conversion to ASCII85 encoding.
>>
>> Another issue with dumping such a large buffer is that it can be slow,
>> especially if dumping to dmesg over a serial port. So add a yield to
>> prevent the 'task has been stuck for 120s' kernel hang check feature
>> from firing.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_devcoredump.c | 76 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_devcoredump.h |  5 ++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
>> index bdb76e834e4c..eec7b89ab48b 100644
>> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
>> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
>> @@ -6,6 +6,7 @@
>>   #include "xe_devcoredump.h"
>>   #include "xe_devcoredump_types.h"
>>   
>> +#include <linux/ascii85.h>
>>   #include <linux/devcoredump.h>
>>   #include <generated/utsrelease.h>
>>   
>> @@ -310,3 +311,78 @@ int xe_devcoredump_init(struct xe_device *xe)
>>   }
>>   
>>   #endif
>> +
>> +/**
>> + * xe_print_blob_ascii85 - print a BLOB to some useful location in ASCII85
>> + *
>> + * The output is split to multiple lines because some print targets, e.g. dmesg
>> + * cannot handle arbitrarily long lines. Note also that printing to dmesg in
>> + * piece-meal fashion is not possible, each separate call to drm_puts() has a
>> + * line-feed automatically added! Therefore, the entire output line must be
>> + * constructed in a local buffer first, then printed in one atomic output call.
>> + *
>> + * There is also a scheduler yield call to prevent the 'task has been stuck for
>> + * 120s' kernel hang check feature from firing when printing to a slow target
>> + * such as dmesg over a serial port.
>> + *
>> + * TODO: Add compression prior to the ASCII85 encoding to shrink huge buffers down.
>> + *
>> + * @p: the printer object to output to
>> + * @blob: the Binary Large OBject to dump out
>> + * @offset: offset in bytes to skip from the front of the BLOB, must be a multiple of sizeof(u32)
>> + * @size: the size in bytes of the BLOB, must be a multiple of sizeof(u32)
>> + */
>> +void xe_print_blob_ascii85(struct drm_printer *p, const void *blob, size_t offset, size_t size)
>> +{
>> +	const u32 *blob32 = (const u32 *)blob;
>> +	char buff[ASCII85_BUFSZ], *line_buff;
>> +	size_t line_pos = 0;
>> +
>> +#define DMESG_MAX_LINE_LEN	800
>> +#define MIN_SPACE		(ASCII85_BUFSZ + 2)		/* 85 + "\n\0" */
>> +
>> +	if (size & 3)
>> +		drm_printf(p, "Size not word aligned: %zu", size);
>> +	if (offset & 3)
>> +		drm_printf(p, "Offset not word aligned: %zu", size);
>> +
>> +	line_buff = kzalloc(sizeof(DMESG_MAX_LINE_LEN), GFP_KERNEL);
>> +	if (IS_ERR(line_buff)) {
>> +		drm_printf(p, "Failed to allocate line buffer: %pe", line_buff);
>> +		return;
>> +	}
>> +
>> +	blob32 += offset / sizeof(*blob32);
>> +	size /= sizeof(*blob32);
>> +
>> +	while (size--) {
>> +		u32 val = *(blob32++);
>> +
>> +		strscpy(line_buff + line_pos, ascii85_encode(val, buff),
>> +			DMESG_MAX_LINE_LEN - line_pos);
>> +		line_pos += strlen(line_buff + line_pos);
>> +
>> +		if ((line_pos + MIN_SPACE) >= DMESG_MAX_LINE_LEN) {
>> +			line_buff[line_pos++] = '\n';
>> +			line_buff[line_pos++] = 0;
>> +
>> +			drm_puts(p, line_buff);
>> +
>> +			line_pos = 0;
>> +
>> +			/* Prevent 'stuck thread' time out errors */
>> +			cond_resched();
>> +		}
>> +	}
>> +
>> +	if (line_pos) {
>> +		line_buff[line_pos++] = '\n';
>> +		line_buff[line_pos++] = 0;
>> +
>> +		drm_puts(p, line_buff);
>> +	}
>> +
>> +	kfree(line_buff);
> Dive by comment kvfree per CI. Will try to plan sometime soon to give
Not for a kzalloc. That's just a kmalloc(GFP_ZERO) which means kfree. 
Although I think the IS_ERR test is not actually correct. It should just 
be testing for null. Not sure how that would result in the CI failure 
though.

John.