Re: [PATCH 1/2] drm/xe/pf: Trigger explicit FLR while disabling VFs

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 25.06.2024 18:19, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pon [2024-cze-24 22:57:36 +0200]:
>> We attempt to unprovision all VFs GuC when disabling them, but
>> GuC may reject such request if the target VF was previously active
>> but VF driver didn't unload with explicit VF reset H2G action or
>> the VMM has not started the VF FLR.
>>
>> To avoid mismatches between configs maintained the PF and GuC,
>> trigger an explicit FLR sequences just before releasing resources.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c | 21 +++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_control.h |  1 +
>>  drivers/gpu/drm/xe/xe_pci_sriov.c           | 14 ++++++++++++++
>>  3 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
>> index 40b8f881fe04..ebf06e037750 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
>> @@ -129,6 +129,27 @@ int xe_gt_sriov_pf_control_stop_vf(struct xe_gt *gt, unsigned int vfid)
>>  	return pf_send_vf_stop(gt, vfid);
>>  }
>>  
>> +/**
>> + * xe_gt_sriov_pf_control_trigger_flr - Start a VF FLR sequence.
>> + * @gt: the &xe_gt
>> + * @vfid: the VF identifier
>> + *
>> + * This function is for PF only.
> 
> NIT: If I haven't missed anything, it seems to me that we don't have any
> assert to check IS_SRIOV_PF, maybe it would be worth adding it here?

we will add one once we start using PF specific data stored somewhere
under gt->sriov.pf, in the meantime we don't enforce it, as any abuse
won't be catastrophic ;)

> 
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int xe_gt_sriov_pf_control_trigger_flr(struct xe_gt *gt, unsigned int vfid)
>> +{
>> +	int err;
>> +
>> +	/* XXX pf_send_vf_flr_start() expects ct->lock */
>> +	mutex_lock(&gt->uc.guc.ct.lock);
>> +	err = pf_send_vf_flr_start(gt, vfid);
>> +	mutex_unlock(&gt->uc.guc.ct.lock);
>> +
>> +	return err;
>> +}
>> +
>>  /**
>>   * DOC: The VF FLR Flow with GuC
>>   *
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.h b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.h
>> index 850a3e37661f..405d1586f991 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.h
>> @@ -14,6 +14,7 @@ struct xe_gt;
>>  int xe_gt_sriov_pf_control_pause_vf(struct xe_gt *gt, unsigned int vfid);
>>  int xe_gt_sriov_pf_control_resume_vf(struct xe_gt *gt, unsigned int vfid);
>>  int xe_gt_sriov_pf_control_stop_vf(struct xe_gt *gt, unsigned int vfid);
>> +int xe_gt_sriov_pf_control_trigger_flr(struct xe_gt *gt, unsigned int vfid);
>>  
>>  #ifdef CONFIG_PCI_IOV
>>  int xe_gt_sriov_pf_control_process_guc2pf(struct xe_gt *gt, const u32 *msg, u32 len);
>> diff --git a/drivers/gpu/drm/xe/xe_pci_sriov.c b/drivers/gpu/drm/xe/xe_pci_sriov.c
>> index 06d0fceb5114..74c8fadc9365 100644
>> --- a/drivers/gpu/drm/xe/xe_pci_sriov.c
>> +++ b/drivers/gpu/drm/xe/xe_pci_sriov.c
>> @@ -6,6 +6,7 @@
>>  #include "xe_assert.h"
>>  #include "xe_device.h"
>>  #include "xe_gt_sriov_pf_config.h"
>> +#include "xe_gt_sriov_pf_control.h"
>>  #include "xe_pci_sriov.h"
>>  #include "xe_pm.h"
>>  #include "xe_sriov.h"
>> @@ -37,6 +38,17 @@ static void pf_unprovision_vfs(struct xe_device *xe, unsigned int num_vfs)
>>  			xe_gt_sriov_pf_config_release(gt, n, true);
>>  }
>>  
>> +static void pf_reset_vfs(struct xe_device *xe, unsigned int num_vfs)
>> +{
>> +	struct xe_gt *gt;
>> +	unsigned int id;
>> +	unsigned int n;
>> +
>> +	for_each_gt(gt, xe, id)
>> +		for (n = 1; n <= num_vfs; n++)
>> +			xe_gt_sriov_pf_control_trigger_flr(gt, n);
>> +}
>> +
>>  static int pf_enable_vfs(struct xe_device *xe, int num_vfs)
>>  {
>>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> @@ -94,6 +106,8 @@ static int pf_disable_vfs(struct xe_device *xe)
>>  
>>  	pci_disable_sriov(pdev);
>>  
>> +	pf_reset_vfs(xe, num_vfs);
>> +
>>  	pf_unprovision_vfs(xe, num_vfs);
>>  
>>  	/* not needed anymore - see pf_enable_vfs() */
> 
> 
> One comment above, but the code seems fine:
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

thanks

>> -- 
>> 2.43.0
>>
>