[PATCH 1/7] drm/ttm: call ttm_bo_swapout directly when ttm shrink
He, Roger
Hongbo.He at amd.com
Thu Dec 21 09:17:55 UTC 2017
Hi Thomas:
Also I wonder what testing is being performed on these changes prior to submission?
Vulkan CTS test with Per VM BO enabled.
After that, Command submit will not need to provide BO list it will use. It is helpful for performance to CPU bound games.
The reason why we enable eviction and swapout here is the test always overalloction.
Thanks
Roger(Hongbo.He)
-----Original Message-----
From: Thomas Hellstrom [mailto:thomas at shipmail.org]
Sent: Thursday, December 21, 2017 3:34 PM
To: He, Roger <Hongbo.He at amd.com>; amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
Subject: Re: [PATCH 1/7] drm/ttm: call ttm_bo_swapout directly when ttm shrink
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 <Hongbo.He at amd.com>
> ---
> 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 <linux/atomic.h>
> #include <linux/reservation.h>
>
> -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, &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(&glob->device_list_mutex);
> spin_lock_init(&glob->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(&glob->swap_lru[i]);
> INIT_LIST_HEAD(&glob->device_list);
> -
> - ttm_mem_init_shrink(&glob->shrink, ttm_bo_swapout);
> - ret = ttm_mem_register_shrink(glob->mem_glob, &glob->shrink);
> - if (unlikely(ret != 0)) {
> - pr_err("Could not register buffer object swapout\n");
> - goto out_no_shrink;
> - }
> -
> atomic_set(&glob->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(&glob->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(&bo->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(&bdev->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(&glob->lock);
> - if (glob->shrink == NULL)
> - goto out;
>
> while (ttm_zones_above_swap_target(glob, from_wq, extra)) {
> - shrink = glob->shrink;
> spin_unlock(&glob->lock);
> - ret = shrink->do_shrink(shrink);
> + ret = ttm_bo_swapout(glob->bo_glob);
> spin_lock(&glob->lock);
> if (unlikely(ret != 0))
> - goto out;
> + break;
> }
> -out:
> +
> spin_unlock(&glob->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 ttm_bo_swapout_all(struct ttm_bo_device *bdev);
> int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo);
> #endif
> diff --git a/include/drm/ttm/ttm_bo_driver.h
> b/include/drm/ttm/ttm_bo_driver.h index 5115718..934fecf 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -522,7 +522,6 @@ struct ttm_bo_global {
> struct kobject kobj;
> struct ttm_mem_global *mem_glob;
> struct page *dummy_read_page;
> - struct ttm_mem_shrink shrink;
> struct mutex device_list_mutex;
> spinlock_t lru_lock;
>
> diff --git a/include/drm/ttm/ttm_memory.h
> b/include/drm/ttm/ttm_memory.h index 2c1e359..85f3ad6 100644
> --- a/include/drm/ttm/ttm_memory.h
> +++ b/include/drm/ttm/ttm_memory.h
> @@ -37,20 +37,6 @@
> #include <linux/mm.h>
>
> /**
> - * struct ttm_mem_shrink - callback to shrink TTM memory usage.
> - *
> - * @do_shrink: The callback function.
> - *
> - * Arguments to the do_shrink functions are intended to be passed
> using
> - * inheritance. That is, the argument class derives from struct
> ttm_mem_shrink,
> - * and can be accessed using container_of().
> - */
> -
> -struct ttm_mem_shrink {
> - int (*do_shrink) (struct ttm_mem_shrink *);
> -};
> -
> -/**
> * struct ttm_mem_global - Global memory accounting structure.
> *
> * @shrink: A single callback to shrink TTM memory usage. Extend
> this @@ -76,7 +62,7 @@ struct ttm_mem_shrink {
> struct ttm_mem_zone;
> struct ttm_mem_global {
> struct kobject kobj;
> - struct ttm_mem_shrink *shrink;
> + struct ttm_bo_global *bo_glob;
> struct workqueue_struct *swap_queue;
> struct work_struct work;
> spinlock_t lock;
> @@ -90,59 +76,6 @@ struct ttm_mem_global {
> #endif
> };
>
> -/**
> - * ttm_mem_init_shrink - initialize a struct ttm_mem_shrink object
> - *
> - * @shrink: The object to initialize.
> - * @func: The callback function.
> - */
> -
> -static inline void ttm_mem_init_shrink(struct ttm_mem_shrink *shrink,
> - int (*func) (struct ttm_mem_shrink *))
> -{
> - shrink->do_shrink = func;
> -}
> -
> -/**
> - * ttm_mem_register_shrink - register a struct ttm_mem_shrink object.
> - *
> - * @glob: The struct ttm_mem_global object to register with.
> - * @shrink: An initialized struct ttm_mem_shrink object to register.
> - *
> - * Returns:
> - * -EBUSY: There's already a callback registered. (May change).
> - */
> -
> -static inline int ttm_mem_register_shrink(struct ttm_mem_global *glob,
> - struct ttm_mem_shrink *shrink)
> -{
> - spin_lock(&glob->lock);
> - if (glob->shrink != NULL) {
> - spin_unlock(&glob->lock);
> - return -EBUSY;
> - }
> - glob->shrink = shrink;
> - spin_unlock(&glob->lock);
> - return 0;
> -}
> -
> -/**
> - * ttm_mem_unregister_shrink - unregister a struct ttm_mem_shrink object.
> - *
> - * @glob: The struct ttm_mem_global object to unregister from.
> - * @shrink: A previously registert struct ttm_mem_shrink object.
> - *
> - */
> -
> -static inline void ttm_mem_unregister_shrink(struct ttm_mem_global *glob,
> - struct ttm_mem_shrink *shrink)
> -{
> - spin_lock(&glob->lock);
> - BUG_ON(glob->shrink != shrink);
> - glob->shrink = NULL;
> - spin_unlock(&glob->lock);
> -}
> -
> extern int ttm_mem_global_init(struct ttm_mem_global *glob);
> extern void ttm_mem_global_release(struct ttm_mem_global *glob);
> extern int ttm_mem_global_alloc(struct ttm_mem_global *glob,
> uint64_t memory,
More information about the dri-devel
mailing list