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

[Julia Filipchuk <julia.filipchuk@intel.com>]

Some questions on this patch.


> +static int
> +xe_guc_log_add_crash_dump(struct drm_printer *p, struct xe_guc_log_snapshot *snapshot,
> +			  struct guc_lic_save *config)

Is this return value just here because of convention?
> +	/* 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;
> +	buf32 = snapshot->copy[chunk_id] + chunk_from;
> +	for (i = 0; i < entry->buf_size / sizeof(u32); i++)
> +		if (buf32[i])
> +			break;

This check looks incorrect.

buf32 is looking at a max GUC_LOG_CHUNK_SIZE chunk but here is checked against
full `entry->buf_size`.

Is it a simplification to check if only the first chunk is zero?



> +
> +	/* Buffer has non-zero data? */
> +	if (i < entry->buf_size / sizeof(u32)) {
> +		struct guc_lfd_data lfd;
> +
> +		size = xe_guc_log_add_lfd_header(&lfd);
> +		lfd.header |= FIELD_PREP(GUC_LFD_DATA_HEADER_MASK_TYPE, GUC_LFD_TYPE_FW_CRASH_DUMP);
> +		/* Calculate data length */
> +		lfd.data_count = DIV_ROUND_UP(entry->buf_size, sizeof(u32));
> +		/* Output GUC_LFD_TYPE_FW_CRASH_DUMP header */
> +		lfd_output_binary(p, (char *)&lfd, size);
> +
> +		/* rd/wr ptr is not used for crash dump */
> +		xe_guc_log_print_chunks(p, snapshot, from, to);
> +	}
> +	return size;
Should this size be additionally incremented by length of printed data?