Re: [PATCH v7 05/10] drm/xe/guc: Use a two stage dump for GuC logs and add more info

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

> +/**
> + * xe_guc_log_snapshot_capture - create a new snapshot copy the GuC log for later dumping
>   * @log: GuC log structure
> - * @p: the printer object to output to
> + * @atomic: is the call inside an atomic section of some kind?
> + *
> + * Return: pointer to a newly allocated snapshot object or null if out of memory. Caller is
> + * responsible for calling xe_guc_log_snapshot_free when done with the snapshot.
>   */
> -void xe_guc_log_print(struct xe_guc_log *log, struct drm_printer *p)
> +struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct xe_guc_log *log, bool atomic)
>  {
> +	struct xe_guc_log_snapshot *snapshot;
>  	struct xe_device *xe = log_to_xe(log);
> -	size_t size;
> -	void *copy;
> +	struct xe_guc *guc = log_to_guc(log);
> +	struct xe_gt *gt = log_to_gt(log);
> +	size_t remain;
> +	int i, err;
>  
>  	if (!log->bo) {
> -		drm_puts(p, "GuC log buffer not allocated");
> -		return;
> +		xe_gt_err(gt, "GuC log buffer not allocated\n");
> +		return NULL;
> +> +
> +	snapshot = xe_guc_log_snapshot_alloc(log, atomic);
> +	if (!snapshot) {
> +		xe_gt_err(gt, "GuC log snapshot not allocated\n");
> +		return NULL;
>  	}
It seems err is not yet set in this context. So it would be a random
stack value leaked? Or are there macros setting err I am not aware of here?
> #define xe_gt_err(_gt, _fmt, ...) \
> 	xe_gt_printk((_gt), err, _fmt, ##__VA_ARGS__)

Suggest to zero the "int err;" allocation. Or set explicitly.

In general why is xe_gt_err used here over drm_puts? Is it a more
specific error handler in this context?


>  
> -	size = log->bo->size;
> +	remain = snapshot->size;
> +	for (i = 0; i < snapshot->num_chunks; i++) {
> +		size_t size = min(GUC_LOG_CHUNK_SIZE, remain);
> +
> +		xe_map_memcpy_from(xe, snapshot->copy[i], &log->bo->vmap,
> +				   i * GUC_LOG_CHUNK_SIZE, size);Suggest cast i to size_t. Is the "i * GUC_LOG_CHUNK_SIZE" implicit
conversion fine?

> +		remain -= size;
> +	}
> +
> +	err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (err) {
> +		snapshot->stamp = ~0;
Is this a convention for timestamps when failed?


> +	drm_printf(p, "GuC firmware: %s\n", snapshot->path);
> +	drm_printf(p, "GuC version %u.%u.%u (wanted %u.%u.%u)\n",
> +		   snapshot->ver_found.major, snapshot->ver_found.minor, snapshot->ver_found.patch,
> +		   snapshot->ver_want.major, snapshot->ver_want.minor, snapshot->ver_want.patch);
Missing colon for "GuC version" print format string.


Reviewed-by: Julia Filipchuk <julia.filipchuk@intel.com>