[PATCH 1/5] drm/i915: Create stolen memory region from local memory
Matthew Auld
matthew.auld at intel.com
Wed Apr 21 09:46:00 UTC 2021
On 20/04/2021 17:06, Tvrtko Ursulin wrote:
>
> On 20/04/2021 14:18, Matthew Auld wrote:
>> From: CQ Tang <cq.tang at 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.
>> v3:
>> - split stolen lmem vs smem ops(Tvrtko)
>> - add shortcut for stolen region in i915(Tvrtko)
>> - sanity check dsm base vs bar size(Xinyun)
>>
>> Signed-off-by: CQ Tang <cq.tang at intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> Cc: Xinyun Liu <xinyun.liu at intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 137 ++++++++++++++++++---
>> drivers/gpu/drm/i915/i915_drv.h | 7 ++
>> drivers/gpu/drm/i915/i915_pci.c | 2 +-
>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>> drivers/gpu/drm/i915/intel_memory_region.c | 8 ++
>> drivers/gpu/drm/i915/intel_memory_region.h | 5 +-
>> 6 files changed, 140 insertions(+), 20 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..2ed1ca9aec75 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;
>
> The grammar in the comment is a bit hard to parse for me, but more
> importantly, this is now not on the device stolen path, right?
>
> [Comes back later, hm no, still called okay at least there is a comment
> now explaining which are the relevant bits.]
>
>> +
>> /*
>> * 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;
>> @@ -688,39 +698,130 @@ 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->mm.stolen_region,
>> size, I915_BO_ALLOC_CONTIGUOUS);
>> }
>> -static int init_stolen(struct intel_memory_region *mem)
>> +static int init_stolen_smem(struct intel_memory_region *mem)
>> {
>> - intel_memory_region_set_name(mem, "stolen");
>> -
>> /*
>> * 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_smem(struct intel_memory_region *mem)
>> +{
>> + i915_gem_cleanup_stolen(mem->i915);
>> +}
>> +
>> +static const struct intel_memory_region_ops
>> i915_region_stolen_smem_ops = {
>> + .init = init_stolen_smem,
>> + .release = release_stolen_smem,
>> + .init_object = _i915_gem_object_stolen_init,
>> +};
>> +
>> +static int init_stolen_lmem(struct intel_memory_region *mem)
>> +{
>> + int err;
>> +
>> + if (GEM_WARN_ON(resource_size(&mem->region) == 0))
>> + return -ENODEV;
>> +
>> + if (!io_mapping_init_wc(&mem->iomap,
>> + mem->io_start,
>> + resource_size(&mem->region)))
>> + return -EIO;
>> +
>> + /*
>> + * For stolen lmem we mostly just care about populating the dsm
>> related
>> + * bits and setting up the drm_mm allocator for the range.
>> + */
>> + err = i915_gem_init_stolen(mem);
>
> Ideally we would be able to split this into two so there would be no
> further smem/lmem stolen forking inside it. That way we would also avoid
> the "mostly" and reach total clarity but okay, can leave for later.
I'll add a TODO comment :)
>
>> + if (err)
>> + goto err_fini;
>> +
>> + return 0;
>> +
>> +err_fini:
>> + io_mapping_fini(&mem->iomap);
>> + return err;
>> }
>> -static void release_stolen(struct intel_memory_region *mem)
>> +static void release_stolen_lmem(struct intel_memory_region *mem)
>> {
>> + io_mapping_fini(&mem->iomap);
>> i915_gem_cleanup_stolen(mem->i915);
>> }
>> -static const struct intel_memory_region_ops i915_region_stolen_ops = {
>> - .init = init_stolen,
>> - .release = release_stolen,
>> +static const struct intel_memory_region_ops
>> i915_region_stolen_lmem_ops = {
>> + .init = init_stolen_lmem,
>> + .release = release_stolen_lmem,
>> .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);
>
> Is this check needed? Looks like the caller will only call this based on
> HAS_REGION. GEM_DEBUG_WARN_ON(!IS_DGFX())?
>
>> +
>> + lmem_base = intel_uncore_read64(uncore, GEN12_DSMBASE);
>> + if (GEM_WARN_ON(lmem_base >= pci_resource_len(pdev, 2)))
>> + return ERR_PTR(-ENODEV);
>> +
>> + 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_lmem_ops);
>> + if (IS_ERR(mem))
>> + return mem;
>> +
>> + drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n",
>> + &mem->io_start);
>
> Printouts I'd prefer if they were done by the common code which calls
> region->init(). Afterwards it could generically print all the region
> data with common formatting and using the set region name. Optional,
> later, is fine.
Yeah, having a common helper to print everything interesting in
intel_memory_region might be quite nice. I'll add a TODO.
>
>> +
>> + intel_memory_region_set_name(mem, "stolen-local");
>
> Should the name just be passed in to intel_memory_region_create or there
> is a good reason for it to be a follow up step?
I don't see a good reason, so yeah it looks like we can just pass it
along. I don't have a strong opinion here.
>
>> +
>> + 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_smem_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);
>
> This helper is possibly not needed any more since the caller is a switch
> statement with a fallthrough. So eliminate the falltrough and call the
> correct setup directly from there? There is the i915->mm.stolen
> assignment to be duplicated in that case so up to you.
Yes, let's go with that.
>
>> +
>> + return mem;
>> }
>> struct drm_i915_gem_object *
>> @@ -728,7 +829,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->mm.stolen_region;
>> struct drm_i915_gem_object *obj;
>> struct drm_mm_node *stolen;
>> int ret;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index e20294e9227a..0b44333eb703 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -514,6 +514,13 @@ struct intel_l3_parity {
>> };
>> struct i915_gem_mm {
>> + /*
>> + * Shortcut for the stolen region. This points to either
>> + * INTEL_REGION_STOLEN_SMEM for integrated platforms, or
>> + * INTEL_REGION_STOLEN_LMEM for discrete, or NULL if the device
>> doesn't
>> + * support stolen.
>> + */
>> + struct intel_memory_region *stolen_region;
>> /** Memory allocator for GTT stolen memory */
>> struct drm_mm stolen;
>> /** Protects the usage of the GTT stolen memory allocator. This is
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c
>> b/drivers/gpu/drm/i915/i915_pci.c
>> index 44e7b94db63d..d4673e43a46d 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -908,7 +908,7 @@ static const struct intel_device_info rkl_info = {
>> };
>> #define DGFX_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 f80d656331f4..ea20058bc13f 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..9941a4a07fde 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,8 +282,12 @@ 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);
>> + if (!IS_ERR(mem))
>> + i915->mm.stolen_region = mem;
>> break;
>> default:
>> continue;
>> 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
More information about the dri-devel
mailing list