RE: [PATCH 3/5] drm/ttm: remove the backing store if no placement is given

[He, Roger]

Acked-by: Roger He 

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Sunday, March 25, 2018 6:58 PM
To: linaro-mm-...@lists.linaro.org; linux-me...@vger.kernel.org; 
dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; 
sumit.sem...@linaro.org
Subject: [PATCH 3/5] drm/ttm: remove the backing store if no placement is given

Pipeline removal of the BOs backing store when the placement is given during 
validation.

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

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
98e06f8bf23b..17e821f01d0a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1078,6 +1078,18 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
uint32_t new_flags;
 
reservation_object_assert_held(bo->resv);
+
+   /*
+* Remove the backing store if no placement is given.
+*/
+   if (!placement->num_placement && !placement->num_busy_placement) {
+   ret = ttm_bo_pipeline_gutting(bo);
+   if (ret)
+   return ret;
+
+   return ttm_tt_create(bo, false);
+   }
+
/*
 * Check whether we need to move buffer.
 */
--
2.14.1

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

RE: [PATCH 2/5] drm/ttm: keep a reference to transfer pipelined BOs

[He, Roger]

Reviewed-by: Roger He 

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Friday, March 16, 2018 9:21 PM
To: linaro-mm-...@lists.linaro.org; linux-me...@vger.kernel.org; 
dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Subject: [PATCH 2/5] drm/ttm: keep a reference to transfer pipelined BOs

Make sure the transfered BO is never destroy before the transfer BO.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 50 +++
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 1f730b3f18e5..1ee20558ee31 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -39,6 +39,11 @@
 #include 
 #include 
 
+struct ttm_transfer_obj {
+   struct ttm_buffer_object base;
+   struct ttm_buffer_object *bo;
+};
+
 void ttm_bo_free_old_node(struct ttm_buffer_object *bo)  {
ttm_bo_mem_put(bo, >mem);
@@ -435,7 +440,11 @@ EXPORT_SYMBOL(ttm_bo_move_memcpy);
 
 static void ttm_transfered_destroy(struct ttm_buffer_object *bo)  {
-   kfree(bo);
+   struct ttm_transfer_obj *fbo;
+
+   fbo = container_of(bo, struct ttm_transfer_obj, base);
+   ttm_bo_unref(>bo);
+   kfree(fbo);
 }
 
 /**
@@ -456,14 +465,15 @@ static void ttm_transfered_destroy(struct 
ttm_buffer_object *bo)  static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
  struct ttm_buffer_object **new_obj)  {
-   struct ttm_buffer_object *fbo;
+   struct ttm_transfer_obj *fbo;
int ret;
 
fbo = kmalloc(sizeof(*fbo), GFP_KERNEL);
if (!fbo)
return -ENOMEM;
 
-   *fbo = *bo;
+   fbo->base = *bo;
+   fbo->bo = ttm_bo_reference(bo);
 
/**
 * Fix up members that we shouldn't copy directly:
@@ -471,25 +481,25 @@ static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
 */
 
atomic_inc(>bdev->glob->bo_count);
-   INIT_LIST_HEAD(>ddestroy);
-   INIT_LIST_HEAD(>lru);
-   INIT_LIST_HEAD(>swap);
-   INIT_LIST_HEAD(>io_reserve_lru);
-   mutex_init(>wu_mutex);
-   fbo->moving = NULL;
-   drm_vma_node_reset(>vma_node);
-   atomic_set(>cpu_writers, 0);
-
-   kref_init(>list_kref);
-   kref_init(>kref);
-   fbo->destroy = _transfered_destroy;
-   fbo->acc_size = 0;
-   fbo->resv = >ttm_resv;
-   reservation_object_init(fbo->resv);
-   ret = reservation_object_trylock(fbo->resv);
+   INIT_LIST_HEAD(>base.ddestroy);
+   INIT_LIST_HEAD(>base.lru);
+   INIT_LIST_HEAD(>base.swap);
+   INIT_LIST_HEAD(>base.io_reserve_lru);
+   mutex_init(>base.wu_mutex);
+   fbo->base.moving = NULL;
+   drm_vma_node_reset(>base.vma_node);
+   atomic_set(>base.cpu_writers, 0);
+
+   kref_init(>base.list_kref);
+   kref_init(>base.kref);
+   fbo->base.destroy = _transfered_destroy;
+   fbo->base.acc_size = 0;
+   fbo->base.resv = >base.ttm_resv;
+   reservation_object_init(fbo->base.resv);
+   ret = reservation_object_trylock(fbo->base.resv);
WARN_ON(!ret);
 
-   *new_obj = fbo;
+   *new_obj = >base;
return 0;
 }
 
--
2.14.1

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

RE: [PATCH 1/6] drm/ttm: add ttm_bo_pipeline_gutting

[He, Roger]

Patch 1,4,5: Acked-by: Roger He 
Patch 2,3,6: Reviewed-by: Roger He 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Tuesday, March 06, 2018 10:44 PM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 1/6] drm/ttm: add ttm_bo_pipeline_gutting

Allows us to gut a BO of it's backing store when the driver says that it isn't 
needed any more.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo.c  | 15 ---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 24 
 include/drm/ttm/ttm_bo_driver.h   |  9 +
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
ad142a92eb80..98e06f8bf23b 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -622,14 +622,23 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
 
reservation_object_assert_held(bo->resv);
 
+   placement.num_placement = 0;
+   placement.num_busy_placement = 0;
+   bdev->driver->evict_flags(bo, );
+
+   if (!placement.num_placement && !placement.num_busy_placement) {
+   ret = ttm_bo_pipeline_gutting(bo);
+   if (ret)
+   return ret;
+
+   return ttm_tt_create(bo, false);
+   }
+
evict_mem = bo->mem;
evict_mem.mm_node = NULL;
evict_mem.bus.io_reserved_vm = false;
evict_mem.bus.io_reserved_count = 0;
 
-   placement.num_placement = 0;
-   placement.num_busy_placement = 0;
-   bdev->driver->evict_flags(bo, );
ret = ttm_bo_mem_space(bo, , _mem, ctx);
if (ret) {
if (ret != -ERESTARTSYS) {
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 6d6a3f46143b..1f730b3f18e5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -801,3 +801,27 @@ int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,
return 0;
 }
 EXPORT_SYMBOL(ttm_bo_pipeline_move);
+
+int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo) {
+   struct ttm_buffer_object *ghost;
+   int ret;
+
+   ret = ttm_buffer_object_transfer(bo, );
+   if (ret)
+   return ret;
+
+   ret = reservation_object_copy_fences(ghost->resv, bo->resv);
+   /* Last resort, wait for the BO to be idle when we are OOM */
+   if (ret)
+   ttm_bo_wait(bo, false, false);
+
+   memset(>mem, 0, sizeof(bo->mem));
+   bo->mem.mem_type = TTM_PL_SYSTEM;
+   bo->ttm = NULL;
+
+   ttm_bo_unreserve(ghost);
+   ttm_bo_unref();
+
+   return 0;
+}
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h 
index f8e2515b401f..39cd6b086d3a 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -849,6 +849,15 @@ int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,
 struct dma_fence *fence, bool evict,
 struct ttm_mem_reg *new_mem);
 
+/**
+ * ttm_bo_pipeline_gutting.
+ *
+ * @bo: A pointer to a struct ttm_buffer_object.
+ *
+ * Pipelined gutting a BO of it's backing store.
+ */
+int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo);
+
 /**
  * ttm_io_prot
  *
--
2.14.1

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

RE: [PATCH 4/5] drm/ttm: add ttm_sg_tt_init

[He, Roger]

Patch 1: Acked-by: Roger He 

Patch 2~5: Reviewed-by: Roger He 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Monday, March 05, 2018 8:07 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Ben Skeggs 
; Ilia Mirkin ; nouveau 

Subject: Re: [PATCH 4/5] drm/ttm: add ttm_sg_tt_init

Ping?

Am 27.02.2018 um 13:07 schrieb Christian König:
> Hi guys,
>
> at least on amdgpu and radeon the page array allocated by 
> ttm_dma_tt_init is completely unused in the case of DMA-buf sharing.
> So I'm trying to get rid of that by only allocating the DMA address 
> array.
>
> Now the only other user of DMA-buf together with ttm_dma_tt_init is 
> Nouveau. So my question is are you guys using the page array anywhere 
> in your kernel driver in case of a DMA-buf sharing?
>
> If no then I could just make this the default behavior for all drivers 
> and save quite a bit of memory for everybody.
>
> Thanks,
> Christian.
>
> Am 27.02.2018 um 12:49 schrieb Christian König:
>> This allows drivers to only allocate dma addresses, but not a page 
>> array.
>>
>> Signed-off-by: Christian König 
>> ---
>>   drivers/gpu/drm/ttm/ttm_tt.c | 54
>> 
>>   include/drm/ttm/ttm_tt.h |  2 ++
>>   2 files changed, 47 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c 
>> b/drivers/gpu/drm/ttm/ttm_tt.c index 8e0b525cda00..971133106ec2 
>> 100644
>> --- a/drivers/gpu/drm/ttm/ttm_tt.c
>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
>> @@ -108,6 +108,16 @@ static int
>> ttm_dma_tt_alloc_page_directory(struct ttm_dma_tt *ttm)
>>   return 0;
>>   }
>>   +static int ttm_sg_tt_alloc_page_directory(struct ttm_dma_tt *ttm)
>> +{
>> +    ttm->dma_address = kvmalloc_array(ttm->ttm.num_pages,
>> +  sizeof(*ttm->dma_address),
>> +  GFP_KERNEL | __GFP_ZERO);
>> +    if (!ttm->dma_address)
>> +    return -ENOMEM;
>> +    return 0;
>> +}
>> +
>>   #ifdef CONFIG_X86
>>   static inline int ttm_tt_set_page_caching(struct page *p,
>>     enum ttm_caching_state c_old, @@ -227,8 
>> +237,8 @@ void ttm_tt_destroy(struct ttm_tt *ttm)
>>   ttm->func->destroy(ttm);
>>   }
>>   -int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,
>> -    unsigned long size, uint32_t page_flags)
>> +void ttm_tt_init_fields(struct ttm_tt *ttm, struct ttm_bo_device 
>> +*bdev,
>> +    unsigned long size, uint32_t page_flags)
>>   {
>>   ttm->bdev = bdev;
>>   ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; @@ 
>> -236,6 +246,12 @@ int ttm_tt_init(struct ttm_tt *ttm, struct 
>> ttm_bo_device *bdev,
>>   ttm->page_flags = page_flags;
>>   ttm->state = tt_unpopulated;
>>   ttm->swap_storage = NULL;
>> +}
>> +
>> +int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,
>> +    unsigned long size, uint32_t page_flags) {
>> +    ttm_tt_init_fields(ttm, bdev, size, page_flags);
>>     if (ttm_tt_alloc_page_directory(ttm)) {
>>   ttm_tt_destroy(ttm);
>> @@ -258,12 +274,7 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, 
>> struct ttm_bo_device *bdev,
>>   {
>>   struct ttm_tt *ttm = _dma->ttm;
>>   -    ttm->bdev = bdev;
>> -    ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> -    ttm->caching_state = tt_cached;
>> -    ttm->page_flags = page_flags;
>> -    ttm->state = tt_unpopulated;
>> -    ttm->swap_storage = NULL;
>> +    ttm_tt_init_fields(ttm, bdev, size, page_flags);
>>     INIT_LIST_HEAD(_dma->pages_list);
>>   if (ttm_dma_tt_alloc_page_directory(ttm_dma)) { @@ -275,11 
>> +286,36 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct 
>> ttm_bo_device *bdev,
>>   }
>>   EXPORT_SYMBOL(ttm_dma_tt_init);
>>   +int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct 
>> ttm_bo_device *bdev,
>> +   unsigned long size, uint32_t page_flags) {
>> +    struct ttm_tt *ttm = _dma->ttm;
>> +    int ret;
>> +
>> +    ttm_tt_init_fields(ttm, bdev, size, page_flags);
>> +
>> +    INIT_LIST_HEAD(_dma->pages_list);
>> +    if (page_flags & TTM_PAGE_FLAG_SG)
>> +    ret = ttm_sg_tt_alloc_page_directory(ttm_dma);
>> +    else
>> +    ret = ttm_dma_tt_alloc_page_directory(ttm_dma);
>> +    if (ret) {
>> +    ttm_tt_destroy(ttm);
>> +    pr_err("Failed allocating page table\n");
>> +    return -ENOMEM;
>> +    }
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(ttm_sg_tt_init);
>> +
>>   void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma)
>>   {
>>   struct ttm_tt *ttm = _dma->ttm;
>>   -    kvfree(ttm->pages);
>> +    if (ttm->pages)
>> +    kvfree(ttm->pages);
>> +    else
>> +    kvfree(ttm_dma->dma_address);
>>   ttm->pages = NULL;
>>   ttm_dma->dma_address = NULL;
>>   }
>> diff --git 

RE: [PATCH 2/2] drm/ttm: cleanup ttm_tt_create

[He, Roger]

Series is:  Reviewed-by: Roger He 


-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Friday, February 23, 2018 8:25 PM
To: dri-devel@lists.freedesktop.org
Subject: [PATCH 2/2] drm/ttm: cleanup ttm_tt_create

Cleanup ttm_tt_create a bit.

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

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 
917942d03047..0ee3b8f11605 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -53,11 +53,9 @@
 int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc)  {
struct ttm_bo_device *bdev = bo->bdev;
-   int ret = 0;
uint32_t page_flags = 0;
 
reservation_object_assert_held(bo->resv);
-   bo->ttm = NULL;
 
if (bdev->need_dma32)
page_flags |= TTM_PAGE_FLAG_DMA32;
@@ -69,28 +67,27 @@ int ttm_tt_create(struct ttm_buffer_object *bo, bool 
zero_alloc)
case ttm_bo_type_device:
if (zero_alloc)
page_flags |= TTM_PAGE_FLAG_ZERO_ALLOC;
+   break;
case ttm_bo_type_kernel:
-   bo->ttm = bdev->driver->ttm_tt_create(bdev, bo->num_pages << 
PAGE_SHIFT,
- page_flags);
-   if (unlikely(bo->ttm == NULL))
-   ret = -ENOMEM;
break;
case ttm_bo_type_sg:
-   bo->ttm = bdev->driver->ttm_tt_create(bdev, bo->num_pages << 
PAGE_SHIFT,
- page_flags | 
TTM_PAGE_FLAG_SG);
-   if (unlikely(bo->ttm == NULL)) {
-   ret = -ENOMEM;
-   break;
-   }
-   bo->ttm->sg = bo->sg;
+   page_flags |= TTM_PAGE_FLAG_SG;
break;
default:
+   bo->ttm = NULL;
pr_err("Illegal buffer object type\n");
-   ret = -EINVAL;
-   break;
+   return -EINVAL;
}
 
-   return ret;
+   bo->ttm = bdev->driver->ttm_tt_create(bdev, bo->num_pages << PAGE_SHIFT,
+ page_flags);
+   if (unlikely(bo->ttm == NULL))
+   return -ENOMEM;
+
+   if (bo->type == ttm_bo_type_sg)
+   bo->ttm->sg = bo->sg;
+
+   return 0;
 }
 
 /**
--
2.14.1

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

RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[He, Roger]

I missed the Per-VM-BO share the reservation object with root bo. So context is 
not NULL here.
So,  this patch is:

Reviewed-by: Roger He <hongbo...@amd.com>

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
Sent: Friday, February 23, 2018 8:06 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

Am 23.02.2018 um 10:46 schrieb He, Roger:
>
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Christian K?nig
> Sent: Tuesday, February 20, 2018 8:58 PM
> To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
> linux-ker...@vger.kernel.org
> Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
> swapout.
>
> This solves the problem that when we swapout a BO from a domain we sometimes 
> couldn't make room for it because holding the lock blocks all other BOs with 
> this reservation object.
>
> Signed-off-by: Christian König <christian.koe...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 33 -
>   1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct 
> ttm_buffer_object *bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
>   /**
> - * Check the target bo is allowable to be evicted or swapout, including 
> cases:
> - *
> - * a. if share same reservation object with ctx->resv, have 
> assumption
> - * reservation objects should already be locked, so not lock again 
> and
> - * return true directly when either the opreation 
> allow_reserved_eviction
> - * or the target bo already is in delayed free list;
> - *
> - * b. Otherwise, trylock it.
> + * Check if the target bo is allowed to be evicted or swapedout.
>*/
>   static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> - struct ttm_operation_ctx *ctx, bool *locked)
> +struct ttm_operation_ctx *ctx,
> +bool *locked)
>   {
> - bool ret = false;
> + /* First check if we can lock it */
> + *locked = reservation_object_trylock(bo->resv);
> + if (*locked)
> + return true;
>   
> - *locked = false;
> + /* Check if it's locked because it is part of the current operation 
> +*/
>   if (bo->resv == ctx->resv) {
>   reservation_object_assert_held(bo->resv);
> - if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
> - ret = true;
> - } else {
> - *locked = reservation_object_trylock(bo->resv);
> - ret = *locked;
> + return ctx->allow_reserved_eviction ||
> + !list_empty(>ddestroy);
>   }
>   
> - return ret;
> + /* Check if it's locked because it was already evicted */
> + if (ww_mutex_is_owned_by(>resv->lock, NULL))
> + return true;
>
> For the special case: when command submission with Per-VM-BO enabled, 
> All BOs  a/b/c are always valid BO. After the validation of BOs a and 
> b, when validation of BO c, is it possible to return true and then evict BO a 
> and b by mistake ?
> Because a/b/c share same task_struct.

No, that's why I check the context as well. BOs explicitly reserved 
have a non NULL context while BOs trylocked for swapout havea NULL 
context.

BOs have a non NULL context only when command submission and reserved 
by ttm_eu_re6serve_buffers  .
But for Per-VM-BO a/b/c they always are not in BO list, so they will be 
not reserved and have always NULL context.
So above case also can happen. Anything missing here?  

>
> + /* Some other thread is using it, don't touch it */
> + return false;
>   }
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> --
> 2.14.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Friday, February 23, 2018 8:06 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

Am 23.02.2018 um 10:46 schrieb He, Roger:
>
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Christian K?nig
> Sent: Tuesday, February 20, 2018 8:58 PM
> To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
> linux-ker...@vger.kernel.org
> Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
> swapout.
>
> This solves the problem that when we swapout a BO from a domain we sometimes 
> couldn't make room for it because holding the lock blocks all other BOs with 
> this reservation object.
>
> Signed-off-by: Christian König <christian.koe...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 33 -
>   1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct 
> ttm_buffer_object *bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
>   /**
> - * Check the target bo is allowable to be evicted or swapout, including 
> cases:
> - *
> - * a. if share same reservation object with ctx->resv, have 
> assumption
> - * reservation objects should already be locked, so not lock again 
> and
> - * return true directly when either the opreation 
> allow_reserved_eviction
> - * or the target bo already is in delayed free list;
> - *
> - * b. Otherwise, trylock it.
> + * Check if the target bo is allowed to be evicted or swapedout.
>*/
>   static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> - struct ttm_operation_ctx *ctx, bool *locked)
> +struct ttm_operation_ctx *ctx,
> +bool *locked)
>   {
> - bool ret = false;
> + /* First check if we can lock it */
> + *locked = reservation_object_trylock(bo->resv);
> + if (*locked)
> + return true;
>   
> - *locked = false;
> + /* Check if it's locked because it is part of the current operation 
> +*/
>   if (bo->resv == ctx->resv) {
>   reservation_object_assert_held(bo->resv);
> - if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
> - ret = true;
> - } else {
> - *locked = reservation_object_trylock(bo->resv);
> - ret = *locked;
> + return ctx->allow_reserved_eviction ||
> + !list_empty(>ddestroy);
>   }
>   
> - return ret;
> + /* Check if it's locked because it was already evicted */
> + if (ww_mutex_is_owned_by(>resv->lock, NULL))
> + return true;
>
> For the special case: when command submission with Per-VM-BO enabled, 
> All BOs  a/b/c are always valid BO. After the validation of BOs a and 
> b, when validation of BO c, is it possible to return true and then evict BO a 
> and b by mistake ?
> Because a/b/c share same task_struct.

No, that's why I check the context as well. BOs explicitly reserved 
have a non NULL context while BOs trylocked for swapout have a NULL context.

When BOs have a non NULL context only when command submission and reserved by 
ttm_eu_re6serve_buffers  .
But for Per-VM-BO a/b/c they always are not in BO list, so they will be not 
reserved and have always NULL context.
So above case also can happen. Anything missing here?  

Thanks
Roger(Hongbo.He)
>
> + /* Some other thread is using it, don't touch it */
> + return false;
>   }
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> --
> 2.14.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

RE: [PATCH 2/4] drm/amdgpu: use new ww_mutex_is_owned_by function

[He, Roger]

Reviewed-by: Roger He 

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Tuesday, February 20, 2018 8:58 PM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
linux-ker...@vger.kernel.org
Subject: [PATCH 2/4] drm/amdgpu: use new ww_mutex_is_owned_by function

Instead of accessing ww_mutex internals directly use the provided function to 
check if the ww_mutex was indeed locked by the current command submission.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index eaa3cb0c3ad1..6db26a3144dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1594,7 +1594,7 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser 
*parser,
*map = mapping;
 
/* Double check that the BO is reserved by this CS */
-   if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != >ticket)
+   if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, >ticket))
return -EINVAL;
 
if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
--
2.14.1

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

RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[He, Roger]

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Tuesday, February 20, 2018 8:58 PM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
linux-ker...@vger.kernel.org
Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

This solves the problem that when we swapout a BO from a domain we sometimes 
couldn't make room for it because holding the lock blocks all other BOs with 
this reservation object.

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

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
d90b1cf10b27..3a44c2ee4155 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
*bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 /**
- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
  */
 static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
-   struct ttm_operation_ctx *ctx, bool *locked)
+  struct ttm_operation_ctx *ctx,
+  bool *locked)
 {
-   bool ret = false;
+   /* First check if we can lock it */
+   *locked = reservation_object_trylock(bo->resv);
+   if (*locked)
+   return true;
 
-   *locked = false;
+   /* Check if it's locked because it is part of the current operation */
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
-   if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
-   ret = true;
-   } else {
-   *locked = reservation_object_trylock(bo->resv);
-   ret = *locked;
+   return ctx->allow_reserved_eviction ||
+   !list_empty(>ddestroy);
}
 
-   return ret;
+   /* Check if it's locked because it was already evicted */
+   if (ww_mutex_is_owned_by(>resv->lock, NULL))
+   return true;

For the special case: when command submission with Per-VM-BO enabled,
All BOs  a/b/c are always valid BO. After the validation of BOs a and b,  
when validation of BO c, is it possible to return true and then evict BO a and 
b by mistake ?
Because a/b/c share same task_struct.

Thanks
Roger(Hongbo.He)

+   /* Some other thread is using it, don't touch it */
+   return false;
 }
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
--
2.14.1

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

RE: [PATCH 4/4] drm/ttm: keep BOs reserved until end of eviction

[He, Roger]

looks good to me.  Reviewed-by: Roger He 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Tuesday, February 20, 2018 8:58 PM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
linux-ker...@vger.kernel.org
Subject: [PATCH 4/4] drm/ttm: keep BOs reserved until end of eviction

This avoids problems when BOs are evicted but directly moved back into the 
domain from other threads.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 37 +
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
3a44c2ee4155..593a0216faff 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -742,7 +742,8 @@ static bool ttm_bo_evict_swapout_allowable(struct 
ttm_buffer_object *bo,  static int ttm_mem_evict_first(struct ttm_bo_device 
*bdev,
   uint32_t mem_type,
   const struct ttm_place *place,
-  struct ttm_operation_ctx *ctx)
+  struct ttm_operation_ctx *ctx,
+  struct list_head *evicted)
 {
struct ttm_bo_global *glob = bdev->glob;
struct ttm_mem_type_manager *man = >man[mem_type]; @@ -792,17 
+793,28 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 
ret = ttm_bo_evict(bo, ctx);
if (locked) {
-   ttm_bo_unreserve(bo);
+   list_add_tail(>lru, evicted);
} else {
spin_lock(>lru_lock);
ttm_bo_add_to_lru(bo);
spin_unlock(>lru_lock);
+   kref_put(>list_kref, ttm_bo_release_list);
}
 
-   kref_put(>list_kref, ttm_bo_release_list);
return ret;
 }
 
+static void ttm_mem_evict_cleanup(struct list_head *evicted) {
+   struct ttm_buffer_object *bo, *tmp;
+
+   list_for_each_entry_safe(bo, tmp, evicted, lru) {
+   list_del_init(>lru);
+   ttm_bo_unreserve(bo);
+   kref_put(>list_kref, ttm_bo_release_list);
+   }
+}
+
 void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem)  {
struct ttm_mem_type_manager *man = >bdev->man[mem->mem_type]; @@ 
-852,20 +864,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object 
*bo,  {
struct ttm_bo_device *bdev = bo->bdev;
struct ttm_mem_type_manager *man = >man[mem_type];
+   struct list_head evicted;
int ret;
 
+   INIT_LIST_HEAD();
do {
ret = (*man->func->get_node)(man, bo, place, mem);
if (unlikely(ret != 0))
return ret;
if (mem->mm_node)
break;
-   ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
+   ret = ttm_mem_evict_first(bdev, mem_type, place, ctx, );
if (unlikely(ret != 0))
-   return ret;
+   goto error;
} while (1);
mem->mem_type = mem_type;
-   return ttm_bo_add_move_fence(bo, man, mem);
+   ret = ttm_bo_add_move_fence(bo, man, mem);
+
+error:
+   ttm_mem_evict_cleanup();
+   return ret;
 }
 
 static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man, @@ 
-1345,6 +1363,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device 
*bdev,
struct ttm_operation_ctx ctx = { false, false };
struct ttm_mem_type_manager *man = >man[mem_type];
struct ttm_bo_global *glob = bdev->glob;
+   struct list_head evicted;
struct dma_fence *fence;
int ret;
unsigned i;
@@ -1352,18 +1371,20 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device 
*bdev,
/*
 * Can't use standard list traversal since we're unlocking.
 */
-
+   INIT_LIST_HEAD();
spin_lock(>lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
while (!list_empty(>lru[i])) {
spin_unlock(>lru_lock);
-   ret = ttm_mem_evict_first(bdev, mem_type, NULL, );
+   ret = ttm_mem_evict_first(bdev, mem_type, NULL, ,
+ );
if (ret)
return ret;
spin_lock(>lru_lock);
}
}
spin_unlock(>lru_lock);
+   ttm_mem_evict_cleanup();
 
spin_lock(>move_lock);
fence = dma_fence_get(man->move);
--
2.14.1

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

RE: [PATCH] drm/ttm: check if free mem space is under the lower limit

[He, Roger]

-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com] 
Sent: Thursday, February 22, 2018 10:06 PM
To: He, Roger <hongbo...@amd.com>
Cc: Koenig, Christian <christian.koe...@amd.com>; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/ttm: check if free mem space is under the lower limit

