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@xxxxxxxxx> 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. Splitsystem vs local stolen setup to fix this.- kill all the region->devmem/is_devmem stuff. We already differentiatethe different types of stolen so such things shouldn't be needed anymore. Signed-off-by: CQ Tang <cq.tang@xxxxxxxxx> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx> --- 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.cindex 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
