Re: [PATCH 1/7] drm/ttm: call ttm_bo_swapout directly when ttm shrink

2017-12-20 Thread Thomas Hellstrom 

Roger,

5 out of 7 patches in this series are completely lacking a commit log 
message, and thats not OK. Really.


https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#describe-your-changes

I'll review these, but IIRC the no_wait in the memory accounting code is 
different in that it doesn't allow sleeping,
whereas in the bo code we had originally had no_wait_gpu and also 
no_wait. (IIRC Maarten or Jerome removed the latter due to lack of 
users.) Seems like these patches confuse the to. At the very least that 
requires some form of motivation.


Also I wonder what testing is being performed on these changes prior to 
submission?
We have pretty high number of critical deployments out there, that we 
need to support.


/Thomas


On 12/20/2017 11:34 AM, Roger He wrote:

then remove superfluous functions

Change-Id: Iea020f0e30a239e0265e7a1500168c7d7f819bd9
Signed-off-by: Roger He 
---
  drivers/gpu/drm/ttm/ttm_bo.c | 21 +++-
  drivers/gpu/drm/ttm/ttm_memory.c | 12 ++-
  include/drm/ttm/ttm_bo_api.h |  1 +
  include/drm/ttm/ttm_bo_driver.h  |  1 -
  include/drm/ttm/ttm_memory.h | 69 +---
  5 files changed, 9 insertions(+), 95 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 60bb5c1..fa57aa8 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -42,7 +42,6 @@
  #include 
  #include 
  
-static int ttm_bo_swapout(struct ttm_mem_shrink *shrink);

  static void ttm_bo_global_kobj_release(struct kobject *kobj);
  
  static struct attribute ttm_bo_count = {

@@ -1454,7 +1453,6 @@ static void ttm_bo_global_kobj_release(struct kobject 
*kobj)
struct ttm_bo_global *glob =
container_of(kobj, struct ttm_bo_global, kobj);
  
-	ttm_mem_unregister_shrink(glob->mem_glob, >shrink);

__free_page(glob->dummy_read_page);
kfree(glob);
  }
