Re: [PATCH v7 5/6] drm/xe/guc: Only add GuC crash dump if available

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

On 2025-10-20 7:11 p.m., Julia Filipchuk wrote:
> Some questions on this patch.
> 
> 
>> +static int
>> +xe_guc_log_add_crash_dump(struct drm_printer *p, struct xe_guc_log_snapshot *snapshot,
>> +			  struct guc_lic_save *config)
> 
> Is this return value just here because of convention?
>> +	/* Check if crash dump section are all zero */
>> +	from = entry->offset;
>> +	to = entry->offset + entry->buf_size;
>> +	chunk_from = from % GUC_LOG_CHUNK_SIZE;
>> +	chunk_id = from / GUC_LOG_CHUNK_SIZE;
>> +	buf32 = snapshot->copy[chunk_id] + chunk_from;
>> +	for (i = 0; i < entry->buf_size / sizeof(u32); i++)
>> +		if (buf32[i])
>> +			break;
> 
> This check looks incorrect.
> 
> buf32 is looking at a max GUC_LOG_CHUNK_SIZE chunk but here is checked against
> full `entry->buf_size`.
> 
> Is it a simplification to check if only the first chunk is zero?
There is a story behind: In v6 review, John raised the concern of the 
crash dump is very small, only in 8K, 12K or 16K. That's why we use this 
simplified code.
> 
> 
> 
>> +
>> +	/* Buffer has non-zero data? */
>> +	if (i < entry->buf_size / sizeof(u32)) {
>> +		struct guc_lfd_data lfd;
>> +
>> +		size = xe_guc_log_add_lfd_header(&lfd);
>> +		lfd.header |= FIELD_PREP(GUC_LFD_DATA_HEADER_MASK_TYPE, GUC_LFD_TYPE_FW_CRASH_DUMP);
>> +		/* Calculate data length */
>> +		lfd.data_count = DIV_ROUND_UP(entry->buf_size, sizeof(u32));
>> +		/* Output GUC_LFD_TYPE_FW_CRASH_DUMP header */
>> +		lfd_output_binary(p, (char *)&lfd, size);
>> +
>> +		/* rd/wr ptr is not used for crash dump */
>> +		xe_guc_log_print_chunks(p, snapshot, from, to);
>> +	}
>> +	return size;
> Should this size be additionally incremented by length of printed data?
The size is the actual data size used in buffer, data output by 
lfd_output_binary do not use buffer, that's why not add up.

Regards,
Zhanjun Dong
> 
>