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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20/04/2021 17:06, Tvrtko Ursulin wrote:

On 20/04/2021 14:18, Matthew Auld wrote:
From: CQ Tang <cq.tang@xxxxxxxxx>

Add "REGION_STOLEN" device info to dg1, create stolen memory
region from upper portion of local device memory, starting
from DSMBASE.

v2:
     - s/drm_info/drm_dbg; userspace likely doesn't care about stolen.
     - mem->type is only setup after the region probe, so setting the name        as stolen-local or stolen-system based on this value won't work. Split
       system vs local stolen setup to fix this.
     - kill all the region->devmem/is_devmem stuff. We already differentiate
       the different types of stolen so such things shouldn't be needed
       anymore.
v3:
     - split stolen lmem vs smem ops(Tvrtko)
     - add shortcut for stolen region in i915(Tvrtko)
     - sanity check dsm base vs bar size(Xinyun)

Signed-off-by: CQ Tang <cq.tang@xxxxxxxxx>
Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Cc: Xinyun Liu <xinyun.liu@xxxxxxxxx>
---
  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 137 ++++++++++++++++++---
  drivers/gpu/drm/i915/i915_drv.h            |   7 ++
  drivers/gpu/drm/i915/i915_pci.c            |   2 +-
  drivers/gpu/drm/i915/i915_reg.h            |   1 +
  drivers/gpu/drm/i915/intel_memory_region.c |   8 ++
  drivers/gpu/drm/i915/intel_memory_region.h |   5 +-
  6 files changed, 140 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index b0597de206de..2ed1ca9aec75 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -10,6 +10,7 @@
  #include <drm/drm_mm.h>
  #include <drm/i915_drm.h>
+#include "gem/i915_gem_lmem.h"
  #include "gem/i915_gem_region.h"
  #include "i915_drv.h"
  #include "i915_gem_stolen.h"
@@ -121,6 +122,14 @@ static int i915_adjust_stolen(struct drm_i915_private *i915,
          }
      }
+    /*
+     * With device local memory, we don't need to check the address range,
+     * this is device memory physical address, could overlap with system
+     * memory.
+     */
+    if (HAS_LMEM(i915))
+        return 0;

The grammar in the comment is a bit hard to parse for me, but more importantly, this is now not on the device stolen path, right?

[Comes back later, hm no, still called okay at least there is a comment now explaining which are the relevant bits.]

+
      /*
       * Verify that nothing else uses this physical address. Stolen
       * memory should be reserved by the BIOS and hidden from the
@@ -374,8 +383,9 @@ static void icl_get_stolen_reserved(struct drm_i915_private *i915,
      }
  }
-static int i915_gem_init_stolen(struct drm_i915_private *i915)
+static int i915_gem_init_stolen(struct intel_memory_region *mem)
  {
+    struct drm_i915_private *i915 = mem->i915;
      struct intel_uncore *uncore = &i915->uncore;
      resource_size_t reserved_base, stolen_top;
      resource_size_t reserved_total, reserved_size;
@@ -396,10 +406,10 @@ static int i915_gem_init_stolen(struct drm_i915_private *i915)
          return 0;
      }
-    if (resource_size(&intel_graphics_stolen_res) == 0)
+    if (resource_size(&mem->region) == 0)
          return 0;
-    i915->dsm = intel_graphics_stolen_res;
+    i915->dsm = mem->region;
      if (i915_adjust_stolen(i915, &i915->dsm))
          return 0;
@@ -688,39 +698,130 @@ struct drm_i915_gem_object *
  i915_gem_object_create_stolen(struct drm_i915_private *i915,
                    resource_size_t size)
  {
-    return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM],
+    return i915_gem_object_create_region(i915->mm.stolen_region,
                           size, I915_BO_ALLOC_CONTIGUOUS);
  }
-static int init_stolen(struct intel_memory_region *mem)
+static int init_stolen_smem(struct intel_memory_region *mem)
  {
-    intel_memory_region_set_name(mem, "stolen");
-
      /*
       * Initialise stolen early so that we may reserve preallocated
       * objects for the BIOS to KMS transition.
       */
-    return i915_gem_init_stolen(mem->i915);
+    return i915_gem_init_stolen(mem);
+}
+
+static void release_stolen_smem(struct intel_memory_region *mem)
+{
+    i915_gem_cleanup_stolen(mem->i915);
+}
+
+static const struct intel_memory_region_ops i915_region_stolen_smem_ops = {
+    .init = init_stolen_smem,
+    .release = release_stolen_smem,
+    .init_object = _i915_gem_object_stolen_init,
+};
+
+static int init_stolen_lmem(struct intel_memory_region *mem)
+{
+    int err;
+
+    if (GEM_WARN_ON(resource_size(&mem->region) == 0))
+        return -ENODEV;
+
+    if (!io_mapping_init_wc(&mem->iomap,
+                mem->io_start,
+                resource_size(&mem->region)))
+        return -EIO;
+
+    /*
+     * For stolen lmem we mostly just care about populating the dsm related
+     * bits and setting up the drm_mm allocator for the range.
+     */
+    err = i915_gem_init_stolen(mem);

Ideally we would be able to split this into two so there would be no further smem/lmem stolen forking inside it. That way we would also avoid the "mostly" and reach total clarity but okay, can leave for later.

I'll add a TODO comment :)


+    if (err)
+        goto err_fini;
+
+    return 0;
+
+err_fini:
+    io_mapping_fini(&mem->iomap);
+    return err;
  }
-static void release_stolen(struct intel_memory_region *mem)
+static void release_stolen_lmem(struct intel_memory_region *mem)
  {
+    io_mapping_fini(&mem->iomap);
      i915_gem_cleanup_stolen(mem->i915);
  }
-static const struct intel_memory_region_ops i915_region_stolen_ops = {
-    .init = init_stolen,
-    .release = release_stolen,
+static const struct intel_memory_region_ops i915_region_stolen_lmem_ops = {
+    .init = init_stolen_lmem,
+    .release = release_stolen_lmem,
      .init_object = _i915_gem_object_stolen_init,
  };
+static struct intel_memory_region *
+setup_lmem_stolen(struct drm_i915_private *i915)
+{
+    struct intel_uncore *uncore = &i915->uncore;
+    struct pci_dev *pdev = i915->drm.pdev;
+    struct intel_memory_region *mem;
+    resource_size_t io_start;
+    resource_size_t lmem_size;
+    u64 lmem_base;
+
+    if (!IS_DGFX(i915))
+        return ERR_PTR(-ENODEV);

Is this check needed? Looks like the caller will only call this based on HAS_REGION. GEM_DEBUG_WARN_ON(!IS_DGFX())?

+
+    lmem_base = intel_uncore_read64(uncore, GEN12_DSMBASE);
+    if (GEM_WARN_ON(lmem_base >= pci_resource_len(pdev, 2)))
+        return ERR_PTR(-ENODEV);
+
+    lmem_size = pci_resource_len(pdev, 2) - lmem_base;
+    io_start = pci_resource_start(pdev, 2) + lmem_base;
+
+    mem = intel_memory_region_create(i915, lmem_base, lmem_size,
+                     I915_GTT_PAGE_SIZE_4K, io_start,
+                     &i915_region_stolen_lmem_ops);
+    if (IS_ERR(mem))
+        return mem;
+
+    drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n",
+        &mem->io_start);

Printouts I'd prefer if they were done by the common code which calls region->init(). Afterwards it could generically print all the region data with common formatting and using the set region name. Optional, later, is fine.

Yeah, having a common helper to print everything interesting in intel_memory_region might be quite nice. I'll add a TODO.


+
+    intel_memory_region_set_name(mem, "stolen-local");

Should the name just be passed in to intel_memory_region_create or there is a good reason for it to be a follow up step?

I don't see a good reason, so yeah it looks like we can just pass it along. I don't have a strong opinion here.


+
+    return mem;
+}
+
+static struct intel_memory_region*
+setup_smem_stolen(struct drm_i915_private *i915)
+{
+    struct intel_memory_region *mem;
+
+    mem = intel_memory_region_create(i915,
+                     intel_graphics_stolen_res.start,
+                     resource_size(&intel_graphics_stolen_res),
+                     PAGE_SIZE, 0,
+                     &i915_region_stolen_smem_ops);
+    if (IS_ERR(mem))
+        return mem;
+
+    intel_memory_region_set_name(mem, "stolen-system");
+
+    return mem;
+}
+
  struct intel_memory_region *i915_gem_stolen_setup(struct drm_i915_private *i915)
  {
-    return intel_memory_region_create(i915,
-                      intel_graphics_stolen_res.start,
-                      resource_size(&intel_graphics_stolen_res),
-                      PAGE_SIZE, 0,
-                      &i915_region_stolen_ops);
+    struct intel_memory_region *mem;
+
+    mem = setup_lmem_stolen(i915);
+    if (mem == ERR_PTR(-ENODEV))
+        mem = setup_smem_stolen(i915);

This helper is possibly not needed any more since the caller is a switch statement with a fallthrough. So eliminate the falltrough and call the correct setup directly from there? There is the i915->mm.stolen assignment to be duplicated in that case so up to you.

Yes, let's go with that.


+
+    return mem;
  }
  struct drm_i915_gem_object *
