Re: [PATCH v9 4/4] drm/xe/guc: Extract GuC capture lists to register snapshot

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

Please see my comments inline below.

Zhanjun

On 2024-06-14 8:31 a.m., Michal Wajdeczko wrote:
...
>> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
>> index ddfa855458ab..e1afda9070f4 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc.h
>> @@ -59,6 +59,29 @@ static inline u16 xe_engine_class_to_guc_class(enum xe_engine_class class)
>>   	}
>>   }
>>   
>> +static inline u16 xe_guc_class_to_capture_class(uint class)
>> +{
>> +	switch (class) {
>> +	case GUC_RENDER_CLASS:
>> +	case GUC_COMPUTE_CLASS:
>> +		return GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE;
>> +	case GUC_GSC_OTHER_CLASS:
>> +		return GUC_CAPTURE_LIST_CLASS_GSC_OTHER;
>> +	case GUC_VIDEO_CLASS:
>> +	case GUC_VIDEOENHANCE_CLASS:
>> +	case GUC_BLITTER_CLASS:
>> +		return class;
>> +	default:
>> +		XE_WARN_ON(class);
>> +		return -1;
> 
> it doesn't look like a safe value nor that you handle it correctly

The converted class ID will be used to find the match class in the list 
and NOT be used as index of array, so the -1 for search is safe.

> 
>> +	}
>> +}
>> +
>> +static inline u16 xe_engine_class_to_guc_capture_class(enum xe_engine_class class)
>> +{
>> +	return xe_guc_class_to_capture_class(xe_guc_class_to_capture_class(class));
> 
> are you sure this is correct ?
Oops, that's a bug. Thanks for point out.

...

>> +	/* Update the state of log buffer err-cap state */
>> +	xe_map_wr(guc_to_xe(guc), &guc->log.bo->vmap,
>> +		  log_buf_state_offset + offsetof(struct guc_log_buffer_state, read_ptr), u32,
>> +		  write_offset);
>> +	/* Clear the flush_to_file from local first, the local was loaded by above
>> +	 * xe_map_memcpy_from.
>> +	 */
>> +	log_buf_state_local.flush_to_file = 0;
>> +	/* Then write out the "updated local" through xe_map_wr() */
>> +	xe_map_wr(guc_to_xe(guc), &guc->log.bo->vmap,
>> +		  log_buf_state_offset + offsetof(struct guc_log_buffer_state, flags), u32,
>> +		  log_buf_state_local.flags);
>> +	__guc_capture_flushlog_complete(guc);
>> +}
>> +
> 
> public functions require kernel-doc
> 
>> +void xe_guc_capture_process(struct xe_guc *guc)
>> +{
>> +	if (guc->capture)
>> +		__guc_capture_process_output(guc);
>> +}
Will add

...

>>   	trace_xe_exec_queue_reset(q);
>>   
>>   	/*
>> @@ -1715,6 +1728,24 @@ int xe_guc_exec_queue_reset_handler(struct xe_guc *guc, u32 *msg, u32 len)
>>   	return 0;
>>   }
>>   
> 
> missing kerne-doc
Will add

> 
>> +int xe_guc_error_capture_handler(struct xe_guc *guc, u32 *msg, u32 len)
> 
> maybe this should be defined as xe_guc_capture_msg_handler() and placed
> in xe_guc_capture.c as it doesn't look that it needs anything from
> xe_guc_submit.c code
> 
>> +{
>> +	u32 status;
>> +
>> +	if (unlikely(len != 1)) {
> 
> magic "1"
Will changed to a macro define
> 
>> +		xe_gt_dbg(guc_to_gt(guc), "Invalid length %u", len);
>> +		return -EPROTO;
>> +	}
>> +
>> +	status = msg[0] & XE_GUC_STATE_CAPTURE_EVENT_STATUS_MASK;
>> +	if (status == XE_GUC_STATE_CAPTURE_EVENT_STATUS_NOSPACE)
>> +		xe_gt_warn(guc_to_gt(guc), "G2H-Error capture no space");
> 
> btw, is there anything to capture if GuC reported 'NOSPACE' ?

We have the gt_warn to warn it in dmesg, let user know what happens.
The next guc_capture_process will try to process whatever received.

...