Hi Michal,
please find few late nits below
thanks for the comments!
On a multi-tile platform, each tile has its own registers + GGTT space, and BAR 0 is extended to cover all of them.
Up to four gts are supported in i915->gt[], with slot zero
s/gts/GTs (to match as below)
OK!
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 622cdfed8a8b..17927da9e23e 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -27,7 +27,8 @@ #include "shmem_utils.h" #include "pxp/intel_pxp.h"
-void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +static void +__intel_gt_init_early(struct intel_gt *gt)
no need to split line
yeah... this was a change I was always very tempted to do but decided to leave it as it is because the patch is not mine. Will do!
{ spin_lock_init(>->irq_lock);
@@ -47,19 +48,27 @@ void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) intel_rps_init_early(>->rps); }
+/* Preliminary initialization of Tile 0 */
maybe:
void intel_gts_init_early(struct drm_i915_private *i915)
We had a discussion about the use of 'gts' vs 'gt' and all the previous refactoring patches[*] where coming because the use of 'gts' brings confusion: what does gts mean? GTS or GTs? So that we decided to just use gt in its singular form and if needed, perhaps, use 'multi_gt' for plural.
The function below is indeed used only during probe so that we can remove the first parameter and have it as you suggest.
[*] /i915->gt/i915->gt0/ and /i915->gts[]/i915->gt[]/
{ struct intel_gt *gt = &i915->gt0; ...
void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) { gt->i915 = i915; gt->uncore = &i915->uncore;
- __intel_gt_init_early(gt);
}
[...]
-void intel_gt_driver_late_release(struct intel_gt *gt) +void intel_gt_driver_late_release(struct drm_i915_private *i915)
as breaks naming style maybe there should be different helper like:
void intel_gts_driver_late_release(struct drm_i915_private *i915) { struct intel_gt *gt; unsigned int id;
for_each_gt(gt, i915, id) intel_gt_driver_late_release(gt); }
then we can use "intel_gts" prefix to indicate that we want to operate on all GTs, not just single "intel_gt"
As I explained earlier, the 'gts' name brings confusion. Perhaps we can call it something like 'intel_gt_all_driver_late_release()', but it looks a bit forced.
Open for suggestions.
{
- struct intel_gt *gt;
- unsigned int id;
- /* We need to wait for inflight RCU frees to release their grip */ rcu_barrier();
- intel_uc_driver_late_release(>->uc);
- intel_gt_fini_requests(gt);
- intel_gt_fini_reset(gt);
- intel_gt_fini_timelines(gt);
- intel_engines_free(gt);
- for_each_gt(gt, i915, id) {
intel_uc_driver_late_release(>->uc);intel_gt_fini_requests(gt);intel_gt_fini_reset(gt);intel_gt_fini_timelines(gt);intel_engines_free(gt);- }
}
/** @@ -909,6 +922,112 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg) return intel_uncore_read_fw(gt->uncore, reg); }
+static int +intel_gt_tile_setup(struct intel_gt *gt, phys_addr_t phys_addr)
no need to split lines
Yep!
+{
- struct drm_i915_private *i915 = gt->i915;
can be moved to "if" below
OK
- unsigned int id = gt->info.id;
- int ret;
- if (id) {
struct intel_uncore_mmio_debug *mmio_debug;struct intel_uncore *uncore;/* For multi-tile platforms BAR0 must have at least 16MB per tile */if (GEM_WARN_ON(pci_resource_len(to_pci_dev(i915->drm.dev), 0) <(id + 1) * SZ_16M))return -EINVAL;we don't use here BAR0 so maybe we can move this check to intel_gt_probe_all() where we look for BAR phys_addr ?
OK, then I will remove it from this patch and I will add it in the next series where we add the first multitile machine support.
In intel_gt_probe_all(), right now, I don't know yet whether the platform is single tile or multitile and consequently check BAR0 or not.
- /*
* i915->gt[0] == &i915->gt0*/+#define I915_MAX_GT 4
- struct intel_gt *gt[I915_MAX_GT];
- struct { struct i915_gem_contexts { spinlock_t lock; /* locks list */
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 5625c9c38993..6a6324a08e72 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -30,6 +30,9 @@ enum intel_memory_type { enum intel_region_id { INTEL_REGION_SMEM = 0, INTEL_REGION_LMEM,
for completeness we should have:
INTEL_REGION_LMEM_0 = INTEL_REGION_LMEM,
Makes sense, fortunately this is used only 15 times, won't be as painful as it was for /i915->gt/i915->gt0/.
- INTEL_REGION_LMEM1,
- INTEL_REGION_LMEM2,
- INTEL_REGION_LMEM3,
but likely not needed any of them since all we need is:
INTEL_REGION_LMEM_n = INTEL_REGION_LMEM + I915_MAX_GT - 1,
but I'm not sure that I915_MAX_GT is available here, maybe it should defined in separate header not in i915_drv.h or we should have
I915_MAX_GT is available here, but to do something like you say we need to shift all the id's:
enum intel_region_id { INTEL_REGION_SMEM = 0, INTEL_REGION_LMEM, INTEL_REGION_STOLEN_SMEM = INTEL_REGION_LMEM + I915_MAX_GT, ... }
#define INTEL_REGION_LMEM(n) (INTEL_REGION_LMEM + I915_MAX_GT - n + 1)
Otherwise we would have some inconsistent ID.
But it doesn't look very pretty to me, though.
#define I915_MAX_LMEM 4
and then somewhere
BUILD_BUG_ON(I915_MAX_LMEM != I915_MAX_GT);
To be honest I'm not a big fan of I915_MAX_GT and when I will find some time I will try to get rid of it, as I think that the maximum number of gt's should be calculated during probe and stored somewhere in i915->max_gt, but this is a different story.
~Michal
Thanks a lot for your review!
Andi