Re: [Intel-gfx] [PATCH 03/19] drm/i915: Create stolen memory region from local memory

[Matthew Auld <matthew.auld@intel.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel