| Message ID | 20210412090526.30547-4-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More DG1 enabling | expand |
On 12/04/2021 10:05, Matthew Auld wrote: > From: CQ Tang <cq.tang@intel.com> > > Add "REGION_STOLEN" device info to dg1, create stolen memory > region from upper portion of local device memory, starting > from DSMBASE. > > v2: > - s/drm_info/drm_dbg; userspace likely doesn't care about stolen. > - mem->type is only setup after the region probe, so setting the name > as stolen-local or stolen-system based on this value won't work. Split > system vs local stolen setup to fix this. > - kill all the region->devmem/is_devmem stuff. We already differentiate > the different types of stolen so such things shouldn't be needed > anymore. > > Signed-off-by: CQ Tang <cq.tang@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 99 +++++++++++++++++++--- > drivers/gpu/drm/i915/gem/i915_gem_stolen.h | 3 + > drivers/gpu/drm/i915/i915_pci.c | 2 +- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_memory_region.c | 6 ++ > drivers/gpu/drm/i915/intel_memory_region.h | 5 +- > 6 files changed, 102 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > index b0597de206de..56dd58bef5ee 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > @@ -10,6 +10,7 @@ > #include <drm/drm_mm.h> > #include <drm/i915_drm.h> > > +#include "gem/i915_gem_lmem.h" > #include "gem/i915_gem_region.h" > #include "i915_drv.h" > #include "i915_gem_stolen.h" > @@ -121,6 +122,14 @@ static int i915_adjust_stolen(struct drm_i915_private *i915, > } > } > > + /* > + * With device local memory, we don't need to check the address range, > + * this is device memory physical address, could overlap with system > + * memory. > + */ > + if (HAS_LMEM(i915)) > + return 0; > + > /* > * Verify that nothing else uses this physical address. Stolen > * memory should be reserved by the BIOS and hidden from the > @@ -374,8 +383,9 @@ static void icl_get_stolen_reserved(struct drm_i915_private *i915, > } > } > > -static int i915_gem_init_stolen(struct drm_i915_private *i915) > +static int i915_gem_init_stolen(struct intel_memory_region *mem) > { > + struct drm_i915_private *i915 = mem->i915; > struct intel_uncore *uncore = &i915->uncore; > resource_size_t reserved_base, stolen_top; > resource_size_t reserved_total, reserved_size; > @@ -396,10 +406,10 @@ static int i915_gem_init_stolen(struct drm_i915_private *i915) > return 0; > } > > - if (resource_size(&intel_graphics_stolen_res) == 0) > + if (resource_size(&mem->region) == 0) > return 0; > > - i915->dsm = intel_graphics_stolen_res; > + i915->dsm = mem->region; > > if (i915_adjust_stolen(i915, &i915->dsm)) > return 0; > @@ -684,23 +694,36 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem, > return ret; > } > > +struct intel_memory_region *i915_stolen_region(struct drm_i915_private *i915) > +{ > + if (HAS_LMEM(i915)) > + return i915->mm.regions[INTEL_REGION_STOLEN_LMEM]; > + > + return i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; > +} Could be a bikeshedding comment only - especially since I think this path gets very little used at runtime so it is most likely pointless to fiddle with it, but it just strikes me a bit not fully elegant to do: i915_gem_object_create_stolen -> i915_gem_object_create_region -> i915_stolen_region And end up in here, when alternative could be at driver init: i915->stolen_region_id = HAS_LMEM() ? ... : ...; i915_gem_object_create_stolen -> i915_gem_object_create_region(i915->mm.regions[i915->stolen_region_id]); Or pointer to region. Would avoid having to export i915_stolen_region as well. Or is i915->dsm already the right thing? Because.. > + > struct drm_i915_gem_object * > i915_gem_object_create_stolen(struct drm_i915_private *i915, > resource_size_t size) > { > - return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM], > + return i915_gem_object_create_region(i915_stolen_region(i915), > size, I915_BO_ALLOC_CONTIGUOUS); > } > > static int init_stolen(struct intel_memory_region *mem) > { > - intel_memory_region_set_name(mem, "stolen"); > + if (HAS_LMEM(mem->i915)) { > + if (!io_mapping_init_wc(&mem->iomap, > + mem->io_start, > + resource_size(&mem->region))) > + return -EIO; > + } > > /* > * Initialise stolen early so that we may reserve preallocated > * objects for the BIOS to KMS transition. > */ > - return i915_gem_init_stolen(mem->i915); > + return i915_gem_init_stolen(mem); ... I find the mem region init paths a bit convoluted, stolen especially, and struggle to figure it out every time. For instance we have i915_region_stolen_ops shared between system and local stolen. But then shared vfuncs branch depending on system vs stolen? i915_gem_init_stolen is shared - but which parts of it are relevant for local stolen? > } > > static void release_stolen(struct intel_memory_region *mem) > @@ -714,13 +737,65 @@ static const struct intel_memory_region_ops i915_region_stolen_ops = { > .init_object = _i915_gem_object_stolen_init, > }; > > +static struct intel_memory_region * > +setup_lmem_stolen(struct drm_i915_private *i915) > +{ > + struct intel_uncore *uncore = &i915->uncore; > + struct pci_dev *pdev = i915->drm.pdev; > + struct intel_memory_region *mem; > + resource_size_t io_start; > + resource_size_t lmem_size; > + u64 lmem_base; > + > + if (!IS_DGFX(i915)) > + return ERR_PTR(-ENODEV); > + > + lmem_base = intel_uncore_read64(uncore, GEN12_DSMBASE); > + lmem_size = pci_resource_len(pdev, 2) - lmem_base; > + io_start = pci_resource_start(pdev, 2) + lmem_base; > + > + mem = intel_memory_region_create(i915, lmem_base, lmem_size, > + I915_GTT_PAGE_SIZE_4K, io_start, > + &i915_region_stolen_ops); > + if (IS_ERR(mem)) > + return mem; > + > + drm_dbg(&i915->drm, "Stolen Local memory: %pR\n", &mem->region); > + drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n", > + &mem->io_start); Could these messages be consolidated with the system stolen ones (i915_gem_setup_stolen?) and based off the memory_region data printed from common i915_gem_stolen_setup? > + > + intel_memory_region_set_name(mem, "stolen-local"); > + > + return mem; > +} > + > +static struct intel_memory_region* Space before asterisk. > +setup_smem_stolen(struct drm_i915_private *i915) > +{ > + struct intel_memory_region *mem; > + > + mem = intel_memory_region_create(i915, > + intel_graphics_stolen_res.start, > + resource_size(&intel_graphics_stolen_res), > + PAGE_SIZE, 0, > + &i915_region_stolen_ops); > + if (IS_ERR(mem)) > + return mem; > + > + intel_memory_region_set_name(mem, "stolen-system"); I assume this name, although changed from the current ("stolen"), is not exported anywhere to matter? > + > + return mem; > +} > + > struct intel_memory_region *i915_gem_stolen_setup(struct drm_i915_private *i915) > { > - return intel_memory_region_create(i915, > - intel_graphics_stolen_res.start, > - resource_size(&intel_graphics_stolen_res), > - PAGE_SIZE, 0, > - &i915_region_stolen_ops); > + struct intel_memory_region *mem; > + > + mem = setup_lmem_stolen(i915); > + if (mem == ERR_PTR(-ENODEV)) > + mem = setup_smem_stolen(i915); > + > + return mem; > } > > struct drm_i915_gem_object * > @@ -728,7 +803,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915, > resource_size_t stolen_offset, > resource_size_t size) > { > - struct intel_memory_region *mem = i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; > + struct intel_memory_region *mem = i915_stolen_region(i915); > struct drm_i915_gem_object *obj; > struct drm_mm_node *stolen; > int ret; > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h > index b03489706796..2d1ce7fec61c 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h > @@ -22,6 +22,9 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, > void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv, > struct drm_mm_node *node); > struct intel_memory_region *i915_gem_stolen_setup(struct drm_i915_private *i915); > + > +struct intel_memory_region *i915_stolen_region(struct drm_i915_private *i915); > + > struct drm_i915_gem_object * > i915_gem_object_create_stolen(struct drm_i915_private *dev_priv, > resource_size_t size); > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index 480553746794..53f5d1e6daef 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -906,7 +906,7 @@ static const struct intel_device_info rkl_info = { > > #define GEN12_DGFX_FEATURES \ > GEN12_FEATURES, \ > - .memory_regions = REGION_SMEM | REGION_LMEM, \ > + .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM, \ > .has_master_unit_irq = 1, \ > .has_llc = 0, \ > .has_snoop = 1, \ > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index e087bcd21911..4108f2a7ebfa 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -12191,6 +12191,7 @@ enum skl_power_gate { > #define GEN12_GLOBAL_MOCS(i) _MMIO(0x4000 + (i) * 4) /* Global MOCS regs */ > > #define GEN12_GSMBASE _MMIO(0x108100) > +#define GEN12_DSMBASE _MMIO(0x1080C0) > > /* gamt regs */ > #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4) > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c > index bf837b6bb185..ac90b76a3fa0 100644 > --- a/drivers/gpu/drm/i915/intel_memory_region.c > +++ b/drivers/gpu/drm/i915/intel_memory_region.c > @@ -22,6 +22,10 @@ static const struct { > .class = INTEL_MEMORY_STOLEN_SYSTEM, > .instance = 0, > }, > + [INTEL_REGION_STOLEN_LMEM] = { > + .class = INTEL_MEMORY_STOLEN_LOCAL, > + .instance = 0, > + }, > }; > > struct intel_memory_region * > @@ -278,6 +282,8 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915) > case INTEL_MEMORY_SYSTEM: > mem = i915_gem_shmem_setup(i915); > break; > + case INTEL_MEMORY_STOLEN_LOCAL: > + fallthrough; > case INTEL_MEMORY_STOLEN_SYSTEM: > mem = i915_gem_stolen_setup(i915); > break; > diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h > index edd49067c8ca..4c8ec15af55f 100644 > --- a/drivers/gpu/drm/i915/intel_memory_region.h > +++ b/drivers/gpu/drm/i915/intel_memory_region.h > @@ -26,18 +26,21 @@ enum intel_memory_type { > INTEL_MEMORY_SYSTEM = 0, > INTEL_MEMORY_LOCAL, > INTEL_MEMORY_STOLEN_SYSTEM, > + INTEL_MEMORY_STOLEN_LOCAL, > }; > > enum intel_region_id { > INTEL_REGION_SMEM = 0, > INTEL_REGION_LMEM, > INTEL_REGION_STOLEN_SMEM, > + INTEL_REGION_STOLEN_LMEM, > INTEL_REGION_UNKNOWN, /* Should be last */ > }; > > #define REGION_SMEM BIT(INTEL_REGION_SMEM) > #define REGION_LMEM BIT(INTEL_REGION_LMEM) > #define REGION_STOLEN_SMEM BIT(INTEL_REGION_STOLEN_SMEM) > +#define REGION_STOLEN_LMEM BIT(INTEL_REGION_STOLEN_LMEM) > > #define I915_ALLOC_MIN_PAGE_SIZE BIT(0) > #define I915_ALLOC_CONTIGUOUS BIT(1) > @@ -82,7 +85,7 @@ struct intel_memory_region { > u16 type; > u16 instance; > enum intel_region_id id; > - char name[8]; > + char name[16]; > > struct list_head reserved; > > Regards, Tvrtko
On 14/04/2021 16:01, Tvrtko Ursulin wrote: > > On 12/04/2021 10:05, Matthew Auld wrote: >> From: CQ Tang <cq.tang@intel.com> >> >> Add "REGION_STOLEN" device info to dg1, create stolen memory >> region from upper portion of local device memory, starting >> from DSMBASE. >> >> v2: >> - s/drm_info/drm_dbg; userspace likely doesn't care about stolen. >> - mem->type is only setup after the region probe, so setting the >> name >> as stolen-local or stolen-system based on this value won't >> work. Split >> system vs local stolen setup to fix this. >> - kill all the region->devmem/is_devmem stuff. We already >> differentiate >> the different types of stolen so such things shouldn't be needed >> anymore. >> >> Signed-off-by: CQ Tang <cq.tang@intel.com> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 99 +++++++++++++++++++--- >> drivers/gpu/drm/i915/gem/i915_gem_stolen.h | 3 + >> drivers/gpu/drm/i915/i915_pci.c | 2 +- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_memory_region.c | 6 ++ >> drivers/gpu/drm/i915/intel_memory_region.h | 5 +- >> 6 files changed, 102 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >> index b0597de206de..56dd58bef5ee 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >> @@ -10,6 +10,7 @@ >> #include <drm/drm_mm.h> >> #include <drm/i915_drm.h> >> +#include "gem/i915_gem_lmem.h" >> #include "gem/i915_gem_region.h" >> #include "i915_drv.h" >> #include "i915_gem_stolen.h" >> @@ -121,6 +122,14 @@ static int i915_adjust_stolen(struct >> drm_i915_private *i915, >> } >> } >> + /* >> + * With device local memory, we don't need to check the address >> range, >> + * this is device memory physical address, could overlap with system >> + * memory. >> + */ >> + if (HAS_LMEM(i915)) >> + return 0; >> + >> /* >> * Verify that nothing else uses this physical address. Stolen >> * memory should be reserved by the BIOS and hidden from the >> @@ -374,8 +383,9 @@ static void icl_get_stolen_reserved(struct >> drm_i915_private *i915, >> } >> } >> -static int i915_gem_init_stolen(struct drm_i915_private *i915) >> +static int i915_gem_init_stolen(struct intel_memory_region *mem) >> { >> + struct drm_i915_private *i915 = mem->i915; >> struct intel_uncore *uncore = &i915->uncore; >> resource_size_t reserved_base, stolen_top; >> resource_size_t reserved_total, reserved_size; >> @@ -396,10 +406,10 @@ static int i915_gem_init_stolen(struct >> drm_i915_private *i915) >> return 0; >> } >> - if (resource_size(&intel_graphics_stolen_res) == 0) >> + if (resource_size(&mem->region) == 0) >> return 0; >> - i915->dsm = intel_graphics_stolen_res; >> + i915->dsm = mem->region; >> if (i915_adjust_stolen(i915, &i915->dsm)) >> return 0; >> @@ -684,23 +694,36 @@ static int _i915_gem_object_stolen_init(struct >> intel_memory_region *mem, >> return ret; >> } >> +struct intel_memory_region *i915_stolen_region(struct >> drm_i915_private *i915) >> +{ >> + if (HAS_LMEM(i915)) >> + return i915->mm.regions[INTEL_REGION_STOLEN_LMEM]; >> + >> + return i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; >> +} > > Could be a bikeshedding comment only - especially since I think this > path gets very little used at runtime so it is most likely pointless to > fiddle with it, but it just strikes me a bit not fully elegant to do: > > i915_gem_object_create_stolen > -> i915_gem_object_create_region > -> i915_stolen_region > > And end up in here, when alternative could be at driver init: > > i915->stolen_region_id = HAS_LMEM() ? ... : ...; > > i915_gem_object_create_stolen > -> > i915_gem_object_create_region(i915->mm.regions[i915->stolen_region_id]); > > Or pointer to region. Would avoid having to export i915_stolen_region as > well. > > Or is i915->dsm already the right thing? Because.. I guess we could just have an i915->stolen_region short-cut or something? > >> + >> struct drm_i915_gem_object * >> i915_gem_object_create_stolen(struct drm_i915_private *i915, >> resource_size_t size) >> { >> - return >> i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM], >> + return i915_gem_object_create_region(i915_stolen_region(i915), >> size, I915_BO_ALLOC_CONTIGUOUS); >> } >> static int init_stolen(struct intel_memory_region *mem) >> { >> - intel_memory_region_set_name(mem, "stolen"); >> + if (HAS_LMEM(mem->i915)) { >> + if (!io_mapping_init_wc(&mem->iomap, >> + mem->io_start, >> + resource_size(&mem->region))) >> + return -EIO; >> + } >> /* >> * Initialise stolen early so that we may reserve preallocated >> * objects for the BIOS to KMS transition. >> */ >> - return i915_gem_init_stolen(mem->i915); >> + return i915_gem_init_stolen(mem); > > ... I find the mem region init paths a bit convoluted, stolen > especially, and struggle to figure it out every time. > > For instance we have i915_region_stolen_ops shared between system and > local stolen. But then shared vfuncs branch depending on system vs stolen? We could split the intel_memory_region ops? Maybe that will make it slightly less muddled? The probing is slightly different, but that's kind of expected since it's quite different from the HW pov. But once we get an intel_memory_region, it should be the same whether it's stolen device memory or whatever. > > i915_gem_init_stolen is shared - but which parts of it are relevant for > local stolen? Asking all the difficult questions :) It's just to populate dsm I think. I can rip that out and then we don't call i915_gem_init_stolen() for the stolen device memory path? Maybe that will look slightly better? > >> } >> static void release_stolen(struct intel_memory_region *mem) >> @@ -714,13 +737,65 @@ static const struct intel_memory_region_ops >> i915_region_stolen_ops = { >> .init_object = _i915_gem_object_stolen_init, >> }; >> +static struct intel_memory_region * >> +setup_lmem_stolen(struct drm_i915_private *i915) >> +{ >> + struct intel_uncore *uncore = &i915->uncore; >> + struct pci_dev *pdev = i915->drm.pdev; >> + struct intel_memory_region *mem; >> + resource_size_t io_start; >> + resource_size_t lmem_size; >> + u64 lmem_base; >> + >> + if (!IS_DGFX(i915)) >> + return ERR_PTR(-ENODEV); >> + >> + lmem_base = intel_uncore_read64(uncore, GEN12_DSMBASE); >> + lmem_size = pci_resource_len(pdev, 2) - lmem_base; >> + io_start = pci_resource_start(pdev, 2) + lmem_base; >> + >> + mem = intel_memory_region_create(i915, lmem_base, lmem_size, >> + I915_GTT_PAGE_SIZE_4K, io_start, >> + &i915_region_stolen_ops); >> + if (IS_ERR(mem)) >> + return mem; >> + >> + drm_dbg(&i915->drm, "Stolen Local memory: %pR\n", &mem->region); >> + drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n", >> + &mem->io_start); > > Could these messages be consolidated with the system stolen ones > (i915_gem_setup_stolen?) and based off the memory_region data printed > from common i915_gem_stolen_setup? > >> + >> + intel_memory_region_set_name(mem, "stolen-local"); >> + >> + return mem; >> +} >> + >> +static struct intel_memory_region* > > Space before asterisk. > >> +setup_smem_stolen(struct drm_i915_private *i915) >> +{ >> + struct intel_memory_region *mem; >> + >> + mem = intel_memory_region_create(i915, >> + intel_graphics_stolen_res.start, >> + resource_size(&intel_graphics_stolen_res), >> + PAGE_SIZE, 0, >> + &i915_region_stolen_ops); >> + if (IS_ERR(mem)) >> + return mem; >> + >> + intel_memory_region_set_name(mem, "stolen-system"); > > I assume this name, although changed from the current ("stolen"), is not > exported anywhere to matter? Yeah, it's just for internal use, and some debugfs. > >> + >> + return mem; >> +} >> + >> struct intel_memory_region *i915_gem_stolen_setup(struct >> drm_i915_private *i915) >> { >> - return intel_memory_region_create(i915, >> - intel_graphics_stolen_res.start, >> - resource_size(&intel_graphics_stolen_res), >> - PAGE_SIZE, 0, >> - &i915_region_stolen_ops); >> + struct intel_memory_region *mem; >> + >> + mem = setup_lmem_stolen(i915); >> + if (mem == ERR_PTR(-ENODEV)) >> + mem = setup_smem_stolen(i915); >> + >> + return mem; >> } >> struct drm_i915_gem_object * >> @@ -728,7 +803,7 @@ >> i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private >> *i915, >> resource_size_t stolen_offset, >> resource_size_t size) >> { >> - struct intel_memory_region *mem = >> i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; >> + struct intel_memory_region *mem = i915_stolen_region(i915); >> struct drm_i915_gem_object *obj; >> struct drm_mm_node *stolen; >> int ret; >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h >> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h >> index b03489706796..2d1ce7fec61c 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h >> @@ -22,6 +22,9 @@ int i915_gem_stolen_insert_node_in_range(struct >> drm_i915_private *dev_priv, >> void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv, >> struct drm_mm_node *node); >> struct intel_memory_region *i915_gem_stolen_setup(struct >> drm_i915_private *i915); >> + >> +struct intel_memory_region *i915_stolen_region(struct >> drm_i915_private *i915); >> + >> struct drm_i915_gem_object * >> i915_gem_object_create_stolen(struct drm_i915_private *dev_priv, >> resource_size_t size); >> diff --git a/drivers/gpu/drm/i915/i915_pci.c >> b/drivers/gpu/drm/i915/i915_pci.c >> index 480553746794..53f5d1e6daef 100644 >> --- a/drivers/gpu/drm/i915/i915_pci.c >> +++ b/drivers/gpu/drm/i915/i915_pci.c >> @@ -906,7 +906,7 @@ static const struct intel_device_info rkl_info = { >> #define GEN12_DGFX_FEATURES \ >> GEN12_FEATURES, \ >> - .memory_regions = REGION_SMEM | REGION_LMEM, \ >> + .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM, \ >> .has_master_unit_irq = 1, \ >> .has_llc = 0, \ >> .has_snoop = 1, \ >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> index e087bcd21911..4108f2a7ebfa 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -12191,6 +12191,7 @@ enum skl_power_gate { >> #define GEN12_GLOBAL_MOCS(i) _MMIO(0x4000 + (i) * 4) /* Global >> MOCS regs */ >> #define GEN12_GSMBASE _MMIO(0x108100) >> +#define GEN12_DSMBASE _MMIO(0x1080C0) >> /* gamt regs */ >> #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4) >> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c >> b/drivers/gpu/drm/i915/intel_memory_region.c >> index bf837b6bb185..ac90b76a3fa0 100644 >> --- a/drivers/gpu/drm/i915/intel_memory_region.c >> +++ b/drivers/gpu/drm/i915/intel_memory_region.c >> @@ -22,6 +22,10 @@ static const struct { >> .class = INTEL_MEMORY_STOLEN_SYSTEM, >> .instance = 0, >> }, >> + [INTEL_REGION_STOLEN_LMEM] = { >> + .class = INTEL_MEMORY_STOLEN_LOCAL, >> + .instance = 0, >> + }, >> }; >> struct intel_memory_region * >> @@ -278,6 +282,8 @@ int intel_memory_regions_hw_probe(struct >> drm_i915_private *i915) >> case INTEL_MEMORY_SYSTEM: >> mem = i915_gem_shmem_setup(i915); >> break; >> + case INTEL_MEMORY_STOLEN_LOCAL: >> + fallthrough; >> case INTEL_MEMORY_STOLEN_SYSTEM: >> mem = i915_gem_stolen_setup(i915); >> break; >> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h >> b/drivers/gpu/drm/i915/intel_memory_region.h >> index edd49067c8ca..4c8ec15af55f 100644 >> --- a/drivers/gpu/drm/i915/intel_memory_region.h >> +++ b/drivers/gpu/drm/i915/intel_memory_region.h >> @@ -26,18 +26,21 @@ enum intel_memory_type { >> INTEL_MEMORY_SYSTEM = 0, >> INTEL_MEMORY_LOCAL, >> INTEL_MEMORY_STOLEN_SYSTEM, >> + INTEL_MEMORY_STOLEN_LOCAL, >> }; >> enum intel_region_id { >> INTEL_REGION_SMEM = 0, >> INTEL_REGION_LMEM, >> INTEL_REGION_STOLEN_SMEM, >> + INTEL_REGION_STOLEN_LMEM, >> INTEL_REGION_UNKNOWN, /* Should be last */ >> }; >> #define REGION_SMEM BIT(INTEL_REGION_SMEM) >> #define REGION_LMEM BIT(INTEL_REGION_LMEM) >> #define REGION_STOLEN_SMEM BIT(INTEL_REGION_STOLEN_SMEM) >> +#define REGION_STOLEN_LMEM BIT(INTEL_REGION_STOLEN_LMEM) >> #define I915_ALLOC_MIN_PAGE_SIZE BIT(0) >> #define I915_ALLOC_CONTIGUOUS BIT(1) >> @@ -82,7 +85,7 @@ struct intel_memory_region { >> u16 type; >> u16 instance; >> enum intel_region_id id; >> - char name[8]; >> + char name[16]; >> struct list_head reserved; >> > > Regards, > > Tvrtko
On 16/04/2021 16:04, Matthew Auld wrote: > On 14/04/2021 16:01, Tvrtko Ursulin wrote: >> >> On 12/04/2021 10:05, Matthew Auld wrote: >>> From: CQ Tang <cq.tang@intel.com> >>> >>> Add "REGION_STOLEN" device info to dg1, create stolen memory >>> region from upper portion of local device memory, starting >>> from DSMBASE. >>> >>> v2: >>> - s/drm_info/drm_dbg; userspace likely doesn't care about stolen. >>> - mem->type is only setup after the region probe, so setting the >>> name >>> as stolen-local or stolen-system based on this value won't >>> work. Split >>> system vs local stolen setup to fix this. >>> - kill all the region->devmem/is_devmem stuff. We already >>> differentiate >>> the different types of stolen so such things shouldn't be needed >>> anymore. >>> >>> Signed-off-by: CQ Tang <cq.tang@intel.com> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 99 +++++++++++++++++++--- >>> drivers/gpu/drm/i915/gem/i915_gem_stolen.h | 3 + >>> drivers/gpu/drm/i915/i915_pci.c | 2 +- >>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>> drivers/gpu/drm/i915/intel_memory_region.c | 6 ++ >>> drivers/gpu/drm/i915/intel_memory_region.h | 5 +- >>> 6 files changed, 102 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >>> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >>> index b0597de206de..56dd58bef5ee 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c >>> @@ -10,6 +10,7 @@ >>> #include <drm/drm_mm.h> >>> #include <drm/i915_drm.h> >>> +#include "gem/i915_gem_lmem.h" >>> #include "gem/i915_gem_region.h" >>> #include "i915_drv.h" >>> #include "i915_gem_stolen.h" >>> @@ -121,6 +122,14 @@ static int i915_adjust_stolen(struct >>> drm_i915_private *i915, >>> } >>> } >>> + /* >>> + * With device local memory, we don't need to check the address >>> range, >>> + * this is device memory physical address, could overlap with >>> system >>> + * memory. >>> + */ >>> + if (HAS_LMEM(i915)) >>> + return 0; >>> + >>> /* >>> * Verify that nothing else uses this physical address. Stolen >>> * memory should be reserved by the BIOS and hidden from the >>> @@ -374,8 +383,9 @@ static void icl_get_stolen_reserved(struct >>> drm_i915_private *i915, >>> } >>> } >>> -static int i915_gem_init_stolen(struct drm_i915_private *i915) >>> +static int i915_gem_init_stolen(struct intel_memory_region *mem) >>> { >>> + struct drm_i915_private *i915 = mem->i915; >>> struct intel_uncore *uncore = &i915->uncore; >>> resource_size_t reserved_base, stolen_top; >>> resource_size_t reserved_total, reserved_size; >>> @@ -396,10 +406,10 @@ static int i915_gem_init_stolen(struct >>> drm_i915_private *i915) >>> return 0; >>> } >>> - if (resource_size(&intel_graphics_stolen_res) == 0) >>> + if (resource_size(&mem->region) == 0) >>> return 0; >>> - i915->dsm = intel_graphics_stolen_res; >>> + i915->dsm = mem->region; >>> if (i915_adjust_stolen(i915, &i915->dsm)) >>> return 0; >>> @@ -684,23 +694,36 @@ static int _i915_gem_object_stolen_init(struct >>> intel_memory_region *mem, >>> return ret; >>> } >>> +struct intel_memory_region *i915_stolen_region(struct >>> drm_i915_private *i915) >>> +{ >>> + if (HAS_LMEM(i915)) >>> + return i915->mm.regions[INTEL_REGION_STOLEN_LMEM]; >>> + >>> + return i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; >>> +} >> >> Could be a bikeshedding comment only - especially since I think this >> path gets very little used at runtime so it is most likely pointless >> to fiddle with it, but it just strikes me a bit not fully elegant to do: >> >> i915_gem_object_create_stolen >> -> i915_gem_object_create_region >> -> i915_stolen_region >> >> And end up in here, when alternative could be at driver init: >> >> i915->stolen_region_id = HAS_LMEM() ? ... : ...; >> >> i915_gem_object_create_stolen >> -> >> i915_gem_object_create_region(i915->mm.regions[i915->stolen_region_id]); >> >> Or pointer to region. Would avoid having to export i915_stolen_region >> as well. >> >> Or is i915->dsm already the right thing? Because.. > > I guess we could just have an i915->stolen_region short-cut or something? i915->dsm is not it? Where does i915_gem_init_stolen exists for local-stolen then? At the "resource_size(&mem->region) == 0" check? > >> >>> + >>> struct drm_i915_gem_object * >>> i915_gem_object_create_stolen(struct drm_i915_private *i915, >>> resource_size_t size) >>> { >>> - return >>> i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM], >>> >>> + return i915_gem_object_create_region(i915_stolen_region(i915), >>> size, I915_BO_ALLOC_CONTIGUOUS); >>> } >>> static int init_stolen(struct intel_memory_region *mem) >>> { >>> - intel_memory_region_set_name(mem, "stolen"); >>> + if (HAS_LMEM(mem->i915)) { >>> + if (!io_mapping_init_wc(&mem->iomap, >>> + mem->io_start, >>> + resource_size(&mem->region))) >>> + return -EIO; >>> + } >>> /* >>> * Initialise stolen early so that we may reserve preallocated >>> * objects for the BIOS to KMS transition. >>> */ >>> - return i915_gem_init_stolen(mem->i915); >>> + return i915_gem_init_stolen(mem); >> >> ... I find the mem region init paths a bit convoluted, stolen >> especially, and struggle to figure it out every time. >> >> For instance we have i915_region_stolen_ops shared between system and >> local stolen. But then shared vfuncs branch depending on system vs >> stolen? > > We could split the intel_memory_region ops? Maybe that will make it > slightly less muddled? I think so. Each vfunc table with it's own ->init() should make it easier to follow. > The probing is slightly different, but that's kind of expected since > it's quite different from the HW pov. > > But once we get an intel_memory_region, it should be the same whether > it's stolen device memory or whatever. > >> >> i915_gem_init_stolen is shared - but which parts of it are relevant >> for local stolen? > > Asking all the difficult questions :) > > It's just to populate dsm I think. I can rip that out and then we don't > call i915_gem_init_stolen() for the stolen device memory path? Maybe > that will look slightly better? Yes, with the above approach of two struct intel_memory_region_ops? Even if some vfuncs are shared it should be better. I am also confused by ->release ie. i915_gem_cleanup_stolen. How does that work for two stolen regions, I mean one i915->mm.stolen? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index b0597de206de..56dd58bef5ee 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -10,6 +10,7 @@ #include <drm/drm_mm.h> #include <drm/i915_drm.h> +#include "gem/i915_gem_lmem.h" #include "gem/i915_gem_region.h" #include "i915_drv.h" #include "i915_gem_stolen.h" @@ -121,6 +122,14 @@ static int i915_adjust_stolen(struct drm_i915_private *i915, } } + /* + * With device local memory, we don't need to check the address range, + * this is device memory physical address, could overlap with system + * memory. + */ + if (HAS_LMEM(i915)) + return 0; + /* * Verify that nothing else uses this physical address. Stolen * memory should be reserved by the BIOS and hidden from the @@ -374,8 +383,9 @@ static void icl_get_stolen_reserved(struct drm_i915_private *i915, } } -static int i915_gem_init_stolen(struct drm_i915_private *i915) +static int i915_gem_init_stolen(struct intel_memory_region *mem) { + struct drm_i915_private *i915 = mem->i915; struct intel_uncore *uncore = &i915->uncore; resource_size_t reserved_base, stolen_top; resource_size_t reserved_total, reserved_size; @@ -396,10 +406,10 @@ static int i915_gem_init_stolen(struct drm_i915_private *i915) return 0; } - if (resource_size(&intel_graphics_stolen_res) == 0) + if (resource_size(&mem->region) == 0) return 0; - i915->dsm = intel_graphics_stolen_res; + i915->dsm = mem->region; if (i915_adjust_stolen(i915, &i915->dsm)) return 0; @@ -684,23 +694,36 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem, return ret; } +struct intel_memory_region *i915_stolen_region(struct drm_i915_private *i915) +{ + if (HAS_LMEM(i915)) + return i915->mm.regions[INTEL_REGION_STOLEN_LMEM]; + + return i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; +} + struct drm_i915_gem_object * i915_gem_object_create_stolen(struct drm_i915_private *i915, resource_size_t size) { - return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM], + return i915_gem_object_create_region(i915_stolen_region(i915), size, I915_BO_ALLOC_CONTIGUOUS); } static int init_stolen(struct intel_memory_region *mem) { - intel_memory_region_set_name(mem, "stolen"); + if (HAS_LMEM(mem->i915)) { + if (!io_mapping_init_wc(&mem->iomap, + mem->io_start, + resource_size(&mem->region))) + return -EIO; + } /* * Initialise stolen early so that we may reserve preallocated * objects for the BIOS to KMS transition. */ - return i915_gem_init_stolen(mem->i915); + return i915_gem_init_stolen(mem); } static void release_stolen(struct intel_memory_region *mem) @@ -714,13 +737,65 @@ static const struct intel_memory_region_ops i915_region_stolen_ops = { .init_object = _i915_gem_object_stolen_init, }; +static struct intel_memory_region * +setup_lmem_stolen(struct drm_i915_private *i915) +{ + struct intel_uncore *uncore = &i915->uncore; + struct pci_dev *pdev = i915->drm.pdev; + struct intel_memory_region *mem; + resource_size_t io_start; + resource_size_t lmem_size; + u64 lmem_base; + + if (!IS_DGFX(i915)) + return ERR_PTR(-ENODEV); + + lmem_base = intel_uncore_read64(uncore, GEN12_DSMBASE); + lmem_size = pci_resource_len(pdev, 2) - lmem_base; + io_start = pci_resource_start(pdev, 2) + lmem_base; + + mem = intel_memory_region_create(i915, lmem_base, lmem_size, + I915_GTT_PAGE_SIZE_4K, io_start, + &i915_region_stolen_ops); + if (IS_ERR(mem)) + return mem; + + drm_dbg(&i915->drm, "Stolen Local memory: %pR\n", &mem->region); + drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n", + &mem->io_start); + + intel_memory_region_set_name(mem, "stolen-local"); + + return mem; +} + +static struct intel_memory_region* +setup_smem_stolen(struct drm_i915_private *i915) +{ + struct intel_memory_region *mem; + + mem = intel_memory_region_create(i915, + intel_graphics_stolen_res.start, + resource_size(&intel_graphics_stolen_res), + PAGE_SIZE, 0, + &i915_region_stolen_ops); + if (IS_ERR(mem)) + return mem; + + intel_memory_region_set_name(mem, "stolen-system"); + + return mem; +} + struct intel_memory_region *i915_gem_stolen_setup(struct drm_i915_private *i915) { - return intel_memory_region_create(i915, - intel_graphics_stolen_res.start, - resource_size(&intel_graphics_stolen_res), - PAGE_SIZE, 0, - &i915_region_stolen_ops); + struct intel_memory_region *mem; + + mem = setup_lmem_stolen(i915); + if (mem == ERR_PTR(-ENODEV)) + mem = setup_smem_stolen(i915); + + return mem; } struct drm_i915_gem_object * @@ -728,7 +803,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915, resource_size_t stolen_offset, resource_size_t size) { - struct intel_memory_region *mem = i915->mm.regions[INTEL_REGION_STOLEN_SMEM]; + struct intel_memory_region *mem = i915_stolen_region(i915); struct drm_i915_gem_object *obj; struct drm_mm_node *stolen; int ret; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h index b03489706796..2d1ce7fec61c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h @@ -22,6 +22,9 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv, struct drm_mm_node *node); struct intel_memory_region *i915_gem_stolen_setup(struct drm_i915_private *i915); + +struct intel_memory_region *i915_stolen_region(struct drm_i915_private *i915); + struct drm_i915_gem_object * i915_gem_object_create_stolen(struct drm_i915_private *dev_priv, resource_size_t size); diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 480553746794..53f5d1e6daef 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -906,7 +906,7 @@ static const struct intel_device_info rkl_info = { #define GEN12_DGFX_FEATURES \ GEN12_FEATURES, \ - .memory_regions = REGION_SMEM | REGION_LMEM, \ + .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM, \ .has_master_unit_irq = 1, \ .has_llc = 0, \ .has_snoop = 1, \ diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index e087bcd21911..4108f2a7ebfa 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -12191,6 +12191,7 @@ enum skl_power_gate { #define GEN12_GLOBAL_MOCS(i) _MMIO(0x4000 + (i) * 4) /* Global MOCS regs */ #define GEN12_GSMBASE _MMIO(0x108100) +#define GEN12_DSMBASE _MMIO(0x1080C0) /* gamt regs */ #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4) diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c index bf837b6bb185..ac90b76a3fa0 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.c +++ b/drivers/gpu/drm/i915/intel_memory_region.c @@ -22,6 +22,10 @@ static const struct { .class = INTEL_MEMORY_STOLEN_SYSTEM, .instance = 0, }, + [INTEL_REGION_STOLEN_LMEM] = { + .class = INTEL_MEMORY_STOLEN_LOCAL, + .instance = 0, + }, }; struct intel_memory_region * @@ -278,6 +282,8 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915) case INTEL_MEMORY_SYSTEM: mem = i915_gem_shmem_setup(i915); break; + case INTEL_MEMORY_STOLEN_LOCAL: + fallthrough; case INTEL_MEMORY_STOLEN_SYSTEM: mem = i915_gem_stolen_setup(i915); break; diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index edd49067c8ca..4c8ec15af55f 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -26,18 +26,21 @@ enum intel_memory_type { INTEL_MEMORY_SYSTEM = 0, INTEL_MEMORY_LOCAL, INTEL_MEMORY_STOLEN_SYSTEM, + INTEL_MEMORY_STOLEN_LOCAL, }; enum intel_region_id { INTEL_REGION_SMEM = 0, INTEL_REGION_LMEM, INTEL_REGION_STOLEN_SMEM, + INTEL_REGION_STOLEN_LMEM, INTEL_REGION_UNKNOWN, /* Should be last */ }; #define REGION_SMEM BIT(INTEL_REGION_SMEM) #define REGION_LMEM BIT(INTEL_REGION_LMEM) #define REGION_STOLEN_SMEM BIT(INTEL_REGION_STOLEN_SMEM) +#define REGION_STOLEN_LMEM BIT(INTEL_REGION_STOLEN_LMEM) #define I915_ALLOC_MIN_PAGE_SIZE BIT(0) #define I915_ALLOC_CONTIGUOUS BIT(1) @@ -82,7 +85,7 @@ struct intel_memory_region { u16 type; u16 instance; enum intel_region_id id; - char name[8]; + char name[16]; struct list_head reserved;