Re: [PATCH 1/4] drm/mm: add "best_match" flag to drm_mm_insert_node()

Hi

On Mon, Aug 5, 2013 at 9:46 AM, Daniel Vetter daniel@ffwll.ch wrote:

On Sat, Jul 27, 2013 at 01:36:27PM +0200, David Herrmann wrote:

...

Add a "best_match" flag similar to the drm_mm_search_*() helpers so we can convert TTM to use them in follow up patches. We can also inline the non-generic helpers and move them into the header to allow compile-time optimizations.

To make calls to drm_mm_{search,insert}_node() more readable, this converts the boolean argument to a flagset. There are pending patches that add additional flags for top-down allocators and more.

v2:

• use flag parameter instead of boolean "best_match"
• convert *_search_free() helpers to also use flags argument

Signed-off-by: David Herrmann dh.herrmann@gmail.com

I'm not sure whether the use of an enum here is proper C style (usually bitflag arguments tend to be unsigned ints), but that's a bikeshed. So

I saw it used in libxkbcommon and it's actually pretty nice because gcc supports some type-safety for it. It checks whether the passed flags are really from the enum (even if OR/AND-combined). And as it is no external API, we have no problems with the "enum resizing" once you add more flags.

But if anyone feels uncomfortable with it, I can change it to "unsigned int".

Thanks for reviewing! David

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

---

drivers/gpu/drm/drm_mm.c | 37 ++++++++--------------- drivers/gpu/drm/drm_vma_manager.c | 4 +-- drivers/gpu/drm/i915/i915_gem.c | 3 +- drivers/gpu/drm/i915/i915_gem_stolen.c | 12 +++++--- drivers/gpu/drm/sis/sis_mm.c | 6 ++-- drivers/gpu/drm/ttm/ttm_bo_manager.c | 3 +- drivers/gpu/drm/via/via_mm.c | 4 +-- include/drm/drm_mm.h | 54 ++++++++++++++++++++++------------ 8 files changed, 68 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index fe304f9..9a38327 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -212,12 +212,13 @@ EXPORT_SYMBOL(drm_mm_get_block_generic); */ int drm_mm_insert_node_generic(struct drm_mm *mm, struct drm_mm_node *node, unsigned long size, unsigned alignment,

•                        unsigned long color)
  

•                        unsigned long color,
  

•                        enum drm_mm_search_flags flags)
  

{ struct drm_mm_node *hole_node;

  hole_node = drm_mm_search_free_generic(mm, size, alignment,

•                                        color, 0);
  

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

@@ -226,13 +227,6 @@ int drm_mm_insert_node_generic(struct drm_mm *mm, struct drm_mm_node *node, } EXPORT_SYMBOL(drm_mm_insert_node_generic);

-int drm_mm_insert_node(struct drm_mm *mm, struct drm_mm_node *node,

•                unsigned long size, unsigned alignment)
  

-{

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

-} -EXPORT_SYMBOL(drm_mm_insert_node);

static void drm_mm_insert_helper_range(struct drm_mm_node *hole_node, struct drm_mm_node *node, unsigned long size, unsigned alignment, @@ -313,13 +307,14 @@ EXPORT_SYMBOL(drm_mm_get_block_range_generic); */ 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 start, unsigned long end)
  

•                                 unsigned long start, unsigned long end,
  

•                                 enum drm_mm_search_flags flags)
  

{ struct drm_mm_node *hole_node;

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

•                                                 start, end, 0);
  

•                                                 start, end, flags);
  if (!hole_node)
          return -ENOSPC;
  
  

@@ -330,14 +325,6 @@ int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, struct drm_mm_node *n } EXPORT_SYMBOL(drm_mm_insert_node_in_range_generic);

-int drm_mm_insert_node_in_range(struct drm_mm *mm, struct drm_mm_node *node,

•                         unsigned long size, unsigned alignment,
  

•                         unsigned long start, unsigned long end)
  

-{

• return drm_mm_insert_node_in_range_generic(mm, node, size, alignment, 0, start, end);
  

-} -EXPORT_SYMBOL(drm_mm_insert_node_in_range);

/**

• Remove a memory node from the allocator.

*/ @@ -413,7 +400,7 @@ struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm, unsigned long size, unsigned alignment, unsigned long color,

•                                        bool best_match)
  

•                                        enum drm_mm_search_flags flags)
  

{ struct drm_mm_node *entry; struct drm_mm_node *best; @@ -436,7 +423,7 @@ struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm, if (!check_free_hole(adj_start, adj_end, size, alignment)) continue;

•         if (!best_match)
  

•         if (!(flags & DRM_MM_SEARCH_BEST))
                  return entry;
  
          if (entry->size < best_size) {
  

@@ -455,7 +442,7 @@ struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_mm *mm, unsigned long color, unsigned long start, unsigned long end,

•                                                 bool best_match)
  

•                                                 enum drm_mm_search_flags flags)
  

{ struct drm_mm_node *entry; struct drm_mm_node *best; @@ -483,7 +470,7 @@ struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_mm *mm, if (!check_free_hole(adj_start, adj_end, size, alignment)) continue;

•         if (!best_match)
  

•         if (!(flags & DRM_MM_SEARCH_BEST))
                  return entry;
  
          if (entry->size < best_size) {
  

@@ -629,8 +616,8 @@ EXPORT_SYMBOL(drm_mm_scan_add_block);

• corrupted.
• When the scan list is empty, the selected memory nodes can be freed. An

• • immediately following drm_mm_search_free with best_match = 0 will then return
• • the just freed block (because its at the top of the free_stack list).

• • immediately following drm_mm_search_free with !DRM_MM_SEARCH_BEST will then
• • return the just freed block (because its at the top of the free_stack list).
  • Returns one if this block should be evicted, zero otherwise. Will always
  • return zero when no hole has been found.

diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c index b966cea..3837481 100644 --- a/drivers/gpu/drm/drm_vma_manager.c +++ b/drivers/gpu/drm/drm_vma_manager.c @@ -241,8 +241,8 @@ int drm_vma_offset_add(struct drm_vma_offset_manager *mgr, goto out_unlock; }

• ret = drm_mm_insert_node_generic(&mgr->vm_addr_space_mm,
  

•                                  &node->vm_node, pages, 0, 0);
  

• ret = drm_mm_insert_node(&mgr->vm_addr_space_mm, &node->vm_node,
  

•                          pages, 0, DRM_MM_SEARCH_DEFAULT);
  if (ret)
          goto out_unlock;
  
  

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8673a00..f3065a0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3092,7 +3092,8 @@ search_free: ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space, &obj->gtt_space, size, alignment,

•                                           obj->cache_level, 0, gtt_max);
  

•                                           obj->cache_level, 0, gtt_max,
  

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

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 5521833..e355170 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -115,10 +115,12 @@ static int i915_setup_compression(struct drm_device *dev, int size)

  /* Try to over-allocate to reduce reallocations and fragmentation */
  compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen,

•                                    size <<= 1, 4096, 0);
  

•                                    size <<= 1, 4096,
  

•                                    DRM_MM_SEARCH_DEFAULT);
  if (!compressed_fb)
          compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen,
  

•                                            size >>= 1, 4096, 0);
  

•                                            size >>= 1, 4096,
  

•                                            DRM_MM_SEARCH_DEFAULT);
  if (compressed_fb)
          compressed_fb = drm_mm_get_block(compressed_fb, size, 4096);
  if (!compressed_fb)
  

@@ -130,7 +132,8 @@ static int i915_setup_compression(struct drm_device *dev, int size) I915_WRITE(DPFC_CB_BASE, compressed_fb->start); } else { compressed_llb = drm_mm_search_free(&dev_priv->mm.stolen,

•                                             4096, 4096, 0);
  

•                                             4096, 4096,
  

•                                             DRM_MM_SEARCH_DEFAULT);
          if (compressed_llb)
                  compressed_llb = drm_mm_get_block(compressed_llb,
                                                    4096, 4096);
  

@@ -328,7 +331,8 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size) if (size == 0) return NULL;

• stolen = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096, 0);
  

• stolen = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096,
  

•                             DRM_MM_SEARCH_DEFAULT);
  if (stolen)
          stolen = drm_mm_get_block(stolen, size, 4096);
  if (stolen == NULL)
  

diff --git a/drivers/gpu/drm/sis/sis_mm.c b/drivers/gpu/drm/sis/sis_mm.c index 9a43d98..23a2349 100644 --- a/drivers/gpu/drm/sis/sis_mm.c +++ b/drivers/gpu/drm/sis/sis_mm.c @@ -109,7 +109,8 @@ static int sis_drm_alloc(struct drm_device *dev, struct drm_file *file, if (pool == AGP_TYPE) { retval = drm_mm_insert_node(&dev_priv->agp_mm, &item->mm_node,

•                                     mem->size, 0);
  

•                                     mem->size, 0,
  

•                                     DRM_MM_SEARCH_DEFAULT);
          offset = item->mm_node.start;
  } else {
  

#if defined(CONFIG_FB_SIS) || defined(CONFIG_FB_SIS_MODULE) @@ -121,7 +122,8 @@ static int sis_drm_alloc(struct drm_device *dev, struct drm_file *file, #else retval = drm_mm_insert_node(&dev_priv->vram_mm, &item->mm_node,

•                                     mem->size, 0);
  

•                                     mem->size, 0,
  

•                                     DRM_MM_SEARCH_DEFAULT);
          offset = item->mm_node.start;
  

#endif } diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c index e4367f9..e4be29e 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c @@ -69,7 +69,8 @@ static int ttm_bo_man_get_node(struct ttm_mem_type_manager *man, spin_lock(&rman->lock); node = drm_mm_search_free_in_range(mm, mem->num_pages, mem->page_alignment,

