[PATCH 1/3] drm/amdgpu: only move VM BOs in the LRU during validation v2

[PATCH 1/3] drm/amdgpu: only move VM BOs in the LRU during validation v2

[Christian König]

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

This should save us a bunch of command submission overhead.

v2: move the LRU move to the right place to avoid the move for the root BO
    and handle the shadow BOs as well. This turned out to be a bug fix because
    the move needs to happen before the kmap.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> (v1)
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 15 +++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 58 +++++++---------------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 --
 3 files changed, 16 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index cd5c08a..7fb4baa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -669,10 +669,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	}
 
 error_validate:
-	if (r) {
-		amdgpu_vm_move_pt_bos_in_lru(p->adev, &fpriv->vm);
+	if (r)
 		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
-	}
 
 error_free_pages:
 
@@ -720,21 +718,18 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
  * If error is set than unvalidate buffer, otherwise just free memory
  * used by parsing context.
  **/
-static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error, bool backoff)
+static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
+				  bool backoff)
 {
-	struct amdgpu_fpriv *fpriv = parser->filp->driver_priv;
 	unsigned i;
 
-	if (!error) {
-		amdgpu_vm_move_pt_bos_in_lru(parser->adev, &fpriv->vm);
-
+	if (!error)
 		ttm_eu_fence_buffer_objects(&parser->ticket,
 					    &parser->validated,
 					    parser->fence);
-	} else if (backoff) {
+	else if (backoff)
 		ttm_eu_backoff_reservation(&parser->ticket,
 					   &parser->validated);
-	}
 	dma_fence_put(parser->fence);
 
 	if (parser->ctx)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a1d4294..a375135 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -159,7 +159,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
  */
 static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
 				    int (*validate)(void *, struct amdgpu_bo *),
-				    void *param, bool use_cpu_for_update)
+				    void *param, bool use_cpu_for_update,
+				    struct ttm_bo_global *glob)
 {
 	unsigned i;
 	int r;
@@ -183,12 +184,18 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
 		if (r)
 			return r;
 
+		spin_lock(&glob->lru_lock);
+		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
+		if (entry->bo->shadow)
+			ttm_bo_move_to_lru_tail(&entry->bo->shadow->tbo);
+		spin_unlock(&glob->lru_lock);
+
 		/*
 		 * Recurse into the sub directory. This is harmless because we
 		 * have only a maximum of 5 layers.
 		 */
 		r = amdgpu_vm_validate_level(entry, validate, param,
-					     use_cpu_for_update);
+					     use_cpu_for_update, glob);
 		if (r)
 			return r;
 	}
