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

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

i915_gem_init_stolen is shared - but which parts of it are relevant for local stolen?

}

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?

• 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

On 16/04/2021 16:04, Matthew Auld wrote:

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?

i915->dsm is not it? Where does i915_gem_init_stolen exists for local-stolen then? At the "resource_size(&mem->region) == 0" check?

...

...

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?

I think so. Each vfunc table with it's own ->init() should make it easier to follow.

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?

Yes, with the above approach of two struct intel_memory_region_ops? Even if some vfuncs are shared it should be better.

I am also confused by ->release ie. i915_gem_cleanup_stolen. How does that work for two stolen regions, I mean one i915->mm.stolen?

Regards,

Tvrtko

On 12/04/2021 10:05, Matthew Auld wrote:

Underneath it's the same stuff, so things like the PTE_LM bits for the GTT should just keep working as-is.

Signed-off-by: Matthew Auld matthew.auld@intel.com

drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c index ce1c83c13d05..017db8f71130 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c @@ -19,7 +19,10 @@ const struct drm_i915_gem_object_ops i915_gem_lmem_obj_ops = {

bool i915_gem_object_is_lmem(struct drm_i915_gem_object *obj) {

• return obj->ops == &i915_gem_lmem_obj_ops;

• struct intel_memory_region *mr = obj->mm.region;

• return mr && (mr->type == INTEL_MEMORY_LOCAL ||

•       mr->type == INTEL_MEMORY_STOLEN_LOCAL);
  

  }

  struct drm_i915_gem_object *

Passable I guess. Although there is also i915_gem_object_is_stolen so it is not immediately clear what are the semantics of i915_gem_object_is_lmem vs that one. Almost like we need more "hierarchy" in region types, or flags of some sort, but I haven't looked at the callers to have a good idea what would work best.

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

On 12/04/2021 10:05, Matthew Auld wrote:

From: CQ Tang cq.tang@intel.com

Since stolen can now be device local-memory underneath, we should try to enforce any min_page_size restrictions when allocating pages.

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 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index 56dd58bef5ee..f713eabb7671 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -677,7 +677,8 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem, if (!stolen) return -ENOMEM;

• ret = i915_gem_stolen_insert_node(i915, stolen, size, 4096);

• ret = i915_gem_stolen_insert_node(i915, stolen, size,

• 			  mem->min_page_size);
  

  if (ret) goto err_free;

@@ -817,8 +818,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915,

/* KISS and expect everything to be page-aligned */ if (GEM_WARN_ON(size == 0) ||

•    GEM_WARN_ON(!IS_ALIGNED(size, I915_GTT_PAGE_SIZE)) ||
  

•    GEM_WARN_ON(!IS_ALIGNED(stolen_offset, I915_GTT_MIN_ALIGNMENT)))
  

•    GEM_WARN_ON(!IS_ALIGNED(size, mem->min_page_size)) ||
  

•    GEM_WARN_ON(!IS_ALIGNED(stolen_offset, mem->min_page_size)))
  

  return ERR_PTR(-EINVAL);

  stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

On 12/04/2021 10:05, Matthew Auld wrote:

From: CQ Tang cq.tang@intel.com

Stolen memory is always allocated as physically contiguous pages, mark the object flags as such.

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 | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index f713eabb7671..49a2dfcc8ba7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -633,14 +633,15 @@ static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = {

static int __i915_gem_object_create_stolen(struct intel_memory_region *mem, struct drm_i915_gem_object *obj,

• 			   struct drm_mm_node *stolen)
  

• 			   struct drm_mm_node *stolen,
  

• 			   unsigned int flags)
  

  { static struct lock_class_key lock_class; unsigned int cache_level; int err;

  drm_gem_private_object_init(&mem->i915->drm, &obj->base, stolen->size);

• i915_gem_object_init(obj, &i915_gem_object_stolen_ops, &lock_class, 0);

• i915_gem_object_init(obj, &i915_gem_object_stolen_ops, &lock_class, flags);

  obj->stolen = stolen; obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;

@@ -682,7 +683,7 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem, if (ret) goto err_free;

• ret = __i915_gem_object_create_stolen(mem, obj, stolen);

• ret = __i915_gem_object_create_stolen(mem, obj, stolen, flags); if (ret) goto err_remove;

@@ -840,7 +841,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915, goto err_stolen; }

• ret = __i915_gem_object_create_stolen(mem, obj, stolen);

• ret = __i915_gem_object_create_stolen(mem, obj, stolen,

• 			      I915_BO_ALLOC_CONTIGUOUS);
  

  if (ret) goto err_object_free;

Are all stolen objects always contiguous or only ones allocated by i915_gem_object_create_stolen_for_preallocated? If former should __i915_gem_object_create_stolen just set the flag without the need to pass it in?

Regards,

Tvrtko

On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

On 12/04/2021 10:05, Matthew Auld wrote:

...

From: Venkata Sandeep Dhanalakota venkata.s.dhanalakota@intel.com

Determine the possible coherent map type based on object location, and if target has llc or if user requires an always coherent mapping.

Cc: Matthew Auld matthew.auld@intel.com Cc: CQ Tang cq.tang@intel.com Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Venkata Sandeep Dhanalakota venkata.s.dhanalakota@intel.com

---

drivers/gpu/drm/i915/gt/intel_engine_cs.c | 3 ++- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- drivers/gpu/drm/i915/gt/intel_ring.c | 9 ++++++--- drivers/gpu/drm/i915/gt/selftest_context.c | 3 ++- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 4 ++-- drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +++- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +++- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 4 +++- drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++-- drivers/gpu/drm/i915/selftests/igt_spinner.c | 4 ++-- 11 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index efe935f80c1a..b79568d370f5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -664,7 +664,8 @@ static int init_status_page(struct intel_engine_cs *engine) if (ret) goto err;

• vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
  

• vaddr = i915_gem_object_pin_map(obj,
  

•                                 i915_coherent_map_type(engine->i915, obj, true));
  if (IS_ERR(vaddr)) {
          ret = PTR_ERR(vaddr);
          goto err_unpin;
  

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 7c9af86fdb1e..47f4397095e5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct intel_context *ce)

  if (ce->state) {
          struct drm_i915_gem_object *obj = ce->state->obj;

•         int type = i915_coherent_map_type(ce->engine->i915);
  

•         int type = i915_coherent_map_type(ce->engine->i915, obj, true);
          void *map;
  
          if (!i915_gem_object_trylock(obj))
  

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index e86897cde984..aafe2a4df496 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce, GEM_BUG_ON(!i915_vma_is_pinned(ce->state));

  *vaddr = i915_gem_object_pin_map(ce->state->obj,

•                                  i915_coherent_map_type(ce->engine->i915) |
  

•                                  i915_coherent_map_type(ce->engine->i915,
  

•                                                         ce->state->obj,
  

•                                                         false) |
                                   I915_MAP_OVERRIDE);
  
  return PTR_ERR_OR_ZERO(*vaddr);
  

diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c b/drivers/gpu/drm/i915/gt/intel_ring.c index aee0a77c77e0..3cf6c7e68108 100644 --- a/drivers/gpu/drm/i915/gt/intel_ring.c +++ b/drivers/gpu/drm/i915/gt/intel_ring.c @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring *ring, struct i915_gem_ww_ctx *ww)

  if (i915_vma_is_map_and_fenceable(vma))
          addr = (void __force *)i915_vma_pin_iomap(vma);

• else
  

•         addr = i915_gem_object_pin_map(vma->obj,
  

•                                        i915_coherent_map_type(vma->vm->i915));
  

• else {
  

•         int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false);
  

•         addr = i915_gem_object_pin_map(vma->obj, type);
  

• }
  

• if (IS_ERR(addr)) {
          ret = PTR_ERR(addr);
          goto err_ring;
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c index b9bdd1d23243..26685b927169 100644 --- a/drivers/gpu/drm/i915/gt/selftest_context.c +++ b/drivers/gpu/drm/i915/gt/selftest_context.c @@ -88,7 +88,8 @@ static int __live_context_size(struct intel_engine_cs *engine) goto err;

  vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj,

•                                          i915_coherent_map_type(engine->i915));
  

•                                          i915_coherent_map_type(engine->i915,
  

•                                                                 ce->state->obj, false));
  if (IS_ERR(vaddr)) {
          err = PTR_ERR(vaddr);
          intel_context_unpin(ce);
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c index 746985971c3a..5b63d4df8c93 100644 --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct intel_gt *gt) h->seqno = memset(vaddr, 0xff, PAGE_SIZE);

  vaddr = i915_gem_object_pin_map_unlocked(h->obj,

•                                          i915_coherent_map_type(gt->i915));
  

•                                          i915_coherent_map_type(gt->i915, h->obj, false));
  if (IS_ERR(vaddr)) {
          err = PTR_ERR(vaddr);
          goto err_unpin_hws;
  

@@ -130,7 +130,7 @@ hang_create_request(struct hang *h, struct intel_engine_cs *engine) return ERR_CAST(obj); }

• vaddr = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(gt->i915));
  

• vaddr = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(gt->i915, obj, false));
  if (IS_ERR(vaddr)) {
          i915_gem_object_put(obj);
          i915_vm_put(vm);
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index 85e7df6a5123..d8f6623524e8 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -1221,7 +1221,9 @@ static int compare_isolation(struct intel_engine_cs *engine, }

  lrc = i915_gem_object_pin_map_unlocked(ce->state->obj,

•                               i915_coherent_map_type(engine->i915));
  

•                                        i915_coherent_map_type(engine->i915,
  

•                                                               ce->state->obj,
  

•                                                               false));
  if (IS_ERR(lrc)) {
          err = PTR_ERR(lrc);
          goto err_B1;
  

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 78305b2ec89d..adae04c47aab 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct intel_guc *guc, u32 size, if (IS_ERR(vma)) return PTR_ERR(vma);

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WB);
  

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj,
  

•                                          i915_coherent_map_type(guc_to_gt(guc)->i915,
  

•                                                                 vma->obj, true));
  if (IS_ERR(vaddr)) {
          i915_vma_unpin_and_release(&vma, 0);
          return PTR_ERR(vaddr);
  

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 2126dd81ac38..56d2144dc6a0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc) if (IS_ERR(vma)) return PTR_ERR(vma);

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WB);
  

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj,
  

•                                          i915_coherent_map_type(gt->i915,
  

•                                                                 vma->obj, true));
  if (IS_ERR(vaddr)) {
          i915_vma_unpin_and_release(&vma, 0);
          return PTR_ERR(vaddr);
  

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 69e43bf91a15..2abbc06712a4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -78,6 +78,7 @@ #include "gem/i915_gem_context_types.h" #include "gem/i915_gem_shrinker.h" #include "gem/i915_gem_stolen.h" +#include "gem/i915_gem_lmem.h"

#include "gt/intel_engine.h" #include "gt/intel_gt_types.h" @@ -1921,9 +1922,15 @@ static inline int intel_hws_csb_write_index(struct drm_i915_private *i915) }

static inline enum i915_map_type -i915_coherent_map_type(struct drm_i915_private *i915) +i915_coherent_map_type(struct drm_i915_private *i915,

•                struct drm_i915_gem_object *obj, bool always_coherent)
  

  {

• return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
  

• if (i915_gem_object_is_lmem(obj))
  

•         return I915_MAP_WC;
  

• if (HAS_LLC(i915) || always_coherent)
  

•         return I915_MAP_WB;
  

• else
  

•         return I915_MAP_WC;
  

Seems this patch is doing two things.

First it is adding lmem support to this helper by always returning WC for lmem objects.

Secondly it is introducing an idea of "always coherent" in a helper called i915_coherent_map_type. Could someone explain what is coherent vs always coherent?

And also, why is always coherent happy with WB? Sounds counter intuitive to me.

All this does is try to keep the existing behaviour intact, whilst also ensuring that all lmem objects are mapped using only WC, no matter what. The always_coherent=true thing is for the existing places where we sometimes map the object using WB, without first considering whether the device has the fast shared LLC vs snooping. Yes, it's slightly ugly :)

Regards,

Tvrtko

...

}

#endif diff --git a/drivers/gpu/drm/i915/selftests/igt_spinner.c b/drivers/gpu/drm/i915/selftests/igt_spinner.c index cfbbe415b57c..5fe397b7d1d9 100644 --- a/drivers/gpu/drm/i915/selftests/igt_spinner.c +++ b/drivers/gpu/drm/i915/selftests/igt_spinner.c @@ -94,9 +94,9 @@ int igt_spinner_pin(struct igt_spinner *spin, }

  if (!spin->batch) {

•         unsigned int mode =
  

•                 i915_coherent_map_type(spin->gt->i915);
  

•         unsigned int mode;
  
  

•         mode = i915_coherent_map_type(spin->gt->i915, spin->obj, false);
          vaddr = igt_spinner_pin_obj(ce, ww, spin->obj, mode, &spin->batch_vma);
          if (IS_ERR(vaddr))
                  return PTR_ERR(vaddr);
  
  

---

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

On Thu, 15 Apr 2021 at 09:21, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

On 14/04/2021 17:20, Matthew Auld wrote:

...

On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 12/04/2021 10:05, Matthew Auld wrote:

...

From: Venkata Sandeep Dhanalakota venkata.s.dhanalakota@intel.com

Determine the possible coherent map type based on object location, and if target has llc or if user requires an always coherent mapping.

Cc: Matthew Auld matthew.auld@intel.com Cc: CQ Tang cq.tang@intel.com Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Venkata Sandeep Dhanalakota venkata.s.dhanalakota@intel.com

---

drivers/gpu/drm/i915/gt/intel_engine_cs.c | 3 ++- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- drivers/gpu/drm/i915/gt/intel_ring.c | 9 ++++++--- drivers/gpu/drm/i915/gt/selftest_context.c | 3 ++- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 4 ++-- drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +++- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +++- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 4 +++- drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++-- drivers/gpu/drm/i915/selftests/igt_spinner.c | 4 ++-- 11 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index efe935f80c1a..b79568d370f5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -664,7 +664,8 @@ static int init_status_page(struct intel_engine_cs *engine) if (ret) goto err;

• vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
  

• vaddr = i915_gem_object_pin_map(obj,
  

•                                 i915_coherent_map_type(engine->i915, obj, true));
   if (IS_ERR(vaddr)) {
           ret = PTR_ERR(vaddr);
           goto err_unpin;
  

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 7c9af86fdb1e..47f4397095e5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct intel_context *ce)

   if (ce->state) {
           struct drm_i915_gem_object *obj = ce->state->obj;

•         int type = i915_coherent_map_type(ce->engine->i915);
  

•         int type = i915_coherent_map_type(ce->engine->i915, obj, true);
           void *map;
  
           if (!i915_gem_object_trylock(obj))
  

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index e86897cde984..aafe2a4df496 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce, GEM_BUG_ON(!i915_vma_is_pinned(ce->state));

   *vaddr = i915_gem_object_pin_map(ce->state->obj,

•                                  i915_coherent_map_type(ce->engine->i915) |
  

•                                  i915_coherent_map_type(ce->engine->i915,
  

•                                                         ce->state->obj,
  

•                                                         false) |
                                    I915_MAP_OVERRIDE);
  
   return PTR_ERR_OR_ZERO(*vaddr);
  

diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c b/drivers/gpu/drm/i915/gt/intel_ring.c index aee0a77c77e0..3cf6c7e68108 100644 --- a/drivers/gpu/drm/i915/gt/intel_ring.c +++ b/drivers/gpu/drm/i915/gt/intel_ring.c @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring *ring, struct i915_gem_ww_ctx *ww)

   if (i915_vma_is_map_and_fenceable(vma))
           addr = (void __force *)i915_vma_pin_iomap(vma);

• else
  

•         addr = i915_gem_object_pin_map(vma->obj,
  

•                                        i915_coherent_map_type(vma->vm->i915));
  

• else {
  

•         int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false);
  

•         addr = i915_gem_object_pin_map(vma->obj, type);
  

• }
  

•  if (IS_ERR(addr)) {
           ret = PTR_ERR(addr);
           goto err_ring;
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c index b9bdd1d23243..26685b927169 100644 --- a/drivers/gpu/drm/i915/gt/selftest_context.c +++ b/drivers/gpu/drm/i915/gt/selftest_context.c @@ -88,7 +88,8 @@ static int __live_context_size(struct intel_engine_cs *engine) goto err;

   vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj,

•                                          i915_coherent_map_type(engine->i915));
  

•                                          i915_coherent_map_type(engine->i915,
  

•                                                                 ce->state->obj, false));
   if (IS_ERR(vaddr)) {
           err = PTR_ERR(vaddr);
           intel_context_unpin(ce);
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c index 746985971c3a..5b63d4df8c93 100644 --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct intel_gt *gt) h->seqno = memset(vaddr, 0xff, PAGE_SIZE);

   vaddr = i915_gem_object_pin_map_unlocked(h->obj,

•                                          i915_coherent_map_type(gt->i915));
  

•                                          i915_coherent_map_type(gt->i915, h->obj, false));
   if (IS_ERR(vaddr)) {
           err = PTR_ERR(vaddr);
           goto err_unpin_hws;
  

@@ -130,7 +130,7 @@ hang_create_request(struct hang *h, struct intel_engine_cs *engine) return ERR_CAST(obj); }

• vaddr = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(gt->i915));
  

• vaddr = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(gt->i915, obj, false));
   if (IS_ERR(vaddr)) {
           i915_gem_object_put(obj);
           i915_vm_put(vm);
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index 85e7df6a5123..d8f6623524e8 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -1221,7 +1221,9 @@ static int compare_isolation(struct intel_engine_cs *engine, }

   lrc = i915_gem_object_pin_map_unlocked(ce->state->obj,

•                               i915_coherent_map_type(engine->i915));
  

•                                        i915_coherent_map_type(engine->i915,
  

•                                                               ce->state->obj,
  

•                                                               false));
   if (IS_ERR(lrc)) {
           err = PTR_ERR(lrc);
           goto err_B1;
  

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 78305b2ec89d..adae04c47aab 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct intel_guc *guc, u32 size, if (IS_ERR(vma)) return PTR_ERR(vma);

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WB);
  

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj,
  

•                                          i915_coherent_map_type(guc_to_gt(guc)->i915,
  

•                                                                 vma->obj, true));
   if (IS_ERR(vaddr)) {
           i915_vma_unpin_and_release(&vma, 0);
           return PTR_ERR(vaddr);
  

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 2126dd81ac38..56d2144dc6a0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc) if (IS_ERR(vma)) return PTR_ERR(vma);

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WB);
  

• vaddr = i915_gem_object_pin_map_unlocked(vma->obj,
  

•                                          i915_coherent_map_type(gt->i915,
  

•                                                                 vma->obj, true));
   if (IS_ERR(vaddr)) {
           i915_vma_unpin_and_release(&vma, 0);
           return PTR_ERR(vaddr);
  

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 69e43bf91a15..2abbc06712a4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -78,6 +78,7 @@ #include "gem/i915_gem_context_types.h" #include "gem/i915_gem_shrinker.h" #include "gem/i915_gem_stolen.h" +#include "gem/i915_gem_lmem.h"

#include "gt/intel_engine.h" #include "gt/intel_gt_types.h" @@ -1921,9 +1922,15 @@ static inline int intel_hws_csb_write_index(struct drm_i915_private *i915) }

static inline enum i915_map_type -i915_coherent_map_type(struct drm_i915_private *i915) +i915_coherent_map_type(struct drm_i915_private *i915,

•                struct drm_i915_gem_object *obj, bool always_coherent)
  

  {

• return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
  

• if (i915_gem_object_is_lmem(obj))
  

•         return I915_MAP_WC;
  

• if (HAS_LLC(i915) || always_coherent)
  

•         return I915_MAP_WB;
  

• else
  

•         return I915_MAP_WC;
  

Seems this patch is doing two things.

First it is adding lmem support to this helper by always returning WC for lmem objects.

Secondly it is introducing an idea of "always coherent" in a helper called i915_coherent_map_type. Could someone explain what is coherent vs always coherent?

And also, why is always coherent happy with WB? Sounds counter intuitive to me.

All this does is try to keep the existing behaviour intact, whilst also ensuring that all lmem objects are mapped using only WC, no matter what. The always_coherent=true thing is for the existing places where we sometimes map the object using WB, without first considering whether the device has the fast shared LLC vs snooping. Yes, it's slightly ugly :)

Not fully following - if we had to write kerneldoc for always_coherent input argument - what it would say?

@always_coherent - If true we should always try to map the object using WB. If false we should only map as WB if the device supports the fast shared LLC, in the case of snooped devices we will map use WC. Note that If the resource is lmem then we will always map as WC, regardless of the value of always_coherent, since that's all we currently support.

Maybe the naming is poor?

Regards,

Tvrtko

On 15/04/2021 12:05, Tvrtko Ursulin wrote:

On 15/04/2021 10:23, Matthew Auld wrote:

...

On Thu, 15 Apr 2021 at 09:21, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 14/04/2021 17:20, Matthew Auld wrote:

...

On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 12/04/2021 10:05, Matthew Auld wrote:

...

From: Venkata Sandeep Dhanalakota venkata.s.dhanalakota@intel.com

Determine the possible coherent map type based on object location, and if target has llc or if user requires an always coherent mapping.

Cc: Matthew Auld matthew.auld@intel.com Cc: CQ Tang cq.tang@intel.com Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Venkata Sandeep Dhanalakota

venkata.s.dhanalakota@intel.com

drivers/gpu/drm/i915/gt/intel_engine_cs.c    |  3 ++-     drivers/gpu/drm/i915/gt/intel_engine_pm.c    |  2 +-     drivers/gpu/drm/i915/gt/intel_lrc.c          |  4 +++-     drivers/gpu/drm/i915/gt/intel_ring.c         |  9 ++++++---     drivers/gpu/drm/i915/gt/selftest_context.c   |  3 ++-     drivers/gpu/drm/i915/gt/selftest_hangcheck.c |  4 ++--     drivers/gpu/drm/i915/gt/selftest_lrc.c       |  4 +++-     drivers/gpu/drm/i915/gt/uc/intel_guc.c       |  4 +++-     drivers/gpu/drm/i915/gt/uc/intel_huc.c       |  4 +++-     drivers/gpu/drm/i915/i915_drv.h              | 11 +++++++++--     drivers/gpu/drm/i915/selftests/igt_spinner.c |  4 ++--     11 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index efe935f80c1a..b79568d370f5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -664,7 +664,8 @@ static int init_status_page(struct intel_engine_cs *engine)         if (ret)                 goto err;

-     vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); +     vaddr = i915_gem_object_pin_map(obj,

i915_coherent_map_type(engine->i915, obj, true));         if (IS_ERR(vaddr)) {                 ret = PTR_ERR(vaddr);                 goto err_unpin; diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 7c9af86fdb1e..47f4397095e5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct intel_context *ce)

if (ce->state) {                 struct drm_i915_gem_object *obj = ce->state->obj; -             int type = i915_coherent_map_type(ce->engine->i915); +             int type = i915_coherent_map_type(ce->engine->i915, obj, true);                 void *map;

if (!i915_gem_object_trylock(obj)) diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index e86897cde984..aafe2a4df496 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce,         GEM_BUG_ON(!i915_vma_is_pinned(ce->state));

*vaddr = i915_gem_object_pin_map(ce->state->obj,

i915_coherent_map_type(ce->engine->i915) |

i915_coherent_map_type(ce->engine->i915,

ce->state->obj,

false) |                                          I915_MAP_OVERRIDE);

return PTR_ERR_OR_ZERO(*vaddr); diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c b/drivers/gpu/drm/i915/gt/intel_ring.c index aee0a77c77e0..3cf6c7e68108 100644 --- a/drivers/gpu/drm/i915/gt/intel_ring.c +++ b/drivers/gpu/drm/i915/gt/intel_ring.c @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring *ring, struct i915_gem_ww_ctx *ww)

if (i915_vma_is_map_and_fenceable(vma))                 addr = (void __force *)i915_vma_pin_iomap(vma); -     else -             addr = i915_gem_object_pin_map(vma->obj,

i915_coherent_map_type(vma->vm->i915)); +     else { +             int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false);

+             addr = i915_gem_object_pin_map(vma->obj, type); +     }

if (IS_ERR(addr)) {                 ret = PTR_ERR(addr);                 goto err_ring; diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c index b9bdd1d23243..26685b927169 100644 --- a/drivers/gpu/drm/i915/gt/selftest_context.c +++ b/drivers/gpu/drm/i915/gt/selftest_context.c @@ -88,7 +88,8 @@ static int __live_context_size(struct intel_engine_cs *engine)                 goto err;

vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj,

i915_coherent_map_type(engine->i915));

i915_coherent_map_type(engine->i915,

ce->state->obj, false));         if (IS_ERR(vaddr)) {                 err = PTR_ERR(vaddr);                 intel_context_unpin(ce); diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c index 746985971c3a..5b63d4df8c93 100644 --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct intel_gt *gt)         h->seqno = memset(vaddr, 0xff, PAGE_SIZE);

vaddr = i915_gem_object_pin_map_unlocked(h->obj,

i915_coherent_map_type(gt->i915));

i915_coherent_map_type(gt->i915, h->obj, false));         if (IS_ERR(vaddr)) {                 err = PTR_ERR(vaddr);                 goto err_unpin_hws; @@ -130,7 +130,7 @@ hang_create_request(struct hang *h, struct intel_engine_cs *engine)                 return ERR_CAST(obj);         }

-     vaddr = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(gt->i915)); +     vaddr = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(gt->i915, obj, false));         if (IS_ERR(vaddr)) {                 i915_gem_object_put(obj);                 i915_vm_put(vm); diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index 85e7df6a5123..d8f6623524e8 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -1221,7 +1221,9 @@ static int compare_isolation(struct intel_engine_cs *engine,         }

lrc = i915_gem_object_pin_map_unlocked(ce->state->obj,

i915_coherent_map_type(engine->i915));

i915_coherent_map_type(engine->i915, +                                                                   ce->state->obj,

+                                                                   false));

if (IS_ERR(lrc)) {                 err = PTR_ERR(lrc);                 goto err_B1; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 78305b2ec89d..adae04c47aab 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct intel_guc *guc, u32 size,         if (IS_ERR(vma))                 return PTR_ERR(vma);

-     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WB); +     vaddr = i915_gem_object_pin_map_unlocked(vma->obj,

i915_coherent_map_type(guc_to_gt(guc)->i915,

vma->obj, true));         if (IS_ERR(vaddr)) {                 i915_vma_unpin_and_release(&vma, 0);                 return PTR_ERR(vaddr); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 2126dd81ac38..56d2144dc6a0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc)         if (IS_ERR(vma))                 return PTR_ERR(vma);

