diff mbox

[4/4] drm/mm: Remove DRM_MM_SEARCH_BEST

Message ID 1414658202-5737-1-git-send-email-michel@daenzer.net (mailing list archive)
State New, archived
Headers show

Commit Message

Michel Dänzer Oct. 30, 2014, 8:36 a.m. UTC 
From: Michel Dänzer <michel.daenzer@amd.com>

In the words of Daniel Vetter:

«I think SEARCH_BEST is pretty much always a bad idea - it rips apart
allocations from the same execbuf, and usually those get recycled around
the same time. Which means you'll just fragment your mm even more if you
try to find the best hole instead of just picking one and the stuffing
the entire execbuf into it. So imo we might as well just kill it.»

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---

This patch replaces the previous patches 4 and 5.

Not sure about the function header comment hunk, maybe what it says is
true even with DRM_MM_SEARCH_BELOW?

 drivers/gpu/drm/drm_mm.c             | 36 +++++-------------------------------
 drivers/gpu/drm/ttm/ttm_bo_manager.c |  2 +-
 include/drm/drm_mm.h                 |  3 +--
 3 files changed, 7 insertions(+), 34 deletions(-)

Comments

Alex Deucher Oct. 31, 2014, 6:10 p.m. UTC | #1 
On Thu, Oct 30, 2014 at 4:36 AM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> In the words of Daniel Vetter:
>
> «I think SEARCH_BEST is pretty much always a bad idea - it rips apart
> allocations from the same execbuf, and usually those get recycled around
> the same time. Which means you'll just fragment your mm even more if you
> try to find the best hole instead of just picking one and the stuffing
> the entire execbuf into it. So imo we might as well just kill it.»
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Applied this series to my -next tree.

Thanks,

Alex

