Re: [V9 PATCH] drm/xe: Mutual exclusivity between CCS-mode and PF

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

On 11/28/2025 1:38 PM, Nareshkumar Gollakoti wrote:
> Due to SLA agreement between PF and VFs,the alternate CCS-mode
> cannot be changed when VFs are already enabled.
> Similarly, enabling VFs is not permitted when the alternate
> CCS-mode is active. Additionally, the sysfs entry for
> CCS-mode is not created for SR-IOV VF mode.

maybe this VF-only change deserves to be in a earlier separate patch?
we could even add Fixes tag, as VFs should never expose this file
(unless it would be read-only file with (expected) fixed "1" mode)

> 
> Signed-off-by: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>
> ---
> 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
> 
> v4:
> - sysfs interface for CCS mode is not initialized
>   when operating in SRIOV VF Mode.
> - xe_sriov_pf_has_vfs_enabled() check is sufficient while CCS mode
>   enablement.
> - remove unnecessary comments as flow is self explanatory.
> 
> v5:(review comments from Michal)
> - Add xe device level CCS mode block with mutex lock and CCS mode state
> - necessesary functions to manage ccs mode state to provide strict mutual
>   exclusive support b/w CCS mode & SRIOV VF enabling
> 
> v6:
> - Re modeled implementation based on lockdown the PF using custom guard
>   supported functions by Michal
> 
> v7:
> - Corrected patch style as message written as subject
> - Used public PF lockdown functions instead internal funcions(Michal)
> - Creating CCS Mode entries only on PF Mode
> 
> v8:(Michal)
> - updated short subject and few comments
> - used guard for mutex
> - Add a check of PF Mode to ensure use of xe_sriov_pf_lockdown only in
>   PF Mode
> - Added default CCS mode check to xe_gt_ccs_mode_default(gt) function
> 
> v9:(Michal)
> - Added xe_gt_ccs_mode_default(gt) as static inline and it can be used
>   across driver to use between default or alternate CCS mode
> - removed comment from obvious code
> ---
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.c | 47 +++++++++++++++++++++++------
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.h | 12 ++++++++
>  2 files changed, 49 insertions(+), 10 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..8621c39cdf58 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.h"
>  
>  static void __xe_gt_apply_ccs_mode(struct xe_gt *gt, u32 num_engines)
>  {
> @@ -108,6 +109,35 @@ ccs_mode_show(struct device *kdev,
>  	return sysfs_emit(buf, "%u\n", gt->ccs_mode);
>  }
>  
> +static int gt_prepare_ccs_mode_enabling(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	if (!IS_SRIOV_PF(xe))
> +		return 0;
> +
> +	/*
> +	 * We can't change CCS-mode when VFs are already enabled
> +	 * and we must prevent enabling VFs when alternate
> +	 * CCS-mode is active
> +	 */
> +	if (xe_gt_ccs_mode_default(gt))
> +		return xe_sriov_pf_lockdown(xe);
> +
> +	return 0;
> +}
> +
> +static void gt_finish_ccs_mode_enabling(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	if (IS_SRIOV_PF(xe)) {

maybe we can use the same logic flow as in 'prepare' function above:
exit early if !PF

> +		/* Allow enabling VFs, if CCS-mode changed to default mode */
> +		if (xe_gt_ccs_mode_default(gt))
> +			xe_sriov_pf_end_lockdown(xe);
> +	}
> +}
> +
>  static ssize_t
>  ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  	       const char *buff, size_t count)
> @@ -117,12 +147,6 @@ 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)));
> -		return -EOPNOTSUPP;
> -	}
> -
>  	ret = kstrtou32(buff, 0, &num_engines);
>  	if (ret)
>  		return ret;
> @@ -139,13 +163,16 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  	}
>  
>  	/* CCS mode can only be updated when there are no drm clients */
> -	mutex_lock(&xe->drm.filelist_mutex);
> +	guard(mutex)(&xe->drm.filelist_mutex);
>  	if (!list_empty(&xe->drm.filelist)) {
> -		mutex_unlock(&xe->drm.filelist_mutex);
>  		xe_gt_dbg(gt, "Rejecting compute mode change as there are active drm clients\n");
>  		return -EBUSY;
>  	}
>  
> +	ret = gt_prepare_ccs_mode_enabling(gt);
> +	if (ret)
> +		return ret;

don't you want to add some dbg message why the change is rejected"

  		xe_gt_dbg(gt, "Rejecting compute mode change as VFs are enabled\n");

> +
>  	if (gt->ccs_mode != num_engines) {

btw, shouldn't this be checked earlier?
then if requested mode matches current mode we could still return success,
even it there are drm clients and/or VFs are enabled (or was this done on
purpose?

>  		xe_gt_info(gt, "Setting compute mode to %d\n", num_engines);
>  		gt->ccs_mode = num_engines;
> @@ -153,7 +180,7 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  		xe_gt_reset(gt);
>  	}
>  
> -	mutex_unlock(&xe->drm.filelist_mutex);
> +	gt_finish_ccs_mode_enabling(gt);
>  
>  	return count;
>  }
> @@ -191,7 +218,7 @@ int xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt)
>  	struct xe_device *xe = gt_to_xe(gt);
>  	int err;
>  
> -	if (!xe_gt_ccs_mode_enabled(gt))
> +	if (!xe_gt_ccs_mode_enabled(gt) || IS_SRIOV_VF(xe))
>  		return 0;
>  
>  	err = sysfs_create_files(gt->sysfs, gt_ccs_mode_attrs);
> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> index f8779852cf0d..c5b459ef2f79 100644
> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> @@ -20,5 +20,17 @@ static inline bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt)
>  	return hweight32(CCS_MASK(gt)) > 1;
>  }
>  
> +/**
> + * xe_gt_ccs_mode_default - check if CCS mode is default (single CCS mode)

add () after function name:

    * xe_gt_ccs_mode_default() - Check if ...

> + * @gt: GT structure

add empty line before 'Return' tag:

    *

> + * Return:
> + * %true if CCS mode is default(i.e. single CCS mode)
> + * %false if alternate/multi CCS mode

note that above will be rendered as single line, so maybe:

    * Return: %true if actual CCS is mode is single mode, or
    *         %false otherwise (CCS in alternate/multi mode)


> + */
> +static inline bool xe_gt_ccs_mode_default(struct xe_gt *gt)
> +{
> +	return gt->ccs_mode == 1;
> +}
> +
>  #endif
>