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

[Niranjana Vishwanathapura]

On Fri, Dec 01, 2023 at 09:52:28PM +0100, Andi Shyti wrote:
>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 ()
>

Ok

>> +	u32 config = 0;
>
>this should actually be sorted in a reverse Christmas tree shape.
>

Ok

>> +	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? :-)
>

Ok

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

Ok

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

Ok, will simplify it.

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

Earlier version of the patch series has other fields in this stucture.
Ok, will simple define it as,
u32 ccs_mode;

with definition,
@ccs_mode: Number of compute engines to be enabled

Thanks,
Niranjana

>Andi
>
>> +