[PATCH 1/3] drm/suballoc: Introduce a generic suballocation manager
Thomas Hellström
thomas.hellstrom at linux.intel.com
Fri Feb 17 11:21:29 UTC 2023
On 2/17/23 12:00, Christian König wrote:
> Am 16.02.23 um 15:48 schrieb Thomas Hellström:
>> Initially we tried to leverage the amdgpu suballocation manager.
>> It turnes out, however, that it tries extremely hard not to enable
>> signalling on the fences that hold the memory up for freeing, which
>> makes
>> it hard to understand and to fix potential issues with it.
>>
>> So in a simplification effort, introduce a drm suballocation manager
>> as a
>> wrapper around an existing allocator (drm_mm) and to avoid using queues
>> for freeing, thus avoiding throttling on free which is an undesired
>> feature as typically the throttling needs to be done uninterruptible.
>>
>> This variant is probably more cpu-hungry but can be improved at the cost
>> of additional complexity. Ideas for that are documented in the
>> drm_suballoc.c file.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Co-developed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>> drivers/gpu/drm/Kconfig | 4 +
>> drivers/gpu/drm/Makefile | 3 +
>> drivers/gpu/drm/drm_suballoc.c | 301 +++++++++++++++++++++++++++++++++
>> include/drm/drm_suballoc.h | 112 ++++++++++++
>> 4 files changed, 420 insertions(+)
>> create mode 100644 drivers/gpu/drm/drm_suballoc.c
>> create mode 100644 include/drm/drm_suballoc.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index dc0f94f02a82..8fbe57407c60 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -232,6 +232,10 @@ config DRM_GEM_SHMEM_HELPER
>> help
>> Choose this if you need the GEM shmem helper functions
>> +config DRM_SUBALLOC_HELPER
>> + tristate
>> + depends on DRM
>> +
>> config DRM_SCHED
>> tristate
>> depends on DRM
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index ab4460fcd63f..1e04d135e866 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -88,6 +88,9 @@ obj-$(CONFIG_DRM_GEM_DMA_HELPER) += drm_dma_helper.o
>> drm_shmem_helper-y := drm_gem_shmem_helper.o
>> obj-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_shmem_helper.o
>> +drm_suballoc_helper-y := drm_suballoc.o
>> +obj-$(CONFIG_DRM_SUBALLOC_HELPER) += drm_suballoc_helper.o
>> +
>> drm_vram_helper-y := drm_gem_vram_helper.o
>> obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
>> diff --git a/drivers/gpu/drm/drm_suballoc.c
>> b/drivers/gpu/drm/drm_suballoc.c
>> new file mode 100644
>> index 000000000000..6e0292dea548
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_suballoc.c
>> @@ -0,0 +1,301 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_suballoc.h>
>> +
>> +/**
>> + * DOC:
>> + * This suballocator intends to be a wrapper around a range allocator
>> + * that is aware also of deferred range freeing with fences. Currently
>> + * we hard-code the drm_mm as the range allocator.
>> + * The approach, while rather simple, suffers from three performance
>> + * issues that can all be fixed if needed at the tradeoff of more
>> and / or
>> + * more complex code:
>> + *
>> + * 1) It's cpu-hungry, the drm_mm allocator is overkill. Either code a
>> + * much simpler range allocator, or let the caller decide by providing
>> + * ops that wrap any range allocator. Also could avoid waking up unless
>> + * there is a reasonable chance of enough space in the range manager.
>
> That's most likely highly problematic.
>
> The suballocator in radeon/amdgpu was designed so that it resembles a
> ring buffer and is therefor rather CPU efficient.
>
> We could make the allocator much more trivial, but using drm_mm for
> this is a sledgehammer and therefore a pretty clear no-go.
>
I don't think the ring vs non-ring is the big problem here, because (at
least with the original implementation), if allocations are actually
made and released in a ring-like fashion, the drm_mm free-list would
consist of one or two blocks and therefore pretty efficient even for
that case, and if slightly longer that would still not be an issue
compared to the fence lists maintained in the older allocator.
The problem is more all the other stuff that was added and built on top
like the interval / rb tree.
I still like the idea (originating from Gallium's helpers) to separate
whatever is allocating from the fence delayed free.
Any chance you could do a quick performance comparison? If not, anything
against merging this without the amd / radeon changes until we can land
a simpler allocator?
Thanks,
Thomas
Thomas
> Regards,
> Christian.
>
>> + *
>> + * 2) We unnecessarily install the fence callbacks too early, forcing
>> + * enable_signaling() too early causing extra driver effort. This is
>> likely
>> + * not an issue if used with the drm_scheduler since it calls
>> + * enable_signaling() early anyway.
>> + *
>> + * 3) Long processing in irq (disabled) context. We've mostly worked
>> around
>> + * that already by using the idle_list. If that workaround is deemed to
>> + * complex for little gain, we can remove it and use spin_lock_irq()
>> + * throughout the manager. If we want to shorten processing in irq
>> context
>> + * even further, we can skip the spin_trylock in
>> __drm_suballoc_free() and
>> + * avoid freeing allocations from irq context altogeher. However drm_mm
>> + * should be quite fast at freeing ranges.
>> + *
>> + * 4) Shrinker that starts processing the list items in 2) and 3) to
>> play
>> + * better with the system.
>> + */
>> +
>> +static void drm_suballoc_process_idle(struct drm_suballoc_manager
>> *sa_manager);
>> +
>> +/**
>> + * drm_suballoc_manager_init() - Initialise the drm_suballoc_manager
>> + * @sa_manager: pointer to the sa_manager
>> + * @size: number of bytes we want to suballocate
>> + * @align: alignment for each suballocated chunk
>> + *
>> + * Prepares the suballocation manager for suballocations.
>> + */
>> +void drm_suballoc_manager_init(struct drm_suballoc_manager *sa_manager,
>> + u64 size, u64 align)
>> +{
>> + spin_lock_init(&sa_manager->lock);
>> + spin_lock_init(&sa_manager->idle_list_lock);
>> + mutex_init(&sa_manager->alloc_mutex);
>> + drm_mm_init(&sa_manager->mm, 0, size);
>> + init_waitqueue_head(&sa_manager->wq);
>> + sa_manager->range_size = size;
>> + sa_manager->alignment = align;
>> + INIT_LIST_HEAD(&sa_manager->idle_list);
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_manager_init);
>> +
>> +/**
>> + * drm_suballoc_manager_fini() - Destroy the drm_suballoc_manager
>> + * @sa_manager: pointer to the sa_manager
>> + *
>> + * Cleans up the suballocation manager after use. All fences added
>> + * with drm_suballoc_free() must be signaled, or we cannot clean up
>> + * the entire manager.
>> + */
>> +void drm_suballoc_manager_fini(struct drm_suballoc_manager *sa_manager)
>> +{
>> + drm_suballoc_process_idle(sa_manager);
>> + drm_mm_takedown(&sa_manager->mm);
>> + mutex_destroy(&sa_manager->alloc_mutex);
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_manager_fini);
>> +
>> +static void __drm_suballoc_free(struct drm_suballoc *sa)
>> +{
>> + struct drm_suballoc_manager *sa_manager = sa->manager;
>> + struct dma_fence *fence;
>> +
>> + /*
>> + * In order to avoid protecting the potentially lengthy drm_mm
>> manager
>> + * *allocation* processing with an irq-disabling lock,
>> + * defer touching the drm_mm for freeing until we're in task
>> context,
>> + * with no irqs disabled, or happen to succeed in taking the
>> manager
>> + * lock.
>> + */
>> + if (!in_task() || irqs_disabled()) {
>> + unsigned long irqflags;
>> +
>> + if (spin_trylock(&sa_manager->lock))
>> + goto locked;
>> +
>> + spin_lock_irqsave(&sa_manager->idle_list_lock, irqflags);
>> + list_add_tail(&sa->idle_link, &sa_manager->idle_list);
>> + spin_unlock_irqrestore(&sa_manager->idle_list_lock, irqflags);
>> + wake_up(&sa_manager->wq);
>> + return;
>> + }
>> +
>> + spin_lock(&sa_manager->lock);
>> +locked:
>> + drm_mm_remove_node(&sa->node);
>> +
>> + fence = sa->fence;
>> + sa->fence = NULL;
>> + spin_unlock(&sa_manager->lock);
>> + /* Maybe only wake if first mm hole is sufficiently large? */
>> + wake_up(&sa_manager->wq);
>> + dma_fence_put(fence);
>> + kfree(sa);
>> +}
>> +
>> +/* Free all deferred idle allocations */
>> +static void drm_suballoc_process_idle(struct drm_suballoc_manager
>> *sa_manager)
>> +{
>> + /*
>> + * prepare_to_wait() / wake_up() semantics ensure that any list
>> + * addition that was done before wake_up() is visible when
>> + * this code is called from the wait loop.
>> + */
>> + if (!list_empty_careful(&sa_manager->idle_list)) {
>> + struct drm_suballoc *sa, *next;
>> + unsigned long irqflags;
>> + LIST_HEAD(list);
>> +
>> + spin_lock_irqsave(&sa_manager->idle_list_lock, irqflags);
>> + list_splice_init(&sa_manager->idle_list, &list);
>> + spin_unlock_irqrestore(&sa_manager->idle_list_lock, irqflags);
>> +
>> + list_for_each_entry_safe(sa, next, &list, idle_link)
>> + __drm_suballoc_free(sa);
>> + }
>> +}
>> +
>> +static void
>> +drm_suballoc_fence_signaled(struct dma_fence *fence, struct
>> dma_fence_cb *cb)
>> +{
>> + struct drm_suballoc *sa = container_of(cb, typeof(*sa), cb);
>> +
>> + __drm_suballoc_free(sa);
>> +}
>> +
>> +static int drm_suballoc_tryalloc(struct drm_suballoc *sa, u64 size)
>> +{
>> + struct drm_suballoc_manager *sa_manager = sa->manager;
>> + int err;
>> +
>> + drm_suballoc_process_idle(sa_manager);
>> + spin_lock(&sa_manager->lock);
>> + err = drm_mm_insert_node_generic(&sa_manager->mm, &sa->node, size,
>> + sa_manager->alignment, 0,
>> + DRM_MM_INSERT_EVICT);
>> + spin_unlock(&sa_manager->lock);
>> + return err;
>> +}
>> +
>> +/**
>> + * drm_suballoc_new() - Make a suballocation.
>> + * @sa_manager: pointer to the sa_manager
>> + * @size: number of bytes we want to suballocate.
>> + * @gfp: Allocation context.
>> + * @intr: Whether to sleep interruptibly if sleeping.
>> + *
>> + * Try to make a suballocation of size @size, which will be rounded
>> + * up to the alignment specified in specified in
>> drm_suballoc_manager_init().
>> + *
>> + * Returns a new suballocated bo, or an ERR_PTR.
>> + */
>> +struct drm_suballoc*
>> +drm_suballoc_new(struct drm_suballoc_manager *sa_manager, u64 size,
>> + gfp_t gfp, bool intr)
>> +{
>> + struct drm_suballoc *sa;
>> + DEFINE_WAIT(wait);
>> + int err = 0;
>> +
>> + if (size > sa_manager->range_size)
>> + return ERR_PTR(-ENOSPC);
>> +
>> + sa = kzalloc(sizeof(*sa), gfp);
>> + if (!sa)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + /* Avoid starvation using the alloc_mutex */
>> + if (intr)
>> + err = mutex_lock_interruptible(&sa_manager->alloc_mutex);
>> + else
>> + mutex_lock(&sa_manager->alloc_mutex);
>> + if (err) {
>> + kfree(sa);
>> + return ERR_PTR(err);
>> + }
>> +
>> + sa->manager = sa_manager;
>> + err = drm_suballoc_tryalloc(sa, size);
>> + if (err != -ENOSPC)
>> + goto out;
>> +
>> + for (;;) {
>> + prepare_to_wait(&sa_manager->wq, &wait,
>> + intr ? TASK_INTERRUPTIBLE :
>> + TASK_UNINTERRUPTIBLE);
>> +
>> + err = drm_suballoc_tryalloc(sa, size);
>> + if (err != -ENOSPC)
>> + break;
>> +
>> + if (intr && signal_pending(current)) {
>> + err = -ERESTARTSYS;
>> + break;
>> + }
>> +
>> + io_schedule();
>> + }
>> + finish_wait(&sa_manager->wq, &wait);
>> +
>> +out:
>> + mutex_unlock(&sa_manager->alloc_mutex);
>> + if (!sa->node.size) {
>> + kfree(sa);
>> + WARN_ON(!err);
>> + sa = ERR_PTR(err);
>> + }
>> +
>> + return sa;
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_new);
>> +
>> +/**
>> + * drm_suballoc_free() - Free a suballocation
>> + * @suballoc: pointer to the suballocation
>> + * @fence: fence that signals when suballocation is idle
>> + * @queue: the index to which queue the suballocation will be placed
>> on the free list.
>> + *
>> + * Free the suballocation. The suballocation can be re-used after
>> @fence
>> + * signals.
>> + */
>> +void
>> +drm_suballoc_free(struct drm_suballoc *sa, struct dma_fence *fence)
>> +{
>> + if (!sa)
>> + return;
>> +
>> + if (!fence || dma_fence_is_signaled(fence)) {
>> + __drm_suballoc_free(sa);
>> + return;
>> + }
>> +
>> + sa->fence = dma_fence_get(fence);
>> + if (dma_fence_add_callback(fence, &sa->cb,
>> drm_suballoc_fence_signaled))
>> + __drm_suballoc_free(sa);
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_free);
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +
>> +/**
>> + * drm_suballoc_dump_debug_info() - Dump the suballocator state
>> + * @sa_manager: The suballoc manager.
>> + * @p: Pointer to a drm printer for output.
>> + * @suballoc_base: Constant to add to the suballocated offsets on
>> printout.
>> + *
>> + * This function dumps the suballocator state. Note that the caller has
>> + * to explicitly order frees and calls to this function in order for
>> the
>> + * freed node to show up as protected by a fence.
>> + */
>> +void drm_suballoc_dump_debug_info(struct drm_suballoc_manager
>> *sa_manager,
>> + struct drm_printer *p, u64 suballoc_base)
>> +{
>> + const struct drm_mm_node *entry;
>> +
>> + spin_lock(&sa_manager->lock);
>> + drm_mm_for_each_node(entry, &sa_manager->mm) {
>> + struct drm_suballoc *sa =
>> + container_of(entry, typeof(*sa), node);
>> +
>> + drm_printf(p, " ");
>> + drm_printf(p, "[0x%010llx 0x%010llx] size %8lld",
>> + (unsigned long long)suballoc_base + entry->start,
>> + (unsigned long long)suballoc_base + entry->start +
>> + entry->size, (unsigned long long)entry->size);
>> +
>> + if (sa->fence)
>> + drm_printf(p, " protected by 0x%016llx on context %llu",
>> + (unsigned long long)sa->fence->seqno,
>> + (unsigned long long)sa->fence->context);
>> +
>> + drm_printf(p, "\n");
>> + }
>> + spin_unlock(&sa_manager->lock);
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_dump_debug_info);
>> +#endif
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_DESCRIPTION("Simple range suballocator helper");
>> +MODULE_LICENSE("GPL and additional rights");
>> diff --git a/include/drm/drm_suballoc.h b/include/drm/drm_suballoc.h
>> new file mode 100644
>> index 000000000000..910952b3383b
>> --- /dev/null
>> +++ b/include/drm/drm_suballoc.h
>> @@ -0,0 +1,112 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>> +#ifndef _DRM_SUBALLOC_H_
>> +#define _DRM_SUBALLOC_H_
>> +
>> +#include <drm/drm_mm.h>
>> +
>> +#include <linux/dma-fence.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct drm_suballoc_manager - Wrapper for fenced range allocations
>> + * @mm: The range manager. Protected by @lock.
>> + * @range_size: The total size of the range.
>> + * @alignment: Range alignment.
>> + * @wq: Wait queue for sleeping allocations on contention.
>> + * @idle_list: List of idle but not yet freed allocations. Protected by
>> + * @idle_list_lock.
>> + * @task: Task waiting for allocation. Protected by @lock.
>> + */
>> +struct drm_suballoc_manager {
>> + /** @lock: Manager lock. Protects @mm. */
>> + spinlock_t lock;
>> + /**
>> + * @idle_list_lock: Lock to protect the idle_list.
>> + * Disable irqs when locking.
>> + */
>> + spinlock_t idle_list_lock;
>> + /** @alloc_mutex: Mutex to protect against stavation. */
>> + struct mutex alloc_mutex;
>> + struct drm_mm mm;
>> + u64 range_size;
>> + u64 alignment;
>> + wait_queue_head_t wq;
>> + struct list_head idle_list;
>> +};
>> +
>> +/**
>> + * struct drm_suballoc: Suballocated range.
>> + * @node: The drm_mm representation of the range.
>> + * @fence: dma-fence indicating whether allocation is active or idle.
>> + * Assigned on call to free the allocation so doesn't need protection.
>> + * @cb: dma-fence callback structure. Used for callbacks when the
>> fence signals.
>> + * @manager: The struct drm_suballoc_manager the range belongs to.
>> Immutable.
>> + * @idle_link: Link for the manager idle_list. Progected by the
>> + * drm_suballoc_manager::idle_lock.
>> + */
>> +struct drm_suballoc {
>> + struct drm_mm_node node;
>> + struct dma_fence *fence;
>> + struct dma_fence_cb cb;
>> + struct drm_suballoc_manager *manager;
>> + struct list_head idle_link;
>> +};
>> +
>> +void drm_suballoc_manager_init(struct drm_suballoc_manager *sa_manager,
>> + u64 size, u64 align);
>> +
>> +void drm_suballoc_manager_fini(struct drm_suballoc_manager
>> *sa_manager);
>> +
>> +struct drm_suballoc *drm_suballoc_new(struct drm_suballoc_manager
>> *sa_manager,
>> + u64 size, gfp_t gfp, bool intr);
>> +
>> +void drm_suballoc_free(struct drm_suballoc *sa, struct dma_fence
>> *fence);
>> +
>> +/**
>> + * drm_suballoc_soffset - Range start.
>> + * @sa: The struct drm_suballoc.
>> + *
>> + * Return: The start of the allocated range.
>> + */
>> +static inline u64 drm_suballoc_soffset(struct drm_suballoc *sa)
>> +{
>> + return sa->node.start;
>> +}
>> +
>> +/**
>> + * drm_suballoc_eoffset - Range end.
>> + * @sa: The struct drm_suballoc.
>> + *
>> + * Return: The end of the allocated range + 1.
>> + */
>> +static inline u64 drm_suballoc_eoffset(struct drm_suballoc *sa)
>> +{
>> + return sa->node.start + sa->node.size;
>> +}
>> +
>> +/**
>> + * drm_suballoc_size - Range size.
>> + * @sa: The struct drm_suballoc.
>> + *
>> + * Return: The size of the allocated range.
>> + */
>> +static inline u64 drm_suballoc_size(struct drm_suballoc *sa)
>> +{
>> + return sa->node.size;
>> +}
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +void drm_suballoc_dump_debug_info(struct drm_suballoc_manager
>> *sa_manager,
>> + struct drm_printer *p, u64 suballoc_base);
>> +#else
>> +static inline void
>> +drm_suballoc_dump_debug_info(struct drm_suballoc_manager *sa_manager,
>> + struct drm_printer *p, u64 suballoc_base)
>> +{ }
>> +
>> +#endif
>> +
>> +#endif /* _DRM_SUBALLOC_H_ */
>
More information about the dri-devel
mailing list