[PATCH 1/5] drm/amdgpu: fix cgs alignment handling

[PATCH 1/5] drm/amdgpu: fix cgs alignment handling

[Christian König]

From: Christian König <christian.koenig@amd.com>

This always allocated on PAGE_SIZE alignment.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
index d450a96..8b5fa22 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
@@ -121,7 +121,7 @@ static int amdgpu_cgs_alloc_gpu_mem(struct cgs_device *cgs_device,
 	placement.busy_placement = &place;
 	placement.num_busy_placement = 1;
 
-	ret = amdgpu_bo_create_restricted(adev, size, PAGE_SIZE,
+	ret = amdgpu_bo_create_restricted(adev, size, align,
 					  true, domain, flags,
 					  NULL, &placement, NULL,
 					  0, &obj);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/5] drm/amd: remove min/max addr handling from cgs

[Christian König]

From: Christian König <christian.koenig@amd.com>

Nobody is actually using this and it causes a bunch of unused and buggy code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c       | 48 ++-------------------------
 drivers/gpu/drm/amd/include/cgs_common.h      |  7 ++--
 drivers/gpu/drm/amd/powerplay/smumgr/smumgr.c |  2 +-
 3 files changed, 6 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
index 8b5fa22..53d1591 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
@@ -45,7 +45,6 @@ struct amdgpu_cgs_device {
 static int amdgpu_cgs_alloc_gpu_mem(struct cgs_device *cgs_device,
 				    enum cgs_gpu_mem_type type,
 				    uint64_t size, uint64_t align,
-				    uint64_t min_offset, uint64_t max_offset,
 				    cgs_handle_t *handle)
 {
 	CGS_FUNC_ADEV;
@@ -53,13 +52,6 @@ static int amdgpu_cgs_alloc_gpu_mem(struct cgs_device *cgs_device,
 	int ret = 0;
 	uint32_t domain = 0;
 	struct amdgpu_bo *obj;
-	struct ttm_placement placement;
-	struct ttm_place place;
-
-	if (min_offset > max_offset) {
-		BUG_ON(1);
-		return -EINVAL;
-	}
 
 	/* fail if the alignment is not a power of 2 */
 	if (((align != 1) && (align & (align - 1)))
@@ -73,41 +65,19 @@ static int amdgpu_cgs_alloc_gpu_mem(struct cgs_device *cgs_device,
 		flags = AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
 			AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
 		domain = AMDGPU_GEM_DOMAIN_VRAM;
-		if (max_offset > adev->mc.real_vram_size)
-			return -EINVAL;
-		place.fpfn = min_offset >> PAGE_SHIFT;
-		place.lpfn = max_offset >> PAGE_SHIFT;
-		place.flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
-			TTM_PL_FLAG_VRAM;
 		break;
 	case CGS_GPU_MEM_TYPE__INVISIBLE_CONTIG_FB:
 	case CGS_GPU_MEM_TYPE__INVISIBLE_FB:
 		flags = AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
 			AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
 		domain = AMDGPU_GEM_DOMAIN_VRAM;
-		if (adev->mc.visible_vram_size < adev->mc.real_vram_size) {
-			place.fpfn =
-				max(min_offset, adev->mc.visible_vram_size) >> PAGE_SHIFT;
-			place.lpfn =
-				min(max_offset, adev->mc.real_vram_size) >> PAGE_SHIFT;
-			place.flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
-				TTM_PL_FLAG_VRAM;
-		}
-
 		break;
 	case CGS_GPU_MEM_TYPE__GART_CACHEABLE:
 		domain = AMDGPU_GEM_DOMAIN_GTT;
-		place.fpfn = min_offset >> PAGE_SHIFT;
-		place.lpfn = max_offset >> PAGE_SHIFT;
-		place.flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_TT;
 		break;
 	case CGS_GPU_MEM_TYPE__GART_WRITECOMBINE:
 		flags = AMDGPU_GEM_CREATE_CPU_GTT_USWC;
 		domain = AMDGPU_GEM_DOMAIN_GTT;
-		place.fpfn = min_offset >> PAGE_SHIFT;
-		place.lpfn = max_offset >> PAGE_SHIFT;
-		place.flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_TT |
-			TTM_PL_FLAG_UNCACHED;
 		break;
 	default:
 		return -EINVAL;
@@ -116,15 +86,8 @@ static int amdgpu_cgs_alloc_gpu_mem(struct cgs_device *cgs_device,
 
 	*handle = 0;
 
-	placement.placement = &place;
-	placement.num_placement = 1;
-	placement.busy_placement = &place;
-	placement.num_busy_placement = 1;
-
-	ret = amdgpu_bo_create_restricted(adev, size, align,
-					  true, domain, flags,
-					  NULL, &placement, NULL,
-					  0, &obj);
+	ret = amdgpu_bo_create(adev, size, align, true, domain, flags,
+			       NULL, NULL, 0, &obj);
 	if (ret) {
 		DRM_ERROR("(%d) bo create failed\n", ret);
 		return ret;
@@ -155,19 +118,14 @@ static int amdgpu_cgs_gmap_gpu_mem(struct cgs_device *cgs_device, cgs_handle_t h
 				   uint64_t *mcaddr)
 {
 	int r;
-	u64 min_offset, max_offset;
 	struct amdgpu_bo *obj = (struct amdgpu_bo *)handle;
 
 	WARN_ON_ONCE(obj->placement.num_placement > 1);
 
-	min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
-	max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
-
 	r = amdgpu_bo_reserve(obj, true);
 	if (unlikely(r != 0))
 		return r;
-	r = amdgpu_bo_pin_restricted(obj, obj->preferred_domains,
-				     min_offset, max_offset, mcaddr);
+	r = amdgpu_bo_pin(obj, obj->preferred_domains, mcaddr);
 	amdgpu_bo_unreserve(obj);
 	return r;
 }
diff --git a/drivers/gpu/drm/amd/include/cgs_common.h b/drivers/gpu/drm/amd/include/cgs_common.h
index 92eaa81..2c1f13e 100644
--- a/drivers/gpu/drm/amd/include/cgs_common.h
+++ b/drivers/gpu/drm/amd/include/cgs_common.h
@@ -193,8 +193,6 @@ struct cgs_acpi_method_info {
  * @type:	memory type
  * @size:	size in bytes
  * @align:	alignment in bytes
- * @min_offset: minimum offset from start of heap
- * @max_offset: maximum offset from start of heap
  * @handle:	memory handle (output)
  *
  * The memory types CGS_GPU_MEM_TYPE_*_CONTIG_FB force contiguous
@@ -216,7 +214,6 @@ struct cgs_acpi_method_info {
  */
 typedef int (*cgs_alloc_gpu_mem_t)(struct cgs_device *cgs_device, enum cgs_gpu_mem_type type,
 				   uint64_t size, uint64_t align,
-				   uint64_t min_offset, uint64_t max_offset,
 				   cgs_handle_t *handle);
 
 /**
@@ -479,8 +476,8 @@ struct cgs_device
 #define CGS_OS_CALL(func,dev,...) \
 	(((struct cgs_device *)dev)->os_ops->func(dev, ##__VA_ARGS__))
 
-#define cgs_alloc_gpu_mem(dev,type,size,align,min_off,max_off,handle)	\
-	CGS_CALL(alloc_gpu_mem,dev,type,size,align,min_off,max_off,handle)
+#define cgs_alloc_gpu_mem(dev,type,size,align,handle)	\
+	CGS_CALL(alloc_gpu_mem,dev,type,size,align,handle)
 #define cgs_free_gpu_mem(dev,handle)		\
 	CGS_CALL(free_gpu_mem,dev,handle)
 #define cgs_gmap_gpu_mem(dev,handle,mcaddr)	\
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smumgr.c
index f62c875..c7a1cc7 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smumgr.c
@@ -315,7 +315,7 @@ int smu_allocate_memory(void *device, uint32_t size,
 		return -EINVAL;
 
 	ret = cgs_alloc_gpu_mem(device, type, size, byte_align,
-				0, 0, (cgs_handle_t *)handle);
+				(cgs_handle_t *)handle);
 	if (ret)
 		return -ENOMEM;
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create

[Christian König]

From: Christian König <christian.koenig@amd.com>

Fix USWC handling by cleaning up the function and removing
quite a bit of unused code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 83 +++++++++---------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  8 ---
 2 files changed, 23 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 52d0109..726a662 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -64,11 +64,12 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object *bo)
 	return false;
 }
 
-static void amdgpu_ttm_placement_init(struct amdgpu_device *adev,
-				      struct ttm_placement *placement,
-				      struct ttm_place *places,
-				      u32 domain, u64 flags)
+void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
 {
+	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
+	struct ttm_placement *placement = &abo->placement;
+	struct ttm_place *places = abo->placements;
+	u64 flags = abo->flags;
 	u32 c = 0;
 
 	if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
@@ -151,27 +152,6 @@ static void amdgpu_ttm_placement_init(struct amdgpu_device *adev,
 	placement->busy_placement = places;
 }
 
-void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
-{
-	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
-
-	amdgpu_ttm_placement_init(adev, &abo->placement, abo->placements,
-				  domain, abo->flags);
-}
-
-static void amdgpu_fill_placement_to_bo(struct amdgpu_bo *bo,
-					struct ttm_placement *placement)
-{
-	BUG_ON(placement->num_placement > (AMDGPU_GEM_DOMAIN_MAX + 1));
-
-	memcpy(bo->placements, placement->placement,
-	       placement->num_placement * sizeof(struct ttm_place));
-	bo->placement.num_placement = placement->num_placement;
-	bo->placement.num_busy_placement = placement->num_busy_placement;
-	bo->placement.placement = bo->placements;
-	bo->placement.busy_placement = bo->placements;
-}
-
 /**
  * amdgpu_bo_create_reserved - create reserved BO for kernel use
  *
@@ -303,14 +283,13 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
 		*cpu_addr = NULL;
 }
 
-int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
-				unsigned long size, int byte_align,
-				bool kernel, u32 domain, u64 flags,
-				struct sg_table *sg,
-				struct ttm_placement *placement,
-				struct reservation_object *resv,
-				uint64_t init_value,
-				struct amdgpu_bo **bo_ptr)
+static int amdgpu_bo_do_create(struct amdgpu_device *adev,
+			       unsigned long size, int byte_align,
+			       bool kernel, u32 domain, u64 flags,
+			       struct sg_table *sg,
+			       struct reservation_object *resv,
+			       uint64_t init_value,
+			       struct amdgpu_bo **bo_ptr)
 {
 	struct amdgpu_bo *bo;
 	enum ttm_bo_type type;
@@ -384,10 +363,11 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
 #endif
 
-	amdgpu_fill_placement_to_bo(bo, placement);
-	/* Kernel allocation are uninterruptible */
+	bo->tbo.bdev = &adev->mman.bdev;
+	amdgpu_ttm_placement_from_domain(bo, domain);
 
 	initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
+	/* Kernel allocation are uninterruptible */
 	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
 				 &bo->placement, page_align, !kernel, NULL,
 				 acc_size, sg, resv, &amdgpu_ttm_bo_destroy);
@@ -442,27 +422,17 @@ static int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
 				   unsigned long size, int byte_align,
 				   struct amdgpu_bo *bo)
 {
-	struct ttm_placement placement = {0};
-	struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
 	int r;
 
 	if (bo->shadow)
 		return 0;
 
-	memset(&placements, 0, sizeof(placements));
-	amdgpu_ttm_placement_init(adev, &placement, placements,
-				  AMDGPU_GEM_DOMAIN_GTT,
-				  AMDGPU_GEM_CREATE_CPU_GTT_USWC |
-				  AMDGPU_GEM_CREATE_SHADOW);
-
-	r = amdgpu_bo_create_restricted(adev, size, byte_align, true,
-					AMDGPU_GEM_DOMAIN_GTT,
-					AMDGPU_GEM_CREATE_CPU_GTT_USWC |
-					AMDGPU_GEM_CREATE_SHADOW,
-					NULL, &placement,
-					bo->tbo.resv,
-					0,
-					&bo->shadow);
+	r = amdgpu_bo_do_create(adev, size, byte_align, true,
+				AMDGPU_GEM_DOMAIN_GTT,
+				AMDGPU_GEM_CREATE_CPU_GTT_USWC |
+				AMDGPU_GEM_CREATE_SHADOW,
+				NULL, bo->tbo.resv, 0,
+				&bo->shadow);
 	if (!r) {
 		bo->shadow->parent = amdgpu_bo_ref(bo);
 		mutex_lock(&adev->shadow_list_lock);
@@ -484,18 +454,11 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
 		     uint64_t init_value,
 		     struct amdgpu_bo **bo_ptr)
 {
-	struct ttm_placement placement = {0};
-	struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
 	uint64_t parent_flags = flags & ~AMDGPU_GEM_CREATE_SHADOW;
 	int r;
 
-	memset(&placements, 0, sizeof(placements));
-	amdgpu_ttm_placement_init(adev, &placement, placements,
-				  domain, parent_flags);
-
-	r = amdgpu_bo_create_restricted(adev, size, byte_align, kernel, domain,
-					parent_flags, sg, &placement, resv,
-					init_value, bo_ptr);
+	r = amdgpu_bo_do_create(adev, size, byte_align, kernel, domain,
+				parent_flags, sg, resv, init_value, bo_ptr);
 	if (r)
 		return r;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index a4891be..39b6bf6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -195,14 +195,6 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
 			    struct reservation_object *resv,
 			    uint64_t init_value,
 			    struct amdgpu_bo **bo_ptr);
-int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
-				unsigned long size, int byte_align,
-				bool kernel, u32 domain, u64 flags,
-				struct sg_table *sg,
-				struct ttm_placement *placement,
-			        struct reservation_object *resv,
-				uint64_t init_value,
-				struct amdgpu_bo **bo_ptr);
 int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
 			      unsigned long size, int align,
 			      u32 domain, struct amdgpu_bo **bo_ptr,
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Tuesday, September 12, 2017 5:09 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create
> 
> From: Christian König <christian.koenig@amd.com>
> 
> Fix USWC handling by cleaning up the function and removing
> quite a bit of unused code.

Can you clarify what was broken?

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 83 +++++++++------------
> ---------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  8 ---
>  2 files changed, 23 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 52d0109..726a662 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -64,11 +64,12 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct
> ttm_buffer_object *bo)
>  	return false;
>  }
> 
> -static void amdgpu_ttm_placement_init(struct amdgpu_device *adev,
> -				      struct ttm_placement *placement,
> -				      struct ttm_place *places,
> -				      u32 domain, u64 flags)
> +void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
> domain)
>  {
> +	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
> +	struct ttm_placement *placement = &abo->placement;
> +	struct ttm_place *places = abo->placements;
> +	u64 flags = abo->flags;
>  	u32 c = 0;
> 
>  	if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
> @@ -151,27 +152,6 @@ static void amdgpu_ttm_placement_init(struct
> amdgpu_device *adev,
>  	placement->busy_placement = places;
>  }
> 
> -void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
> domain)
> -{
> -	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
> -
> -	amdgpu_ttm_placement_init(adev, &abo->placement, abo-
> >placements,
> -				  domain, abo->flags);
> -}
> -
> -static void amdgpu_fill_placement_to_bo(struct amdgpu_bo *bo,
> -					struct ttm_placement *placement)
> -{
> -	BUG_ON(placement->num_placement >
> (AMDGPU_GEM_DOMAIN_MAX + 1));
> -
> -	memcpy(bo->placements, placement->placement,
> -	       placement->num_placement * sizeof(struct ttm_place));
> -	bo->placement.num_placement = placement->num_placement;
> -	bo->placement.num_busy_placement = placement-
> >num_busy_placement;
> -	bo->placement.placement = bo->placements;
> -	bo->placement.busy_placement = bo->placements;
> -}
> -
>  /**
>   * amdgpu_bo_create_reserved - create reserved BO for kernel use
>   *
> @@ -303,14 +283,13 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo
> **bo, u64 *gpu_addr,
>  		*cpu_addr = NULL;
>  }
> 
> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
> -				unsigned long size, int byte_align,
> -				bool kernel, u32 domain, u64 flags,
> -				struct sg_table *sg,
> -				struct ttm_placement *placement,
> -				struct reservation_object *resv,
> -				uint64_t init_value,
> -				struct amdgpu_bo **bo_ptr)
> +static int amdgpu_bo_do_create(struct amdgpu_device *adev,
> +			       unsigned long size, int byte_align,
> +			       bool kernel, u32 domain, u64 flags,
> +			       struct sg_table *sg,
> +			       struct reservation_object *resv,
> +			       uint64_t init_value,
> +			       struct amdgpu_bo **bo_ptr)

Still seems like amdgpu_bo_create_restricted is a better name than do_create.

>  {
>  	struct amdgpu_bo *bo;
>  	enum ttm_bo_type type;
> @@ -384,10 +363,11 @@ int amdgpu_bo_create_restricted(struct
> amdgpu_device *adev,
>  		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>  #endif
> 
> -	amdgpu_fill_placement_to_bo(bo, placement);
> -	/* Kernel allocation are uninterruptible */
> +	bo->tbo.bdev = &adev->mman.bdev;
> +	amdgpu_ttm_placement_from_domain(bo, domain);
> 
>  	initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
> +	/* Kernel allocation are uninterruptible */
>  	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size,
> type,
>  				 &bo->placement, page_align, !kernel, NULL,
>  				 acc_size, sg, resv,
> &amdgpu_ttm_bo_destroy);
> @@ -442,27 +422,17 @@ static int amdgpu_bo_create_shadow(struct
> amdgpu_device *adev,
>  				   unsigned long size, int byte_align,
>  				   struct amdgpu_bo *bo)
>  {
> -	struct ttm_placement placement = {0};
> -	struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>  	int r;
> 
>  	if (bo->shadow)
>  		return 0;
> 
> -	memset(&placements, 0, sizeof(placements));
> -	amdgpu_ttm_placement_init(adev, &placement, placements,
> -				  AMDGPU_GEM_DOMAIN_GTT,
> -				  AMDGPU_GEM_CREATE_CPU_GTT_USWC
> |
> -				  AMDGPU_GEM_CREATE_SHADOW);
> -
> -	r = amdgpu_bo_create_restricted(adev, size, byte_align, true,
> -					AMDGPU_GEM_DOMAIN_GTT,
> -
> 	AMDGPU_GEM_CREATE_CPU_GTT_USWC |
> -					AMDGPU_GEM_CREATE_SHADOW,
> -					NULL, &placement,
> -					bo->tbo.resv,
> -					0,
> -					&bo->shadow);
> +	r = amdgpu_bo_do_create(adev, size, byte_align, true,
> +				AMDGPU_GEM_DOMAIN_GTT,
> +				AMDGPU_GEM_CREATE_CPU_GTT_USWC |
> +				AMDGPU_GEM_CREATE_SHADOW,
> +				NULL, bo->tbo.resv, 0,
> +				&bo->shadow);
>  	if (!r) {
>  		bo->shadow->parent = amdgpu_bo_ref(bo);
>  		mutex_lock(&adev->shadow_list_lock);
> @@ -484,18 +454,11 @@ int amdgpu_bo_create(struct amdgpu_device
> *adev,
>  		     uint64_t init_value,
>  		     struct amdgpu_bo **bo_ptr)
>  {
> -	struct ttm_placement placement = {0};
> -	struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>  	uint64_t parent_flags = flags & ~AMDGPU_GEM_CREATE_SHADOW;
>  	int r;
> 
> -	memset(&placements, 0, sizeof(placements));
> -	amdgpu_ttm_placement_init(adev, &placement, placements,
> -				  domain, parent_flags);
> -
> -	r = amdgpu_bo_create_restricted(adev, size, byte_align, kernel,
> domain,
> -					parent_flags, sg, &placement, resv,
> -					init_value, bo_ptr);
> +	r = amdgpu_bo_do_create(adev, size, byte_align, kernel, domain,
> +				parent_flags, sg, resv, init_value, bo_ptr);
>  	if (r)
>  		return r;
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index a4891be..39b6bf6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -195,14 +195,6 @@ int amdgpu_bo_create(struct amdgpu_device
> *adev,
>  			    struct reservation_object *resv,
>  			    uint64_t init_value,
>  			    struct amdgpu_bo **bo_ptr);
> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
> -				unsigned long size, int byte_align,
> -				bool kernel, u32 domain, u64 flags,
> -				struct sg_table *sg,
> -				struct ttm_placement *placement,
> -			        struct reservation_object *resv,
> -				uint64_t init_value,
> -				struct amdgpu_bo **bo_ptr);
>  int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>  			      unsigned long size, int align,
>  			      u32 domain, struct amdgpu_bo **bo_ptr,
> --
> 2.7.4
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create

[Christian König]

Am 12.09.2017 um 17:55 schrieb Deucher, Alexander:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Christian König
>> Sent: Tuesday, September 12, 2017 5:09 AM
>> To: amd-gfx@lists.freedesktop.org
>> Subject: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create
>>
>> From: Christian König <christian.koenig@amd.com>
>>
>> Fix USWC handling by cleaning up the function and removing
>> quite a bit of unused code.
> Can you clarify what was broken?

We adjusted the BO flags for USWC handling, but those never took effect 
because the placement was passed in instead of generated inside this 
function.

>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 83 +++++++++------------
>> ---------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  8 ---
>>   2 files changed, 23 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 52d0109..726a662 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -64,11 +64,12 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct
>> ttm_buffer_object *bo)
>>   	return false;
>>   }
>>
>> -static void amdgpu_ttm_placement_init(struct amdgpu_device *adev,
>> -				      struct ttm_placement *placement,
>> -				      struct ttm_place *places,
>> -				      u32 domain, u64 flags)
>> +void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
>> domain)
>>   {
>> +	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> +	struct ttm_placement *placement = &abo->placement;
>> +	struct ttm_place *places = abo->placements;
>> +	u64 flags = abo->flags;
>>   	u32 c = 0;
>>
>>   	if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
>> @@ -151,27 +152,6 @@ static void amdgpu_ttm_placement_init(struct
>> amdgpu_device *adev,
>>   	placement->busy_placement = places;
>>   }
>>
>> -void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
>> domain)
>> -{
>> -	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> -
>> -	amdgpu_ttm_placement_init(adev, &abo->placement, abo-
>>> placements,
>> -				  domain, abo->flags);
>> -}
>> -
>> -static void amdgpu_fill_placement_to_bo(struct amdgpu_bo *bo,
>> -					struct ttm_placement *placement)
>> -{
>> -	BUG_ON(placement->num_placement >
>> (AMDGPU_GEM_DOMAIN_MAX + 1));
>> -
>> -	memcpy(bo->placements, placement->placement,
>> -	       placement->num_placement * sizeof(struct ttm_place));
>> -	bo->placement.num_placement = placement->num_placement;
>> -	bo->placement.num_busy_placement = placement-
>>> num_busy_placement;
>> -	bo->placement.placement = bo->placements;
>> -	bo->placement.busy_placement = bo->placements;
>> -}
>> -
>>   /**
>>    * amdgpu_bo_create_reserved - create reserved BO for kernel use
>>    *
>> @@ -303,14 +283,13 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo
>> **bo, u64 *gpu_addr,
>>   		*cpu_addr = NULL;
>>   }
>>
>> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>> -				unsigned long size, int byte_align,
>> -				bool kernel, u32 domain, u64 flags,
>> -				struct sg_table *sg,
>> -				struct ttm_placement *placement,
>> -				struct reservation_object *resv,
>> -				uint64_t init_value,
>> -				struct amdgpu_bo **bo_ptr)
>> +static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>> +			       unsigned long size, int byte_align,
>> +			       bool kernel, u32 domain, u64 flags,
>> +			       struct sg_table *sg,
>> +			       struct reservation_object *resv,
>> +			       uint64_t init_value,
>> +			       struct amdgpu_bo **bo_ptr)
> Still seems like amdgpu_bo_create_restricted is a better name than do_create.

How about amdgpu_bo_create_impl ?

Point is the function isn't restricted in any way any more.

Christian.

>
>>   {
>>   	struct amdgpu_bo *bo;
>>   	enum ttm_bo_type type;
>> @@ -384,10 +363,11 @@ int amdgpu_bo_create_restricted(struct
>> amdgpu_device *adev,
>>   		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>   #endif
>>
>> -	amdgpu_fill_placement_to_bo(bo, placement);
>> -	/* Kernel allocation are uninterruptible */
>> +	bo->tbo.bdev = &adev->mman.bdev;
>> +	amdgpu_ttm_placement_from_domain(bo, domain);
>>
>>   	initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
>> +	/* Kernel allocation are uninterruptible */
>>   	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size,
>> type,
>>   				 &bo->placement, page_align, !kernel, NULL,
>>   				 acc_size, sg, resv,
>> &amdgpu_ttm_bo_destroy);
>> @@ -442,27 +422,17 @@ static int amdgpu_bo_create_shadow(struct
>> amdgpu_device *adev,
>>   				   unsigned long size, int byte_align,
>>   				   struct amdgpu_bo *bo)
>>   {
>> -	struct ttm_placement placement = {0};
>> -	struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>>   	int r;
>>
>>   	if (bo->shadow)
>>   		return 0;
>>
>> -	memset(&placements, 0, sizeof(placements));
>> -	amdgpu_ttm_placement_init(adev, &placement, placements,
>> -				  AMDGPU_GEM_DOMAIN_GTT,
>> -				  AMDGPU_GEM_CREATE_CPU_GTT_USWC
>> |
>> -				  AMDGPU_GEM_CREATE_SHADOW);
>> -
>> -	r = amdgpu_bo_create_restricted(adev, size, byte_align, true,
>> -					AMDGPU_GEM_DOMAIN_GTT,
>> -
>> 	AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>> -					AMDGPU_GEM_CREATE_SHADOW,
>> -					NULL, &placement,
>> -					bo->tbo.resv,
>> -					0,
>> -					&bo->shadow);
>> +	r = amdgpu_bo_do_create(adev, size, byte_align, true,
>> +				AMDGPU_GEM_DOMAIN_GTT,
>> +				AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>> +				AMDGPU_GEM_CREATE_SHADOW,
>> +				NULL, bo->tbo.resv, 0,
>> +				&bo->shadow);
>>   	if (!r) {
>>   		bo->shadow->parent = amdgpu_bo_ref(bo);
>>   		mutex_lock(&adev->shadow_list_lock);
>> @@ -484,18 +454,11 @@ int amdgpu_bo_create(struct amdgpu_device
>> *adev,
>>   		     uint64_t init_value,
>>   		     struct amdgpu_bo **bo_ptr)
>>   {
>> -	struct ttm_placement placement = {0};
>> -	struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>>   	uint64_t parent_flags = flags & ~AMDGPU_GEM_CREATE_SHADOW;
>>   	int r;
>>
>> -	memset(&placements, 0, sizeof(placements));
>> -	amdgpu_ttm_placement_init(adev, &placement, placements,
>> -				  domain, parent_flags);
>> -
>> -	r = amdgpu_bo_create_restricted(adev, size, byte_align, kernel,
>> domain,
>> -					parent_flags, sg, &placement, resv,
>> -					init_value, bo_ptr);
>> +	r = amdgpu_bo_do_create(adev, size, byte_align, kernel, domain,
>> +				parent_flags, sg, resv, init_value, bo_ptr);
>>   	if (r)
>>   		return r;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index a4891be..39b6bf6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -195,14 +195,6 @@ int amdgpu_bo_create(struct amdgpu_device
>> *adev,
>>   			    struct reservation_object *resv,
>>   			    uint64_t init_value,
>>   			    struct amdgpu_bo **bo_ptr);
>> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>> -				unsigned long size, int byte_align,
>> -				bool kernel, u32 domain, u64 flags,
>> -				struct sg_table *sg,
>> -				struct ttm_placement *placement,
>> -			        struct reservation_object *resv,
>> -				uint64_t init_value,
>> -				struct amdgpu_bo **bo_ptr);
>>   int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>>   			      unsigned long size, int align,
>>   			      u32 domain, struct amdgpu_bo **bo_ptr,
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create

[Alex Deucher]

On Tue, Sep 12, 2017 at 2:35 PM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 12.09.2017 um 17:55 schrieb Deucher, Alexander:
>>>
>>> -----Original Message-----
>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>>> Of Christian König
>>> Sent: Tuesday, September 12, 2017 5:09 AM
>>> To: amd-gfx@lists.freedesktop.org
>>> Subject: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create
>>>
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Fix USWC handling by cleaning up the function and removing
>>> quite a bit of unused code.
>>
>> Can you clarify what was broken?
>
>
> We adjusted the BO flags for USWC handling, but those never took effect
> because the placement was passed in instead of generated inside this
> function.

Ah, yes, I see now.  Can you add that to the commit message?

>
>
>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 83 +++++++++------------
>>> ---------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  8 ---
>>>   2 files changed, 23 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 52d0109..726a662 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -64,11 +64,12 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct
>>> ttm_buffer_object *bo)
>>>         return false;
>>>   }
>>>
>>> -static void amdgpu_ttm_placement_init(struct amdgpu_device *adev,
>>> -                                     struct ttm_placement *placement,
>>> -                                     struct ttm_place *places,
>>> -                                     u32 domain, u64 flags)
>>> +void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
>>> domain)
>>>   {
>>> +       struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>>> +       struct ttm_placement *placement = &abo->placement;
>>> +       struct ttm_place *places = abo->placements;
>>> +       u64 flags = abo->flags;
>>>         u32 c = 0;
>>>
>>>         if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
>>> @@ -151,27 +152,6 @@ static void amdgpu_ttm_placement_init(struct
>>> amdgpu_device *adev,
>>>         placement->busy_placement = places;
>>>   }
>>>
>>> -void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
>>> domain)
>>> -{
>>> -       struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>>> -
>>> -       amdgpu_ttm_placement_init(adev, &abo->placement, abo-
>>>>
>>>> placements,
>>>
>>> -                                 domain, abo->flags);
>>> -}
>>> -
>>> -static void amdgpu_fill_placement_to_bo(struct amdgpu_bo *bo,
>>> -                                       struct ttm_placement *placement)
>>> -{
>>> -       BUG_ON(placement->num_placement >
>>> (AMDGPU_GEM_DOMAIN_MAX + 1));
>>> -
>>> -       memcpy(bo->placements, placement->placement,
>>> -              placement->num_placement * sizeof(struct ttm_place));
>>> -       bo->placement.num_placement = placement->num_placement;
>>> -       bo->placement.num_busy_placement = placement-
>>>>
>>>> num_busy_placement;
>>>
>>> -       bo->placement.placement = bo->placements;
>>> -       bo->placement.busy_placement = bo->placements;
>>> -}
>>> -
>>>   /**
>>>    * amdgpu_bo_create_reserved - create reserved BO for kernel use
>>>    *
>>> @@ -303,14 +283,13 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo
>>> **bo, u64 *gpu_addr,
>>>                 *cpu_addr = NULL;
>>>   }
>>>
>>> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>>> -                               unsigned long size, int byte_align,
>>> -                               bool kernel, u32 domain, u64 flags,
>>> -                               struct sg_table *sg,
>>> -                               struct ttm_placement *placement,
>>> -                               struct reservation_object *resv,
>>> -                               uint64_t init_value,
>>> -                               struct amdgpu_bo **bo_ptr)
>>> +static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>>> +                              unsigned long size, int byte_align,
>>> +                              bool kernel, u32 domain, u64 flags,
>>> +                              struct sg_table *sg,
>>> +                              struct reservation_object *resv,
>>> +                              uint64_t init_value,
>>> +                              struct amdgpu_bo **bo_ptr)
>>
>> Still seems like amdgpu_bo_create_restricted is a better name than
>> do_create.
>
>
> How about amdgpu_bo_create_impl ?
>
> Point is the function isn't restricted in any way any more.

Sorry, I was mixing this up with the pinning code in my head.
Objection withdrawn.  With the updated description, patch is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Alex


>
> Christian.
>
>
>>
>>>   {
>>>         struct amdgpu_bo *bo;
>>>         enum ttm_bo_type type;
>>> @@ -384,10 +363,11 @@ int amdgpu_bo_create_restricted(struct
>>> amdgpu_device *adev,
>>>                 bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>   #endif
>>>
>>> -       amdgpu_fill_placement_to_bo(bo, placement);
>>> -       /* Kernel allocation are uninterruptible */
>>> +       bo->tbo.bdev = &adev->mman.bdev;
>>> +       amdgpu_ttm_placement_from_domain(bo, domain);
>>>
>>>         initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
>>> +       /* Kernel allocation are uninterruptible */
>>>         r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size,
>>> type,
>>>                                  &bo->placement, page_align, !kernel,
>>> NULL,
>>>                                  acc_size, sg, resv,
>>> &amdgpu_ttm_bo_destroy);
>>> @@ -442,27 +422,17 @@ static int amdgpu_bo_create_shadow(struct
>>> amdgpu_device *adev,
>>>                                    unsigned long size, int byte_align,
>>>                                    struct amdgpu_bo *bo)
>>>   {
>>> -       struct ttm_placement placement = {0};
>>> -       struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>>>         int r;
>>>
>>>         if (bo->shadow)
>>>                 return 0;
>>>
>>> -       memset(&placements, 0, sizeof(placements));
>>> -       amdgpu_ttm_placement_init(adev, &placement, placements,
>>> -                                 AMDGPU_GEM_DOMAIN_GTT,
>>> -                                 AMDGPU_GEM_CREATE_CPU_GTT_USWC
>>> |
>>> -                                 AMDGPU_GEM_CREATE_SHADOW);
>>> -
>>> -       r = amdgpu_bo_create_restricted(adev, size, byte_align, true,
>>> -                                       AMDGPU_GEM_DOMAIN_GTT,
>>> -
>>>         AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>>> -                                       AMDGPU_GEM_CREATE_SHADOW,
>>> -                                       NULL, &placement,
>>> -                                       bo->tbo.resv,
>>> -                                       0,
>>> -                                       &bo->shadow);
>>> +       r = amdgpu_bo_do_create(adev, size, byte_align, true,
>>> +                               AMDGPU_GEM_DOMAIN_GTT,
>>> +                               AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>>> +                               AMDGPU_GEM_CREATE_SHADOW,
>>> +                               NULL, bo->tbo.resv, 0,
>>> +                               &bo->shadow);
>>>         if (!r) {
>>>                 bo->shadow->parent = amdgpu_bo_ref(bo);
>>>                 mutex_lock(&adev->shadow_list_lock);
>>> @@ -484,18 +454,11 @@ int amdgpu_bo_create(struct amdgpu_device
>>> *adev,
>>>                      uint64_t init_value,
>>>                      struct amdgpu_bo **bo_ptr)
>>>   {
>>> -       struct ttm_placement placement = {0};
>>> -       struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>>>         uint64_t parent_flags = flags & ~AMDGPU_GEM_CREATE_SHADOW;
>>>         int r;
>>>
>>> -       memset(&placements, 0, sizeof(placements));
>>> -       amdgpu_ttm_placement_init(adev, &placement, placements,
>>> -                                 domain, parent_flags);
>>> -
>>> -       r = amdgpu_bo_create_restricted(adev, size, byte_align, kernel,
>>> domain,
>>> -                                       parent_flags, sg, &placement,
>>> resv,
>>> -                                       init_value, bo_ptr);
>>> +       r = amdgpu_bo_do_create(adev, size, byte_align, kernel, domain,
>>> +                               parent_flags, sg, resv, init_value,
>>> bo_ptr);
>>>         if (r)
>>>                 return r;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> index a4891be..39b6bf6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> @@ -195,14 +195,6 @@ int amdgpu_bo_create(struct amdgpu_device
>>> *adev,
>>>                             struct reservation_object *resv,
>>>                             uint64_t init_value,
>>>                             struct amdgpu_bo **bo_ptr);
>>> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>>> -                               unsigned long size, int byte_align,
>>> -                               bool kernel, u32 domain, u64 flags,
>>> -                               struct sg_table *sg,
>>> -                               struct ttm_placement *placement,
>>> -                               struct reservation_object *resv,
>>> -                               uint64_t init_value,
>>> -                               struct amdgpu_bo **bo_ptr);
>>>   int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>>>                               unsigned long size, int align,
>>>                               u32 domain, struct amdgpu_bo **bo_ptr,
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Christian König]

From: Christian König <christian.koenig@amd.com>

Nobody is using the min/max interface any more.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++-------------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
 2 files changed, 6 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 726a662..8a8add3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
 		*bo = NULL;
 }
 
