Re: [PATCH v2 2/2] drm/i915/gt: Enable only one CCS for compute workload

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

On 21/02/2024 00:14, Andi Shyti wrote:
> Hi Tvrtko,
> 
> On Tue, Feb 20, 2024 at 02:48:31PM +0000, Tvrtko Ursulin wrote:
>> On 20/02/2024 14:35, Andi Shyti wrote:
>>> Enable only one CCS engine by default with all the compute sices
>>
>> slices
> 
> Thanks!
> 
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>> index 833987015b8b..7041acc77810 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>> @@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
>>>    		if (engine->uabi_class == I915_NO_UABI_CLASS)
>>>    			continue;
>>> +		/*
>>> +		 * Do not list and do not count CCS engines other than the first
>>> +		 */
>>> +		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
>>> +		    engine->uabi_instance > 0) {
>>> +			i915->engine_uabi_class_count[engine->uabi_class]--;
>>> +			continue;
>>> +		}
>>
>> It's a bit ugly to decrement after increment, instead of somehow
>> restructuring the loop to satisfy both cases more elegantly.
> 
> yes, agree, indeed I had a hard time here to accept this change
> myself.
> 
> But moving the check above where the counter was incremented it
> would have been much uglier.
> 
> This check looks ugly everywhere you place it :-)

One idea would be to introduce a separate local counter array for 
name_instance, so not use i915->engine_uabi_class_count[]. First one 
increments for every engine, second only for the exposed ones. That way 
feels wouldn't be too ugly.

> In any case, I'm working on a patch that is splitting this
> function in two parts and there is some refactoring happening
> here (for the first initialization and the dynamic update).
> 
> Please let me know if it's OK with you or you want me to fix it
> in this run.
> 
>> And I wonder if
>> internally (in dmesg when engine name is logged) we don't end up with ccs0
>> ccs0 ccs0 ccs0.. for all instances.
> 
> I don't see this. Even in sysfs we see only one ccs. Where is it?

When you run this patch on something with two or more ccs-es, the 
"renamed ccs... to ccs.." debug logs do not all log the new name as ccs0?

Regards,

Tvrtko

> 
>>> +
>>>    		rb_link_node(&engine->uabi_node, prev, p);
>>>    		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
> 
> [...]
> 
>>> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
>>> index 3baa2f54a86e..d5a5143971f5 100644
>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>> @@ -124,6 +124,7 @@ static int query_geometry_subslices(struct drm_i915_private *i915,
>>>    	return fill_topology_info(sseu, query_item, sseu->geometry_subslice_mask);
>>>    }
>>> +
>>
>> Zap please.
> 
> yes... yes... I noticed it after sending the patch :-)
> 
> Thanks,
> Andi