RE: [PATCH 3/5] drm/ttm: add page order support in ttm_pages_put

2017-11-22 Thread He, Roger 
Hi Christian:

Maybe we can get back the logic for page order 0 in ttm_pages_put.

I mean: handle order 0 with set_pages_array_wb and handle order 9 with 
set_pages_wb.
If that, no performance concern.
How about that?

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Wednesday, November 22, 2017 6:01 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/5] drm/ttm: add page order support in ttm_pages_put

That completely negates the advantage of setting write back on multiple pages 
at once.

In other words this way we wouldn't need the array any more at all and could 
remove the whole complicated handling.

I'm pretty close to saying just go ahead with that and even clean up the whole 
stuff with the static array, but I'm not sure if that doesn't result in some 
performance problems.

A possible alternative is the (untested) patch attached, this way we move the 
__free_page()/_free_pages() call out of ttm_pages_put and just need to add the 
correct number of pages to the array in the loop.

Regards,
Christian.

Am 22.11.2017 um 10:17 schrieb Roger He:
> Change-Id: Ia55b206d95812c5afcfd6cec29f580758d1f50f0
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_page_alloc.c | 19 +++
>   1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 9b48b93..2dc83c0 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -299,13 +299,16 @@ static struct ttm_page_pool *ttm_get_pool(int flags, 
> bool huge,
>   }
>   
>   /* set memory back to wb and free the pages. */ -static void 
> ttm_pages_put(struct page *pages[], unsigned npages)
> +static void ttm_pages_put(struct page *pages[], unsigned npages,
> + unsigned int order)
>   {
> - unsigned i;
> - if (set_pages_array_wb(pages, npages))
> - pr_err("Failed to set %d pages to wb!\n", npages);
> - for (i = 0; i < npages; ++i)
> - __free_page(pages[i]);
> + unsigned int i, pages_nr = (1 << order);
> +
> + for (i = 0; i < npages; ++i) {
> + if (set_pages_wb(pages[i], pages_nr))
> + pr_err("Failed to set %d pages to wb!\n", pages_nr);
> + __free_pages(pages[i], order);
> + }
>   }
>   
>   static void ttm_pool_update_free_locked(struct ttm_page_pool *pool, 
> @@ -368,7 +371,7 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, 
> unsigned nr_free,
>*/
>   spin_unlock_irqrestore(>lock, irq_flags);
>   
> - ttm_pages_put(pages_to_free, freed_pages);
> + ttm_pages_put(pages_to_free, freed_pages, pool->order);
>   if (likely(nr_free != FREE_ALL_PAGES))
>   nr_free -= freed_pages;
>   
> @@ -403,7 +406,7 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, 
> unsigned nr_free,
>   spin_unlock_irqrestore(>lock, irq_flags);
>   
>   if (freed_pages)
> - ttm_pages_put(pages_to_free, freed_pages);
> + ttm_pages_put(pages_to_free, freed_pages, pool->order);
>   out:
>   if (pages_to_free != static_buf)
>   kfree(pages_to_free);


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/ttm: add page order support in ttm_pages_put

2017-11-22 Thread Christian König 
That completely negates the advantage of setting write back on multiple 
pages at once.


In other words this way we wouldn't need the array any more at all and 
could remove the whole complicated handling.


I'm pretty close to saying just go ahead with that and even clean up the 
whole stuff with the static array, but I'm not sure if that doesn't 
result in some performance problems.


A possible alternative is the (untested) patch attached, this way we 
move the __free_page()/_free_pages() call out of ttm_pages_put and just 
need to add the correct number of pages to the array in the loop.


Regards,
Christian.

Am 22.11.2017 um 10:17 schrieb Roger He:

Change-Id: Ia55b206d95812c5afcfd6cec29f580758d1f50f0
Signed-off-by: Roger He 
---
  drivers/gpu/drm/ttm/ttm_page_alloc.c | 19 +++
  1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 9b48b93..2dc83c0 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -299,13 +299,16 @@ static struct ttm_page_pool *ttm_get_pool(int flags, bool 
huge,
  }
  
  /* set memory back to wb and free the pages. */

-static void ttm_pages_put(struct page *pages[], unsigned npages)
+static void ttm_pages_put(struct page *pages[], unsigned npages,
+   unsigned int order)
  {
-   unsigned i;
-   if (set_pages_array_wb(pages, npages))
-   pr_err("Failed to set %d pages to wb!\n", npages);
-   for (i = 0; i < npages; ++i)
-   __free_page(pages[i]);
+   unsigned int i, pages_nr = (1 << order);
+
+   for (i = 0; i < npages; ++i) {
+   if (set_pages_wb(pages[i], pages_nr))
+   pr_err("Failed to set %d pages to wb!\n", pages_nr);
+   __free_pages(pages[i], order);
+   }
  }
  
  static void ttm_pool_update_free_locked(struct ttm_page_pool *pool,

@@ -368,7 +371,7 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, 
unsigned nr_free,
 */
spin_unlock_irqrestore(>lock, irq_flags);
  
-			ttm_pages_put(pages_to_free, freed_pages);

+   ttm_pages_put(pages_to_free, freed_pages, pool->order);
if (likely(nr_free != FREE_ALL_PAGES))
nr_free -= freed_pages;
  
@@ -403,7 +406,7 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,

spin_unlock_irqrestore(>lock, irq_flags);
  
  	if (freed_pages)

-   ttm_pages_put(pages_to_free, freed_pages);
+   ttm_pages_put(pages_to_free, freed_pages, pool->order);
  out:
if (pages_to_free != static_buf)
kfree(pages_to_free);



>From 4fcc6c40e3596e12a9712b01a44d9c0265f04582 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= 
Date: Wed, 22 Nov 2017 10:54:58 +0100
Subject: [PATCH] drm/ttm: move __free_page out of ttm_pages_put
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This way we can easier free the pages with the right order.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 72ea037b7090..4deb9c8441b5 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -286,11 +286,8 @@ static struct ttm_page_pool *ttm_get_pool(int flags, bool huge,
 /* set memory back to wb and free the pages. */
 static void ttm_pages_put(struct page *pages[], unsigned npages)
 {
-	unsigned i;
 	if (set_pages_array_wb(pages, npages))
 		pr_err("Failed to set %d pages to wb!\n", npages);
-	for (i = 0; i < npages; ++i)
-		__free_page(pages[i]);
 }
 
 static void ttm_pool_update_free_locked(struct ttm_page_pool *pool,
@@ -315,7 +312,8 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,
 {
 	static struct page *static_buf[NUM_PAGES_TO_ALLOC];
 	unsigned long irq_flags;
-	struct page *p;
+	struct list_head freed;
+	struct page *p, *tmp;
 	struct page **pages_to_free;
 	unsigned freed_pages = 0,
 		 npages_to_free = nr_free;
@@ -333,6 +331,8 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,
 		return 0;
 	}
 
+	INIT_LIST_HEAD();
+
 restart:
 	spin_lock_irqsave(>lock, irq_flags);
 
@@ -345,6 +345,8 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,
 		if (freed_pages >= NUM_PAGES_TO_ALLOC) {
 			/* remove range of pages from the pool */
 			__list_del(p->lru.prev, >list);
+			/* and add to freed list */
+			__list_add(>lru, , );
 
 			ttm_pool_update_free_locked(pool, freed_pages);
 			/**
@@ -380,6 +382,8 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,