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

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

On 2025-10-02 12:00 p.m., Lucas De Marchi wrote:
> On Thu, Oct 02, 2025 at 11:03:01AM -0400, 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)
> 
> can you also add a patch to remove the excess use of parenthesis and
> indentation? Should ahave been:
> 
> #if XE_GUC_LOG_CRASH_BUFFER_SIZE % SZ_1M == 0
> 

In case of CRASH_BUFFER_SIZE defined with expression like:

SZ_2M - SZ_1M

expanded macro would be
   #if  SZ_2M - SZ_1M % SZ_1M == 0

due to '%' op is higher than '-'
SZ_1M % SZ_1M is 0
SZ_2M - 0 is SZ_2M
SZ_2M != 0
Result: false

That gives us unwantted result, expected should be:
(SZ_2M - SZ_1M)  % SZ_1M == 0
Result: true

We might still need parenthesis to prevent operator precedence issues

> 
> 
>>     #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)
> 
> ditto
> 
>>     #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) |
>>         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
>>      *  |        Crash Dump logs        |
>> -     *  +===============================+ + CRASH_SIZE
>> -     *  |          Debug logs           |
>> -     *  +===============================+ + DEBUG_SIZE
> 
> what happened with DEBUG_SIZE? in drivers/gpu/drm/xe/xe_guc.c it's
> referred as DEBUG_BUFFER_SIZE
> 
> Lucas De Marchi
> 
>> +     *  +===============================+ + 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;

The comment in layout is DEBUG_SIZE, while code is DEBUG_BUFFER_SIZE,
will cleanup in next rev

Regards,
Zhanjun Dong

>> }
>>
>> #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,
>> -- 
>> 2.34.1
>>