Re: [Intel-xe] [PATCH v2 1/3] drm/xe: Enable Fixed CCS mode setting

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

Hi Niranjana,

looks good, just a few comments below.

...

> +static void __xe_gt_apply_ccs_mode(struct xe_gt *gt, u32 num_engines)
> +{
> +	u32 mode = CCS_MODE_CSLICE_0_3_MASK; /* disable all by default */
> +	struct xe_device *xe = gt_to_xe(gt);
> +	struct xe_hw_engine *hwe;
> +	int num_slices = hweight32(CCS_MASK(gt));
> +	int width, cslice;
> +	enum xe_hw_engine_id id;

please put 'id' and 'hwe' inside the for ()

> +	u32 config = 0;

this should actually be sorted in a reverse Christmas tree shape.

> +	xe_assert(xe, xe_gt_ccs_mode_enabled(gt));
> +
> +	xe_assert(xe, num_engines && num_engines <= num_slices);
> +	xe_assert(xe, !(num_slices % num_engines));
> +
> +	/*
> +	 * Loop over all available slices and assign each a user engine.
> +	 *
> +	 * With 1 engine (ccs0):
> +	 *   slice 0, 1, 2, 3: ccs0
> +	 *
> +	 * With 2 engines (ccs0, ccs1):
> +	 *   slice 0, 2: ccs0
> +	 *   slice 1, 3: ccs1
> +	 *
> +	 * With 4 engines (ccs0, ccs1, ccs2, ccs3):
> +	 *   slice 0: ccs0
> +	 *   slice 1: ccs1
> +	 *   slice 2: ccs2
> +	 *   slice 3: ccs3
> +	 */
> +	for (width = num_slices / num_engines, cslice = 0; width--;) {

why not

	for (....; width > 0; width--) {
		...
	}

as normally it's done? :-)

> +		for_each_hw_engine(hwe, gt, id) {
> +			if (hwe->class != XE_ENGINE_CLASS_COMPUTE)
> +				continue;
> +
> +			if (hwe->logical_instance >= num_engines)
> +				break;
> +
> +			config |= BIT(hwe->instance) << XE_HW_ENGINE_CCS0;
> +
> +			/* If a slice is fused off, leave disabled */
> +			while ((CCS_MASK(gt) & BIT(cslice)) == 0)
> +				cslice++;
> +
> +			mode &= ~CCS_MODE_CSLICE(cslice, CCS_MODE_CSLICE_MASK);
> +			mode |= CCS_MODE_CSLICE(cslice, hwe->instance);
> +			cslice++;
> +		}
> +	}
> +
> +	xe_mmio_write32(gt, CCS_MODE, mode);
> +
> +	xe_gt_info(gt, "CCS_MODE=%x config:%08x, num_engines:%d, num_slices:%d\n",
> +		   mode, config, num_engines, num_slices);
> +}
> +
> +void xe_gt_apply_ccs_mode(struct xe_gt *gt)
> +{
> +	if (gt->ccs_mode.num_engines)
> +		__xe_gt_apply_ccs_mode(gt, gt->ccs_mode.num_engines);
> +}

I don't see the need for this wrapper here... just:

void xe_gt_apply_ccs_mod(struct xe_gt *gt)
{
	u32 num_engines = gt->css_mode.num_engines;

	if (!num_engines)
		return;
	
	...
}

...

> +static inline bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt)
> +{
> +	/* Enable CCS mode interface on all platforms with more than one CCS engine */

This comment is misleading, since it's not doing any operation,
please, replace "Enable..." with "Check if it's enabled..."

> +	return hweight32(CCS_MASK(gt)) > 1;
> +}
> +
> +#endif

...

> +	/**
> +	 * @ccs_mode: Fixed mapping between CCS engines and compute slices.
> +	 * Through the per-gt 'ccs_mode' sysfs interface, the user can specify a
> +	 * fixed number of compute hardware engines to which the available compute
> +	 * slices are to be allocated. By default all compute slices are allocated
> +	 * to the first available compute engine instance. This user configuration
> +	 * change triggers a gt reset and it is expected that there are no open
> +	 * drm clients while doing so.
> +	 */
> +	struct {
> +		/** @num_engines: Number of CCS engines enabled */
> +		u32 num_engines;
> +	} ccs_mode;

why the need to have a struct here? It looks free complexity to
me.

Andi

> +