-int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
-			     u64 min_offset, u64 max_offset,
-			     u64 *gpu_addr)
+int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+	unsigned lpfn;
 	int r, i;
-	unsigned fpfn, lpfn;
 
 	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
 		return -EPERM;
 
-	if (WARN_ON_ONCE(min_offset > max_offset))
-		return -EINVAL;
-
 	/* A shared bo cannot be migrated to VRAM */
 	if (bo->prime_shared_count && (domain == AMDGPU_GEM_DOMAIN_VRAM))
 		return -EINVAL;
@@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 		if (gpu_addr)
 			*gpu_addr = amdgpu_bo_gpu_offset(bo);
 
-		if (max_offset != 0) {
-			u64 domain_start = bo->tbo.bdev->man[mem_type].gpu_offset;
-			WARN_ON_ONCE(max_offset <
-				     (amdgpu_bo_gpu_offset(bo) - domain_start));
-		}
-
 		return 0;
 	}
 
@@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 	for (i = 0; i < bo->placement.num_placement; i++) {
 		/* force to pin into visible video ram */
 		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
-		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) &&
-		    (!max_offset || max_offset >
-		     adev->mc.visible_vram_size)) {
-			if (WARN_ON_ONCE(min_offset >
-					 adev->mc.visible_vram_size))
-				return -EINVAL;
-			fpfn = min_offset >> PAGE_SHIFT;
+		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) {
 			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
-		} else {
-			fpfn = min_offset >> PAGE_SHIFT;
-			lpfn = max_offset >> PAGE_SHIFT;
+			if (!bo->placements[i].lpfn ||
+			    (lpfn && lpfn < bo->placements[i].lpfn))
+				bo->placements[i].lpfn = lpfn;
 		}
