Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool

2021-03-05 Thread Christian König 

Am 05.03.21 um 00:20 schrieb John Stultz:

This patch reworks the ttm_pool logic to utilize the recently
added drm_page_pool code.

This adds drm_page_pool structures to the ttm_pool_type
structures, and then removes all the ttm_pool_type shrinker
logic (as its handled in the drm_page_pool shrinker).

NOTE: There is one mismatch in the interfaces I'm not totally
happy with. The ttm_pool tracks all of its pooled pages across
a number of different pools, and tries to keep this size under
the specified page_pool_size value. With the drm_page_pool,
there may other users, however there is still one global
shrinker list of pools. So we can't easily reduce the ttm
pool under the ttm specified size without potentially doing
a lot of shrinking to other non-ttm pools. So either we can:
   1) Try to split it so each user of drm_page_pools manages its
  own pool shrinking.
   2) Push the max value into the drm_page_pool, and have it
  manage shrinking to fit under that global max. Then share
  those size/max values out so the ttm_pool debug output
  can have more context.

I've taken the second path in this patch set, but wanted to call
it out so folks could look closely.


That's perfectly fine with me. A global approach for the different page 
pool types is desired anyway as far as I can see.




Thoughts would be greatly appreciated here!

Cc: Daniel Vetter 
Cc: Christian Koenig 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Chris Goldsworthy 
Cc: Laura Abbott 
Cc: Brian Starkey 
Cc: Hridya Valsaraju 
Cc: Suren Baghdasaryan 
Cc: Sandeep Patil 
Cc: Daniel Mentz 
Cc: Ørjan Eide 
Cc: Robin Murphy 
Cc: Ezequiel Garcia 
Cc: Simon Ser 
Cc: James Jones 
Cc: linux-me...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: John Stultz 
---
v7:
* Major refactoring to use drm_page_pools inside the
   ttm_pool_type structure. This allows us to use container_of to
   get the needed context to free a page. This also means less
   code is changed overall.
v8:
* Reworked to use the new cleanly rewritten drm_page_pool logic
---
  drivers/gpu/drm/Kconfig|   1 +
  drivers/gpu/drm/ttm/ttm_pool.c | 156 ++---
  include/drm/ttm/ttm_pool.h |   6 +-
  3 files changed, 31 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7cbcecb8f7df..a6cbdb63f6c7 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -184,6 +184,7 @@ config DRM_PAGE_POOL
  config DRM_TTM
tristate
depends on DRM && MMU
+   select DRM_PAGE_POOL
help
  GPU memory management subsystem for devices with multiple
  GPU memory types. Will be enabled automatically if a device driver
diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 6e27cb1bf48b..f74ea801d7ab 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -39,6 +39,7 @@
  #include 
  #endif
  
+#include 

  #include 
  #include 
  #include 
@@ -68,8 +69,6 @@ static struct ttm_pool_type 
global_dma32_write_combined[MAX_ORDER];
  static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];
  
  static struct mutex shrinker_lock;

-static struct list_head shrinker_list;
-static struct shrinker mm_shrinker;
  
  /* Allocate pages of size 1 << order with the given gfp_flags */

  static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t 
gfp_flags,
@@ -125,8 +124,9 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool 
*pool, gfp_t gfp_flags,
  }
  
  /* Reset the caching and pages of size 1 << order */

-static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
-  unsigned int order, struct page *p)
+static unsigned long ttm_pool_free_page(struct ttm_pool *pool,
+   enum ttm_caching caching,
+   unsigned int order, struct page *p)
  {
unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
struct ttm_pool_dma *dma;
@@ -142,7 +142,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum 
ttm_caching caching,
  
  	if (!pool || !pool->use_dma_alloc) {

__free_pages(p, order);
-   return;
+   return 1UL << order;
}
  
  	if (order)

@@ -153,6 +153,16 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum 
ttm_caching caching,
dma_free_attrs(pool->dev, (1UL << order) * PAGE_SIZE, vaddr, dma->addr,
   attr);
kfree(dma);
+   return 1UL << order;


The returned value is always the same. So you wrapper can do this and we 
don't really need to change the function here.



+}
+
+static unsigned long ttm_subpool_free_page(struct drm_page_pool *subpool,
+  struct page *p)


Better call this ttm_pool_free_callback.


+{
+   struct ttm_pool_type *pt;
+
+   pt = container_of(subpool, struct ttm_pool_type, subpool);
+

Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool

2021-03-04 Thread kernel test robot 
Hi John,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc1]
[cannot apply to linux/master drm-intel/for-linux-next drm-tip/drm-tip 
next-20210305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
3cb60ee6323968b694208c4cbd56a7176396e931
config: parisc-randconfig-r024-20210305 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# 
https://github.com/0day-ci/linux/commit/701300f424bbe73234f3dc3b62581a5e6ddef78a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
git checkout 701300f424bbe73234f3dc3b62581a5e6ddef78a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross 
ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All errors (new ones prefixed by >>):

   hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function 
`drm_page_pool_shrinker_setup':
>> (.text.drm_page_pool_shrinker_setup+0x0): multiple definition of 
>> `init_module'; drivers/gpu/drm/drm_drv.o:(.init.text+0x0): first defined here
   hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function 
`drm_page_pool_shrinker_teardown':
>> (.text.drm_page_pool_shrinker_teardown+0x0): multiple definition of 
>> `cleanup_module'; drivers/gpu/drm/drm_drv.o:(.text.drm_core_exit+0x0): first 
>> defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip

[PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool

2021-03-04 Thread John Stultz 
This patch reworks the ttm_pool logic to utilize the recently
added drm_page_pool code.

This adds drm_page_pool structures to the ttm_pool_type
structures, and then removes all the ttm_pool_type shrinker
logic (as its handled in the drm_page_pool shrinker).

NOTE: There is one mismatch in the interfaces I'm not totally
happy with. The ttm_pool tracks all of its pooled pages across
a number of different pools, and tries to keep this size under
the specified page_pool_size value. With the drm_page_pool,
there may other users, however there is still one global
shrinker list of pools. So we can't easily reduce the ttm
pool under the ttm specified size without potentially doing
a lot of shrinking to other non-ttm pools. So either we can:
  1) Try to split it so each user of drm_page_pools manages its
 own pool shrinking.
  2) Push the max value into the drm_page_pool, and have it
 manage shrinking to fit under that global max. Then share
 those size/max values out so the ttm_pool debug output
 can have more context.

I've taken the second path in this patch set, but wanted to call
it out so folks could look closely.

Thoughts would be greatly appreciated here!

Cc: Daniel Vetter 
Cc: Christian Koenig 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Chris Goldsworthy 
Cc: Laura Abbott 
Cc: Brian Starkey 
Cc: Hridya Valsaraju 
Cc: Suren Baghdasaryan 
Cc: Sandeep Patil 
Cc: Daniel Mentz 
Cc: Ørjan Eide 
Cc: Robin Murphy 
Cc: Ezequiel Garcia 
Cc: Simon Ser 
Cc: James Jones 
Cc: linux-me...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: John Stultz 
---
v7:
* Major refactoring to use drm_page_pools inside the
  ttm_pool_type structure. This allows us to use container_of to
  get the needed context to free a page. This also means less
  code is changed overall.
v8:
* Reworked to use the new cleanly rewritten drm_page_pool logic
---
 drivers/gpu/drm/Kconfig|   1 +
 drivers/gpu/drm/ttm/ttm_pool.c | 156 ++---
 include/drm/ttm/ttm_pool.h |   6 +-
 3 files changed, 31 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7cbcecb8f7df..a6cbdb63f6c7 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -184,6 +184,7 @@ config DRM_PAGE_POOL
 config DRM_TTM
tristate
depends on DRM && MMU
+   select DRM_PAGE_POOL
help
  GPU memory management subsystem for devices with multiple
  GPU memory types. Will be enabled automatically if a device driver
diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 6e27cb1bf48b..f74ea801d7ab 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -39,6 +39,7 @@
 #include 
 #endif
 
+#include 
 #include 
 #include 
 #include 
@@ -68,8 +69,6 @@ static struct ttm_pool_type 
global_dma32_write_combined[MAX_ORDER];
 static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];
 
 static struct mutex shrinker_lock;
-static struct list_head shrinker_list;
-static struct shrinker mm_shrinker;
 
 /* Allocate pages of size 1 << order with the given gfp_flags */
 static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
@@ -125,8 +124,9 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool 
*pool, gfp_t gfp_flags,
 }
 
 /* Reset the caching and pages of size 1 << order */
-static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
-  unsigned int order, struct page *p)
+static unsigned long ttm_pool_free_page(struct ttm_pool *pool,
+   enum ttm_caching caching,
+   unsigned int order, struct page *p)
 {
unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
struct ttm_pool_dma *dma;
@@ -142,7 +142,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum 
ttm_caching caching,
 
if (!pool || !pool->use_dma_alloc) {
__free_pages(p, order);
-   return;
+   return 1UL << order;
}
 
if (order)
@@ -153,6 +153,16 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum 
ttm_caching caching,
dma_free_attrs(pool->dev, (1UL << order) * PAGE_SIZE, vaddr, dma->addr,
   attr);
kfree(dma);
+   return 1UL << order;
+}
+
+static unsigned long ttm_subpool_free_page(struct drm_page_pool *subpool,
+  struct page *p)
+{
+   struct ttm_pool_type *pt;
+
+   pt = container_of(subpool, struct ttm_pool_type, subpool);
+   return ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
 }
 
 /* Apply a new caching to an array of pages */
@@ -216,40 +226,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, 
dma_addr_t dma_addr,
   DMA_BIDIRECTIONAL);
 }
 
-/* Give pages into a specific pool_type */
-static void ttm_pool_type_give(struct ttm_pool_type *pt, struct pag