Re: [PATCH] drm/xe/: Mutual Exclusivity b/w Multi CCS Mode & SRIOV VF Provisioning

["Summers, Stuart" <stuart.summers@intel.com>]

On Mon, 2025-09-29 at 14:06 +0530, Nareshkumar Gollakoti wrote:
> Due to SLA agreement between PF and VFs, multi CCS mode can't
> be enabled when VFs are already enabled.
> Similarly, enabling VFs is disabled when multi ccs mode enabled.

I have a similar comment inline below as well, but while you're
describing what you're doing here, I don't understand why we need to do
it. The previous check should have covered this too right?

> 
> v2:function xe_device_is_vf_enabled has been refactored to
> xe_sriov_pf_has_vfs_enabled and moved to xe_sriov_pf_helper.h.
> The code now distinctly checks for SR-IOV VF mode and
> SR-IOV PF with VFs enabled.
> Log messages have been updated to explicitly state the current mode.
> The function xe_multi_ccs_mode_enabled is moved to xe_device.h
> 
> v3: Described missed arg documentation for
> xe_sriov_pf_has_vfs_enabled
> 
> Signed-off-by: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.h           |  8 ++++++++
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.c      | 14 +++++++++++---
>  drivers/gpu/drm/xe/xe_pci_sriov.c        |  6 ++++++
>  drivers/gpu/drm/xe/xe_sriov_pf_helpers.h | 13 +++++++++++++
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.h
> b/drivers/gpu/drm/xe/xe_device.h
> index 32cc6323b7f6..986f9cabb897 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -172,6 +172,14 @@ static inline bool xe_device_has_lmtt(struct
> xe_device *xe)
>         return IS_DGFX(xe);
>  }
>  
> +static inline bool xe_multi_ccs_mode_enabled(struct xe_device *xe)
> +{
> +       /* Multi CCS mode supported exclusively on GT0 */
> +       struct xe_gt *gt = xe_device_get_gt(xe, 0);
> +
> +       return gt->ccs_mode > 1;
> +}
> +
>  u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size);
>  
>  void xe_device_snapshot_print(struct xe_device *xe, struct
> drm_printer *p);
> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> index 50fffc9ebf62..584f3245fc7d 100644
> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> @@ -13,6 +13,7 @@
>  #include "xe_gt_sysfs.h"
>  #include "xe_mmio.h"
>  #include "xe_sriov.h"
> +#include "xe_sriov_pf_helpers.h"
>  
>  static void __xe_gt_apply_ccs_mode(struct xe_gt *gt, u32
> num_engines)
>  {
> @@ -117,9 +118,16 @@ ccs_mode_store(struct device *kdev, struct
> device_attribute *attr,
>         u32 num_engines, num_slices;
>         int ret;
>  
> -       if (IS_SRIOV(xe)) {
> -               xe_gt_dbg(gt, "Can't change compute mode when running
> as %s\n",
> -                        
> xe_sriov_mode_to_string(xe_device_sriov_mode(xe)));
> +       /*
> +        * Check if the device is:
> +        * 1. Operating as an SR-IOV Virtual Function (VF), or
> +        * 2. An SR-IOV Physical Function (PF) with one or more VFs
> enabled.
> +        * Enabling multi CCS mode is not permitted in either
> scenario.
> +        */

IMO this comment isn't needed. The two conditions you have below are
self-explanatory.

> +       if (IS_SRIOV_VF(xe) || xe_sriov_pf_has_vfs_enabled(xe)) {

Also as I mentioned earlier, why do we need to make this change here?
Is there a requirement to enable this in SRIOV_PF mode? My
understanding is we wanted to explicitly stick to the default mode (one
CCS) in this case.

Thanks,
Stuart

> +               const char *mode_str =
> !strcmp(xe_sriov_mode_to_string(xe_device_sriov_mode(xe)),
> +                                       "SR-IOV VF") ? "SR-IOV VF" :
> "SR-IOV PF with VFs Enabled";
> +               xe_gt_dbg(gt, "Can't change compute mode when running
> as %s\n", mode_str);
>                 return -EOPNOTSUPP;
>         }
>  
> diff --git a/drivers/gpu/drm/xe/xe_pci_sriov.c
> b/drivers/gpu/drm/xe/xe_pci_sriov.c
> index af05db07162e..71c1d998ba82 100644
> --- a/drivers/gpu/drm/xe/xe_pci_sriov.c
> +++ b/drivers/gpu/drm/xe/xe_pci_sriov.c
> @@ -155,6 +155,12 @@ static int pf_enable_vfs(struct xe_device *xe,
> int num_vfs)
>         xe_assert(xe, num_vfs <= total_vfs);
>         xe_sriov_dbg(xe, "enabling %u VF%s\n", num_vfs,
> str_plural(num_vfs));
>  
> +       if (xe_multi_ccs_mode_enabled(xe)) {
> +               xe_sriov_info(xe, "Disable multi-ccs mode before
> enabling VF's\n");
> +
> +               return -ECANCELED;
> +       }
> +
>         err = xe_sriov_pf_wait_ready(xe);
>         if (err)
>                 goto out;
> diff --git a/drivers/gpu/drm/xe/xe_sriov_pf_helpers.h
> b/drivers/gpu/drm/xe/xe_sriov_pf_helpers.h
> index dd1df950b021..e26837091375 100644
> --- a/drivers/gpu/drm/xe/xe_sriov_pf_helpers.h
> +++ b/drivers/gpu/drm/xe/xe_sriov_pf_helpers.h
> @@ -43,4 +43,17 @@ static inline struct mutex
> *xe_sriov_pf_master_mutex(struct xe_device *xe)
>         return &xe->sriov.pf.master_lock;
>  }
>  
> +/**
> + * xe_sriov_pf_has_vfs_enabled() - Determines if the PF has any VFs
> enabled
> + * @xe: ptr to xe_device
> + *
> + * Return: true if one or more VFs are enabled on the PF, false
> otherwise.
> + */
> +static inline bool xe_sriov_pf_has_vfs_enabled(const struct
> xe_device *xe)
> +{
> +       struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +
> +       return pci_num_vf(pdev) > 0;
> +}
> +
>  #endif