On Thu, Feb 22, 2018 at 6:43 AM, He, Roger <hongbo...@amd.com> wrote:
>
>
> -Original Message-
> From: Koenig, Christian
> Sent: Thursday, February 22, 2018 7:28 PM
> To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm/ttm: check if free mem space is under the 
> lower limit
>
> Am 22.02.2018 um 11:10 schrieb Roger He:
>> the free mem space and the lower limit both include two parts:
>> system memory and swap space.
>>
>> For the OOM triggered by TTM, that is the case as below:
>> first swap space is full of swapped out pages and soon system memory 
>> also is filled up with ttm pages. and then any memory allocation 
>> request will run into OOM.
>>
>> to cover two cases:
>> a. if no swap disk at all or free swap space is under swap mem
>> limit but available system mem is bigger than sys mem limit,
>> allow TTM allocation;
>>
>> b. if the available system mem is less than sys mem limit but
>> free swap space is bigger than swap mem limit, allow TTM
>> allocation.
>>
>> v2: merge two memory limit(swap and system) into one
>> v3: keep original behavior except ttm_opt_ctx->flags with
>>  TTM_OPT_FLAG_FORCE_ALLOC
>> v4: always set force_alloc as tx->flags & TTM_OPT_FLAG_FORCE_ALLOC
>> v5: add an attribute for lower_mem_limit
>>
>> Signed-off-by: Roger He <hongbo...@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_memory.c | 94 
>> 
>>   drivers/gpu/drm/ttm/ttm_page_alloc.c |  3 +
>>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  3 +
>>   include/drm/ttm/ttm_memory.h |  5 ++
>>   4 files changed, 105 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c
>> b/drivers/gpu/drm/ttm/ttm_memory.c
>> index aa0c381..d015e39 100644
>> --- a/drivers/gpu/drm/ttm/ttm_memory.c
>> +++ b/drivers/gpu/drm/ttm/ttm_memory.c
>> @@ -36,6 +36,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>
>>   #define TTM_MEMORY_ALLOC_RETRIES 4
>>
>> @@ -166,6 +167,52 @@ static struct kobj_type ttm_mem_zone_kobj_type = {
>>   .default_attrs = ttm_mem_zone_attrs,
>>   };
>>
>> +static struct attribute ttm_mem_global_lower_mem_limit = {
>> + .name = "lower_mem_limit",
>> + .mode = S_IRUGO | S_IWUSR
>> +};
>> +
>> +static ssize_t ttm_mem_global_show(struct kobject *kobj,
>> +  struct attribute *attr,
>> +  char *buffer) {
>> + struct ttm_mem_global *glob =
>> + container_of(kobj, struct ttm_mem_global, kobj);
>> + uint64_t val = 0;
>> +
>> + spin_lock(>lock);
>> + val = glob->lower_mem_limit;
>> + spin_unlock(>lock);
>> +
>> + return snprintf(buffer, PAGE_SIZE, "%llu\n",
>> + (unsigned long long) val << 2);
>
> What is that shift good for?
>
> Because the unit of lower_mem_limit is 4KB, so (val << 2) can get KB for 
> consistent with other parameters as below(all are showed with KB bytes.):

Might want to add a comment or use a define for the shift so others 
doen't get confused in the future.

Sure.

Thanks
Roger(Hongbo.He)
>
> static struct attribute *ttm_mem_zone_attrs[] = {
> _mem_sys,
> _mem_emer,
> _mem_max,
> _mem_swap,
> _mem_used,
> NULL
> };
>
>> +}
>> +
>> +static ssize_t ttm_mem_global_store(struct kobject *kobj,
>> +   struct attribute *attr,
>> +   const char *buffer,
>> +   size_t size) {
>> + int chars;
>> + uint64_t val64;
>> + unsigned long val;
>> + struct ttm_mem_global *glob =
>> + container_of(kobj, struct ttm_mem_global, kobj);
>> +
>> + chars = sscanf(buffer, "%lu", );
>> + if (chars == 0)
>> + return size;
>> +
>> + val64 = val;
>> + val64 >>= 2;
>
> Dito?
> Covert from KB to 4K page size here.
>
>> +
>> + spin_lock(&

RE: [PATCH 8/8] drm/bochs: remove the default ttm_tt_populate callbacks

[He, Roger]

Series is:  Reviewed-by: Roger He 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Thursday, February 22, 2018 7:16 PM
To: dri-devel@lists.freedesktop.org
Subject: [PATCH 8/8] drm/bochs: remove the default ttm_tt_populate callbacks

TTM calls the default implementation now.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/bochs/bochs_mm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 704e879711e4..5525b6660340 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -196,8 +196,6 @@ static struct ttm_tt *bochs_ttm_tt_create(struct 
ttm_bo_device *bdev,
 
 struct ttm_bo_driver bochs_bo_driver = {
.ttm_tt_create = bochs_ttm_tt_create,
-   .ttm_tt_populate = ttm_pool_populate,
-   .ttm_tt_unpopulate = ttm_pool_unpopulate,
.init_mem_type = bochs_bo_init_mem_type,
.eviction_valuable = ttm_bo_eviction_valuable,
.evict_flags = bochs_bo_evict_flags,
-- 
2.14.1

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

RE: [PATCH 6/6] drm/ttm: drop persistent_swap_storage from ttm_bo_init and co

[He, Roger]

Series is:  Reviewed-by: Roger He 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Thursday, February 22, 2018 11:02 PM
To: dri-devel@lists.freedesktop.org
Subject: [PATCH 6/6] drm/ttm: drop persistent_swap_storage from ttm_bo_init and 
co

Never used as parameter, the only driver actually using this is nouveau and 
there it is initialized after the BO is initialized.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  4 ++--
 drivers/gpu/drm/ast/ast_ttm.c   |  2 +-
 drivers/gpu/drm/bochs/bochs_mm.c|  2 +-
 drivers/gpu/drm/cirrus/cirrus_ttm.c |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_ttm.c   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_bo.c|  2 +-
 drivers/gpu/drm/qxl/qxl_object.c|  2 +-
 drivers/gpu/drm/radeon/radeon_object.c  |  4 ++--
 drivers/gpu/drm/ttm/ttm_bo.c|  9 ++---
 drivers/gpu/drm/virtio/virtgpu_object.c |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c  |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_mob.c |  5 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c|  4 ++--
 drivers/staging/vboxvideo/vbox_ttm.c|  2 +-
 include/drm/ttm/ttm_bo_api.h| 16 +---
 16 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index c2a4b7215c46..216799ccb545 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -418,8 +418,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
amdgpu_ttm_placement_from_domain(bo, domain);
 
r = ttm_bo_init_reserved(>mman.bdev, >tbo, size, type,
->placement, page_align, , NULL,
-acc_size, sg, resv, _ttm_bo_destroy);
+>placement, page_align, , acc_size,
+sg, resv, _ttm_bo_destroy);
if (unlikely(r != 0))
return r;
 
diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c 
index 77d2035dc7b7..211224f6bdd3 100644
--- a/drivers/gpu/drm/ast/ast_ttm.c
+++ b/drivers/gpu/drm/ast/ast_ttm.c
@@ -321,7 +321,7 @@ int ast_bo_create(struct drm_device *dev, int size, int 
align,
 
ret = ttm_bo_init(>ttm.bdev, >bo, size,
  ttm_bo_type_device, >placement,
- align >> PAGE_SHIFT, false, NULL, acc_size,
+ align >> PAGE_SHIFT, false, acc_size,
  NULL, NULL, ast_bo_ttm_destroy);
if (ret)
goto error;
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 96edf005bfea..73722484e12b 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -368,7 +368,7 @@ static int bochs_bo_create(struct drm_device *dev, int 
size, int align,
 
ret = ttm_bo_init(>ttm.bdev, >bo, size,
  ttm_bo_type_device, >placement,
- align >> PAGE_SHIFT, false, NULL, acc_size,
+ align >> PAGE_SHIFT, false, acc_size,
  NULL, NULL, bochs_bo_ttm_destroy);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c 
b/drivers/gpu/drm/cirrus/cirrus_ttm.c
index 3413389c0fbe..6cd0233b3bf8 100644
--- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
+++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
@@ -328,7 +328,7 @@ int cirrus_bo_create(struct drm_device *dev, int size, int 
align,
 
ret = ttm_bo_init(>ttm.bdev, >bo, size,
  ttm_bo_type_device, >placement,
- align >> PAGE_SHIFT, false, NULL, acc_size,
+ align >> PAGE_SHIFT, false, acc_size,
  NULL, NULL, cirrus_bo_ttm_destroy);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c 
b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
index 50e317a2a4ca..8dfffdbb6b07 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
@@ -317,7 +317,7 @@ int hibmc_bo_create(struct drm_device *dev, int size, int 
align,
 
ret = ttm_bo_init(>bdev, >bo, size,
  ttm_bo_type_device, >placement,
- align >> PAGE_SHIFT, false, NULL, acc_size,
+ align >> PAGE_SHIFT, false, acc_size,
  NULL, NULL, hibmc_bo_ttm_destroy);
if (ret) {
hibmc_bo_unref();
diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c 
b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index cd55ff5f0f0a..69beb2046008 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ 

RE: [PATCH] drm/ttm: check if free mem space is under the lower limit

[He, Roger]

-Original Message-
From: Koenig, Christian 
Sent: Thursday, February 22, 2018 8:54 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/ttm: check if free mem space is under the lower limit

Am 22.02.2018 um 12:43 schrieb He, Roger:
>
> -Original Message-
> From: Koenig, Christian
> Sent: Thursday, February 22, 2018 7:28 PM
> To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm/ttm: check if free mem space is under the 
> lower limit
>
> Am 22.02.2018 um 11:10 schrieb Roger He:
>> the free mem space and the lower limit both include two parts:
>> system memory and swap space.
>>
>> For the OOM triggered by TTM, that is the case as below:
>> first swap space is full of swapped out pages and soon system memory 
>> also is filled up with ttm pages. and then any memory allocation 
>> request will run into OOM.
>>
>> to cover two cases:
>> a. if no swap disk at all or free swap space is under swap mem
>>  limit but available system mem is bigger than sys mem limit,
>>  allow TTM allocation;
>>
>> b. if the available system mem is less than sys mem limit but
>>  free swap space is bigger than swap mem limit, allow TTM
>>  allocation.
>>
>> v2: merge two memory limit(swap and system) into one
>> v3: keep original behavior except ttm_opt_ctx->flags with
>>   TTM_OPT_FLAG_FORCE_ALLOC
>> v4: always set force_alloc as tx->flags & TTM_OPT_FLAG_FORCE_ALLOC
>> v5: add an attribute for lower_mem_limit
>>
>> Signed-off-by: Roger He <hongbo...@amd.com>
>> ---
>>drivers/gpu/drm/ttm/ttm_memory.c | 94 
>> 
>>drivers/gpu/drm/ttm/ttm_page_alloc.c |  3 +
>>drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  3 +
>>include/drm/ttm/ttm_memory.h |  5 ++
>>4 files changed, 105 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c
>> b/drivers/gpu/drm/ttm/ttm_memory.c
>> index aa0c381..d015e39 100644
>> --- a/drivers/gpu/drm/ttm/ttm_memory.c
>> +++ b/drivers/gpu/drm/ttm/ttm_memory.c
>> @@ -36,6 +36,7 @@
>>#include 
>>#include 
>>#include 
>> +#include 
>>
>>#define TTM_MEMORY_ALLOC_RETRIES 4
>>
>> @@ -166,6 +167,52 @@ static struct kobj_type ttm_mem_zone_kobj_type = {
>>  .default_attrs = ttm_mem_zone_attrs,
>>};
>>
>> +static struct attribute ttm_mem_global_lower_mem_limit = {
>> +.name = "lower_mem_limit",
>> +.mode = S_IRUGO | S_IWUSR
>> +};
>> +
>> +static ssize_t ttm_mem_global_show(struct kobject *kobj,
>> + struct attribute *attr,
>> + char *buffer)
>> +{
>> +struct ttm_mem_global *glob =
>> +container_of(kobj, struct ttm_mem_global, kobj);
>> +uint64_t val = 0;
>> +
>> +spin_lock(>lock);
>> +val = glob->lower_mem_limit;
>> +spin_unlock(>lock);
>> +
>> +return snprintf(buffer, PAGE_SIZE, "%llu\n",
>> +(unsigned long long) val << 2);
>   What is that shift good for?
>
> Because the unit of lower_mem_limit is 4KB, so (val << 2) can get KB for 
> consistent with other parameters as below(all are showed with KB bytes.):

Ok that makes sense.

>
> static struct attribute *ttm_mem_zone_attrs[] = {
>   _mem_sys,
>   _mem_emer,
>   _mem_max,
>   _mem_swap,
>   _mem_used,
>   NULL
> };
>   
>> +}
>> +
>> +static ssize_t ttm_mem_global_store(struct kobject *kobj,
>> +  struct attribute *attr,
>> +  const char *buffer,
>> +  size_t size)
>> +{
>> +int chars;
>> +uint64_t val64;
>> +unsigned long val;
>> +struct ttm_mem_global *glob =
>> +container_of(kobj, struct ttm_mem_global, kobj);
>> +
>> +chars = sscanf(buffer, "%lu", );
>> +if (chars == 0)
>> +return size;
>> +
>> +val64 = val;
>> +val64 >>= 2;
>   Dito?
> Covert from KB to 4K page size here.
>
>> +
>> +spin_lock(>lock);
>> +glob->lower_mem_limit = val64;
>> +spin_unlock(>lock);
>> +
>> +return size;
>> +}
>> +
>>static void ttm_mem_global_kobj_release(struct kobject *kobj)
>>{
>>  struc

RE: [PATCH] drm/ttm: check if free mem space is under the lower limit

[He, Roger]

-Original Message-
From: Koenig, Christian 
Sent: Thursday, February 22, 2018 7:28 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/ttm: check if free mem space is under the lower limit

Am 22.02.2018 um 11:10 schrieb Roger He:
> the free mem space and the lower limit both include two parts:
> system memory and swap space.
>
> For the OOM triggered by TTM, that is the case as below:
> first swap space is full of swapped out pages and soon system memory 
> also is filled up with ttm pages. and then any memory allocation 
> request will run into OOM.
>
> to cover two cases:
> a. if no swap disk at all or free swap space is under swap mem
> limit but available system mem is bigger than sys mem limit,
> allow TTM allocation;
>
> b. if the available system mem is less than sys mem limit but
> free swap space is bigger than swap mem limit, allow TTM
> allocation.
>
> v2: merge two memory limit(swap and system) into one
> v3: keep original behavior except ttm_opt_ctx->flags with
>  TTM_OPT_FLAG_FORCE_ALLOC
> v4: always set force_alloc as tx->flags & TTM_OPT_FLAG_FORCE_ALLOC
> v5: add an attribute for lower_mem_limit
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_memory.c | 94 
> 
>   drivers/gpu/drm/ttm/ttm_page_alloc.c |  3 +
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  3 +
>   include/drm/ttm/ttm_memory.h |  5 ++
>   4 files changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c 
> b/drivers/gpu/drm/ttm/ttm_memory.c
> index aa0c381..d015e39 100644
> --- a/drivers/gpu/drm/ttm/ttm_memory.c
> +++ b/drivers/gpu/drm/ttm/ttm_memory.c
> @@ -36,6 +36,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   
>   #define TTM_MEMORY_ALLOC_RETRIES 4
>   
> @@ -166,6 +167,52 @@ static struct kobj_type ttm_mem_zone_kobj_type = {
>   .default_attrs = ttm_mem_zone_attrs,
>   };
>   
> +static struct attribute ttm_mem_global_lower_mem_limit = {
> + .name = "lower_mem_limit",
> + .mode = S_IRUGO | S_IWUSR
> +};
> +
> +static ssize_t ttm_mem_global_show(struct kobject *kobj,
> +  struct attribute *attr,
> +  char *buffer)
> +{
> + struct ttm_mem_global *glob =
> + container_of(kobj, struct ttm_mem_global, kobj);
> + uint64_t val = 0;
> +
> + spin_lock(>lock);
> + val = glob->lower_mem_limit;
> + spin_unlock(>lock);
> +
> + return snprintf(buffer, PAGE_SIZE, "%llu\n",
> + (unsigned long long) val << 2);

What is that shift good for?

Because the unit of lower_mem_limit is 4KB, so (val << 2) can get KB for 
consistent with other parameters as below(all are showed with KB bytes.):

static struct attribute *ttm_mem_zone_attrs[] = {
_mem_sys,
_mem_emer,
_mem_max,
_mem_swap,
_mem_used,
NULL
};
 
> +}
> +
> +static ssize_t ttm_mem_global_store(struct kobject *kobj,
> +   struct attribute *attr,
> +   const char *buffer,
> +   size_t size)
> +{
> + int chars;
> + uint64_t val64;
> + unsigned long val;
> + struct ttm_mem_global *glob =
> + container_of(kobj, struct ttm_mem_global, kobj);
> +
> + chars = sscanf(buffer, "%lu", );
> + if (chars == 0)
> + return size;
> +
> + val64 = val;
> + val64 >>= 2;

Dito?
Covert from KB to 4K page size here.

> +
> + spin_lock(>lock);
> + glob->lower_mem_limit = val64;
> + spin_unlock(>lock);
> +
> + return size;
> +}
> +
>   static void ttm_mem_global_kobj_release(struct kobject *kobj)
>   {
>   struct ttm_mem_global *glob =
> @@ -174,8 +221,20 @@ static void ttm_mem_global_kobj_release(struct kobject 
> *kobj)
>   kfree(glob);
>   }
>   
> +static struct attribute *ttm_mem_global_attrs[] = {
> + _mem_global_lower_mem_limit,
> + NULL
> +};
> +
> +static const struct sysfs_ops ttm_mem_global_ops = {
> + .show = _mem_global_show,
> + .store = _mem_global_store,
> +};
> +
>   static struct kobj_type ttm_mem_glob_kobj_type = {
>   .release = _mem_global_kobj_release,
> + .sysfs_ops = _mem_global_ops,
> + .default_attrs = ttm_mem_global_attrs,
>   };
>   
>   static bool ttm_zones_above_swap_target(struct ttm_mem_global *glob, 
> @@ -375,6 +434,11 @@ int ttm_mem_global_init(struct ttm_mem_global

RE: [radeon-alex:amd-staging-dkms-4.13 3272/3830] drivers/staging//vboxvideo/vbox_ttm.c:190:9: error: incompatible type for argument 2 of 'ttm_bo_move_memcpy'

[He, Roger]

Hi Kevin:

Please help to check if the below patch is merged into staging branch?
If not, please cherry pick that to fix build error. Thanks!

commit 3f3a7c8259312084291859d3b623db4317365a07
Author: Christian König <ckoenig.leichtzumer...@gmail.com>
Date:   Fri Nov 24 11:32:59 2017 +0100

staging: vboxvideo: adapt to new TTM interface

Fixes interface changes done in the following commits:
drm/ttm: add operation ctx to ttm_bo_validate v2
drm/ttm: add context to driver move callback as well


-Original Message-
From: kbuild test robot [mailto:fengguang...@intel.com] 
Sent: Friday, February 16, 2018 7:01 AM
To: He, Roger <hongbo...@amd.com>
Cc: kbuild-...@01.org; dri-devel@lists.freedesktop.org; Ma, Le <le...@amd.com>; 
Koenig, Christian <christian.koe...@amd.com>
Subject: [radeon-alex:amd-staging-dkms-4.13 3272/3830] 
drivers/staging//vboxvideo/vbox_ttm.c:190:9: error: incompatible type for 
argument 2 of 'ttm_bo_move_memcpy'

tree:   git://people.freedesktop.org/~agd5f/linux.git amd-staging-dkms-4.13
head:   7bde112fab15c0a28c1d056959167cd4393bf538
commit: d08b4d092e33c348cb01367e02e5dd2dd8104a46 [3272/3830] drm/ttm: use an 
ttm operation ctx for ttm_bo_move_xxx
config: i386-randconfig-a0-201806 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
git checkout d08b4d092e33c348cb01367e02e5dd2dd8104a46
# save the attached .config to linux build tree
make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   drivers/staging//vboxvideo/vbox_ttm.c: In function 'vbox_bo_move':
>> drivers/staging//vboxvideo/vbox_ttm.c:190:9: error: incompatible type for 
>> argument 2 of 'ttm_bo_move_memcpy'
 return ttm_bo_move_memcpy(bo, interruptible, no_wait_gpu, new_mem);
^
   In file included from drivers/staging//vboxvideo/vbox_drv.h:44:0,
from drivers/staging//vboxvideo/vbox_ttm.c:30:
   include/drm/ttm/ttm_bo_driver.h:1022:5: note: expected 'struct 
ttm_operation_ctx *' but argument is of type 'bool'
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
^
   drivers/staging//vboxvideo/vbox_ttm.c:190:9: error: incompatible type for 
argument 3 of 'ttm_bo_move_memcpy'
 return ttm_bo_move_memcpy(bo, interruptible, no_wait_gpu, new_mem);
^
   In file included from drivers/staging//vboxvideo/vbox_drv.h:44:0,
from drivers/staging//vboxvideo/vbox_ttm.c:30:
   include/drm/ttm/ttm_bo_driver.h:1022:5: note: expected 'struct ttm_mem_reg 
*' but argument is of type 'bool'
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
^
>> drivers/staging//vboxvideo/vbox_ttm.c:190:9: error: too many arguments to 
>> function 'ttm_bo_move_memcpy'
 return ttm_bo_move_memcpy(bo, interruptible, no_wait_gpu, new_mem);
^
   In file included from drivers/staging//vboxvideo/vbox_drv.h:44:0,
from drivers/staging//vboxvideo/vbox_ttm.c:30:
   include/drm/ttm/ttm_bo_driver.h:1022:5: note: declared here
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
^
   drivers/staging//vboxvideo/vbox_ttm.c: At top level:
   drivers/staging//vboxvideo/vbox_ttm.c:240:2: warning: initialization from 
incompatible pointer type
 .move = vbox_bo_move,
 ^
   drivers/staging//vboxvideo/vbox_ttm.c:240:2: warning: (near initialization 
for 'vbox_bo_driver.move')
   drivers/staging//vboxvideo/vbox_ttm.c: In function 'vbox_bo_pin':
   drivers/staging//vboxvideo/vbox_ttm.c:392:8: error: too many arguments to 
function 'ttm_bo_validate'
 ret = ttm_bo_validate(>bo, >placement, false, false);
   ^
   In file included from drivers/staging//vboxvideo/vbox_drv.h:43:0,
from drivers/staging//vboxvideo/vbox_ttm.c:30:
   include/drm/ttm/ttm_bo_api.h:339:5: note: declared here
int ttm_bo_validate(struct ttm_buffer_object *bo,
^
   drivers/staging//vboxvideo/vbox_ttm.c: In function 'vbox_bo_unpin':
   drivers/staging//vboxvideo/vbox_ttm.c:419:8: error: too many arguments to 
function 'ttm_bo_validate'
 ret = ttm_bo_validate(>bo, >placement, false, false);
   ^
   In file included from drivers/staging//vboxvideo/vbox_drv.h:43:0,
from drivers/staging//vboxvideo/vbox_ttm.c:30:
   include/drm/ttm/ttm_bo_api.h:339:5: note: declared here
int ttm_bo_validate(struct ttm_buffer_object *bo,
^
   drivers/staging//vboxvideo/vbox_ttm.c: In function 'vbox_bo_push_sysram':
   drivers/staging//vboxvideo/vbox_ttm.c:451:8: error: too many arguments to 
function 'ttm_bo_validate'
 ret = ttm_bo_validate(>bo, >placement, false, false);
   ^
   In file included from drivers/staging//vboxvideo/vbox_drv.h:43:0,
from drivers/staging//vboxvideo/vbox_ttm.c:30:
   include/drm/ttm/ttm_bo_api.h:339:5: note: declared here
int ttm_bo_validate(struct ttm_buffer_object *bo,
^
   drivers/stagi

RE: [PATCH 2/3] drm/ttm: add input parameter force_alloc for ttm_bo_force_list_clean

[He, Roger]

Ok. please ignore patch3 since I have some minor changes.
Will send out later.

Thanks
Roger(Hongbo.He)
-Original Message-
From: Koenig, Christian 
Sent: Friday, February 09, 2018 5:38 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/ttm: add input parameter force_alloc for 
ttm_bo_force_list_clean

Am 09.02.2018 um 08:30 schrieb Roger He:
> if it is  true, allocate TTM pages regardless of zone global memory 
> account limit. For example suspend, We should avoid TTM memory 
> allocate failure to lead to whole process fail.
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 13 -
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index a907311..685baad 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1342,15 +1342,17 @@ int ttm_bo_create(struct ttm_bo_device *bdev,
>   EXPORT_SYMBOL(ttm_bo_create);
>   
>   static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
> -unsigned mem_type)
> + unsigned mem_type, bool force_alloc)
>   {
> - struct ttm_operation_ctx ctx = { false, false };
> + struct ttm_operation_ctx ttm_opt_ctx = { false, false };
>   struct ttm_mem_type_manager *man = >man[mem_type];
>   struct ttm_bo_global *glob = bdev->glob;
>   struct dma_fence *fence;
>   int ret;
>   unsigned i;
>   
> + if (force_alloc)
> + ttm_opt_ctx.flags = TTM_OPT_FLAG_FORCE_ALLOC;

Just unconditional set that flag here as well. ttm_bo_force_list_clean() is 
only called on two occasions:
1. By ttm_bo_evict_mm() during suspend.
2. By ttm_bo_clean_mm() when the driver unloads.

On both cases we absolutely don't want any memory allocation failure.






Christian.

>   /*
>* Can't use standard list traversal since we're unlocking.
>*/
> @@ -1359,7 +1361,8 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device 
> *bdev,
>   for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   while (!list_empty(>lru[i])) {
>   spin_unlock(>lru_lock);
> - ret = ttm_mem_evict_first(bdev, mem_type, NULL, );
> + ret = ttm_mem_evict_first(bdev, mem_type, NULL,
> +   _opt_ctx);
>   if (ret)
>   return ret;
>   spin_lock(>lru_lock);
> @@ -1403,7 +1406,7 @@ int ttm_bo_clean_mm(struct ttm_bo_device *bdev, 
> unsigned mem_type)
>   
>   ret = 0;
>   if (mem_type > 0) {
> - ret = ttm_bo_force_list_clean(bdev, mem_type);
> + ret = ttm_bo_force_list_clean(bdev, mem_type, true);
>   if (ret) {
>   pr_err("Cleanup eviction failed\n");
>   return ret;
> @@ -1433,7 +1436,7 @@ int ttm_bo_evict_mm(struct ttm_bo_device *bdev, 
> unsigned mem_type)
>   return 0;
>   }
>   
> - return ttm_bo_force_list_clean(bdev, mem_type);
> + return ttm_bo_force_list_clean(bdev, mem_type, true);
>   }
>   EXPORT_SYMBOL(ttm_bo_evict_mm);
>   

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

RE: [PATCH 3/4] drm/ttm: add input parameter force_alloc for ttm_bo_evict_mm

[He, Roger]

I can't think of an use case when we don't want this to succeed.

That is true. seems I can simplify more here.

Thanks
Roger(Hongbo.He)
-Original Message-
From: Koenig, Christian 
Sent: Thursday, February 08, 2018 8:58 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/4] drm/ttm: add input parameter force_alloc for 
ttm_bo_evict_mm

Am 08.02.2018 um 10:06 schrieb Roger He:
> if true, allocate TTM pages regardless of zone global memory account 
> limit. For suspend, We should avoid TTM memory allocate failure then 
> result in suspend failure.

Why the extra parameter for amdgpu_bo_evict_vram ?

I can't think of an use case when we don't want this to succeed.

Christian.

>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |  4 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  4 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  2 +-
>   drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
>   drivers/gpu/drm/qxl/qxl_object.c|  4 ++--
>   drivers/gpu/drm/radeon/radeon_device.c  |  6 +++---
>   drivers/gpu/drm/radeon/radeon_object.c  |  5 +++--
>   drivers/gpu/drm/radeon/radeon_object.h  |  3 ++-
>   drivers/gpu/drm/ttm/ttm_bo.c| 16 ++--
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  6 +++---
>   include/drm/ttm/ttm_bo_api.h|  5 -
>   12 files changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index ee76b46..59ee12c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -763,7 +763,7 @@ static int amdgpu_debugfs_evict_vram(struct seq_file *m, 
> void *data)
>   struct drm_device *dev = node->minor->dev;
>   struct amdgpu_device *adev = dev->dev_private;
>   
> - seq_printf(m, "(%d)\n", amdgpu_bo_evict_vram(adev));
> + seq_printf(m, "(%d)\n", amdgpu_bo_evict_vram(adev, true));
>   return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e3fa3d7..3c5f9ca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2168,7 +2168,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool 
> suspend, bool fbcon)
>   }
>   }
>   /* evict vram memory */
> - amdgpu_bo_evict_vram(adev);
> + amdgpu_bo_evict_vram(adev, true);
>   
>   amdgpu_fence_driver_suspend(adev);
>   
> @@ -2178,7 +2178,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool 
> suspend, bool fbcon)
>* This second call to evict vram is to evict the gart page table
>* using the CPU.
>*/
> - amdgpu_bo_evict_vram(adev);
> + amdgpu_bo_evict_vram(adev, true);
>   
>   pci_save_state(dev->pdev);
>   if (suspend) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 0338ef6..db813f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -803,14 +803,14 @@ int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>   return r;
>   }
>   
> -int amdgpu_bo_evict_vram(struct amdgpu_device *adev)
> +int amdgpu_bo_evict_vram(struct amdgpu_device *adev, bool 
> +force_alloc)
>   {
>   /* late 2.6.33 fix IGP hibernate - we need pm ops to do this correct */
>   if (0 && (adev->flags & AMD_IS_APU)) {
>   /* Useless to evict on IGP chips */
>   return 0;
>   }
> - return ttm_bo_evict_mm(>mman.bdev, TTM_PL_VRAM);
> + return ttm_bo_evict_mm(>mman.bdev, TTM_PL_VRAM, force_alloc);
>   }
>   
>   static const char *amdgpu_vram_names[] = { diff --git 
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index c2b02f5..6724cdc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -227,7 +227,7 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 
> domain,
>u64 min_offset, u64 max_offset,
>u64 *gpu_addr);
>   int amdgpu_bo_unpin(struct amdgpu_bo *bo); -int 
> amdgpu_bo_evict_vram(struct amdgpu_device *adev);
> +int amdgpu_bo_evict_vram(struct amdgpu_device *adev, bool 
> +force_alloc);
>   int amdgpu_bo_init(struct amdgpu_device *adev);
>   vo

RE: [PATCH 0/5] prevent OOM triggered by TTM

[He, Roger]

Sure, will make it turn off as default and make the limit configurable.

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Wednesday, February 07, 2018 4:41 PM
To: He, Roger <hongbo...@amd.com>; Thomas Hellstrom <tho...@shipmail.org>; 
amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH 0/5] prevent OOM triggered by TTM

> AMD prefers return value of no memory rather than OOM for now.
Yeah, but Thomas is right that the default for this feature should be that it 
is turned off.

That's why I said that we should make the limit configurable.

Regards,
Christian.

Am 07.02.2018 um 09:25 schrieb He, Roger:
>   Why should TTM be different in that aspect? It would be good to know 
> your reasoning WRT this?
>
> Now, in TTM struct ttm_bo_device it already has member no_retry to indicate 
> your option.
> If you prefer no OOM triggered by TTM, set it as true. The default is false 
> to keep original behavior.
> AMD prefers return value of no memory rather than OOM for now.
>
>
>
>   One thing I looked at at one point was to have TTM do the swapping 
> itself instead of handing it off to the shmem system. That way we 
> could pre-allocate swap entries for all swappable (BO) memory, making 
> sure that we wouldn't run out of swap space when,
>
> I prefer current mechanism of swap out. At the beginning the swapped pages 
> stay in system memory by shmem until OS move to status with high memory 
> pressure, that has an obvious advantage. For example, if the BO is swapped 
> out into shmem, but not really be flushed into swap disk. When validate it 
> and swap in it at this moment, the overhead is small compared to swap in from 
> disk. In addition, No need swap space reservation for TTM pages when 
> allocation since swap disk is shared not only for TTM exclusive.
> So again we provide a flag no_retry in struct ttm_bo_device to let driver set 
> according to its request.
>
>
> Thanks
> Roger(Hongbo.He)
>
> -Original Message-
> From: Thomas Hellstrom [mailto:tho...@shipmail.org]
> Sent: Wednesday, February 07, 2018 2:43 PM
> To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
> dri-devel@lists.freedesktop.org
> Cc: Koenig, Christian <christian.koe...@amd.com>
> Subject: Re: [PATCH 0/5] prevent OOM triggered by TTM
>
> Hi, Roger,
>
> On 02/06/2018 10:04 AM, Roger He wrote:
>> currently ttm code has no any allocation limit. So it allows pages 
>> allocatation unlimited until OOM. Because if swap space is full of 
>> swapped pages and then system memory will be filled up with ttm pages.
>> and then any memory allocation request will trigger OOM.
>>
> I'm a bit curious, isn't this the way things are supposed to work on a linux 
> system?
> If all memory resources are used up, the OOM killer will kill the most memory 
> hungry (perhaps rogue) process rather than processes being nice and try to 
> find out themselves whether allocations will succeed?
> Why should TTM be different in that aspect? It would be good to know your 
> reasoning WRT this?
>
> Admittedly, graphics process OOM memory accounting doesn't work very well, 
> due to not all BOs not being CPU mapped, but it looks like there is recent 
> work towards fixing this?
>
> One thing I looked at at one point was to have TTM do the swapping itself 
> instead of handing it off to the shmem system. That way we could pre-allocate 
> swap entries for all swappable (BO) memory, making sure that we wouldn't run 
> out of swap space when, for example, hibernating and that would also limit 
> the pinned non-swappable memory (from TTM driver kernel allocations for 
> example) to half the system memory resources.
>
> Thanks,
> Thomas
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

RE: [PATCH 0/5] prevent OOM triggered by TTM

[He, Roger]

Why should TTM be different in that aspect? It would be good to know 
your reasoning WRT this?

Now, in TTM struct ttm_bo_device it already has member no_retry to indicate 
your option.
If you prefer no OOM triggered by TTM, set it as true. The default is false to 
keep original behavior. 
AMD prefers return value of no memory rather than OOM for now.



One thing I looked at at one point was to have TTM do the swapping 
itself instead of handing it off to the shmem system. That way we could 
pre-allocate swap entries for all swappable (BO) memory, making sure that we 
wouldn't run out of swap space when, 

I prefer current mechanism of swap out. At the beginning the swapped pages stay 
in system memory by shmem until OS move to status with high memory pressure, 
that has an obvious advantage. For example, if the BO is swapped out into 
shmem, but not really be flushed into swap disk. When validate it and swap in 
it at this moment, the overhead is small compared to swap in from disk. In 
addition, No need swap space reservation for TTM pages when allocation since 
swap disk is shared not only for TTM exclusive.
So again we provide a flag no_retry in struct ttm_bo_device to let driver set 
according to its request.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Thomas Hellstrom [mailto:tho...@shipmail.org] 
Sent: Wednesday, February 07, 2018 2:43 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH 0/5] prevent OOM triggered by TTM

Hi, Roger,

On 02/06/2018 10:04 AM, Roger He wrote:
> currently ttm code has no any allocation limit. So it allows pages 
> allocatation unlimited until OOM. Because if swap space is full of 
> swapped pages and then system memory will be filled up with ttm pages. 
> and then any memory allocation request will trigger OOM.
>

I'm a bit curious, isn't this the way things are supposed to work on a linux 
system?
If all memory resources are used up, the OOM killer will kill the most memory 
hungry (perhaps rogue) process rather than processes being nice and try to find 
out themselves whether allocations will succeed?
Why should TTM be different in that aspect? It would be good to know your 
reasoning WRT this?

Admittedly, graphics process OOM memory accounting doesn't work very well, due 
to not all BOs not being CPU mapped, but it looks like there is recent work 
towards fixing this?

One thing I looked at at one point was to have TTM do the swapping itself 
instead of handing it off to the shmem system. That way we could pre-allocate 
swap entries for all swappable (BO) memory, making sure that we wouldn't run 
out of swap space when, for example, hibernating and that would also limit the 
pinned non-swappable memory (from TTM driver kernel allocations for example) to 
half the system memory resources.

Thanks,
Thomas

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

RE: [PATCH 0/5] prevent OOM triggered by TTM

[He, Roger]

Move the new call out of ttm_mem_global_reserve() and into 
ttm_page_alloc.c or ttm_page_alloc_dma.c (but keep it in ttm_memory.c).  
ttm_mem_global_reserve() is called for each page allocated and 
si_mem_available() is a bit to heavy for that.

Good idea! Agree with you completely, because initially I also concern that but 
no better way at that time.
Going to improve the patches. Thanks!

-Original Message-
From: Koenig, Christian 
Sent: Tuesday, February 06, 2018 5:52 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Cc: tho...@shipmail.org
Subject: Re: [PATCH 0/5] prevent OOM triggered by TTM

Nice work, but a few comments.

First of all you need to reorder the patches. Adding the exceptions to the 
restrictions should come first, then the restriction itself. 
Otherwise we might break a setup in between the patches and that is bad for 
bisecting.

Then make all values configurable, e.g. take a closer look at ttm_memory.c. 
Just add attributes directly under the memory_accounting directory (see 
ttm_mem_global_init).

Additional to that you can't put device specific information (the no_retry 
flag) into ttm_mem_global, that is driver unspecific and won't work like this.

Move the new call out of ttm_mem_global_reserve() and into ttm_page_alloc.c or 
ttm_page_alloc_dma.c (but keep it in ttm_memory.c). 
ttm_mem_global_reserve() is called for each page allocated and
si_mem_available() is a bit to heavy for that.

Maybe name TTM_OPT_FLAG_ALLOW_ALLOC_ANYWAY something like _FORCE_ALLOCATION or 
_ALLOW_OOM.

And please also try if a criteria like (si_mem_available() +
get_nr_swap_pages()) < limit works as well. This way we would have only a 
single new limit.

Regards,
Christian.

Am 06.02.2018 um 10:04 schrieb Roger He:
> currently ttm code has no any allocation limit. So it allows pages 
> allocatation unlimited until OOM. Because if swap space is full of 
> swapped pages and then system memory will be filled up with ttm pages. 
> and then any memory allocation request will trigger OOM.
>
>
> the following patches is for prevent OOM triggered by TTM.
> the basic idea is when allocating TTM pages, check the free swap space 
> firt. if it is less than the fixe limit, reject the allocation request.
> but there are two exceptions which should allow it regardless of zone 
> memory account limit.
> a. page fault
> for ttm_mem_global_reserve if serving for page fault routine,
> because page fault routing already grabbed system memory so the
> allowance of this exception is harmless. Otherwise, it will trigger
>  OOM killer.
> b. suspend
> anyway, we should allow suspend success always.
>
>
> at last, if bdev.no_retry is false (by defaut), keep the original 
> behavior no any change.
>
> Roger He (5):
>drm/ttm: check if the free swap space is under limit 256MB
>drm/ttm: keep original behavior except with flag no_retry
>drm/ttm: use bit flag to replace allow_reserved_eviction in
>  ttm_operation_ctx
>drm/ttm: add bit flag TTM_OPT_FLAG_ALLOW_ALLOC_ANYWAY
>drm/ttm: add input parameter allow_allo_anyway for ttm_bo_evict_mm
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  4 +--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |  4 +--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 10 +++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  8 +++--
>   drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
>   drivers/gpu/drm/qxl/qxl_object.c|  4 +--
>   drivers/gpu/drm/radeon/radeon_device.c  |  6 ++--
>   drivers/gpu/drm/radeon/radeon_object.c  |  6 ++--
>   drivers/gpu/drm/radeon/radeon_object.h  |  3 +-
>   drivers/gpu/drm/ttm/ttm_bo.c| 19 +++
>   drivers/gpu/drm/ttm/ttm_bo_vm.c |  6 ++--
>   drivers/gpu/drm/ttm/ttm_memory.c| 51 
> ++---
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c|  1 -
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  6 ++--
>   include/drm/ttm/ttm_bo_api.h| 14 ++--
>   include/drm/ttm/ttm_memory.h|  6 
>   18 files changed, 111 insertions(+), 43 deletions(-)
>

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

RE: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[He, Roger]

Patch1 & 2 & 4,   Reviewed-by: Roger He 
Patch 5:  Acked-by: Roger He 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Saturday, February 03, 2018 3:10 AM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

This allows access to pages allocated through the driver with optional IOMMU 
mapping.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 -
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 648c449aaa79..795ceaeb82d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1929,38 +1929,51 @@ static const struct file_operations amdgpu_ttm_gtt_fops 
= {
 
 #endif
 
-static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
-  size_t size, loff_t *pos)
+static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
+size_t size, loff_t *pos)
 {
struct amdgpu_device *adev = file_inode(f)->i_private;
-   int r;
-   uint64_t phys;
struct iommu_domain *dom;
+   ssize_t result = 0;
+   int r;
 
-   // always return 8 bytes
-   if (size != 8)
-   return -EINVAL;
+   dom = iommu_get_domain_for_dev(adev->dev);
 
-   // only accept page addresses
-   if (*pos & 0xFFF)
-   return -EINVAL;
+   while (size) {
+   phys_addr_t addr = *pos & PAGE_MASK;
+   loff_t off = *pos & ~PAGE_MASK;
+   size_t bytes = PAGE_SIZE - off;
+   unsigned long pfn;
+   struct page *p;
+   void *ptr;
 
-   dom = iommu_get_domain_for_dev(adev->dev);
-   if (dom)
-   phys = iommu_iova_to_phys(dom, *pos);
-   else
-   phys = *pos;
+   addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
 
-   r = copy_to_user(buf, , 8);
-   if (r)
-   return -EFAULT;
+   pfn = addr >> PAGE_SHIFT;
+   if (!pfn_valid(pfn))
+   return -EPERM;
+
+   p = pfn_to_page(pfn);
+   if (p->mapping != adev->mman.bdev.dev_mapping)
+   return -EPERM;
+
+   ptr = kmap(p);
+   r = copy_to_user(buf, ptr, bytes);
+   kunmap(p);
+   if (r)
+   return -EFAULT;
 
-   return 8;
+   size -= bytes;
+   *pos += bytes;
+   result += bytes;
+   }
+
+   return result;
 }
 
-static const struct file_operations amdgpu_ttm_iova_fops = {
+static const struct file_operations amdgpu_ttm_iomem_fops = {
.owner = THIS_MODULE,
-   .read = amdgpu_iova_to_phys_read,
+   .read = amdgpu_iomem_read,
.llseek = default_llseek
 };
 
@@ -1973,7 +1986,7 @@ static const struct {  #ifdef 
CONFIG_DRM_AMDGPU_GART_DEBUGFS
{ "amdgpu_gtt", _ttm_gtt_fops, TTM_PL_TT },  #endif
-   { "amdgpu_iova", _ttm_iova_fops, TTM_PL_SYSTEM },
+   { "amdgpu_iomem", _ttm_iomem_fops, TTM_PL_SYSTEM },
 };
 
 #endif
--
2.14.1

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

RE: [PATCH 3/5] drm/radeon: remove extra TT unpopulated check

[He, Roger]

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Saturday, February 03, 2018 3:10 AM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 3/5] drm/radeon: remove extra TT unpopulated check

The subsystem chould check that, not the driver.

Commit log typo, should be "should" rather than " chould".
With that fix,  this patch is Reviewed-by: Roger He 

Thanks
Roger(Hongbo.He)

Signed-off-by: Christian König 
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
b/drivers/gpu/drm/radeon/radeon_ttm.c
index a0a839bc39bf..42e3ee81a96e 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -728,9 +728,6 @@ static int radeon_ttm_tt_populate(struct ttm_tt *ttm,
struct radeon_device *rdev;
bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
 
-   if (ttm->state != tt_unpopulated)
-   return 0;
-
if (gtt && gtt->userptr) {
ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
if (!ttm->sg)
-- 
2.14.1

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

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

Probably it is necessary to summarize, and I have done below experiments:

A: use a fixed limit to stopping swapping out as below:
 int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx 
*ctx)   {
if get_nr_swap_pages() < 256MB  return no memory;

 }
 Set the value as 256MB not work on my platform which has 8GB system memory 
& 8GB swap disk.
 Set it as 4GB can pass, but 4GB not work on test machine with 16GB system 
memory & 8GB swap disk.
 So set the limit as fixed value doesn't work always.

B. use the fixed limit as scaling with system memory, something as below:
 int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx 
*ctx)   {
if get_nr_swap_pages() < 1/2 * system memory size;

 }
 So far, it can work on all test machine.
 But it has an obvious defect.
 For example,  if the platform has 32G system memory & 8G swap disk.
 1/2 * ram = 16G which is bigger than swap disk, so TTM swap for TTM is 
disallowed even when swap disk is empty at the start.
 So seems this approach is not reasonable. 

C. Then we work out new patches as in mailist today.


Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
He, Roger
Sent: Friday, February 02, 2018 3:54 PM
To: Koenig, Christian <christian.koe...@amd.com>; Zhou, David(ChunMing) 
<david1.z...@amd.com>; dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Can you try to use a fixed limit like I suggested once more?
E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Maybe you missed previous mail. I explain again here.
Set the value as 256MB not work on my platform.  My machine has 8GB system 
memory, and 8GB swap disk.
On my machine, set it as 4G can work.
But 4G also not work on test machine with 16GB system memory & 8GB swap disk.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Koenig, Christian
Sent: Friday, February 02, 2018 3:46 PM
To: He, Roger <hongbo...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>; 
dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Can you try to use a fixed limit like I suggested once more?

E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Regards,
Christian.

Am 02.02.2018 um 07:57 schrieb He, Roger:
>   Use the limit as total ram*1/2 seems work very well.
>   No OOM although swap disk reaches full at peak for piglit test.
>
> But for this approach, David noticed that has an obvious defect.
> For example,  if the platform has 32G system memory, 8G swap disk.
> 1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed 
> at all.
> For now we work out an improved version based on get_nr_swap_pages().
> Going to send out later.
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: He, Roger
> Sent: Thursday, February 01, 2018 4:03 PM
> To: Koenig, Christian <christian.koe...@amd.com>; Zhou,
> David(ChunMing) <david1.z...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 'He, Roger' 
> <hongbo...@amd.com>
> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
> Just now, I tried with fixed limit.  But not work always.
> For example: set the limit as 4GB on my platform with 8GB system memory, it 
> can pass.
> But when run with platform with 16GB system memory, it failed since OOM.
>
> And I guess it also depends on app's behavior.
> I mean some apps  make OS to use more swap space as well.
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of He, Roger
> Sent: Thursday, February 01, 2018 1:48 PM
> To: Koenig, Christian <christian.koe...@amd.com>; Zhou,
> David(ChunMing) <david1.z...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
>   But what we could do is to rely on a fixed limit like the Intel driver 
> does and I suggested before.
>   E.g. don't copy anything into a shmemfile when there is only x MB of 
> swap space left.
>
> Here I think we can do it further, let the limit value scaling with total 
> system memory.
> For example: total system memory * 1/2.
> If that it will match the platform configuration better.
>
>   Roger c

RE: [PATCH 1/5] drm/ttm: add max_swap_mem in ttm_mem_global

[He, Roger]

Need call si_swapinfo to fill those valules .
void si_swapinfo(struct sysinfo *val)

But that function is not exported as well.

Thanks
Roger(Hongbo.He)
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Chunming Zhou
Sent: Friday, February 02, 2018 3:38 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH 1/5] drm/ttm: add max_swap_mem in ttm_mem_global




On 2018年02月02日 15:34, Chunming Zhou wrote:


On 2018年02月02日 15:22, Roger He wrote:

set its initial value as 1/2 * free swap cache size when module initial.
and adjust this value when allocate TTM memory

Signed-off-by: Roger He <hongbo...@amd.com><mailto:hongbo...@amd.com>
---
  drivers/gpu/drm/ttm/ttm_memory.c | 10 --
  include/drm/ttm/ttm_memory.h |  2 ++
  2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
index aa0c381..b48931d 100644
--- a/drivers/gpu/drm/ttm/ttm_memory.c
+++ b/drivers/gpu/drm/ttm/ttm_memory.c
@@ -36,6 +36,7 @@
  #include 
  #include 
  #include 
+#include 
#define TTM_MEMORY_ALLOC_RETRIES 4
  @@ -372,9 +373,9 @@ int ttm_mem_global_init(struct ttm_mem_global *glob)
  kobject_put(>kobj);
  return ret;
  }
-
+/* set it as 1/2 * swap free space we can get at that time */
+glob->max_swap_mem = get_nr_swap_pages() << (PAGE_SHIFT - 1);
  si_meminfo();
Hi Roger,

I just find si_meminfo can get total swap size, see struct sysinfo definition:
 struct sysinfo {
__kernel_long_t uptime; /* Seconds since boot */
__kernel_ulong_t loads[3];  /* 1, 5, and 15 minute load averages */
__kernel_ulong_t totalram;  /* Total usable main memory size */
__kernel_ulong_t freeram;   /* Available memory size */
__kernel_ulong_t sharedram; /* Amount of shared memory */
__kernel_ulong_t bufferram; /* Memory used by buffers */
__kernel_ulong_t totalswap; /* Total swap space size */
__kernel_ulong_t freeswap;  /* swap space still available */
__u16 procs;/* Number of current processes */
...

can sysinfo.totalswap be used for your change?

Regards,
David Zhou

-
  ret = ttm_mem_init_kernel_zone(glob, );
  if (unlikely(ret != 0))
  goto out_no_zone;
@@ -473,12 +474,17 @@ static int ttm_mem_global_reserve(struct ttm_mem_global 
*glob,
struct ttm_mem_zone *single_zone,
uint64_t amount, bool reserve)
  {
+uint64_t free_swap_mem = get_nr_swap_pages() << (PAGE_SHIFT - 1);
  uint64_t limit;
  int ret = -ENOMEM;
  unsigned int i;
  struct ttm_mem_zone *zone;
spin_lock(>lock);
+/* adjust the max_swap_mem to cover the new inserted swap space */
+if (glob->max_swap_mem < free_swap_mem)
+glob->max_swap_mem = free_swap_mem;
Seems using max() for exchange is more obvious, otherwise looks ok to me.

Regards,
David Zhou

+
  for (i = 0; i < glob->num_zones; ++i) {
  zone = glob->zones[i];
  if (single_zone && zone != single_zone)
diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
index 8936285..ad5a557 100644
--- a/include/drm/ttm/ttm_memory.h
+++ b/include/drm/ttm/ttm_memory.h
@@ -49,6 +49,7 @@
   * @work: The workqueue callback for the shrink queue.
   * @lock: Lock to protect the @shrink - and the memory accounting members,
   * that is, essentially the whole structure with some exceptions.
+ * @max_swap_mem: upper limit of swap space TTM can use
   * @zones: Array of pointers to accounting zones.
   * @num_zones: Number of populated entries in the @zones array.
   * @zone_kernel: Pointer to the kernel zone.
@@ -67,6 +68,7 @@ struct ttm_mem_global {
  struct workqueue_struct *swap_queue;
  struct work_struct work;
  spinlock_t lock;
+uint64_t max_swap_mem;
  struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
  unsigned int num_zones;
  struct ttm_mem_zone *zone_kernel;


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

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

Can you try to use a fixed limit like I suggested once more?
E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Maybe you missed previous mail. I explain again here.
Set the value as 256MB not work on my platform.  My machine has 8GB system 
memory, and 8GB swap disk.
On my machine, set it as 4G can work.
But 4G also not work on test machine with 16GB system memory & 8GB swap disk.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Koenig, Christian 
Sent: Friday, February 02, 2018 3:46 PM
To: He, Roger <hongbo...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>; 
dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Can you try to use a fixed limit like I suggested once more?

E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Regards,
Christian.

Am 02.02.2018 um 07:57 schrieb He, Roger:
>   Use the limit as total ram*1/2 seems work very well.
>   No OOM although swap disk reaches full at peak for piglit test.
>
> But for this approach, David noticed that has an obvious defect.
> For example,  if the platform has 32G system memory, 8G swap disk.
> 1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed 
> at all.
> For now we work out an improved version based on get_nr_swap_pages().
> Going to send out later.
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: He, Roger
> Sent: Thursday, February 01, 2018 4:03 PM
> To: Koenig, Christian <christian.koe...@amd.com>; Zhou, 
> David(ChunMing) <david1.z...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 'He, Roger' 
> <hongbo...@amd.com>
> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
> Just now, I tried with fixed limit.  But not work always.
> For example: set the limit as 4GB on my platform with 8GB system memory, it 
> can pass.
> But when run with platform with 16GB system memory, it failed since OOM.
>
> And I guess it also depends on app's behavior.
> I mean some apps  make OS to use more swap space as well.
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of He, Roger
> Sent: Thursday, February 01, 2018 1:48 PM
> To: Koenig, Christian <christian.koe...@amd.com>; Zhou, 
> David(ChunMing) <david1.z...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
>   But what we could do is to rely on a fixed limit like the Intel driver 
> does and I suggested before.
>   E.g. don't copy anything into a shmemfile when there is only x MB of 
> swap space left.
>
> Here I think we can do it further, let the limit value scaling with total 
> system memory.
> For example: total system memory * 1/2.
> If that it will match the platform configuration better.
>
>   Roger can you test that approach once more with your fix for the OOM 
> issues in the page fault handler?
>
> Sure. Use the limit as total ram*1/2 seems work very well.
> No OOM although swap disk reaches full at peak for piglit test.
> I speculate this case happens but no OOM because:
>
> a. run a while, swap disk be used close to 1/2* total size and but not over 
> 1/2 * total.
> b. all subsequent swapped pages stay in system memory until no space there.
>   Then the swapped pages in shmem be flushed into swap disk. And probably 
> OS also need some swap space.
>   For this case, it is easy to get full for swap disk.
> c. but because now free swap size < 1/2 * total, so no swap out happen  after 
> that.
>  And at least 1/4* system memory will left because below check in 
> ttm_mem_global_reserve will ensure that.
>   if (zone->used_mem > limit)
>   goto out_unlock;
>  
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: Koenig, Christian
> Sent: Wednesday, January 31, 2018 4:13 PM
> To: He, Roger <hongbo...@amd.com>; Zhou, David(ChunMing) 
> <david1.z...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
> Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel 
> driver does and I suggested before.
>
> E.g. don't copy anything into a shmemfile when there is only x MB of swap 
> space left.
>
> Roger can you 

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.

But for this approach, David noticed that has an obvious defect. 
For example,  if the platform has 32G system memory, 8G swap disk.
1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed 
at all.
For now we work out an improved version based on get_nr_swap_pages().
Going to send out later.

Thanks
Roger(Hongbo.He)
-Original Message-
From: He, Roger 
Sent: Thursday, February 01, 2018 4:03 PM
To: Koenig, Christian <christian.koe...@amd.com>; Zhou, David(ChunMing) 
<david1.z...@amd.com>; dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 'He, Roger' 
<hongbo...@amd.com>
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Just now, I tried with fixed limit.  But not work always.
For example: set the limit as 4GB on my platform with 8GB system memory, it can 
pass.
But when run with platform with 16GB system memory, it failed since OOM.

And I guess it also depends on app's behavior.
I mean some apps  make OS to use more swap space as well.

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
He, Roger
Sent: Thursday, February 01, 2018 1:48 PM
To: Koenig, Christian <christian.koe...@amd.com>; Zhou, David(ChunMing) 
<david1.z...@amd.com>; dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

But what we could do is to rely on a fixed limit like the Intel driver 
does and I suggested before.
E.g. don't copy anything into a shmemfile when there is only x MB of 
swap space left.

Here I think we can do it further, let the limit value scaling with total 
system memory.
For example: total system memory * 1/2. 
If that it will match the platform configuration better. 

Roger can you test that approach once more with your fix for the OOM 
issues in the page fault handler?

Sure. Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.
I speculate this case happens but no OOM because:

a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 
* total.
b. all subsequent swapped pages stay in system memory until no space there.
 Then the swapped pages in shmem be flushed into swap disk. And probably OS 
also need some swap space.
 For this case, it is easy to get full for swap disk.
c. but because now free swap size < 1/2 * total, so no swap out happen  after 
that. 
And at least 1/4* system memory will left because below check in 
ttm_mem_global_reserve will ensure that.
if (zone->used_mem > limit)
goto out_unlock;

Thanks
Roger(Hongbo.He)
-Original Message-
From: Koenig, Christian
Sent: Wednesday, January 31, 2018 4:13 PM
To: He, Roger <hongbo...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>; 
dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel 
driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of swap space 
left.

Roger can you test that approach once more with your fix for the OOM issues in 
the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
>   I think this patch isn't need at all. You can directly read 
> total_swap_pages variable in TTM.
>
> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using 
> will result in:
> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Chunming Zhou
> Sent: Wednesday, January 31, 2018 3:15 PM
> To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; Koenig, 
> Christian <christian.koe...@amd.com>
> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
> Hi Roger,
>
> I think this patch isn't need at all. You can directly read total_swap_pages 
> variable in TTM. See the comment:
>
> /* protected with swap_lock. reading in vm_swap_full() doesn't need 
> lock */ long total_swap_pages;
>
> there are many places using it directly, you just couldn't change its value. 
> Reading it doesn't need lock.
>
>
> Regar

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

Just now, I tried with fixed limit.  But not work always.
For example: set the limit as 4GB on my platform with 8GB system memory, it can 
pass.
But when run with platform with 16GB system memory, it failed since OOM.

And I guess it also depends on app's behavior.
I mean some apps  make OS to use more swap space as well.

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
He, Roger
Sent: Thursday, February 01, 2018 1:48 PM
To: Koenig, Christian <christian.koe...@amd.com>; Zhou, David(ChunMing) 
<david1.z...@amd.com>; dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

But what we could do is to rely on a fixed limit like the Intel driver 
does and I suggested before.
E.g. don't copy anything into a shmemfile when there is only x MB of 
swap space left.

Here I think we can do it further, let the limit value scaling with total 
system memory.
For example: total system memory * 1/2. 
If that it will match the platform configuration better. 

Roger can you test that approach once more with your fix for the OOM 
issues in the page fault handler?

Sure. Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.
I speculate this case happens but no OOM because:

a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 
* total.
b. all subsequent swapped pages stay in system memory until no space there.
 Then the swapped pages in shmem be flushed into swap disk. And probably OS 
also need some swap space.
 For this case, it is easy to get full for swap disk.
c. but because now free swap size < 1/2 * total, so no swap out happen  after 
that. 
And at least 1/4* system memory will left because below check in 
ttm_mem_global_reserve will ensure that.
if (zone->used_mem > limit)
goto out_unlock;

Thanks
Roger(Hongbo.He)
-Original Message-
From: Koenig, Christian
Sent: Wednesday, January 31, 2018 4:13 PM
To: He, Roger <hongbo...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>; 
dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel 
driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of swap space 
left.

Roger can you test that approach once more with your fix for the OOM issues in 
the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
>   I think this patch isn't need at all. You can directly read 
> total_swap_pages variable in TTM.
>
> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using 
> will result in:
> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Chunming Zhou
> Sent: Wednesday, January 31, 2018 3:15 PM
> To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; Koenig, 
> Christian <christian.koe...@amd.com>
> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
> Hi Roger,
>
> I think this patch isn't need at all. You can directly read total_swap_pages 
> variable in TTM. See the comment:
>
> /* protected with swap_lock. reading in vm_swap_full() doesn't need 
> lock */ long total_swap_pages;
>
> there are many places using it directly, you just couldn't change its value. 
> Reading it doesn't need lock.
>
>
> Regards,
>
> David Zhou
>
>
> On 2018年01月29日 16:29, Roger He wrote:
>> ttm module needs it to determine its internal parameter setting.
>>
>> Signed-off-by: Roger He <hongbo...@amd.com>
>> ---
>>include/linux/swap.h |  6 ++
>>mm/swapfile.c| 15 +++
>>2 files changed, 21 insertions(+)
>>
>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 
>> c2b8128..708d66f 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>>struct backing_dev_info;
>>extern int init_swap_address_space(unsigned int type, unsigned long 
>> nr_pages);
>>extern void exit_swap_address_space(unsigned int type);
>> +extern long get_total_swap_pages(void);
>>

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

Hi Michal:

How about only  
EXPORT_SYMBOL_GPL(total_swap_pages) ?

Thanks
Roger(Hongbo.He)

-Original Message-
From: He, Roger 
Sent: Wednesday, January 31, 2018 1:52 PM
To: 'Michal Hocko' <mho...@kernel.org>; Koenig, Christian 
<christian.koe...@amd.com>
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 
dri-devel@lists.freedesktop.org
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

I do think you should completely ignore the size of the swap space. 
IMHO you should forbid further allocations when your currentbuffer 
storage cannot be reclaimed. So you need some form of feedback mechanism that 
would tell you: "Your buffers have grown too much". If you cannot do 
that then simply assume that you cannot swap at all rather than rely on having 
some portion   of it for yourself. 

If we assume the swap cache size is zero always, that is overkill for GTT size 
actually user can get. And not make sense as well I think.

There are many other users of memory outside of your subsystem. Any 
scaling based on the 50% of resource belonging to me is simply broken.

And that is only a threshold to avoid  overuse  rather than really reserved to 
TTM at the start. In addition, for most cases TTM only uses a little or not use 
swap disk at all. Only special test case use more or probably that is 
intentional.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Michal Hocko [mailto:mho...@kernel.org]
Sent: Tuesday, January 30, 2018 8:29 PM
To: Koenig, Christian <christian.koe...@amd.com>
Cc: He, Roger <hongbo...@amd.com>; linux...@kvack.org; 
linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

On Tue 30-01-18 11:32:49, Christian König wrote:
> Am 30.01.2018 um 11:18 schrieb Michal Hocko:
> > On Tue 30-01-18 10:00:07, Christian König wrote:
> > > Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> > > > On Tue 30-01-18 02:56:51, He, Roger wrote:
> > > > > Hi Michal:
> > > > > 
> > > > > We need a API to tell TTM module the system totally has how 
> > > > > many swap cache.  Then TTM module can use it to restrict how 
> > > > > many the swap cache it can use to prevent triggering OOM.  For 
> > > > > Now we set the threshold of swap size TTM used as 1/2 * total 
> > > > > size and leave the rest for others use.
> > > > Why do you so much memory? Are you going to use TB of memory on 
> > > > large systems? What about memory hotplug when the memory is 
> > > > added/released?
> > > For graphics and compute applications on GPUs it isn't unusual to 
> > > use large amounts of system memory.
> > > 
> > > Our standard policy in TTM is to allow 50% of system memory to be 
> > > pinned for use with GPUs (the hardware can't do page faults).
> > > 
> > > When that limit is exceeded (or the shrinker callbacks tell us to 
> > > make room) we wait for any GPU work to finish and copy buffer 
> > > content into a shmem file.
> > > 
> > > This copy into a shmem file can easily trigger the OOM killer if 
> > > there isn't any swap space left and that is something we want to avoid.
> > > 
> > > So what we want to do is to apply this 50% rule to swap space as 
> > > well and deny allocation of buffer objects when it is exceeded.
> > How does that help when the rest of the system might eat swap?
> 
> Well it doesn't, but that is not the problem here.
> 
> When an application keeps calling malloc() it sooner or later is 
> confronted with an OOM killer.
> 
> But when it keeps for example allocating OpenGL textures the 
> expectation is that this sooner or later starts to fail because we run 
> out of memory and not trigger the OOM killer.

There is nothing like running out of memory and not triggering the OOM killer. 
You can make a _particular_ allocation to bail out without the oom killer. Just 
use __GFP_NORETRY. But that doesn't make much difference when you have already 
depleted your memory and live with the bare remainings. Any desperate soul 
trying to get its memory will simply trigger the OOM.

> So what we do is to allow the application to use all of video memory + 
> a certain amount of system memory + swap space as last resort fallback (e.g.
> when you Alt+Tab from your full screen game back to your browser).
> 
> The problem we try to solve is that we haven't limited the use of swap 
> space somehow.

I do think you should completely ignore the size of the swap space. IMHO you 
should forbid further allocations when your current buffer storage ca

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

But what we could do is to rely on a fixed limit like the Intel driver 
does and I suggested before.
E.g. don't copy anything into a shmemfile when there is only x MB of 
swap space left.

Here I think we can do it further, let the limit value scaling with total 
system memory.
For example: total system memory * 1/2. 
If that it will match the platform configuration better. 

Roger can you test that approach once more with your fix for the OOM 
issues in the page fault handler?

Sure. Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.
I speculate this case happens but no OOM because:

a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 
* total.
b. all subsequent swapped pages stay in system memory until no space there.
 Then the swapped pages in shmem be flushed into swap disk. And probably OS 
also need some swap space.
 For this case, it is easy to get full for swap disk.
c. but because now free swap size < 1/2 * total, so no swap out happen  after 
that. 
And at least 1/4* system memory will left because below check in 
ttm_mem_global_reserve will ensure that.
if (zone->used_mem > limit)
goto out_unlock;

Thanks
Roger(Hongbo.He)
-Original Message-
From: Koenig, Christian 
Sent: Wednesday, January 31, 2018 4:13 PM
To: He, Roger <hongbo...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>; 
dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel 
driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of swap space 
left.

Roger can you test that approach once more with your fix for the OOM issues in 
the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
>   I think this patch isn't need at all. You can directly read 
> total_swap_pages variable in TTM.
>
> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using 
> will result in:
> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".
>
> Thanks
> Roger(Hongbo.He)
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Chunming Zhou
> Sent: Wednesday, January 31, 2018 3:15 PM
> To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
> Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; Koenig, 
> Christian <christian.koe...@amd.com>
> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 
> expose total_swap_pages
>
> Hi Roger,
>
> I think this patch isn't need at all. You can directly read total_swap_pages 
> variable in TTM. See the comment:
>
> /* protected with swap_lock. reading in vm_swap_full() doesn't need 
> lock */ long total_swap_pages;
>
> there are many places using it directly, you just couldn't change its value. 
> Reading it doesn't need lock.
>
>
> Regards,
>
> David Zhou
>
>
> On 2018年01月29日 16:29, Roger He wrote:
>> ttm module needs it to determine its internal parameter setting.
>>
>> Signed-off-by: Roger He <hongbo...@amd.com>
>> ---
>>include/linux/swap.h |  6 ++
>>mm/swapfile.c| 15 +++
>>2 files changed, 21 insertions(+)
>>
>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 
>> c2b8128..708d66f 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>>struct backing_dev_info;
>>extern int init_swap_address_space(unsigned int type, unsigned long 
>> nr_pages);
>>extern void exit_swap_address_space(unsigned int type);
>> +extern long get_total_swap_pages(void);
>>
>>#else /* CONFIG_SWAP */
>>
>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>>{
>>}
>>
>> +long get_total_swap_pages(void)
>> +{
>> +return 0;
>> +}
>> +
>>#define free_swap_and_cache(e) ({(is_migration_entry(e) || 
>> is_device_private_entry(e));})
>>#define swapcache_prepare(e) ({(is_migration_entry(e) || 
>> is_device_private_entry(e));})
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb 
>> 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>>
>>atomic_t nr_rotate_swap = ATOMIC

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

I think this patch isn't need at all. You can directly read 
total_swap_pages variable in TTM.

Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will 
result in:
"WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Chunming Zhou
Sent: Wednesday, January 31, 2018 3:15 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; Koenig, Christian 
<christian.koe...@amd.com>
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Hi Roger,

I think this patch isn't need at all. You can directly read total_swap_pages 
variable in TTM. See the comment:

/* protected with swap_lock. reading in vm_swap_full() doesn't need lock */ 
long total_swap_pages;

there are many places using it directly, you just couldn't change its value. 
Reading it doesn't need lock.


Regards,

David Zhou


On 2018年01月29日 16:29, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   include/linux/swap.h |  6 ++
>   mm/swapfile.c| 15 +++
>   2 files changed, 21 insertions(+)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index c2b8128..708d66f 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>   struct backing_dev_info;
>   extern int init_swap_address_space(unsigned int type, unsigned long 
> nr_pages);
>   extern void exit_swap_address_space(unsigned int type);
> +extern long get_total_swap_pages(void);
>   
>   #else /* CONFIG_SWAP */
>   
> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>   {
>   }
>   
> +long get_total_swap_pages(void)
> +{
> + return 0;
> +}
> +
>   #define free_swap_and_cache(e) ({(is_migration_entry(e) || 
> is_device_private_entry(e));})
>   #define swapcache_prepare(e) ({(is_migration_entry(e) || 
> is_device_private_entry(e));})
>   
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02..a0062eb 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>   
>   atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>   
> +/*
> + * expose this value for others use
> + */
> +long get_total_swap_pages(void)
> +{
> + long ret;
> +
> + spin_lock(_lock);
> + ret = total_swap_pages;
> + spin_unlock(_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
> +
>   static inline unsigned char swap_count(unsigned char ent)
>   {
>   return ent & ~SWAP_HAS_CACHE;   /* may include SWAP_HAS_CONT flag */

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

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

I do think you should completely ignore the size of the swap space. 
IMHO you should forbid further allocations when your currentbuffer 
storage cannot be reclaimed. So you need some form of feedback mechanism that 
would tell you: "Your buffers have grown too much". If you cannot do 
that then simply assume that you cannot swap at all rather than rely on having 
some portion   of it for yourself. 

If we assume the swap cache size is zero always, that is overkill for GTT size 
actually user can get. And not make sense as well I think.

There are many other users of memory outside of your subsystem. Any 
scaling based on the 50% of resource belonging to me is simply broken.

And that is only a threshold to avoid  overuse  rather than really reserved to 
TTM at the start. In addition, for most cases TTM only uses a little or not use 
swap disk at all. Only special test case use more or probably that is 
intentional.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Michal Hocko [mailto:mho...@kernel.org] 
Sent: Tuesday, January 30, 2018 8:29 PM
To: Koenig, Christian <christian.koe...@amd.com>
Cc: He, Roger <hongbo...@amd.com>; linux...@kvack.org; 
linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

On Tue 30-01-18 11:32:49, Christian König wrote:
> Am 30.01.2018 um 11:18 schrieb Michal Hocko:
> > On Tue 30-01-18 10:00:07, Christian König wrote:
> > > Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> > > > On Tue 30-01-18 02:56:51, He, Roger wrote:
> > > > > Hi Michal:
> > > > > 
> > > > > We need a API to tell TTM module the system totally has how 
> > > > > many swap cache.  Then TTM module can use it to restrict how 
> > > > > many the swap cache it can use to prevent triggering OOM.  For 
> > > > > Now we set the threshold of swap size TTM used as 1/2 * total 
> > > > > size and leave the rest for others use.
> > > > Why do you so much memory? Are you going to use TB of memory on 
> > > > large systems? What about memory hotplug when the memory is 
> > > > added/released?
> > > For graphics and compute applications on GPUs it isn't unusual to 
> > > use large amounts of system memory.
> > > 
> > > Our standard policy in TTM is to allow 50% of system memory to be 
> > > pinned for use with GPUs (the hardware can't do page faults).
> > > 
> > > When that limit is exceeded (or the shrinker callbacks tell us to 
> > > make room) we wait for any GPU work to finish and copy buffer 
> > > content into a shmem file.
> > > 
> > > This copy into a shmem file can easily trigger the OOM killer if 
> > > there isn't any swap space left and that is something we want to avoid.
> > > 
> > > So what we want to do is to apply this 50% rule to swap space as 
> > > well and deny allocation of buffer objects when it is exceeded.
> > How does that help when the rest of the system might eat swap?
> 
> Well it doesn't, but that is not the problem here.
> 
> When an application keeps calling malloc() it sooner or later is 
> confronted with an OOM killer.
> 
> But when it keeps for example allocating OpenGL textures the 
> expectation is that this sooner or later starts to fail because we run 
> out of memory and not trigger the OOM killer.

There is nothing like running out of memory and not triggering the OOM killer. 
You can make a _particular_ allocation to bail out without the oom killer. Just 
use __GFP_NORETRY. But that doesn't make much difference when you have already 
depleted your memory and live with the bare remainings. Any desperate soul 
trying to get its memory will simply trigger the OOM.

> So what we do is to allow the application to use all of video memory + 
> a certain amount of system memory + swap space as last resort fallback (e.g.
> when you Alt+Tab from your full screen game back to your browser).
> 
> The problem we try to solve is that we haven't limited the use of swap 
> space somehow.

I do think you should completely ignore the size of the swap space. IMHO you 
should forbid further allocations when your current buffer storage cannot be 
reclaimed. So you need some form of feedback mechanism that would tell you: 
"Your buffers have grown too much". If you cannot do that then simply assume 
that you cannot swap at all rather than rely on having some portion of it for 
yourself. There are many other users of memory outside of your subsystem. Any 
scaling based on the 50% of resource belonging to me is simply broken.
--
Michal Hocko
SUSE Labs
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

get_nr_swap_pages is the only API we can accessed from other module now.
It can't cover the case of the dynamic swap size increment.
I mean: user can use "swapon" to enable new swap file or swap disk 
dynamically or "swapoff" to disable swap space.

Above is why we always to get swap cache size rather than getting it once at 
module initialization time.
That is internal in TTM. Please ignore that.

And why TTM needs get_total_swap_pages instead of using get_nr_swap_pages 
directly. That because
even though the TTM buffer has been swapped out, at the start they also stay in 
system memory by shmem. Later at some point when
Under high memory pressure, Those buffers all are flushed into swap disk and 
used more swap disk size or even use up all swap size. That is not what we want 
and still has random OOM. So we need a API to get total swap size and control 
the swap size used by TTM very accurately.

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
He, Roger
Sent: Tuesday, January 30, 2018 10:57 AM
To: Michal Hocko <mho...@kernel.org>
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 
dri-devel@lists.freedesktop.org; Koenig, Christian <christian.koe...@amd.com>
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

Hi Michal:

We need a API to tell TTM module the system totally has how many swap cache.
Then TTM module can use it to restrict how many the swap cache it can use to 
prevent triggering OOM.
For Now we set the threshold of swap size TTM used as 1/2 * total size and 
leave the rest for others use.


get_nr_swap_pages is the only API we can accessed from other module now.
It can't cover the case of the dynamic swap size increment.
I mean: user can use "swapon" to enable new swap file or swap disk 
dynamically or "swapoff" to disable swap space.

Thanks
Roger(Hongbo.He)

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Michal Hocko
Sent: Tuesday, January 30, 2018 12:31 AM
To: He, Roger <hongbo...@amd.com>
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 
dri-devel@lists.freedesktop.org; Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

On Mon 29-01-18 16:29:42, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.

Could you be more specific why?

> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>  include/linux/swap.h |  6 ++
>  mm/swapfile.c| 15 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h index 
> c2b8128..708d66f 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *); struct 
> backing_dev_info;  extern int init_swap_address_space(unsigned int 
> type, unsigned long nr_pages);  extern void 
> exit_swap_address_space(unsigned int type);
> +extern long get_total_swap_pages(void);
>  
>  #else /* CONFIG_SWAP */
>  
> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)  { 
> }
>  
> +long get_total_swap_pages(void)
> +{
> + return 0;
> +}
> +
>  #define free_swap_and_cache(e) ({(is_migration_entry(e) ||
> is_device_private_entry(e));})  #define swapcache_prepare(e)
> ({(is_migration_entry(e) || is_device_private_entry(e));})
>  
> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb
> 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>  
>  atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>  
> +/*
> + * expose this value for others use
> + */
> +long get_total_swap_pages(void)
> +{
> + long ret;
> +
> + spin_lock(_lock);
> + ret = total_swap_pages;
> + spin_unlock(_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
> +
>  static inline unsigned char swap_count(unsigned char ent)  {
>   return ent & ~SWAP_HAS_CACHE;   /* may include SWAP_HAS_CONT flag */
> --
> 2.7.4

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

RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

[He, Roger]

Hi Michal:

We need a API to tell TTM module the system totally has how many swap cache.
Then TTM module can use it to restrict how many the swap cache it can use to 
prevent triggering OOM.
For Now we set the threshold of swap size TTM used as 1/2 * total size and 
leave the rest for others use.

But get_nr_swap_pages is the only API we can accessed from other module now.
It can't cover the case of the dynamic swap size increment.
I mean: user can use "swapon" to enable new swap file or swap disk dynamically 
or "swapoff" to disable swap space.

Thanks
Roger(Hongbo.He)

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Michal Hocko
Sent: Tuesday, January 30, 2018 12:31 AM
To: He, Roger <hongbo...@amd.com>
Cc: linux...@kvack.org; linux-ker...@vger.kernel.org; 
dri-devel@lists.freedesktop.org; Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose 
total_swap_pages

On Mon 29-01-18 16:29:42, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.

Could you be more specific why?

> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>  include/linux/swap.h |  6 ++
>  mm/swapfile.c| 15 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h index 
> c2b8128..708d66f 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);  
> struct backing_dev_info;  extern int init_swap_address_space(unsigned 
> int type, unsigned long nr_pages);  extern void 
> exit_swap_address_space(unsigned int type);
> +extern long get_total_swap_pages(void);
>  
>  #else /* CONFIG_SWAP */
>  
> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)  {  
> }
>  
> +long get_total_swap_pages(void)
> +{
> + return 0;
> +}
> +
>  #define free_swap_and_cache(e) ({(is_migration_entry(e) || 
> is_device_private_entry(e));})  #define swapcache_prepare(e) 
> ({(is_migration_entry(e) || is_device_private_entry(e));})
>  
> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb 
> 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>  
>  atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>  
> +/*
> + * expose this value for others use
> + */
> +long get_total_swap_pages(void)
> +{
> + long ret;
> +
> + spin_lock(_lock);
> + ret = total_swap_pages;
> + spin_unlock(_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
> +
>  static inline unsigned char swap_count(unsigned char ent)  {
>   return ent & ~SWAP_HAS_CACHE;   /* may include SWAP_HAS_CONT flag */
> --
> 2.7.4

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

RE: [RFC] Per file OOM badness

[He, Roger]

Basically the idea is right to me.

1. But we need smaller granularity to control the contribution to OOM badness.
 Because when the TTM buffer resides in VRAM rather than evict to system 
memory, we should not take this account into badness.
 But I think it is not easy to implement.

2. If the TTM buffer(GTT here) is mapped to user for CPU access, not quite sure 
the buffer size is already taken into account for kernel.
 If yes, at last the size will be counted again by your patches.

So, I am thinking if we can counted the TTM buffer size into: 
struct mm_rss_stat {
atomic_long_t count[NR_MM_COUNTERS];
};
Which is done by kernel based on CPU VM (page table).

Something like that:
When GTT allocate suceess:
add_mm_counter(vma->vm_mm, MM_ANONPAGES, buffer_size);

When GTT swapped out:
dec_mm_counter from MM_ANONPAGES frist, then 
add_mm_counter(vma->vm_mm, MM_SWAPENTS, buffer_size);  // or MM_SHMEMPAGES or 
add new item.

Update the corresponding item in mm_rss_stat always.
If that, we can control the status update accurately. 
What do you think about that?
And is there any side-effect for this approach?


Thanks
Roger(Hongbo.He)

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Andrey Grodzovsky
Sent: Friday, January 19, 2018 12:48 AM
To: linux-ker...@vger.kernel.org; linux...@kvack.org; 
dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Cc: Koenig, Christian 
Subject: [RFC] Per file OOM badness

Hi, this series is a revised version of an RFC sent by Christian König a few 
years ago. The original RFC can be found at 
https://lists.freedesktop.org/archives/dri-devel/2015-September/089778.html

This is the same idea and I've just adressed his concern from the original RFC 
and switched to a callback into file_ops instead of a new member in struct file.

Thanks,
Andrey

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

RE: [RFC] Per file OOM badness

[He, Roger]

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of 
Michal Hocko
Sent: Friday, January 19, 2018 1:14 AM
To: Grodzovsky, Andrey 
Cc: linux...@kvack.org; amd-...@lists.freedesktop.org; 
linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Koenig, 
Christian 
Subject: Re: [RFC] Per file OOM badness

On Thu 18-01-18 18:00:06, Michal Hocko wrote:
> On Thu 18-01-18 11:47:48, Andrey Grodzovsky wrote:
> > Hi, this series is a revised version of an RFC sent by Christian 
> > König a few years ago. The original RFC can be found at 
> > https://lists.freedesktop.org/archives/dri-devel/2015-September/0897
> > 78.html
> > 
> > This is the same idea and I've just adressed his concern from the 
> > original RFC and switched to a callback into file_ops instead of a new 
> > member in struct file.
> 
> Please add the full description to the cover letter and do not make 
> people hunt links.
> 
> Here is the origin cover letter text
> : I'm currently working on the issue that when device drivers allocate 
> memory on
> : behalf of an application the OOM killer usually doesn't knew about 
> that unless
> : the application also get this memory mapped into their address space.
> : 
> : This is especially annoying for graphics drivers where a lot of the 
> VRAM
> : usually isn't CPU accessible and so doesn't make sense to map into 
> the
> : address space of the process using it.
> : 
> : The problem now is that when an application starts to use a lot of 
> VRAM those
> : buffers objects sooner or later get swapped out to system memory, 
> but when we
> : now run into an out of memory situation the OOM killer obviously 
> doesn't knew
> : anything about that memory and so usually kills the wrong process.

OK, but how do you attribute that memory to a particular OOM killable 
entity? And how do you actually enforce that those resources get freed on the 
oom killer action?

Here I think we need more fine granularity for distinguishing the buffer is 
taking VRAM or system memory.

> : The following set of patches tries to address this problem by 
> introducing a per
> : file OOM badness score, which device drivers can use to give the OOM 
> killer a
> : hint how many resources are bound to a file descriptor so that it 
> can make
> : better decisions which process to kill.

But files are not killable, they can be shared... In other words this doesn't 
help the oom killer to make an educated guess at all.

> : 
> : So question at every one: What do you think about this approach?

I thing is just just wrong semantically. Non-reclaimable memory is a 
pain, especially when there is way too much of it. If you can free that memory 
somehow then you can hook into slab shrinker API and react on the memory 
pressure. If you can account such amemory to a particular process and 
make sure that the consumption is bound by the process life time then we can 
think of an accounting that oom_badness can consider when selecting a victim.

I think you are misunderstanding here.
Actually for now, the memory in TTM Pools already has mm_shrink which is 
implemented in ttm_pool_mm_shrink_init.
And here the memory we want to make it contribute to OOM badness is not in TTM 
Pools.
Because when TTM buffer allocation success, the memory already is removed from 
TTM Pools.  

Thanks
Roger(Hongbo.He)

--
Michal Hocko
SUSE Labs
___
amd-gfx mailing list
amd-...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 2/2] drm/ttm: Don't unreserve swapped BOs that were previously reserved

[He, Roger]

good catch! 

Thanks
Roger(Hongbo.He)
-Original Message-
From: Kuehling, Felix 
Sent: Friday, January 19, 2018 12:56 AM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Koenig, 
Christian <christian.koe...@amd.com>; He, Roger <hongbo...@amd.com>
Cc: Kuehling, Felix <felix.kuehl...@amd.com>
Subject: [PATCH 2/2] drm/ttm: Don't unreserve swapped BOs that were previously 
reserved

If ttm_bo_swapout doesn't own the lock, don't release it. Someone else probably 
depends on it still being locked.

Signed-off-by: Felix Kuehling <felix.kuehl...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
62518b6..893003f 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1780,8 +1780,8 @@ int ttm_bo_swapout(struct ttm_bo_global *glob, struct 
ttm_operation_ctx *ctx)
 * Unreserve without putting on LRU to avoid swapping out an
 * already swapped buffer.
 */
-
-   reservation_object_unlock(bo->resv);
+   if (locked)
+   reservation_object_unlock(bo->resv);
kref_put(>list_kref, ttm_bo_release_list);
return ret;
 }
--
2.7.4

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

RE: [PATCH] drm/ttm: add updated_glob_count in dma_page

[He, Roger]

-Original Message-
From: Koenig, Christian 
Sent: Wednesday, January 17, 2018 3:37 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Cc: Grodzovsky, Andrey <andrey.grodzov...@amd.com>
Subject: Re: [PATCH] drm/ttm: add updated_glob_count in dma_page

Am 17.01.2018 um 06:54 schrieb Roger He:
> add this for correctly updating global mem count in ttm_mem_zone.
> before that when ttm_mem_global_alloc_page fails, we would update all 
> dma_page's global mem count in ttm_dma->pages_list. but actually here 
> we should not update for the last dma_page.
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 57 
> +---
>   1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index c7f01a4..3e78ea4 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -121,6 +121,8 @@ struct dma_pool {
>* @vaddr: The virtual address of the page and a flag if the page belongs 
> to a
>* huge pool
>* @dma: The bus address of the page. If the page is not allocated
> + * @updated_glob_count: to indicate whether we already updated global
> + * memory count in ttm_mem_zone
>*   via the DMA API, it will be -1.
>*/
>   struct dma_page {
> @@ -128,6 +130,7 @@ struct dma_page {
>   unsigned long vaddr;
>   struct page *p;
>   dma_addr_t dma;
> + bool updated_glob_count;

That is not a good idea at all because it massively increases how much 
memory we will use.

Please put that in the vaddr instead. The lower bits should always be 
zero, so we already put the VADDR_FLAG_HUGE_POOL flag in there.

Good idea, I will try.

Thanks
Roger(Hongbo.He)
>   };
>   
>   /*
> @@ -874,18 +877,18 @@ static int ttm_dma_page_pool_fill_locked(struct 
> dma_pool *pool,
>   }
>   
>   /*
> - * @return count of pages still required to fulfill the request.
>* The populate list is actually a stack (not that is matters as TTM
>* allocates one page at a time.
> + * return dma_page pointer if success, otherwise NULL.
>*/
> -static int ttm_dma_pool_get_pages(struct dma_pool *pool,
> +static struct dma_page *ttm_dma_pool_get_pages(struct dma_pool *pool,
> struct ttm_dma_tt *ttm_dma,
> unsigned index)
>   {
> - struct dma_page *d_page;
> + struct dma_page *d_page = NULL;
>   struct ttm_tt *ttm = _dma->ttm;
>   unsigned long irq_flags;
> - int count, r = -ENOMEM;
> + int count;
>   
>   spin_lock_irqsave(>lock, irq_flags);
>   count = ttm_dma_page_pool_fill_locked(pool, _flags); @@ -894,12 
> +897,11 @@ static int ttm_dma_pool_get_pages(struct dma_pool *pool,
>   ttm->pages[index] = d_page->p;
>   ttm_dma->dma_address[index] = d_page->dma;
>   list_move_tail(_page->page_list, _dma->pages_list);
> - r = 0;
>   pool->npages_in_use += 1;
>   pool->npages_free -= 1;
>   }
>   spin_unlock_irqrestore(>lock, irq_flags);
> - return r;
> + return d_page;
>   }
>   
>   static gfp_t ttm_dma_pool_gfp_flags(struct ttm_dma_tt *ttm_dma, bool 
> huge) @@ -934,6 +936,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, 
> struct device *dev,
>   struct ttm_mem_global *mem_glob = ttm->glob->mem_glob;
>   unsigned long num_pages = ttm->num_pages;
>   struct dma_pool *pool;
> + struct dma_page *d_page;
>   enum pool_type type;
>   unsigned i;
>   int ret;
> @@ -962,8 +965,8 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct 
> device *dev,
>   while (num_pages >= HPAGE_PMD_NR) {
>   unsigned j;
>   
> - ret = ttm_dma_pool_get_pages(pool, ttm_dma, i);
> - if (ret != 0)
> + d_page = ttm_dma_pool_get_pages(pool, ttm_dma, i);
> + if (!d_page)
>   break;
>   
>   ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], @@ 
> -973,6 
> +976,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
>   return -ENOMEM;
>   }
>   
> + d_page->updated_glob_count = true;
>   for (j = i + 1; j < (i + HPAGE_PMD_NR); ++j) {
>   ttm->pages[j] = ttm->pages[j - 1] + 1;
>   ttm_dma->dma_address[j] = ttm_dma->dma_address[j - 1] + 
> @@ -996,8 
> +1000,8 @@ int tt

RE: [PATCH v2 1/2] drm/ttm: Allow page allocations w/o triggering OOM..

[He, Roger]

Reviewed-by: Roger He <hongbo...@amd.com>

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Andrey Grodzovsky
Sent: Tuesday, January 16, 2018 11:18 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Koenig, 
Christian <christian.koe...@amd.com>
Cc: He, Roger <hongbo...@amd.com>
Subject: [PATCH v2 1/2] drm/ttm: Allow page allocations w/o triggering OOM..

This to allow drivers to choose to avoid OOM invocation and handle page 
allocation failures instead.

v2:
Remove extra new lines.

Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c |  3 +++
 drivers/gpu/drm/ttm/ttm_page_alloc.c |  6 ++
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  3 +++
 drivers/gpu/drm/ttm/ttm_tt.c | 13 +++--
 include/drm/ttm/ttm_bo_driver.h  |  4 
 5 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
2eb71ff..f32aab1 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -234,6 +234,9 @@ static int ttm_bo_add_ttm(struct ttm_buffer_object *bo, 
bool zero_alloc)
if (bdev->need_dma32)
page_flags |= TTM_PAGE_FLAG_DMA32;
 
+   if (bdev->no_retry)
+   page_flags |= TTM_PAGE_FLAG_NO_RETRY;
+
switch (bo->type) {
case ttm_bo_type_device:
if (zero_alloc)
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 0eab24e..f34c843 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -741,6 +741,9 @@ static int ttm_page_pool_get_pages(struct ttm_page_pool 
*pool,
if (ttm_flags & TTM_PAGE_FLAG_ZERO_ALLOC)
gfp_flags |= __GFP_ZERO;
 
+   if (ttm_flags & TTM_PAGE_FLAG_NO_RETRY)
+   gfp_flags |= __GFP_RETRY_MAYFAIL;
+
/* ttm_alloc_new_pages doesn't reference pool so we can run
 * multiple requests in parallel.
 **/
@@ -893,6 +896,9 @@ static int ttm_get_pages(struct page **pages, unsigned 
npages, int flags,
if (flags & TTM_PAGE_FLAG_ZERO_ALLOC)
gfp_flags |= __GFP_ZERO;
 
+   if (flags & TTM_PAGE_FLAG_NO_RETRY)
+   gfp_flags |= __GFP_RETRY_MAYFAIL;
+
if (flags & TTM_PAGE_FLAG_DMA32)
gfp_flags |= GFP_DMA32;
else
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index c7f01a4..6949ef7 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -920,6 +920,9 @@ static gfp_t ttm_dma_pool_gfp_flags(struct ttm_dma_tt 
*ttm_dma, bool huge)
gfp_flags &= ~__GFP_COMP;
}
 
+   if (ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY)
+   gfp_flags |= __GFP_RETRY_MAYFAIL;
+
return gfp_flags;
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 
5a046a3..9e4d43d 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -301,7 +301,11 @@ int ttm_tt_swapin(struct ttm_tt *ttm)
swap_space = swap_storage->f_mapping;
 
for (i = 0; i < ttm->num_pages; ++i) {
-   from_page = shmem_read_mapping_page(swap_space, i);
+   gfp_t gfp_mask = mapping_gfp_mask(swap_space);
+
+   gfp_mask |= (ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY ? 
__GFP_RETRY_MAYFAIL : 0);
+   from_page = shmem_read_mapping_page_gfp(swap_space, i, 
gfp_mask);
+
if (IS_ERR(from_page)) {
ret = PTR_ERR(from_page);
goto out_err;
@@ -350,10 +354,15 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file 
*persistent_swap_storage)
swap_space = swap_storage->f_mapping;
 
for (i = 0; i < ttm->num_pages; ++i) {
+   gfp_t gfp_mask = mapping_gfp_mask(swap_space);
+
+   gfp_mask |= (ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY ? 
+__GFP_RETRY_MAYFAIL : 0);
+
from_page = ttm->pages[i];
if (unlikely(from_page == NULL))
continue;
-   to_page = shmem_read_mapping_page(swap_space, i);
+
+   to_page = shmem_read_mapping_page_gfp(swap_space, i, gfp_mask);
if (IS_ERR(to_page)) {
ret = PTR_ERR(to_page);
goto out_err;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h 
index 94064b1..9b417eb 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -86,6 +86,7 @@ struct ttm_backend_func {
 #define TTM_PAGE_FLAG

RE: [PATCH 2/2] drm/amdgpu: Use new TTM flag to avoid OOM triggering.

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Tuesday, January 16, 2018 8:46 PM
To: Grodzovsky, Andrey <andrey.grodzov...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; He, Roger <hongbo...@amd.com>; 
dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/amdgpu: Use new TTM flag to avoid OOM triggering.

Am 16.01.2018 um 13:43 schrieb Andrey Grodzovsky:
>
>
> On 01/16/2018 03:54 AM, Christian König wrote:
>> Am 16.01.2018 um 07:18 schrieb He, Roger:
>>> -Original Message-
>>> From: Andrey Grodzovsky [mailto:andrey.grodzov...@amd.com]
>>> Sent: Saturday, January 13, 2018 6:29 AM
>>> To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
>>> Cc: Koenig, Christian <christian.koe...@amd.com>; He, Roger 
>>> <hongbo...@amd.com>; Grodzovsky, Andrey <andrey.grodzov...@amd.com>
>>> Subject: [PATCH 2/2] drm/amdgpu: Use new TTM flag to avoid OOM 
>>> triggering.
>>>
>>> This to have a load time option to avoid OOM on RAM allocations.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h    | 1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 4 
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 
>>>   3 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index b7c181e..1387239 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -127,6 +127,7 @@ extern int amdgpu_job_hang_limit;  extern int 
>>> amdgpu_lbpw;  extern int amdgpu_compute_multipipe;  extern int 
>>> amdgpu_gpu_recovery;
>>> +extern int amdgpu_alloc_no_oom;
>>>     #ifdef CONFIG_DRM_AMDGPU_SI
>>>   extern int amdgpu_si_support;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index d96f9ac..6e98189 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -130,6 +130,7 @@ int amdgpu_job_hang_limit = 0;  int amdgpu_lbpw 
>>> = -1;  int amdgpu_compute_multipipe = -1;  int amdgpu_gpu_recovery = 
>>> -1; /* auto */
>>> +int amdgpu_alloc_no_oom = -1; /* auto */
>>>
>>> How about turn it on as default?
>>
>> I think we can even go a step further, drop the module parameter and 
>> just turn it always on for amdgpu.
>>
>> Christian.
>
> Will fix, just a reminder that Roger's patches - [PATCH 1/2] drm/ttm: 
> don't update global memory count for some special cases [PATCH 2/2] 
> drm/ttm: only free pages rather than update global memory count 
> together
>
> Needs to be merged before my patches since the fix a TTM bug on 
> allocation failure.

The second is merged, but I had some comments on the first and Roger 
hasn't replied yet.

Roger what's the status on that one?

Already fixed locally, but not tested yet.  Try to send out today.

Thanks
Roger(Hongbo.He)

>
> Thanks,
> Andrey
>
>>
>>>
>>> Thanks
>>> Roger(Hongbo.He)
>>>
>>> MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in 
>>> megabytes");  module_param_named(vramlimit, amdgpu_vram_limit, int, 
>>> 0600); @@ -285,6 +286,9 @@ module_param_named(compute_multipipe,
>>> amdgpu_compute_multipipe, int, 0444); MODULE_PARM_DESC(gpu_recovery, 
>>> "Enable GPU recovery mechanism, (1 = enable, 0 = disable, -1 = 
>>> auto"); module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 
>>> 0444);
>>>   +MODULE_PARM_DESC(alloc_no_oom, "Allocate RAM without triggering 
>>> OOM
>>> +killer, (1 = enable, 0 = disable, -1 = auto"); 
>>> +module_param_named(alloc_no_oom, amdgpu_alloc_no_oom, int, 0444);
>>> +
>>>   #ifdef CONFIG_DRM_AMDGPU_SI
>>>     #if defined(CONFIG_DRM_RADEON) ||
>>> defined(CONFIG_DRM_RADEON_MODULE) diff --git 
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 5c4c3e0..fc27164 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -420,6 +420,10 @@ static int amdgpu_bo_do_create(struct 
>>> amdgpu_device *adev,  #endif
>>>     bo->tbo.bdev = >mman.bdev;
>>> +
>>> +    if (amdgpu_alloc_no_oom == 1)
>>> +    bo->tbo.bdev->no_retry = true;
>>> +
>>>   amdgpu_ttm_placement_from_domain(bo, domain);
>>>     r = ttm_bo_init_reserved(>mman.bdev, >tbo, size, 
>>> type,
>>> --
>>> 2.7.4
>>>
>>> ___
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

RE: [PATCH 2/2] drm/amdgpu: Use new TTM flag to avoid OOM triggering.

[He, Roger]

-Original Message-
From: Andrey Grodzovsky [mailto:andrey.grodzov...@amd.com] 
Sent: Saturday, January 13, 2018 6:29 AM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>; He, Roger 
<hongbo...@amd.com>; Grodzovsky, Andrey <andrey.grodzov...@amd.com>
Subject: [PATCH 2/2] drm/amdgpu: Use new TTM flag to avoid OOM triggering.

This to have a load time option to avoid OOM on RAM allocations.

Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index b7c181e..1387239 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -127,6 +127,7 @@ extern int amdgpu_job_hang_limit;  extern int amdgpu_lbpw;  
extern int amdgpu_compute_multipipe;  extern int amdgpu_gpu_recovery;
+extern int amdgpu_alloc_no_oom;
 
 #ifdef CONFIG_DRM_AMDGPU_SI
 extern int amdgpu_si_support;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index d96f9ac..6e98189 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -130,6 +130,7 @@ int amdgpu_job_hang_limit = 0;  int amdgpu_lbpw = -1;  int 
amdgpu_compute_multipipe = -1;  int amdgpu_gpu_recovery = -1; /* auto */
+int amdgpu_alloc_no_oom = -1; /* auto */

How about turn it on as default?

Thanks
Roger(Hongbo.He)

MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");  
module_param_named(vramlimit, amdgpu_vram_limit, int, 0600); @@ -285,6 +286,9 
@@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);  
MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 = enable, 0 = 
disable, -1 = auto");  module_param_named(gpu_recovery, amdgpu_gpu_recovery, 
int, 0444);
 
+MODULE_PARM_DESC(alloc_no_oom, "Allocate RAM without triggering OOM 
+killer, (1 = enable, 0 = disable, -1 = auto"); 
+module_param_named(alloc_no_oom, amdgpu_alloc_no_oom, int, 0444);
+
 #ifdef CONFIG_DRM_AMDGPU_SI
 
 #if defined(CONFIG_DRM_RADEON) || defined(CONFIG_DRM_RADEON_MODULE) diff --git 
a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 5c4c3e0..fc27164 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -420,6 +420,10 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, 
 #endif
 
bo->tbo.bdev = >mman.bdev;
+
+   if (amdgpu_alloc_no_oom == 1)
+   bo->tbo.bdev->no_retry = true;
+
amdgpu_ttm_placement_from_domain(bo, domain);
 
r = ttm_bo_init_reserved(>mman.bdev, >tbo, size, type,
--
2.7.4

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

RE: [PATCH 1/2] drm/ttm: Allow page allocations w/o triggering OOM..

[He, Roger]

-Original Message-
From: Andrey Grodzovsky [mailto:andrey.grodzov...@amd.com] 
Sent: Saturday, January 13, 2018 6:29 AM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>; He, Roger 
<hongbo...@amd.com>; Grodzovsky, Andrey <andrey.grodzov...@amd.com>
Subject: [PATCH 1/2] drm/ttm: Allow page allocations w/o triggering OOM..

This to allow drivers to choose to avoid OOM invocation and handle page 
allocation failures instead.

Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c |  3 +++
 drivers/gpu/drm/ttm/ttm_page_alloc.c |  6 ++
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  3 +++
 drivers/gpu/drm/ttm/ttm_tt.c | 13 +++--
 include/drm/ttm/ttm_bo_api.h |  1 +
 include/drm/ttm/ttm_bo_driver.h  |  4 
 6 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
2eb71ff..f32aab1 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -234,6 +234,9 @@ static int ttm_bo_add_ttm(struct ttm_buffer_object *bo, 
bool zero_alloc)
if (bdev->need_dma32)
page_flags |= TTM_PAGE_FLAG_DMA32;
 
+   if (bdev->no_retry)
+   page_flags |= TTM_PAGE_FLAG_NO_RETRY;
+
switch (bo->type) {
case ttm_bo_type_device:
if (zero_alloc)
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 0eab24e..f34c843 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -741,6 +741,9 @@ static int ttm_page_pool_get_pages(struct ttm_page_pool 
*pool,
if (ttm_flags & TTM_PAGE_FLAG_ZERO_ALLOC)
gfp_flags |= __GFP_ZERO;
 
+   if (ttm_flags & TTM_PAGE_FLAG_NO_RETRY)
+   gfp_flags |= __GFP_RETRY_MAYFAIL;
+
/* ttm_alloc_new_pages doesn't reference pool so we can run
 * multiple requests in parallel.
 **/
@@ -893,6 +896,9 @@ static int ttm_get_pages(struct page **pages, unsigned 
npages, int flags,
if (flags & TTM_PAGE_FLAG_ZERO_ALLOC)
gfp_flags |= __GFP_ZERO;
 
+   if (flags & TTM_PAGE_FLAG_NO_RETRY)
+   gfp_flags |= __GFP_RETRY_MAYFAIL;
+
if (flags & TTM_PAGE_FLAG_DMA32)
gfp_flags |= GFP_DMA32;
else
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index c7f01a4..6949ef7 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -920,6 +920,9 @@ static gfp_t ttm_dma_pool_gfp_flags(struct ttm_dma_tt 
*ttm_dma, bool huge)
gfp_flags &= ~__GFP_COMP;
}
 
+   if (ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY)
+   gfp_flags |= __GFP_RETRY_MAYFAIL;
+
return gfp_flags;
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 
5a046a3..9e4d43d 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -301,7 +301,11 @@ int ttm_tt_swapin(struct ttm_tt *ttm)
swap_space = swap_storage->f_mapping;
 
for (i = 0; i < ttm->num_pages; ++i) {
-   from_page = shmem_read_mapping_page(swap_space, i);
+   gfp_t gfp_mask = mapping_gfp_mask(swap_space);
+
+   gfp_mask |= (ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY ? 
__GFP_RETRY_MAYFAIL : 0);
+   from_page = shmem_read_mapping_page_gfp(swap_space, i, 
gfp_mask);
+
if (IS_ERR(from_page)) {
ret = PTR_ERR(from_page);
goto out_err;
@@ -350,10 +354,15 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file 
*persistent_swap_storage)
swap_space = swap_storage->f_mapping;
 
for (i = 0; i < ttm->num_pages; ++i) {
+   gfp_t gfp_mask = mapping_gfp_mask(swap_space);
+
+   gfp_mask |= (ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY ? 
+__GFP_RETRY_MAYFAIL : 0);
+
from_page = ttm->pages[i];
if (unlikely(from_page == NULL))
continue;
-   to_page = shmem_read_mapping_page(swap_space, i);
+
+   to_page = shmem_read_mapping_page_gfp(swap_space, i, gfp_mask);
if (IS_ERR(to_page)) {
ret = PTR_ERR(to_page);
goto out_err;
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 
2cd025c..099f24b 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -176,6 +176,7 @@ struct ttm_buffer_object {
unsigned long num_pages;
size_t acc_size;
 
+
Please 

RE: [PATCH 1/2] drm/ttm: don't update global memory count for some special cases

[He, Roger]

It is the first step for the GTT size issue.
Need other patches I am working on it.

Thanks
Roger(Hongbo.He)
-Original Message-
From: Zhou, David(ChunMing) 
Sent: Friday, January 12, 2018 2:46 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH 1/2] drm/ttm: don't update global memory count for some 
special cases

Can this fix GTT size issue or not?


Regards,
David Zhou
On 2018年01月12日 14:14, Roger He wrote:
> add input parameter for ttm_dma_unpopulate.
> when ttm_dma_pool_get_pages or ttm_mem_global_alloc_page fail, don't 
> call ttm_mem_global_free_page to update global memory count.
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c  |  2 +-
>   drivers/gpu/drm/nouveau/nouveau_bo.c |  2 +-
>   drivers/gpu/drm/radeon/radeon_ttm.c  |  2 +-
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 19 +++
>   drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c   |  2 +-
>   include/drm/ttm/ttm_page_alloc.h |  5 +++--
>   6 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index e4bb435..723ccf1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1046,7 +1046,7 @@ static void amdgpu_ttm_tt_unpopulate(struct 
> ttm_tt *ttm)
>   
>   #ifdef CONFIG_SWIOTLB
>   if (swiotlb_nr_tbl()) {
> - ttm_dma_unpopulate(>ttm, adev->dev);
> + ttm_dma_unpopulate(>ttm, adev->dev, true);
>   return;
>   }
>   #endif
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c 
> b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index ce328ed..3f7c30f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1632,7 +1632,7 @@ nouveau_ttm_tt_unpopulate(struct ttm_tt *ttm)
>   
>   #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)
>   if (swiotlb_nr_tbl()) {
> - ttm_dma_unpopulate((void *)ttm, dev);
> + ttm_dma_unpopulate((void *)ttm, dev, true);
>   return;
>   }
>   #endif
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index a0a839b..449cc65 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -789,7 +789,7 @@ static void radeon_ttm_tt_unpopulate(struct ttm_tt 
> *ttm)
>   
>   #ifdef CONFIG_SWIOTLB
>   if (swiotlb_nr_tbl()) {
> - ttm_dma_unpopulate(>ttm, rdev->dev);
> + ttm_dma_unpopulate(>ttm, rdev->dev, true);
>   return;
>   }
>   #endif
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index c7f01a4..4cda764 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -969,7 +969,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct 
> device *dev,
>   ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i],
>   pool->size, ctx);
>   if (unlikely(ret != 0)) {
> - ttm_dma_unpopulate(ttm_dma, dev);
> + ttm_dma_unpopulate(ttm_dma, dev, false);
>   return -ENOMEM;
>   }
>   
> @@ -998,14 +998,14 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct 
> device *dev,
>   while (num_pages) {
>   ret = ttm_dma_pool_get_pages(pool, ttm_dma, i);
>   if (ret != 0) {
> - ttm_dma_unpopulate(ttm_dma, dev);
> + ttm_dma_unpopulate(ttm_dma, dev, false);
>   return -ENOMEM;
>   }
>   
>   ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i],
>   pool->size, ctx);
>   if (unlikely(ret != 0)) {
> - ttm_dma_unpopulate(ttm_dma, dev);
> + ttm_dma_unpopulate(ttm_dma, dev, false);
>   return -ENOMEM;
>   }
>   
> @@ -1016,7 +1016,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct 
> device *dev,
>   if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
>   ret = ttm_tt_swapin(ttm);
>   if (unlikely(ret != 0)) {
> - ttm_dma_unpopulate(ttm_dma, dev);
> + ttm_dma_unpopulate(ttm_dma, dev, true);
>   return ret;
>   }
>   

RE: next/master build: 198 builds: 1 failed, 197 passed, 1 error, 148 warnings (next-20180110)

[He, Roger]

Calling kobject_put() after a failed kobject_init_and_add() seemed 
wrong, and it also appears to be missing a kfree(), so I didn't want 
to mess it up any further.

ttm_pool_kobj_release will do that, so no need kfree() here.
I will fix this warning today. Thanks.


Thanks
Roger(Hongbo.He)

-Original Message-
From: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] On Behalf Of Arnd 
Bergmann
Sent: Thursday, January 11, 2018 12:18 AM
To: Koenig, Christian <christian.koe...@amd.com>
Cc: kernelci.org bot <b...@kernelci.org>; Kernel Build Reports Mailman List 
<kernel-build-repo...@lists.linaro.org>; Linux Kernel Mailing List 
<linux-ker...@vger.kernel.org>; Thomas Hellstrom <thellst...@vmware.com>; Dave 
Airlie <airl...@redhat.com>; He, Roger <hongbo...@amd.com>; Maarten Lankhorst 
<maarten.lankho...@canonical.com>; Francisco Jerez <curroje...@riseup.net>; 
Viresh Kumar <vire...@kernel.org>; Shiraz Hashim 
<shiraz.linux.ker...@gmail.com>; Linus Walleij <linus.wall...@linaro.org>; 
dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: next/master build: 198 builds: 1 failed, 197 passed, 1 error, 148 
warnings (next-20180110)

On Wed, Jan 10, 2018 at 4:54 PM, Christian König <christian.koe...@amd.com> 
wrote:
> Hi Arnd,
>
> Am 10.01.2018 um 16:45 schrieb Arnd Bergmann:
>>>
>>> 14 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1186:2: warning: 
>>> ignoring return value of 'register_shrinker', declared with 
>>> attribute warn_unused_result [-Wunused-result]
>>> 14 drivers/gpu/drm/ttm/ttm_page_alloc.c:485:2: warning: ignoring 
>>> return value of 'register_shrinker', declared with attribute 
>>> warn_unused_result [-Wunused-result]
>>
>> ttm and kvm are now the last user of register_shrinker that doesn't 
>> propagate the return code to its caller, all other callers got fixed 
>> in one way or another.
>>
>> I tried to fix this one too, but couldn't come up with a proper way 
>> of unwinding both
>> kobject_init_and_add() and ttm_pool_mm_shrink_init():
>>
>>  ret = kobject_init_and_add(&_manager->kobj, _pool_kobj_type,
>> >kobj, "pool");
>>  if (unlikely(ret != 0)) {
>>  kobject_put(&_manager->kobj);
>>  _manager = NULL;
>>  return ret;
>>  }
>>
>>  ttm_pool_mm_shrink_init(_manager);
>>
>> Calling kobject_put() after a failed kobject_init_and_add() seemed 
>> wrong, and it also appears to be missing a kfree(), so I didn't want 
>> to mess it up any further. Added a few people to Cc that touched this 
>> file most, maybe one of them can have a look, or they already have a 
>> patch waiting to get merged.
>
>
> That isn't urgent, isn't it? So I would say I put it on my TODO list 
> and I'm going to take care of it no later than 4.17.
>
> Otherwise Roger or me could take a look tomorrow.

My understanding is that the warning will be in 4.16, so the fix should be as 
well, if only to get a clean build again. There were around a dozen such 
warnings when the warn_unused_result got added, but the others are all fixed in 
linux-next.

This is how the flag got added:

commit 64067c5cbfa24a2202b92e8fda7323610cad3043
Author: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Date:   Fri Jan 5 13:25:45 2018 +1100

mm,vmscan: mark register_shrinker() as __must_check

There are users not checking for register_shrinker() failure.  Continuing
with ignoring failure can lead to later oops at unregister_shrinker().

Link: 
http://lkml.kernel.org/r/1511265757-15563-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp
Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>


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

RE: [PATCH] drm/ttm: optimize errors checking and free _manager when finishing

[He, Roger]

-Original Message-
From: Xiongwei Song [mailto:sxwj...@gmail.com] 
Sent: Sunday, December 31, 2017 7:40 PM
To: Koenig, Christian <christian.koe...@amd.com>; He, Roger 
<hongbo...@amd.com>; airl...@linux.ie
Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org
Subject: [PATCH] drm/ttm: optimize errors checking and free _manager when 
finishing

In the function ttm_page_alloc_init, kzalloc call is made for variable 
_manager, we need to check its return value, it may return NULL.

In the function ttm_page_alloc_fini, we need to call kfree for variable 
_manager, instead of make _manager NULL directly.

Signed-off-by: Xiongwei Song <sxwj...@gmail.com>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index b5ba6441489f..e20a0b8e352b 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -1007,6 +1007,10 @@ int ttm_page_alloc_init(struct ttm_mem_global *glob, 
unsigned max_pages)
pr_info("Initializing pool allocator\n");
 
_manager = kzalloc(sizeof(*_manager), GFP_KERNEL);
+   if (!_manager) {
+   ret = -ENOMEM;
+   goto out;
+   }
Seems we only need above here for this patch I think. 
The rest is no need, because ttm_pool_kobj_release will kfree _manager.


Thanks
Roger(Hongbo.He)

ttm_page_pool_init_locked(&_manager->wc_pool, GFP_HIGHUSER, "wc", 0);
 
@@ -1034,13 +1038,17 @@ int ttm_page_alloc_init(struct ttm_mem_global *glob, 
unsigned max_pages)
   >kobj, "pool");
if (unlikely(ret != 0)) {
kobject_put(&_manager->kobj);
-   _manager = NULL;
-   return ret;
+   goto out_free_mgr;
}
 
ttm_pool_mm_shrink_init(_manager);
 
return 0;
+out_free_mgr:
+   kfree(_manager);
+   _manager = NULL;
+out:
+   return ret;
 }
 
 void ttm_page_alloc_fini(void)
@@ -1055,6 +1063,7 @@ void ttm_page_alloc_fini(void)
ttm_page_pool_free(&_manager->pools[i], FREE_ALL_PAGES, true);
 
kobject_put(&_manager->kobj);
+   kfree(_manager);
_manager = NULL;
 }
 
--
2.15.1

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

RE: [PATCH] staging: vboxvideo adapt to new TTM interface

[He, Roger]

+Alex

Do you know who can help about it?


Thanks
Roger(Hongbo.He)

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
He, Roger
Sent: Thursday, December 28, 2017 6:21 PM
To: dri-devel@lists.freedesktop.org; Bridgman, John <john.bridg...@amd.com>; 
Gregory, Robert <robert.greg...@amd.com>; Khan, Tahir <tahir.k...@amd.com>; 
Shamim, Zafar <zafar.sha...@amd.com>
Cc: hdego...@redhat.com; Koenig, Christian <christian.koe...@amd.com>; 
gre...@linuxfoundation.org
Subject: RE: [PATCH] staging: vboxvideo adapt to new TTM interface

Seems I have no permission to push the patch into amd-staging-drm-next.
Needs Whitelisted.

http://git.amd.com:8080/#/c/124051/1
anyone can help?

Thanks
Roger(Hongbo.He)
-Original Message-
From: Zhou, David(ChunMing) 
Sent: Thursday, December 28, 2017 12:24 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Cc: hdego...@redhat.com; gre...@linuxfoundation.org; Koenig, Christian 
<christian.koe...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>
Subject: Re: [PATCH] staging: vboxvideo adapt to new TTM interface

Reviewed-by: Chunming Zhou <david1.z...@amd.com>


On 2017年12月28日 11:35, Roger He wrote:
> Fixes interface change done in the following commit:
> eb86c98 drm/ttm: use an operation ctx for ttm_tt_populate in ttm_bo_driver
>
> i missed this driver because it is in staging dir.
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/staging/vboxvideo/vbox_ttm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vboxvideo/vbox_ttm.c 
> b/drivers/staging/vboxvideo/vbox_ttm.c
> index 231c89e..55f14c9 100644
> --- a/drivers/staging/vboxvideo/vbox_ttm.c
> +++ b/drivers/staging/vboxvideo/vbox_ttm.c
> @@ -213,9 +213,10 @@ static struct ttm_tt *vbox_ttm_tt_create(struct 
> ttm_bo_device *bdev,
>   return tt;
>   }
>   
> -static int vbox_ttm_tt_populate(struct ttm_tt *ttm)
> +static int vbox_ttm_tt_populate(struct ttm_tt *ttm,
> + struct ttm_operation_ctx *ctx)
>   {
> - return ttm_pool_populate(ttm);
> + return ttm_pool_populate(ttm, ctx);
>   }
>   
>   static void vbox_ttm_tt_unpopulate(struct ttm_tt *ttm)

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

RE: [PATCH] staging: vboxvideo adapt to new TTM interface

[He, Roger]

Seems I have no permission to push the patch into amd-staging-drm-next.
Needs Whitelisted.

http://git.amd.com:8080/#/c/124051/1
anyone can help?

Thanks
Roger(Hongbo.He)
-Original Message-
From: Zhou, David(ChunMing) 
Sent: Thursday, December 28, 2017 12:24 PM
To: He, Roger <hongbo...@amd.com>; dri-devel@lists.freedesktop.org
Cc: hdego...@redhat.com; gre...@linuxfoundation.org; Koenig, Christian 
<christian.koe...@amd.com>; Zhou, David(ChunMing) <david1.z...@amd.com>
Subject: Re: [PATCH] staging: vboxvideo adapt to new TTM interface

Reviewed-by: Chunming Zhou <david1.z...@amd.com>


On 2017年12月28日 11:35, Roger He wrote:
> Fixes interface change done in the following commit:
> eb86c98 drm/ttm: use an operation ctx for ttm_tt_populate in ttm_bo_driver
>
> i missed this driver because it is in staging dir.
>
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/staging/vboxvideo/vbox_ttm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vboxvideo/vbox_ttm.c 
> b/drivers/staging/vboxvideo/vbox_ttm.c
> index 231c89e..55f14c9 100644
> --- a/drivers/staging/vboxvideo/vbox_ttm.c
> +++ b/drivers/staging/vboxvideo/vbox_ttm.c
> @@ -213,9 +213,10 @@ static struct ttm_tt *vbox_ttm_tt_create(struct 
> ttm_bo_device *bdev,
>   return tt;
>   }
>   
> -static int vbox_ttm_tt_populate(struct ttm_tt *ttm)
> +static int vbox_ttm_tt_populate(struct ttm_tt *ttm,
> + struct ttm_operation_ctx *ctx)
>   {
> - return ttm_pool_populate(ttm);
> + return ttm_pool_populate(ttm, ctx);
>   }
>   
>   static void vbox_ttm_tt_unpopulate(struct ttm_tt *ttm)

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

RE: [PATCH 2/3] drm/ttm: add new function to check if bo is allowable to evict or swapout

[He, Roger]

+ Thomas 

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Roger He
Sent: Monday, December 25, 2017 2:08 PM
To: dri-devel@lists.freedesktop.org
Cc: He, Roger <hongbo...@amd.com>; Koenig, Christian <christian.koe...@amd.com>
Subject: [PATCH 2/3] drm/ttm: add new function to check if bo is allowable to 
evict or swapout

extract a function as ttm_bo_evict_swapout_allowable since eviction and swapout 
can share same logic.

v2: modify commit message and add description in the code

Change-Id: I80a475a93fceed8d66d74a1832c815a0756341ac
Signed-off-by: Roger He <hongbo...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 40 ++--
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
e7595b4..5e64091 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -706,6 +706,34 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
*bo,  }  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
+/**
+ * Check the target bo is allowable to be evicted or swapout, including cases:
+ *
+ * a. if share same reservation object with ctx->resv, have assumption
+ * reservation objects should already be locked, so not lock again and
+ * return true directly when either the opreation 
+allow_reserved_eviction
+ * or the target bo already is in delayed free list;
+ *
+ * b. Otherwise, trylock it.
+ */
+static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
+   struct ttm_operation_ctx *ctx, bool *locked) {
+   bool ret = false;
+
+   *locked = false;
+   if (bo->resv == ctx->resv) {
+   reservation_object_assert_held(bo->resv);
+   if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
+   ret = true;
+   } else {
+   *locked = reservation_object_trylock(bo->resv);
+   ret = *locked;
+   }
+
+   return ret;
+}
+
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
   uint32_t mem_type,
   const struct ttm_place *place, @@ -721,21 
+749,13 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
spin_lock(>lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
list_for_each_entry(bo, >lru[i], lru) {
-   if (bo->resv == ctx->resv) {
-   if (!ctx->allow_reserved_eviction &&
-   list_empty(>ddestroy))
-   continue;
-   } else {
-   locked = reservation_object_trylock(bo->resv);
-   if (!locked)
-   continue;
-   }
+   if (!ttm_bo_evict_swapout_allowable(bo, ctx, ))
+   continue;
 
if (place && !bdev->driver->eviction_valuable(bo,
  place)) {
if (locked)
reservation_object_unlock(bo->resv);
-   locked = false;
continue;
}
break;
--
2.7.4

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

RE: [PATCH 6/7] drm/ttm: add ttm_bo_evict_swapout_allowable to check bo is allowable to evict or swapout

[He, Roger]

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Thomas Hellstrom
Sent: Thursday, December 21, 2017 3:51 PM
To: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 6/7] drm/ttm: add ttm_bo_evict_swapout_allowable to check 
bo is allowable to evict or swapout

On 12/20/2017 11:35 AM, Roger He wrote:
> extract this function since eviction and swapout share same logic

But it's only used in eviction?

Yes, currently it only used in eviction. And will be used in Swapout.

>
> Change-Id: I80a475a93fceed8d66d74a1832c815a0756341ac
> Signed-off-by: Roger He 
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 29 +++--
>   1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index e7595b4..313925c 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -706,6 +706,23 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
> *bo,
>   }
>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
> +static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> + struct ttm_operation_ctx *ctx, bool *locked)

Could we have a description in the code what this function actually 
does? admittedly it checks whether a swapout or eviction is allowable, 
but it aso performs a recursive tryreserve(), and that is of value to any 
future code reader.?

Ok.

Thanks
Roger(Hongbo.He)

/Thomas


> +{
> + bool ret = false;
> +
> + *locked = false;
> + if (bo->resv == ctx->resv) {
> + if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
> + ret = true;
> + } else {
> + *locked = reservation_object_trylock(bo->resv);
> + ret = *locked;
> + }
> +
> + return ret;
> +}
> +
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>  uint32_t mem_type,
>  const struct ttm_place *place, @@ -721,21 
> +738,13 @@ 
> static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   spin_lock(>lru_lock);
>   for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   list_for_each_entry(bo, >lru[i], lru) {
> - if (bo->resv == ctx->resv) {
> - if (!ctx->allow_reserved_eviction &&
> - list_empty(>ddestroy))
> - continue;
> - } else {
> - locked = reservation_object_trylock(bo->resv);
> - if (!locked)
> - continue;
> - }
> + if (!ttm_bo_evict_swapout_allowable(bo, ctx, ))
> + continue;
>   
>   if (place && !bdev->driver->eviction_valuable(bo,
> place)) {
>   if (locked)
>   reservation_object_unlock(bo->resv);
> - locked = false;
>   continue;
>   }
>   break;


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

RE: [PATCH] drm/ttm: drop the spin in delayed delete if the trylock doesn't work

[He, Roger]

Reviewed-by: Roger He 

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Friday, December 22, 2017 2:06 AM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Subject: [PATCH] drm/ttm: drop the spin in delayed delete if the trylock 
doesn't work

Thomas actually noticed that, but I didn't realized what he meant until now.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
60bb5c12b568..84dfa2368a72 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -592,6 +592,8 @@ static bool ttm_bo_delayed_delete(struct ttm_bo_device 
*bdev, bool remove_all)
 
} else if (reservation_object_trylock(bo->resv)) {
ttm_bo_cleanup_refs(bo, false, !remove_all, true);
+   } else {
+   spin_unlock(>lru_lock);
}
 
kref_put(>list_kref, ttm_bo_release_list);
--
2.11.0

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

RE: [PATCH 3/7] drm/ttm: use an operation ctx for ttm_mem_global_alloc_page

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Wednesday, December 20, 2017 9:36 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/7] drm/ttm: use an operation ctx for 
ttm_mem_global_alloc_page

Commit message!

Am 20.12.2017 um 11:34 schrieb Roger He:
> Change-Id: I4104a12e09a374b6477a0dd5a8fce26dce27a746
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_memory.c | 15 ---
>   drivers/gpu/drm/ttm/ttm_page_alloc.c |  6 +-
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  8 ++--
>   include/drm/ttm/ttm_memory.h |  3 ++-
>   4 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c 
> b/drivers/gpu/drm/ttm/ttm_memory.c
> index 525d3b6..8df0755 100644
> --- a/drivers/gpu/drm/ttm/ttm_memory.c
> +++ b/drivers/gpu/drm/ttm/ttm_memory.c
> @@ -539,15 +539,14 @@ int ttm_mem_global_alloc(struct ttm_mem_global *glob, 
> uint64_t memory,
>   EXPORT_SYMBOL(ttm_mem_global_alloc);
>   
>   int ttm_mem_global_alloc_page(struct ttm_mem_global *glob,
> -   struct page *page, uint64_t size)
> +   struct page *page, uint64_t size,
> +   struct ttm_operation_ctx *ctx)
>   {
> -
> + int ret;
>   struct ttm_mem_zone *zone = NULL;
> - struct ttm_operation_ctx ctx = {
> - .interruptible = false,
> - .no_wait_gpu = false
> - };
> + bool tmp_no_wait_gpu = ctx->no_wait_gpu;

Mhm, please drop that. That the function might wait for the GPU even 
when the caller requested not to do so sounds like a bug to me.

Yes. I will remove this. Later I will send new patches. Will appreciate If 
Thomas can help to verify that.

Thanks
Roger(Hongbo.He)  
>   
> + ctx->no_wait_gpu = false;
>   /**
>* Page allocations may be registed in a single zone
>* only if highmem or !dma32.
> @@ -560,7 +559,9 @@ int ttm_mem_global_alloc_page(struct ttm_mem_global *glob,
>   if (glob->zone_dma32 && page_to_pfn(page) > 0x0010UL)
>   zone = glob->zone_kernel;
>   #endif
> - return ttm_mem_global_alloc_zone(glob, zone, size, );
> + ret = ttm_mem_global_alloc_zone(glob, zone, size, ctx);
> + ctx->no_wait_gpu = tmp_no_wait_gpu;
> + return ret;
>   }
>   
>   void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct 
> page *page, diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index b5ba644..8f93ff3 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -1061,6 +1061,10 @@ void ttm_page_alloc_fini(void)
>   int ttm_pool_populate(struct ttm_tt *ttm)
>   {
>   struct ttm_mem_global *mem_glob = ttm->glob->mem_glob;
> + struct ttm_operation_ctx ctx = {
> + .interruptible = false,
> + .no_wait_gpu = false
> + };
>   unsigned i;
>   int ret;
>   
> @@ -1076,7 +1080,7 @@ int ttm_pool_populate(struct ttm_tt *ttm)
>   
>   for (i = 0; i < ttm->num_pages; ++i) {
>   ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i],
> - PAGE_SIZE);
> + PAGE_SIZE, );
>   if (unlikely(ret != 0)) {
>   ttm_pool_unpopulate(ttm);
>   return -ENOMEM;
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index bda00b2..8aac86a 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -927,6 +927,10 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct 
> device *dev)
>   {
>   struct ttm_tt *ttm = _dma->ttm;
>   struct ttm_mem_global *mem_glob = ttm->glob->mem_glob;
> + struct ttm_operation_ctx ctx = {
> + .interruptible = false,
> + .no_wait_gpu = false
> + };
>   unsigned long num_pages = ttm->num_pages;
>   struct dma_pool *pool;
>   enum pool_type type;
> @@ -962,7 +966,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct 
> device *dev)
>   break;
>   
>   ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i],
> - pool->size);
> + pool->size, );
>   if (unlikely(ret != 0)) {
>   ttm_dma_unpopula

RE: [PATCH 3/3] drm/ttm: cleanup some old defines

[He, Roger]

The series is: Reviewed-by: Roger He 

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Saturday, December 16, 2017 2:32 AM
To: dri-devel@lists.freedesktop.org
Subject: [PATCH 3/3] drm/ttm: cleanup some old defines

Use pr_debug instead of TTM_DEBUG, fix the lockdep assert and remove the unused 
constant.

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

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
5cc96b232c17..60bb5c12b568 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -42,10 +42,6 @@
 #include 
 #include 
 
-#define TTM_ASSERT_LOCKED(param)
-#define TTM_DEBUG(fmt, arg...)
-#define TTM_BO_HASH_ORDER 13
-
 static int ttm_bo_swapout(struct ttm_mem_shrink *shrink);  static void 
ttm_bo_global_kobj_release(struct kobject *kobj);
 
@@ -233,7 +229,7 @@ static int ttm_bo_add_ttm(struct ttm_buffer_object *bo, 
bool zero_alloc)
int ret = 0;
uint32_t page_flags = 0;
 
-   TTM_ASSERT_LOCKED(>mutex);
+   reservation_object_assert_held(bo->resv);
bo->ttm = NULL;
 
if (bdev->need_dma32)
@@ -1544,12 +1540,12 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev)
cancel_delayed_work_sync(>wq);
 
if (ttm_bo_delayed_delete(bdev, true))
-   TTM_DEBUG("Delayed destroy list was clean\n");
+   pr_debug("Delayed destroy list was clean\n");
 
spin_lock(>lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i)
if (list_empty(>man[0].lru[0]))
-   TTM_DEBUG("Swap list %d was clean\n", i);
+   pr_debug("Swap list %d was clean\n", i);
spin_unlock(>lru_lock);
 
drm_vma_offset_manager_destroy(>vma_manager);
--
2.11.0

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

RE: [PATCH] drm/ttm: enable eviction for Per-VM-BO

[He, Roger]

-Original Message-
From: Thomas Hellstrom [mailto:tho...@shipmail.org] 
Sent: Friday, December 15, 2017 2:25 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH] drm/ttm: enable eviction for Per-VM-BO

Hi.

On 12/14/2017 09:10 AM, Roger He wrote:
> Change-Id: I0c6ece0decd18d30ccc94e5c7ca106d351941c62
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 11 +--
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index 098b22e..ba5b486 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -707,7 +707,6 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
> *bo,
>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> -struct reservation_object *resv,
>  uint32_t mem_type,
>  const struct ttm_place *place,
>  struct ttm_operation_ctx *ctx) @@ -722,8 +721,9 
> @@ static 
> int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   spin_lock(>lru_lock);
>   for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   list_for_each_entry(bo, >lru[i], lru) {
> - if (bo->resv == resv) {
> - if (list_empty(>ddestroy))
> + if (bo->resv == ctx->resv) {
> + if (!ctx->allow_reserved_eviction &&
> + list_empty(>ddestroy))
>   continue;

It looks to me like a value of locked==true could leak through from a 
previous loop iteration here, so that locked ends up to betrue if 
(bo->resv == resv  && ctx->allow_reserved_eviction), even if no locking was 
taking place. Perhaps-re-initialize locked to   false on each loop 
iteration?

At  the beginning, I also have this concern about incorrect using of locked. 
So, add patch a few days ago.

init locked again to prevent incorrect unlock
is it help?


Thanks
Roger(Hongbo.He)

/Thomas

>   } else {
>   locked = reservation_object_trylock(bo->resv);
> @@ -835,7 +835,7 @@ static int ttm_bo_mem_force_space(struct 
> ttm_buffer_object *bo,
>   return ret;
>   if (mem->mm_node)
>   break;
> - ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place, ctx);
> + ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
>   if (unlikely(ret != 0))
>   return ret;
>   } while (1);
> @@ -1332,8 +1332,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device 
> *bdev,
>   for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   while (!list_empty(>lru[i])) {
>   spin_unlock(>lru_lock);
> - ret = ttm_mem_evict_first(bdev, NULL, mem_type,
> -   NULL, );
> + ret = ttm_mem_evict_first(bdev, mem_type, NULL, );
>   if (ret)
>   return ret;
>   spin_lock(>lru_lock);


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

RE: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[He, Roger]

Hi Thomas:

Really sorry for that and will keep that in mind.
If necessary, next time I will send cover letter to provide more background and 
details.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Thomas Hellstrom [mailto:tho...@shipmail.org] 
Sent: Thursday, December 14, 2017 3:21 AM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into 
ttm_operation_ctx

Hi,

I think this series is quite poorly documented. We should have a log message 
explaining the purpose of the commit.
Also since it's not obvious what the series is attempting to achieve, please 
add a 0/X series header message..

/Thomas


On 12/12/2017 10:33 AM, Roger He wrote:
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   include/drm/ttm/ttm_bo_api.h | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h 
> b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>*
>* @interruptible: Sleep interruptible if sleeping.
>* @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>*
>* Context for TTM operations like changing buffer placement or general 
> memory
>* allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>   struct ttm_operation_ctx {
>   bool interruptible;
>   bool no_wait_gpu;
> + bool on_alloc_stage;
> + struct reservation_object *resv;
>   uint64_t bytes_moved;
>   };
>   


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

RE: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Tuesday, December 12, 2017 6:40 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6
> Signed-off-by: Roger He <hongbo...@amd.com>

I would squash this one with patch #6.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 7 +++
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index eb8c568..22b6ca5 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device 
> *bdev,
>   spin_lock(>lru_lock);
>   for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   list_for_each_entry(bo, >lru[i], lru) {
> - if (bo->resv == resv) {
> - if (list_empty(>ddestroy))
> - continue;
> - } else {
> + if (!ctx ||
> + !(ctx->on_alloc_stage &&
> + bo->resv == ctx->resv)) {

Coding style: The lines stating with "!(ctx" and "bo->resv" are to far 
to the right.

Additional to that I think ctx is mandatory and doesn't need a check 
(but might be wrong). If it isn't it's probably time to make itmandatory.

And I would use (ctx->on_alloc_stage || list_empty(>ddestroy)) as 
check, we probably still want to be able to handledeleted BOs here 
during CS.

Change that as below, I think it can cover the case you described. No matter 
the Bo is in deleted list or not.
if (!ctx->allow_reserved_eviction || bo->resv != ctx->resv) {
locked = 
kcl_reservation_object_trylock(bo->resv);
if (!locked)
continue;
}

Thanks
Roger(Hongbo.He)



Christian.

>   locked = reservation_object_trylock(bo->resv);
>   if (!locked)
>   continue;

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

RE: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

[He, Roger]

That is a bug fix, isn't it? If yes maybe add CC:stable and commit it 
first before all other patches.

Fortunately so far there is no issue directly resulted from that.

Thanks
Roger(Hongbo.He)

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Tuesday, December 12, 2017 6:37 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe
> Signed-off-by: Roger He <hongbo...@amd.com>

Reviewed-by: Christian König <christian.koe...@amd.com>

That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first 
before all other patches.

Christian.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> place)) {
>   if (locked)
>   reservation_object_unlock(bo->resv);
> + locked = false;
>   continue;
>   }
>   break;

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

RE: [PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Tuesday, December 12, 2017 6:34 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx

Am 12.12.2017 um 10:33 schrieb Roger He:
> include ttm_bo_move_memcpy and ttm_bo_move_ttm
>
> Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a
> Signed-off-by: Roger He <hongbo...@amd.com>

Reviewed-by: Christian König <christian.koe...@amd.com>

   But please separate that out and wait for a few days before 
committing, maybe some nouveau devs have objections.

Ok.

Thanks
Roger(Hongbo.He)

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++
>   drivers/gpu/drm/nouveau/nouveau_bo.c| 7 +++
>   drivers/gpu/drm/qxl/qxl_ttm.c   | 3 +--
>   drivers/gpu/drm/radeon/radeon_ttm.c | 7 +++
>   drivers/gpu/drm/ttm/ttm_bo.c| 6 ++
>   drivers/gpu/drm/ttm/ttm_bo_util.c   | 8 
>   include/drm/ttm/ttm_bo_driver.h | 4 ++--
>   7 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 7db9556..c307a7d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object 
> *bo, bool evict,
>   if (unlikely(r)) {
>   goto out_cleanup;
>   }
> - r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem);
> + r = ttm_bo_move_ttm(bo, ctx, new_mem);
>   out_cleanup:
>   ttm_bo_mem_put(bo, _mem);
>   return r;
> @@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object 
> *bo, bool evict,
>   if (unlikely(r)) {
>   return r;
>   }
> - r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, _mem);
> + r = ttm_bo_move_ttm(bo, ctx, _mem);
>   if (unlikely(r)) {
>   goto out_cleanup;
>   }
> @@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object 
> *bo, bool evict,
>   
>   if (r) {
>   memcpy:
> - r = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -ctx->no_wait_gpu, new_mem);
> + r = ttm_bo_move_memcpy(bo, ctx, new_mem);
>   if (r) {
>   return r;
>   }
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c 
> b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 949bf6b..6b6fb20 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, 
> bool evict, bool intr,
>   if (ret)
>   goto out;
>   
> - ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg);
> + ret = ttm_bo_move_ttm(bo, , new_reg);
>   out:
>   ttm_bo_mem_put(bo, _reg);
>   return ret;
> @@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, 
> bool evict, bool intr,
>   if (ret)
>   return ret;
>   
> - ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, _reg);
> + ret = ttm_bo_move_ttm(bo, , _reg);
>   if (ret)
>   goto out;
>   
> @@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool 
> evict,
>   /* Fallback to software copy. */
>   ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   if (ret == 0)
> - ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -  ctx->no_wait_gpu, new_reg);
> + ret = ttm_bo_move_memcpy(bo, ctx, new_reg);
>   
>   out:
>   if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) { 
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c 
> b/drivers/gpu/drm/qxl/qxl_ttm.c index d866f32..78ce118 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool 
> evict,
>   qxl_move_null(bo, new_mem);
>   return 0;
>   }
> - return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu,
> -   new_mem);
> + return ttm_bo_move_memcpy(bo, ctx, new_mem);
>   }
>   
>   static void qxl_bo_move_notify(struct ttm_buffer_object *bo, diff 
> --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 98e30d7..557fd79 100644
> --- a/drivers/gpu/drm/radeon/ra

RE: [PATCH 1/7] drm/ttm:fix incorrect calculate on shrink_pages

[He, Roger]

Reviewed-by: Roger He 

Thanks
Roger(Hongbo.He)
-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of Monk 
Liu
Sent: Tuesday, December 12, 2017 5:47 PM
To: amd-...@lists.freedesktop.org
Cc: Liu, Monk 
Subject: [PATCH 1/7] drm/ttm:fix incorrect calculate on shrink_pages

shrink_pages is in unit of Order after ttm_page_pool_free, but it is used by 
nr_free in next round so need change it into native page unit

Change-Id: I33b77ac1616e24b1b881eee54c3bd7342cfa9ab8
Signed-off-by: Monk Liu 
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 44343a2..71945cc 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -455,6 +455,7 @@ ttm_pool_shrink_scan(struct shrinker *shrink, struct 
shrink_control *sc)
freed += (nr_free_pool - shrink_pages) << pool->order;
if (freed >= sc->nr_to_scan)
break;
+   shrink_pages <<= pool->order;
}
mutex_unlock();
return freed;
--
2.7.4

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

RE: [PATCH] drm/ttm: swap consecutive allocated pooled pages v4

[He, Roger]

Reviewed-by: Roger He <hongbo...@amd.com>

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Tuesday, December 05, 2017 8:55 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; He, Roger 
<hongbo...@amd.com>
Subject: [PATCH] drm/ttm: swap consecutive allocated pooled pages v4

When we detect consecutive allocation of pages swap them to avoid accidentally 
freeing them as huge page.

v2: use swap
v3: check if it's really the first allocated page
v4: don't touch the loop variable

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index b6f7ce286fb1..44343a2bf55c 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -958,8 +958,15 @@ static int ttm_get_pages(struct page **pages, unsigned 
npages, int flags,
r = ttm_page_pool_get_pages(pool, , flags, cstate,
npages - count, 0);
 
-   list_for_each_entry(p, , lru)
-   pages[count++] = p;
+   first = count;
+   list_for_each_entry(p, , lru) {
+   struct page *tmp = p;
+
+   /* Swap the pages if we detect consecutive order */
+   if (count > first && pages[count - 1] == tmp - 1)
+   swap(tmp, pages[count - 1]);
+   pages[count++] = tmp;
+   }
 
if (r) {
/* If there is any pages in the list put them back to
--
2.11.0

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

RE: [PATCH 2/2] drm/ttm: swap consecutive allocated pooled pages v3

[He, Roger]

Hold on,  only Patch 1 is Reviewed-by: Roger He <hongbo...@amd.com>.

For Patch 2:

+   list_for_each_entry(p, , lru) {
+   /* Swap the pages if we detect consecutive order */
+   if (count > first && pages[count - 1] == p - 1) {   

+   swap(p, pages[count - 1]);// swap seems breaking 
the plist, after that iteration plist will not work.
}
pages[count++] = temp;
+   }

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
He, Roger
Sent: Tuesday, December 05, 2017 10:01 AM
To: Christian König <ckoenig.leichtzumer...@gmail.com>; 
dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Subject: RE: [PATCH 2/2] drm/ttm: swap consecutive allocated pooled pages v3

Series is:
Reviewed-by: Roger He <hongbo...@amd.com>

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Monday, December 04, 2017 8:46 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; He, Roger 
<hongbo...@amd.com>
Subject: [PATCH 2/2] drm/ttm: swap consecutive allocated pooled pages v3

When we detect consecutive allocation of pages swap them to avoid accidentally 
freeing them as huge page.

v2: use swap
v3: check if it's really the first allocated page

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 7c4d4edce0ba..da8a50f7c8fe 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -954,8 +954,13 @@ static int ttm_get_pages(struct page **pages, unsigned 
npages, int flags,
r = ttm_page_pool_get_pages(pool, , flags, cstate,
npages - count, 0);
 
-   list_for_each_entry(p, , lru)
+   first = count;
+   list_for_each_entry(p, , lru) {
+   /* Swap the pages if we detect consecutive order */
+   if (count > first && pages[count - 1] == p - 1)
+   swap(p, pages[count - 1]);
pages[count++] = p;
+   }
 
if (r) {
/* If there is any pages in the list put them back to
--
2.11.0

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

RE: [PATCH 2/2] drm/ttm: swap consecutive allocated pooled pages v3

[He, Roger]

Series is:
Reviewed-by: Roger He <hongbo...@amd.com>

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Monday, December 04, 2017 8:46 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; He, Roger 
<hongbo...@amd.com>
Subject: [PATCH 2/2] drm/ttm: swap consecutive allocated pooled pages v3

When we detect consecutive allocation of pages swap them to avoid accidentally 
freeing them as huge page.

v2: use swap
v3: check if it's really the first allocated page

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 7c4d4edce0ba..da8a50f7c8fe 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -954,8 +954,13 @@ static int ttm_get_pages(struct page **pages, unsigned 
npages, int flags,
r = ttm_page_pool_get_pages(pool, , flags, cstate,
npages - count, 0);
 
-   list_for_each_entry(p, , lru)
+   first = count;
+   list_for_each_entry(p, , lru) {
+   /* Swap the pages if we detect consecutive order */
+   if (count > first && pages[count - 1] == p - 1)
+   swap(p, pages[count - 1]);
pages[count++] = p;
+   }
 
if (r) {
/* If there is any pages in the list put them back to
--
2.11.0

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

RE: [PATCH 1/2] drm/ttm: swap consecutive allocated cached pages v2

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Monday, December 04, 2017 8:13 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; He, Roger 
<hongbo...@amd.com>; ckoenig.leichtzumer...@gmail.com
Subject: [PATCH 1/2] drm/ttm: swap consecutive allocated cached pages v2

When we detect consecutive allocation of pages swap them to avoid accidentally 
freeing them as huge page.

v2: use swap

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index c82d94cbbabc..b6c5148607e9 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -921,6 +921,10 @@ static int ttm_get_pages(struct page **pages, unsigned 
npages, int flags,
return -ENOMEM;
}
 
How about add first index as below?

first = i;  //init  first here
while (npages) {
p = alloc_page(gfp_flags);
if (!p) {
pr_debug("Unable to allocate page\n");
return -ENOMEM;
}

/* Swap the pages if we detect consecutive order */
if (i > first && pages[i - 1] == p - 1) 
 //change to (i > first), because I worry if npages is 
513, the first regular page is exactly consecutive with last page of huge page. 
Then swap them is not what I want here.
swap(p, pages[i - 1]);

pages[i++] = p;
--npages;
}

pages[i++] = p;
--npages;
}
--
2.11.0

___
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

[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 2/4] drm/ttm: add page order support in ttm_pages_put

[He, Roger]

Hi Christian:

This is old patch, I already send new patches. And it is clean and simple.  
Please check that.

Thanks
Roger(Hongbo.He)

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Wednesday, November 22, 2017 5:29 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <christian.koe...@amd.com>
Subject: Re: [PATCH 2/4] drm/ttm: add page order support in ttm_pages_put

I would rather completely nuke ttm_pages_put() instead of coming up with more 
workarounds here.

Going to provide a cleanup patch to show what I mean, just give me a minute.

Regards,
Christian.

Am 22.11.2017 um 06:36 schrieb Roger He:
> Change-Id: Ia55b206d95812c5afcfd6cec29f580758d1f50f0
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_page_alloc.c | 42 
> +---
>   1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 0a0c653..de64209 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -285,13 +285,39 @@ 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]);
> + struct page **pages_to_free = NULL;
> + struct page **pages_array;
> + struct page *p;
> + unsigned int i, j, pages_nr = (1 << order);
> +
> + if (order > 0) {
> + pages_to_free = kmalloc_array(pages_nr, sizeof(struct page *),
> + GFP_KERNEL);
> + if (!pages_to_free) {
> + pr_err("Failed to allocate memory for ttm pages put 
> operation\n");
> + return;
> + }
> + }
> +
> + for (i = 0; i < npages; ++i) {
> + if (order > 0) {
> + p = pages[i];
> + for (j = 0; j < pages_nr; ++j)
> + pages_to_free[j] = p++;
> +
> + pages_array = pages_to_free;
> + } else
> + pages_array = pages;
> +
> + if (set_pages_array_wb(pages_array, pages_nr))
> + pr_err("Failed to set %d pages to wb!\n", pages_nr);
> + __free_pages(pages[i], order);
> + }
> +
> + kfree(pages_to_free);
>   }
>   
>   static void ttm_pool_update_free_locked(struct ttm_page_pool *pool, 
> @@ -354,7 +380,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;
>   
> @@ -389,7 +415,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 1/4] drm/ttm: add page order in page pool

[He, Roger]

-Original Message-
From: Koenig, Christian 
Sent: Wednesday, November 22, 2017 3:48 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/ttm: add page order in page pool

Am 22.11.2017 um 06:36 schrieb Roger He:
> to indicate page order for each element in the pool
>
> Change-Id: Ic609925ca5d2a5d4ad49d6becf505388ce3624cf
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_page_alloc.c | 42 
> ++--
>   1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 72ea037..0a0c653 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -81,6 +81,7 @@ struct ttm_page_pool {
>   char*name;
>   unsigned long   nfrees;
>   unsigned long   nrefills;
> + unsigned intorder;
>   };
>   
>   /**
> @@ -412,6 +413,7 @@ ttm_pool_shrink_scan(struct shrinker *shrink, struct 
> shrink_control *sc)
>   struct ttm_page_pool *pool;
>   int shrink_pages = sc->nr_to_scan;
>   unsigned long freed = 0;
> + unsigned int nr_free_pool;
>   
>   if (!mutex_trylock())
>   return SHRINK_STOP;
> @@ -421,10 +423,15 @@ ttm_pool_shrink_scan(struct shrinker *shrink, struct 
> shrink_control *sc)
>   unsigned nr_free = shrink_pages;
>   if (shrink_pages == 0)
>   break;
> +
>   pool = &_manager->pools[(i + pool_offset)%NUM_POOLS];
>   /* OK to use static buffer since global mutex is held. */
> - shrink_pages = ttm_page_pool_free(pool, nr_free, true);
> - freed += nr_free - shrink_pages;
> + nr_free_pool = (nr_free >> pool->order);
> + if (nr_free_pool == 0)
> + continue;
> +
> + shrink_pages = ttm_page_pool_free(pool, nr_free_pool, true);
> + freed += ((nr_free_pool - shrink_pages) << pool->order);
>   }
>   mutex_unlock();
>   return freed;
> @@ -436,9 +443,12 @@ ttm_pool_shrink_count(struct shrinker *shrink, struct 
> shrink_control *sc)
>   {
>   unsigned i;
>   unsigned long count = 0;
> + struct ttm_page_pool *pool;
>   
> - for (i = 0; i < NUM_POOLS; ++i)
> - count += _manager->pools[i].npages;
> + for (i = 0; i < NUM_POOLS; ++i) {
> + pool = &_manager->pools[i];
> + count += (pool->npages << pool->order);
> + }
>   
>   return count;
>   }
> @@ -932,7 +942,7 @@ static int ttm_get_pages(struct page **pages, unsigned 
> npages, int flags,
>   }
>   
>   static void ttm_page_pool_init_locked(struct ttm_page_pool *pool, gfp_t 
> flags,
> - char *name)
> + char *name, unsigned int order)
>   {
>   spin_lock_init(>lock);
>   pool->fill_lock = false;
> @@ -940,8 +950,18 @@ static void ttm_page_pool_init_locked(struct 
> ttm_page_pool *pool, gfp_t flags,
>   pool->npages = pool->nfrees = 0;
>   pool->gfp_flags = flags;
>   pool->name = name;
> + pool->order = order;
>   }
>   
> +/**
> + * Actually if TRANSPARENT_HUGEPAGE not enabled, we will not use
> + * wc_pool_huge and uc_pool_huge, so no matter whatever the page
> + * order are for those two pools
> + */
> +#ifndef CONFIG_TRANSPARENT_HUGEPAGE
> +#define  HPAGE_PMD_ORDER 9
> +#endif
> +

That still won't work and sorry I wasn't 100% clear in the last mail.

When CONFIG_TRANSPARENT_HUGEPAGE isn't set HPAGE_PMD_ORDER is defined as 
BUILD_BUG().

So you will still run into problems when that config option isn't set.

>   int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages)
>   {
>   int ret;

I suggest to just handle it here like this

#ifdef CONFIG_TRANSPARENT_HUGEPAGE
     unsigned order = HPAGE_PMD_ORDER;
#else
     unsigned order = 0;
#endif

Apart from that the patch looks good to me, Christian.


Ok, going to modify it. Thanks!

Thanks
Roger(Hongbo.He)

> @@ -952,23 +972,23 @@ int ttm_page_alloc_init(struct ttm_mem_global 
> *glob, unsigned max_pages)
>   
>   _manager = kzalloc(sizeof(*_manager), GFP_KERNEL);
>   
> - ttm_page_pool_init_locked(&_manager->wc_pool, GFP_HIGHUSER, "wc");
> + ttm_page_pool_init_locked(&_manager->wc_pool, GFP_HIGHUSER, "wc", 
> +0);
>   
> - ttm_page_pool_init_locked(&_manager->uc_pool, GFP_HIGHUSER, "uc"

RE: [PATCH 2/4] drm/ttm: add page order support in ttm_pages_put

[He, Roger]

From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Chunming Zhou
Sent: Wednesday, November 22, 2017 2:02 PM
To: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/4] drm/ttm: add page order support in ttm_pages_put




On 2017年11月22日 13:36, Roger He wrote:

Change-Id: Ia55b206d95812c5afcfd6cec29f580758d1f50f0

Signed-off-by: Roger He 

---

 drivers/gpu/drm/ttm/ttm_page_alloc.c | 42 +---

 1 file changed, 34 insertions(+), 8 deletions(-)



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

index 0a0c653..de64209 100644

--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c

+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c

@@ -285,13 +285,39 @@ 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]);

+ struct page **pages_to_free = NULL;

+ struct page **pages_array;

+ struct page *p;

+ unsigned int i, j, pages_nr = (1 << order);

+

+ if (order > 0) {

+pages_to_free = kmalloc_array(pages_nr, sizeof(struct page *),

+   GFP_KERNEL);

+if (!pages_to_free) {

+pr_err("Failed to allocate memory for ttm pages put 
operation\n");

+return;

+}

+ }

+

+ for (i = 0; i < npages; ++i) {

+if (order > 0) {

+p = pages[i];

+for (j = 0; j < pages_nr; ++j)

+pages_to_free[j] = p++;

+

+pages_array = pages_to_free;

+} else

+pages_array = pages;

+

+if (set_pages_array_wb(pages_array, pages_nr))
you can use set_pages_wb(pages[i], 1 << order) to instead of creating page 
array marked red, this way, you will not need to kmalloc and patch#3.

and more, if you select set_pages_wb, you also need to clone it in TTM like 
set_pages_array_wb for non-x86 case.

good idea, going to refine code.

Thanks
Roger(Hongbo.He)


Regards,
David Zhou




+pr_err("Failed to set %d pages to wb!\n", pages_nr);

+__free_pages(pages[i], order);

+ }

+

+ kfree(pages_to_free);

 }



 static void ttm_pool_update_free_locked(struct ttm_page_pool *pool,

@@ -354,7 +380,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;



@@ -389,7 +415,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 1/2] drm/ttm: fix ttm_mem_evict_first once more

[He, Roger]

Reviewed-by: Roger He 

Thanks
Roger(Hongbo.He)
-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Wednesday, November 15, 2017 8:32 PM
To: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 1/2] drm/ttm: fix ttm_mem_evict_first once more