@@ -220,54 +227,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		return 0;
 
 	return amdgpu_vm_validate_level(&vm->root, validate, param,
-					vm->use_cpu_for_update);
+					vm->use_cpu_for_update,
+					adev->mman.bdev.glob);
 }
 
 /**
- * amdgpu_vm_move_level_in_lru - move one level of PT BOs to the LRU tail
- *
- * @adev: amdgpu device instance
- * @vm: vm providing the BOs
- *
- * Move the PT BOs to the tail of the LRU.
- */
-static void amdgpu_vm_move_level_in_lru(struct amdgpu_vm_pt *parent)
-{
-	unsigned i;
-
-	if (!parent->entries)
-		return;
-
-	for (i = 0; i <= parent->last_entry_used; ++i) {
-		struct amdgpu_vm_pt *entry = &parent->entries[i];
-
-		if (!entry->bo)
-			continue;
-
-		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
-		amdgpu_vm_move_level_in_lru(entry);
-	}
-}
-
-/**
- * amdgpu_vm_move_pt_bos_in_lru - move the PT BOs to the LRU tail
- *
- * @adev: amdgpu device instance
- * @vm: vm providing the BOs
- *
- * Move the PT BOs to the tail of the LRU.
- */
-void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
-				  struct amdgpu_vm *vm)
-{
-	struct ttm_bo_global *glob = adev->mman.bdev.glob;
-
-	spin_lock(&glob->lru_lock);
-	amdgpu_vm_move_level_in_lru(&vm->root);
-	spin_unlock(&glob->lru_lock);
-}
-
- /**
  * amdgpu_vm_alloc_levels - allocate the PD/PT levels
  *
  * @adev: amdgpu_device pointer
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 34d9174..bac09ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -220,8 +220,6 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*callback)(void *p, struct amdgpu_bo *bo),
 			      void *param);
-void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
-				  struct amdgpu_vm *vm);
 int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 			struct amdgpu_vm *vm,
 			uint64_t saddr, uint64_t size);
-- 
2.7.4

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

[PATCH 2/3] drm/amdgpu: only bind VM shadows after validation v2

[Christian König]

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

No need to do this on every CS.

v2: remove all other bind, reorder code

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a375135..0308bb4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -165,6 +165,14 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
 	unsigned i;
 	int r;
 
+	if (parent->bo->shadow) {
+		struct amdgpu_bo *shadow = parent->bo->shadow;
+
+		r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
+		if (r)
+			return r;
+	}
+
 	if (use_cpu_for_update) {
 		r = amdgpu_bo_kmap(parent->bo, NULL);
 		if (r)
@@ -1030,11 +1038,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 
 		params.func = amdgpu_vm_cpu_set_ptes;
 	} else {
-		if (shadow) {
-			r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
-			if (r)
-				return r;
-		}
 		ring = container_of(vm->entity.sched, struct amdgpu_ring,
 				    sched);
 
@@ -1070,15 +1073,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 		if (bo == NULL)
 			continue;
 
-		if (bo->shadow) {
-			struct amdgpu_bo *pt_shadow = bo->shadow;
-
-			r = amdgpu_ttm_bind(&pt_shadow->tbo,
-					    &pt_shadow->tbo.mem);
-			if (r)
-				return r;
-		}
-
 		pt = amdgpu_bo_gpu_offset(bo);
 		pt = amdgpu_gart_get_vm_pde(adev, pt);
 		if (parent->entries[pt_idx].addr == pt ||
-- 
2.7.4

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

[PATCH 3/3] drm/amdgpu: fix Vega10 HW config for 2MB pages

[Christian König]

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

Those values weren't correct. This should result in quite some speedup.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 4 ++--
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index 408723e..6c8040e 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -144,8 +144,8 @@ static void gfxhub_v1_0_init_cache_regs(struct amdgpu_device *adev)
 	WREG32_SOC15(GC, 0, mmVM_L2_CNTL2, tmp);
 
 	tmp = mmVM_L2_CNTL3_DEFAULT;
-	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 12);
-	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, L2_CACHE_BIGK_FRAGMENT_SIZE, 9);
+	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 9);
+	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, L2_CACHE_BIGK_FRAGMENT_SIZE, 6);
 	WREG32_SOC15(GC, 0, mmVM_L2_CNTL3, tmp);
 
 	tmp = mmVM_L2_CNTL4_DEFAULT;
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
index ad8def3..74cb647 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
@@ -158,8 +158,8 @@ static void mmhub_v1_0_init_cache_regs(struct amdgpu_device *adev)
 	WREG32_SOC15(MMHUB, 0, mmVM_L2_CNTL2, tmp);
 
 	tmp = mmVM_L2_CNTL3_DEFAULT;
-	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 12);
-	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, L2_CACHE_BIGK_FRAGMENT_SIZE, 9);
+	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 9);
+	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, L2_CACHE_BIGK_FRAGMENT_SIZE, 6);
 	WREG32_SOC15(MMHUB, 0, mmVM_L2_CNTL3, tmp);
 
 	tmp = mmVM_L2_CNTL4_DEFAULT;
-- 
2.7.4

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

Re: [PATCH 1/3] drm/amdgpu: only move VM BOs in the LRU during validation v2

[zhoucm1]

The series is Acked-by: Chunming Zhou <david1.zhou@amd.com>


On 2017年07月29日 19:32, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> This should save us a bunch of command submission overhead.
>
> v2: move the LRU move to the right place to avoid the move for the root BO
>      and handle the shadow BOs as well. This turned out to be a bug fix because
>      the move needs to happen before the kmap.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> (v1)
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 15 +++------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 58 +++++++---------------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 --
>   3 files changed, 16 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index cd5c08a..7fb4baa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -669,10 +669,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   	}
>   
>   error_validate:
> -	if (r) {
> -		amdgpu_vm_move_pt_bos_in_lru(p->adev, &fpriv->vm);
> +	if (r)
>   		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
> -	}
>   
>   error_free_pages:
>   
> @@ -720,21 +718,18 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>    * If error is set than unvalidate buffer, otherwise just free memory
>    * used by parsing context.
>    **/
> -static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error, bool backoff)
> +static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
> +				  bool backoff)
>   {
> -	struct amdgpu_fpriv *fpriv = parser->filp->driver_priv;
>   	unsigned i;
>   
> -	if (!error) {
> -		amdgpu_vm_move_pt_bos_in_lru(parser->adev, &fpriv->vm);
> -
> +	if (!error)
>   		ttm_eu_fence_buffer_objects(&parser->ticket,
>   					    &parser->validated,
>   					    parser->fence);
> -	} else if (backoff) {
> +	else if (backoff)
>   		ttm_eu_backoff_reservation(&parser->ticket,
>   					   &parser->validated);
> -	}
>   	dma_fence_put(parser->fence);
>   
>   	if (parser->ctx)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a1d4294..a375135 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -159,7 +159,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>    */
>   static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>   				    int (*validate)(void *, struct amdgpu_bo *),
> -				    void *param, bool use_cpu_for_update)
> +				    void *param, bool use_cpu_for_update,
> +				    struct ttm_bo_global *glob)
>   {
>   	unsigned i;
>   	int r;
> @@ -183,12 +184,18 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>   		if (r)
>   			return r;
>   
> +		spin_lock(&glob->lru_lock);
> +		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
> +		if (entry->bo->shadow)
> +			ttm_bo_move_to_lru_tail(&entry->bo->shadow->tbo);
> +		spin_unlock(&glob->lru_lock);
> +
>   		/*
>   		 * Recurse into the sub directory. This is harmless because we
>   		 * have only a maximum of 5 layers.
>   		 */
>   		r = amdgpu_vm_validate_level(entry, validate, param,
> -					     use_cpu_for_update);
> +					     use_cpu_for_update, glob);
>   		if (r)
>   			return r;
>   	}
> @@ -220,54 +227,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		return 0;
>   
>   	return amdgpu_vm_validate_level(&vm->root, validate, param,
> -					vm->use_cpu_for_update);
> +					vm->use_cpu_for_update,
> +					adev->mman.bdev.glob);
>   }
>   
>   /**
> - * amdgpu_vm_move_level_in_lru - move one level of PT BOs to the LRU tail
> - *
> - * @adev: amdgpu device instance
> - * @vm: vm providing the BOs
> - *
> - * Move the PT BOs to the tail of the LRU.
> - */
> -static void amdgpu_vm_move_level_in_lru(struct amdgpu_vm_pt *parent)
> -{
> -	unsigned i;
> -
> -	if (!parent->entries)
> -		return;
> -
> -	for (i = 0; i <= parent->last_entry_used; ++i) {
> -		struct amdgpu_vm_pt *entry = &parent->entries[i];
> -
> -		if (!entry->bo)
> -			continue;
> -
> -		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
> -		amdgpu_vm_move_level_in_lru(entry);
> -	}
> -}
> -
> -/**
> - * amdgpu_vm_move_pt_bos_in_lru - move the PT BOs to the LRU tail
> - *
> - * @adev: amdgpu device instance
> - * @vm: vm providing the BOs
> - *
> - * Move the PT BOs to the tail of the LRU.
> - */
> -void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
> -				  struct amdgpu_vm *vm)
> -{
> -	struct ttm_bo_global *glob = adev->mman.bdev.glob;
> -
> -	spin_lock(&glob->lru_lock);
> -	amdgpu_vm_move_level_in_lru(&vm->root);
> -	spin_unlock(&glob->lru_lock);
> -}
> -
> - /**
>    * amdgpu_vm_alloc_levels - allocate the PD/PT levels
>    *
>    * @adev: amdgpu_device pointer
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 34d9174..bac09ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -220,8 +220,6 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			      int (*callback)(void *p, struct amdgpu_bo *bo),
>   			      void *param);
> -void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
> -				  struct amdgpu_vm *vm);
>   int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   			struct amdgpu_vm *vm,
>   			uint64_t saddr, uint64_t size);

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

RE: [PATCH 3/3] drm/amdgpu: fix Vega10 HW config for 2MB pages

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Saturday, July 29, 2017 7:32 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH 3/3] drm/amdgpu: fix Vega10 HW config for 2MB pages
> 
> From: Christian König <christian.koenig@amd.com>
> 
> Those values weren't correct. This should result in quite some speedup.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

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

> ---
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 4 ++--
>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> index 408723e..6c8040e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> @@ -144,8 +144,8 @@ static void gfxhub_v1_0_init_cache_regs(struct
> amdgpu_device *adev)
>  	WREG32_SOC15(GC, 0, mmVM_L2_CNTL2, tmp);
> 
>  	tmp = mmVM_L2_CNTL3_DEFAULT;
> -	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 12);
> -	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3,
> L2_CACHE_BIGK_FRAGMENT_SIZE, 9);
> +	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 9);
> +	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3,
> L2_CACHE_BIGK_FRAGMENT_SIZE, 6);
>  	WREG32_SOC15(GC, 0, mmVM_L2_CNTL3, tmp);
> 
>  	tmp = mmVM_L2_CNTL4_DEFAULT;
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> index ad8def3..74cb647 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> @@ -158,8 +158,8 @@ static void mmhub_v1_0_init_cache_regs(struct
> amdgpu_device *adev)
>  	WREG32_SOC15(MMHUB, 0, mmVM_L2_CNTL2, tmp);
> 
>  	tmp = mmVM_L2_CNTL3_DEFAULT;
> -	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 12);
> -	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3,
> L2_CACHE_BIGK_FRAGMENT_SIZE, 9);
> +	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3, BANK_SELECT, 9);
> +	tmp = REG_SET_FIELD(tmp, VM_L2_CNTL3,
> L2_CACHE_BIGK_FRAGMENT_SIZE, 6);
>  	WREG32_SOC15(MMHUB, 0, mmVM_L2_CNTL3, tmp);
> 
>  	tmp = mmVM_L2_CNTL4_DEFAULT;
> --
> 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 2/3] drm/amdgpu: only bind VM shadows after validation v2

[Felix Kuehling]

Hi Christian,

If I'm reading this correctly, now you're only binding page directories,
but not the leaf page tables. Is that intentional?

Regards,
  Felix


On 17-07-29 07:32 AM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> No need to do this on every CS.
>
> v2: remove all other bind, reorder code
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a375135..0308bb4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -165,6 +165,14 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>  	unsigned i;
>  	int r;
>  
> +	if (parent->bo->shadow) {
> +		struct amdgpu_bo *shadow = parent->bo->shadow;
> +
> +		r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
> +		if (r)
> +			return r;
> +	}
> +
>  	if (use_cpu_for_update) {
>  		r = amdgpu_bo_kmap(parent->bo, NULL);
>  		if (r)
> @@ -1030,11 +1038,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>  
>  		params.func = amdgpu_vm_cpu_set_ptes;
>  	} else {
> -		if (shadow) {
> -			r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
> -			if (r)
> -				return r;
> -		}
>  		ring = container_of(vm->entity.sched, struct amdgpu_ring,
>  				    sched);
>  
> @@ -1070,15 +1073,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>  		if (bo == NULL)
>  			continue;
>  
> -		if (bo->shadow) {
> -			struct amdgpu_bo *pt_shadow = bo->shadow;
> -
> -			r = amdgpu_ttm_bind(&pt_shadow->tbo,
> -					    &pt_shadow->tbo.mem);
> -			if (r)
> -				return r;
> -		}
> -
>  		pt = amdgpu_bo_gpu_offset(bo);
>  		pt = amdgpu_gart_get_vm_pde(adev, pt);
>  		if (parent->entries[pt_idx].addr == pt ||

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

Re: [PATCH 1/3] drm/amdgpu: only move VM BOs in the LRU during validation v2

[Felix Kuehling]

Patches 1 and 3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

I'm not sure about Patch 2. See my comment in reply to that patch.

Regards,
  Felix


On 17-07-29 07:32 AM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> This should save us a bunch of command submission overhead.
>
> v2: move the LRU move to the right place to avoid the move for the root BO
>     and handle the shadow BOs as well. This turned out to be a bug fix because
>     the move needs to happen before the kmap.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> (v1)
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 15 +++------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 58 +++++++---------------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 --
>  3 files changed, 16 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index cd5c08a..7fb4baa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -669,10 +669,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  	}
>  
>  error_validate:
> -	if (r) {
> -		amdgpu_vm_move_pt_bos_in_lru(p->adev, &fpriv->vm);
> +	if (r)
>  		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
> -	}
>  
>  error_free_pages:
>  
> @@ -720,21 +718,18 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>   * If error is set than unvalidate buffer, otherwise just free memory
>   * used by parsing context.
>   **/
> -static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error, bool backoff)
> +static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
> +				  bool backoff)
>  {
> -	struct amdgpu_fpriv *fpriv = parser->filp->driver_priv;
>  	unsigned i;
>  
> -	if (!error) {
> -		amdgpu_vm_move_pt_bos_in_lru(parser->adev, &fpriv->vm);
> -
> +	if (!error)
>  		ttm_eu_fence_buffer_objects(&parser->ticket,
>  					    &parser->validated,
>  					    parser->fence);
> -	} else if (backoff) {
> +	else if (backoff)
>  		ttm_eu_backoff_reservation(&parser->ticket,
>  					   &parser->validated);
> -	}
>  	dma_fence_put(parser->fence);
>  
>  	if (parser->ctx)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a1d4294..a375135 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -159,7 +159,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   */
>  static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>  				    int (*validate)(void *, struct amdgpu_bo *),
> -				    void *param, bool use_cpu_for_update)
> +				    void *param, bool use_cpu_for_update,
> +				    struct ttm_bo_global *glob)
>  {
>  	unsigned i;
>  	int r;
> @@ -183,12 +184,18 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>  		if (r)
>  			return r;
>  
> +		spin_lock(&glob->lru_lock);
> +		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
> +		if (entry->bo->shadow)
> +			ttm_bo_move_to_lru_tail(&entry->bo->shadow->tbo);
> +		spin_unlock(&glob->lru_lock);
> +
>  		/*
>  		 * Recurse into the sub directory. This is harmless because we
>  		 * have only a maximum of 5 layers.
>  		 */
>  		r = amdgpu_vm_validate_level(entry, validate, param,
> -					     use_cpu_for_update);
> +					     use_cpu_for_update, glob);
>  		if (r)
>  			return r;
>  	}
> @@ -220,54 +227,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		return 0;
>  
>  	return amdgpu_vm_validate_level(&vm->root, validate, param,
> -					vm->use_cpu_for_update);
> +					vm->use_cpu_for_update,
> +					adev->mman.bdev.glob);
>  }
>  
>  /**
> - * amdgpu_vm_move_level_in_lru - move one level of PT BOs to the LRU tail
> - *
> - * @adev: amdgpu device instance
> - * @vm: vm providing the BOs
> - *
> - * Move the PT BOs to the tail of the LRU.
> - */
> -static void amdgpu_vm_move_level_in_lru(struct amdgpu_vm_pt *parent)
> -{
> -	unsigned i;
> -
> -	if (!parent->entries)
> -		return;
> -
> -	for (i = 0; i <= parent->last_entry_used; ++i) {
> -		struct amdgpu_vm_pt *entry = &parent->entries[i];
> -
> -		if (!entry->bo)
> -			continue;
> -
> -		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
> -		amdgpu_vm_move_level_in_lru(entry);
> -	}
> -}
> -
> -/**
> - * amdgpu_vm_move_pt_bos_in_lru - move the PT BOs to the LRU tail
> - *
> - * @adev: amdgpu device instance
> - * @vm: vm providing the BOs
> - *
> - * Move the PT BOs to the tail of the LRU.
> - */
> -void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
> -				  struct amdgpu_vm *vm)
> -{
> -	struct ttm_bo_global *glob = adev->mman.bdev.glob;
> -
> -	spin_lock(&glob->lru_lock);
> -	amdgpu_vm_move_level_in_lru(&vm->root);
> -	spin_unlock(&glob->lru_lock);
> -}
> -
> - /**
>   * amdgpu_vm_alloc_levels - allocate the PD/PT levels
>   *
>   * @adev: amdgpu_device pointer
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 34d9174..bac09ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -220,8 +220,6 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>  int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  			      int (*callback)(void *p, struct amdgpu_bo *bo),
>  			      void *param);
> -void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
> -				  struct amdgpu_vm *vm);
>  int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>  			struct amdgpu_vm *vm,
>  			uint64_t saddr, uint64_t size);

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

Re: [PATCH 2/3] drm/amdgpu: only bind VM shadows after validation v2

[Christian König]

Am 31.07.2017 um 17:24 schrieb Felix Kuehling:
> Hi Christian,
>
> If I'm reading this correctly, now you're only binding page directories,
> but not the leaf page tables. Is that intentional?

You are not reading this correctly I think.

Leave page tables don have parent->entries==NULL and we check for that 
condition after the bind as far as I can see.

Regards,
Christian.

>
> Regards,
>    Felix
>
>
> On 17-07-29 07:32 AM, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> No need to do this on every CS.
>>
>> v2: remove all other bind, reorder code
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++--------------
>>   1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index a375135..0308bb4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -165,6 +165,14 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>>   	unsigned i;
>>   	int r;
>>   
>> +	if (parent->bo->shadow) {
>> +		struct amdgpu_bo *shadow = parent->bo->shadow;
>> +
>> +		r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
>> +		if (r)
>> +			return r;
>> +	}
>> +
>>   	if (use_cpu_for_update) {
>>   		r = amdgpu_bo_kmap(parent->bo, NULL);
>>   		if (r)
>> @@ -1030,11 +1038,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>>   
>>   		params.func = amdgpu_vm_cpu_set_ptes;
>>   	} else {
>> -		if (shadow) {
>> -			r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
>> -			if (r)
>> -				return r;
>> -		}
>>   		ring = container_of(vm->entity.sched, struct amdgpu_ring,
>>   				    sched);
>>   
>> @@ -1070,15 +1073,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>>   		if (bo == NULL)
>>   			continue;
>>   
>> -		if (bo->shadow) {
>> -			struct amdgpu_bo *pt_shadow = bo->shadow;
>> -
>> -			r = amdgpu_ttm_bind(&pt_shadow->tbo,
>> -					    &pt_shadow->tbo.mem);
>> -			if (r)
>> -				return r;
>> -		}
>> -
>>   		pt = amdgpu_bo_gpu_offset(bo);
>>   		pt = amdgpu_gart_get_vm_pde(adev, pt);
>>   		if (parent->entries[pt_idx].addr == pt ||


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

Re: [PATCH 2/3] drm/amdgpu: only bind VM shadows after validation v2

[Felix Kuehling]

On 17-07-31 11:30 AM, Christian König wrote:
> Am 31.07.2017 um 17:24 schrieb Felix Kuehling:
>> Hi Christian,
>>
>> If I'm reading this correctly, now you're only binding page directories,
>> but not the leaf page tables. Is that intentional?
>
> You are not reading this correctly I think.
>
> Leave page tables don have parent->entries==NULL and we check for that
> condition after the bind as far as I can see.

Yes, I read it wrong. Never mind. Reviewed-by: Felix Kuehling
<Felix.Kuehling@amd.com>


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