diff mbox series

[1/5] drm/i915: Create stolen memory region from local memory

Message ID 20210420131842.164163-1-matthew.auld@intel.com (mailing list archive)
State New
Headers show
Series [1/5] drm/i915: Create stolen memory region from local memory | expand

Commit Message

Matthew Auld April 20, 2021, 1:18 p.m. UTC
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.
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@intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Xinyun Liu <xinyun.liu@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(-)

Comments

Tvrtko Ursulin April 20, 2021, 4:06 p.m. UTC | #1
On 20/04/2021 14:18, 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.
> 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@intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Xinyun Liu <xinyun.liu@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.

> +	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.

> +
> +	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?

> +
> +	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.

> +
> +	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
Matthew Auld April 21, 2021, 9:46 a.m. UTC | #2
On 20/04/2021 17:06, Tvrtko Ursulin wrote:
> 
> On 20/04/2021 14:18, 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.
>> 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@intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Xinyun Liu <xinyun.liu@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
diff mbox series

Patch

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;
+
 	/*
 	 * 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);
+	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);
+
+	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);
+
+	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_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);
+
+	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;