Re: [PATCH v2] drm/xe/guc: Update GuC log buffer type value

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

On 9/25/2025 5:20 PM, Dong, Zhanjun wrote:
> 
> 
> On 2025-09-23 6:20 p.m., Michal Wajdeczko wrote:
>>
>>
>> On 9/4/2025 5:14 PM, Zhanjun Dong wrote:
>>> Update GuC log buffer type value, to align with the GuC specification.
>>>
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>>> ---
>>> Change list:
>>> v2: Use SZ_4K, instead of PAGE_SIZE
>>>      Expand for loop with switch and fallthrough
>>> ---
>>>   drivers/gpu/drm/xe/abi/guc_log_abi.h |  2 +-
>>>   drivers/gpu/drm/xe/xe_guc_log.c      | 20 ++++++++++++--------
>>>   2 files changed, 13 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/abi/guc_log_abi.h b/drivers/gpu/drm/xe/abi/guc_log_abi.h
>>> index 554630b7ccd9..b1819679fa35 100644
>>> --- a/drivers/gpu/drm/xe/abi/guc_log_abi.h
>>> +++ b/drivers/gpu/drm/xe/abi/guc_log_abi.h
>>> @@ -10,8 +10,8 @@
>>>     /* GuC logging buffer types */
>>>   enum guc_log_buffer_type {
>>> -    GUC_LOG_BUFFER_CRASH_DUMP,
>>>       GUC_LOG_BUFFER_DEBUG,
>>> +    GUC_LOG_BUFFER_CRASH_DUMP,
>>>       GUC_LOG_BUFFER_CAPTURE,
>>>   };
>>>   diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>>> index c01ccb35dc75..6cbb0378d2ba 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>>> @@ -50,7 +50,7 @@ static size_t guc_log_size(void)
>>>        *  |     Capture state header      |
>>>        *  +-------------------------------+ 96B
>>>        *  |                               |
>>> -     *  +===============================+ PAGE_SIZE (4KB)
>>> +     *  +===============================+ 4KB
>>>        *  |        Crash Dump logs        |
>>>        *  +===============================+ + CRASH_SIZE
>>>        *  |          Debug logs           |
>>> @@ -58,7 +58,7 @@ static size_t guc_log_size(void)
>>>        *  |         Capture logs          |
>>>        *  +===============================+ + CAPTURE_SIZE
>>>        */
>>> -    return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
>>> +    return SZ_4K + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
>>>           CAPTURE_BUFFER_SIZE;
>>
>> nit: can you do the math in the same order as on diagram?
>>
>>     return 4K + DEBUG + CRASH + CAPTURE
> Actually the order in diagram is reversed, I will update it in next rev.>
>> and btw, since in this function you are using hardcoded values
>>
>>>   }
>>>   @@ -327,13 +327,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)

btw, this should be named as
	xe_guc_log_get_buffer_offset()
or just
	xe_guc_log_buffer_offset()

since it takes xe_guc_log, not xe_guc

>>>   {
>>> -    enum guc_log_buffer_type i;
>>> -    u32 offset = PAGE_SIZE;/* for the log_buffer_states */
>>> +    u32 offset = SZ_4K; /* 1st section size */
>>>   -    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_get_log_buffer_size(log, GUC_LOG_BUFFER_CRASH_DUMP);
>>
>> do we need to use fancy helpers to finally get the same hardcoded value?
>>
>> here it is just CRASH_BUFFER_SIZE
>>
>>> +        fallthrough;
>>> +    case GUC_LOG_BUFFER_CRASH_DUMP:
>>> +        offset += xe_guc_get_log_buffer_size(log, GUC_LOG_BUFFER_DEBUG);
>>
>> and here DEBUG_BUFFER_SIZE
>>
>>> +        fallthrough;
>>> +    case GUC_LOG_BUFFER_DEBUG:
>>> +        break;
>>>       }
>>>         return offset;
>>
>> then since XXX_BUFFER_SIZE macros are already in .h maybe we get kill those helpers?
> The value is fixed for now, with the helpers, logic inside could be easy to change for future, currently, caller of xe_guc_get_log_buffer_size for crash and debug log are inside of this file only, but user of capture has external reference on xe_guc_capture.c.

if those buffer sizes are about to change into some dynamic/runtime sizes
then those macros shall be removed or at least made private to the .c file

but in the meantime, since those are public *and* your guc_log_size()
function is also using them as-is and bypassing helpers get_size(type)
then I see no reason why macros can't be used in buffer_offset()

either make buffer_offset() to use macros, or
convert guc_log_size() to use helpers

mixing different approaches is very error prone

> 
> Consider the helper won't cause code hard to read and only return fixed value, make it easy to be optimized, I would keep it as is. >
>> and btw, those BUFFER_SIZE macros in .h should have likely have XE_GUC_LOG prefix
>> (since those are Xe driver specific) - something for next (or prerequisite patch)
> Yes, need to add prefix. I will make another patch to change it.

maybe all those small fixes could be done in one series?

1 add prefix to BUFFER_SIZE macros
2 make them private to log.c *or* drop xe_guc_log_get_buffer_size() helpers
3 fix get_log_buffer_offset to use macros *or* fix guc_log_size() to use helpers
4 rename
	xe_guc_get_log_buffer_size
	xe_guc_get_log_buffer_offset
	xe_guc_check_log_buf_overflow
 to
	xe_guc_log_buffer_size
	xe_guc_log_buffer_offset
	xe_guc_log_check_overflow


> 
> Regards,
> Zhanjun Dong>
>>
>