| Message ID | 20201127120718.454037-119-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DG1 + LMEM enabling | expand |
Quoting Matthew Auld (2020-11-27 12:06:34) > 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 > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_region_lmem.c | 52 ++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c > index 939cf0d195a5..eafef7034680 100644 > --- a/drivers/gpu/drm/i915/intel_region_lmem.c > +++ b/drivers/gpu/drm/i915/intel_region_lmem.c > @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915) > return mem; > } > > +static void 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; > + > + *size = SZ_1M; > + > + DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", > + *start, *start + *size); > +} > + > +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; > + > + get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size); > + reserve_start = region_start; > + reserve_end = region_start + region_size; > + > + if (!reserve_end) > + return 0; > + > + DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n", > + reserve_start, reserve_end); > + ret = i915_buddy_alloc_range(&mem->mm, &mem->reserved, > + reserve_start, > + reserve_end - reserve_start); Isn't this now relative to the stolen offset? Should this be reserved, or excluded like stolen? -Chris
On 27/11/2020 13:52, Chris Wilson wrote: > Quoting Matthew Auld (2020-11-27 12:06:34) >> 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 >> Signed-off-by: Imre Deak <imre.deak@intel.com> >> --- >> drivers/gpu/drm/i915/intel_region_lmem.c | 52 ++++++++++++++++++++++++ >> 1 file changed, 52 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c >> index 939cf0d195a5..eafef7034680 100644 >> --- a/drivers/gpu/drm/i915/intel_region_lmem.c >> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c >> @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915) >> return mem; >> } >> >> +static void 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; >> + >> + *size = SZ_1M; >> + >> + DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", >> + *start, *start + *size); >> +} >> + >> +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; >> + >> + get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size); >> + reserve_start = region_start; >> + reserve_end = region_start + region_size; >> + >> + if (!reserve_end) >> + return 0; >> + >> + DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n", >> + reserve_start, reserve_end); >> + ret = i915_buddy_alloc_range(&mem->mm, &mem->reserved, >> + reserve_start, >> + reserve_end - reserve_start); > > Isn't this now relative to the stolen offset? Should this be reserved, > or excluded like stolen? AFAIK stolen is just snipped off at the end of lmem, so I don't think it really matters if we exclude or reserve. But for this if we exclude then the region.start might have "strange" alignment, which is annoying since alloc(some_power_of_two) might not give us the expected alignment, whereas if we reserve then the allocator is aware, and so we should get the proper alignment. Maybe you have better ideas with how to handle this, but I think keeping the alignment property is nice. > -Chris >
Quoting Matthew Auld (2020-11-30 11:09:57) > On 27/11/2020 13:52, Chris Wilson wrote: > > Quoting Matthew Auld (2020-11-27 12:06:34) > >> 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 > >> Signed-off-by: Imre Deak <imre.deak@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_region_lmem.c | 52 ++++++++++++++++++++++++ > >> 1 file changed, 52 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c > >> index 939cf0d195a5..eafef7034680 100644 > >> --- a/drivers/gpu/drm/i915/intel_region_lmem.c > >> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c > >> @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915) > >> return mem; > >> } > >> > >> +static void 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; > >> + > >> + *size = SZ_1M; > >> + > >> + DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", > >> + *start, *start + *size); > >> +} > >> + > >> +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; > >> + > >> + get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size); > >> + reserve_start = region_start; > >> + reserve_end = region_start + region_size; > >> + > >> + if (!reserve_end) > >> + return 0; > >> + > >> + DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n", > >> + reserve_start, reserve_end); > >> + ret = i915_buddy_alloc_range(&mem->mm, &mem->reserved, > >> + reserve_start, > >> + reserve_end - reserve_start); > > > > Isn't this now relative to the stolen offset? Should this be reserved, > > or excluded like stolen? > > AFAIK stolen is just snipped off at the end of lmem, so I don't think it > really matters if we exclude or reserve. Right, misread, thought it was moving the start point. > But for this if we exclude then > the region.start might have "strange" alignment, which is annoying since > alloc(some_power_of_two) might not give us the expected alignment, > whereas if we reserve then the allocator is aware, and so we should get > the proper alignment. Maybe you have better ideas with how to handle > this, but I think keeping the alignment property is nice. The only tweak I would look at is making this reservation be the property of the VGA decode. But if this promises not to live into production, kiss. -Chris
diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c index 939cf0d195a5..eafef7034680 100644 --- a/drivers/gpu/drm/i915/intel_region_lmem.c +++ b/drivers/gpu/drm/i915/intel_region_lmem.c @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915) return mem; } +static void 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; + + *size = SZ_1M; + + DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n", + *start, *start + *size); +} + +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; + + get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size); + reserve_start = region_start; + reserve_end = region_start + region_size; + + if (!reserve_end) + return 0; + + DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n", + reserve_start, reserve_end); + ret = i915_buddy_alloc_range(&mem->mm, &mem->reserved, + reserve_start, + reserve_end - reserve_start); + if (ret) + DRM_ERROR("LMEM: reserving low memory region failed\n"); + + return ret; +} + static struct intel_memory_region * setup_lmem(struct drm_i915_private *dev_priv) { @@ -160,6 +202,16 @@ setup_lmem(struct drm_i915_private *dev_priv) 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_INFO("Intel graphics LMEM: %pR\n", &mem->region); DRM_INFO("Intel graphics LMEM IO start: %llx\n",