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

["Kumar G, Naresh" <naresh.kumar.g@intel.com>]

Hi Michal,
On 26-11-2025 00:43, Michal Wajdeczko wrote:
> 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)
> 
>>
Noted and update in next revision
> 
> 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
>
Noted

>> +					   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
> 
will update in next version
>> +	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?
> 
yes it can be done anyhow prior is just input validations. will update 
in next revision.
>> +	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"
> 
That's a good point. Since this function isn't part of the current 
patch, we can address the API name change in a follow-up patch. For now, 
I'd prefer to proceed with this patch using the existing name, and we 
can update the function name in the subsequent change.??

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