The code path isn't hit at the moment, but we need to take the lock to add the 
BO back to the LRU.

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

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
07d9c6e5b6ca..7c1eac4f4b4b 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -793,10 +793,13 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
spin_unlock(>lru_lock);
 
ret = ttm_bo_evict(bo, interruptible, no_wait_gpu);
-   if (locked)
+   if (locked) {
ttm_bo_unreserve(bo);
-   else
+   } else {
+   spin_lock(>lru_lock);
ttm_bo_add_to_lru(bo);
+   spin_unlock(>lru_lock);
+   }
 
kref_put(>list_kref, ttm_bo_release_list);
return ret;
--
2.11.0

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

RE: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

Please see comments inline

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Wednesday, November 08, 2017 11:00 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

Needed for the next patch.

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 52 
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
6f55310a9d09..d23592cfe42e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct 
ttm_buffer_object *bo)  }
 
 /**
- * function ttm_bo_cleanup_refs_and_unlock
+ * function ttm_bo_cleanup_refs
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
  * Must be called with lru_lock and reservation held, this function
- * will drop both before returning.
+ * will drop the lru lock and optionally the reservation lock before returning.
  *
  * @interruptible Any sleeps should occur interruptibly.
  * @no_wait_gpu   Never wait for gpu. Return -EBUSY instead.
+ * @unlock_resv   Unlock the reservation lock as well.
  */
 
-static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
- bool interruptible,
- bool no_wait_gpu)
+static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
+  bool interruptible, bool no_wait_gpu,
+  bool unlock_resv)
 {
struct ttm_bo_global *glob = bo->glob;
struct reservation_object *resv;
@@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct 
ttm_buffer_object *bo,
if (ret && !no_wait_gpu) {
long lret;
 
-   reservation_object_unlock(bo->resv);
+   if (unlock_resv)
+   reservation_object_unlock(bo->resv);
spin_unlock(>lru_lock);
 
lret = reservation_object_wait_timeout_rcu(resv, true, @@ 
-531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct 
ttm_buffer_object *bo,
return -EBUSY;
 
spin_lock(>lru_lock);
-   ret = __ttm_bo_reserve(bo, false, true, NULL);
-
-   /*
-* We raced, and lost, someone else holds the reservation now,
-* and is probably busy in ttm_bo_cleanup_memtype_use.
-*
-* Even if it's not the case, because we finished waiting any
-* delayed destruction would succeed, so just return success
-* here.
-*/
-   if (ret) {
-   spin_unlock(>lru_lock);
-   return 0;
+   if (unlock_resv) {
+   ret = __ttm_bo_reserve(bo, false, true, NULL);
+   /*
+* We raced, and lost, someone else holds the 
reservation now,
+* and is probably busy in ttm_bo_cleanup_memtype_use.
+*
+* Even if it's not the case, because we finished 
waiting any
+* delayed destruction would succeed, so just return 
success
+* here.
+*/
+   if (ret) {
+   spin_unlock(>lru_lock);
+   return 0;
+   }
+   } else {
+   ret = 0;
}
}

[Roger]: 
//Looks like we also need the condition adjudge here.  Otherwise, it 
will unlock the vm root bo reservation that is not what we want here I think.
if (unlock_resv) //need this condition here
reservation_object_unlock(bo->resv);

return 0;
}

 
@@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device 
*bdev, bool remove_all)
}
 
