Re: [Intel-gfx] [PATCH v3 05/10] drm/i915: Prepare for multiple gts
Hi 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(>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. > > > > yes, I noticed them! > > > > Patch 3, 5 and 10 are very much connected with each other: 3 > > prepares for one tile, 5 prepares for multitile and 10 does the > > multitile. While in between other patches are doing other things. > > > > On top of some cleanups we could also rearrange the patches with > > some squashing and reordering to have them a bit more linear and > > also easier to review. > > Yes. Maybe make intel_gt_tile_setup accept more arguments so it can be truly > used to setup a gt? > > intel_gt_tile_setup(gt, id, name, type, engine_mask) > > The usual thing where patch which adds something extends the prototype to > include more stuff. If that applies here. > > I know it is originally my patch but I don't have the time to rework it, > much less the whole series, so usual dispensation to take over authorship if > changes are large applies. as no one is stepping forward, if you and Matt are OK, I can try to venture in some refactoring of these three patches (3, 5 and 10). Andi
Re: [Intel-gfx] [PATCH v3 05/10] drm/i915: Prepare for multiple gts
On 02/11/2021 11:26, Andi Shyti wrote: Hi 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(>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. yes, I noticed them! Patch 3, 5 and 10 are very much connected with each other: 3 prepares for one tile, 5 prepares for multitile and 10 does the multitile. While in between other patches are doing other things. On top of some cleanups we could also rearrange the patches with some squashing and reordering to have them a bit more linear and also easier to review. Yes. Maybe make intel_gt_tile_setup accept more arguments so it can be truly used to setup a gt? intel_gt_tile_setup(gt, id, name, type, engine_mask) The usual thing where patch which adds something extends the prototype to include more stuff. If that applies here. I know it is originally my patch but I don't have the time to rework it, much less the whole series, so usual dispensation to take over authorship if changes are large applies. Regards, Tvrtko
Re: [Intel-gfx] [PATCH v3 05/10] drm/i915: Prepare for multiple gts
Hi 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(>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. yes, I noticed them! Patch 3, 5 and 10 are very much connected with each other: 3 prepares for one tile, 5 prepares for multitile and 10 does the multitile. While in between other patches are doing other things. On top of some cleanups we could also rearrange the patches with some squashing and reordering to have them a bit more linear and also easier to review. Andi
Re: [Intel-gfx] [PATCH v3 05/10] drm/i915: Prepare for multiple gts
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(>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 = >uncore;
+ mmio_debug = >mmio_debug;
+ }
+
+ intel_uncore_init_early(uncore, gt);
ret = intel_uncore_setup_mmio(gt->uncore, phys_addr);