-		if (fpfn > bo->placements[i].fpfn)
-			bo->placements[i].fpfn = fpfn;
-		if (!bo->placements[i].lpfn ||
-		    (lpfn && lpfn < bo->placements[i].lpfn))
-			bo->placements[i].lpfn = lpfn;
 		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
 	}
 
@@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 	return r;
 }
 
-int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
-{
-	return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
-}
-
 int amdgpu_bo_unpin(struct amdgpu_bo *bo)
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 39b6bf6..4b2c042 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
 struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
 void amdgpu_bo_unref(struct amdgpu_bo **bo);
 int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
-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_init(struct amdgpu_device *adev);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Tuesday, September 12, 2017 5:09 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> 
> From: Christian König <christian.koenig@amd.com>
> 
> Nobody is using the min/max interface any more.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
> -------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>  2 files changed, 6 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 726a662..8a8add3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>  		*bo = NULL;
>  }
> 
> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> -			     u64 min_offset, u64 max_offset,
> -			     u64 *gpu_addr)
> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>  {
>  	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +	unsigned lpfn;
>  	int r, i;
> -	unsigned fpfn, lpfn;
> 
>  	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>  		return -EPERM;
> 
> -	if (WARN_ON_ONCE(min_offset > max_offset))
> -		return -EINVAL;
> -
>  	/* A shared bo cannot be migrated to VRAM */
>  	if (bo->prime_shared_count && (domain ==
> AMDGPU_GEM_DOMAIN_VRAM))
>  		return -EINVAL;
> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>  		if (gpu_addr)
>  			*gpu_addr = amdgpu_bo_gpu_offset(bo);
> 
> -		if (max_offset != 0) {
> -			u64 domain_start = bo->tbo.bdev-
> >man[mem_type].gpu_offset;
> -			WARN_ON_ONCE(max_offset <
> -				     (amdgpu_bo_gpu_offset(bo) -
> domain_start));
> -		}
> -
>  		return 0;
>  	}
> 
> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>  	for (i = 0; i < bo->placement.num_placement; i++) {
>  		/* force to pin into visible video ram */
>  		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
> -		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
> &&
> -		    (!max_offset || max_offset >
> -		     adev->mc.visible_vram_size)) {
> -			if (WARN_ON_ONCE(min_offset >
> -					 adev->mc.visible_vram_size))
> -				return -EINVAL;
> -			fpfn = min_offset >> PAGE_SHIFT;
> +		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
> {
>  			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
> -		} else {
> -			fpfn = min_offset >> PAGE_SHIFT;
> -			lpfn = max_offset >> PAGE_SHIFT;
> +			if (!bo->placements[i].lpfn ||
> +			    (lpfn && lpfn < bo->placements[i].lpfn))
> +				bo->placements[i].lpfn = lpfn;
>  		}
> -		if (fpfn > bo->placements[i].fpfn)
> -			bo->placements[i].fpfn = fpfn;
> -		if (!bo->placements[i].lpfn ||
> -		    (lpfn && lpfn < bo->placements[i].lpfn))
> -			bo->placements[i].lpfn = lpfn;
>  		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>  	}
> 
> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>  	return r;
>  }
> 
> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> -{
> -	return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
> -}
> -
>  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>  {
>  	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 39b6bf6..4b2c042 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>  void amdgpu_bo_unref(struct amdgpu_bo **bo);
>  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
> -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_init(struct amdgpu_device *adev);
> --
> 2.7.4
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[zhoucm1]

On 2017年09月12日 23:59, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Christian König
>> Sent: Tuesday, September 12, 2017 5:09 AM
>> To: amd-gfx@lists.freedesktop.org
>> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>>
>> From: Christian König <christian.koenig@amd.com>
>>
>> Nobody is using the min/max interface any more.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.
Yes, if our ISP ip is enabled, this reserve memory is must.

David Zhou
>
> Alex
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
>> -------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>>   2 files changed, 6 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 726a662..8a8add3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>>   		*bo = NULL;
>>   }
>>
>> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>> -			     u64 min_offset, u64 max_offset,
>> -			     u64 *gpu_addr)
>> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>>   {
>>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> +	unsigned lpfn;
>>   	int r, i;
>> -	unsigned fpfn, lpfn;
>>
>>   	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>>   		return -EPERM;
>>
>> -	if (WARN_ON_ONCE(min_offset > max_offset))
>> -		return -EINVAL;
>> -
>>   	/* A shared bo cannot be migrated to VRAM */
>>   	if (bo->prime_shared_count && (domain ==
>> AMDGPU_GEM_DOMAIN_VRAM))
>>   		return -EINVAL;
>> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> *bo, u32 domain,
>>   		if (gpu_addr)
>>   			*gpu_addr = amdgpu_bo_gpu_offset(bo);
>>
>> -		if (max_offset != 0) {
>> -			u64 domain_start = bo->tbo.bdev-
>>> man[mem_type].gpu_offset;
>> -			WARN_ON_ONCE(max_offset <
>> -				     (amdgpu_bo_gpu_offset(bo) -
>> domain_start));
>> -		}
>> -
>>   		return 0;
>>   	}
>>
>> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> *bo, u32 domain,
>>   	for (i = 0; i < bo->placement.num_placement; i++) {
>>   		/* force to pin into visible video ram */
>>   		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>> -		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
>> &&
>> -		    (!max_offset || max_offset >
>> -		     adev->mc.visible_vram_size)) {
>> -			if (WARN_ON_ONCE(min_offset >
>> -					 adev->mc.visible_vram_size))
>> -				return -EINVAL;
>> -			fpfn = min_offset >> PAGE_SHIFT;
>> +		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>> {
>>   			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>> -		} else {
>> -			fpfn = min_offset >> PAGE_SHIFT;
>> -			lpfn = max_offset >> PAGE_SHIFT;
>> +			if (!bo->placements[i].lpfn ||
>> +			    (lpfn && lpfn < bo->placements[i].lpfn))
>> +				bo->placements[i].lpfn = lpfn;
>>   		}
>> -		if (fpfn > bo->placements[i].fpfn)
>> -			bo->placements[i].fpfn = fpfn;
>> -		if (!bo->placements[i].lpfn ||
>> -		    (lpfn && lpfn < bo->placements[i].lpfn))
>> -			bo->placements[i].lpfn = lpfn;
>>   		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>>   	}
>>
>> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> *bo, u32 domain,
>>   	return r;
>>   }
>>
>> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>> -{
>> -	return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
>> -}
>> -
>>   int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>   {
>>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index 39b6bf6..4b2c042 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>>   struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>>   void amdgpu_bo_unref(struct amdgpu_bo **bo);
>>   int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
>> -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_init(struct amdgpu_device *adev);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Christian König]