@@ -1479,6 +1477,7 @@ int ttm_bo_global_init(struct drm_global_reference *ref)
mutex_init(>device_list_mutex);
spin_lock_init(>lru_lock);
glob->mem_glob = bo_ref->mem_glob;
+   glob->mem_glob->bo_glob = glob;
glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32);
  
  	if (unlikely(glob->dummy_read_page == NULL)) {

@@ -1489,14 +1488,6 @@ int ttm_bo_global_init(struct drm_global_reference *ref)
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i)
INIT_LIST_HEAD(>swap_lru[i]);
INIT_LIST_HEAD(>device_list);
-
-   ttm_mem_init_shrink(>shrink, ttm_bo_swapout);
-   ret = ttm_mem_register_shrink(glob->mem_glob, >shrink);
-   if (unlikely(ret != 0)) {
-   pr_err("Could not register buffer object swapout\n");
-   goto out_no_shrink;
-   }
-
atomic_set(>bo_count, 0);
  
  	ret = kobject_init_and_add(

@@ -1504,8 +1495,6 @@ int ttm_bo_global_init(struct drm_global_reference *ref)
if (unlikely(ret != 0))
kobject_put(>kobj);
return ret;
-out_no_shrink:
-   __free_page(glob->dummy_read_page);
  out_no_drp:
kfree(glob);
return ret;
@@ -1688,11 +1677,8 @@ EXPORT_SYMBOL(ttm_bo_synccpu_write_release);
   * A buffer object shrink method that tries to swap out the first
   * buffer object on the bo_global::swap_lru list.
   */
-
-static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
+int ttm_bo_swapout(struct ttm_bo_global *glob)
  {
-   struct ttm_bo_global *glob =
-   container_of(shrink, struct ttm_bo_global, shrink);
struct ttm_buffer_object *bo;
int ret = -EBUSY;
unsigned i;
@@ -1774,10 +1760,11 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
kref_put(>list_kref, ttm_bo_release_list);
return ret;
  }
+EXPORT_SYMBOL(ttm_bo_swapout);
  
  void ttm_bo_swapout_all(struct ttm_bo_device *bdev)

  {
-   while (ttm_bo_swapout(>glob->shrink) == 0)
+   while (ttm_bo_swapout(bdev->glob) == 0)
;
  }
  EXPORT_SYMBOL(ttm_bo_swapout_all);
diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
index e963749..9130bdf 100644
--- a/drivers/gpu/drm/ttm/ttm_memory.c
+++ b/drivers/gpu/drm/ttm/ttm_memory.c
@@ -214,26 +214,20 @@ static void ttm_shrink(struct ttm_mem_global *glob, bool 
from_wq,
   uint64_t extra)
  {
int ret;
-   struct ttm_mem_shrink *shrink;
  
  	spin_lock(>lock);

-   if (glob->shrink == NULL)
-   goto out;
  
  	while (ttm_zones_above_swap_target(glob, from_wq, extra)) {

-   shrink = glob->shrink;
spin_unlock(>lock);
-   ret = shrink->do_shrink(shrink);
+   ret = ttm_bo_swapout(glob->bo_glob);
spin_lock(>lock);
if (unlikely(ret != 0))
-   goto out;
+   break;
}
-out:
+
spin_unlock(>lock);
  }
  
-

-
  static void ttm_shrink_work(struct

Re: [PATCH 1/7] drm/ttm: call ttm_bo_swapout directly when ttm shrink

2017-12-20 Thread Christian König 

Am 20.12.2017 um 11:34 schrieb Roger He:

then remove superfluous functions


We need a better commit message. Something like:

Remove the extra indirection, cause we have only one implementation anyway.



Change-Id: Iea020f0e30a239e0265e7a1500168c7d7f819bd9
Signed-off-by: Roger He 


With the commit message fixed Reviewed-by: Christian König 
.


Regards,
Christian.


---
  drivers/gpu/drm/ttm/ttm_bo.c | 21 +++-
  drivers/gpu/drm/ttm/ttm_memory.c | 12 ++-
  include/drm/ttm/ttm_bo_api.h |  1 +
  include/drm/ttm/ttm_bo_driver.h  |  1 -
  include/drm/ttm/ttm_memory.h | 69 +---
  5 files changed, 9 insertions(+), 95 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 60bb5c1..fa57aa8 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -42,7 +42,6 @@
  #include 
  #include 
  
-static int ttm_bo_swapout(struct ttm_mem_shrink *shrink);

  static void ttm_bo_global_kobj_release(struct kobject *kobj);
  
  static struct attribute ttm_bo_count = {

@@ -1454,7 +1453,6 @@ static void ttm_bo_global_kobj_release(struct kobject 
*kobj)
struct ttm_bo_global *glob =
container_of(kobj, struct ttm_bo_global, kobj);
  
-	ttm_mem_unregister_shrink(glob->mem_glob, >shrink);

__free_page(glob->dummy_read_page);
kfree(glob);
  }
@@ -1479,6 +1477,7 @@ int ttm_bo_global_init(struct drm_global_reference *ref)
mutex_init(>device_list_mutex);
spin_lock_init(>lru_lock);
glob->mem_glob = bo_ref->mem_glob;
+   glob->mem_glob->bo_glob = glob;
glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32);
  
  	if (unlikely(glob->dummy_read_page == NULL)) {

@@ -1489,14 +1488,6 @@ int ttm_bo_global_init(struct drm_global_reference *ref)
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i)
INIT_LIST_HEAD(>swap_lru[i]);
INIT_LIST_HEAD(>device_list);
-
-   ttm_mem_init_shrink(>shrink, ttm_bo_swapout);
-   ret = ttm_mem_register_shrink(glob->mem_glob, >shrink);
-   if (unlikely(ret != 0)) {
-   pr_err("Could not register buffer object swapout\n");
-   goto out_no_shrink;
-   }
-
atomic_set(>bo_count, 0);
  
  	ret = kobject_init_and_add(

@@ -1504,8 +1495,6 @@ int ttm_bo_global_init(struct drm_global_reference *ref)
if (unlikely(ret != 0))
kobject_put(>kobj);
return ret;
-out_no_shrink:
-   __free_page(glob->dummy_read_page);
  out_no_drp:
kfree(glob);
return ret;
@@ -1688,11 +1677,8 @@ EXPORT_SYMBOL(ttm_bo_synccpu_write_release);
   * A buffer object shrink method that tries to swap out the first
   * buffer object on the bo_global::swap_lru list.
   */
-
-static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
+int ttm_bo_swapout(struct ttm_bo_global *glob)
  {
-   struct ttm_bo_global *glob =
-   container_of(shrink, struct ttm_bo_global, shrink);
struct ttm_buffer_object *bo;
int ret = -EBUSY;
unsigned i;
@@ -1774,10 +1760,11 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
kref_put(>list_kref, ttm_bo_release_list);
return ret;
  }
+EXPORT_SYMBOL(ttm_bo_swapout);
  
  void ttm_bo_swapout_all(struct ttm_bo_device *bdev)

  {
-   while (ttm_bo_swapout(>glob->shrink) == 0)
+   while (ttm_bo_swapout(bdev->glob) == 0)
;
  }
  EXPORT_SYMBOL(ttm_bo_swapout_all);
diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
index e963749..9130bdf 100644
--- a/drivers/gpu/drm/ttm/ttm_memory.c
+++ b/drivers/gpu/drm/ttm/ttm_memory.c
@@ -214,26 +214,20 @@ static void ttm_shrink(struct ttm_mem_global *glob, bool 
from_wq,
   uint64_t extra)
  {
int ret;
-   struct ttm_mem_shrink *shrink;
  
  	spin_lock(>lock);

-   if (glob->shrink == NULL)
-   goto out;
  
  	while (ttm_zones_above_swap_target(glob, from_wq, extra)) {

-   shrink = glob->shrink;
spin_unlock(>lock);
-   ret = shrink->do_shrink(shrink);
+   ret = ttm_bo_swapout(glob->bo_glob);
spin_lock(>lock);
if (unlikely(ret != 0))
-   goto out;
+   break;
}
-out:
+
spin_unlock(>lock);
  }
  
-

-
  static void ttm_shrink_work(struct work_struct *work)
  {
struct ttm_mem_global *glob =
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index c126330..24a8db7 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -752,6 +752,7 @@ ssize_t ttm_bo_io(struct ttm_bo_device *bdev, struct file 
*filp,
  const char __user *wbuf, char __user *rbuf,
  size_t count, loff_t *f_pos, bool write);
  
+int ttm_bo_swapout(struct ttm_bo_global *glob);

  void