Re: [PATCH v2 3/4] drm/xe/vf: Start post-migration fixups with provisinoning query

["Lis, Tomasz" <tomasz.lis@intel.com>]

On 26.09.2024 16:27, Michal Wajdeczko wrote:
>
> On 24.09.2024 22:25, Tomasz Lis wrote:
>> During post-migration recovery, only MMIO communication to GuC is
>> allowed. The VF KMD needs to use that channel to ask for the new
>> provisioning, which includes a new GGTT range assigned to the VF.
> you likely need first to remove below assert from the
> xe_guc_mmio_send_recv()
>
> 	xe_assert(xe, !xe_guc_ct_enabled(&guc->ct));
>
>> v2: query config only instead of handshake; no need to get pm ref as
>>   it's now kept through whole recovery (mwajdeczko)
>>
>> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_sriov_vf.c | 39 ++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf.c b/drivers/gpu/drm/xe/xe_sriov_vf.c
>> index d0c5a0b7e170..fe5eefa736c8 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_vf.c
>> +++ b/drivers/gpu/drm/xe/xe_sriov_vf.c
>> @@ -24,6 +24,34 @@ void xe_sriov_vf_init_early(struct xe_device *xe)
>>   	INIT_WORK(&xe->sriov.vf.migration.worker, migration_worker_func);
>>   }
>>   
>> +/**
>> + * vf_post_migration_requery_guc - Re-initialize GuC communication.
>> + * @xe: the &xe_device struct instance
>> + *
>> + * After migration, we need to reestablish communication with GuC and
>> + * re-query all VF configuration to make sure they match previous
>> + * provisioning. Note that most of VF provisioning shall be the same,
>> + * except GGTT range, since GGTT is not virtualized per-VF.
>> + *
>> + * Returns: 0 if the operation completed successfully, or a negative error
>> + * code otherwise.
>> + */
>> +static int vf_post_migration_requery_guc(struct xe_device *xe)
>> +{
>> +	struct xe_gt *gt;
>> +	unsigned int id;
>> +	int err, ret;
> 	int err, ret = 0;
>
>> +
>> +	err = 0;
>> +	for_each_gt(gt, xe, id) {
>> +		ret = xe_gt_sriov_vf_query_config(gt);
> 		err = xe_gt_sriov_vf_query_config(gt);
>
>> +		if (!err)
>> +			err = ret;
> 		ret = ret ?: err;
>
>> +	}
>> +
>> +	return err;
> 	return ret;

ok, will do. But that doesn't seem any better than it was before.

Well, maybe except the ternary operator, that does look better (though 
it's not a part of c89).

>
>> +}
>> +
>>   /*
>>    * vf_post_migration_notify_resfix_done - Notify all GuCs about resource fixups apply finished.
>>    * @xe: the &xe_device struct instance
>> @@ -44,12 +72,23 @@ static void vf_post_migration_notify_resfix_done(struct xe_device *xe)
>>   
>>   static void vf_post_migration_recovery(struct xe_device *xe)
>>   {
>> +	int err;
>> +
>>   	drm_dbg(&xe->drm, "migration recovery in progress\n");
>>   	xe_pm_runtime_get(xe);
>> +	err = vf_post_migration_requery_guc(xe);
>> +	if (unlikely(err))
>> +		goto fail;
> shouldn't all this be below "add the recovery steps" line ?

If requery failed, why would we continue with fixups? We don't know the 
new GGTT range in that case.

-Tomasz

>
>> +
>>   	/* FIXME: add the recovery steps */
>>   	vf_post_migration_notify_resfix_done(xe);
>>   	xe_pm_runtime_put(xe);
>>   	drm_notice(&xe->drm, "migration recovery ended\n");
>> +	return;
>> +fail:
>> +	xe_pm_runtime_put(xe);
>> +	drm_err(&xe->drm, "migration recovery failed (%pe)\n", ERR_PTR(err));
>> +	xe_device_declare_wedged(xe);
>>   }
>>   
>>   static void migration_worker_func(struct work_struct *w)