[PATCH v6 4/6] drm/xe/guc: Add GuC log init config in LFD format

[Dong, Zhanjun]

There are multiple comments, this reply is focus on 1 topic, see my 
comments below.

Regards,
Zhanjun

On 2025-07-29 8:27 p.m., John Harrison wrote:
> On 7/21/2025 6:35 PM, Zhanjun Dong wrote:
>> Add support to output GuC log init config (LIC) in LFD format.
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_log.c | 204 ++++++++++++++++++++++++++++++++
>>   1 file changed, 204 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/ 
>> xe_guc_log.c
>> index d2256773aaf3..609a9fd5ba0c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
...
>> +
>> +static int
>> +xe_guc_log_output_lfd_init(struct drm_printer *p, struct 
>> xe_guc_log_snapshot *snapshot,
>> +               struct guc_lic_save *config)
>> +{
>> +    int type, len;
>> +    size_t size = 0;
>> +    char buf[GUC_LFD_BUFFER_SIZE] = {0};
> This will add 1KB of zeros as a global static object. You should never 
> give large arrays an empty initialisation like that. I recall someone 
> once doing that with a 16MB array and it took us ages to work out why 
> the driver was so huge in debug builds!  Also, this is putting a 1KB 
> array on the stack. Kernel stack space is limited and you should never 
> put large objects on the stack unless absolutely necessary. Just use 
> kzalloc() for this.
> 

I agree with you for static or global arrays cases, which will cause 
.bss, .rodata or other(depends on platform) read-only segment increase, 
that's because the static/global data is initialized by loader, loader 
copy data from .bss/.rodata segment to memory, that's how it works.
While for this case, which is clear local stack memory to zero, that's 
not load's task, that is compiler's job. Compiler will generate either 
memset call or machine code to clear it to zero, for example on X86, 
disassembly code is:

xe_guc_log_output_lfd_init(struct drm_printer *p, struct 
xe_guc_log_snapshot *snapshot,
  501:	65 48 8b 04 25 28 00 	mov    %gs:0x28,%rax
  508:	00 00
// I set GUC_LFD_BUFFER_SIZE set to 0x258(decimal 600), compiler has 
some calculation and make it 0x268
  50a:	48 89 84 24 68 02 00 	mov    %rax,0x268(%rsp)
  511:	00
  512:	31 c0                	xor    %eax,%eax	
	char buf[GUC_LFD_BUFFER_SIZE] = {0};
  514:	48 c7 44 24 10 00 00 	movq   $0x0,0x10(%rsp)
  51b:	00 00
  51d:	f3 48 ab             	rep stos %rax,%es:(%rdi)
		size += xe_guc_log_add_klv(&buf[size], GUC_LFD_BUFFER_SIZE - size, 
type, config);
  520:	49 8d 47 10          	lea    0x10(%r15),%rax
  524:	4c 8d 34 04          	lea    (%rsp,%rax,1),%r14
	index = xe_guc_log_add_lfd_header(buf, buf_size);

Data cleared by instructions, there is no access to RO segment.

Local buf={0} should be similar to a memset call. If you have concerns 
about support of compiler/compiler options/processor combination, let me 
know, we can change it to memset call.

About the size, I see warning of stack usage is more than 1K, will 
reduce it to 600 or 800 to avoid the warning.
LFD header is small, 600/800 looks good.