Re: [PATCH v4 1/2] drm/xe/guc: Add prefix to guc log buffer macros

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/2/2025 5:03 PM, Zhanjun Dong wrote:
> Add prefix to GuC log buffer macros to follow Xe naming styles.
> Remove helper functions, replaced with macros.
> 
> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc.c         | 22 ++++-----
>  drivers/gpu/drm/xe/xe_guc_capture.c |  6 +--
>  drivers/gpu/drm/xe/xe_guc_log.c     | 71 +++++++----------------------
>  drivers/gpu/drm/xe/xe_guc_log.h     | 14 +++---
>  4 files changed, 36 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index d5adbbb013ec..b0ea9a13847a 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -99,7 +99,7 @@ static u32 guc_ctl_log_params_flags(struct xe_guc *guc)
>  	u32 offset = guc_bo_ggtt_addr(guc, guc->log.bo) >> PAGE_SHIFT;
>  	u32 flags;
>  
> -	#if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)
> +	#if (((XE_GUC_LOG_CRASH_BUFFER_SIZE) % SZ_1M) == 0)
>  	#define LOG_UNIT SZ_1M
>  	#define LOG_FLAG GUC_LOG_LOG_ALLOC_UNITS
>  	#else
> @@ -107,7 +107,7 @@ static u32 guc_ctl_log_params_flags(struct xe_guc *guc)
>  	#define LOG_FLAG 0
>  	#endif
>  
> -	#if (((CAPTURE_BUFFER_SIZE) % SZ_1M) == 0)
> +	#if (((XE_GUC_LOG_CAPTURE_BUFFER_SIZE) % SZ_1M) == 0)
>  	#define CAPTURE_UNIT SZ_1M
>  	#define CAPTURE_FLAG GUC_LOG_CAPTURE_ALLOC_UNITS
>  	#else
> @@ -115,20 +115,20 @@ static u32 guc_ctl_log_params_flags(struct xe_guc *guc)
>  	#define CAPTURE_FLAG 0
>  	#endif
>  
> -	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
> -	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, LOG_UNIT));
> -	BUILD_BUG_ON(!DEBUG_BUFFER_SIZE);
> -	BUILD_BUG_ON(!IS_ALIGNED(DEBUG_BUFFER_SIZE, LOG_UNIT));
> -	BUILD_BUG_ON(!CAPTURE_BUFFER_SIZE);
> -	BUILD_BUG_ON(!IS_ALIGNED(CAPTURE_BUFFER_SIZE, CAPTURE_UNIT));
> +	BUILD_BUG_ON(!XE_GUC_LOG_CRASH_BUFFER_SIZE);
> +	BUILD_BUG_ON(!IS_ALIGNED(XE_GUC_LOG_CRASH_BUFFER_SIZE, LOG_UNIT));
> +	BUILD_BUG_ON(!XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE);
> +	BUILD_BUG_ON(!IS_ALIGNED(XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE, LOG_UNIT));
> +	BUILD_BUG_ON(!XE_GUC_LOG_CAPTURE_BUFFER_SIZE);
> +	BUILD_BUG_ON(!IS_ALIGNED(XE_GUC_LOG_CAPTURE_BUFFER_SIZE, CAPTURE_UNIT));
>  
>  	flags = GUC_LOG_VALID |
>  		GUC_LOG_NOTIFY_ON_HALF_FULL |
>  		CAPTURE_FLAG |
>  		LOG_FLAG |
> -		FIELD_PREP(GUC_LOG_CRASH, CRASH_BUFFER_SIZE / LOG_UNIT - 1) |
> -		FIELD_PREP(GUC_LOG_DEBUG, DEBUG_BUFFER_SIZE / LOG_UNIT - 1) |
> -		FIELD_PREP(GUC_LOG_CAPTURE, CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) |
> +		FIELD_PREP(GUC_LOG_CRASH, XE_GUC_LOG_CRASH_BUFFER_SIZE / LOG_UNIT - 1) |
> +		FIELD_PREP(GUC_LOG_DEBUG, XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE / LOG_UNIT - 1) |
> +		FIELD_PREP(GUC_LOG_CAPTURE, XE_GUC_LOG_CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) |

hmm, but here it looks the in the GUC CTL params, the naming is different

and the GuC spec is using this naming:

	GeneralLog
	StateCapture
	CrashDumpLog

so if you want to do cleanup in naming, please update both places (BUFFER_TYPE and GUC_CTL_LOG_PARAMS)

or just make them consistent

>  		FIELD_PREP(GUC_LOG_BUF_ADDR, offset);
>  
>  	#undef LOG_UNIT
> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 243dad3e2418..acb95a93b530 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -816,7 +816,7 @@ static void check_guc_capture_size(struct xe_guc *guc)
>  {
>  	int capture_size = guc_capture_output_size_est(guc);
>  	int spare_size = capture_size * GUC_CAPTURE_OVERBUFFER_MULTIPLIER;
> -	u32 buffer_size = xe_guc_log_section_size_capture(&guc->log);
> +	u32 buffer_size = XE_GUC_LOG_CAPTURE_BUFFER_SIZE;
>  
>  	/*
>  	 * NOTE: capture_size is much smaller than the capture region
> @@ -922,7 +922,7 @@ guc_capture_init_node(struct xe_guc *guc, struct __guc_capture_parsed_output *no
>   *                  ADS module also calls separately for PF vs VF.
>   *
>   *     --> alloc B: GuC output capture buf (registered via guc_init_params(log_param))
> - *                  Size = #define CAPTURE_BUFFER_SIZE (warns if on too-small)
> + *                  Size = #define XE_GUC_LOG_CAPTURE_BUFFER_SIZE (warns if on too-small)
>   *                  Note2: 'x 3' to hold multiple capture groups
>   *
>   * GUC Runtime notify capture:
> @@ -1368,7 +1368,7 @@ static void __guc_capture_process_output(struct xe_guc *guc)
>  	xe_map_memcpy_from(guc_to_xe(guc), &log_buf_state_local, &guc->log.bo->vmap,
>  			   log_buf_state_offset, sizeof(struct guc_log_buffer_state));
>  
> -	buffer_size = xe_guc_get_log_buffer_size(&guc->log, GUC_LOG_BUFFER_CAPTURE);
> +	buffer_size = XE_GUC_LOG_CAPTURE_BUFFER_SIZE;
>  	read_offset = log_buf_state_local.read_ptr;
>  	write_offset = log_buf_state_local.sampled_write_ptr;
>  	full_count = FIELD_GET(GUC_LOG_BUFFER_STATE_BUFFER_FULL_CNT, log_buf_state_local.flags);
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
> index c01ccb35dc75..fa1d490dca0d 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.c
> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
> @@ -50,16 +50,16 @@ static size_t guc_log_size(void)
>  	 *  |     Capture state header      |
>  	 *  +-------------------------------+ 96B
>  	 *  |                               |
> -	 *  +===============================+ PAGE_SIZE (4KB)
> +	 *  +===============================+ 4KB
> +	 *  |          Event logs           |
> +	 *  +===============================+ + XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE

I'm not sure it is worth to include obvious XE_GUC_LOG prefix in the doc

maybe this diagram should just look like:

	 *  +===============================+  0000h
	 *  | ... header                    |   ^
	 *  |                               |   |
	 *  |                               |  4KB
	 *  |                               |   |
	 *  |                               |   V
	 *  +===============================+   + <-- GENERAL_LOG_OFFSET
	 *  | GENERAL_LOG (events)          |   ^
	 *  |                               |   |
	 *  |                               |  GENERAL_LOG_SIZE
	 *  |                               |   |
	 *  |                               |   V
	 *  +===============================+   + <-- CRASH_DUMP_OFFSET
	 *  | CRASH_DUMP (raw data)         |   ^
	 *  |                               |   |
	 *  |                               |  CRASH_DUMP_SIZE
	 *  |                               |   |
	 *  |                               |   V
	 *  +===============================+   + <-- ERROR_CAPTURE_OFFSET
	 *  | ERROR_CAPTURE (raw data)      |   ^
	 *  |                               |   |
	 *  |                               |  ERROR_CAPTURE_SIZE
	 *  |                               |   |
	 *  |                               |   V
	 *  +===============================+  80100h (max)

and it can be even promoted to full kernel-doc

>  	 *  |        Crash Dump logs        |
> -	 *  +===============================+ + CRASH_SIZE
> -	 *  |          Debug logs           |
> -	 *  +===============================+ + DEBUG_SIZE
> +	 *  +===============================+ + XE_GUC_LOG_CRASH_BUFFER_SIZE
>  	 *  |         Capture logs          |
> -	 *  +===============================+ + CAPTURE_SIZE
> +	 *  +===============================+ + XE_GUC_LOG_CAPTURE_BUFFER_SIZE
>  	 */
> -	return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
> -		CAPTURE_BUFFER_SIZE;
> +	return SZ_4K + XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE + XE_GUC_LOG_CRASH_BUFFER_SIZE +
> +		XE_GUC_LOG_CAPTURE_BUFFER_SIZE;

those are all macros, maybe guc_log_size should be also converted to macro ?
>  }
>  
>  #define GUC_LOG_CHUNK_SIZE	SZ_2M
> @@ -274,49 +274,6 @@ int xe_guc_log_init(struct xe_guc_log *log)
>  
>  ALLOW_ERROR_INJECTION(xe_guc_log_init, ERRNO); /* See xe_pci_probe() */
>  
> -static u32 xe_guc_log_section_size_crash(struct xe_guc_log *log)
> -{
> -	return CRASH_BUFFER_SIZE;
> -}
> -
> -static u32 xe_guc_log_section_size_debug(struct xe_guc_log *log)
> -{
> -	return DEBUG_BUFFER_SIZE;
> -}
> -
> -/**
> - * xe_guc_log_section_size_capture - Get capture buffer size within log sections.
> - * @log: The log object.
> - *
> - * This function will return the capture buffer size within log sections.
> - *
> - * Return: capture buffer size.
> - */
> -u32 xe_guc_log_section_size_capture(struct xe_guc_log *log)
> -{
> -	return CAPTURE_BUFFER_SIZE;
> -}
> -
> -/**
> - * xe_guc_get_log_buffer_size - Get log buffer size for a type.
> - * @log: The log object.
> - * @type: The log buffer type
> - *
> - * Return: buffer size.
> - */
> -u32 xe_guc_get_log_buffer_size(struct xe_guc_log *log, enum guc_log_buffer_type type)
> -{
> -	switch (type) {
> -	case GUC_LOG_BUFFER_CRASH_DUMP:
> -		return xe_guc_log_section_size_crash(log);
> -	case GUC_LOG_BUFFER_DEBUG:
> -		return xe_guc_log_section_size_debug(log);
> -	case GUC_LOG_BUFFER_CAPTURE:
> -		return xe_guc_log_section_size_capture(log);
> -	}
> -	return 0;
> -}
> -
>  /**
>   * xe_guc_get_log_buffer_offset - Get offset in log buffer for a type.
>   * @log: The log object.
> @@ -327,13 +284,17 @@ u32 xe_guc_get_log_buffer_size(struct xe_guc_log *log, enum guc_log_buffer_type
>   */
>  u32 xe_guc_get_log_buffer_offset(struct xe_guc_log *log, enum guc_log_buffer_type type)
>  {
> -	enum guc_log_buffer_type i;
>  	u32 offset = PAGE_SIZE;/* for the log_buffer_states */
>  
> -	for (i = GUC_LOG_BUFFER_CRASH_DUMP; i < GUC_LOG_BUFFER_TYPE_MAX; ++i) {
> -		if (i == type)
> -			break;
> -		offset += xe_guc_get_log_buffer_size(log, i);
> +	switch (type) {
> +	case GUC_LOG_BUFFER_CAPTURE:
> +		offset += XE_GUC_LOG_CRASH_BUFFER_SIZE;
> +		fallthrough;
> +	case GUC_LOG_BUFFER_CRASH_DUMP:
> +		offset += XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE;
> +		fallthrough;
> +	case GUC_LOG_BUFFER_DEBUG:
> +		break;
>  	}
>  
>  	return offset;
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.h b/drivers/gpu/drm/xe/xe_guc_log.h
> index 98a47ac42b08..0cc059cedb9c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.h
> +++ b/drivers/gpu/drm/xe/xe_guc_log.h
> @@ -13,13 +13,13 @@ struct drm_printer;
>  struct xe_device;
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DEBUG_GUC)
> -#define CRASH_BUFFER_SIZE       SZ_1M
> -#define DEBUG_BUFFER_SIZE       SZ_8M
> -#define CAPTURE_BUFFER_SIZE     SZ_2M
> +#define XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE	SZ_8M
> +#define XE_GUC_LOG_CRASH_BUFFER_SIZE		SZ_1M
> +#define XE_GUC_LOG_CAPTURE_BUFFER_SIZE		SZ_2M
>  #else
> -#define CRASH_BUFFER_SIZE	SZ_16K
> -#define DEBUG_BUFFER_SIZE	SZ_64K
> -#define CAPTURE_BUFFER_SIZE	SZ_1M
> +#define XE_GUC_LOG_EVENT_LOG_BUFFER_SIZE	SZ_64K
> +#define XE_GUC_LOG_CRASH_BUFFER_SIZE		SZ_16K
> +#define XE_GUC_LOG_CAPTURE_BUFFER_SIZE		SZ_1M
>  #endif
>  /*
>   * While we're using plain log level in i915, GuC controls are much more...
> @@ -51,8 +51,6 @@ xe_guc_log_get_level(struct xe_guc_log *log)
>  	return log->level;
>  }
>  
> -u32 xe_guc_log_section_size_capture(struct xe_guc_log *log);
> -u32 xe_guc_get_log_buffer_size(struct xe_guc_log *log, enum guc_log_buffer_type type);
>  u32 xe_guc_get_log_buffer_offset(struct xe_guc_log *log, enum guc_log_buffer_type type);
>  bool xe_guc_check_log_buf_overflow(struct xe_guc_log *log,
>  				   enum guc_log_buffer_type type,