> ---
>
> This patch replaces the previous patches 4 and 5.
>
> Not sure about the function header comment hunk, maybe what it says is
> true even with DRM_MM_SEARCH_BELOW?
>
>  drivers/gpu/drm/drm_mm.c             | 36 +++++-------------------------------
>  drivers/gpu/drm/ttm/ttm_bo_manager.c |  2 +-
>  include/drm/drm_mm.h                 |  3 +--
>  3 files changed, 7 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index 04a209e..3e0ef8c 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -409,16 +409,11 @@ static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm,
>                                                       enum drm_mm_search_flags flags)
>  {
>         struct drm_mm_node *entry;
> -       struct drm_mm_node *best;
>         unsigned long adj_start;
>         unsigned long adj_end;
> -       unsigned long best_size;
>
>         BUG_ON(mm->scanned_blocks);
>
> -       best = NULL;
> -       best_size = ~0UL;
> -
>         __drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
>                                flags & DRM_MM_SEARCH_BELOW) {
>                 unsigned long hole_size = adj_end - adj_start;
> @@ -429,19 +424,11 @@ static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm,
>                                 continue;
>                 }
>
> -               if (!check_free_hole(adj_start, adj_end, size, alignment))
> -                       continue;
> -
> -               if (!(flags & DRM_MM_SEARCH_BEST))
> +               if (check_free_hole(adj_start, adj_end, size, alignment))
>                         return entry;
> -
> -               if (hole_size < best_size) {
> -                       best = entry;
> -                       best_size = hole_size;
> -               }
>         }
>
> -       return best;
> +       return NULL;
>  }
>
>  static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_mm *mm,
> @@ -453,16 +440,11 @@ static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_
>                                                         enum drm_mm_search_flags flags)
>  {
>         struct drm_mm_node *entry;
> -       struct drm_mm_node *best;
>         unsigned long adj_start;
>         unsigned long adj_end;
> -       unsigned long best_size;
>
>         BUG_ON(mm->scanned_blocks);
>
> -       best = NULL;
> -       best_size = ~0UL;
> -
>         __drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
>                                flags & DRM_MM_SEARCH_BELOW) {
>                 unsigned long hole_size = adj_end - adj_start;
> @@ -478,19 +460,11 @@ static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_
>                                 continue;
>                 }
>
> -               if (!check_free_hole(adj_start, adj_end, size, alignment))
> -                       continue;
> -
> -               if (!(flags & DRM_MM_SEARCH_BEST))
> +               if (check_free_hole(adj_start, adj_end, size, alignment))
>                         return entry;
> -
> -               if (hole_size < best_size) {
> -                       best = entry;
> -                       best_size = hole_size;
> -               }
>         }
>
> -       return best;
> +       return NULL;
>  }
>
>  /**
> @@ -679,7 +653,7 @@ 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 !DRM_MM_SEARCH_BEST will then
> + * immediately following drm_mm_search_free with !DRM_MM_SEARCH_BELOW will then
>   * return the just freed block (because its at the top of the free_stack list).
>   *
>   * Returns:
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> index aa0bd05..42c2aa9 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> @@ -55,7 +55,7 @@ static int ttm_bo_man_get_node(struct ttm_mem_type_manager *man,
>         struct ttm_range_manager *rman = (struct ttm_range_manager *) man->priv;
>         struct drm_mm *mm = &rman->mm;
>         struct drm_mm_node *node = NULL;
> -       enum drm_mm_search_flags sflags = DRM_MM_SEARCH_BEST;
> +       enum drm_mm_search_flags sflags = DRM_MM_SEARCH_DEFAULT;
>         enum drm_mm_allocator_flags aflags = DRM_MM_CREATE_DEFAULT;
>         unsigned long lpfn;
>         int ret;
> diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
> index a24addf..f38f7a0 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -46,8 +46,7 @@
>
>  enum drm_mm_search_flags {
>         DRM_MM_SEARCH_DEFAULT =         0,
> -       DRM_MM_SEARCH_BEST =            1 << 0,
> -       DRM_MM_SEARCH_BELOW =           1 << 1,
> +       DRM_MM_SEARCH_BELOW =           1 << 0,
>  };
>
>  enum drm_mm_allocator_flags {
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Michel Dänzer Nov. 4, 2014, 8:47 a.m. UTC | #2 
On 01.11.2014 03:10, Alex Deucher wrote:
> On Thu, Oct 30, 2014 at 4:36 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> In the words of Daniel Vetter:
>>
>> «I think SEARCH_BEST is pretty much always a bad idea - it rips apart
>> allocations from the same execbuf, and usually those get recycled around
>> the same time. Which means you'll just fragment your mm even more if you
>> try to find the best hole instead of just picking one and the stuffing
>> the entire execbuf into it. So imo we might as well just kill it.»
>>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>
> Applied this series to my -next tree.

Thanks. Unfortunately, the removal of DRM_MM_SEARCH_BEST caused 
performance regressions, presumably due to excessive eviction from VRAM, 
see https://bugs.freedesktop.org/show_bug.cgi?id=84662#c49 . I can 
reproduce a significant bad effect with the UE4 Elemental demo on my 
development machine. Please drop this patch.

The original patches 4 & 5 have the same effect, so they shouldn't be 
applied either. Maybe we can try this again once TTM is using the MM 
scan roaster for eviction.
Alex Deucher Nov. 4, 2014, 2:30 p.m. UTC | #3 
On Tue, Nov 4, 2014 at 3:47 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 01.11.2014 03:10, Alex Deucher wrote:
>>
>> On Thu, Oct 30, 2014 at 4:36 AM, Michel Dänzer <michel@daenzer.net> wrote:
>>>
>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> In the words of Daniel Vetter:
>>>
>>> «I think SEARCH_BEST is pretty much always a bad idea - it rips apart
>>> allocations from the same execbuf, and usually those get recycled around
>>> the same time. Which means you'll just fragment your mm even more if you
>>> try to find the best hole instead of just picking one and the stuffing
>>> the entire execbuf into it. So imo we might as well just kill it.»
>>>
>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>
>>
>> Applied this series to my -next tree.
>
>
> Thanks. Unfortunately, the removal of DRM_MM_SEARCH_BEST caused performance
> regressions, presumably due to excessive eviction from VRAM, see
> https://bugs.freedesktop.org/show_bug.cgi?id=84662#c49 . I can reproduce a
> significant bad effect with the UE4 Elemental demo on my development
> machine. Please drop this patch.
>
> The original patches 4 & 5 have the same effect, so they shouldn't be
> applied either. Maybe we can try this again once TTM is using the MM scan
> roaster for eviction.
>

I dropped this patch.

Thanks,

Alex
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 04a209e..3e0ef8c 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -409,16 +409,11 @@  static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm,
 						      enum drm_mm_search_flags flags)
 {
 	struct drm_mm_node *entry;
-	struct drm_mm_node *best;
 	unsigned long adj_start;
 	unsigned long adj_end;
-	unsigned long best_size;
 
 	BUG_ON(mm->scanned_blocks);
 
-	best = NULL;
-	best_size = ~0UL;
-
 	__drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
 			       flags & DRM_MM_SEARCH_BELOW) {
 		unsigned long hole_size = adj_end - adj_start;
@@ -429,19 +424,11 @@  static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm,
 				continue;
 		}
 
-		if (!check_free_hole(adj_start, adj_end, size, alignment))
-			continue;
-
-		if (!(flags & DRM_MM_SEARCH_BEST))
+		if (check_free_hole(adj_start, adj_end, size, alignment))
 			return entry;
-
-		if (hole_size < best_size) {
-			best = entry;
-			best_size = hole_size;
-		}
 	}
 
-	return best;
+	return NULL;
 }
 
 static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_mm *mm,
@@ -453,16 +440,11 @@  static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_
 							enum drm_mm_search_flags flags)
 {
 	struct drm_mm_node *entry;
-	struct drm_mm_node *best;
 	unsigned long adj_start;
 	unsigned long adj_end;
-	unsigned long best_size;
 
 	BUG_ON(mm->scanned_blocks);
 
-	best = NULL;
-	best_size = ~0UL;
-
 	__drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
 			       flags & DRM_MM_SEARCH_BELOW) {
 		unsigned long hole_size = adj_end - adj_start;
@@ -478,19 +460,11 @@  static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_
 				continue;
 		}
 
-		if (!check_free_hole(adj_start, adj_end, size, alignment))
-			continue;
-
-		if (!(flags & DRM_MM_SEARCH_BEST))
+		if (check_free_hole(adj_start, adj_end, size, alignment))
 			return entry;
-
-		if (hole_size < best_size) {
-			best = entry;
-			best_size = hole_size;
-		}
 	}
 
-	return best;
+	return NULL;
 }
 
 /**
@@ -679,7 +653,7 @@  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 !DRM_MM_SEARCH_BEST will then
+ * immediately following drm_mm_search_free with !DRM_MM_SEARCH_BELOW will then
  * return the just freed block (because its at the top of the free_stack list).
  *
  * Returns:
diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
index aa0bd05..42c2aa9 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
@@ -55,7 +55,7 @@  static int ttm_bo_man_get_node(struct ttm_mem_type_manager *man,
 	struct ttm_range_manager *rman = (struct ttm_range_manager *) man->priv;
 	struct drm_mm *mm = &rman->mm;
 	struct drm_mm_node *node = NULL;
-	enum drm_mm_search_flags sflags = DRM_MM_SEARCH_BEST;
+	enum drm_mm_search_flags sflags = DRM_MM_SEARCH_DEFAULT;
 	enum drm_mm_allocator_flags aflags = DRM_MM_CREATE_DEFAULT;
 	unsigned long lpfn;
 	int ret;
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index a24addf..f38f7a0 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -46,8 +46,7 @@ 
 
 enum drm_mm_search_flags {
 	DRM_MM_SEARCH_DEFAULT =		0,
-	DRM_MM_SEARCH_BEST =		1 << 0,
-	DRM_MM_SEARCH_BELOW =		1 << 1,
+	DRM_MM_SEARCH_BELOW =		1 << 0,
 };
 
 enum drm_mm_allocator_flags {