Am 13.09.2017 um 04:23 schrieb zhoucm1:
>
>
> On 2017年09月12日 23:59, Deucher, Alexander wrote:
>>> -----Original Message-----
>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>>> Of Christian König
>>> Sent: Tuesday, September 12, 2017 5:09 AM
>>> To: amd-gfx@lists.freedesktop.org
>>> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>>>
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Nobody is using the min/max interface any more.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> I'm not sure it's a good idea to get rid of this.  I can see a need 
>> to reserve memory at specific offsets in memory. Specifically I think 
>> SR-IOV will be placing structures in memory to communicate 
>> configuration details from the host to the guest.  Also, we should be 
>> reserving the vbios scratch area, but we don't currently.
> Yes, if our ISP ip is enabled, this reserve memory is must.

Ok in this case I'm going to drop this one.

Christian.

>
> David Zhou
>>
>> Alex
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 
>>> +++++------------------
>>> -------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>>>   2 files changed, 6 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 726a662..8a8add3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>>>           *bo = NULL;
>>>   }
>>>
>>> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>>> -                 u64 min_offset, u64 max_offset,
>>> -                 u64 *gpu_addr)
>>> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>>>   {
>>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>> +    unsigned lpfn;
>>>       int r, i;
>>> -    unsigned fpfn, lpfn;
>>>
>>>       if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>>>           return -EPERM;
>>>
>>> -    if (WARN_ON_ONCE(min_offset > max_offset))
>>> -        return -EINVAL;
>>> -
>>>       /* A shared bo cannot be migrated to VRAM */
>>>       if (bo->prime_shared_count && (domain ==
>>> AMDGPU_GEM_DOMAIN_VRAM))
>>>           return -EINVAL;
>>> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>> *bo, u32 domain,
>>>           if (gpu_addr)
>>>               *gpu_addr = amdgpu_bo_gpu_offset(bo);
>>>
>>> -        if (max_offset != 0) {
>>> -            u64 domain_start = bo->tbo.bdev-
>>>> man[mem_type].gpu_offset;
>>> -            WARN_ON_ONCE(max_offset <
>>> -                     (amdgpu_bo_gpu_offset(bo) -
>>> domain_start));
>>> -        }
>>> -
>>>           return 0;
>>>       }
>>>
>>> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>> *bo, u32 domain,
>>>       for (i = 0; i < bo->placement.num_placement; i++) {
>>>           /* force to pin into visible video ram */
>>>           if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>>> -            !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
>>> &&
>>> -            (!max_offset || max_offset >
>>> -             adev->mc.visible_vram_size)) {
>>> -            if (WARN_ON_ONCE(min_offset >
>>> -                     adev->mc.visible_vram_size))
>>> -                return -EINVAL;
>>> -            fpfn = min_offset >> PAGE_SHIFT;
>>> +            !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>>> {
>>>               lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>>> -        } else {
>>> -            fpfn = min_offset >> PAGE_SHIFT;
>>> -            lpfn = max_offset >> PAGE_SHIFT;
>>> +            if (!bo->placements[i].lpfn ||
>>> +                (lpfn && lpfn < bo->placements[i].lpfn))
>>> +                bo->placements[i].lpfn = lpfn;
>>>           }
>>> -        if (fpfn > bo->placements[i].fpfn)
>>> -            bo->placements[i].fpfn = fpfn;
>>> -        if (!bo->placements[i].lpfn ||
>>> -            (lpfn && lpfn < bo->placements[i].lpfn))
>>> -            bo->placements[i].lpfn = lpfn;
>>>           bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>>>       }
>>>
>>> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>> *bo, u32 domain,
>>>       return r;
>>>   }
>>>
>>> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>>> -{
>>> -    return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
>>> -}
>>> -
>>>   int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>>   {
>>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> index 39b6bf6..4b2c042 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>>>   struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>>>   void amdgpu_bo_unref(struct amdgpu_bo **bo);
>>>   int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
>>> -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_init(struct amdgpu_device *adev);
>>> -- 
>>> 2.7.4
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 6919 bytes --]

SRIOV need to reserve a memory at an offset that set by GIM/hypervisor side, but I'm not sure how to do it perfectly,  currently we call bo_create to allocate a VRAM BO, and call pin_restrict with "offset" as parameter for "min" and "offset + size" as "max",


I feel strange of above approach frankly speaking (unless the new offset equals to the original offset from bo_create),


Because the original gpu offset (from the bo_create) is different with the new "offset" provided by GIM, what will TTM/DRM do on the range of <original offset, new offset> after we pin the bo to <new offset, new offset+ size> ???


BR Monk


________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
Sent: Tuesday, September 12, 2017 11:59:35 PM
To: 'Christian König'; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
> Of Christian König
> Sent: Tuesday, September 12, 2017 5:09 AM
> To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>
> From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>
> Nobody is using the min/max interface any more.
>
> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>

I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
> -------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>  2 files changed, 6 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 726a662..8a8add3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>                *bo = NULL;
>  }
>
> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> -                          u64 min_offset, u64 max_offset,
> -                          u64 *gpu_addr)
> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>  {
>        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +     unsigned lpfn;
>        int r, i;
> -     unsigned fpfn, lpfn;
>
>        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>                return -EPERM;
>
> -     if (WARN_ON_ONCE(min_offset > max_offset))
> -             return -EINVAL;
> -
>        /* A shared bo cannot be migrated to VRAM */
>        if (bo->prime_shared_count && (domain ==
> AMDGPU_GEM_DOMAIN_VRAM))
>                return -EINVAL;
> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>                if (gpu_addr)
>                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
>
> -             if (max_offset != 0) {
> -                     u64 domain_start = bo->tbo.bdev-
> >man[mem_type].gpu_offset;
> -                     WARN_ON_ONCE(max_offset <
> -                                  (amdgpu_bo_gpu_offset(bo) -
> domain_start));
> -             }
> -
>                return 0;
>        }
>
> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>        for (i = 0; i < bo->placement.num_placement; i++) {
>                /* force to pin into visible video ram */
>                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
> -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
> &&
> -                 (!max_offset || max_offset >
> -                  adev->mc.visible_vram_size)) {
> -                     if (WARN_ON_ONCE(min_offset >
> -                                      adev->mc.visible_vram_size))
> -                             return -EINVAL;
> -                     fpfn = min_offset >> PAGE_SHIFT;
> +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
> {
>                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
> -             } else {
> -                     fpfn = min_offset >> PAGE_SHIFT;
> -                     lpfn = max_offset >> PAGE_SHIFT;
> +                     if (!bo->placements[i].lpfn ||
> +                         (lpfn && lpfn < bo->placements[i].lpfn))
> +                             bo->placements[i].lpfn = lpfn;
>                }
> -             if (fpfn > bo->placements[i].fpfn)
> -                     bo->placements[i].fpfn = fpfn;
> -             if (!bo->placements[i].lpfn ||
> -                 (lpfn && lpfn < bo->placements[i].lpfn))
> -                     bo->placements[i].lpfn = lpfn;
>                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>        }
>
> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>        return r;
>  }
>
> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> -{
> -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
> -}
> -
>  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>  {
>        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 39b6bf6..4b2c042 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>  void amdgpu_bo_unref(struct amdgpu_bo **bo);
>  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
> -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_init(struct amdgpu_device *adev);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #1.2: Type: text/html, Size: 13821 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 7475 bytes --]

amdgpu_bo_create() doesn't necessarily allocate anything, it just 
creates the BO structure.

The backing memory for GTT and CPU domain is only allocated on first 
use, only VRAM is allocated directly.

So just call amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU and then the 
pin with AMDGPU_GEM_DOMAIN_VRAM and your desired offset.

Regards,
Christian.

Am 13.09.2017 um 11:14 schrieb Liu, Monk:
>
> SRIOV need to reserve a memory at an offset that set by GIM/hypervisor 
> side, but I'm not sure how to do it perfectly,  currently we call 
> bo_create to allocate a VRAM BO, and call pin_restrict with "offset" 
> as parameter for "min" and "offset + size" as "max",
>
>
> I feel strange of above approach frankly speaking (unless the new 
> offset equals to the original offset from bo_create),
>
>
> Because the original gpu offset (from the bo_create) is different with 
> the new "offset" provided by GIM, what will TTM/DRM do on the range of 
> <original offset, new offset> after we pin the bo to <new offset, new 
> offset+ size> ???
>
>
> BR Monk
>
>
> ------------------------------------------------------------------------
> *From:* amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of 
> Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
> *Sent:* Tuesday, September 12, 2017 11:59:35 PM
> *To:* 'Christian König'; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Subject:* RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> > -----Original Message-----
> > From: amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
> > Of Christian König
> > Sent: Tuesday, September 12, 2017 5:09 AM
> > To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> >
> > From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
> >
> > Nobody is using the min/max interface any more.
> >
> > Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>
> I'm not sure it's a good idea to get rid of this.  I can see a need to 
> reserve memory at specific offsets in memory. Specifically I think 
> SR-IOV will be placing structures in memory to communicate 
> configuration details from the host to the guest.  Also, we should be 
> reserving the vbios scratch area, but we don't currently.
>
> Alex
>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
> > -------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
> >  2 files changed, 6 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 726a662..8a8add3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
> >                *bo = NULL;
> >  }
> >
> > -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> > -                          u64 min_offset, u64 max_offset,
> > -                          u64 *gpu_addr)
> > +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> >  {
> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> > +     unsigned lpfn;
> >        int r, i;
> > -     unsigned fpfn, lpfn;
> >
> >        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
> >                return -EPERM;
> >
> > -     if (WARN_ON_ONCE(min_offset > max_offset))
> > -             return -EINVAL;
> > -
> >        /* A shared bo cannot be migrated to VRAM */
> >        if (bo->prime_shared_count && (domain ==
> > AMDGPU_GEM_DOMAIN_VRAM))
> >                return -EINVAL;
> > @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> > *bo, u32 domain,
> >                if (gpu_addr)
> >                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
> >
> > -             if (max_offset != 0) {
> > -                     u64 domain_start = bo->tbo.bdev-
> > >man[mem_type].gpu_offset;
> > -                     WARN_ON_ONCE(max_offset <
> > - (amdgpu_bo_gpu_offset(bo) -
> > domain_start));
> > -             }
> > -
> >                return 0;
> >        }
> >
> > @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> > *bo, u32 domain,
> >        for (i = 0; i < bo->placement.num_placement; i++) {
> >                /* force to pin into visible video ram */
> >                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
> > -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
> > &&
> > -                 (!max_offset || max_offset >
> > -                  adev->mc.visible_vram_size)) {
> > -                     if (WARN_ON_ONCE(min_offset >
> > - adev->mc.visible_vram_size))
> > -                             return -EINVAL;
> > -                     fpfn = min_offset >> PAGE_SHIFT;
> > +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
> > {
> >                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
> > -             } else {
> > -                     fpfn = min_offset >> PAGE_SHIFT;
> > -                     lpfn = max_offset >> PAGE_SHIFT;
> > +                     if (!bo->placements[i].lpfn ||
> > +                         (lpfn && lpfn < bo->placements[i].lpfn))
> > +                             bo->placements[i].lpfn = lpfn;
> >                }
> > -             if (fpfn > bo->placements[i].fpfn)
> > -                     bo->placements[i].fpfn = fpfn;
> > -             if (!bo->placements[i].lpfn ||
> > -                 (lpfn && lpfn < bo->placements[i].lpfn))
> > -                     bo->placements[i].lpfn = lpfn;
> >                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> >        }
> >
> > @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> > *bo, u32 domain,
> >        return r;
> >  }
> >
> > -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> > -{
> > -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
> > -}
> > -
> >  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
> >  {
> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > index 39b6bf6..4b2c042 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
> >  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
> >  void amdgpu_bo_unref(struct amdgpu_bo **bo);
> >  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
> > -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_init(struct amdgpu_device *adev);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 13697 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 8265 bytes --]

but we use VRAM domain in the first bo_create stage,  so this BO is allocated directly ...


I think our current approach is incorrect ... because the space from bo_create(VRAM_DOMAIN) is missed somehow

________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Wednesday, September 13, 2017 5:21:13 PM
To: Liu, Monk; Deucher, Alexander; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

amdgpu_bo_create() doesn't necessarily allocate anything, it just creates the BO structure.

The backing memory for GTT and CPU domain is only allocated on first use, only VRAM is allocated directly.

So just call amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU and then the pin with AMDGPU_GEM_DOMAIN_VRAM and your desired offset.

Regards,
Christian.

Am 13.09.2017 um 11:14 schrieb Liu, Monk:

SRIOV need to reserve a memory at an offset that set by GIM/hypervisor side, but I'm not sure how to do it perfectly,  currently we call bo_create to allocate a VRAM BO, and call pin_restrict with "offset" as parameter for "min" and "offset + size" as "max",


I feel strange of above approach frankly speaking (unless the new offset equals to the original offset from bo_create),


Because the original gpu offset (from the bo_create) is different with the new "offset" provided by GIM, what will TTM/DRM do on the range of <original offset, new offset> after we pin the bo to <new offset, new offset+ size> ???


BR Monk


________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org><mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Deucher, Alexander <Alexander.Deucher@amd.com><mailto:Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
Sent: Tuesday, September 12, 2017 11:59:35 PM
To: 'Christian König'; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
> Of Christian König
> Sent: Tuesday, September 12, 2017 5:09 AM
> To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>
> From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org><mailto:christian.koenig@amd.com>
>
> Nobody is using the min/max interface any more.
>
> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org><mailto:christian.koenig-5C7GfCeVMHo@public.gmane.org>

I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
> -------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>  2 files changed, 6 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 726a662..8a8add3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>                *bo = NULL;
>  }
>
> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> -                          u64 min_offset, u64 max_offset,
> -                          u64 *gpu_addr)
> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>  {
>        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +     unsigned lpfn;
>        int r, i;
> -     unsigned fpfn, lpfn;
>
>        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>                return -EPERM;
>
> -     if (WARN_ON_ONCE(min_offset > max_offset))
> -             return -EINVAL;
> -
>        /* A shared bo cannot be migrated to VRAM */
>        if (bo->prime_shared_count && (domain ==
> AMDGPU_GEM_DOMAIN_VRAM))
>                return -EINVAL;
> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>                if (gpu_addr)
>                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
>
> -             if (max_offset != 0) {
> -                     u64 domain_start = bo->tbo.bdev-
> >man[mem_type].gpu_offset;
> -                     WARN_ON_ONCE(max_offset <
> -                                  (amdgpu_bo_gpu_offset(bo) -
> domain_start));
> -             }
> -
>                return 0;
>        }
>
> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>        for (i = 0; i < bo->placement.num_placement; i++) {
>                /* force to pin into visible video ram */
>                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
> -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
> &&
> -                 (!max_offset || max_offset >
> -                  adev->mc.visible_vram_size)) {
> -                     if (WARN_ON_ONCE(min_offset >
> -                                      adev->mc.visible_vram_size))
> -                             return -EINVAL;
> -                     fpfn = min_offset >> PAGE_SHIFT;
> +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
> {
>                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
> -             } else {
> -                     fpfn = min_offset >> PAGE_SHIFT;
> -                     lpfn = max_offset >> PAGE_SHIFT;
> +                     if (!bo->placements[i].lpfn ||
> +                         (lpfn && lpfn < bo->placements[i].lpfn))
> +                             bo->placements[i].lpfn = lpfn;
>                }
> -             if (fpfn > bo->placements[i].fpfn)
> -                     bo->placements[i].fpfn = fpfn;
> -             if (!bo->placements[i].lpfn ||
> -                 (lpfn && lpfn < bo->placements[i].lpfn))
> -                     bo->placements[i].lpfn = lpfn;
>                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>        }
>
> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>        return r;
>  }
>
> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> -{
> -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
> -}
> -
>  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>  {
>        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 39b6bf6..4b2c042 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>  void amdgpu_bo_unref(struct amdgpu_bo **bo);
>  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
> -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_init(struct amdgpu_device *adev);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 16409 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 9120 bytes --]

> but we use VRAM domain in the first bo_create stage,  so this BO is 
> allocated directly ...
>
Yeah, but just stop doing so. amdgpu_bo_create can just be used to 
allocate the BO structure without any backing storage.

So what you do is calling amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU 
(or just 0 might work as well).

And then when you call amdgpu_bo_pin_restricted() you give 
AMDGPU_GEM_DOMAIN_VRAM and your offset and size as min/max value for the 
placement.

This way you can avoid the move and allocate the BO directly where you 
want it.

Regards,
Christian.

Am 13.09.2017 um 11:26 schrieb Liu, Monk:
>
> but we use VRAM domain in the first bo_create stage,  so this BO is 
> allocated directly ...
>
>
> I think our current approach is incorrect ... because the space from 
> bo_create(VRAM_DOMAIN) is missed somehow
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *Sent:* Wednesday, September 13, 2017 5:21:13 PM
> *To:* Liu, Monk; Deucher, Alexander; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Subject:* Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> amdgpu_bo_create() doesn't necessarily allocate anything, it just 
> creates the BO structure.
>
> The backing memory for GTT and CPU domain is only allocated on first 
> use, only VRAM is allocated directly.
>
> So just call amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU and then 
> the pin with AMDGPU_GEM_DOMAIN_VRAM and your desired offset.
>
> Regards,
> Christian.
>
> Am 13.09.2017 um 11:14 schrieb Liu, Monk:
>>
>> SRIOV need to reserve a memory at an offset that set by 
>> GIM/hypervisor side, but I'm not sure how to do it perfectly,  
>> currently we call bo_create to allocate a VRAM BO, and call 
>> pin_restrict with "offset" as parameter for "min" and "offset + size" 
>> as "max",
>>
>>
>> I feel strange of above approach frankly speaking (unless the new 
>> offset equals to the original offset from bo_create),
>>
>>
>> Because the original gpu offset (from the bo_create) is different 
>> with the new "offset" provided by GIM, what will TTM/DRM do on the 
>> range of <original offset, new offset> after we pin the bo to <new 
>> offset, new offset+ size> ???
>>
>>
>> BR Monk
>>
>>
>> ------------------------------------------------------------------------
>> *From:* amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of 
>> Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
>> *Sent:* Tuesday, September 12, 2017 11:59:35 PM
>> *To:* 'Christian König'; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> *Subject:* RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>> > -----Original Message-----
>> > From: amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
>> > Of Christian König
>> > Sent: Tuesday, September 12, 2017 5:09 AM
>> > To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> > Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>> >
>> > From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>> >
>> > Nobody is using the min/max interface any more.
>> >
>> > Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>>
>> I'm not sure it's a good idea to get rid of this.  I can see a need 
>> to reserve memory at specific offsets in memory.  Specifically I 
>> think SR-IOV will be placing structures in memory to communicate 
>> configuration details from the host to the guest.  Also, we should be 
>> reserving the vbios scratch area, but we don't currently.
>>
>> Alex
>>
>> > ---
>> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 
>> +++++------------------
>> > -------
>> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>> >  2 files changed, 6 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > index 726a662..8a8add3 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>> >                *bo = NULL;
>> >  }
>> >
>> > -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>> > -                          u64 min_offset, u64 max_offset,
>> > -                          u64 *gpu_addr)
>> > +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>> >  {
>> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> > +     unsigned lpfn;
>> >        int r, i;
>> > -     unsigned fpfn, lpfn;
>> >
>> >        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>> >                return -EPERM;
>> >
>> > -     if (WARN_ON_ONCE(min_offset > max_offset))
>> > -             return -EINVAL;
>> > -
>> >        /* A shared bo cannot be migrated to VRAM */
>> >        if (bo->prime_shared_count && (domain ==
>> > AMDGPU_GEM_DOMAIN_VRAM))
>> >                return -EINVAL;
>> > @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> > *bo, u32 domain,
>> >                if (gpu_addr)
>> >                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
>> >
>> > -             if (max_offset != 0) {
>> > -                     u64 domain_start = bo->tbo.bdev-
>> > >man[mem_type].gpu_offset;
>> > -                     WARN_ON_ONCE(max_offset <
>> > - (amdgpu_bo_gpu_offset(bo) -
>> > domain_start));
>> > -             }
>> > -
>> >                return 0;
>> >        }
>> >
>> > @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> > *bo, u32 domain,
>> >        for (i = 0; i < bo->placement.num_placement; i++) {
>> >                /* force to pin into visible video ram */
>> >                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>> > -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
>> > &&
>> > -                 (!max_offset || max_offset >
>> > -                  adev->mc.visible_vram_size)) {
>> > -                     if (WARN_ON_ONCE(min_offset >
>> > - adev->mc.visible_vram_size))
>> > -                             return -EINVAL;
>> > -                     fpfn = min_offset >> PAGE_SHIFT;
>> > +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>> > {
>> >                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>> > -             } else {
>> > -                     fpfn = min_offset >> PAGE_SHIFT;
>> > -                     lpfn = max_offset >> PAGE_SHIFT;
>> > +                     if (!bo->placements[i].lpfn ||
>> > +                         (lpfn && lpfn < bo->placements[i].lpfn))
>> > + bo->placements[i].lpfn = lpfn;
>> >                }
>> > -             if (fpfn > bo->placements[i].fpfn)
>> > -                     bo->placements[i].fpfn = fpfn;
>> > -             if (!bo->placements[i].lpfn ||
>> > -                 (lpfn && lpfn < bo->placements[i].lpfn))
>> > -                     bo->placements[i].lpfn = lpfn;
>> >                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>> >        }
>> >
>> > @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> > *bo, u32 domain,
>> >        return r;
>> >  }
>> >
>> > -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>> > -{
>> > -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
>> > -}
>> > -
>> >  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>> >  {
>> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > index 39b6bf6..4b2c042 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>> >  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>> >  void amdgpu_bo_unref(struct amdgpu_bo **bo);
>> >  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
>> > -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_init(struct amdgpu_device *adev);
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > amd-gfx mailing list
>> > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx 
>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 18603 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 9623 bytes --]

yeah, that's right !


Thanks

BR Monk


________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Wednesday, September 13, 2017 5:30 PM
To: Liu, Monk; Deucher, Alexander; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted


but we use VRAM domain in the first bo_create stage,  so this BO is allocated directly ...

Yeah, but just stop doing so. amdgpu_bo_create can just be used to allocate the BO structure without any backing storage.

So what you do is calling amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU (or just 0 might work as well).

And then when you call amdgpu_bo_pin_restricted() you give AMDGPU_GEM_DOMAIN_VRAM and your offset and size as min/max value for the placement.

This way you can avoid the move and allocate the BO directly where you want it.

Regards,
Christian.

Am 13.09.2017 um 11:26 schrieb Liu, Monk:

but we use VRAM domain in the first bo_create stage,  so this BO is allocated directly ...


I think our current approach is incorrect ... because the space from bo_create(VRAM_DOMAIN) is missed somehow

________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org><mailto:deathsimple@vodafone.de>
Sent: Wednesday, September 13, 2017 5:21:13 PM
To: Liu, Monk; Deucher, Alexander; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

amdgpu_bo_create() doesn't necessarily allocate anything, it just creates the BO structure.

The backing memory for GTT and CPU domain is only allocated on first use, only VRAM is allocated directly.

So just call amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU and then the pin with AMDGPU_GEM_DOMAIN_VRAM and your desired offset.

Regards,
Christian.

Am 13.09.2017 um 11:14 schrieb Liu, Monk:

SRIOV need to reserve a memory at an offset that set by GIM/hypervisor side, but I'm not sure how to do it perfectly,  currently we call bo_create to allocate a VRAM BO, and call pin_restrict with "offset" as parameter for "min" and "offset + size" as "max",


I feel strange of above approach frankly speaking (unless the new offset equals to the original offset from bo_create),


Because the original gpu offset (from the bo_create) is different with the new "offset" provided by GIM, what will TTM/DRM do on the range of <original offset, new offset> after we pin the bo to <new offset, new offset+ size> ???


BR Monk


________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org><mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Deucher, Alexander <Alexander.Deucher@amd.com><mailto:Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
Sent: Tuesday, September 12, 2017 11:59:35 PM
To: 'Christian König'; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
> Of Christian König
> Sent: Tuesday, September 12, 2017 5:09 AM
> To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>
> From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org><mailto:christian.koenig@amd.com>
>
> Nobody is using the min/max interface any more.
>
> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org><mailto:christian.koenig-5C7GfCeVMHo@public.gmane.org>

I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
> -------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>  2 files changed, 6 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 726a662..8a8add3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>                *bo = NULL;
>  }
>
> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> -                          u64 min_offset, u64 max_offset,
> -                          u64 *gpu_addr)
> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>  {
>        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +     unsigned lpfn;
>        int r, i;
> -     unsigned fpfn, lpfn;
>
>        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>                return -EPERM;
>
> -     if (WARN_ON_ONCE(min_offset > max_offset))
> -             return -EINVAL;
> -
>        /* A shared bo cannot be migrated to VRAM */
>        if (bo->prime_shared_count && (domain ==
> AMDGPU_GEM_DOMAIN_VRAM))
>                return -EINVAL;
> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>                if (gpu_addr)
>                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
>
> -             if (max_offset != 0) {
> -                     u64 domain_start = bo->tbo.bdev-
> >man[mem_type].gpu_offset;
> -                     WARN_ON_ONCE(max_offset <
> -                                  (amdgpu_bo_gpu_offset(bo) -
> domain_start));
> -             }
> -
>                return 0;
>        }
>
> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>        for (i = 0; i < bo->placement.num_placement; i++) {
>                /* force to pin into visible video ram */
>                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
> -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
> &&
> -                 (!max_offset || max_offset >
> -                  adev->mc.visible_vram_size)) {
> -                     if (WARN_ON_ONCE(min_offset >
> -                                      adev->mc.visible_vram_size))
> -                             return -EINVAL;
> -                     fpfn = min_offset >> PAGE_SHIFT;
> +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
> {
>                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
> -             } else {
> -                     fpfn = min_offset >> PAGE_SHIFT;
> -                     lpfn = max_offset >> PAGE_SHIFT;
> +                     if (!bo->placements[i].lpfn ||
> +                         (lpfn && lpfn < bo->placements[i].lpfn))
> +                             bo->placements[i].lpfn = lpfn;
>                }
> -             if (fpfn > bo->placements[i].fpfn)
> -                     bo->placements[i].fpfn = fpfn;
> -             if (!bo->placements[i].lpfn ||
> -                 (lpfn && lpfn < bo->placements[i].lpfn))
> -                     bo->placements[i].lpfn = lpfn;
>                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>        }
>
> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> *bo, u32 domain,
>        return r;
>  }
>
> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> -{
> -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
> -}
> -
>  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>  {
>        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 39b6bf6..4b2c042 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>  void amdgpu_bo_unref(struct amdgpu_bo **bo);
>  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
> -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_init(struct amdgpu_device *adev);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 18522 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/5] drm/amdgpu: simplify pinning into visible VRAM

[Christian König]

From: Christian König <christian.koenig@amd.com>

Just set the CPU access required flag when we pin it.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 8a8add3..bc9f162 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -632,7 +632,6 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
 int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
-	unsigned lpfn;
 	int r, i;
 
 	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
@@ -656,18 +655,12 @@ int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
 	}
 
 	bo->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
+	/* force to pin into visible video ram */
+	if (!(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
+		bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
 	amdgpu_ttm_placement_from_domain(bo, domain);
-	for (i = 0; i < bo->placement.num_placement; i++) {
-		/* force to pin into visible video ram */
-		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
-		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) {
-			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
-			if (!bo->placements[i].lpfn ||
-			    (lpfn && lpfn < bo->placements[i].lpfn))
-				bo->placements[i].lpfn = lpfn;
-		}
+	for (i = 0; i < bo->placement.num_placement; i++)
 		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
-	}
 
 	r = ttm_bo_validate(&bo->tbo, &bo->placement, false, false);
 	if (unlikely(r)) {
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/5] drm/amdgpu: fix cgs alignment handling

[Alex Deucher]

On Tue, Sep 12, 2017 at 5:08 AM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> This always allocated on PAGE_SIZE alignment.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> index d450a96..8b5fa22 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> @@ -121,7 +121,7 @@ static int amdgpu_cgs_alloc_gpu_mem(struct cgs_device *cgs_device,
>         placement.busy_placement = &place;
>         placement.num_busy_placement = 1;
>
> -       ret = amdgpu_bo_create_restricted(adev, size, PAGE_SIZE,
> +       ret = amdgpu_bo_create_restricted(adev, size, align,
>                                           true, domain, flags,
>                                           NULL, &placement, NULL,
>                                           0, &obj);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx