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