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

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

Hi Satya,

On 03-12-2025 14:22, K V P, Satyanarayana wrote:
> 
> On 28-Nov-25 10:48 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.
>>
>> Signed-off-by: Nareshkumar Gollakoti<naresh.kumar.g@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt_ccs_mode.c | 59 ++++++++++++++++++++++-------
>>   drivers/gpu/drm/xe/xe_gt_ccs_mode.h | 12 ++++++
>>   2 files changed, 57 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>> index e146e00b0ca2..6652c468e1be 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,36 @@ 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;
> 
> The ccs mode will not be enabled for native mode (non-virtualized mode) 
> with this. It should be something like
> 
> if (IS_SRIOV_VF(xe))
> 
>          return 0;
> 
> Same comment for other places as well.
As no CCS-mode sysfs entry for SRIOV VF Mode[1], no need check for VF mode.
[1]: https://patchwork.freedesktop.org/patch/691098/?series=158222&rev=1

In SRIOV PF mode, use xe_sriov_pf_lockdown to block CCS-mode. If VFs are 
  enabled. otherwise, CCS-mode can be allowed.
If not in PF mode, it is considered as Non-SRIOV mode and the function 
returns 0. Therefore, CCS mode is enabled without any restrictions.
> 
>> +
>> +	/*
>> +	 * 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))
>> +		return;
>> +
>> +	/* 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 +148,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,21 +164,27 @@ 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;
>>   	}
>>   
>> -	if (gt->ccs_mode != num_engines) {
>> -		xe_gt_info(gt, "Setting compute mode to %d\n", num_engines);
>> -		gt->ccs_mode = num_engines;
>> -		xe_gt_record_user_engines(gt);
>> -		xe_gt_reset(gt);
>> +	if (gt->ccs_mode == num_engines)
>> +		return count;
>> +
>> +	ret = gt_prepare_ccs_mode_enabling(gt);
>> +	if (ret) {
>> +		xe_gt_dbg(gt, "Rejecting compute mode change as VFs are enabled\n");
>> +		return ret;
>>   	}
> 
> The function gt_prepare_ccs_mode_enabling() never returning non-zero 
> error if VFs are enabled.
That is not ture, gt_prepapre_ccs_mode_enabling() return 
non-zeor(-EBUSY) when vfs enabled.
> 
>>   
>> -	mutex_unlock(&xe->drm.filelist_mutex);
>> +	xe_gt_info(gt, "Setting compute mode to %d\n", num_engines);
>> +	gt->ccs_mode = num_engines;
>> +	xe_gt_record_user_engines(gt);
>> +	xe_gt_reset(gt);
>> +
> 
> Do we need to apply gt reset after changing CCS mode? If yes, add a 
> comment with why do we need it.
> 
yes, ensures clean state as we modified new CCS mode for engine setup.
> -Satya.
> 
Thanks,
Naresh
>> +	gt_finish_ccs_mode_enabling(gt);
>>   
>>   	return count;
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>> index f8779852cf0d..53a595b0882c 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)
>> + * @gt: GT structure
>> + *
>> + * Return: %true if actual CCS 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
>>