Re: [PATCH 46/48] drm: Optionally create mm blocks from top-to-bottom

Hi

On Fri, Dec 6, 2013 at 11:11 PM, Ben Widawsky benjamin.widawsky@intel.com wrote:

From: Chris Wilson chris@chris-wilson.co.uk

Clients like i915 needs to segregate cache domains within the GTT which can lead to small amounts of fragmentation. By allocating the uncached buffers from the bottom and the cacheable buffers from the top, we can reduce the amount of wasted space and also optimize allocation of the mappable portion of the GTT to only those buffers that require CPU access through the GTT.

v2 by Ben: Update callers in i915_gem_object_bind_to_gtt() Turn search flags and allocation flags into separate enums Make checkpatch happy where logical/easy

v3 by Ben: Rebased on top of the many drm_mm changes since the original patches Remove ATOMIC from allocator flags (Chris) Reverse order of TOPDOWN and BOTTOMUP

CC'ing dri-devel as I'm not subscribed to intel-gfx.

Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Ben Widawsky ben@bwidawsk.net

---

drivers/gpu/drm/drm_mm.c | 56 +++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/i915_gem.c | 3 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- include/drm/drm_mm.h | 29 ++++++++++++++++--- 4 files changed, 69 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index af93cc5..4f5e4f6 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -65,7 +65,8 @@ static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_ static void drm_mm_insert_helper(struct drm_mm_node *hole_node, struct drm_mm_node *node, unsigned long size, unsigned alignment,

•                            unsigned long color)
  

•                            unsigned long color,
  

•                            enum drm_mm_allocator_flags flags)
  

{ struct drm_mm *mm = hole_node->mm; unsigned long hole_start = drm_mm_hole_node_start(hole_node); @@ -78,12 +79,22 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node, if (mm->color_adjust) mm->color_adjust(hole_node, color, &adj_start, &adj_end);

•   if (flags & DRM_MM_CREATE_TOP)
  

•           adj_start = adj_end - size;
  

That should be above the call to mm->color_adjust(), imo. But I'm not entirely sure what kind of adjustments drivers do. At least you should be consistend with the range-helper below where you call it *before* the color-adjustment.

    if (alignment) {
            unsigned tmp = adj_start % alignment;

•           if (tmp)
  

•                   adj_start += alignment - tmp;
  

•           if (tmp) {
  

•                   if (flags & DRM_MM_CREATE_TOP)
  

•                           adj_start -= tmp;
  

•                   else
  

•                           adj_start += alignment - tmp;
  

•           }
    }
  
  

•   BUG_ON(adj_start < hole_start);
  

•   BUG_ON(adj_end > hole_end);
  

•   if (adj_start == hole_start) {
            hole_node->hole_follows = 0;
            list_del(&hole_node->hole_stack);
  

@@ -155,16 +166,17 @@ EXPORT_SYMBOL(drm_mm_reserve_node); int drm_mm_insert_node_generic(struct drm_mm *mm, struct drm_mm_node *node, unsigned long size, unsigned alignment, unsigned long color,

•                          enum drm_mm_search_flags flags)
  

•                          enum drm_mm_search_flags sflags,
  

•                          enum drm_mm_allocator_flags aflags)
  

{ struct drm_mm_node *hole_node;

    hole_node = drm_mm_search_free_generic(mm, size, alignment,

•                                          color, flags);
  

•                                          color, sflags);
    if (!hole_node)
            return -ENOSPC;
  
  

•   drm_mm_insert_helper(hole_node, node, size, alignment, color);
  

•   drm_mm_insert_helper(hole_node, node, size, alignment, color, aflags);
    return 0;
  

} EXPORT_SYMBOL(drm_mm_insert_node_generic); @@ -173,7 +185,8 @@ static void drm_mm_insert_helper_range(struct drm_mm_node *hole_node, struct drm_mm_node *node, unsigned long size, unsigned alignment, unsigned long color,

•                                  unsigned long start, unsigned long end)
  

•                                  unsigned long start, unsigned long end,
  

•                                  enum drm_mm_allocator_flags flags)
  

