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

2021-04-19 Thread Tvrtko Ursulin 


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 

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 
Signed-off-by: Matthew Auld 
---
  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 
  #include 
+#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 = >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(_graphics_stolen_res) == 0)
+    if (resource_size(>region) == 0)
  return 0;
-    i915->dsm = intel_graphics_stolen_res;
+    i915->dsm = mem->region;
  if (i915_adjust_stolen(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(>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(>iomap,
+    mem->io_start,
+    resource_size(>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

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

2021-04-16 Thread Matthew Auld 

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


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

From: CQ Tang 

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 
Signed-off-by: Matthew Auld 
---
  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 
  #include 
+#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 = >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(_graphics_stolen_res) == 0)
+    if (resource_size(>region) == 0)
  return 0;
-    i915->dsm = intel_graphics_stolen_res;
+    i915->dsm = mem->region;
  if (i915_adjust_stolen(i915, >dsm))
  return 0;
@@ -684,23 +694,36 @@ static int _i915_gem_object_stolen_init(struct 
intel_memory_region *mem,

  return ret;
  }
+struct intel_memory_region *i915_stolen_region(struct 
drm_i915_private *i915)

+{
+    if (HAS_LMEM(i915))
+    return i915->mm.regions[INTEL_REGION_STOLEN_LMEM];
+
+    return i915->mm.regions[INTEL_REGION_STOLEN_SMEM];
+}


Could be a bikeshedding comment only - especially since I think this 
path gets very little used at runtime so it is most likely pointless to 
fiddle with it, but it just strikes me a bit not fully elegant to do:


i915_gem_object_create_stolen
  -> i915_gem_object_create_region
     -> i915_stolen_region

And end up in here, when alternative could be at driver init:

i915->stolen_region_id = HAS_LMEM() ? ... : ...;

i915_gem_object_create_stolen
  -> 
i915_gem_object_create_region(i915->mm.regions[i915->stolen_region_id]);


Or pointer to region. Would avoid having to export i915_stolen_region as 
well.


Or is i915->dsm already the right thing? Because..


I guess we could just have an i915->stolen_region short-cut or something?




+
  struct drm_i915_gem_object *
  i915_gem_object_create_stolen(struct drm_i915_private *i915,
    resource_size_t size)
  {
-    return 
i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM],

+    return i915_gem_object_create_region(i915_stolen_region(i915),
   size, I915_BO_ALLOC_CONTIGUOUS);
  }
  static int init_stolen(struct intel_memory_region *mem)
  {
-    intel_memory_region_set_name(mem, "stolen");
+    if (HAS_LMEM(mem->i915)) {
+    if (!io_mapping_init_wc(>iomap,
+    mem->io_start,
+    resource_size(>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

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

2021-04-14 Thread Tvrtko Ursulin 



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

From: CQ Tang 

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 
Signed-off-by: Matthew Auld 
---
  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 
  #include 
  
+#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 = >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(_graphics_stolen_res) == 0)

+   if (resource_size(>region) == 0)
return 0;
  
-	i915->dsm = intel_graphics_stolen_res;

+   i915->dsm = mem->region;
  
  	if (i915_adjust_stolen(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(>iomap,
+   mem->io_start,
+   resource_size(>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