[Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions

[Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions

[Matthew Auld]

From: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

In preparation for Xe HP multi-tile architecture with multiple memory
regions, we need to be able differentiate multiple instances of device
local-memory.

Note that the region name is just to give it a human friendly
identifier, instead of using class/instance which also uniquely
identifies the region. So far the region name is only for our own
internal debugging in the kernel(like in the selftests), or debugfs
which prints the list of regions, including the regions name.

v2: add commentary for our current region name use

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c          | 2 ++
 drivers/gpu/drm/i915/gt/intel_region_lmem.c | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 35ff68ada4f1..ca76f93bc03d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -68,6 +68,8 @@ int intel_gt_probe_lmem(struct intel_gt *gt)
 	mem->type = INTEL_MEMORY_LOCAL;
 	mem->instance = 0;
 
+	intel_memory_region_set_name(mem, "local%u", mem->instance);
+
 	GEM_BUG_ON(!HAS_REGION(i915, id));
 	GEM_BUG_ON(i915->mm.regions[id]);
 	i915->mm.regions[id] = mem;
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
index 8c498e96b01d..be6f2c8f5184 100644
--- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
+++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
@@ -90,8 +90,6 @@ region_lmem_init(struct intel_memory_region *mem)
 	if (ret)
 		io_mapping_fini(&mem->iomap);
 
-	intel_memory_region_set_name(mem, "local");
-
 	return ret;
 }
 
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT

[Matthew Auld]

For the PTEs we get an LM bit, to signal whether the page resides in
SMEM or LMEM.

v2: just use gen8_pte_encode for dg1

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c  | 11 ++++++++++-
 drivers/gpu/drm/i915/gt/intel_gtt.h   |  3 +++
 drivers/gpu/drm/i915/gt/intel_ppgtt.c |  3 +++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
index 03a9d4396373..4048b0b50147 100644
--- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
@@ -5,6 +5,7 @@
 
 #include <linux/log2.h>
 
+#include "gem/i915_gem_lmem.h"
 #include "gen8_ppgtt.h"
 #include "i915_scatterlist.h"
 #include "i915_trace.h"
@@ -35,6 +36,9 @@ static u64 gen8_pte_encode(dma_addr_t addr,
 	if (unlikely(flags & PTE_READ_ONLY))
 		pte &= ~_PAGE_RW;
 
+	if (flags & PTE_LM)
+		pte |= GEN12_PPGTT_PTE_LM;
+
 	switch (level) {
 	case I915_CACHE_NONE:
 		pte |= PPAT_UNCACHED;
@@ -558,6 +562,7 @@ static void gen8_ppgtt_insert(struct i915_address_space *vm,
 
 static int gen8_init_scratch(struct i915_address_space *vm)
 {
+	u32 pte_flags;
 	int ret;
 	int i;
 
@@ -581,9 +586,13 @@ static int gen8_init_scratch(struct i915_address_space *vm)
 	if (ret)
 		return ret;
 
+	pte_flags = vm->has_read_only;
+	if (i915_gem_object_is_lmem(vm->scratch[0]))
+		pte_flags |= PTE_LM;
+
 	vm->scratch[0]->encode =
 		gen8_pte_encode(px_dma(vm->scratch[0]),
-				I915_CACHE_LLC, vm->has_read_only);
+				I915_CACHE_LLC, pte_flags);
 
 	for (i = 1; i <= vm->top; i++) {
 		struct drm_i915_gem_object *obj;
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 29c10fde8ce3..4a1d9b5cc75b 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -85,6 +85,8 @@ typedef u64 gen8_pte_t;
 #define BYT_PTE_SNOOPED_BY_CPU_CACHES	REG_BIT(2)
 #define BYT_PTE_WRITEABLE		REG_BIT(1)
 
+#define GEN12_PPGTT_PTE_LM (1 << 11)
+
 /*
  * Cacheability Control is a 4-bit value. The low three bits are stored in bits
  * 3:1 of the PTE, while the fourth bit is stored in bit 11 of the PTE.
@@ -264,6 +266,7 @@ struct i915_address_space {
 			  enum i915_cache_level level,
 			  u32 flags); /* Create a valid PTE */
 #define PTE_READ_ONLY	BIT(0)
+#define PTE_LM		BIT(1)
 
 	void (*allocate_va_range)(struct i915_address_space *vm,
 				  struct i915_vm_pt_stash *stash,
diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
index 3f940ae27028..80580d00f97d 100644
--- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
@@ -7,6 +7,7 @@
 
 #include "i915_trace.h"
 #include "intel_gtt.h"
+#include "gem/i915_gem_lmem.h"
 #include "gen6_ppgtt.h"
 #include "gen8_ppgtt.h"
 
@@ -192,6 +193,8 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
 	pte_flags = 0;
 	if (i915_gem_object_is_readonly(vma->obj))
 		pte_flags |= PTE_READ_ONLY;
+	if (i915_gem_object_is_lmem(vma->obj))
+		pte_flags |= PTE_LM;
 
 	vm->insert_entries(vm, vma, cache_level, pte_flags);
 	wmb();
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 3/3] drm/i915/gtt/dg1: add PTE_LM plumbing for GGTT

[Matthew Auld]

For the PTEs we get an LM bit, to signal whether the page resides in
SMEM or LMEM.

Based on a patch from Michel Thierry.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_ggtt.c | 23 ++++++++++++++++++-----
 drivers/gpu/drm/i915/gt/intel_gtt.h  |  3 ++-
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index fc399ac16eda..3d2f417ed5a1 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -10,6 +10,7 @@
 
 #include <drm/i915_drm.h>
 
+#include "gem/i915_gem_lmem.h"
 #include "intel_gt.h"
 #include "i915_drv.h"
 #include "i915_scatterlist.h"
@@ -189,7 +190,12 @@ static u64 gen8_ggtt_pte_encode(dma_addr_t addr,
 				enum i915_cache_level level,
 				u32 flags)
 {
-	return addr | _PAGE_PRESENT;
+	gen8_pte_t pte = addr | _PAGE_PRESENT;
+
+	if (flags & PTE_LM)
+		pte |= GEN12_GGTT_PTE_LM;
+
+	return pte;
 }
 
 static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
@@ -201,13 +207,13 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 				  dma_addr_t addr,
 				  u64 offset,
 				  enum i915_cache_level level,
-				  u32 unused)
+				  u32 flags)
 {
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	gen8_pte_t __iomem *pte =
 		(gen8_pte_t __iomem *)ggtt->gsm + offset / I915_GTT_PAGE_SIZE;
 
-	gen8_set_pte(pte, gen8_ggtt_pte_encode(addr, level, 0));
+	gen8_set_pte(pte, gen8_ggtt_pte_encode(addr, level, flags));
 
 	ggtt->invalidate(ggtt);
 }
@@ -217,7 +223,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 				     enum i915_cache_level level,
 				     u32 flags)
 {
-	const gen8_pte_t pte_encode = gen8_ggtt_pte_encode(0, level, 0);
+	const gen8_pte_t pte_encode = gen8_ggtt_pte_encode(0, level, flags);
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	gen8_pte_t __iomem *gte;
 	gen8_pte_t __iomem *end;
@@ -459,6 +465,8 @@ static void ggtt_bind_vma(struct i915_address_space *vm,
 	pte_flags = 0;
 	if (i915_gem_object_is_readonly(obj))
 		pte_flags |= PTE_READ_ONLY;
+	if (i915_gem_object_is_lmem(obj))
+		pte_flags |= PTE_LM;
 
 	vm->insert_entries(vm, vma, cache_level, pte_flags);
 	vma->page_sizes.gtt = I915_GTT_PAGE_SIZE;
@@ -794,6 +802,7 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
 	struct drm_i915_private *i915 = ggtt->vm.i915;
 	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	phys_addr_t phys_addr;
+	u32 pte_flags;
 	int ret;
 
 	/* For Modern GENs the PTEs and register space are split in the BAR */
@@ -823,9 +832,13 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
 		return ret;
 	}
 
+	pte_flags = 0;
+	if (i915_gem_object_is_lmem(ggtt->vm.scratch[0]))
+		pte_flags |= PTE_LM;
+
 	ggtt->vm.scratch[0]->encode =
 		ggtt->vm.pte_encode(px_dma(ggtt->vm.scratch[0]),
-				    I915_CACHE_NONE, 0);
+				    I915_CACHE_NONE, pte_flags);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 4a1d9b5cc75b..55873663d37f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -85,7 +85,8 @@ typedef u64 gen8_pte_t;
 #define BYT_PTE_SNOOPED_BY_CPU_CACHES	REG_BIT(2)
 #define BYT_PTE_WRITEABLE		REG_BIT(1)
 
-#define GEN12_PPGTT_PTE_LM (1 << 11)
+#define GEN12_GGTT_PTE_LM	(1 << 1)
+#define GEN12_PPGTT_PTE_LM	(1 << 11)
 
 /*
  * Cacheability Control is a 4-bit value. The low three bits are stored in bits
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT

[Chris Wilson]

Quoting Matthew Auld (2021-02-03 14:13:12)
> For the PTEs we get an LM bit, to signal whether the page resides in
> SMEM or LMEM.
> 
> v2: just use gen8_pte_encode for dg1
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c  | 11 ++++++++++-
>  drivers/gpu/drm/i915/gt/intel_gtt.h   |  3 +++
>  drivers/gpu/drm/i915/gt/intel_ppgtt.c |  3 +++
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index 03a9d4396373..4048b0b50147 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/log2.h>
>  
> +#include "gem/i915_gem_lmem.h"
>  #include "gen8_ppgtt.h"
>  #include "i915_scatterlist.h"
>  #include "i915_trace.h"
> @@ -35,6 +36,9 @@ static u64 gen8_pte_encode(dma_addr_t addr,
>         if (unlikely(flags & PTE_READ_ONLY))
>                 pte &= ~_PAGE_RW;
>  
> +       if (flags & PTE_LM)
> +               pte |= GEN12_PPGTT_PTE_LM;
> +
>         switch (level) {
>         case I915_CACHE_NONE:
>                 pte |= PPAT_UNCACHED;
> @@ -558,6 +562,7 @@ static void gen8_ppgtt_insert(struct i915_address_space *vm,
>  
>  static int gen8_init_scratch(struct i915_address_space *vm)
>  {
> +       u32 pte_flags;
>         int ret;
>         int i;
>  
> @@ -581,9 +586,13 @@ static int gen8_init_scratch(struct i915_address_space *vm)
>         if (ret)
>                 return ret;
>  
> +       pte_flags = vm->has_read_only;
> +       if (i915_gem_object_is_lmem(vm->scratch[0]))
> +               pte_flags |= PTE_LM;
> +
>         vm->scratch[0]->encode =
>                 gen8_pte_encode(px_dma(vm->scratch[0]),
> -                               I915_CACHE_LLC, vm->has_read_only);
> +                               I915_CACHE_LLC, pte_flags);

One day we will fix this interface to just use one flags field, not two.
That day is always tomorrow though.

>         for (i = 1; i <= vm->top; i++) {
>                 struct drm_i915_gem_object *obj;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index 29c10fde8ce3..4a1d9b5cc75b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -85,6 +85,8 @@ typedef u64 gen8_pte_t;
>  #define BYT_PTE_SNOOPED_BY_CPU_CACHES  REG_BIT(2)
>  #define BYT_PTE_WRITEABLE              REG_BIT(1)
>  
> +#define GEN12_PPGTT_PTE_LM (1 << 11)

This is on a 64b value, may be overkill (except it will be a nasty day
if we ever run into such a bug) but use BIT_ULL(11).

>  /*
>   * Cacheability Control is a 4-bit value. The low three bits are stored in bits
>   * 3:1 of the PTE, while the fourth bit is stored in bit 11 of the PTE.
> @@ -264,6 +266,7 @@ struct i915_address_space {
>                           enum i915_cache_level level,
>                           u32 flags); /* Create a valid PTE */
>  #define PTE_READ_ONLY  BIT(0)
> +#define PTE_LM         BIT(1)
>  
>         void (*allocate_va_range)(struct i915_address_space *vm,
>                                   struct i915_vm_pt_stash *stash,
> diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> index 3f940ae27028..80580d00f97d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> @@ -7,6 +7,7 @@
>  
>  #include "i915_trace.h"
>  #include "intel_gtt.h"
> +#include "gem/i915_gem_lmem.h"

Break this out. We try to keep the external includes in the block before
the local includes. That makes it more obvious about the coupling that
shouldn't be there :(

[Hmm. We broke alphabetical ordering.]

>  #include "gen6_ppgtt.h"
>  #include "gen8_ppgtt.h"
>  
> @@ -192,6 +193,8 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
>         pte_flags = 0;
>         if (i915_gem_object_is_readonly(vma->obj))
>                 pte_flags |= PTE_READ_ONLY;
> +       if (i915_gem_object_is_lmem(vma->obj))
> +               pte_flags |= PTE_LM;
>  
>         vm->insert_entries(vm, vma, cache_level, pte_flags);
>         wmb();

Just nits,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/gtt/dg1: add PTE_LM plumbing for GGTT

[Chris Wilson]

Quoting Matthew Auld (2021-02-03 14:13:13)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index 4a1d9b5cc75b..55873663d37f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -85,7 +85,8 @@ typedef u64 gen8_pte_t;
>  #define BYT_PTE_SNOOPED_BY_CPU_CACHES  REG_BIT(2)
>  #define BYT_PTE_WRITEABLE              REG_BIT(1)
>  
> -#define GEN12_PPGTT_PTE_LM (1 << 11)
> +#define GEN12_GGTT_PTE_LM      (1 << 1)
> +#define GEN12_PPGTT_PTE_LM     (1 << 11)

Too subtle. Leave a line between the different GTT.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions

[Chris Wilson]

Quoting Matthew Auld (2021-02-03 14:13:11)
> From: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> 
> In preparation for Xe HP multi-tile architecture with multiple memory
> regions, we need to be able differentiate multiple instances of device
> local-memory.
> 
> Note that the region name is just to give it a human friendly
> identifier, instead of using class/instance which also uniquely
> identifies the region. So far the region name is only for our own
> internal debugging in the kernel(like in the selftests), or debugfs
> which prints the list of regions, including the regions name.
> 
> v2: add commentary for our current region name use
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c          | 2 ++
>  drivers/gpu/drm/i915/gt/intel_region_lmem.c | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 35ff68ada4f1..ca76f93bc03d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -68,6 +68,8 @@ int intel_gt_probe_lmem(struct intel_gt *gt)
>         mem->type = INTEL_MEMORY_LOCAL;
>         mem->instance = 0;
>  
> +       intel_memory_region_set_name(mem, "local%u", mem->instance);
> +
>         GEM_BUG_ON(!HAS_REGION(i915, id));
>         GEM_BUG_ON(i915->mm.regions[id]);
>         i915->mm.regions[id] = mem;
> diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> index 8c498e96b01d..be6f2c8f5184 100644
> --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> @@ -90,8 +90,6 @@ region_lmem_init(struct intel_memory_region *mem)
>         if (ret)
>                 io_mapping_fini(&mem->iomap);
>  
> -       intel_memory_region_set_name(mem, "local");

Ok. So in gt_probe_lmem we set up the struct, and in region_lmem_init()
we set up the HW interface and bookkeeping.

Moving the name to probe_lmem alongside the other naming makes sense,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx