Re: [PATCH v2 1/3] drm/i915/gt: Support fixed CCS mode

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

Hi Tvrtko,

[1]

> > +	/*
> > +	 * 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
> > +	 *
> > +	 * Since the number of slices and the number of engines is
> > +	 * known, and we ensure that there is an exact multiple of
> > +	 * engines for slices, the double loop becomes a loop over each
> > +	 * slice.
> > +	 */
> > +	for (i = num_slices / num_engines; i < num_slices; i++) {
> > +		struct intel_engine_cs *engine;
> > +		intel_engine_mask_t tmp;
> > +
> > +		for_each_engine_masked(engine, gt, ALL_CCS(gt), tmp) {
> > +			/* If a slice is fused off, leave disabled */
> > +			while (!(CCS_MASK(gt) & BIT(slice)))
> > +				slice++;
> > +
> > +			mode &= ~XEHP_CCS_MODE_CSLICE(slice, XEHP_CCS_MODE_CSLICE_MASK);
> > +			mode |= XEHP_CCS_MODE_CSLICE(slice, engine->instance);
> > +
> > +			/* assign the next slice */
> > +			slice++;
> > +		}
> > +	}
> > +
> > +	intel_uncore_write(gt->uncore, XEHP_CCS_MODE, mode);
> > +}
> > +
> > +void intel_gt_apply_ccs_mode(struct intel_gt *gt)
> > +{
> > +	mutex_lock(&gt->ccs.mutex);
> > +	__intel_gt_apply_ccs_mode(gt);
> > +	mutex_unlock(&gt->ccs.mutex);
> > +}
> > +
> > +void intel_gt_init_ccs_mode(struct intel_gt *gt)
> > +{
> > +	mutex_init(&gt->ccs.mutex);
> > +	gt->ccs.mode = 1;
> 
> What is '1'? And this question carries over to the sysfs interface in the
> following patch - who will use it and where it is documented how to use it?

The value '1' is explained in the comment above[1] and in the
comment below[2]. Maybe we should give it an enum meaning? But
that would be something like CCS_MODE_1/2/4, I thinks
ccs.mode = 1/2/4 is more understandable.

> Also, should this setting somehow be gated by an applicable platform? Or if
> not on setting then when acting on it in __intel_gt_apply_ccs_mode?
> 
> Creation of sysfs files as well should be gated by platform too in the
> following patch?

The idea of this series is to disable the CCS load balancing
(which automatically chooses between mode 1/2/4) and used the
a fixed scheme chosen by the user.

(I'm preparing v3 as Chris was so kind to recommend some changes
offline)

Thanks,
Andi

[2]

> > +	/*
> > +	 * Track fixed mapping between CCS engines and compute slices.
> > +	 *
> > +	 * In order to w/a HW that has the inability to dynamically load
> > +	 * balance between CCS engines and EU in the compute slices, we have to
> > +	 * reconfigure a static mapping on the fly. We track the current CCS
> > +	 * configuration (set by thr user through a sysfs interface) and compare
> > +	 * it against the current CCS_MODE (which maps CCS engines to compute
> > +	 * slices). If there is only a single engine selected, we can map it to
> > +	 * all available compute slices for maximal single task performance
> > +	 * (fast/narrow). If there are more then one engine selected, we have to
> > +	 * reduce the number of slices allocated to each engine (wide/slow),
> > +	 * fairly distributing the EU between the equivalent engines.
> > +	 */
> > +	struct {
> > +		struct mutex mutex;
> > +		u32 mode;
> > +	} ccs;