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

[Andi Shyti <andi.shyti@linux.intel.com>]

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))

> +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.

> +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.

Andi