Re: [PATCH 03/10] drm/i915/uapi: expose the avail tracking

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

 



On 26/05/2022 09:33, Tvrtko Ursulin wrote:

On 26/05/2022 09:10, Matthew Auld wrote:
On 26/05/2022 08:58, Tvrtko Ursulin wrote:

On 25/05/2022 19:43, Matthew Auld wrote:
Vulkan would like to have a rough measure of how much device memory can
in theory be allocated. Also add unallocated_cpu_visible_size to track
the visible portion, in case the device is using small BAR.

I have concerns here that it isn't useful and could even be counter-productive. If we give unprivileged access it may be considered a side channel, but if we "lie" (report total region size) to unprivileged clients (like in this patch), then they don't co-operate well and end trashing.

Is Vulkan really sure it wants this and why?

Lionel pointed out: https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VK_EXT_memory_budget.html


So this query would provide VkPhysicalDeviceMemoryBudgetPropertiesEXT::heapBudget. Apart that it wouldn't since we thought to lie. And apart that it's text says:

"""
...how much total memory from each heap the current process can use at any given time, before allocations may start failing or causing performance degradation. The values may change based on other activity in the system that is outside the scope and control of the Vulkan implementation.
"""

It acknowledges itself in the second sentence that the first sentence is questionable.

And VkPhysicalDeviceMemoryBudgetPropertiesEXT::heapUsage would be still missing and would maybe come via fdinfo? Or you plan to add it to this same query later?

IIUC the heapUsage is something like per app usage, which already looks to be tracked in anv, although I don't think it knows if stuff is actually resident or not. The heapBudget looks to then be roughly the heapUsage + info.unallocated.


I like to source knowledge of best practices from the long established world of CPU scheduling and process memory management. Is anyone aware of this kind of techniques there - applications actively looking at free memory data from /proc/meminfo and dynamically adjusting their runtime behaviour based on it? And that they are not single application on a dedicated system type of things.

Or perhaps this Vk extension is envisaged for exactly those kind of scenarios? However if so then userspace can know all this data from probed size and the data set it created.

Also note that the existing behaviour was to lie. I'm not sure what's the best option here.

Uapi reserved -1 for unknown so we could do that?

AFAICT we can do that for the info.unallocated_cpu_visible, but not for the existing info.unallocated without maybe breaking something?


Regards,

Tvrtko


Regards,

Tvrtko

Testcase: igt@i915_query@query-regions-unallocated
Testcase: igt@i915_query@query-regions-sanity-check
Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Cc: Jordan Justen <jordan.l.justen@xxxxxxxxx>
Cc: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
Cc: Akeem G Abodunrin <akeem.g.abodunrin@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_query.c             | 10 +++++-
  drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 ++++++++++++
  drivers/gpu/drm/i915/i915_ttm_buddy_manager.h |  3 ++
  drivers/gpu/drm/i915/intel_memory_region.c    | 14 +++++++++
  drivers/gpu/drm/i915/intel_memory_region.h    |  3 ++
  include/uapi/drm/i915_drm.h                   | 31 ++++++++++++++++++-
  6 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 9aa0b28aa6ee..e095c55f4d4b 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -502,7 +502,15 @@ static int query_memregion_info(struct drm_i915_private *i915,
          else
              info.probed_cpu_visible_size = mr->total;
-        info.unallocated_size = mr->avail;
+        if (perfmon_capable()) {
+            intel_memory_region_avail(mr,
+                          &info.unallocated_size,
+                          &info.unallocated_cpu_visible_size);
+        } else {
+            info.unallocated_size = info.probed_size;
+            info.unallocated_cpu_visible_size =
+                info.probed_cpu_visible_size;
+        }
          if (__copy_to_user(info_ptr, &info, sizeof(info)))
              return -EFAULT;
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index a5109548abc0..aa5c91e44438 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -365,6 +365,26 @@ u64 i915_ttm_buddy_man_visible_size(struct ttm_resource_manager *man)
      return bman->visible_size;
  }
+/**
+ * i915_ttm_buddy_man_visible_size - Query the avail tracking for the manager.
+ *
+ * @man: The buddy allocator ttm manager
+ * @avail: The total available memory in pages for the entire manager.
+ * @visible_avail: The total available memory in pages for the CPU visible + * portion. Note that this will always give the same value as @avail on
+ * configurations that don't have a small BAR.
+ */
+void i915_ttm_buddy_man_avail(struct ttm_resource_manager *man,
+                 u64 *avail, u64 *visible_avail)
+{
+    struct i915_ttm_buddy_manager *bman = to_buddy_manager(man);
+
+    mutex_lock(&bman->lock);
+    *avail = bman->mm.avail >> PAGE_SHIFT;
+    *visible_avail = bman->visible_avail;
+    mutex_unlock(&bman->lock);
+}
+
  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
  void i915_ttm_buddy_man_force_visible_size(struct ttm_resource_manager *man,
                         u64 size)
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
index 52d9586d242c..d64620712830 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
@@ -61,6 +61,9 @@ int i915_ttm_buddy_man_reserve(struct ttm_resource_manager *man,   u64 i915_ttm_buddy_man_visible_size(struct ttm_resource_manager *man);
+void i915_ttm_buddy_man_avail(struct ttm_resource_manager *man,
+                  u64 *avail, u64 *avail_visible);
+
  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
  void i915_ttm_buddy_man_force_visible_size(struct ttm_resource_manager *man,
                         u64 size);
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index e38d2db1c3e3..94ee26e99549 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -279,6 +279,20 @@ void intel_memory_region_set_name(struct intel_memory_region *mem,
      va_end(ap);
  }
+void intel_memory_region_avail(struct intel_memory_region *mr,
+                   u64 *avail, u64 *visible_avail)
+{
+    if (mr->type == INTEL_MEMORY_LOCAL) {
+        i915_ttm_buddy_man_avail(mr->region_private,
+                     avail, visible_avail);
+        *avail <<= PAGE_SHIFT;
+        *visible_avail <<= PAGE_SHIFT;
+    } else {
+        *avail = mr->total;
+        *visible_avail = mr->total;
+    }
+}
+
  void intel_memory_region_destroy(struct intel_memory_region *mem)
  {
      int ret = 0;
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index 3d8378c1b447..2214f251bec3 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -127,6 +127,9 @@ int intel_memory_region_reserve(struct intel_memory_region *mem,
  void intel_memory_region_debug(struct intel_memory_region *mr,
                     struct drm_printer *printer);
+void intel_memory_region_avail(struct intel_memory_region *mr,
+                   u64 *avail, u64 *visible_avail);
+
  struct intel_memory_region *
  i915_gem_ttm_system_setup(struct drm_i915_private *i915,
                u16 type, u16 instance);
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 9df419a45244..e30f31a440b3 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3228,7 +3228,15 @@ struct drm_i915_memory_region_info {
       */
      __u64 probed_size;
-    /** @unallocated_size: Estimate of memory remaining (-1 = unknown) */
+    /**
+     * @unallocated_size: Estimate of memory remaining (-1 = unknown)
+     *
+     * Requires CAP_PERFMON or CAP_SYS_ADMIN to get reliable accounting. +     * Without this (or if this is an older kernel) the value here will +     * always equal the @probed_size. Note this is only currently tracked +     * for I915_MEMORY_CLASS_DEVICE regions (for other types the value here
+     * will always equal the @probed_size).
+     */
      __u64 unallocated_size;
      union {
@@ -3262,6 +3270,27 @@ struct drm_i915_memory_region_info {
               * @probed_size.
               */
              __u64 probed_cpu_visible_size;
+
+            /**
+             * @unallocated_cpu_visible_size: Estimate of CPU
+             * visible memory remaining (-1 = unknown).
+             *
+             * Note this is only tracked for
+             * I915_MEMORY_CLASS_DEVICE regions (for other types the
+             * value here will always equal the
+             * @probed_cpu_visible_size).
+             *
+             * Requires CAP_PERFMON or CAP_SYS_ADMIN to get reliable
+             * accounting.  Without this the value here will always
+             * equal the @probed_cpu_visible_size. Note this is only
+             * currently tracked for I915_MEMORY_CLASS_DEVICE
+             * regions (for other types the value here will also
+             * always equal the @probed_cpu_visible_size).
+             *
+             * If this is an older kernel the value here will be
+             * zero, see also @probed_cpu_visible_size.
+             */
+            __u64 unallocated_cpu_visible_size;
          };
      };
  };



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

  Powered by Linux