Re: [PATCH v1 1/1] drm/xe/guc: Prevent GuC register capture running on VF

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

see my comments below.

Regards,
Zhanjun

On 2024-10-21 3:43 p.m., Michal Wajdeczko wrote:
> 
> 
> On 19.10.2024 00:43, Zhanjun Dong wrote:
>> GuC based register capture is not supported by VF, thus prevent it
>> running on VF.
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_capture.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
>> index 41262bda20ed..5c0d9efaaab4 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
>> @@ -1590,6 +1590,9 @@ xe_engine_manual_capture(struct xe_hw_engine *hwe, struct xe_hw_engine_snapshot
>>   	u16 guc_id = 0;
>>   	u32 lrca = 0;
>>   
>> +	if (IS_SRIOV_VF(xe))
>> +		return;
>> +
>>   	new = guc_capture_get_prealloc_node(guc);
>>   	if (!new)
>>   		return;
>> @@ -1756,7 +1759,7 @@ void xe_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm
>>   	devcoredump = &xe->devcoredump;
>>   	devcore_snapshot = &devcoredump->snapshot;
>>   
>> -	if (!devcore_snapshot->matched_node)
>> +	if (IS_SRIOV_VF(xe) || !devcore_snapshot->matched_node)
>>   		return;
> 
> do we need to check for VF here? if we correctly skipped capture on VFs
> then matched_node should be NULL, no?
Yes, it should be NULL for VF in this case.
> 
>>   
>>   	xe_gt_assert(gt, snapshot->source <= XE_ENGINE_CAPTURE_SOURCE_GUC);
>> @@ -1820,7 +1823,7 @@ xe_guc_capture_get_matching_and_lock(struct xe_sched_job *job)
>>   		return NULL;
>>   
>>   	xe = gt_to_xe(q->gt);
>> -	if (xe->wedged.mode >= 2 || !xe_device_uc_enabled(xe))
>> +	if (xe->wedged.mode >= 2 || !xe_device_uc_enabled(xe) || IS_SRIOV_VF(xe))
>>   		return NULL;
>>   
>>   	ss = &xe->devcoredump.snapshot;
>> @@ -1876,6 +1879,9 @@ xe_engine_snapshot_capture_for_job(struct xe_sched_job *job)
>>   	enum xe_hw_engine_id id;
>>   	u32 adj_logical_mask = q->logical_mask;
>>   
>> +	if (IS_SRIOV_VF(xe))
>> +		return;
>> +
>>   	for_each_hw_engine(hwe, q->gt, id) {
>>   		if (hwe->class != q->hwe->class ||
>>   		    !(BIT(hwe->logical_instance) & adj_logical_mask)) {
>> @@ -1922,6 +1928,9 @@ void xe_guc_capture_put_matched_nodes(struct xe_guc *guc)
>>   	struct xe_devcoredump *devcoredump = &xe->devcoredump;
>>   	struct __guc_capture_parsed_output *n = devcoredump->snapshot.matched_node;
>>   
>> +	if (IS_SRIOV_VF(xe))
>> +		return;
> 
> n (aka matched_node) should be NULL for VFs, no?
Yes, n should be NULL for VF in this case.

The above 2 condition is not needed, new rev will be posted soon.

> 
>> +
>>   	if (n) {
>>   		guc_capture_remove_stale_matches_from_list(guc->capture, n);
>>   		guc_capture_free_outlist_node(guc->capture, n);
>