| Message ID | 20210127120316.370305-5-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/8] drm/i915: make local-memory probing a GT operation | expand |
Quoting Matthew Auld (2021-01-27 12:03:13) > From: Imre Deak <imre.deak@intel.com> > > On DG1 A0/B0 steppings the first 1MB of local memory must be reserved. > One reason for this is that the 0xA0000-0xB0000 range is not accessible > by the display, probably since this region is redirected to another > memory location for legacy VGA compatibility. > > BSpec: 50586 > Testcase: igt/kms_big_fb/linear-64bpp-rotate-0 > > v2: > - Reserve the memory on B0 as well. > > v3: replace DRM_DEBUG/DRM_ERROR with drm_dbg/drm_err > > Signed-off-by: Imre Deak <imre.deak@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_region_lmem.c | 56 +++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c > index 71bb38706dbf..f5c12cbbaa86 100644 > --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c > +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c > @@ -143,6 +143,52 @@ intel_gt_setup_fake_lmem(struct intel_gt *gt) > return mem; > } > > +static bool get_legacy_lowmem_region(struct intel_uncore *uncore, > + u64 *start, u32 *size) > +{ > + *start = 0; > + *size = 0; Redundant now with the return indicating not to trust the values. > + > + if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0)) > + return false; > + > + *size = SZ_1M; > + > + drm_dbg(&uncore->i915->drm, "LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", > + *start, *start + *size); > + > + return true; > +} > + > +static int reserve_lowmem_region(struct intel_uncore *uncore, > + struct intel_memory_region *mem) > +{ > + u64 reserve_start; > + u64 reserve_end; > + u64 region_start; > + u32 region_size; > + int ret; > + > + if (!get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size)) > + return 0; > + > + reserve_start = region_start; > + reserve_end = region_start + region_size; > + > + if (!reserve_end) > + return 0; > + > + drm_dbg(&uncore->i915->drm, "LMEM: reserving low-memory region [0x%llx-0x%llx]\n", > + reserve_start, reserve_end); > + ret = intel_memory_region_reserve(mem, > + reserve_start, > + reserve_end - reserve_start); You are doing this on purpose! u32 start, u64 size -> u64 start, end and then back to u64 start, size The two drm_dbg() are functionally identical (other than giving a telltale for the reserve_end overflow escape). -Chris
Quoting Chris Wilson (2021-01-27 12:19:38) > Quoting Matthew Auld (2021-01-27 12:03:13) > > From: Imre Deak <imre.deak@intel.com> > > > > On DG1 A0/B0 steppings the first 1MB of local memory must be reserved. > > One reason for this is that the 0xA0000-0xB0000 range is not accessible > > by the display, probably since this region is redirected to another > > memory location for legacy VGA compatibility. > > > > BSpec: 50586 > > Testcase: igt/kms_big_fb/linear-64bpp-rotate-0 > > > > v2: > > - Reserve the memory on B0 as well. > > > > v3: replace DRM_DEBUG/DRM_ERROR with drm_dbg/drm_err > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_region_lmem.c | 56 +++++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c > > index 71bb38706dbf..f5c12cbbaa86 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c > > +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c > > @@ -143,6 +143,52 @@ intel_gt_setup_fake_lmem(struct intel_gt *gt) > > return mem; > > } > > > > +static bool get_legacy_lowmem_region(struct intel_uncore *uncore, > > + u64 *start, u32 *size) > > +{ > > + *start = 0; > > + *size = 0; > > Redundant now with the return indicating not to trust the values. > > > + > > + if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0)) > > + return false; > > + > > + *size = SZ_1M; > > + > > + drm_dbg(&uncore->i915->drm, "LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", > > + *start, *start + *size); > > + > > + return true; > > +} > > + > > +static int reserve_lowmem_region(struct intel_uncore *uncore, > > + struct intel_memory_region *mem) > > +{ > > + u64 reserve_start; > > + u64 reserve_end; > > + u64 region_start; > > + u32 region_size; > > + int ret; > > + > > + if (!get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size)) > > + return 0; > > + > > + reserve_start = region_start; > > + reserve_end = region_start + region_size; > > + > > + if (!reserve_end) > > + return 0; > > + > > + drm_dbg(&uncore->i915->drm, "LMEM: reserving low-memory region [0x%llx-0x%llx]\n", > > + reserve_start, reserve_end); > > + ret = intel_memory_region_reserve(mem, > > + reserve_start, > > + reserve_end - reserve_start); > > You are doing this on purpose! > u32 start, u64 size -> u64 start, end and then back to u64 start, size > > The two drm_dbg() are functionally identical (other than giving a > telltale for the reserve_end overflow escape). So other than this patch, the series is Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> I've a question for Tvrtko about how we want the driver_probe callchain to look, but that's a minor nit. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c index 71bb38706dbf..f5c12cbbaa86 100644 --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c @@ -143,6 +143,52 @@ intel_gt_setup_fake_lmem(struct intel_gt *gt) return mem; } +static bool get_legacy_lowmem_region(struct intel_uncore *uncore, + u64 *start, u32 *size) +{ + *start = 0; + *size = 0; + + if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0)) + return false; + + *size = SZ_1M; + + drm_dbg(&uncore->i915->drm, "LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", + *start, *start + *size); + + return true; +} + +static int reserve_lowmem_region(struct intel_uncore *uncore, + struct intel_memory_region *mem) +{ + u64 reserve_start; + u64 reserve_end; + u64 region_start; + u32 region_size; + int ret; + + if (!get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size)) + return 0; + + reserve_start = region_start; + reserve_end = region_start + region_size; + + if (!reserve_end) + return 0; + + drm_dbg(&uncore->i915->drm, "LMEM: reserving low-memory region [0x%llx-0x%llx]\n", + reserve_start, reserve_end); + ret = intel_memory_region_reserve(mem, + reserve_start, + reserve_end - reserve_start); + if (ret) + drm_err(&uncore->i915->drm, "LMEM: reserving low memory region failed\n"); + + return ret; +} + static struct intel_memory_region *setup_lmem(struct intel_gt *gt) { struct drm_i915_private *i915 = gt->i915; @@ -167,6 +213,16 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) I915_GTT_PAGE_SIZE_4K, io_start, &intel_region_lmem_ops); + if (!IS_ERR(mem)) { + int err; + + err = reserve_lowmem_region(uncore, mem); + if (err) { + intel_memory_region_put(mem); + return ERR_PTR(err); + } + } + if (!IS_ERR(mem)) { drm_dbg(&i915->drm, "Local memory: %pR\n", &mem->region); drm_dbg(&i915->drm, "Local memory IO start: %pa\n",