if (!ret)
-   ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
-!remove_all);
+   ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
+ true);
else
spin_unlock(>lru_lock);
 
@@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
kref_get(>list_kref);
 
if (!list_empty(>ddestroy)) {
-   ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
-no_wait_gpu);
+ 

RE: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
   bool interruptible, bool no_wait_gpu,
   bool unlock_resv)
{
..
ttm_bo_del_from_lru(bo);
list_del_init(>ddestroy);
kref_put(>list_kref, ttm_bo_ref_bug);

spin_unlock(>lru_lock);
ttm_bo_cleanup_memtype_use(bo);

if (unlock_resv)
  //[Roger]: not sure we should add condition here as well. If not, for 
per-vm-bo, eviction would unlock the VM root BO resv obj which is not we want I 
think.
reservation_object_unlock(bo->resv);

return 0;
}


Thanks
Roger(Hongbo.He)

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Thursday, November 09, 2017 4:59 PM
To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; He, Roger 
<hongbo...@amd.com>; mic...@daenzer.net
Subject: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

Needed for the next patch.

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 52 
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
6f5d18366e6e..50a678b504f3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct 
ttm_buffer_object *bo)  }
 
 /**
- * function ttm_bo_cleanup_refs_and_unlock
+ * function ttm_bo_cleanup_refs
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
  * Must be called with lru_lock and reservation held, this function
- * will drop both before returning.
+ * will drop the lru lock and optionally the reservation lock before returning.
  *
  * @interruptible Any sleeps should occur interruptibly.
  * @no_wait_gpu   Never wait for gpu. Return -EBUSY instead.
+ * @unlock_resv   Unlock the reservation lock as well.
  */
 