-     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WB); +     vaddr = i915_gem_object_pin_map_unlocked(vma->obj,

i915_coherent_map_type(gt->i915,

vma->obj, true));         if (IS_ERR(vaddr)) {                 i915_vma_unpin_and_release(&vma, 0);                 return PTR_ERR(vaddr); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 69e43bf91a15..2abbc06712a4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -78,6 +78,7 @@     #include "gem/i915_gem_context_types.h"     #include "gem/i915_gem_shrinker.h"     #include "gem/i915_gem_stolen.h" +#include "gem/i915_gem_lmem.h"

#include "gt/intel_engine.h"     #include "gt/intel_gt_types.h" @@ -1921,9 +1922,15 @@ static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)     }

static inline enum i915_map_type -i915_coherent_map_type(struct drm_i915_private *i915) +i915_coherent_map_type(struct drm_i915_private *i915, +                    struct drm_i915_gem_object *obj, bool always_coherent)     { -     return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; +     if (i915_gem_object_is_lmem(obj)) +             return I915_MAP_WC; +     if (HAS_LLC(i915) || always_coherent) +             return I915_MAP_WB; +     else +             return I915_MAP_WC;

Seems this patch is doing two things.

First it is adding lmem support to this helper by always returning WC for lmem objects.

Secondly it is introducing an idea of "always coherent" in a helper called i915_coherent_map_type. Could someone explain what is coherent vs always coherent?

And also, why is always coherent happy with WB? Sounds counter intuitive to me.

All this does is try to keep the existing behaviour intact, whilst also ensuring that all lmem objects are mapped using only WC, no matter what. The always_coherent=true thing is for the existing places where we sometimes map the object using WB, without first considering whether the device has the fast shared LLC vs snooping. Yes, it's slightly ugly :)

Not fully following - if we had to write kerneldoc for always_coherent input argument - what it would say?

@always_coherent - If true we should always try to map the object using WB. If false we should only map as WB if the device supports the fast shared LLC, in the case of snooped devices we will map use WC. Note that If the resource is lmem then we will always map as WC, regardless of the value of always_coherent, since that's all we currently support.

Maybe the naming is poor?

Maybe just confusing to me, not sure yet.

So always_coherent is not about how the callers wants to use it, but about platform knowledge? Or a performance concern for LLC vs snooping cases? Does WB works (coherently) on snooping platforms?

The always_coherent=true is for the existing callers that want WB, regardless of LLC vs snooping.

The other callers use the existing i915_coherent_map_type() which only gives out WB for LLC platforms.

AFAIK, LLC vs snooping should offer the same in terms of coherency, but in terms of performance the shared LLC is much faster, and so for snooping platforms we choose to not enable WB everywhere.

On top of that we now have lmem, but for that we only allow WC. This patch just rolls all of that into one helper, while keeping the existing behaviour unchanged.

Regards,

Tvrtko

On 19/04/2021 15:07, Tvrtko Ursulin wrote:

On 19/04/2021 12:30, Matthew Auld wrote:

...

On 15/04/2021 12:05, Tvrtko Ursulin wrote:

...

On 15/04/2021 10:23, Matthew Auld wrote:

...

On Thu, 15 Apr 2021 at 09:21, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 14/04/2021 17:20, Matthew Auld wrote:

...

On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote: > > > On 12/04/2021 10:05, Matthew Auld wrote: >> From: Venkata Sandeep Dhanalakota venkata.s.dhanalakota@intel.com >> >> Determine the possible coherent map type based on object location, >> and if target has llc or if user requires an always coherent >> mapping. >> >> Cc: Matthew Auld matthew.auld@intel.com >> Cc: CQ Tang cq.tang@intel.com >> Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com >> Signed-off-by: Venkata Sandeep Dhanalakota >> venkata.s.dhanalakota@intel.com >> --- >>     drivers/gpu/drm/i915/gt/intel_engine_cs.c    |  3 ++- >>     drivers/gpu/drm/i915/gt/intel_engine_pm.c    |  2 +- >>     drivers/gpu/drm/i915/gt/intel_lrc.c          |  4 +++- >>     drivers/gpu/drm/i915/gt/intel_ring.c         |  9 ++++++--- >>     drivers/gpu/drm/i915/gt/selftest_context.c   |  3 ++- >>     drivers/gpu/drm/i915/gt/selftest_hangcheck.c |  4 ++-- >>     drivers/gpu/drm/i915/gt/selftest_lrc.c       |  4 +++- >>     drivers/gpu/drm/i915/gt/uc/intel_guc.c       |  4 +++- >>     drivers/gpu/drm/i915/gt/uc/intel_huc.c       |  4 +++- >>     drivers/gpu/drm/i915/i915_drv.h              | 11 +++++++++-- >>     drivers/gpu/drm/i915/selftests/igt_spinner.c |  4 ++-- >>     11 files changed, 36 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> index efe935f80c1a..b79568d370f5 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> @@ -664,7 +664,8 @@ static int init_status_page(struct >> intel_engine_cs *engine) >>         if (ret) >>                 goto err; >> >> -     vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); >> +     vaddr = i915_gem_object_pin_map(obj, >> + i915_coherent_map_type(engine->i915, obj, true)); >>         if (IS_ERR(vaddr)) { >>                 ret = PTR_ERR(vaddr); >>                 goto err_unpin; >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >> index 7c9af86fdb1e..47f4397095e5 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >> @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct intel_context >> *ce) >> >>         if (ce->state) { >>                 struct drm_i915_gem_object *obj = ce->state->obj; >> -             int type = i915_coherent_map_type(ce->engine->i915); >> +             int type = >> i915_coherent_map_type(ce->engine->i915, obj, true); >>                 void *map; >> >>                 if (!i915_gem_object_trylock(obj)) >> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >> b/drivers/gpu/drm/i915/gt/intel_lrc.c >> index e86897cde984..aafe2a4df496 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >> @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce, >>         GEM_BUG_ON(!i915_vma_is_pinned(ce->state)); >> >>         *vaddr = i915_gem_object_pin_map(ce->state->obj, >> - i915_coherent_map_type(ce->engine->i915) | >> + i915_coherent_map_type(ce->engine->i915, >> + ce->state->obj, >> + false) | >>                                          I915_MAP_OVERRIDE); >> >>         return PTR_ERR_OR_ZERO(*vaddr); >> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c >> b/drivers/gpu/drm/i915/gt/intel_ring.c >> index aee0a77c77e0..3cf6c7e68108 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_ring.c >> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c >> @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring *ring, >> struct i915_gem_ww_ctx *ww) >> >>         if (i915_vma_is_map_and_fenceable(vma)) >>                 addr = (void __force *)i915_vma_pin_iomap(vma); >> -     else >> -             addr = i915_gem_object_pin_map(vma->obj, >> - i915_coherent_map_type(vma->vm->i915)); >> +     else { >> +             int type = i915_coherent_map_type(vma->vm->i915, >> vma->obj, false); >> + >> +             addr = i915_gem_object_pin_map(vma->obj, type); >> +     } >> + >>         if (IS_ERR(addr)) { >>                 ret = PTR_ERR(addr); >>                 goto err_ring; >> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c >> b/drivers/gpu/drm/i915/gt/selftest_context.c >> index b9bdd1d23243..26685b927169 100644 >> --- a/drivers/gpu/drm/i915/gt/selftest_context.c >> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c >> @@ -88,7 +88,8 @@ static int __live_context_size(struct >> intel_engine_cs *engine) >>                 goto err; >> >>         vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj, >> - i915_coherent_map_type(engine->i915)); >> + i915_coherent_map_type(engine->i915, >> + ce->state->obj, false)); >>         if (IS_ERR(vaddr)) { >>                 err = PTR_ERR(vaddr); >>                 intel_context_unpin(ce); >> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >> b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >> index 746985971c3a..5b63d4df8c93 100644 >> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >> @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct >> intel_gt *gt) >>         h->seqno = memset(vaddr, 0xff, PAGE_SIZE); >> >>         vaddr = i915_gem_object_pin_map_unlocked(h->obj, >> - i915_coherent_map_type(gt->i915)); >> + i915_coherent_map_type(gt->i915, h->obj, false)); >>         if (IS_ERR(vaddr)) { >>                 err = PTR_ERR(vaddr); >>                 goto err_unpin_hws; >> @@ -130,7 +130,7 @@ hang_create_request(struct hang *h, struct >> intel_engine_cs *engine) >>                 return ERR_CAST(obj); >>         } >> >> -     vaddr = i915_gem_object_pin_map_unlocked(obj, >> i915_coherent_map_type(gt->i915)); >> +     vaddr = i915_gem_object_pin_map_unlocked(obj, >> i915_coherent_map_type(gt->i915, obj, false)); >>         if (IS_ERR(vaddr)) { >>                 i915_gem_object_put(obj); >>                 i915_vm_put(vm); >> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c >> b/drivers/gpu/drm/i915/gt/selftest_lrc.c >> index 85e7df6a5123..d8f6623524e8 100644 >> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c >> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c >> @@ -1221,7 +1221,9 @@ static int compare_isolation(struct >> intel_engine_cs *engine, >>         } >> >>         lrc = i915_gem_object_pin_map_unlocked(ce->state->obj, >> - i915_coherent_map_type(engine->i915)); >> + i915_coherent_map_type(engine->i915, >> + ce->state->obj, >> + false)); >>         if (IS_ERR(lrc)) { >>                 err = PTR_ERR(lrc); >>                 goto err_B1; >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >> index 78305b2ec89d..adae04c47aab 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >> @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct >> intel_guc *guc, u32 size, >>         if (IS_ERR(vma)) >>                 return PTR_ERR(vma); >> >> -     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >> I915_MAP_WB); >> +     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >> + i915_coherent_map_type(guc_to_gt(guc)->i915, >> + vma->obj, true)); >>         if (IS_ERR(vaddr)) { >>                 i915_vma_unpin_and_release(&vma, 0); >>                 return PTR_ERR(vaddr); >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> index 2126dd81ac38..56d2144dc6a0 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct >> intel_huc *huc) >>         if (IS_ERR(vma)) >>                 return PTR_ERR(vma); >> >> -     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >> I915_MAP_WB); >> +     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >> + i915_coherent_map_type(gt->i915, >> + vma->obj, true)); >>         if (IS_ERR(vaddr)) { >>                 i915_vma_unpin_and_release(&vma, 0); >>                 return PTR_ERR(vaddr); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 69e43bf91a15..2abbc06712a4 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -78,6 +78,7 @@ >>     #include "gem/i915_gem_context_types.h" >>     #include "gem/i915_gem_shrinker.h" >>     #include "gem/i915_gem_stolen.h" >> +#include "gem/i915_gem_lmem.h" >> >>     #include "gt/intel_engine.h" >>     #include "gt/intel_gt_types.h" >> @@ -1921,9 +1922,15 @@ static inline int >> intel_hws_csb_write_index(struct drm_i915_private *i915) >>     } >> >>     static inline enum i915_map_type >> -i915_coherent_map_type(struct drm_i915_private *i915) >> +i915_coherent_map_type(struct drm_i915_private *i915, >> +                    struct drm_i915_gem_object *obj, bool >> always_coherent) >>     { >> -     return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; >> +     if (i915_gem_object_is_lmem(obj)) >> +             return I915_MAP_WC; >> +     if (HAS_LLC(i915) || always_coherent) >> +             return I915_MAP_WB; >> +     else >> +             return I915_MAP_WC; > > Seems this patch is doing two things. > > First it is adding lmem support to this helper by always > returning WC > for lmem objects. > > Secondly it is introducing an idea of "always coherent" in a helper > called i915_coherent_map_type. Could someone explain what is > coherent vs > always coherent? > > And also, why is always coherent happy with WB? Sounds counter > intuitive > to me.

All this does is try to keep the existing behaviour intact, whilst also ensuring that all lmem objects are mapped using only WC, no matter what. The always_coherent=true thing is for the existing places where we sometimes map the object using WB, without first considering whether the device has the fast shared LLC vs snooping. Yes, it's slightly ugly :)

Not fully following - if we had to write kerneldoc for always_coherent input argument - what it would say?

@always_coherent - If true we should always try to map the object using WB. If false we should only map as WB if the device supports the fast shared LLC, in the case of snooped devices we will map use WC. Note that If the resource is lmem then we will always map as WC, regardless of the value of always_coherent, since that's all we currently support.

Maybe the naming is poor?

Maybe just confusing to me, not sure yet.

So always_coherent is not about how the callers wants to use it, but about platform knowledge? Or a performance concern for LLC vs snooping cases? Does WB works (coherently) on snooping platforms?

The always_coherent=true is for the existing callers that want WB, regardless of LLC vs snooping.

The other callers use the existing i915_coherent_map_type() which only gives out WB for LLC platforms.

AFAIK, LLC vs snooping should offer the same in terms of coherency, but in terms of performance the shared LLC is much faster, and so for snooping platforms we choose to not enable WB everywhere.

On top of that we now have lmem, but for that we only allow WC. This patch just rolls all of that into one helper, while keeping the existing behaviour unchanged.

Thanks. But I am still struggling with the API. :(

Is the introduction of always_coherent flag in the context of DG1 required even? AFAICT for lmem objects the flag is ignored so no?

If we drop the flag/helper thing, then we need something like:

type = WB; if (i915_gem_object_is_lmem(obj)) type = WC;

vaddr = i915_gem_object_pin_map(obj, type);

In all the places where we currently do:

vaddr = i915_gem_object_pin_map(obj, WB);

Where obj can be lmem, so ctx, ring, guc etc. Is that better or worse? The existing i915_coherent_map_type() callers should work as-is, since DG1 is snooped. And this patch just extends that to cover all cases.

Perhaps we need a new helper instead? Maybe you have a better idea?

Regards,

Tvrtko

On 19/04/2021 16:01, Tvrtko Ursulin wrote:

On 19/04/2021 15:37, Matthew Auld wrote:

...

On 19/04/2021 15:07, Tvrtko Ursulin wrote:

...

On 19/04/2021 12:30, Matthew Auld wrote:

...

On 15/04/2021 12:05, Tvrtko Ursulin wrote:

...

On 15/04/2021 10:23, Matthew Auld wrote:

...

On Thu, 15 Apr 2021 at 09:21, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote: > > > On 14/04/2021 17:20, Matthew Auld wrote: >> On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin >> tvrtko.ursulin@linux.intel.com wrote: >>> >>> >>> On 12/04/2021 10:05, Matthew Auld wrote: >>>> From: Venkata Sandeep Dhanalakota >>>> venkata.s.dhanalakota@intel.com >>>> >>>> Determine the possible coherent map type based on object >>>> location, >>>> and if target has llc or if user requires an always coherent >>>> mapping. >>>> >>>> Cc: Matthew Auld matthew.auld@intel.com >>>> Cc: CQ Tang cq.tang@intel.com >>>> Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com >>>> Signed-off-by: Venkata Sandeep Dhanalakota >>>> venkata.s.dhanalakota@intel.com >>>> --- >>>>     drivers/gpu/drm/i915/gt/intel_engine_cs.c    |  3 ++- >>>>     drivers/gpu/drm/i915/gt/intel_engine_pm.c    |  2 +- >>>>     drivers/gpu/drm/i915/gt/intel_lrc.c          |  4 +++- >>>>     drivers/gpu/drm/i915/gt/intel_ring.c         |  9 ++++++--- >>>>     drivers/gpu/drm/i915/gt/selftest_context.c   |  3 ++- >>>>     drivers/gpu/drm/i915/gt/selftest_hangcheck.c |  4 ++-- >>>>     drivers/gpu/drm/i915/gt/selftest_lrc.c       |  4 +++- >>>>     drivers/gpu/drm/i915/gt/uc/intel_guc.c       |  4 +++- >>>>     drivers/gpu/drm/i915/gt/uc/intel_huc.c       |  4 +++- >>>>     drivers/gpu/drm/i915/i915_drv.h              | 11 +++++++++-- >>>>     drivers/gpu/drm/i915/selftests/igt_spinner.c |  4 ++-- >>>>     11 files changed, 36 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>> index efe935f80c1a..b79568d370f5 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>> @@ -664,7 +664,8 @@ static int init_status_page(struct >>>> intel_engine_cs *engine) >>>>         if (ret) >>>>                 goto err; >>>> >>>> -     vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); >>>> +     vaddr = i915_gem_object_pin_map(obj, >>>> + i915_coherent_map_type(engine->i915, obj, true)); >>>>         if (IS_ERR(vaddr)) { >>>>                 ret = PTR_ERR(vaddr); >>>>                 goto err_unpin; >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>> index 7c9af86fdb1e..47f4397095e5 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>> @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct >>>> intel_context *ce) >>>> >>>>         if (ce->state) { >>>>                 struct drm_i915_gem_object *obj = ce->state->obj; >>>> -             int type = >>>> i915_coherent_map_type(ce->engine->i915); >>>> +             int type = >>>> i915_coherent_map_type(ce->engine->i915, obj, true); >>>>                 void *map; >>>> >>>>                 if (!i915_gem_object_trylock(obj)) >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>> b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>> index e86897cde984..aafe2a4df496 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>> @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce, >>>>         GEM_BUG_ON(!i915_vma_is_pinned(ce->state)); >>>> >>>>         *vaddr = i915_gem_object_pin_map(ce->state->obj, >>>> - i915_coherent_map_type(ce->engine->i915) | >>>> + i915_coherent_map_type(ce->engine->i915, >>>> + ce->state->obj, >>>> + false) | >>>>                                          I915_MAP_OVERRIDE); >>>> >>>>         return PTR_ERR_OR_ZERO(*vaddr); >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c >>>> b/drivers/gpu/drm/i915/gt/intel_ring.c >>>> index aee0a77c77e0..3cf6c7e68108 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_ring.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c >>>> @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring *ring, >>>> struct i915_gem_ww_ctx *ww) >>>> >>>>         if (i915_vma_is_map_and_fenceable(vma)) >>>>                 addr = (void __force *)i915_vma_pin_iomap(vma); >>>> -     else >>>> -             addr = i915_gem_object_pin_map(vma->obj, >>>> - i915_coherent_map_type(vma->vm->i915)); >>>> +     else { >>>> +             int type = i915_coherent_map_type(vma->vm->i915, >>>> vma->obj, false); >>>> + >>>> +             addr = i915_gem_object_pin_map(vma->obj, type); >>>> +     } >>>> + >>>>         if (IS_ERR(addr)) { >>>>                 ret = PTR_ERR(addr); >>>>                 goto err_ring; >>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c >>>> b/drivers/gpu/drm/i915/gt/selftest_context.c >>>> index b9bdd1d23243..26685b927169 100644 >>>> --- a/drivers/gpu/drm/i915/gt/selftest_context.c >>>> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c >>>> @@ -88,7 +88,8 @@ static int __live_context_size(struct >>>> intel_engine_cs *engine) >>>>                 goto err; >>>> >>>>         vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj, >>>> - i915_coherent_map_type(engine->i915)); >>>> + i915_coherent_map_type(engine->i915, >>>> + ce->state->obj, false)); >>>>         if (IS_ERR(vaddr)) { >>>>                 err = PTR_ERR(vaddr); >>>>                 intel_context_unpin(ce); >>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>> b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>> index 746985971c3a..5b63d4df8c93 100644 >>>> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>> @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct >>>> intel_gt *gt) >>>>         h->seqno = memset(vaddr, 0xff, PAGE_SIZE); >>>> >>>>         vaddr = i915_gem_object_pin_map_unlocked(h->obj, >>>> - i915_coherent_map_type(gt->i915)); >>>> + i915_coherent_map_type(gt->i915, h->obj, false)); >>>>         if (IS_ERR(vaddr)) { >>>>                 err = PTR_ERR(vaddr); >>>>                 goto err_unpin_hws; >>>> @@ -130,7 +130,7 @@ hang_create_request(struct hang *h, struct >>>> intel_engine_cs *engine) >>>>                 return ERR_CAST(obj); >>>>         } >>>> >>>> -     vaddr = i915_gem_object_pin_map_unlocked(obj, >>>> i915_coherent_map_type(gt->i915)); >>>> +     vaddr = i915_gem_object_pin_map_unlocked(obj, >>>> i915_coherent_map_type(gt->i915, obj, false)); >>>>         if (IS_ERR(vaddr)) { >>>>                 i915_gem_object_put(obj); >>>>                 i915_vm_put(vm); >>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>> b/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>> index 85e7df6a5123..d8f6623524e8 100644 >>>> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>> @@ -1221,7 +1221,9 @@ static int compare_isolation(struct >>>> intel_engine_cs *engine, >>>>         } >>>> >>>>         lrc = i915_gem_object_pin_map_unlocked(ce->state->obj, >>>> - i915_coherent_map_type(engine->i915)); >>>> + i915_coherent_map_type(engine->i915, >>>> + ce->state->obj, >>>> + false)); >>>>         if (IS_ERR(lrc)) { >>>>                 err = PTR_ERR(lrc); >>>>                 goto err_B1; >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>> index 78305b2ec89d..adae04c47aab 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>> @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct >>>> intel_guc *guc, u32 size, >>>>         if (IS_ERR(vma)) >>>>                 return PTR_ERR(vma); >>>> >>>> -     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>> I915_MAP_WB); >>>> +     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>> + i915_coherent_map_type(guc_to_gt(guc)->i915, >>>> + vma->obj, true)); >>>>         if (IS_ERR(vaddr)) { >>>>                 i915_vma_unpin_and_release(&vma, 0); >>>>                 return PTR_ERR(vaddr); >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>> index 2126dd81ac38..56d2144dc6a0 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>> @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct >>>> intel_huc *huc) >>>>         if (IS_ERR(vma)) >>>>                 return PTR_ERR(vma); >>>> >>>> -     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>> I915_MAP_WB); >>>> +     vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>> + i915_coherent_map_type(gt->i915, >>>> + vma->obj, true)); >>>>         if (IS_ERR(vaddr)) { >>>>                 i915_vma_unpin_and_release(&vma, 0); >>>>                 return PTR_ERR(vaddr); >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>>> b/drivers/gpu/drm/i915/i915_drv.h >>>> index 69e43bf91a15..2abbc06712a4 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -78,6 +78,7 @@ >>>>     #include "gem/i915_gem_context_types.h" >>>>     #include "gem/i915_gem_shrinker.h" >>>>     #include "gem/i915_gem_stolen.h" >>>> +#include "gem/i915_gem_lmem.h" >>>> >>>>     #include "gt/intel_engine.h" >>>>     #include "gt/intel_gt_types.h" >>>> @@ -1921,9 +1922,15 @@ static inline int >>>> intel_hws_csb_write_index(struct drm_i915_private *i915) >>>>     } >>>> >>>>     static inline enum i915_map_type >>>> -i915_coherent_map_type(struct drm_i915_private *i915) >>>> +i915_coherent_map_type(struct drm_i915_private *i915, >>>> +                    struct drm_i915_gem_object *obj, bool >>>> always_coherent) >>>>     { >>>> -     return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; >>>> +     if (i915_gem_object_is_lmem(obj)) >>>> +             return I915_MAP_WC; >>>> +     if (HAS_LLC(i915) || always_coherent) >>>> +             return I915_MAP_WB; >>>> +     else >>>> +             return I915_MAP_WC; >>> >>> Seems this patch is doing two things. >>> >>> First it is adding lmem support to this helper by always >>> returning WC >>> for lmem objects. >>> >>> Secondly it is introducing an idea of "always coherent" in a >>> helper >>> called i915_coherent_map_type. Could someone explain what is >>> coherent vs >>> always coherent? >>> >>> And also, why is always coherent happy with WB? Sounds counter >>> intuitive >>> to me. >> >> All this does is try to keep the existing behaviour intact, whilst >> also ensuring that all lmem objects are mapped using only WC, no >> matter what. The always_coherent=true thing is for the existing >> places >> where we sometimes map the object using WB, without first >> considering >> whether the device has the fast shared LLC vs snooping. Yes, it's >> slightly ugly :) > > Not fully following - if we had to write kerneldoc for > always_coherent > input argument - what it would say?

@always_coherent - If true we should always try to map the object using WB. If false we should only map as WB if the device supports the fast shared LLC, in the case of snooped devices we will map use WC. Note that If the resource is lmem then we will always map as WC, regardless of the value of always_coherent, since that's all we currently support.

Maybe the naming is poor?

Maybe just confusing to me, not sure yet.

So always_coherent is not about how the callers wants to use it, but about platform knowledge? Or a performance concern for LLC vs snooping cases? Does WB works (coherently) on snooping platforms?

The always_coherent=true is for the existing callers that want WB, regardless of LLC vs snooping.

The other callers use the existing i915_coherent_map_type() which only gives out WB for LLC platforms.

AFAIK, LLC vs snooping should offer the same in terms of coherency, but in terms of performance the shared LLC is much faster, and so for snooping platforms we choose to not enable WB everywhere.

On top of that we now have lmem, but for that we only allow WC. This patch just rolls all of that into one helper, while keeping the existing behaviour unchanged.

Thanks. But I am still struggling with the API. :(

Is the introduction of always_coherent flag in the context of DG1 required even? AFAICT for lmem objects the flag is ignored so no?

If we drop the flag/helper thing, then we need something like:

type = WB; if (i915_gem_object_is_lmem(obj))      type = WC;

vaddr = i915_gem_object_pin_map(obj, type);

In all the places where we currently do:

vaddr = i915_gem_object_pin_map(obj, WB);

Where obj can be lmem, so ctx, ring, guc etc. Is that better or worse? The existing i915_coherent_map_type() callers should work as-is, since DG1 is snooped. And this patch just extends that to cover all cases.

Perhaps we need a new helper instead? Maybe you have a better idea?

Not yet. Would it make sense to put something in kerneldoc about when callers might choose always_coherent true vs false? In terms of expected usage (frequency, simplicity?) and any rules with regards when callers need to worry about flushing/ordering when there are mixed read and writes?

Hmmm, looking at this again, maybe for now we should just go with:

type = WB; if (i915_gem_object_is_lmem(obj)) type = WC;

vaddr = i915_gem_object_pin_map(obj, type)

Which is way less confusing, plus there are only a handful of places where we need this, so doesn't seem too bad?

Alternatively, we could wrap that in something like:

/* Returns WB for system memory, or WC for local memory */ void *i915_gem_object_pin_map_default(obj);

Thoughts?

Regards,

Tvrtko

On Wed, 21 Apr 2021 at 16:41, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

On 21/04/2021 12:42, Matthew Auld wrote:

...

On 19/04/2021 16:01, Tvrtko Ursulin wrote:

...

On 19/04/2021 15:37, Matthew Auld wrote:

...

On 19/04/2021 15:07, Tvrtko Ursulin wrote:

...

On 19/04/2021 12:30, Matthew Auld wrote:

...

On 15/04/2021 12:05, Tvrtko Ursulin wrote: > > On 15/04/2021 10:23, Matthew Auld wrote: >> On Thu, 15 Apr 2021 at 09:21, Tvrtko Ursulin >> tvrtko.ursulin@linux.intel.com wrote: >>> >>> >>> On 14/04/2021 17:20, Matthew Auld wrote: >>>> On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin >>>> tvrtko.ursulin@linux.intel.com wrote: >>>>> >>>>> >>>>> On 12/04/2021 10:05, Matthew Auld wrote: >>>>>> From: Venkata Sandeep Dhanalakota >>>>>> venkata.s.dhanalakota@intel.com >>>>>> >>>>>> Determine the possible coherent map type based on object >>>>>> location, >>>>>> and if target has llc or if user requires an always coherent >>>>>> mapping. >>>>>> >>>>>> Cc: Matthew Auld matthew.auld@intel.com >>>>>> Cc: CQ Tang cq.tang@intel.com >>>>>> Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com >>>>>> Signed-off-by: Venkata Sandeep Dhanalakota >>>>>> venkata.s.dhanalakota@intel.com >>>>>> --- >>>>>> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 3 ++- >>>>>> drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- >>>>>> drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- >>>>>> drivers/gpu/drm/i915/gt/intel_ring.c | 9 ++++++--- >>>>>> drivers/gpu/drm/i915/gt/selftest_context.c | 3 ++- >>>>>> drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 4 ++-- >>>>>> drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +++- >>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +++- >>>>>> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 4 +++- >>>>>> drivers/gpu/drm/i915/i915_drv.h | 11 >>>>>> +++++++++-- >>>>>> drivers/gpu/drm/i915/selftests/igt_spinner.c | 4 ++-- >>>>>> 11 files changed, 36 insertions(+), 16 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>> index efe935f80c1a..b79568d370f5 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>> @@ -664,7 +664,8 @@ static int init_status_page(struct >>>>>> intel_engine_cs *engine) >>>>>> if (ret) >>>>>> goto err; >>>>>> >>>>>> - vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); >>>>>> + vaddr = i915_gem_object_pin_map(obj, >>>>>> + i915_coherent_map_type(engine->i915, obj, true)); >>>>>> if (IS_ERR(vaddr)) { >>>>>> ret = PTR_ERR(vaddr); >>>>>> goto err_unpin; >>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>> index 7c9af86fdb1e..47f4397095e5 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>> @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct >>>>>> intel_context *ce) >>>>>> >>>>>> if (ce->state) { >>>>>> struct drm_i915_gem_object *obj = >>>>>> ce->state->obj; >>>>>> - int type = >>>>>> i915_coherent_map_type(ce->engine->i915); >>>>>> + int type = >>>>>> i915_coherent_map_type(ce->engine->i915, obj, true); >>>>>> void *map; >>>>>> >>>>>> if (!i915_gem_object_trylock(obj)) >>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>> b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>> index e86897cde984..aafe2a4df496 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>> @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce, >>>>>> GEM_BUG_ON(!i915_vma_is_pinned(ce->state)); >>>>>> >>>>>> *vaddr = i915_gem_object_pin_map(ce->state->obj, >>>>>> - i915_coherent_map_type(ce->engine->i915) | >>>>>> + i915_coherent_map_type(ce->engine->i915, >>>>>> + ce->state->obj, >>>>>> + false) | >>>>>> I915_MAP_OVERRIDE); >>>>>> >>>>>> return PTR_ERR_OR_ZERO(*vaddr); >>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>> b/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>> index aee0a77c77e0..3cf6c7e68108 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>> @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring >>>>>> *ring, struct i915_gem_ww_ctx *ww) >>>>>> >>>>>> if (i915_vma_is_map_and_fenceable(vma)) >>>>>> addr = (void __force *)i915_vma_pin_iomap(vma); >>>>>> - else >>>>>> - addr = i915_gem_object_pin_map(vma->obj, >>>>>> - i915_coherent_map_type(vma->vm->i915)); >>>>>> + else { >>>>>> + int type = >>>>>> i915_coherent_map_type(vma->vm->i915, vma->obj, false); >>>>>> + >>>>>> + addr = i915_gem_object_pin_map(vma->obj, type); >>>>>> + } >>>>>> + >>>>>> if (IS_ERR(addr)) { >>>>>> ret = PTR_ERR(addr); >>>>>> goto err_ring; >>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>> b/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>> index b9bdd1d23243..26685b927169 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>> @@ -88,7 +88,8 @@ static int __live_context_size(struct >>>>>> intel_engine_cs *engine) >>>>>> goto err; >>>>>> >>>>>> vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj, >>>>>> - i915_coherent_map_type(engine->i915)); >>>>>> + i915_coherent_map_type(engine->i915, >>>>>> + ce->state->obj, false)); >>>>>> if (IS_ERR(vaddr)) { >>>>>> err = PTR_ERR(vaddr); >>>>>> intel_context_unpin(ce); >>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>> b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>> index 746985971c3a..5b63d4df8c93 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>> @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct >>>>>> intel_gt *gt) >>>>>> h->seqno = memset(vaddr, 0xff, PAGE_SIZE); >>>>>> >>>>>> vaddr = i915_gem_object_pin_map_unlocked(h->obj, >>>>>> - i915_coherent_map_type(gt->i915)); >>>>>> + i915_coherent_map_type(gt->i915, h->obj, false)); >>>>>> if (IS_ERR(vaddr)) { >>>>>> err = PTR_ERR(vaddr); >>>>>> goto err_unpin_hws; >>>>>> @@ -130,7 +130,7 @@ hang_create_request(struct hang *h, >>>>>> struct intel_engine_cs *engine) >>>>>> return ERR_CAST(obj); >>>>>> } >>>>>> >>>>>> - vaddr = i915_gem_object_pin_map_unlocked(obj, >>>>>> i915_coherent_map_type(gt->i915)); >>>>>> + vaddr = i915_gem_object_pin_map_unlocked(obj, >>>>>> i915_coherent_map_type(gt->i915, obj, false)); >>>>>> if (IS_ERR(vaddr)) { >>>>>> i915_gem_object_put(obj); >>>>>> i915_vm_put(vm); >>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>> b/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>> index 85e7df6a5123..d8f6623524e8 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>> @@ -1221,7 +1221,9 @@ static int compare_isolation(struct >>>>>> intel_engine_cs *engine, >>>>>> } >>>>>> >>>>>> lrc = i915_gem_object_pin_map_unlocked(ce->state->obj, >>>>>> - i915_coherent_map_type(engine->i915)); >>>>>> + i915_coherent_map_type(engine->i915, >>>>>> + ce->state->obj, >>>>>> + false)); >>>>>> if (IS_ERR(lrc)) { >>>>>> err = PTR_ERR(lrc); >>>>>> goto err_B1; >>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>> index 78305b2ec89d..adae04c47aab 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>> @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct >>>>>> intel_guc *guc, u32 size, >>>>>> if (IS_ERR(vma)) >>>>>> return PTR_ERR(vma); >>>>>> >>>>>> - vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>> I915_MAP_WB); >>>>>> + vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>> + i915_coherent_map_type(guc_to_gt(guc)->i915, >>>>>> + vma->obj, true)); >>>>>> if (IS_ERR(vaddr)) { >>>>>> i915_vma_unpin_and_release(&vma, 0); >>>>>> return PTR_ERR(vaddr); >>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>> index 2126dd81ac38..56d2144dc6a0 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>> @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct >>>>>> intel_huc *huc) >>>>>> if (IS_ERR(vma)) >>>>>> return PTR_ERR(vma); >>>>>> >>>>>> - vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>> I915_MAP_WB); >>>>>> + vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>> + i915_coherent_map_type(gt->i915, >>>>>> + vma->obj, true)); >>>>>> if (IS_ERR(vaddr)) { >>>>>> i915_vma_unpin_and_release(&vma, 0); >>>>>> return PTR_ERR(vaddr); >>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>>>>> b/drivers/gpu/drm/i915/i915_drv.h >>>>>> index 69e43bf91a15..2abbc06712a4 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>>>> @@ -78,6 +78,7 @@ >>>>>> #include "gem/i915_gem_context_types.h" >>>>>> #include "gem/i915_gem_shrinker.h" >>>>>> #include "gem/i915_gem_stolen.h" >>>>>> +#include "gem/i915_gem_lmem.h" >>>>>> >>>>>> #include "gt/intel_engine.h" >>>>>> #include "gt/intel_gt_types.h" >>>>>> @@ -1921,9 +1922,15 @@ static inline int >>>>>> intel_hws_csb_write_index(struct drm_i915_private *i915) >>>>>> } >>>>>> >>>>>> static inline enum i915_map_type >>>>>> -i915_coherent_map_type(struct drm_i915_private *i915) >>>>>> +i915_coherent_map_type(struct drm_i915_private *i915, >>>>>> + struct drm_i915_gem_object *obj, bool >>>>>> always_coherent) >>>>>> { >>>>>> - return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; >>>>>> + if (i915_gem_object_is_lmem(obj)) >>>>>> + return I915_MAP_WC; >>>>>> + if (HAS_LLC(i915) || always_coherent) >>>>>> + return I915_MAP_WB; >>>>>> + else >>>>>> + return I915_MAP_WC; >>>>> >>>>> Seems this patch is doing two things. >>>>> >>>>> First it is adding lmem support to this helper by always >>>>> returning WC >>>>> for lmem objects. >>>>> >>>>> Secondly it is introducing an idea of "always coherent" in a >>>>> helper >>>>> called i915_coherent_map_type. Could someone explain what is >>>>> coherent vs >>>>> always coherent? >>>>> >>>>> And also, why is always coherent happy with WB? Sounds counter >>>>> intuitive >>>>> to me. >>>> >>>> All this does is try to keep the existing behaviour intact, whilst >>>> also ensuring that all lmem objects are mapped using only WC, no >>>> matter what. The always_coherent=true thing is for the existing >>>> places >>>> where we sometimes map the object using WB, without first >>>> considering >>>> whether the device has the fast shared LLC vs snooping. Yes, it's >>>> slightly ugly :) >>> >>> Not fully following - if we had to write kerneldoc for >>> always_coherent >>> input argument - what it would say? >> >> @always_coherent - If true we should always try to map the object >> using WB. If false we should only map as WB if the device >> supports the >> fast shared LLC, in the case of snooped devices we will map use WC. >> Note that If the resource is lmem then we will always map as WC, >> regardless of the value of always_coherent, since that's all we >> currently support. >> >> Maybe the naming is poor? > > Maybe just confusing to me, not sure yet. > > So always_coherent is not about how the callers wants to use it, > but about platform knowledge? Or a performance concern for LLC vs > snooping cases? Does WB works (coherently) on snooping platforms?

The always_coherent=true is for the existing callers that want WB, regardless of LLC vs snooping.

The other callers use the existing i915_coherent_map_type() which only gives out WB for LLC platforms.

AFAIK, LLC vs snooping should offer the same in terms of coherency, but in terms of performance the shared LLC is much faster, and so for snooping platforms we choose to not enable WB everywhere.

On top of that we now have lmem, but for that we only allow WC. This patch just rolls all of that into one helper, while keeping the existing behaviour unchanged.

Thanks. But I am still struggling with the API. :(

Is the introduction of always_coherent flag in the context of DG1 required even? AFAICT for lmem objects the flag is ignored so no?

If we drop the flag/helper thing, then we need something like:

type = WB; if (i915_gem_object_is_lmem(obj)) type = WC;

vaddr = i915_gem_object_pin_map(obj, type);

In all the places where we currently do:

vaddr = i915_gem_object_pin_map(obj, WB);

Where obj can be lmem, so ctx, ring, guc etc. Is that better or worse? The existing i915_coherent_map_type() callers should work as-is, since DG1 is snooped. And this patch just extends that to cover all cases.

Perhaps we need a new helper instead? Maybe you have a better idea?

Not yet. Would it make sense to put something in kerneldoc about when callers might choose always_coherent true vs false? In terms of expected usage (frequency, simplicity?) and any rules with regards when callers need to worry about flushing/ordering when there are mixed read and writes?

Hmmm, looking at this again, maybe for now we should just go with:

type = WB; if (i915_gem_object_is_lmem(obj)) type = WC;

vaddr = i915_gem_object_pin_map(obj, type)

Which is way less confusing, plus there are only a handful of places where we need this, so doesn't seem too bad?

Alternatively, we could wrap that in something like:

/* Returns WB for system memory, or WC for local memory */ void *i915_gem_object_pin_map_default(obj);

Thoughts?

I went and looked at the use sites to try and figure it out.

First thing, the bool always_coherent story is only relevant when we decide to place some object in system memory. Otherwise mapping is always WC so I guess our code needs to handle it anyway. Well, if the assumption is that we can change the location of the objects and it all just keeps working? Or that is not the goal?

I guess your concern is that mapping as WC has different semantics, and that might somehow break the caller?

Let see about the users (ignoring selftests):

1. lrc_reg_state and ring; always_coherent=false

Update frequency medium and mostly write from the CPU side.

They say always_coherent=false - which means they have to handle being given a WC mapping anyway.

What is the benefit of ever selecting WB here?

2. Engine status page; always_coherent=true

Frequently read and written from the CPU and GPU so cost of snooping is therefore fine? Apart from having to be ready to deal with WC anyway.

3. dbg_poison_ce; always_coherent=true

Writes to lrc_reg_state once - meh. Could just as well always ask for WC.

4. intel_guc_allocate_and_map_vma; always_coherent=true

This one has three users:

a) guc_stage_desc_pool_create stage_desc_pool_vaddr

This one seems write once at init.

b) intel_guc_ct_init

