[Intel-gfx] [PATCH 03/10] drm/i915/uapi: expose the avail tracking
Matthew Auld
matthew.auld at intel.com
Mon May 30 17:05:06 UTC 2022
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 at i915_query@query-regions-unallocated
>>>> Testcase: igt at i915_query@query-regions-sanity-check
>>>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>>>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>>> Cc: Kenneth Graunke <kenneth at whitecape.org>
>>>> Cc: Akeem G Abodunrin <akeem.g.abodunrin at intel.com>
>>>> ---
>>>> 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;
>>>> };
>>>> };
>>>> };
More information about the Intel-gfx
mailing list