From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Herrmann Subject: Re: [PATCH 46/48] drm: Optionally create mm blocks from top-to-bottom Date: Sun, 8 Dec 2013 15:28:04 +0100 Message-ID: References: <20131206215521.GA6922@bwidawsk.net> <1386367941-7131-1-git-send-email-benjamin.widawsky@intel.com> <1386367941-7131-46-git-send-email-benjamin.widawsky@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1386367941-7131-46-git-send-email-benjamin.widawsky@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Ben Widawsky Cc: "dri-devel@lists.freedesktop.org" , Intel GFX , Ben Widawsky List-Id: dri-devel@lists.freedesktop.org Hi On Fri, Dec 6, 2013 at 11:11 PM, Ben Widawsky wrote: > From: Chris Wilson > > 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 > Signed-off-by: Chris Wilson > Signed-off-by: Ben Widawsky > --- > 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 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 >