-static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
- bool interruptible,
- bool no_wait_gpu)
+static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
+  bool interruptible, bool no_wait_gpu,
+  bool unlock_resv)
 {
struct ttm_bo_global *glob = bo->glob;
struct reservation_object *resv;
@@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct 
ttm_buffer_object *bo,
if (ret && !no_wait_gpu) {
long lret;
 
-   reservation_object_unlock(bo->resv);
+   if (unlock_resv)
+   reservation_object_unlock(bo->resv);
spin_unlock(>lru_lock);
 
lret = reservation_object_wait_timeout_rcu(resv, true, @@ 
-531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct 
ttm_buffer_object *bo,
return -EBUSY;
 
spin_lock(>lru_lock);
-   ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
-
-   /*
-* We raced, and lost, someone else holds the reservation now,
-* and is probably busy in ttm_bo_cleanup_memtype_use.
-*
-* Even if it's not the case, because we finished waiting any
-* delayed destruction would succeed, so just return success
-* here.
-*/
-   if (ret) {
-   spin_unlock(>lru_lock);
-   return 0;
+   if (unlock_resv) {
+   ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
+   /*
+* We raced, and lost, someone else holds the 
reservation now,
+* and is probably busy in ttm_bo_cleanup_memtype_use.
+*
+* Even if it's not the case, because we finished 
waiting any
+* delayed destruction would succeed, so just return 
success
+* here.
+*/
+   if (ret) {
+   spin_unlock(>lru_lock);
+   return 0;
+   }
+   } else {
+   ret = 0;
}
}
 
@@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device 
*bdev, bool remove_all)
}
 
if (!ret)
-   ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
-!remove_all);
+   

Recall: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

He, Roger would like to recall the message, "[PATCH 3/4] drm/ttm: make 
unlocking in ttm_bo_cleanup_refs optional".
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Recall: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

He, Roger would like to recall the message, "[PATCH 3/4] drm/ttm: make 
unlocking in ttm_bo_cleanup_refs optional".
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Recall: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

He, Roger would like to recall the message, "[PATCH 3/4] drm/ttm: make 
unlocking in ttm_bo_cleanup_refs optional".
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/ttm: set bo->resv point to tbo->ttm_resv after individualize_resv

[He, Roger]

I guess this because you move the resv changing out of lock of bo->resv.
Because at the beginning ttm_mem_evict_first may __ttm_bo_reserve(bo->resv) 
success, and then bo->resv has been changed by another thread. That is not 
matched and at this point bo->ttm_resv also may been freed already.

 And I think it is not easy to put it out of two lock of bo->resv and 
bo->ttm_resv.

Thanks
Roger(Hongbo.He)
-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Michel D?nzer
Sent: Wednesday, November 08, 2017 12:16 AM
To: Christian König 
Cc: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
Subject: Re: [PATCH] drm/ttm: set bo->resv point to tbo->ttm_resv after 
individualize_resv

On 07/11/17 02:44 PM, Christian König wrote:
> Set bo->resv to ttm_resv during BO cleanup. This way freed BOs can be 
> better reaped during eviction.
> 
> Signed-off-by: Roger He 
> Signed-off-by: Christian König 

KASAN caught some badness while running piglit with this applied, see the 
attached dmesg excerpts.


At least some of this might be pre-existing bugs being exposed by this change. 
E.g. I've been chasing another use-after-free, with ttm_bo_delayed_delete 
trying to reserve a BO which has already been destroyed. Looks like maybe the 
ddestroy list handling isn't quite watertight yet.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel