Re: [PATCH] drm/ttm/pool: Revert to clear-on-alloc to honor TTM_TT_FLAG_ZERO_ALLOC

2024-06-24 Thread Christian König 

Am 21.06.24 um 17:43 schrieb Nirmoy Das:

Hi Christian,

On 6/21/2024 4:54 PM, Christian König wrote:

Am 20.06.24 um 18:01 schrieb Nirmoy Das:

Currently ttm pool is not honoring TTM_TT_FLAG_ZERO_ALLOC flag and
clearing pages on free. It does help with allocation latency but 
clearing

happens even if drm driver doesn't passes the flag. If clear on free
is needed then a new flag can be added for that purpose.


Mhm, thinking more about it that will most likely get push back from 
others as well.



Agreed, it is diverting a lot from a known behavior.



How about the attached patch? We just skip clearing pages when the 
driver set the ZERO_ALLOC flag again before freeing them.


Maybe rename the flag or add a new one for that, but in general that 
looks like the option with the least impact.



I would prefer a few flag (TTM_TT_FLAG_CLEARED_ALLOC ?) which driver 
can set before freeing. I can resend the patch if you are


fine with it.



Yeah, please go ahead with that. But probably better name it 
TTM_TT_FLAG_CLEARED_ON_FREE.


Thanks,
Christian.




Regards,

Nirmoy




Regards,
Christian.



Cc: Christian Koenig 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/ttm/ttm_pool.c | 31 +--
  1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c 
b/drivers/gpu/drm/ttm/ttm_pool.c

index 6e1fd6985ffc..cbbd722185ee 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -224,15 +224,6 @@ static void ttm_pool_unmap(struct ttm_pool 
*pool, dma_addr_t dma_addr,

  /* Give pages into a specific pool_type */
  static void ttm_pool_type_give(struct ttm_pool_type *pt, struct 
page *p)

  {
-    unsigned int i, num_pages = 1 << pt->order;
-
-    for (i = 0; i < num_pages; ++i) {
-    if (PageHighMem(p))
-    clear_highpage(p + i);
-    else
-    clear_page(page_address(p + i));
-    }
-
  spin_lock(&pt->lock);
  list_add(&p->lru, &pt->pages);
  spin_unlock(&pt->lock);
@@ -240,15 +231,26 @@ static void ttm_pool_type_give(struct 
ttm_pool_type *pt, struct page *p)

  }
    /* Take pages from a specific pool_type, return NULL when 
nothing available */

-static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
+static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, 
bool clear)

  {
  struct page *p;
    spin_lock(&pt->lock);
  p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
  if (p) {
+    unsigned int i, num_pages = 1 << pt->order;
+
  atomic_long_sub(1 << pt->order, &allocated_pages);
  list_del(&p->lru);
+    if (clear) {
+    for (i = 0; i < num_pages; ++i) {
+    if (PageHighMem(p))
+    clear_highpage(p + i);
+    else
+    clear_page(page_address(p + i));
+    }
+    }
+
  }
  spin_unlock(&pt->lock);
  @@ -279,7 +281,7 @@ static void ttm_pool_type_fini(struct 
ttm_pool_type *pt)

  list_del(&pt->shrinker_list);
  spin_unlock(&shrinker_lock);
  -    while ((p = ttm_pool_type_take(pt)))
+    while ((p = ttm_pool_type_take(pt, false)))
  ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
  }
  @@ -330,7 +332,7 @@ static unsigned int ttm_pool_shrink(void)
  list_move_tail(&pt->shrinker_list, &shrinker_list);
  spin_unlock(&shrinker_lock);
  -    p = ttm_pool_type_take(pt);
