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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 04/01/2024 21:23, Andi Shyti wrote:
> 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

Do you mean this is mode '1':

  * With 1 engine (ccs0):
  *   slice 0, 1, 2, 3: ccs0

?

But I don't see where it says what do different modes mean on different 
SKU configurations.

It also does not say what should the num_slices sysfs file be used for.

Does "mode N" mean "assign each command streamer N compute slices"? Or 
"assign each compute slice N command streamers"?

I wonder if we should add something user friendly into 
Documentation/ABI/*/sysfs-... Joonas your thoughts?

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

Okay lets wait for v2 and I will then see if I will this that will make 
it clearer to casual observers.

Regards,

Tvrtko


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