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

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

 




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

Signed-off-by: CQ Tang <cq.tang@xxxxxxxxx>
Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
---
  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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux