Re: [Intel-xe] [PATCH v2 2/3] drm/xe: Allow userspace to configure CCS mode

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Fri, Dec 01, 2023 at 09:57:49PM +0100, Andi Shyti wrote:
>Hi Niranjana,
>
>> +static ssize_t
>> +ccs_mode_show(struct device *kdev,
>> +	      struct device_attribute *attr, char *buf)
>> +{
>> +	struct xe_gt *gt = kobj_to_gt(&kdev->kobj);
>> +
>> +	return sysfs_emit(buf, "Enabled compute engines %d; Number of compute slices %d\n",
>> +			  gt->ccs_mode.num_engines, hweight32(CCS_MASK(gt)));
>> +}
>
>Please add separate interfaces called:
>
>	- compute_engines
>	- compute_slices
>
>and each of them providing respecively
>
>	- gt->ccs_mode.num_engines
>	- hweight32(CCS_MASK(gt))
>

Thanks Andi for the review.
Yah, I like that idea.
I will make it,
- ccs_mode, a RW interface for reading/configuring ccs_mode(num_engines)
- compute_slices, a read-only interface for reading number of available compute slices.

>> +static ssize_t
>> +ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>> +	       const char *buff, size_t count)
>> +{
>> +	struct xe_gt *gt = kobj_to_gt(&kdev->kobj);
>> +	u32 num_engines, num_slices;
>> +	int ret;
>> +
>> +	ret = kstrtou32(buff, 0, &num_engines);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Ensure number of engines specified is valid and there is an
>> +	 * exact multiple of engines for slices.
>> +	 */
>> +	num_slices = hweight32(CCS_MASK(gt));
>> +	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;
>> +	}
>> +
>> +	if (gt->ccs_mode.num_engines != num_engines) {
>> +		xe_gt_info(gt, "Setting compute mode to %d\n", num_engines);
>> +		gt->ccs_mode.num_engines = num_engines;
>> +		xe_gt_reset_async(gt);
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(ccs_mode);
>> +
>> +static void xe_gt_ccs_mode_sysfs_fini(struct drm_device *drm, void *arg)
>> +{
>> +	struct xe_gt *gt = arg;
>> +
>> +	sysfs_remove_file(gt->sysfs, &dev_attr_ccs_mode.attr);
>> +}
>
>I think there's no need to remove sysfs files.
>

I see all instances of sysfs_create_file/s have a corresponding
sysfs_remove_file/s, except in xe_tile_sysfs.c case.
So, I think it is better to keep it.

>> +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))
>> +		return 0;
>> +
>> +	err = sysfs_create_file(gt->sysfs, &dev_attr_ccs_mode.attr);
>> +	if (err)
>> +		return err;
>> +
>> +	err = drmm_add_action_or_reset(&xe->drm, xe_gt_ccs_mode_sysfs_fini, gt);
>> +	if (err)
>> +		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed, err: %d\n",
>> +			 __func__, err);
>> +
>> +	return err;
>
>Please, don't fail if sysfs fails. Make this function void.
>

The caller of this function is just printing a message upon error.
I guess we can just add the message here if sysfs_create_file fails and
make this function return type void. Will change.

Niranjana

>Andi