Re: [Intel-gfx] [PATCH v3 05/10] drm/i915: Prepare for multiple gts

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

On 01/11/2021 23:11, Andi Shyti wrote:
> Hi Matt and Tvrtko,
> 
> [...]
> 
>>   static int
>>   intel_gt_tile_setup(struct intel_gt *gt, unsigned int id, phys_addr_t phys_addr)
> 
> we don't actually need 'id', it's gt->info.id. It's introduced in
> patch 3 with the value '0' but it's not needed.

I have a suspicion code got munged up over endless rebases and refactors.

This patch is the one which introduces the id member to gt->info. But it is not setting it, even though I suspect the intent was for intel_gt_tile_setup to do that.

Instead gt->info.id is only set to a valid value in last patch of this series inside intel_gt_probe_all:

+		gt->i915 = i915;
+		gt->name = gtdef->name;
+		gt->type = gtdef->type;
+		gt->info.engine_mask = gtdef->engine_mask;
+		gt->info.id = i;
+
+		drm_dbg(&i915->drm, "Setting up %s %u\n", gt->name, gt->info.id);
+		ret = intel_gt_tile_setup(gt, i, phys_addr + gtdef->mapping_base);

And intel_gt_tile_setup then calls __intel_gt_init_early which assigns gt->i915 yet again.

So I'd say there is probably space to bring this all into a more streamlined flow, even more than what you suggest below.

Regards,

Tvrtko
  
>>   {
>> +	struct drm_i915_private *i915 = gt->i915;
>> +	struct intel_uncore *uncore;
>> +	struct intel_uncore_mmio_debug *mmio_debug;
>>   	int ret;
>>   
>> -	intel_uncore_init_early(gt->uncore, gt);
>> +	if (id) {
> 
> if (gt->info.id) ?
> 
> Andi
> 
>> +		uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);
>> +		if (!uncore)
>> +			return -ENOMEM;
>> +
>> +		mmio_debug = kzalloc(sizeof(*mmio_debug), GFP_KERNEL);
>> +		if (!mmio_debug) {
>> +			kfree(uncore);
>> +			return -ENOMEM;
>> +		}
>> +
>> +		__intel_gt_init_early(gt, uncore, i915);
>> +	} else {
>> +		uncore = &i915->uncore;
>> +		mmio_debug = &i915->mmio_debug;
>> +	}
>> +
>> +	intel_uncore_init_early(uncore, gt);
>>   
>>   	ret = intel_uncore_setup_mmio(gt->uncore, phys_addr);