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

2017-12-21 Thread Thomas Hellstrom 

On 12/21/2017 07:05 AM, He, Roger wrote:


-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


Yes, I agree.

/Thomas


___
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

2017-12-20 Thread 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/7] drm/ttm: use an operation ctx for ttm_mem_global_alloc_page

2017-12-20 Thread Christian König 

Commit message!

Am 20.12.2017 um 11:34 schrieb Roger He:

Change-Id: I4104a12e09a374b6477a0dd5a8fce26dce27a746
Signed-off-by: Roger He 
---
  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.


Christian.

  
+	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_unpopulate(ttm_dma, dev);
return -ENOMEM;
@@ -998,7 +1002,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);
+   pool->size, );
if (unlikely(ret != 0)) {
ttm_dma_unpopulate(ttm_dma, dev);
return -ENOMEM;
diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
index 755c107..8936285 100644
--- a/include/drm/ttm/ttm_memory.h
+++ b/include/drm/ttm/ttm_memory.h
@@ -84,7 +84,8 @@ extern int ttm_mem_global_alloc(struct ttm_mem_global *glob, 
uint64_t memory,
  extern void ttm_mem_global_free(struct ttm_mem_global *glob,
uint64_t amount);
  extern int ttm_mem_global_alloc_page(struct ttm_mem_global