•                                 placement->fpfn, lpfn, 1);
  

•                                 placement->fpfn, lpfn,
  

•                                 DRM_MM_SEARCH_BEST);
          if (unlikely(node == NULL)) {
                  spin_unlock(&rman->lock);
                  return 0;
  

diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c index 0ab93ff..7e3ad87 100644 --- a/drivers/gpu/drm/via/via_mm.c +++ b/drivers/gpu/drm/via/via_mm.c @@ -140,11 +140,11 @@ int via_mem_alloc(struct drm_device *dev, void *data, if (mem->type == VIA_MEM_AGP) retval = drm_mm_insert_node(&dev_priv->agp_mm, &item->mm_node,

•                                     tmpSize, 0);
  

•                                     tmpSize, 0, DRM_MM_SEARCH_DEFAULT);
  else
          retval = drm_mm_insert_node(&dev_priv->vram_mm,
                                      &item->mm_node,
  

•                                     tmpSize, 0);
  

•                                     tmpSize, 0, DRM_MM_SEARCH_DEFAULT);
  if (retval)
          goto fail_alloc;
  
  

diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h index 98cb50e..439d1a1 100644 --- a/include/drm/drm_mm.h +++ b/include/drm/drm_mm.h @@ -44,6 +44,11 @@ #include <linux/seq_file.h> #endif

+enum drm_mm_search_flags {

• DRM_MM_SEARCH_DEFAULT =         0,
  

• DRM_MM_SEARCH_BEST =            1 << 0,
  

+};

struct drm_mm_node { struct list_head node_list; struct list_head hole_stack; @@ -189,28 +194,41 @@ static inline struct drm_mm_node *drm_mm_get_block_atomic_range( start, end, 1); }

-extern int drm_mm_insert_node(struct drm_mm *mm,

•                       struct drm_mm_node *node,
  

•                       unsigned long size,
  

•                       unsigned alignment);
  

-extern int drm_mm_insert_node_in_range(struct drm_mm *mm,

•                                struct drm_mm_node *node,
  

•                                unsigned long size,
  

•                                unsigned alignment,
  

•                                unsigned long start,
  

•                                unsigned long end);
  

extern int drm_mm_insert_node_generic(struct drm_mm *mm, struct drm_mm_node *node, unsigned long size, unsigned alignment,

•                               unsigned long color);
  

•                               unsigned long color,
  

•                               enum drm_mm_search_flags flags);
  

+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);
  

+}

extern 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 start,

•                                unsigned long end);
  

•                                unsigned long end,
  

•                                enum drm_mm_search_flags flags);
  

+static inline int drm_mm_insert_node_in_range(struct drm_mm *mm,

•                                       struct drm_mm_node *node,
  

•                                       unsigned long size,
  

•                                       unsigned alignment,
  

•                                       unsigned long start,
  

•                                       unsigned long end,
  

•                                       enum drm_mm_search_flags flags)
  

+{

• return drm_mm_insert_node_in_range_generic(mm, node, size, alignment,
  

•                                            0, start, end, flags);
  

+}

extern void drm_mm_put_block(struct drm_mm_node *cur); extern void drm_mm_remove_node(struct drm_mm_node *node); extern void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new); @@ -218,7 +236,7 @@ extern struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm, unsigned long size, unsigned alignment, unsigned long color,

•                                               bool best_match);
  

•                                               enum drm_mm_search_flags flags);
  

extern struct drm_mm_node *drm_mm_search_free_in_range_generic( const struct drm_mm *mm, unsigned long size, @@ -226,13 +244,13 @@ extern struct drm_mm_node *drm_mm_search_free_in_range_generic( unsigned long color, unsigned long start, unsigned long end,

•                                         bool best_match);
  

•                                         enum drm_mm_search_flags flags);
  

static inline struct drm_mm_node *drm_mm_search_free(const struct drm_mm *mm, unsigned long size, unsigned alignment,

•                                              bool best_match)
  

•                                              enum drm_mm_search_flags flags)
  

{

• return drm_mm_search_free_generic(mm,size, alignment, 0, best_match);
  

• return drm_mm_search_free_generic(mm,size, alignment, 0, flags);
  

} static inline struct drm_mm_node *drm_mm_search_free_in_range( const struct drm_mm *mm, @@ -240,10 +258,10 @@ static inline struct drm_mm_node *drm_mm_search_free_in_range( unsigned alignment, unsigned long start, unsigned long end,

•                                         bool best_match)
  

•                                         enum drm_mm_search_flags flags)
  

{ return drm_mm_search_free_in_range_generic(mm, size, alignment, 0,

•                                            start, end, best_match);
  

•                                            start, end, flags);
  

}

extern void drm_mm_init(struct drm_mm *mm,

1.8.3.4

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch