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

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

On 2025-10-07 1:08 p.m., Lucas De Marchi wrote:
> On Tue, Oct 07, 2025 at 12:44:59PM -0400, Dong, Zhanjun wrote:
>>
>>
>> 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
> 
> but it's not defined like that. If you need to define it like that then
> the right define would be:
> 
> #define XE_GUC_LOG_CRASH_BUFFER_SIZE (SZ_2M - SZ_1M)
> 
> so the callers may use normal precedence rules without worrying how it's
> implemented.

Yes, it's not defined like that.

My concern is who response for ensure the operator precedence, I think 
it should be the macro, who contains the operator, response to ensure that,
so that would be:

#define plus(a, b)   (a) + (b)

The plus response for that, not a and b. So if a or b changed at later 
time, the plus still works.

Regards,
Zhanjun Dong

> 
> Lucas De Marchi
> 
>>
>> 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
>>>>
>>