{ struct drm_mm *mm = hole_node->mm; unsigned long hole_start = drm_mm_hole_node_start(hole_node); @@ -188,13 +201,20 @@ static void drm_mm_insert_helper_range(struct drm_mm_node *hole_node, if (adj_end > end) adj_end = end;

•   if (flags & DRM_MM_CREATE_TOP)
  

•           adj_start = adj_end - size;
  

•   if (mm->color_adjust)
            mm->color_adjust(hole_node, color, &adj_start, &adj_end);
  
    if (alignment) {
            unsigned tmp = adj_start % alignment;
  

•           if (tmp)
  

•                   adj_start += alignment - tmp;
  

•           if (tmp) {
  

•                   if (flags & DRM_MM_CREATE_TOP)
  

•                           adj_start -= tmp;
  

•                   else
  

•                           adj_start += alignment - tmp;
  

•           }
    }
  
    if (adj_start == hole_start) {
  

@@ -211,6 +231,8 @@ static void drm_mm_insert_helper_range(struct drm_mm_node *hole_node, INIT_LIST_HEAD(&node->hole_stack); list_add(&node->node_list, &hole_node->node_list);

•   BUG_ON(node->start < start);
  

•   BUG_ON(node->start < adj_start);
    BUG_ON(node->start + node->size > adj_end);
    BUG_ON(node->start + node->size > end);
  
  

@@ -227,21 +249,23 @@ static void drm_mm_insert_helper_range(struct drm_mm_node *hole_node,

• restricted allocations. The preallocated memory node must be cleared.

*/ int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, struct drm_mm_node *node,

•                                   unsigned long size, unsigned alignment, unsigned long color,
  

•                                   unsigned long size, unsigned alignment,
  

•                                   unsigned long color,
                                    unsigned long start, unsigned long end,
  

•                                   enum drm_mm_search_flags flags)
  

•                                   enum drm_mm_search_flags sflags,
  

•                                   enum drm_mm_allocator_flags aflags)
  

{ struct drm_mm_node *hole_node;

    hole_node = drm_mm_search_free_in_range_generic(mm,
                                                    size, alignment, color,

•                                                   start, end, flags);
  

•                                                   start, end, sflags);
    if (!hole_node)
            return -ENOSPC;
  
    drm_mm_insert_helper_range(hole_node, node,
                               size, alignment, color,
  

•                              start, end);
  

•                              start, end, aflags);
    return 0;
  

} EXPORT_SYMBOL(drm_mm_insert_node_in_range_generic); @@ -315,7 +339,8 @@ static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm, best = NULL; best_size = ~0UL;

•   drm_mm_for_each_hole(entry, mm, adj_start, adj_end) {
  

•   __drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
  

•                          flags & DRM_MM_SEARCH_BELOW) {
            if (mm->color_adjust) {
                    mm->color_adjust(entry, color, &adj_start, &adj_end);
                    if (adj_end <= adj_start)
  

@@ -356,7 +381,8 @@ static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_ best = NULL; best_size = ~0UL;

•   drm_mm_for_each_hole(entry, mm, adj_start, adj_end) {
  

•   __drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
  

•                          flags & DRM_MM_SEARCH_BELOW) {
            if (adj_start < start)
                    adj_start = start;
            if (adj_end > end)
  

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a03c262..1360f89 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3280,7 +3280,8 @@ search_free: ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node, size, alignment, obj->cache_level, 1, gtt_max,

•                                             DRM_MM_SEARCH_DEFAULT);
  

•                                             DRM_MM_SEARCH_DEFAULT,
  

•                                             DRM_MM_CREATE_DEFAULT);
    if (ret) {
            ret = i915_gem_evict_something(dev, vm, size, alignment,
                                           obj->cache_level,
  

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 998f9a0..37832e4 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -861,7 +861,7 @@ alloc: &ppgtt->node, GEN6_PD_SIZE, GEN6_PD_ALIGN, 0, 0, dev_priv->gtt.base.total,

•                                             DRM_MM_SEARCH_DEFAULT);
  

•                                             DRM_MM_BOTTOMUP);
    if (ret == -ENOSPC && !retried) {
            ret = i915_gem_evict_something(dev, &dev_priv->gtt.base,
                                           GEN6_PD_SIZE, GEN6_PD_ALIGN,
  

diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h index cba6786..36c1d8f 100644 --- a/include/drm/drm_mm.h +++ b/include/drm/drm_mm.h @@ -47,8 +47,17 @@ enum drm_mm_search_flags { DRM_MM_SEARCH_DEFAULT = 0, DRM_MM_SEARCH_BEST = 1 << 0,

•   DRM_MM_SEARCH_BELOW =           1 << 1,
  

SEARCH_BEST and SEARCH_BELOW are (almost) mutually exclusive, right? Hm, but doesn't really hurt if someone specifies both so should be fine.

Regarding the names: Something like DRM_MM_SEARCH_FROM_TOP sounds better to my German ears, but what do I know.. (same below for CREATE_TOP->CREATE_FROM_TOP).

};

+enum drm_mm_allocator_flags {

•   DRM_MM_CREATE_DEFAULT =         0,
  

•   DRM_MM_CREATE_TOP =             1 << 0,
  

+};

+#define DRM_MM_BOTTOMUP DRM_MM_SEARCH_DEFAULT, DRM_MM_CREATE_DEFAULT +#define DRM_MM_TOPDOWN DRM_MM_SEARCH_BELOW, DRM_MM_CREATE_TOP

That is ugly. Is that really needed? The drm_mm calls already have a lot of arguments, I don't think we should hide the extra flag just to save one. If it's about the more obvious name, why not add "static inline" helpers? debuggers hate pre-processor magic..

Apart from the adjust_color() thing, I'm fine with this patch whether you change names/arguments or not. So with that fixed: Reviewed-by: David Herrmann dh.herrmann@gmail.com

Thanks David

struct drm_mm_node { struct list_head node_list; struct list_head hole_stack; @@ -133,6 +142,14 @@ static inline unsigned long drm_mm_hole_node_end(struct drm_mm_node *hole_node) 1 : 0; \ entry = list_entry(entry->hole_stack.next, struct drm_mm_node, hole_stack))

+#define __drm_mm_for_each_hole(entry, mm, hole_start, hole_end, backwards) \

•   for (entry = list_entry((backwards) ? (mm)->hole_stack.prev : (mm)->hole_stack.next, struct drm_mm_node, hole_stack); \
  

•        &entry->hole_stack != &(mm)->hole_stack ? \
  

•        hole_start = drm_mm_hole_node_start(entry), \
  

•        hole_end = drm_mm_hole_node_end(entry), \
  

•        1 : 0; \
  

•        entry = list_entry((backwards) ? entry->hole_stack.prev : entry->hole_stack.next, struct drm_mm_node, hole_stack))
  

/*

• Basic range manager support (drm_mm.c)

*/ @@ -143,14 +160,16 @@ extern int drm_mm_insert_node_generic(struct drm_mm *mm, unsigned long size, unsigned alignment, unsigned long color,

•                                 enum drm_mm_search_flags flags);
  

•                                 enum drm_mm_search_flags sflags,
  

•                                 enum drm_mm_allocator_flags aflags);
  

static inline int drm_mm_insert_node(struct drm_mm *mm, struct drm_mm_node *node, unsigned long size, unsigned alignment, enum drm_mm_search_flags flags) {

•   return drm_mm_insert_node_generic(mm, node, size, alignment, 0, flags);
  

•   return drm_mm_insert_node_generic(mm, node, size, alignment, 0, flags,
  

•                                     DRM_MM_CREATE_DEFAULT);
  

}

extern int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, @@ -160,7 +179,8 @@ extern int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, unsigned long color, unsigned long start, unsigned long end,

•                                  enum drm_mm_search_flags flags);
  

•                                  enum drm_mm_search_flags sflags,
  

•                                  enum drm_mm_allocator_flags aflags);
  

static inline int drm_mm_insert_node_in_range(struct drm_mm *mm, struct drm_mm_node *node, unsigned long size, @@ -170,7 +190,8 @@ static inline int drm_mm_insert_node_in_range(struct drm_mm *mm, enum drm_mm_search_flags flags) { return drm_mm_insert_node_in_range_generic(mm, node, size, alignment,

•                                              0, start, end, flags);
  

•                                              0, start, end, flags,
  

•                                              DRM_MM_CREATE_DEFAULT);
  

}

extern void drm_mm_remove_node(struct drm_mm_node *node);

1.8.4.2