Use for CT communication so similar to CSB on engine status page in principle. But code also has to deal with WC when object is in lmem.

c) intel_guc_ads_create

CPU appears to only write on init and GPU reset.

5. intel_huc_rsa_data_create; always_coheret=true

Called from intel_huc_init so it appears write once from CPU. Not sure why it would need a coherent mapping if that is correct.

I think this exercise left me equally confused. Because flushing and read-write ordering rules are different between WB and WC. And code which accesses all these mappings either has to know which one is in use, or does not care. For the latter case we have to be sure about for every path.

Users of pin_map() are generally meant to call flush_map() where appropriate, which should do the right thing for us. For WC it only needs to flush the wcb. For WB it's more complicated since that depends on if the object is considered coherent or not, if it is then we don't need to do anything, otherwise we need to clflush.

Also note that we If we just map the buffer as WB, that by itself doesn't magically enable snooping for the pages AFAIK. We still have to tell the GPU that these pages are meant to be coherent, which we always do for LLC platforms I think, since the shared LLC is considered fast, whereas on snooping platforms, we don't enable this by default, and have this as CACHE_NONE instead(see shmem_object_init for example), and incur the cost of additional clflushing. Doing an explicit i915_gem_object_set_coherency(I915_CACHE_LLC) I think will mark the object as coherent for us. I think there are also some matching GTT bits for caching.

