[PATCH v6 6/6] drm/xe/guc: Only add GuC crash dump if available

[Dong, Zhanjun]

On 2025-08-04 5:21 p.m., John Harrison wrote:
> On 8/4/2025 1:14 PM, John Harrison wrote:
>> On 7/21/2025 6:35 PM, Zhanjun Dong wrote:
>>> Add GuC crash dump data empty check. LFD will only include crash dump
>>> section when data is not empty.
>>>
>>> Signed-off-by: Zhanjun Dong<zhanjun.dong at intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_guc_log.c | 60 +++++++++++++++++++++++++++++++++
>>>   1 file changed, 60 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/ 
>>> xe_guc_log.c
>>> index a8910211c579..f5f31f15e7e3 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>>> @@ -502,6 +502,65 @@ xe_guc_log_add_log_event(struct drm_printer *p, 
>>> struct xe_guc_log_snapshot *snap
>>>       return size;
>>>   }
>>>   +static int
>>> +xe_guc_log_add_crash_dump(struct drm_printer *p, struct 
>>> xe_guc_log_snapshot *snapshot,
>>> +              struct guc_lic_save *config)
>>> +{
>>> +    struct guc_log_buffer_entry_list *entry;
>>> +    int chunk_from, chunk_id, to_chunk_id;
>>> +    int pos, from, to;
>>> +    size_t size = 0;
>>> +    char buf[GUC_LFD_BUFFER_SIZE] = {0};
>>> +
>>> +    entry = &config->entry[GUC_LOG_BUFFER_STATE_HEADER_ENTRY_CRASH];
>>> +
>>> +    /* Skip zero sized crash dump */
>>> +    if (!entry->buf_size)
>>> +        return 0;
>>> +
>>> +    /* Check if crash dump section are all zero */
>>> +    from = entry->offset;
>>> +    to = entry->offset + entry->buf_size;
>>> +    chunk_from = from % GUC_LOG_CHUNK_SIZE;
>>> +    chunk_id = from / GUC_LOG_CHUNK_SIZE;
>>> +    to_chunk_id = to / GUC_LOG_CHUNK_SIZE;
>>> +    pos = from;
>>> +
>>> +    do {
>>> +        size_t size = (to_chunk_id > chunk_id ? GUC_LOG_CHUNK_SIZE : 
>>> to) - chunk_from;
>>> +        u32 *buf32 = snapshot->copy[chunk_id] + chunk_from;
>>> +        int i;
>>> +
>>> +        for (i = 0; i < size / sizeof(u32); i++)
>>> +            if (buf32[i])
>>> +                break;
>>> +        if (i < size / sizeof(u32)) {
>>> +            pos += i * sizeof(u32) - chunk_from;
>>> +            break;
>>> +        }
>>> +        pos += size;
>>> +        chunk_id++;
>>> +        chunk_from = 0;
>>> +    } while (pos < to);
>>> +
>>> +    /* Buffer has non-zero data? */
>>> +    if (pos < to) {
>>> +        struct guc_lfd_data *lfd = (void *)buf;
>>> +
>>> +        size = xe_guc_log_add_lfd_header(buf, GUC_LFD_BUFFER_SIZE);
>>> +        lfd->dw0 |= FIELD_PREP(GUC_LFD_DATA_DESC_TYPE, 
>>> GUC_LFD_TYPE_FW_CRASH_DUMP);
>>> +        /* Calculate data length */
>>> +        lfd->dw_size = DIV_ROUND_UP(entry->buf_size, sizeof(u32));
>>> +        /* Output GUC_LFD_TYPE_FW_CRASH_DUMP header */
>>> +        xe_print_blob_ascii85(p, NULL, 0, buf, 0, size);
>>> +
>>> +        /* rd/wr ptr is not used for crash dump */
>>> +        xe_guc_log_print_over_chunks(p, snapshot, entry->offset,
>>> +                         entry->offset + entry->buf_size);
>>> +    }
>>> +    return size;
>> Can't help but think that this is all way too complicated.
>>
>> The crash dump buffer is never going to be more than 1MB. And that 
>> size is the smallest we can allocate when using large units in order 
>> to get a larger log buffer (because the units are shared). My 
>> understanding is that the actual size used is only a few pages 
>> (currently working on getting a definitive answer from someone who 
>> knows). So it would be far simpler to just save the crash buffer to 
>> its own single, contiguous allocation in the snapshot with no chunking 
>> required.
> Just got an answer back. The size is needed is 12KB currently but will 
> shortly rise to 16KB. So a private allocation in the snapshot will be 
> sufficient and is definitely going to be simpler than going through the 
> chunking stuff.
Nice to hear no need to handle across chunks.
It could be a single buffer print.

> 
> Unfortunately, that also means that our current default of 8KB in non- 
> debug builds is too small! Can you post a patch to bump that up to 16KB?
Sure, I will. As it will be out of LFD scope, that will be a separate patch.

Regard,
Zhanjun Dong>
> Thanks,
> John.
> 
>>
>> Then all the above complication disappears. You get a simple "for(i = 
>> 0; i < size; i++) if(i) break;" to check for empty and a single print 
>> buffer call.
>>
>> John.
>>
>>> +}
>>> +
>>>   static void
>>>   xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot, 
>>> struct drm_printer *p)
>>>   {
>>> @@ -522,6 +581,7 @@ xe_guc_log_snapshot_print_lfd(struct 
>>> xe_guc_log_snapshot *snapshot, struct drm_p
>>>           return;
>>>         xe_guc_log_add_log_event(p, snapshot, &config);
>>> +    xe_guc_log_add_crash_dump(p, snapshot, &config);
>>>   }
>>>     /**
>>
>