[PATCH 1/3] drm/suballoc: Introduce a generic suballocation manager

Christian König christian.koenig at amd.com
Fri Feb 17 11:00:58 UTC 2023


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.

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