Also for DG1 you apparently can't disable snooping, as per what Daniel was saying in another thread.

The write on init / reset ones are easy enough and it doesn't really matter for them to use the coherent helper.

Lrc_reg_state as well I think can be WC with explicit flushing - it has to on lmem, no?

I doubt it has to be, since the GPU still just accesses it through the GTT.

This leaves the status page (CSB, etc) and GuC CT. Those are frequent R/W but also code has to be able to handle WC so what is the benefit of WB? It ends up faster than if it was WC, considering explicit flushes/barriers are still in there?

No idea for GuC, but for the hwsp it's still in system memory, and is WB, even for discrete. Chris measured this to be more performant with our execlists submission path than say just sticking it in lmem, and mapping it as WC.

Regards,

Tvrtko

On 26/04/2021 09:57, Matthew Auld wrote:

On Wed, 21 Apr 2021 at 20:13, Matthew Auld matthew.william.auld@gmail.com wrote:

...

On Wed, 21 Apr 2021 at 16:41, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 21/04/2021 12:42, Matthew Auld wrote:

...

On 19/04/2021 16:01, Tvrtko Ursulin wrote:

...

On 19/04/2021 15:37, Matthew Auld wrote:

...

On 19/04/2021 15:07, Tvrtko Ursulin wrote: > > On 19/04/2021 12:30, Matthew Auld wrote: >> On 15/04/2021 12:05, Tvrtko Ursulin wrote: >>> >>> On 15/04/2021 10:23, Matthew Auld wrote: >>>> On Thu, 15 Apr 2021 at 09:21, Tvrtko Ursulin >>>> tvrtko.ursulin@linux.intel.com wrote: >>>>> >>>>> >>>>> On 14/04/2021 17:20, Matthew Auld wrote: >>>>>> On Wed, 14 Apr 2021 at 16:22, Tvrtko Ursulin >>>>>> tvrtko.ursulin@linux.intel.com wrote: >>>>>>> >>>>>>> >>>>>>> On 12/04/2021 10:05, Matthew Auld wrote: >>>>>>>> From: Venkata Sandeep Dhanalakota >>>>>>>> venkata.s.dhanalakota@intel.com >>>>>>>> >>>>>>>> Determine the possible coherent map type based on object >>>>>>>> location, >>>>>>>> and if target has llc or if user requires an always coherent >>>>>>>> mapping. >>>>>>>> >>>>>>>> Cc: Matthew Auld matthew.auld@intel.com >>>>>>>> Cc: CQ Tang cq.tang@intel.com >>>>>>>> Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com >>>>>>>> Signed-off-by: Venkata Sandeep Dhanalakota >>>>>>>> venkata.s.dhanalakota@intel.com >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 3 ++- >>>>>>>> drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- >>>>>>>> drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- >>>>>>>> drivers/gpu/drm/i915/gt/intel_ring.c | 9 ++++++--- >>>>>>>> drivers/gpu/drm/i915/gt/selftest_context.c | 3 ++- >>>>>>>> drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 4 ++-- >>>>>>>> drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +++- >>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +++- >>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 4 +++- >>>>>>>> drivers/gpu/drm/i915/i915_drv.h | 11 >>>>>>>> +++++++++-- >>>>>>>> drivers/gpu/drm/i915/selftests/igt_spinner.c | 4 ++-- >>>>>>>> 11 files changed, 36 insertions(+), 16 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>>>> index efe935f80c1a..b79568d370f5 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >>>>>>>> @@ -664,7 +664,8 @@ static int init_status_page(struct >>>>>>>> intel_engine_cs *engine) >>>>>>>> if (ret) >>>>>>>> goto err; >>>>>>>> >>>>>>>> - vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); >>>>>>>> + vaddr = i915_gem_object_pin_map(obj, >>>>>>>> + i915_coherent_map_type(engine->i915, obj, true)); >>>>>>>> if (IS_ERR(vaddr)) { >>>>>>>> ret = PTR_ERR(vaddr); >>>>>>>> goto err_unpin; >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>>>> index 7c9af86fdb1e..47f4397095e5 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c >>>>>>>> @@ -23,7 +23,7 @@ static void dbg_poison_ce(struct >>>>>>>> intel_context *ce) >>>>>>>> >>>>>>>> if (ce->state) { >>>>>>>> struct drm_i915_gem_object *obj = >>>>>>>> ce->state->obj; >>>>>>>> - int type = >>>>>>>> i915_coherent_map_type(ce->engine->i915); >>>>>>>> + int type = >>>>>>>> i915_coherent_map_type(ce->engine->i915, obj, true); >>>>>>>> void *map; >>>>>>>> >>>>>>>> if (!i915_gem_object_trylock(obj)) >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>>>> b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>>>> index e86897cde984..aafe2a4df496 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>>>>> @@ -903,7 +903,9 @@ lrc_pre_pin(struct intel_context *ce, >>>>>>>> GEM_BUG_ON(!i915_vma_is_pinned(ce->state)); >>>>>>>> >>>>>>>> *vaddr = i915_gem_object_pin_map(ce->state->obj, >>>>>>>> - i915_coherent_map_type(ce->engine->i915) | >>>>>>>> + i915_coherent_map_type(ce->engine->i915, >>>>>>>> + ce->state->obj, >>>>>>>> + false) | >>>>>>>> I915_MAP_OVERRIDE); >>>>>>>> >>>>>>>> return PTR_ERR_OR_ZERO(*vaddr); >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>>>> b/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>>>> index aee0a77c77e0..3cf6c7e68108 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c >>>>>>>> @@ -53,9 +53,12 @@ int intel_ring_pin(struct intel_ring >>>>>>>> *ring, struct i915_gem_ww_ctx *ww) >>>>>>>> >>>>>>>> if (i915_vma_is_map_and_fenceable(vma)) >>>>>>>> addr = (void __force *)i915_vma_pin_iomap(vma); >>>>>>>> - else >>>>>>>> - addr = i915_gem_object_pin_map(vma->obj, >>>>>>>> - i915_coherent_map_type(vma->vm->i915)); >>>>>>>> + else { >>>>>>>> + int type = >>>>>>>> i915_coherent_map_type(vma->vm->i915, vma->obj, false); >>>>>>>> + >>>>>>>> + addr = i915_gem_object_pin_map(vma->obj, type); >>>>>>>> + } >>>>>>>> + >>>>>>>> if (IS_ERR(addr)) { >>>>>>>> ret = PTR_ERR(addr); >>>>>>>> goto err_ring; >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>>>> b/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>>>> index b9bdd1d23243..26685b927169 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c >>>>>>>> @@ -88,7 +88,8 @@ static int __live_context_size(struct >>>>>>>> intel_engine_cs *engine) >>>>>>>> goto err; >>>>>>>> >>>>>>>> vaddr = i915_gem_object_pin_map_unlocked(ce->state->obj, >>>>>>>> - i915_coherent_map_type(engine->i915)); >>>>>>>> + i915_coherent_map_type(engine->i915, >>>>>>>> + ce->state->obj, false)); >>>>>>>> if (IS_ERR(vaddr)) { >>>>>>>> err = PTR_ERR(vaddr); >>>>>>>> intel_context_unpin(ce); >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>>>> b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>>>> index 746985971c3a..5b63d4df8c93 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c >>>>>>>> @@ -69,7 +69,7 @@ static int hang_init(struct hang *h, struct >>>>>>>> intel_gt *gt) >>>>>>>> h->seqno = memset(vaddr, 0xff, PAGE_SIZE); >>>>>>>> >>>>>>>> vaddr = i915_gem_object_pin_map_unlocked(h->obj, >>>>>>>> - i915_coherent_map_type(gt->i915)); >>>>>>>> + i915_coherent_map_type(gt->i915, h->obj, false)); >>>>>>>> if (IS_ERR(vaddr)) { >>>>>>>> err = PTR_ERR(vaddr); >>>>>>>> goto err_unpin_hws; >>>>>>>> @@ -130,7 +130,7 @@ hang_create_request(struct hang *h, >>>>>>>> struct intel_engine_cs *engine) >>>>>>>> return ERR_CAST(obj); >>>>>>>> } >>>>>>>> >>>>>>>> - vaddr = i915_gem_object_pin_map_unlocked(obj, >>>>>>>> i915_coherent_map_type(gt->i915)); >>>>>>>> + vaddr = i915_gem_object_pin_map_unlocked(obj, >>>>>>>> i915_coherent_map_type(gt->i915, obj, false)); >>>>>>>> if (IS_ERR(vaddr)) { >>>>>>>> i915_gem_object_put(obj); >>>>>>>> i915_vm_put(vm); >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>>>> b/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>>>> index 85e7df6a5123..d8f6623524e8 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c >>>>>>>> @@ -1221,7 +1221,9 @@ static int compare_isolation(struct >>>>>>>> intel_engine_cs *engine, >>>>>>>> } >>>>>>>> >>>>>>>> lrc = i915_gem_object_pin_map_unlocked(ce->state->obj, >>>>>>>> - i915_coherent_map_type(engine->i915)); >>>>>>>> + i915_coherent_map_type(engine->i915, >>>>>>>> + ce->state->obj, >>>>>>>> + false)); >>>>>>>> if (IS_ERR(lrc)) { >>>>>>>> err = PTR_ERR(lrc); >>>>>>>> goto err_B1; >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>>>> index 78305b2ec89d..adae04c47aab 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c >>>>>>>> @@ -682,7 +682,9 @@ int intel_guc_allocate_and_map_vma(struct >>>>>>>> intel_guc *guc, u32 size, >>>>>>>> if (IS_ERR(vma)) >>>>>>>> return PTR_ERR(vma); >>>>>>>> >>>>>>>> - vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>>>> I915_MAP_WB); >>>>>>>> + vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>>>> + i915_coherent_map_type(guc_to_gt(guc)->i915, >>>>>>>> + vma->obj, true)); >>>>>>>> if (IS_ERR(vaddr)) { >>>>>>>> i915_vma_unpin_and_release(&vma, 0); >>>>>>>> return PTR_ERR(vaddr); >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>>>> index 2126dd81ac38..56d2144dc6a0 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>>>>>>> @@ -82,7 +82,9 @@ static int intel_huc_rsa_data_create(struct >>>>>>>> intel_huc *huc) >>>>>>>> if (IS_ERR(vma)) >>>>>>>> return PTR_ERR(vma); >>>>>>>> >>>>>>>> - vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>>>> I915_MAP_WB); >>>>>>>> + vaddr = i915_gem_object_pin_map_unlocked(vma->obj, >>>>>>>> + i915_coherent_map_type(gt->i915, >>>>>>>> + vma->obj, true)); >>>>>>>> if (IS_ERR(vaddr)) { >>>>>>>> i915_vma_unpin_and_release(&vma, 0); >>>>>>>> return PTR_ERR(vaddr); >>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>>>>>>> b/drivers/gpu/drm/i915/i915_drv.h >>>>>>>> index 69e43bf91a15..2abbc06712a4 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>>>>>> @@ -78,6 +78,7 @@ >>>>>>>> #include "gem/i915_gem_context_types.h" >>>>>>>> #include "gem/i915_gem_shrinker.h" >>>>>>>> #include "gem/i915_gem_stolen.h" >>>>>>>> +#include "gem/i915_gem_lmem.h" >>>>>>>> >>>>>>>> #include "gt/intel_engine.h" >>>>>>>> #include "gt/intel_gt_types.h" >>>>>>>> @@ -1921,9 +1922,15 @@ static inline int >>>>>>>> intel_hws_csb_write_index(struct drm_i915_private *i915) >>>>>>>> } >>>>>>>> >>>>>>>> static inline enum i915_map_type >>>>>>>> -i915_coherent_map_type(struct drm_i915_private *i915) >>>>>>>> +i915_coherent_map_type(struct drm_i915_private *i915, >>>>>>>> + struct drm_i915_gem_object *obj, bool >>>>>>>> always_coherent) >>>>>>>> { >>>>>>>> - return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; >>>>>>>> + if (i915_gem_object_is_lmem(obj)) >>>>>>>> + return I915_MAP_WC; >>>>>>>> + if (HAS_LLC(i915) || always_coherent) >>>>>>>> + return I915_MAP_WB; >>>>>>>> + else >>>>>>>> + return I915_MAP_WC; >>>>>>> >>>>>>> Seems this patch is doing two things. >>>>>>> >>>>>>> First it is adding lmem support to this helper by always >>>>>>> returning WC >>>>>>> for lmem objects. >>>>>>> >>>>>>> Secondly it is introducing an idea of "always coherent" in a >>>>>>> helper >>>>>>> called i915_coherent_map_type. Could someone explain what is >>>>>>> coherent vs >>>>>>> always coherent? >>>>>>> >>>>>>> And also, why is always coherent happy with WB? Sounds counter >>>>>>> intuitive >>>>>>> to me. >>>>>> >>>>>> All this does is try to keep the existing behaviour intact, whilst >>>>>> also ensuring that all lmem objects are mapped using only WC, no >>>>>> matter what. The always_coherent=true thing is for the existing >>>>>> places >>>>>> where we sometimes map the object using WB, without first >>>>>> considering >>>>>> whether the device has the fast shared LLC vs snooping. Yes, it's >>>>>> slightly ugly :) >>>>> >>>>> Not fully following - if we had to write kerneldoc for >>>>> always_coherent >>>>> input argument - what it would say? >>>> >>>> @always_coherent - If true we should always try to map the object >>>> using WB. If false we should only map as WB if the device >>>> supports the >>>> fast shared LLC, in the case of snooped devices we will map use WC. >>>> Note that If the resource is lmem then we will always map as WC, >>>> regardless of the value of always_coherent, since that's all we >>>> currently support. >>>> >>>> Maybe the naming is poor? >>> >>> Maybe just confusing to me, not sure yet. >>> >>> So always_coherent is not about how the callers wants to use it, >>> but about platform knowledge? Or a performance concern for LLC vs >>> snooping cases? Does WB works (coherently) on snooping platforms? >> >> The always_coherent=true is for the existing callers that want WB, >> regardless of LLC vs snooping. >> >> The other callers use the existing i915_coherent_map_type() which >> only gives out WB for LLC platforms. >> >> AFAIK, LLC vs snooping should offer the same in terms of coherency, >> but in terms of performance the shared LLC is much faster, and so >> for snooping platforms we choose to not enable WB everywhere. >> >> On top of that we now have lmem, but for that we only allow WC. >> This patch just rolls all of that into one helper, while keeping >> the existing behaviour unchanged. > > Thanks. But I am still struggling with the API. :( > > Is the introduction of always_coherent flag in the context of DG1 > required even? AFAICT for lmem objects the flag is ignored so no?

If we drop the flag/helper thing, then we need something like:

type = WB; if (i915_gem_object_is_lmem(obj)) type = WC;

vaddr = i915_gem_object_pin_map(obj, type);

In all the places where we currently do:

vaddr = i915_gem_object_pin_map(obj, WB);

Where obj can be lmem, so ctx, ring, guc etc. Is that better or worse? The existing i915_coherent_map_type() callers should work as-is, since DG1 is snooped. And this patch just extends that to cover all cases.

Perhaps we need a new helper instead? Maybe you have a better idea?

Not yet. Would it make sense to put something in kerneldoc about when callers might choose always_coherent true vs false? In terms of expected usage (frequency, simplicity?) and any rules with regards when callers need to worry about flushing/ordering when there are mixed read and writes?

Hmmm, looking at this again, maybe for now we should just go with:

type = WB; if (i915_gem_object_is_lmem(obj)) type = WC;

vaddr = i915_gem_object_pin_map(obj, type)

Which is way less confusing, plus there are only a handful of places where we need this, so doesn't seem too bad?

Alternatively, we could wrap that in something like:

/* Returns WB for system memory, or WC for local memory */ void *i915_gem_object_pin_map_default(obj);

Thoughts?

I went and looked at the use sites to try and figure it out.

First thing, the bool always_coherent story is only relevant when we decide to place some object in system memory. Otherwise mapping is always WC so I guess our code needs to handle it anyway. Well, if the assumption is that we can change the location of the objects and it all just keeps working? Or that is not the goal?

I guess your concern is that mapping as WC has different semantics, and that might somehow break the caller?

...

Let see about the users (ignoring selftests):

1. lrc_reg_state and ring; always_coherent=false

Update frequency medium and mostly write from the CPU side.

They say always_coherent=false - which means they have to handle being given a WC mapping anyway.

What is the benefit of ever selecting WB here?

2. Engine status page; always_coherent=true

Frequently read and written from the CPU and GPU so cost of snooping is therefore fine? Apart from having to be ready to deal with WC anyway.

3. dbg_poison_ce; always_coherent=true

Writes to lrc_reg_state once - meh. Could just as well always ask for WC.

4. intel_guc_allocate_and_map_vma; always_coherent=true

This one has three users:

a) guc_stage_desc_pool_create stage_desc_pool_vaddr

This one seems write once at init.

b) intel_guc_ct_init

Use for CT communication so similar to CSB on engine status page in principle. But code also has to deal with WC when object is in lmem.

c) intel_guc_ads_create

CPU appears to only write on init and GPU reset.

5. intel_huc_rsa_data_create; always_coheret=true

Called from intel_huc_init so it appears write once from CPU. Not sure why it would need a coherent mapping if that is correct.

I think this exercise left me equally confused. Because flushing and read-write ordering rules are different between WB and WC. And code which accesses all these mappings either has to know which one is in use, or does not care. For the latter case we have to be sure about for every path.

Users of pin_map() are generally meant to call flush_map() where appropriate, which should do the right thing for us. For WC it only needs to flush the wcb. For WB it's more complicated since that depends on if the object is considered coherent or not, if it is then we don't need to do anything, otherwise we need to clflush.

Also note that we If we just map the buffer as WB, that by itself doesn't magically enable snooping for the pages AFAIK. We still have to tell the GPU that these pages are meant to be coherent, which we always do for LLC platforms I think, since the shared LLC is considered fast, whereas on snooping platforms, we don't enable this by default, and have this as CACHE_NONE instead(see shmem_object_init for example), and incur the cost of additional clflushing. Doing an explicit i915_gem_object_set_coherency(I915_CACHE_LLC) I think will mark the object as coherent for us. I think there are also some matching GTT bits for caching.

Also for DG1 you apparently can't disable snooping, as per what Daniel was saying in another thread.

...

The write on init / reset ones are easy enough and it doesn't really matter for them to use the coherent helper.

Lrc_reg_state as well I think can be WC with explicit flushing - it has to on lmem, no?

I doubt it has to be, since the GPU still just accesses it through the GTT.

...

This leaves the status page (CSB, etc) and GuC CT. Those are frequent R/W but also code has to be able to handle WC so what is the benefit of WB? It ends up faster than if it was WC, considering explicit flushes/barriers are still in there?

No idea for GuC, but for the hwsp it's still in system memory, and is WB, even for discrete. Chris measured this to be more performant with our execlists submission path than say just sticking it in lmem, and mapping it as WC.

Ping? How should we proceed with this patch?

I just re-freshed my memory on when the write combine buffer gets flushed and realized uncached reads are also an implicit flush. So my complications from earlier reply were purely mine and I think you can proceed with the patch as is.

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

On 12/04/2021 10:05, Matthew Auld wrote:

From: Anusha Srivatsa anusha.srivatsa@intel.com

In the scenario where local memory is available, we have rely on CPU access via lmem directly instead of aperture.

v2: gmch is only relevant for much older hw, therefore we can drop the has_aperture check since it should always be present on such platforms. (Chris)

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Chris P Wilson chris.p.wilson@intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Cc: CQ Tang cq.tang@intel.com Signed-off-by: Anusha Srivatsa anusha.srivatsa@intel.com

---

drivers/gpu/drm/i915/display/intel_fbdev.c | 22 +++++++++++++++------- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 15 +++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_lmem.h | 5 +++++ drivers/gpu/drm/i915/i915_vma.c | 19 +++++++++++++------ 4 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index 2b37959da747..4af40229f5ec 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -139,14 +139,22 @@ static int intelfb_alloc(struct drm_fb_helper *helper, size = mode_cmd.pitches[0] * mode_cmd.height; size = PAGE_ALIGN(size);

• /* If the FB is too big, just don't use it since fbdev is not very

• * important and we should probably use that space with FBC or other
  

• * features. */
  

  obj = ERR_PTR(-ENODEV);
• if (size * 2 < dev_priv->stolen_usable_size)

• obj = i915_gem_object_create_stolen(dev_priv, size);
  

• if (IS_ERR(obj))

• obj = i915_gem_object_create_shmem(dev_priv, size);
  

• if (HAS_LMEM(dev_priv)) {

• obj = i915_gem_object_create_lmem(dev_priv, size,
  

• 				  I915_BO_ALLOC_CONTIGUOUS);
  

Has to be contiguous? Question for display experts I guess.

[Comes back later.] Ah for iomap? Put a comment to that effect perhaps?

• } else {

• /*
  

•  * If the FB is too big, just don't use it since fbdev is not very
  

•  * important and we should probably use that space with FBC or other
  

•  * features.
  

•  */
  

• if (size * 2 < dev_priv->stolen_usable_size)
  

• 	obj = i915_gem_object_create_stolen(dev_priv, size);
  

• if (IS_ERR(obj))
  

• 	obj = i915_gem_object_create_shmem(dev_priv, size);
  

• }

Could we keep the IS_ERR ordered allocation order to save having to re-indent? Bike shed so optional..

• if (IS_ERR(obj)) { drm_err(&dev_priv->drm, "failed to allocate framebuffer\n"); return PTR_ERR(obj);

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c index 017db8f71130..f44bdd08f7cb 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c @@ -17,6 +17,21 @@ const struct drm_i915_gem_object_ops i915_gem_lmem_obj_ops = { .release = i915_gem_object_release_memory_region, };

+void __iomem * +i915_gem_object_lmem_io_map(struct drm_i915_gem_object *obj,

• 	    unsigned long n,
  

• 	    unsigned long size)
  

+{

• resource_size_t offset;
• GEM_BUG_ON(!i915_gem_object_is_contiguous(obj));
• offset = i915_gem_object_get_dma_address(obj, n);
• offset -= obj->mm.region->region.start;
• return io_mapping_map_wc(&obj->mm.region->iomap, offset, size);

+}

• bool i915_gem_object_is_lmem(struct drm_i915_gem_object *obj) { struct intel_memory_region *mr = obj->mm.region;

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.h b/drivers/gpu/drm/i915/gem/i915_gem_lmem.h index 036d53c01de9..fac6bc5a5ebb 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.h @@ -14,6 +14,11 @@ struct intel_memory_region;

extern const struct drm_i915_gem_object_ops i915_gem_lmem_obj_ops;

+void __iomem * +i915_gem_object_lmem_io_map(struct drm_i915_gem_object *obj,

• 	    unsigned long n,
  

• 	    unsigned long size);
  

• bool i915_gem_object_is_lmem(struct drm_i915_gem_object *obj);

  struct drm_i915_gem_object *

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 07490db51cdc..e24d33aecac4 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -27,6 +27,7 @@

#include "display/intel_frontbuffer.h"

+#include "gem/i915_gem_lmem.h" #include "gt/intel_engine.h" #include "gt/intel_engine_heartbeat.h" #include "gt/intel_gt.h" @@ -448,9 +449,11 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) void __iomem *ptr; int err;

• if (GEM_WARN_ON(!i915_vma_is_map_and_fenceable(vma))) {

• err = -ENODEV;
  

• goto err;
  

• if (!i915_gem_object_is_lmem(vma->obj)) {

• if (GEM_WARN_ON(!i915_vma_is_map_and_fenceable(vma))) {
  

• 	err = -ENODEV;
  

• 	goto err;
  

• }
  

  }

  GEM_BUG_ON(!i915_vma_is_ggtt(vma));

@@ -458,9 +461,13 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)

ptr = READ_ONCE(vma->iomap); if (ptr == NULL) {

• ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap,
  

• 			vma->node.start,
  

• 			vma->node.size);
  

• if (i915_gem_object_is_lmem(vma->obj))
  

• 	ptr = i915_gem_object_lmem_io_map(vma->obj, 0,
  

• 					  vma->obj->base.size);
  

Can the vma size be bigger than the object here? Given how below works of vma->node.size.

• else
  

• 	ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap,
  

• 				vma->node.start,
  

• 				vma->node.size);
  

Looks a bit odd that this calls the same io_mapping_map_wc as i915_gem_object_lmem_io_map ends up doing. Perhaps that suggests there should be a single helper here but I am not sure what would be elegant.

Regards,

Tvrtko

if (ptr == NULL) {
	err = -ENOMEM;
	goto err;

On 16/04/2021 15:25, Matthew Auld wrote:

On 14/04/2021 16:33, Tvrtko Ursulin wrote:

...

On 12/04/2021 10:05, Matthew Auld wrote:

...

From: Anusha Srivatsa anusha.srivatsa@intel.com

In the scenario where local memory is available, we have rely on CPU access via lmem directly instead of aperture.

v2: gmch is only relevant for much older hw, therefore we can drop the has_aperture check since it should always be present on such platforms. (Chris)

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Chris P Wilson chris.p.wilson@intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Cc: CQ Tang cq.tang@intel.com Signed-off-by: Anusha Srivatsa anusha.srivatsa@intel.com

---

drivers/gpu/drm/i915/display/intel_fbdev.c | 22 +++++++++++++++-------   drivers/gpu/drm/i915/gem/i915_gem_lmem.c   | 15 +++++++++++++++   drivers/gpu/drm/i915/gem/i915_gem_lmem.h   |  5 +++++   drivers/gpu/drm/i915/i915_vma.c            | 19 +++++++++++++------   4 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index 2b37959da747..4af40229f5ec 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -139,14 +139,22 @@ static int intelfb_alloc(struct drm_fb_helper *helper,       size = mode_cmd.pitches[0] * mode_cmd.height;       size = PAGE_ALIGN(size); -    /* If the FB is too big, just don't use it since fbdev is not very -     * important and we should probably use that space with FBC or other -     * features. */       obj = ERR_PTR(-ENODEV); -    if (size * 2 < dev_priv->stolen_usable_size) -        obj = i915_gem_object_create_stolen(dev_priv, size); -    if (IS_ERR(obj)) -        obj = i915_gem_object_create_shmem(dev_priv, size); +    if (HAS_LMEM(dev_priv)) { +        obj = i915_gem_object_create_lmem(dev_priv, size, +                          I915_BO_ALLOC_CONTIGUOUS);

Has to be contiguous? Question for display experts I guess.

[Comes back later.] Ah for iomap? Put a comment to that effect perhaps?

I don't think it has to be, since we could in theory just use pin_map() underneath, which can already deal with non-contiguous chunks of lmem, although that might bring in ww locking. I think for now just add a comment and mark this as XXX, and potentially revisit as follow up?

Sure.

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

On Mon, Apr 12, 2021 at 10:05:20AM +0100, Matthew Auld wrote:

From: Clint Taylor clinton.a.taylor@intel.com

Read OPROM SPI through MMIO and find VBT entry since we can't use OpRegion and PCI mapping may not work on some systems due to the BIOS not leaving the Option ROM mapped.

I was surprised to see we still don't have this patch applied. There is some coding style to fix, but if we don't have it we are basically relying on the fallback of using a fake/hardcoded vbt. I will do some fixups and re-submit.

Lucas De Marchi

v2 by Jani:

• switch to intel_uncore_read/intel_uncore_write

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Tomas Winkler tomas.winkler@intel.com Cc: Jon Bloomfield jon.bloomfield@intel.com Signed-off-by: Clint Taylor clinton.a.taylor@intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com

---

drivers/gpu/drm/i915/display/intel_bios.c | 80 +++++++++++++++++++++-- drivers/gpu/drm/i915/i915_reg.h | 8 +++ 2 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index ea4837d485a1..f9dc651f1652 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -2238,6 +2238,66 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size) return vbt; }

+static struct vbt_header *spi_oprom_get_vbt(struct drm_i915_private *i915) +{

• u32 count, data, found, store = 0;
• u32 static_region, oprom_offset;
• u32 oprom_size = 0x200000;
• u16 vbt_size;
• u32 *vbt;
• static_region = intel_uncore_read(&i915->uncore, SPI_STATIC_REGIONS);
• static_region &= OPTIONROM_SPI_REGIONID_MASK;
• intel_uncore_write(&i915->uncore, PRIMARY_SPI_REGIONID, static_region);
• oprom_offset = intel_uncore_read(&i915->uncore, OROM_OFFSET);
• oprom_offset &= OROM_OFFSET_MASK;
• for (count = 0; count < oprom_size; count += 4) {

• intel_uncore_write(&i915->uncore, PRIMARY_SPI_ADDRESS, oprom_offset + count);
  

• data = intel_uncore_read(&i915->uncore, PRIMARY_SPI_TRIGGER);
  

• if (data == *((const u32 *)"$VBT")) {
  

• 	found = oprom_offset + count;
  

• 	break;
  

• }
  

• }
• if (count >= oprom_size)

• goto err_not_found;
  

• /* Get VBT size and allocate space for the VBT */
• intel_uncore_write(&i915->uncore, PRIMARY_SPI_ADDRESS, found +

•    offsetof(struct vbt_header, vbt_size));
  

• vbt_size = intel_uncore_read(&i915->uncore, PRIMARY_SPI_TRIGGER);
• vbt_size &= 0xffff;
• vbt = kzalloc(vbt_size, GFP_KERNEL);
• if (!vbt) {

• DRM_ERROR("Unable to allocate %u bytes for VBT storage\n",
  

• 	  vbt_size);
  

• goto err_not_found;
  

• }
• for (count = 0; count < vbt_size; count += 4) {

• intel_uncore_write(&i915->uncore, PRIMARY_SPI_ADDRESS, found + count);
  

• data = intel_uncore_read(&i915->uncore, PRIMARY_SPI_TRIGGER);
  

• *(vbt + store++) = data;
  

• }
• if (!intel_bios_is_valid_vbt(vbt, vbt_size))

• goto err_free_vbt;
  

• DRM_DEBUG_KMS("Found valid VBT in SPI flash\n");
• return (struct vbt_header *)vbt;

+err_free_vbt:

• kfree(vbt);

+err_not_found:

• return NULL;

+}

static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915) { struct pci_dev *pdev = to_pci_dev(i915->drm.dev); @@ -2287,6 +2347,8 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915)

pci_unmap_rom(pdev, oprom);

• DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
• return vbt;

err_free_vbt: @@ -2321,17 +2383,23 @@ void intel_bios_init(struct drm_i915_private *i915)

init_vbt_defaults(i915);

• /* If the OpRegion does not have VBT, look in PCI ROM. */

• /*

• * If the OpRegion does not have VBT, look in SPI flash through MMIO or
  

• * PCI mapping
  

• */
  

• if (!vbt && IS_DGFX(i915)) {

• oprom_vbt = spi_oprom_get_vbt(i915);
  

• vbt = oprom_vbt;
  

• }
• if (!vbt) { oprom_vbt = oprom_get_vbt(i915);

• if (!oprom_vbt)
  

• 	goto out;
  

• vbt = oprom_vbt;

• drm_dbg_kms(&i915->drm, "Found valid VBT in PCI ROM\n");
  

  }

• if (!vbt)

• goto out;
  

• bdb = get_bdb_header(vbt); i915->vbt.version = bdb->version;

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index da73dc939e58..54ff63b86df6 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -12540,6 +12540,14 @@ enum skl_power_gate { #define DP_PIN_ASSIGNMENT_MASK(idx) (0xf << ((idx) * 4)) #define DP_PIN_ASSIGNMENT(idx, x) ((x) << ((idx) * 4))

+#define PRIMARY_SPI_TRIGGER _MMIO(0x102040) +#define PRIMARY_SPI_ADDRESS _MMIO(0x102080) +#define PRIMARY_SPI_REGIONID _MMIO(0x102084) +#define SPI_STATIC_REGIONS _MMIO(0x102090) +#define OPTIONROM_SPI_REGIONID_MASK REG_GENMASK(7, 0) +#define OROM_OFFSET _MMIO(0x1020c0) +#define OROM_OFFSET_MASK REG_GENMASK(20, 16)

/* This register controls the Display State Buffer (DSB) engines. */ #define _DSBSL_INSTANCE_BASE 0x70B00

#define DSBSL_INSTANCE(pipe, id) (_DSBSL_INSTANCE_BASE + \

2.26.3

---

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

On Mon, 12 Apr 2021, Matthew Auld matthew.auld@intel.com wrote:

From: Anshuman Gupta anshuman.gupta@intel.com

Sanitize OPROM header, CPD signature and OPROM PCI version. OPROM_HEADER, EXPANSION_ROM_HEADER and OPROM_MEU_BLOB structures and PCI struct offsets are provided by GSC counterparts. These are yet to be Documented in B.Spec. After successful sanitization, extract VBT from opregion image.

So I don't understand what the point is with two consecutive patches where the latter rewrites a lot of the former.

BR, Jani.

v2:

• Used macro for OPROM header magic 0xaa55 [Rodrigo]
• Added a OPROM layout. [Uma]
• Extract opregion from OPROM package and then extract VBT from opregion to have backward compatibility with older IFWI.

v3:

• Moved opreg stuff to intel_opregion.{c,h}. [Uma]
• Memory leak and intel_oprom_verify_signature return value fixes. [Uma]

v4:

• Fix return code storage for oprom_image_parse_helper (Matt)

v5 by Jani:

• switch to intel_uncore_read/intel_uncore_write

v6 by Khajapasha:

• Rename intel_oprom_verify_signature() to intel_spi_get_oprom_opreg() [Jani, Nikula]
• Use u32 data type for opregion size [Jani, Nikula]

Cc: Jani Nikula jani.nikula@intel.com Cc: Uma Shankar uma.shankar@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Signed-off-by: Mohammed Khajapasha mohammed.khajapasha@intel.com

---

drivers/gpu/drm/i915/display/intel_bios.c | 47 +++-- drivers/gpu/drm/i915/display/intel_opregion.c | 169 ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_opregion.h | 38 +++- 3 files changed, 227 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index f9dc651f1652..59eec8333723 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -2240,37 +2240,36 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)

static struct vbt_header *spi_oprom_get_vbt(struct drm_i915_private *i915) {

• u32 count, data, found, store = 0;
• u32 static_region, oprom_offset;
• u32 oprom_size = 0x200000;

• u32 count, found, opreg_size;
• u32 *vbt, *oprom_opreg = NULL; u16 vbt_size;

• u32 *vbt;

• u8 *parse_ptr;

• static_region = intel_uncore_read(&i915->uncore, SPI_STATIC_REGIONS);
• static_region &= OPTIONROM_SPI_REGIONID_MASK;
• intel_uncore_write(&i915->uncore, PRIMARY_SPI_REGIONID, static_region);
• oprom_offset = intel_uncore_read(&i915->uncore, OROM_OFFSET);
• oprom_offset &= OROM_OFFSET_MASK;

• if (intel_spi_get_oprom_opreg(i915, &oprom_opreg, &opreg_size)) {

• drm_err(&i915->drm, "oprom signature verification failed\n");
  

• goto err_not_found;
  

• }

• for (count = 0; count < oprom_size; count += 4) {

• intel_uncore_write(&i915->uncore, PRIMARY_SPI_ADDRESS, oprom_offset + count);
  

• data = intel_uncore_read(&i915->uncore, PRIMARY_SPI_TRIGGER);
  

• if (!oprom_opreg) {

• drm_err(&i915->drm, "opregion not found\n");
  

• goto err_not_found;
  

• }

• if (data == *((const u32 *)"$VBT")) {
  

• 	found = oprom_offset + count;
  

• for (count = 0; count < opreg_size; count += 4) {

• if (oprom_opreg[count / 4] == *((const u32 *)"$VBT")) {
  

• 	found = count;
  break;
  

  } }

• if (count >= oprom_size)

• if (count >= opreg_size) {

• drm_err(&i915->drm, "VBT not found in opregion\n");
  

  goto err_not_found;

• }

  /* Get VBT size and allocate space for the VBT */

• intel_uncore_write(&i915->uncore, PRIMARY_SPI_ADDRESS, found +

•    offsetof(struct vbt_header, vbt_size));
  

• vbt_size = intel_uncore_read(&i915->uncore, PRIMARY_SPI_TRIGGER);
• vbt_size &= 0xffff;

• parse_ptr = (u8 *)oprom_opreg + found;

• vbt_size = ((struct vbt_header *)parse_ptr)->vbt_size;

  vbt = kzalloc(vbt_size, GFP_KERNEL); if (!vbt) {

@@ -2279,16 +2278,12 @@ static struct vbt_header *spi_oprom_get_vbt(struct drm_i915_private *i915) goto err_not_found; }

• for (count = 0; count < vbt_size; count += 4) {

• intel_uncore_write(&i915->uncore, PRIMARY_SPI_ADDRESS, found + count);
  

• data = intel_uncore_read(&i915->uncore, PRIMARY_SPI_TRIGGER);
  

• *(vbt + store++) = data;
  

• }

• memcpy(vbt, parse_ptr, vbt_size); if (!intel_bios_is_valid_vbt(vbt, vbt_size)) goto err_free_vbt;

  DRM_DEBUG_KMS("Found valid VBT in SPI flash\n");

• kfree(oprom_opreg);

  return (struct vbt_header *)vbt;

diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c index dfd724e506b5..e9ccd8265a1f 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.c +++ b/drivers/gpu/drm/i915/display/intel_opregion.c @@ -983,6 +983,175 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) return err; }

+static int oprom_image_parse_helper(u8 *parse_ptr, u8 *last_img, u8 *code_type,

• 		    struct drm_i915_private *i915)
  

+{

• u8 size_512_bytes;
• if (((union oprom_header *)parse_ptr)->signature != OPROM_IMAGE_MAGIC) {

• drm_err(&i915->drm, "Wrong OPROM header signature.\n");
  

• return -EINVAL;
  

• }
• size_512_bytes = parse_ptr[((struct expansion_rom_header *)parse_ptr)->pcistructoffset + PCI_IMAGE_LENGTH_OFFSET];
• *code_type = parse_ptr[((struct expansion_rom_header *)parse_ptr)->pcistructoffset + PCI_CODE_TYPE_OFFSET];
• *last_img = parse_ptr[((struct expansion_rom_header *)parse_ptr)->pcistructoffset + PCI_LAST_IMAGE_INDICATOR_OFFSET];
• return size_512_bytes;

+}

+static void spi_read_oprom_helper(size_t len, u32 offset, u32 *buf,

• 		  struct drm_i915_private *dev_priv)
  

+{

• u32 count, data;
• for (count = 0; count < len; count += 4) {

• intel_uncore_write(&dev_priv->uncore, PRIMARY_SPI_ADDRESS, offset + count);
  

• data = intel_uncore_read(&dev_priv->uncore, PRIMARY_SPI_TRIGGER);
  

• buf[count / 4] = data;
  

• }

+}

+/**

• • •    DASH+G OPROM IMAGE LAYOUT           +
      

• • +--------+-------+---------------------------+
• • | Offset | Value | ROM Header Fields +-----> Image 1 (CSS)
• • +--------------------------------------------+
• • | 0h | 55h | ROM Signature Byte1 |
• • | 1h | AAh | ROM Signature Byte2 |
• • | 2h | xx | Reserved |
• • | 18+19h| xx | Ptr to PCI DataStructure |
• • +----------------+---------------------------+
• • | PCI Data Structure |
• • +--------------------------------------------+
• • | . . . |
• • | . . . |
• • | 10 + xx + Image Length |
• • | 14 + xx + Code Type |
• • | 15 + xx + Last Image Indicator |
• • | . . . |
• • +--------------------------------------------+
• • | MEU BLOB |
• • +--------------------------------------------+
• • | CPD Header |
• • | CPD Entry |
• • | Reserved |
• • | SignedDataPart1 |
• • | PublicKey |
• • | RSA Signature |
• • | SignedDataPart2 |
• • | IFWI Metadata |
• • +--------+-------+---------------------------+
• • | . | . | . |
• • | . | . | . |
• • +--------------------------------------------+
• • | Offset | Value | ROM Header Fields +-----> Image 2 (Config Data) (Offset: 0x800)
• • +--------------------------------------------+
• • | 0h | 55h | ROM Signature Byte1 |
• • | 1h | AAh | ROM Signature Byte2 |
• • | 2h | xx | Reserved |
• • | 18+19h| xx | Ptr to PCI DataStructure |
• • +----------------+---------------------------+
• • | PCI Data Structure |
• • +--------------------------------------------+
• • | . . . |
• • | . . . |
• • | 10 + xx + Image Length |
• • | 14 + xx + Code Type |
• • | 15 + xx + Last Image Indicator |
• • | . . . |
• • | 1A + 3C + Ptr to Opregion Signature |
• • | . . . |
• • | . . . |
• • | 83Ch + IntelGraphicsMem | <---+ Opregion Signature
• • +--------+-----------------------------------+
• • intel_spi_get_oprom_opreg() get OPROM image.
• • @i915: pointer to i915 device.
• • @opreg: pointer to opregion buffer output.
• • @opreg_size: pointer to opregion size output.
• */

+int +intel_spi_get_oprom_opreg(struct drm_i915_private *i915, u32 **opreg,

• 	  u32 *opreg_size)
  

+{

• u8 img_sig[sizeof(OPREGION_SIGNATURE)];
• u8 code_type, last_img;
• u32 static_region, offset, img_len;
• u32 *oprom_img, *oprom_img_hdr;
• u16 opreg_base;
• u8 *parse_ptr;
• int img_size;
• int ret = -EINVAL;
• /* initialize SPI to read the OPROM */
• static_region = intel_uncore_read(&i915->uncore, SPI_STATIC_REGIONS);
• static_region &= OPTIONROM_SPI_REGIONID_MASK;
• intel_uncore_write(&i915->uncore, PRIMARY_SPI_REGIONID, static_region);
• /* read OPROM offset in SPI flash */
• offset = intel_uncore_read(&i915->uncore, OROM_OFFSET);
• offset &= OROM_OFFSET_MASK;
• oprom_img_hdr = kzalloc(OPROM_INITIAL_READ_SIZE, GFP_KERNEL);
• if (!oprom_img_hdr)

• return -ENOMEM;
  

• do {

• spi_read_oprom_helper(OPROM_INITIAL_READ_SIZE, offset,
  

• 		      oprom_img_hdr, i915);
  

• img_size = oprom_image_parse_helper((u8 *)oprom_img_hdr, &last_img,
  

• 				    &code_type, i915);
  

• if (img_size <= 0) {
  

• 	ret = -EINVAL;
  

• 	goto err_free_hdr;
  

• }
  

• img_len = img_size * OPROM_BYTE_BOUNDARY;
  

• oprom_img = kzalloc(img_len, GFP_KERNEL);
  

• if (!oprom_img) {
  

• 	ret = -ENOMEM;
  

• 	goto err_free_hdr;
  

• }
  

• spi_read_oprom_helper(img_len, offset, oprom_img, i915);
  

• parse_ptr = (u8 *)oprom_img;
  

• offset = offset + img_len;
  

• /* opregion base offset */
  

• opreg_base = ((struct expansion_rom_header *)parse_ptr)->opregion_base;
  

• /* CPD or opreg signature is present at opregion_base offset */
  

• memcpy(img_sig, parse_ptr + opreg_base, sizeof(OPREGION_SIGNATURE));
  

• if (!memcmp(img_sig, OPREGION_SIGNATURE, sizeof(OPREGION_SIGNATURE) - 1)) {
  

• 	*opreg = oprom_img;
  

• 	*opreg_size = img_len;
  

• 	drm_dbg_kms(&i915->drm, "Found opregion image\n");
  

• 	ret = 0;
  

• 	break;
  

• } else if (!memcmp(img_sig, CPD_SIGNATURE, NUM_CPD_BYTES)) {
  

• 	if (code_type != OPROM_CSS_CODE_TYPE) {
  

• 		drm_err(&i915->drm, "Invalid OPROM\n");
  

• 		ret = -EINVAL;
  

• 		goto err_free_img;
  

• 	}
  

• 	drm_dbg_kms(&i915->drm, "Found CSS image\n");
  

• 	/* proceed here onwards for signature authentication */
  

• 	kfree(oprom_img);
  

• 	continue;
  

• }
  

• } while (last_img != LAST_IMG_INDICATOR);
• return ret;

+err_free_img:

• kfree(oprom_img);

+err_free_hdr:

• kfree(oprom_img_hdr);
• return ret;

+}

static int intel_use_opregion_panel_type_callback(const struct dmi_system_id *id) { DRM_INFO("Using panel type from OpRegion on %s\n", id->ident); diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h index 4aa68ffbd30e..de53dde10dd9 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.h +++ b/drivers/gpu/drm/i915/display/intel_opregion.h @@ -54,6 +54,34 @@ struct intel_opregion {

#define OPREGION_SIZE (8 * 1024)

+#define CPD_SIGNATURE "$CPD" /* CPD Signature */ +#define NUM_CPD_BYTES 4 +#define PCI_IMAGE_LENGTH_OFFSET 0x10 +#define PCI_CODE_TYPE_OFFSET 0x14 +#define PCI_LAST_IMAGE_INDICATOR_OFFSET 0x15 +#define LAST_IMG_INDICATOR 0x80 +#define OPROM_IMAGE_MAGIC 0xAA55 /* Little Endian */ +#define OPROM_CSS_CODE_TYPE 0xF0 +#define OPROM_BYTE_BOUNDARY 512 /* OPROM image sizes are indicated in 512 byte boundaries */ +#define OPROM_INITIAL_READ_SIZE 60 /* Read 60 bytes to compute the Img Len from PCI structure */

+union oprom_header {

• u32 data;
• struct {

• u16 signature;  /* Offset[0x0]: Header 0x55 0xAA */
  

• u8 sizein512bytes;
  

• u8 reserved;
  

• };

+};

+struct expansion_rom_header {

• union oprom_header header; /* Offset[0x0]: Oprom Header */
• u16 vbiospostoffset; /* Offset[0x4]: pointer to VBIOS entry point */
• u8 resvd[0x12];
• u16 pcistructoffset; /* Offset[0x18]: Contains pointer PCI Data Structure */
• u16 opregion_base; /* Offset[0x1A]: Offset to Opregion Base start */

+};

#ifdef CONFIG_ACPI

int intel_opregion_setup(struct drm_i915_private *dev_priv); @@ -72,6 +100,9 @@ int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, pci_power_t state); int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv);

+int intel_spi_get_oprom_opreg(struct drm_i915_private *i915, u32 **opreg,

• 	      u32 *opreg_size);
  

#else /* CONFIG_ACPI*/

static inline int intel_opregion_setup(struct drm_i915_private *dev_priv) @@ -117,6 +148,11 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) return -ENODEV; }

-#endif /* CONFIG_ACPI */ +static int intel_spi_get_oprom_opreg(struct drm_i915_private *i915, u32 **opreg,

• 		     u32 *opreg_size)
  

+{

• return 0;

+}

+#endif /* CONFIG_ACPI */ #endif