Re: [PATCH v7 3/6] drm/xe/guc: Add GuC log init config in LFD format

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

Thanks for review, please see my inline comments below.

Regards,
Zhanjun Dong

On 2025-10-20 7:44 p.m., Julia Filipchuk wrote:
>> +/* Magic keys define */
>> +#define GUC_LFD_DRIVER_KEY_STREAMING		0x8086AAAA474C5346
>> +#define GUC_LFD_LOG_BUFFER_MARKER_2		0xDEADFEED
>> +#define GUC_LFD_CRASH_DUMP_BUFFER_MARKER_2	0x8086DEAD
>> +#define GUC_LFD_STATE_CAPTURE_BUFFER_MARKER_2	0xBEEFFEED
>> +#define GUC_LFD_LOG_BUFFER_MARKER_1V2		0xCABBA9E6
>> +#define GUC_LFD_STATE_CAPTURE_BUFFER_MARKER_1V2	0xCABBA9F7
>> +#define GUC_LFD_DATA_HEADER_MAGIC		0x8086
> Possibly fix alignment
Confirmed the alignment is correct in source code, misalignment here 
shall be caused by '+'
> 
>> +/* LFD supported LIC type range */
>> +#define GUC_LIC_TYPE_FIRST			GUC_LIC_TYPE_GUC_SW_VERSION
>> +#define GUC_LIC_TYPE_LAST			GUC_LIC_TYPE_BUILD_PLATFORM_ID
>> +#define GUC_LFD_TYPE_FW_RANGE_FIRST		GUC_LFD_TYPE_FW_VERSION
>> +#define GUC_LFD_TYPE_FW_RANGE_LAST		GUC_LFD_TYPE_BUILD_PLATFORM_ID
> Does it make sense to define these near the relevant enums?
Those enums referenced is defined in xxx_abi.h, where defines are about 
GuC interface layer; here guc log.c, implementation layer, is what we 
supported. It is possible that some features are defined in interface 
layer, but implementation layer(xe_guc_log.c) skip/don't support it.
Keep XXX_FIRST and XXX_LAST these implementation related macros within 
the implementation layer is the way we follow this layer concept.
> 
>> +
>> +void
>> +xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p);
>> +void
>> +xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p)
> Should this just be declared static?
> 
This is the way we add code without trigger the
"<function xxx> not referenced"
build error.
In the last patch in this series, it will be updated as static.