@@ -728,7 +829,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915,
                             resource_size_t stolen_offset,
                             resource_size_t size)
  {
-    struct intel_memory_region *mem = i915->mm.regions[INTEL_REGION_STOLEN_SMEM];
+    struct intel_memory_region *mem = i915->mm.stolen_region;
      struct drm_i915_gem_object *obj;
      struct drm_mm_node *stolen;
      int ret;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e20294e9227a..0b44333eb703 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -514,6 +514,13 @@ struct intel_l3_parity {
  };
  struct i915_gem_mm {
+    /*
+     * Shortcut for the stolen region. This points to either
+     * INTEL_REGION_STOLEN_SMEM for integrated platforms, or
+     * INTEL_REGION_STOLEN_LMEM for discrete, or NULL if the device doesn't
+     * support stolen.
+     */
+    struct intel_memory_region *stolen_region;
      /** Memory allocator for GTT stolen memory */
      struct drm_mm stolen;
      /** Protects the usage of the GTT stolen memory allocator. This is
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 44e7b94db63d..d4673e43a46d 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -908,7 +908,7 @@ static const struct intel_device_info rkl_info = {
  };
  #define DGFX_FEATURES \
-    .memory_regions = REGION_SMEM | REGION_LMEM, \
+    .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM, \
      .has_master_unit_irq = 1, \
      .has_llc = 0, \
      .has_snoop = 1, \
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f80d656331f4..ea20058bc13f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -12191,6 +12191,7 @@ enum skl_power_gate {
  #define GEN12_GLOBAL_MOCS(i)    _MMIO(0x4000 + (i) * 4) /* Global MOCS regs */
  #define GEN12_GSMBASE            _MMIO(0x108100)
+#define GEN12_DSMBASE            _MMIO(0x1080C0)
  /* gamt regs */
  #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4)
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index bf837b6bb185..9941a4a07fde 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -22,6 +22,10 @@ static const struct {
          .class = INTEL_MEMORY_STOLEN_SYSTEM,
          .instance = 0,
      },
+    [INTEL_REGION_STOLEN_LMEM] = {
+        .class = INTEL_MEMORY_STOLEN_LOCAL,
+        .instance = 0,
+    },
  };
  struct intel_memory_region *
@@ -278,8 +282,12 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
          case INTEL_MEMORY_SYSTEM:
              mem = i915_gem_shmem_setup(i915);
              break;
+        case INTEL_MEMORY_STOLEN_LOCAL:
+            fallthrough;
          case INTEL_MEMORY_STOLEN_SYSTEM:
              mem = i915_gem_stolen_setup(i915);
+            if (!IS_ERR(mem))
+                i915->mm.stolen_region = mem;
              break;
          default:
              continue;
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index edd49067c8ca..4c8ec15af55f 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -26,18 +26,21 @@ enum intel_memory_type {
      INTEL_MEMORY_SYSTEM = 0,
      INTEL_MEMORY_LOCAL,
      INTEL_MEMORY_STOLEN_SYSTEM,
+    INTEL_MEMORY_STOLEN_LOCAL,
  };
  enum intel_region_id {
      INTEL_REGION_SMEM = 0,
      INTEL_REGION_LMEM,
      INTEL_REGION_STOLEN_SMEM,
+    INTEL_REGION_STOLEN_LMEM,
      INTEL_REGION_UNKNOWN, /* Should be last */
  };
  #define REGION_SMEM     BIT(INTEL_REGION_SMEM)
  #define REGION_LMEM     BIT(INTEL_REGION_LMEM)
  #define REGION_STOLEN_SMEM   BIT(INTEL_REGION_STOLEN_SMEM)
+#define REGION_STOLEN_LMEM   BIT(INTEL_REGION_STOLEN_LMEM)
  #define I915_ALLOC_MIN_PAGE_SIZE  BIT(0)
  #define I915_ALLOC_CONTIGUOUS     BIT(1)
@@ -82,7 +85,7 @@ struct intel_memory_region {
      u16 type;
      u16 instance;
      enum intel_region_id id;
-    char name[8];
+    char name[16];
      struct list_head reserved;

Regards,

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux