* [Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT
2021-02-03 14:13 [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions Matthew Auld
@ 2021-02-03 14:13 ` Matthew Auld
2021-02-03 14:40 ` Chris Wilson
2021-02-03 14:13 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/gtt/dg1: add PTE_LM plumbing for GGTT Matthew Auld
2021-02-03 14:44 ` [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions Chris Wilson
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Auld @ 2021-02-03 14:13 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT
2021-02-03 14:13 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT Matthew Auld
@ 2021-02-03 14:40 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2021-02-03 14:40 UTC (permalink / raw)
To: Matthew Auld, intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-gfx] [PATCH v2 3/3] drm/i915/gtt/dg1: add PTE_LM plumbing for GGTT
2021-02-03 14:13 [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions Matthew Auld
2021-02-03 14:13 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT Matthew Auld
@ 2021-02-03 14:13 ` Matthew Auld
2021-02-03 14:42 ` Chris Wilson
2021-02-03 14:44 ` [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions Chris Wilson
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Auld @ 2021-02-03 14:13 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions
2021-02-03 14:13 [Intel-gfx] [PATCH v2 1/3] drm/i915: Distinction of memory regions Matthew Auld
2021-02-03 14:13 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/gtt/dg1: add PTE_LM plumbing for ppGTT Matthew Auld
2021-02-03 14:13 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/gtt/dg1: add PTE_LM plumbing for GGTT Matthew Auld
@ 2021-02-03 14:44 ` Chris Wilson
2 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2021-02-03 14:44 UTC (permalink / raw)
To: Matthew Auld, intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread