Re: [PATCH] drm/amd/amdgpu: Code comments for the amdgpu_ttm.c driver. (v2)

2018-05-18 Thread Tom St Denis 



On 05/18/2018 05:52 AM, Christian König wrote:

Am 17.05.2018 um 17:34 schrieb Alex Deucher:
On Tue, May 15, 2018 at 10:02 AM, Tom St Denis  
wrote:

NFC just comments.

(v2):  Updated based on feedback from Alex Deucher.

Signed-off-by: Tom St Denis 

Reviewed-by: Alex Deucher 


Just one comment "Pin pages of memory pointed to..." better write "Grab 
a reference to the memory pointed to...".


get_user_pages() does not pin anything! I've heard that misconception so 
many times now that I can't remember how often we had to explain it and 
we should definitely not leak it into the documentation.


With that fixed the patch is Reviewed-by: Christian König 
.


Hi Christian,

Well it's been pushed but as per Alex's email last night I'll swing back 
to add Sphinx style comments to the file and fix that in the same patch.


Thanks,
tom




Regards,
Christian.




---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 347 
+++-

  1 file changed, 340 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

index dfd22db13fb1..2eaaa1fb7b59 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -63,16 +63,44 @@ static void amdgpu_ttm_debugfs_fini(struct 
amdgpu_device *adev);

  /*
   * Global memory.
   */
+
+/**
+ * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
+ * memory object
+ *
+ * @ref: Object for initialization.
+ *
+ * This is called by drm_global_item_ref() when an object is being
+ * initialized.
+ */
  static int amdgpu_ttm_mem_global_init(struct drm_global_reference 
*ref)

  {
 return ttm_mem_global_init(ref->object);
  }

+/**
+ * amdgpu_ttm_mem_global_release - Drop reference to a memory object
+ *
+ * @ref: Object being removed
+ *
+ * This is called by drm_global_item_unref() when an object is being
+ * released.
+ */
  static void amdgpu_ttm_mem_global_release(struct 
drm_global_reference *ref)

  {
 ttm_mem_global_release(ref->object);
  }

+/**
+ * amdgpu_ttm_global_init - Initialize global TTM memory reference
+ * structures.
+ *
+ * @adev:  AMDGPU device for which the global structures need to be
+ * registered.
+ *
+ * This is called as part of the AMDGPU ttm init from amdgpu_ttm_init()
+ * during bring up.
+ */
  static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
  {
 struct drm_global_reference *global_ref;
@@ -80,7 +108,9 @@ static int amdgpu_ttm_global_init(struct 
amdgpu_device *adev)

 struct drm_sched_rq *rq;
 int r;

+   /* ensure reference is false in case init fails */
 adev->mman.mem_global_referenced = false;
+
 global_ref = >mman.mem_global_ref;
 global_ref->global_type = DRM_GLOBAL_TTM_MEM;
 global_ref->size = sizeof(struct ttm_mem_global);
@@ -146,6 +176,18 @@ static int amdgpu_invalidate_caches(struct 
ttm_bo_device *bdev, uint32_t flags)

 return 0;
  }

+/**
+ * amdgpu_init_mem_type -  Initialize a memory manager for a 
specific
+ * type of 
memory request.

+ *
+ * @bdev:  The TTM BO device object (contains a reference to
+ * amdgpu_device)
+ * @type:  The type of memory requested
+ * @man:
+ *
+ * This is called by ttm_bo_init_mm() when a buffer object is being
+ * initialized.
+ */
  static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, 
uint32_t type,

 struct ttm_mem_type_manager *man)
  {
@@ -161,6 +203,7 @@ static int amdgpu_init_mem_type(struct 
ttm_bo_device *bdev, uint32_t type,

 man->default_caching = TTM_PL_FLAG_CACHED;
 break;
 case TTM_PL_TT:
+   /* GTT memory  */
 man->func = _gtt_mgr_func;
 man->gpu_offset = adev->gmc.gart_start;
 man->available_caching = TTM_PL_MASK_CACHING;
@@ -193,6 +236,14 @@ static int amdgpu_init_mem_type(struct 
ttm_bo_device *bdev, uint32_t type,

 return 0;
  }

+/**
+ * amdgpu_evict_flags - Compute placement flags
+ *
+ * @bo: The buffer object to evict
+ * @placement: Possible destination(s) for evicted BO
+ *
+ * Fill in placement data when ttm_bo_evict() is called
+ */
  static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
 struct ttm_placement *placement)
  {
@@ -204,12 +255,14 @@ static void amdgpu_evict_flags(struct 
ttm_buffer_object *bo,

 .flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM
 };

+   /* Don't handle scatter gather BOs */
 if (bo->type == ttm_bo_type_sg) {
 placement->num_placement = 0;
 placement->num_busy_placement = 0;
 return;

Re: [PATCH] drm/amd/amdgpu: Code comments for the amdgpu_ttm.c driver. (v2)

2018-05-18 Thread Christian König 

Am 17.05.2018 um 17:34 schrieb Alex Deucher:

On Tue, May 15, 2018 at 10:02 AM, Tom St Denis  wrote:

NFC just comments.

(v2):  Updated based on feedback from Alex Deucher.

Signed-off-by: Tom St Denis 

Reviewed-by: Alex Deucher 


Just one comment "Pin pages of memory pointed to..." better write "Grab 
a reference to the memory pointed to...".


get_user_pages() does not pin anything! I've heard that misconception so 
many times now that I can't remember how often we had to explain it and 
we should definitely not leak it into the documentation.


With that fixed the patch is Reviewed-by: Christian König 
.


Regards,
Christian.




---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 347 +++-
  1 file changed, 340 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dfd22db13fb1..2eaaa1fb7b59 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -63,16 +63,44 @@ static void amdgpu_ttm_debugfs_fini(struct amdgpu_device 
*adev);
  /*
   * Global memory.
   */
+
+/**
+ * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
+ * memory object
+ *
+ * @ref: Object for initialization.
+ *
+ * This is called by drm_global_item_ref() when an object is being
+ * initialized.
+ */
  static int amdgpu_ttm_mem_global_init(struct drm_global_reference *ref)
  {
 return ttm_mem_global_init(ref->object);
  }

+/**
+ * amdgpu_ttm_mem_global_release - Drop reference to a memory object
+ *
+ * @ref: Object being removed
+ *
+ * This is called by drm_global_item_unref() when an object is being
+ * released.
+ */
  static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
  {
 ttm_mem_global_release(ref->object);
  }

+/**
+ * amdgpu_ttm_global_init - Initialize global TTM memory reference
+ * structures.
+ *
+ * @adev:  AMDGPU device for which the global structures need to be
+ * registered.
+ *
+ * This is called as part of the AMDGPU ttm init from amdgpu_ttm_init()
+ * during bring up.
+ */
  static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
  {
 struct drm_global_reference *global_ref;
@@ -80,7 +108,9 @@ static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
 struct drm_sched_rq *rq;
 int r;

+   /* ensure reference is false in case init fails */
 adev->mman.mem_global_referenced = false;
+
 global_ref = >mman.mem_global_ref;
 global_ref->global_type = DRM_GLOBAL_TTM_MEM;
 global_ref->size = sizeof(struct ttm_mem_global);
@@ -146,6 +176,18 @@ static int amdgpu_invalidate_caches(struct ttm_bo_device 
*bdev, uint32_t flags)
 return 0;
  }

+/**
+ * amdgpu_init_mem_type -  Initialize a memory manager for a specific
+ * type of memory request.
+ *
+ * @bdev:  The TTM BO device object (contains a reference to
+ * amdgpu_device)
+ * @type:  The type of memory requested
+ * @man:
+ *
+ * This is called by ttm_bo_init_mm() when a buffer object is being
+ * initialized.
+ */
  static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 struct ttm_mem_type_manager *man)
  {
@@ -161,6 +203,7 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, 
uint32_t type,
 man->default_caching = TTM_PL_FLAG_CACHED;
 break;
 case TTM_PL_TT:
+   /* GTT memory  */
 man->func = _gtt_mgr_func;
 man->gpu_offset = adev->gmc.gart_start;
 man->available_caching = TTM_PL_MASK_CACHING;
@@ -193,6 +236,14 @@ static int amdgpu_init_mem_type(struct ttm_bo_device 
*bdev, uint32_t type,
 return 0;
  }

+/**
+ * amdgpu_evict_flags - Compute placement flags
+ *
+ * @bo: The buffer object to evict
+ * @placement: Possible destination(s) for evicted BO
+ *
+ * Fill in placement data when ttm_bo_evict() is called
+ */
  static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
 struct ttm_placement *placement)
  {
@@ -204,12 +255,14 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
 .flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM
 };

+   /* Don't handle scatter gather BOs */
 if (bo->type == ttm_bo_type_sg) {
 placement->num_placement = 0;
 placement->num_busy_placement = 0;
 return;
 }

+   /* Object isn't an AMDGPU object so ignore */
 if (!amdgpu_ttm_bo_is_amdgpu_bo(bo)) {
 placement->placement = 
 placement->busy_placement = 
@@ -217,10 +270,12 @@ static void

Re: [PATCH] drm/amd/amdgpu: Code comments for the amdgpu_ttm.c driver. (v2)

2018-05-17 Thread Alex Deucher 
On Tue, May 15, 2018 at 10:02 AM, Tom St Denis  wrote:
> NFC just comments.
>
> (v2):  Updated based on feedback from Alex Deucher.
>
> Signed-off-by: Tom St Denis 

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 347 
> +++-
>  1 file changed, 340 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index dfd22db13fb1..2eaaa1fb7b59 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -63,16 +63,44 @@ static void amdgpu_ttm_debugfs_fini(struct amdgpu_device 
> *adev);
>  /*
>   * Global memory.
>   */
> +
> +/**
> + * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
> + * memory object
> + *
> + * @ref: Object for initialization.
> + *
> + * This is called by drm_global_item_ref() when an object is being
> + * initialized.
> + */
>  static int amdgpu_ttm_mem_global_init(struct drm_global_reference *ref)
>  {
> return ttm_mem_global_init(ref->object);
>  }
>
> +/**
> + * amdgpu_ttm_mem_global_release - Drop reference to a memory object
> + *
> + * @ref: Object being removed
> + *
> + * This is called by drm_global_item_unref() when an object is being
> + * released.
> + */
>  static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
>  {
> ttm_mem_global_release(ref->object);
>  }
>
> +/**
> + * amdgpu_ttm_global_init - Initialize global TTM memory reference
> + * structures.
> + *
> + * @adev:  AMDGPU device for which the global structures need to be
> + * registered.
> + *
> + * This is called as part of the AMDGPU ttm init from amdgpu_ttm_init()
> + * during bring up.
> + */
>  static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
>  {
> struct drm_global_reference *global_ref;
> @@ -80,7 +108,9 @@ static int amdgpu_ttm_global_init(struct amdgpu_device 
> *adev)
> struct drm_sched_rq *rq;
> int r;
>
> +   /* ensure reference is false in case init fails */
> adev->mman.mem_global_referenced = false;
> +
> global_ref = >mman.mem_global_ref;
> global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> global_ref->size = sizeof(struct ttm_mem_global);
> @@ -146,6 +176,18 @@ static int amdgpu_invalidate_caches(struct ttm_bo_device 
> *bdev, uint32_t flags)
> return 0;
>  }
>
> +/**
> + * amdgpu_init_mem_type -  Initialize a memory manager for a specific
> + * type of memory 
> request.
> + *
> + * @bdev:  The TTM BO device object (contains a reference to
> + * amdgpu_device)
> + * @type:  The type of memory requested
> + * @man:
> + *
> + * This is called by ttm_bo_init_mm() when a buffer object is being
> + * initialized.
> + */
>  static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> struct ttm_mem_type_manager *man)
>  {
> @@ -161,6 +203,7 @@ static int amdgpu_init_mem_type(struct ttm_bo_device 
> *bdev, uint32_t type,
> man->default_caching = TTM_PL_FLAG_CACHED;
> break;
> case TTM_PL_TT:
> +   /* GTT memory  */
> man->func = _gtt_mgr_func;
> man->gpu_offset = adev->gmc.gart_start;
> man->available_caching = TTM_PL_MASK_CACHING;
> @@ -193,6 +236,14 @@ static int amdgpu_init_mem_type(struct ttm_bo_device 
> *bdev, uint32_t type,
> return 0;
>  }
>
> +/**
> + * amdgpu_evict_flags - Compute placement flags
> + *
> + * @bo: The buffer object to evict
> + * @placement: Possible destination(s) for evicted BO
> + *
> + * Fill in placement data when ttm_bo_evict() is called
> + */
>  static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
> struct ttm_placement *placement)
>  {
> @@ -204,12 +255,14 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
> *bo,
> .flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM
> };
>
> +   /* Don't handle scatter gather BOs */
> if (bo->type == ttm_bo_type_sg) {
> placement->num_placement = 0;
> placement->num_busy_placement = 0;
> return;
> }
>
> +   /* Object isn't an AMDGPU object so ignore */
> if (!amdgpu_ttm_bo_is_amdgpu_bo(bo)) {
> placement->placement = 
> placement->busy_placement = 
> @@ -217,10 +270,12 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
> *bo,
> placement->num_busy_placement = 1;
> return;
> }
> +
> abo = ttm_to_amdgpu_bo(bo);
> switch (bo->mem.mem_type) {
> case TTM_PL_VRAM:
> if

[PATCH] drm/amd/amdgpu: Code comments for the amdgpu_ttm.c driver. (v2)

2018-05-15 Thread Tom St Denis 
NFC just comments.

(v2):  Updated based on feedback from Alex Deucher.

Signed-off-by: Tom St Denis 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 347 +++-
 1 file changed, 340 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dfd22db13fb1..2eaaa1fb7b59 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -63,16 +63,44 @@ static void amdgpu_ttm_debugfs_fini(struct amdgpu_device 
*adev);
 /*
  * Global memory.
  */
+
+/**
+ * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
+ * memory object
+ *
+ * @ref: Object for initialization.
+ *
+ * This is called by drm_global_item_ref() when an object is being
+ * initialized.
+ */
 static int amdgpu_ttm_mem_global_init(struct drm_global_reference *ref)
 {
return ttm_mem_global_init(ref->object);
 }
 
+/**
+ * amdgpu_ttm_mem_global_release - Drop reference to a memory object
+ *
+ * @ref: Object being removed
+ *
+ * This is called by drm_global_item_unref() when an object is being
+ * released.
+ */
 static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
 {
ttm_mem_global_release(ref->object);
 }
 
+/**
+ * amdgpu_ttm_global_init - Initialize global TTM memory reference
+ * structures.
+ *
+ * @adev:  AMDGPU device for which the global structures need to be
+ * registered.
+ *
+ * This is called as part of the AMDGPU ttm init from amdgpu_ttm_init()
+ * during bring up.
+ */
 static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
 {
struct drm_global_reference *global_ref;
@@ -80,7 +108,9 @@ static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
struct drm_sched_rq *rq;
int r;
 
+   /* ensure reference is false in case init fails */
adev->mman.mem_global_referenced = false;
+
global_ref = >mman.mem_global_ref;
global_ref->global_type = DRM_GLOBAL_TTM_MEM;
global_ref->size = sizeof(struct ttm_mem_global);
@@ -146,6 +176,18 @@ static int amdgpu_invalidate_caches(struct ttm_bo_device 
*bdev, uint32_t flags)
return 0;
 }
 
+/**
+ * amdgpu_init_mem_type -  Initialize a memory manager for a specific
+ * type of memory request.
+ *
+ * @bdev:  The TTM BO device object (contains a reference to
+ * amdgpu_device)
+ * @type:  The type of memory requested
+ * @man:
+ *
+ * This is called by ttm_bo_init_mm() when a buffer object is being
+ * initialized.
+ */
 static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
struct ttm_mem_type_manager *man)
 {
@@ -161,6 +203,7 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, 
uint32_t type,
man->default_caching = TTM_PL_FLAG_CACHED;
break;
case TTM_PL_TT:
+   /* GTT memory  */
man->func = _gtt_mgr_func;
man->gpu_offset = adev->gmc.gart_start;
man->available_caching = TTM_PL_MASK_CACHING;
@@ -193,6 +236,14 @@ static int amdgpu_init_mem_type(struct ttm_bo_device 
*bdev, uint32_t type,
return 0;
 }
 
+/**
+ * amdgpu_evict_flags - Compute placement flags
+ *
+ * @bo: The buffer object to evict
+ * @placement: Possible destination(s) for evicted BO
+ *
+ * Fill in placement data when ttm_bo_evict() is called
+ */
 static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
struct ttm_placement *placement)
 {
@@ -204,12 +255,14 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
.flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM
};
 
+   /* Don't handle scatter gather BOs */
if (bo->type == ttm_bo_type_sg) {
placement->num_placement = 0;
placement->num_busy_placement = 0;
return;
}
 
+   /* Object isn't an AMDGPU object so ignore */
if (!amdgpu_ttm_bo_is_amdgpu_bo(bo)) {
placement->placement = 
placement->busy_placement = 
@@ -217,10 +270,12 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
placement->num_busy_placement = 1;
return;
}
+
abo = ttm_to_amdgpu_bo(bo);
switch (bo->mem.mem_type) {
case TTM_PL_VRAM:
if (!adev->mman.buffer_funcs_enabled) {
+   /* Move to system memory */
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_CPU);
} else if (adev->gmc.visible_vram_size < 
adev->gmc.real_vram_size &&
   !(abo->flags & 
AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) &&
@@ -238,6 +293,7 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,