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

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

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?

> + *
> + * 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>
> -- 
> 2.43.0
> 

--