[PATCH v6 5/6] drm/xe/guc: Add GuC log event buffer output in LFD format
Dong, Zhanjun
zhanjun.dong at intel.com
Fri Aug 22 00:19:10 UTC 2025
See my comments inline below.
Regards,
Zhanjun Dong
On 2025-07-30 1:40 p.m., John Harrison wrote:
> On 7/21/2025 6:35 PM, 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 | 88 ++++++++++++++++++++++++++++++++-
>> 1 file changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/
>> xe_guc_log.c
>> index 609a9fd5ba0c..a8910211c579 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> @@ -421,10 +421,92 @@ xe_guc_log_output_lfd_init(struct drm_printer
>> *p, struct xe_guc_log_snapshot *sn
>> return size;
>> }
>> +static void
>> +xe_guc_log_print_over_chunks(struct drm_printer *p, struct
>> xe_guc_log_snapshot *snapshot,
>> + u32 from, u32 to)
> _over_ ?
or across?>
>> +{
>> + 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)
>> +{
>> + /* 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);
> The log does not necessarily reach the end. Indeed, the v2 format is 5
> dwords which does not fit neatly into a page. So you need to stop at
> buff_wrap_offset not buf_size.
Thanks, will stop at buff_wrap_offset >
> Also, remember that the primary aim here is for streaming output -
> continuously dumping data, not just a single snapshot. And in that case,
> we should be sending a FLUSH_LOG H2G command and then dumping up to
> sampled_log_buf_wr_ptr rather than wr_ptr. And in the case of streaming
> output, taking a snapshot copy of the log is probably not worth doing.
> We can just dump directly from the live buffer.
>
> I'm not seeing anything that writes to rd_ptr. The KMD is responsible
> for updating the read pointer after reading the data. The GuC does not
> know what we have or have not read.
Will do flush and write to rd_ptr later, for now, I can use guc_log to
verify lfd output contents.
>
>> +
>> + /* 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, struct
>> xe_guc_log_snapshot *snapshot,
>> + struct guc_lic_save *config)
>> +{
>> + size_t size;
>> + u32 data_len;
>> + struct guc_lfd_data *lfd;
>> + struct guc_log_buffer_entry_list *entry;
>> + struct guc_lfd_data_log_events_buf *events_buf;
>> + char buf[GUC_LFD_BUFFER_SIZE] = {0};
> As per other patch, this is a very bad idea for multiple reasons.
>
>> +
>> + lfd = (void *)buf;
>> + entry = &config->entry[GUC_LOG_BUFFER_STATE_HEADER_ENTRY_LOG];
>> +
>> + /* Skip empty log */
>> + if (entry->rd_ptr == entry->wr_ptr)
>> + return 0;
>> +
>> + size = xe_guc_log_add_lfd_header(buf, GUC_LFD_BUFFER_SIZE);
>> + lfd->dw0 |= FIELD_PREP(GUC_LFD_DATA_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);
>> + size += data_len;
> This all seems a very complicated way of doing things. Why not just have
> a structure which is the event buffer LFD block - header, magic,
> version, etc. Create one of those on the stack as a local variable, fill
> it in and write it out. No need for complicated type casting,
> offsetting, pointer arithmetic, etc.
Will optimize
>
>> +
>> + /* Calculate data length */
>> + data_len += (entry->wr_ptr + entry->buf_size - entry->rd_ptr) %
>> entry->buf_size;
> As above, this is not correct as the end of the log is the wrap_offset
> not the size. And in the case where the log is not wrapped, the size is
> just wr - rd and size is not relevant.
Yes, to be adjust to use wrap_offset>
>> + /* make length u32 aligned */
>> + lfd->dw_size = DIV_ROUND_UP(data_len, sizeof(u32));
> Is this just a belt and braces check? The log entries are multiples of
> either 4 or 5 words so data_len must be 32-bit aligned unless something
> is badly broken somewhere.
kind of, just make sure u32 aligned.>
> John.
>
>> +
>> + /* Output GUC_LFD_TYPE_LOG_EVENTS_BUFFER header */
>> + xe_print_blob_ascii85(p, NULL, 0, buf, 0, size);
>> + xe_guc_log_lfd_print_from_chunks(p, snapshot, entry);
>> +
>> + /* log event buffer content did not use buf, exclude from size
>> count */
>> + return size;
>> +}
>> +
>> static void
>> xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot,
>> struct drm_printer *p)
>> {
>> struct guc_lic_save config;
>> + size_t size;
>> if (!snapshot || !snapshot->size)
>> return;
>> @@ -435,7 +517,11 @@ xe_guc_log_snapshot_print_lfd(struct
>> xe_guc_log_snapshot *snapshot, struct drm_p
>> /* Output LFD stream */
>> xe_guc_log_load_lic(snapshot->copy[0], &config);
>> - xe_guc_log_output_lfd_init(p, snapshot, &config);
>> + size = xe_guc_log_output_lfd_init(p, snapshot, &config);
>> + if (size < 0)
>> + return;
>> +
>> + xe_guc_log_add_log_event(p, snapshot, &config);
>> }
>> /**
>
More information about the Intel-xe
mailing list