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

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

subject is little too long, maybe:

	"drm/xe: Mutual exclusivity between CCS-mode and PF"

On 11/25/2025 5:57 PM, Nareshkumar Gollakoti wrote:
> Use PF lockdown supported functions to enforce mutual exclusivity between
> CCS Mode and SRIOV VF enabling/provisioning during CCS Mode enabling.

please explain in commit message "why" we need this

[1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

and also mention about a change for the VF case (no sysfs file in VF mode)

> 

and then you can move whole below change log under ---

> 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
> 
> Signed-off-by: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.c | 48 +++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 9 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..468c3a6790d0 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,29 @@ ccs_mode_show(struct device *kdev,
>  	return sysfs_emit(buf, "%u\n", gt->ccs_mode);
>  }
>  
> +static int xe_gt_prepare_ccs_mode_enabling(struct xe_device *xe,

nit: usually we don't use xe_ prefix for static functions

and there is no point in passing *xe since there is *gt

> +					   struct xe_gt *gt)
> +{
> +	/*
> +	 * The arm guard is only activated during CCS mode enabling,
> +	 * and this shuould happen when CCS mode is in default mode.
> +	 * lockdown arm guard ensures there is no VFS enabling
> +	 * as CCS mode enabling in progress/enabled.

this should rather say just something like:

	* We can't change CCS-mode when VFs are already enabled and we
	* must prevent enabling VFs when alternate CCS-mode is active.

> +	 */
> +	if (!(gt->ccs_mode > 1))

can we have helper which name would describe this magic condition?

	bool xe_gt_ccs_mode_default(gt)

> +		return xe_sriov_pf_lockdown(xe);

note that all xe_sriov_pf_xxx() functions expect to be called only in the PF mode

so before calling this xe_sriov_pf_lockdown() you must use IS_SRIOV_PF(xe)

> +
> +	return 0;
> +}
> +
> +static void xe_gt_finish_ccs_mode_enabling(struct xe_device *xe,
> +					   struct xe_gt *gt)
> +{
> +	/* disarm the guard, if CCS mode is reverted to default */

"guard" is just an implementation detail of the "PF lockdown" feature

> +	if (!(gt->ccs_mode > 1))
> +		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,15 +141,13 @@ 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 = xe_gt_prepare_ccs_mode_enabling(xe, gt);

shouldn't this be done under below mutex?

> +	if (ret)
> +		return ret;
>  
>  	ret = kstrtou32(buff, 0, &num_engines);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	/*
>  	 * Ensure numbers of engines specified is valid and there is an
> @@ -135,7 +157,8 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  	if (!num_engines || num_engines > num_slices || num_slices % num_engines) {
>  		xe_gt_dbg(gt, "Invalid compute config, %d engines %d slices\n",
>  			  num_engines, num_slices);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err;
>  	}
>  
>  	/* CCS mode can only be updated when there are no drm clients */
> @@ -143,7 +166,8 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  	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 = -EBUSY;
> +		goto err;
>  	}
>  
>  	if (gt->ccs_mode != num_engines) {
> @@ -155,7 +179,13 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  
>  	mutex_unlock(&xe->drm.filelist_mutex);

to avoid such manual unlocks, you may want to start using:

	guard(mutex)(&xe->drm.filelist_mutex);

but then make sure to do not use "goto"

>  
> +	xe_gt_finish_ccs_mode_enabling(xe, gt);
> +
>  	return count;

	return ret ?: count;

> +err:
> +	xe_gt_finish_ccs_mode_enabling(xe, gt);
> +
> +	return ret;
>  }
>  
>  static DEVICE_ATTR_RW(ccs_mode);
> @@ -191,7 +221,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))

btw, the "xe_gt_ccs_mode_enabled" name is little misleading,
IMO better name would be "xe_gt_ccs_mode_supported"

> +	if (!xe_gt_ccs_mode_enabled(gt) || IS_SRIOV_VF(xe))
>  		return 0;
>  
>  	err = sysfs_create_files(gt->sysfs, gt_ccs_mode_attrs);