+    p = ttm_pool_type_take(pt, false);
  if (p) {
  ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
  num_pages = 1 << pt->order;
@@ -457,10 +459,11 @@ int ttm_pool_alloc(struct ttm_pool *pool, 
struct ttm_tt *tt,

   num_pages;
   order = min_t(unsigned int, order, __fls(num_pages))) {
  struct ttm_pool_type *pt;
+    bool clear = tt->page_flags & TTM_TT_FLAG_ZERO_ALLOC;
    page_caching = tt->caching;
  pt = ttm_pool_select_type(pool, tt->caching, order);
-    p = pt ? ttm_pool_type_take(pt) : NULL;
+    p = pt ? ttm_pool_type_take(pt, clear) : NULL;
  if (p) {
  r = ttm_pool_apply_caching(caching, pages,
 tt->caching);
@@ -480,7 +483,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct 
ttm_tt *tt,

  if (num_pages < (1 << order))
  break;
  -    p = ttm_pool_type_take(pt);
+    p = ttm_pool_type_take(pt, clear);
  } while (p);
  }

Re: [PATCH] drm/ttm/pool: Revert to clear-on-alloc to honor TTM_TT_FLAG_ZERO_ALLOC

2024-06-21 Thread Nirmoy Das 

Hi Christian,

On 6/21/2024 4:54 PM, Christian König wrote:

Am 20.06.24 um 18:01 schrieb Nirmoy Das:

Currently ttm pool is not honoring TTM_TT_FLAG_ZERO_ALLOC flag and
clearing pages on free. It does help with allocation latency but 
clearing

happens even if drm driver doesn't passes the flag. If clear on free
is needed then a new flag can be added for that purpose.


Mhm, thinking more about it that will most likely get push back from 
others as well.



Agreed, it is diverting a lot from a known behavior.



How about the attached patch? We just skip clearing pages when the 
driver set the ZERO_ALLOC flag again before freeing them.


Maybe rename the flag or add a new one for that, but in general that 
looks like the option with the least impact.



I would prefer a few flag (TTM_TT_FLAG_CLEARED_ALLOC ?) which driver can 
set before freeing. I can resend the patch if you are


fine with it.


Regards,

Nirmoy




Regards,
Christian.



Cc: Christian Koenig 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/ttm/ttm_pool.c | 31 +--
  1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c 
b/drivers/gpu/drm/ttm/ttm_pool.c

index 6e1fd6985ffc..cbbd722185ee 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -224,15 +224,6 @@ static void ttm_pool_unmap(struct ttm_pool 
*pool, dma_addr_t dma_addr,

  /* Give pages into a specific pool_type */
  static void ttm_pool_type_give(struct ttm_pool_type *pt, struct 
page *p)

  {
-    unsigned int i, num_pages = 1 << pt->order;
-
-    for (i = 0; i < num_pages; ++i) {
-    if (PageHighMem(p))
-    clear_highpage(p + i);
-    else
-    clear_page(page_address(p + i));
-    }
-
  spin_lock(&pt->lock);
  list_add(&p->lru, &pt->pages);
  spin_unlock(&pt->lock);
@@ -240,15 +231,26 @@ static void ttm_pool_type_give(struct 
ttm_pool_type *pt, struct page *p)

  }
    /* Take pages from a specific pool_type, return NULL when nothing 
available */

-static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
+static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, 
bool clear)

  {
  struct page *p;
    spin_lock(&pt->lock);
  p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
  if (p) {
+    unsigned int i, num_pages = 1 << pt->order;
+
  atomic_long_sub(1 << pt->order, &allocated_pages);
  list_del(&p->lru);
+    if (clear) {
+    for (i = 0; i < num_pages; ++i) {
+    if (PageHighMem(p))
+    clear_highpage(p + i);
+    else
+    clear_page(page_address(p + i));
+    }
+    }
+
  }
  spin_unlock(&pt->lock);
  @@ -279,7 +281,7 @@ static void ttm_pool_type_fini(struct 
ttm_pool_type *pt)

  list_del(&pt->shrinker_list);
  spin_unlock(&shrinker_lock);
  -    while ((p = ttm_pool_type_take(pt)))
+    while ((p = ttm_pool_type_take(pt, false)))
  ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
  }
  @@ -330,7 +332,7 @@ static unsigned int ttm_pool_shrink(void)
  list_move_tail(&pt->shrinker_list, &shrinker_list);
  spin_unlock(&shrinker_lock);
  -    p = ttm_pool_type_take(pt);
+    p = ttm_pool_type_take(pt, false);
  if (p) {
  ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
  num_pages = 1 << pt->order;
@@ -457,10 +459,11 @@ int ttm_pool_alloc(struct ttm_pool *pool, 
struct ttm_tt *tt,

   num_pages;
   order = min_t(unsigned int, order, __fls(num_pages))) {
  struct ttm_pool_type *pt;
+    bool clear = tt->page_flags & TTM_TT_FLAG_ZERO_ALLOC;
    page_caching = tt->caching;
  pt = ttm_pool_select_type(pool, tt->caching, order);
-    p = pt ? ttm_pool_type_take(pt) : NULL;
+    p = pt ? ttm_pool_type_take(pt, clear) : NULL;
  if (p) {
  r = ttm_pool_apply_caching(caching, pages,
 tt->caching);
@@ -480,7 +483,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct 
ttm_tt *tt,

  if (num_pages < (1 << order))
  break;
  -    p = ttm_pool_type_take(pt);
+    p = ttm_pool_type_take(pt, clear);
  } while (p);
  }

Re: [PATCH] drm/ttm/pool: Revert to clear-on-alloc to honor TTM_TT_FLAG_ZERO_ALLOC

2024-06-21 Thread Christian König 

Am 20.06.24 um 18:01 schrieb Nirmoy Das:

Currently ttm pool is not honoring TTM_TT_FLAG_ZERO_ALLOC flag and
clearing pages on free. It does help with allocation latency but clearing
happens even if drm driver doesn't passes the flag. If clear on free
is needed then a new flag can be added for that purpose.


Mhm, thinking more about it that will most likely get push back from 
others as well.


How about the attached patch? We just skip clearing pages when the 
driver set the ZERO_ALLOC flag again before freeing them.


Maybe rename the flag or add a new one for that, but in general that 
looks like the option with the least impact.


Regards,
Christian.



Cc: Christian Koenig 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/ttm/ttm_pool.c | 31 +--
  1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 6e1fd6985ffc..cbbd722185ee 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -224,15 +224,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, 
dma_addr_t dma_addr,
  /* Give pages into a specific pool_type */
  static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
  {
-   unsigned int i, num_pages = 1 << pt->order;
-
-   for (i = 0; i < num_pages; ++i) {
-   if (PageHighMem(p))
-   clear_highpage(p + i);
-   else
-   clear_page(page_address(p + i));
-   }
-
spin_lock(&pt->lock);
list_add(&p->lru, &pt->pages);
spin_unlock(&pt->lock);
@@ -240,15 +231,26 @@ static void ttm_pool_type_give(struct ttm_pool_type *pt, 
struct page *p)
  }
  
  /* Take pages from a specific pool_type, return NULL when nothing available */

-static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
+static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, bool clear)
  {
struct page *p;
  
  	spin_lock(&pt->lock);

p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
if (p) {
+   unsigned int i, num_pages = 1 << pt->order;
+
atomic_long_sub(1 << pt->order, &allocated_pages);
list_del(&p->lru);
+   if (clear) {
+   for (i = 0; i < num_pages; ++i) {
+   if (PageHighMem(p))
+   clear_highpage(p + i);
+   else
+   clear_page(page_address(p + i));
+   }
+   }
+
}
spin_unlock(&pt->lock);
  
@@ -279,7 +281,7 @@ static void ttm_pool_type_fini(struct ttm_pool_type *pt)

list_del(&pt->shrinker_list);
spin_unlock(&shrinker_lock);
  
-	while ((p = ttm_pool_type_take(pt)))

+   while ((p = ttm_pool_type_take(pt, false)))
ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
  }
  
@@ -330,7 +332,7 @@ static unsigned int ttm_pool_shrink(void)

list_move_tail(&pt->shrinker_list, &shrinker_list);
spin_unlock(&shrinker_lock);
  
-	p = ttm_pool_type_take(pt);

+   p = ttm_pool_type_take(pt, false);
if (p) {
ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
num_pages = 1 << pt->order;
@@ -457,10 +459,11 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt 
*tt,
 num_pages;
 order = min_t(unsigned int, order, __fls(num_pages))) {
struct ttm_pool_type *pt;
+   bool clear = tt->page_flags & TTM_TT_FLAG_ZERO_ALLOC;
  
  		page_caching = tt->caching;

pt = ttm_pool_select_type(pool, tt->caching, order);
-   p = pt ? ttm_pool_type_take(pt) : NULL;
+   p = pt ? ttm_pool_type_take(pt, clear) : NULL;
if (p) {
r = ttm_pool_apply_caching(caching, pages,
   tt->caching);
@@ -480,7 +483,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
if (num_pages < (1 << order))
break;
  
-p = ttm_pool_type_take(pt);

+   p = ttm_pool_type_take(pt, clear);
} while (p);
}
  
From 466b7b315af74bae635b9245a1d9e6619a3da171 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= 
Date: Fri, 21 Jun 2024 16:50:59 +0200
Subject: [PATCH] drm/ttm: skip page clear if ZERO_ALLOC flag is set on free
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This allows the driver to clear the pages using some DMA instead of the
CPU.

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

diff --git a/drivers/gpu/drm/ttm/ttm_

[PATCH] drm/ttm/pool: Revert to clear-on-alloc to honor TTM_TT_FLAG_ZERO_ALLOC

2024-06-20 Thread Nirmoy Das 
Currently ttm pool is not honoring TTM_TT_FLAG_ZERO_ALLOC flag and
clearing pages on free. It does help with allocation latency but clearing
happens even if drm driver doesn't passes the flag. If clear on free
is needed then a new flag can be added for that purpose.

Cc: Christian Koenig 
Cc: "Thomas Hellström" 
Cc: Matthew Auld 
Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/ttm/ttm_pool.c | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 6e1fd6985ffc..cbbd722185ee 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -224,15 +224,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, 
dma_addr_t dma_addr,
 /* Give pages into a specific pool_type */
 static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
 {
-   unsigned int i, num_pages = 1 << pt->order;
-
-   for (i = 0; i < num_pages; ++i) {
-   if (PageHighMem(p))
-   clear_highpage(p + i);
-   else
-   clear_page(page_address(p + i));
-   }
-
spin_lock(&pt->lock);
list_add(&p->lru, &pt->pages);
spin_unlock(&pt->lock);
@@ -240,15 +231,26 @@ static void ttm_pool_type_give(struct ttm_pool_type *pt, 
struct page *p)
 }
 
 /* Take pages from a specific pool_type, return NULL when nothing available */
-static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
+static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, bool clear)
 {
struct page *p;
 
spin_lock(&pt->lock);
p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
if (p) {
+   unsigned int i, num_pages = 1 << pt->order;
+
atomic_long_sub(1 << pt->order, &allocated_pages);
list_del(&p->lru);
+   if (clear) {
+   for (i = 0; i < num_pages; ++i) {
+   if (PageHighMem(p))
+   clear_highpage(p + i);
+   else
+   clear_page(page_address(p + i));
+   }
+   }
+
}
spin_unlock(&pt->lock);
 
@@ -279,7 +281,7 @@ static void ttm_pool_type_fini(struct ttm_pool_type *pt)
list_del(&pt->shrinker_list);
spin_unlock(&shrinker_lock);
 
-   while ((p = ttm_pool_type_take(pt)))
+   while ((p = ttm_pool_type_take(pt, false)))
ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
 }
 
@@ -330,7 +332,7 @@ static unsigned int ttm_pool_shrink(void)
list_move_tail(&pt->shrinker_list, &shrinker_list);
spin_unlock(&shrinker_lock);
 
-   p = ttm_pool_type_take(pt);
+   p = ttm_pool_type_take(pt, false);
if (p) {
ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
num_pages = 1 << pt->order;
@@ -457,10 +459,11 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt 
*tt,
 num_pages;
 order = min_t(unsigned int, order, __fls(num_pages))) {
struct ttm_pool_type *pt;
+   bool clear = tt->page_flags & TTM_TT_FLAG_ZERO_ALLOC;
 
page_caching = tt->caching;
pt = ttm_pool_select_type(pool, tt->caching, order);
-   p = pt ? ttm_pool_type_take(pt) : NULL;
+   p = pt ? ttm_pool_type_take(pt, clear) : NULL;
if (p) {
r = ttm_pool_apply_caching(caching, pages,
   tt->caching);
@@ -480,7 +483,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
if (num_pages < (1 << order))
break;
 
-   p = ttm_pool_type_take(pt);
+   p = ttm_pool_type_take(pt, clear);
} while (p);
}
 
-- 
2.42.0