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

[Varun Gupta <varun.gupta@intel.com>]

>Multi CCS mode can only be enabled when SRIOV is in PF mode with
>no VFs provisioned.
>Similarly, provisioning VFs is not allowed when Multi CCS mode is active.
>
>Signed-off-by: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>
>---
> drivers/gpu/drm/xe/xe_gt_ccs_mode.c |  7 ++++---
> drivers/gpu/drm/xe/xe_gt_ccs_mode.h | 10 ++++++++++
> drivers/gpu/drm/xe/xe_pci_sriov.c   |  7 +++++++
> drivers/gpu/drm/xe/xe_sriov.h       | 12 ++++++++++++
> 4 files changed, 33 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>index 50fffc9ebf62..39df801e9cd5 100644
>--- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>+++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>@@ -117,9 +117,10 @@ 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)));
>+	if (IS_SRIOV_VF_ENABLED(xe)) {
>+		xe_gt_dbg(gt, "Can't change compute mode when running as %s / %s\n",
>+			  xe_sriov_mode_to_string(xe_device_sriov_mode(xe)),
>+			  "One or more VFs enabled");

Consider using the conditional (ternary) operator to differentiate 
between the two SR-IOV cases instead of hardcoding the second string.

> 		return -EOPNOTSUPP;
> 	}
> 
>diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>index f8779852cf0d..1a80a293daa2 100644
>--- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>+++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>@@ -20,5 +20,15 @@ static inline bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt)
> 	return hweight32(CCS_MASK(gt)) > 1;
> }
> 
>+static inline bool xe_is_primary_gt_multi_ccs_enabled(struct xe_device *xe)
>+{
>+	/* multi CCS mode supported exclusively on GT0 */

%s:/multi/Multi

>+	struct xe_gt *gt = xe_device_get_gt(xe, 0);
>+
>+	return (gt->ccs_mode > 1);
>+}
>+
>+#define IS_PRIMARY_GT_MULTI_CCS_ENABLED(xe) xe_is_primary_gt_multi_ccs_enabled(xe)

Rename to IS_GT0_MULTI_CCS_ENABLED and xe_is_gt0_multi_ccs_enabled

>+
> #endif
> 
>diff --git a/drivers/gpu/drm/xe/xe_pci_sriov.c b/drivers/gpu/drm/xe/xe_pci_sriov.c
>index af05db07162e..96e3fd51b8f5 100644
>--- a/drivers/gpu/drm/xe/xe_pci_sriov.c
>+++ b/drivers/gpu/drm/xe/xe_pci_sriov.c
>@@ -19,6 +19,7 @@
> #include "xe_sriov_pf.h"
> #include "xe_sriov_pf_helpers.h"
> #include "xe_sriov_printk.h"
>+#include "xe_gt_ccs_mode.h"
> 
> static int pf_needs_provisioning(struct xe_gt *gt, unsigned int num_vfs)
> {
>@@ -153,6 +154,12 @@ static int pf_enable_vfs(struct xe_device *xe, int num_vfs)
> 	xe_assert(xe, IS_SRIOV_PF(xe));
> 	xe_assert(xe, num_vfs > 0);
> 	xe_assert(xe, num_vfs <= total_vfs);
>+
>+	if (IS_PRIMARY_GT_MULTI_CCS_ENABLED(xe)) {
>+		xe_sriov_info(xe, "Can't change the number of VFs while multi-CCS mode is enabled.");

Use "Cannot enable VF's" instead of "Can't change number of VF's"

>+
>+		return -EOPNOTSUPP;
>+	}
> 	xe_sriov_dbg(xe, "enabling %u VF%s\n", num_vfs, str_plural(num_vfs));
> 
> 	err = xe_sriov_pf_wait_ready(xe);
>diff --git a/drivers/gpu/drm/xe/xe_sriov.h b/drivers/gpu/drm/xe/xe_sriov.h
>index 6db45df55615..78019cee61fe 100644
>--- a/drivers/gpu/drm/xe/xe_sriov.h
>+++ b/drivers/gpu/drm/xe/xe_sriov.h
>@@ -36,6 +36,16 @@ static inline bool xe_device_is_sriov_vf(const struct xe_device *xe)
> 	return xe_device_sriov_mode(xe) == XE_SRIOV_MODE_VF;
> }
> 
>+static inline bool xe_device_is_vf_enabled(const struct xe_device *xe)
>+{
>+	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>+
>+	if (xe_device_is_sriov_vf(xe))
>+		return true;

Maybe remove this check since next return checks for provisioned VF's
and return true even when SRIOV-VF mode

>+
>+	return (pci_num_vf(pdev) > 0);
>+}
>+
> #ifdef CONFIG_PCI_IOV
> #define IS_SRIOV_PF(xe) xe_device_is_sriov_pf(xe)
> #else
>@@ -45,4 +55,6 @@ static inline bool xe_device_is_sriov_vf(const struct xe_device *xe)
> 
> #define IS_SRIOV(xe) (IS_SRIOV_PF(xe) || IS_SRIOV_VF(xe))
> 
>+#define IS_SRIOV_VF_ENABLED(xe) xe_device_is_vf_enabled(xe)
>+
> #endif

Thanks,
Varun