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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

On Fri, Dec 6, 2013 at 11:11 PM, Ben Widawsky
<benjamin.widawsky@xxxxxxxxx> wrote:
> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxx>

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux