[PATCH v4 5/6] drm/xe/guc: Add GuC log event buffer output in LFD format
Dong, Zhanjun
zhanjun.dong at intel.com
Mon Jul 14 23:32:04 UTC 2025
Please see my comments inline below.
Regards,
Zhanjun Dong
On 2025-06-28 10:46 a.m., Michal Wajdeczko wrote:
>
>
> On 23.04.2025 23:58, Zhanjun Dong wrote:
>> Add GuC log event buffer output in LFD format.
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc_log.c | 87 +++++++++++++++++++++++++++++++--
>> 1 file changed, 84 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>> index 3fdf3823e2d0..5aa9b9dbe3f3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> @@ -33,6 +33,7 @@ struct guc_log_buffer_entry_list {
>> };
>>
>> struct guc_log_init_config_save {
>> + u32 version;
>> /*
>> * Array of init config KLVs.
>> * Range from GUC_LOG_LIC_TYPE_FIRST to GUC_LOG_LIC_TYPE_LAST
>> @@ -336,7 +337,6 @@ static int xe_guc_log_add_os_id(void *buf, int buf_size, u32 id)
>> return -ENOMEM;
>>
>> strscpy(os_id->build_version, version, info_len);
>> -
>
> unrelated change
To be removed>
>> return xe_guc_log_add_typed_payload(buf, buf_size, GUC_LFD_TYPE_OS_ID,
>> info_len + sizeof(*os_id), os_id);
>> }
>> @@ -366,6 +366,7 @@ static void xe_guc_log_load_lic(void *guc_log, struct guc_log_init_config_save *
>> int i = 0;
>>
>> memset(config, 0, sizeof(struct guc_log_init_config_save));
>> + config->version = p->version;
>> while (p) {
>> for (i = 0; i < GUC_LOG_BUFFER_STATE_HEADER_ENTRY_COUNT; i++) {
>> if (p->marker[0] == entry_markers[i].key[0] &&
>> @@ -380,8 +381,7 @@ static void xe_guc_log_load_lic(void *guc_log, struct guc_log_init_config_save *
>> p++;
>> } else {
>> /* Load log init config */
>> - xe_guc_log_loop_log_init((void *)p,
>> - config);
>> + xe_guc_log_loop_log_init((void *)p, config);
>
> unrelated change
To be removed>
>>
>> /* Init config is the last */
>> return;
>> @@ -417,6 +417,86 @@ xe_guc_log_output_lfd_init(char *buf, struct xe_guc_log_snapshot *snapshot,
>> return index;
>> }
>>
>> +static void
>> +xe_guc_log_print_over_chunks(struct drm_printer *p, struct xe_guc_log_snapshot *snapshot,
>> + u32 from, u32 to)
>
> guc_log_snapshot_print_chunks(
> struct xe_guc_log_snapshot *snapshot,
> u32 from, u32 to,
> struct drm_printer *p)
>
>> +{
>> + int chunk_from = from % GUC_LOG_CHUNK_SIZE;
>> + int chunk_id = from / GUC_LOG_CHUNK_SIZE;
>> + int to_chunk_id = to / GUC_LOG_CHUNK_SIZE;
>> + int chunk_to = to % GUC_LOG_CHUNK_SIZE;
>> + int pos = from;
>> +
>> + do {
>> + size_t size = (to_chunk_id > chunk_id ? GUC_LOG_CHUNK_SIZE : chunk_to) - chunk_from;
>> +
>> + xe_print_blob_ascii85(p, NULL, 0, snapshot->copy[chunk_id], chunk_from, size);
>> + pos += size;
>> + chunk_id++;
>> + chunk_from = 0;
>> + } while (pos < to);
>> +}
>> +
>> +static void
>> +xe_guc_log_lfd_print_from_chunks(struct drm_printer *p, struct xe_guc_log_snapshot *snapshot,
>> + struct guc_log_buffer_entry_list *entry)
>
> I'm not sure if having this function helps,
> IMO better to keep this code at the caller, where you are already
> accessing the data from the 'entry' in "Calculate data length"
will do>
>> +{
>> + /* Output data from guc log chunks directly */
>> + if (entry->rd_ptr < entry->wr_ptr) {
>> + xe_guc_log_print_over_chunks(p, snapshot,
>> + entry->offset + entry->rd_ptr,
>> + entry->offset + entry->wr_ptr);
>> + } else {
>> + /* print from rd to buf end 1st */
>> + xe_guc_log_print_over_chunks(p, snapshot,
>> + entry->offset + entry->rd_ptr,
>> + entry->offset + entry->buf_size);
>> +
>> + /* print from buf start to wr */
>> + xe_guc_log_print_over_chunks(p, snapshot, entry->offset,
>> + entry->offset + entry->wr_ptr);
>> + }
>> +}
>> +
>> +static inline int
>> +xe_guc_log_add_log_event(struct drm_printer *p, char *buf, struct xe_guc_log_snapshot *snapshot,
>> + struct guc_log_init_config_save *config)
>
> guc_log_snapshot_print_events(
> struct xe_guc_log_snapshot *snapshot,
> const struct guc_log_init_config_save *config,
> char *buf,
> struct drm_printer *p)
will do>
> but do we really need this 'buf' ?
> those lfd headers are quite small and could be defined on stack
Let me check and might use stack on next rev.
>
>> +{
>> + struct guc_logfile_lfd_data *lfd = (void *)buf;
>> + struct guc_lfd_data_log_events_buf *events_buf;
>> + struct guc_log_buffer_entry_list *entry;
>> + u32 data_len;
>> + int index;
>> +
>> + memset(buf, 0, GUC_LOG_CHUNK_SIZE);
>> + entry = &config->entry[GUC_LOG_BUFFER_STATE_HEADER_ENTRY_LOG];
>> +
>> + /* Skip empty log */
>> + if (entry->rd_ptr == entry->wr_ptr)
>> + return 0;
>> +
>> + index = xe_guc_log_add_lfd_header(buf, GUC_LOG_CHUNK_SIZE);
>> + lfd->dw0 |= FIELD_PREP(GUC_LOGFILE_LFD_DESC_TYPE, GUC_LFD_TYPE_LOG_EVENTS_BUFFER);
>> + events_buf = (void *)&lfd->data;
>> + events_buf->log_events_format_version = config->version;
>> +
>> + /* Adjust to log_format_buf */
>> + data_len = offsetof(struct guc_lfd_data_log_events_buf, log_format_buf);
>> + index += data_len;
>> +
>> + /* Calculate data length */
>> + data_len += (entry->wr_ptr + entry->buf_size - entry->rd_ptr) % entry->buf_size;
>> + /* make length u32 aligned */
>> + lfd->desc_dw_size = DIV_ROUND_UP(data_len, sizeof(u32));
>> +
>> + /* Output GUC_LFD_TYPE_LOG_EVENTS_BUFFER header */
>> + xe_print_blob_ascii85(p, NULL, 0, buf, 0, (size_t)index);
>> +
>> + xe_guc_log_lfd_print_from_chunks(p, snapshot, entry);
>> + /* log event buffer content did not use buf, exclude from size count */
>> + return index;
>> +}
>> +
>> static void
>> xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot, struct drm_printer *p,
>> struct xe_guc_log *log)
>> @@ -441,6 +521,7 @@ xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot, struct drm_p
>> goto lfd_out;
>>
>> xe_print_blob_ascii85(p, "[LOG].data", 0, lfd_buf, 0, (size_t)index);
>> + index = xe_guc_log_add_log_event(p, lfd_buf, snapshot, &config);
>>
>> lfd_out:
>> drm_printf(p, "\n");
>
More information about the Intel-xe
mailing list