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

[John Harrison <john.c.harrison@intel.com>]

On 9/10/2024 17:48, Julia Filipchuk wrote:
>> +/**
>> + * 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?
The 'err' inside the xe_gt_err macro is not a local variable from 
outside the macro. It is used to generate the next function call down - 
"drm_err". There is no drm_puts any more because this function is now 
just an allocator, not a printer. So there is no print stream to call 
drm_puts on.

>
>
>>   
>> -	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?
It would only be an issue if the GuC log buffer were over 2GB in size. 
However, the log is limited to 16MB by the GuC interface spec. So there 
is no possibility of 32bit overflow.

>
>> +		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?
Not sure if there is a convention as such, but ~0 seems even less likely 
to be valid than zero. And one has to pick something (or add in the 
extra complication of a 'is_stamp_valid' flag).

>
>
>> +	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.
Yup.

>
>
> Reviewed-by: Julia Filipchuk <julia.filipchuk@intel.com>
Note that you shouldn't really give an r-b when you have so many 
questions / changes requested.

John.