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

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


Am 17.02.23 um 12:21 schrieb Thomas Hellström:
>
> 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.

That's actually a bad idea. See the ring like approach works because the 
fences used in amdgpu/radeon are used in a ring like fashion. E.g. the 
sub allocator mainly provides the temporary space for page table 
updates. Those in turn are then used by commands written into a ring buffer.

>
> 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?

Only if you can stick the allocator inside Xe and not drm, cause this 
seems to be for a different use case than the allocators inside 
radeon/amdgpu.

Regards,
Christian.

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