[PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[Felix Kuehling]

The intent of two commits was lost in the last rebase:

810955b drm/amdgpu: Fix acquiring VM on large-BAR systems
b5d21aa drm/amdgpu: Don't use shadow BO for compute context

This commit restores the original behaviour:
* Don't set AMDGPU_GEM_CREATE_NO_CPU_ACCESS for page directories
  to allow them to be reused for compute VMs
* Don't create shadow BOs for page tables in compute VMs

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ea5e277..5e7a3de 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -577,10 +577,13 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
  *
  * @adev: amdgpu_device pointer
  * @vm: requesting vm
+ * @level: level in the page table hierarchy
+ * @no_shadow: disable creation of shadow BO for this VM
  * @bp: resulting BO allocation parameters
  */
 static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-			       int level, struct amdgpu_bo_param *bp)
+			       int level, bool no_shadow,
+			       struct amdgpu_bo_param *bp)
 {
 	memset(bp, 0, sizeof(*bp));
 
@@ -595,9 +598,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
 	if (vm->use_cpu_for_update)
 		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
-	else
-		bp->flags |= AMDGPU_GEM_CREATE_SHADOW |
-			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
+	else if (!no_shadow)
+		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
 	bp->type = ttm_bo_type_kernel;
 	if (vm->root.base.bo)
 		bp->resv = vm->root.base.bo->tbo.resv;
@@ -626,6 +628,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 				  unsigned level, bool ats)
 {
 	unsigned shift = amdgpu_vm_level_shift(adev, level);
+	bool no_shadow = !vm->root.base.bo->shadow;
 	struct amdgpu_bo_param bp;
 	unsigned pt_idx, from, to;
 	int r;
@@ -650,7 +653,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 	saddr = saddr & ((1 << shift) - 1);
 	eaddr = eaddr & ((1 << shift) - 1);
 
-	amdgpu_vm_bo_param(adev, vm, level, &bp);
+	amdgpu_vm_bo_param(adev, vm, level, no_shadow, &bp);
 
 	/* walk over the address space and allocate the page tables */
 	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
@@ -2709,6 +2712,7 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		   int vm_context, unsigned int pasid)
 {
+	bool no_shadow = (vm_context == AMDGPU_VM_CONTEXT_COMPUTE);
 	struct amdgpu_bo_param bp;
 	struct amdgpu_bo *root;
 	int r, i;
@@ -2748,7 +2752,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		  "CPU update of VM recommended only for large BAR system\n");
 	vm->last_update = NULL;
 
-	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
+	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, no_shadow,
+			   &bp);
 	r = amdgpu_bo_create(adev, &bp, &root);
 	if (r)
 		goto error_free_sched_entity;
-- 
2.7.4

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

Re: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[Zhang, Jerry (Junwei)]

On 09/06/2018 08:28 AM, Felix Kuehling wrote:
> The intent of two commits was lost in the last rebase:
>
> 810955b drm/amdgpu: Fix acquiring VM on large-BAR systems
> b5d21aa drm/amdgpu: Don't use shadow BO for compute context
>
> This commit restores the original behaviour:
> * Don't set AMDGPU_GEM_CREATE_NO_CPU_ACCESS for page directories
>    to allow them to be reused for compute VMs
> * Don't create shadow BOs for page tables in compute VMs
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>

Personally making no_shadow -> shadow looks more simple.
anyway, that patches restoring what it's missing.

Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ea5e277..5e7a3de 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -577,10 +577,13 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requesting vm
> + * @level: level in the page table hierarchy
> + * @no_shadow: disable creation of shadow BO for this VM
>    * @bp: resulting BO allocation parameters
>    */
>   static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			       int level, struct amdgpu_bo_param *bp)
> +			       int level, bool no_shadow,
> +			       struct amdgpu_bo_param *bp)
>   {
>   	memset(bp, 0, sizeof(*bp));
>
> @@ -595,9 +598,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>   	if (vm->use_cpu_for_update)
>   		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -	else
> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW |
> -			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
> +	else if (!no_shadow)
> +		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
>   	bp->type = ttm_bo_type_kernel;
>   	if (vm->root.base.bo)
>   		bp->resv = vm->root.base.bo->tbo.resv;
> @@ -626,6 +628,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   				  unsigned level, bool ats)
>   {
>   	unsigned shift = amdgpu_vm_level_shift(adev, level);
> +	bool no_shadow = !vm->root.base.bo->shadow;
>   	struct amdgpu_bo_param bp;
>   	unsigned pt_idx, from, to;
>   	int r;
> @@ -650,7 +653,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   	saddr = saddr & ((1 << shift) - 1);
>   	eaddr = eaddr & ((1 << shift) - 1);
>
> -	amdgpu_vm_bo_param(adev, vm, level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, level, no_shadow, &bp);
>
>   	/* walk over the address space and allocate the page tables */
>   	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
> @@ -2709,6 +2712,7 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, unsigned int pasid)
>   {
> +	bool no_shadow = (vm_context == AMDGPU_VM_CONTEXT_COMPUTE);
>   	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *root;
>   	int r, i;
> @@ -2748,7 +2752,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		  "CPU update of VM recommended only for large BAR system\n");
>   	vm->last_update = NULL;
>
> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, no_shadow,
> +			   &bp);
>   	r = amdgpu_bo_create(adev, &bp, &root);
>   	if (r)
>   		goto error_free_sched_entity;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[zhoucm1]

On 2018年09月06日 08:28, Felix Kuehling wrote:
> The intent of two commits was lost in the last rebase:
>
> 810955b drm/amdgpu: Fix acquiring VM on large-BAR systems
> b5d21aa drm/amdgpu: Don't use shadow BO for compute context
>
> This commit restores the original behaviour:
> * Don't set AMDGPU_GEM_CREATE_NO_CPU_ACCESS for page directories
>    to allow them to be reused for compute VMs
> * Don't create shadow BOs for page tables in compute VMs
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ea5e277..5e7a3de 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -577,10 +577,13 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requesting vm
> + * @level: level in the page table hierarchy
> + * @no_shadow: disable creation of shadow BO for this VM
>    * @bp: resulting BO allocation parameters
>    */
>   static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			       int level, struct amdgpu_bo_param *bp)
> +			       int level, bool no_shadow,
> +			       struct amdgpu_bo_param *bp)
How about adding no_shadow to bp? Which also can consider to be parm of bo.

Regards,
David Zhou
>   {
>   	memset(bp, 0, sizeof(*bp));
>   
> @@ -595,9 +598,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>   	if (vm->use_cpu_for_update)
>   		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -	else
> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW |
> -			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
> +	else if (!no_shadow)
> +		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
>   	bp->type = ttm_bo_type_kernel;
>   	if (vm->root.base.bo)
>   		bp->resv = vm->root.base.bo->tbo.resv;
> @@ -626,6 +628,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   				  unsigned level, bool ats)
>   {
>   	unsigned shift = amdgpu_vm_level_shift(adev, level);
> +	bool no_shadow = !vm->root.base.bo->shadow;
>   	struct amdgpu_bo_param bp;
>   	unsigned pt_idx, from, to;
>   	int r;
> @@ -650,7 +653,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   	saddr = saddr & ((1 << shift) - 1);
>   	eaddr = eaddr & ((1 << shift) - 1);
>   
> -	amdgpu_vm_bo_param(adev, vm, level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, level, no_shadow, &bp);
>   
>   	/* walk over the address space and allocate the page tables */
>   	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
> @@ -2709,6 +2712,7 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, unsigned int pasid)
>   {
> +	bool no_shadow = (vm_context == AMDGPU_VM_CONTEXT_COMPUTE);
>   	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *root;
>   	int r, i;
> @@ -2748,7 +2752,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		  "CPU update of VM recommended only for large BAR system\n");
>   	vm->last_update = NULL;
>   
> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, no_shadow,
> +			   &bp);
>   	r = amdgpu_bo_create(adev, &bp, &root);
>   	if (r)
>   		goto error_free_sched_entity;

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

Re: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[Christian König]

Am 06.09.2018 um 02:28 schrieb Felix Kuehling:
> The intent of two commits was lost in the last rebase:
>
> 810955b drm/amdgpu: Fix acquiring VM on large-BAR systems
> b5d21aa drm/amdgpu: Don't use shadow BO for compute context
>
> This commit restores the original behaviour:
> * Don't set AMDGPU_GEM_CREATE_NO_CPU_ACCESS for page directories
>    to allow them to be reused for compute VMs
> * Don't create shadow BOs for page tables in compute VMs
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ea5e277..5e7a3de 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -577,10 +577,13 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requesting vm
> + * @level: level in the page table hierarchy
> + * @no_shadow: disable creation of shadow BO for this VM
>    * @bp: resulting BO allocation parameters
>    */
>   static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			       int level, struct amdgpu_bo_param *bp)
> +			       int level, bool no_shadow,
> +			       struct amdgpu_bo_param *bp)
>   {
>   	memset(bp, 0, sizeof(*bp));
>   
> @@ -595,9 +598,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>   	if (vm->use_cpu_for_update)
>   		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -	else
> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW |
> -			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
> +	else if (!no_shadow)
> +		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
>   	bp->type = ttm_bo_type_kernel;
>   	if (vm->root.base.bo)
>   		bp->resv = vm->root.base.bo->tbo.resv;
> @@ -626,6 +628,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   				  unsigned level, bool ats)
>   {
>   	unsigned shift = amdgpu_vm_level_shift(adev, level);
> +	bool no_shadow = !vm->root.base.bo->shadow;

Please move that logic into amdgpu_vm_bo_param().

Christian.

>   	struct amdgpu_bo_param bp;
>   	unsigned pt_idx, from, to;
>   	int r;
> @@ -650,7 +653,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   	saddr = saddr & ((1 << shift) - 1);
>   	eaddr = eaddr & ((1 << shift) - 1);
>   
> -	amdgpu_vm_bo_param(adev, vm, level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, level, no_shadow, &bp);
>   
>   	/* walk over the address space and allocate the page tables */
>   	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
> @@ -2709,6 +2712,7 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, unsigned int pasid)
>   {
> +	bool no_shadow = (vm_context == AMDGPU_VM_CONTEXT_COMPUTE);
>   	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *root;
>   	int r, i;
> @@ -2748,7 +2752,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		  "CPU update of VM recommended only for large BAR system\n");
>   	vm->last_update = NULL;
>   
> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, no_shadow,
> +			   &bp);
>   	r = amdgpu_bo_create(adev, &bp, &root);
>   	if (r)
>   		goto error_free_sched_entity;

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

RE: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[Russell, Kent]

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Thursday, September 06, 2018 3:36 AM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Russell, Kent <Kent.Russell@amd.com>
Subject: Re: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

Am 06.09.2018 um 02:28 schrieb Felix Kuehling:
> The intent of two commits was lost in the last rebase:
>
> 810955b drm/amdgpu: Fix acquiring VM on large-BAR systems b5d21aa 
> drm/amdgpu: Don't use shadow BO for compute context
>
> This commit restores the original behaviour:
> * Don't set AMDGPU_GEM_CREATE_NO_CPU_ACCESS for page directories
>    to allow them to be reused for compute VMs
> * Don't create shadow BOs for page tables in compute VMs
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ea5e277..5e7a3de 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -577,10 +577,13 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requesting vm
> + * @level: level in the page table hierarchy
> + * @no_shadow: disable creation of shadow BO for this VM
>    * @bp: resulting BO allocation parameters
>    */
>   static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			       int level, struct amdgpu_bo_param *bp)
> +			       int level, bool no_shadow,
> +			       struct amdgpu_bo_param *bp)
>   {
>   	memset(bp, 0, sizeof(*bp));
>   
> @@ -595,9 +598,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>   	if (vm->use_cpu_for_update)
>   		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -	else
> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW |
> -			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
> +	else if (!no_shadow)
> +		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
>   	bp->type = ttm_bo_type_kernel;
>   	if (vm->root.base.bo)
>   		bp->resv = vm->root.base.bo->tbo.resv; @@ -626,6 +628,7 @@ static 
> int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   				  unsigned level, bool ats)
>   {
>   	unsigned shift = amdgpu_vm_level_shift(adev, level);
> +	bool no_shadow = !vm->root.base.bo->shadow;

Please move that logic into amdgpu_vm_bo_param().
[KR] The issue with that is that in the other call below, we don't have access to the vm context via the vm struct, so we can't move the logic in for both calls.

Christian.

>   	struct amdgpu_bo_param bp;
>   	unsigned pt_idx, from, to;
>   	int r;
> @@ -650,7 +653,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   	saddr = saddr & ((1 << shift) - 1);
>   	eaddr = eaddr & ((1 << shift) - 1);
>   
> -	amdgpu_vm_bo_param(adev, vm, level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, level, no_shadow, &bp);
>   
>   	/* walk over the address space and allocate the page tables */
>   	for (pt_idx = from; pt_idx <= to; ++pt_idx) { @@ -2709,6 +2712,7 @@ 
> void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, unsigned int pasid)
>   {
> +	bool no_shadow = (vm_context == AMDGPU_VM_CONTEXT_COMPUTE);
>   	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *root;
>   	int r, i;
> @@ -2748,7 +2752,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		  "CPU update of VM recommended only for large BAR system\n");
>   	vm->last_update = NULL;
>   
> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
> +	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, no_shadow,
> +			   &bp);
>   	r = amdgpu_bo_create(adev, &bp, &root);
>   	if (r)
>   		goto error_free_sched_entity;

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

Re: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase

[Christian König]

Am 06.09.2018 um 13:46 schrieb Russell, Kent:
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Thursday, September 06, 2018 3:36 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Russell, Kent <Kent.Russell@amd.com>
> Subject: Re: [PATCH 1/1] drm/amdgpu: Fix compute VM BO params after rebase
>
> Am 06.09.2018 um 02:28 schrieb Felix Kuehling:
>> The intent of two commits was lost in the last rebase:
>>
>> 810955b drm/amdgpu: Fix acquiring VM on large-BAR systems b5d21aa
>> drm/amdgpu: Don't use shadow BO for compute context
>>
>> This commit restores the original behaviour:
>> * Don't set AMDGPU_GEM_CREATE_NO_CPU_ACCESS for page directories
>>     to allow them to be reused for compute VMs
>> * Don't create shadow BOs for page tables in compute VMs
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
>>    1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index ea5e277..5e7a3de 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -577,10 +577,13 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>>     *
>>     * @adev: amdgpu_device pointer
>>     * @vm: requesting vm
>> + * @level: level in the page table hierarchy
>> + * @no_shadow: disable creation of shadow BO for this VM
>>     * @bp: resulting BO allocation parameters
>>     */
>>    static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> -			       int level, struct amdgpu_bo_param *bp)
>> +			       int level, bool no_shadow,
>> +			       struct amdgpu_bo_param *bp)
>>    {
>>    	memset(bp, 0, sizeof(*bp));
>>    
>> @@ -595,9 +598,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>    	if (vm->use_cpu_for_update)
>>    		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>> -	else
>> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW |
>> -			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
>> +	else if (!no_shadow)
>> +		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
>>    	bp->type = ttm_bo_type_kernel;
>>    	if (vm->root.base.bo)
>>    		bp->resv = vm->root.base.bo->tbo.resv; @@ -626,6 +628,7 @@ static
>> int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>>    				  unsigned level, bool ats)
>>    {
>>    	unsigned shift = amdgpu_vm_level_shift(adev, level);
>> +	bool no_shadow = !vm->root.base.bo->shadow;
> Please move that logic into amdgpu_vm_bo_param().
> [KR] The issue with that is that in the other call below, we don't have access to the vm context via the vm struct, so we can't move the logic in for both calls.

Yeah, but you can look at the root PD (if present) and see if it has a 
shadow BO.

And for the root PD we can clear the flag manually after calling 
amdgpu_vm_bo_param().

Christian.

>
> Christian.
>
>>    	struct amdgpu_bo_param bp;
>>    	unsigned pt_idx, from, to;
>>    	int r;
>> @@ -650,7 +653,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>>    	saddr = saddr & ((1 << shift) - 1);
>>    	eaddr = eaddr & ((1 << shift) - 1);
>>    
>> -	amdgpu_vm_bo_param(adev, vm, level, &bp);
>> +	amdgpu_vm_bo_param(adev, vm, level, no_shadow, &bp);
>>    
>>    	/* walk over the address space and allocate the page tables */
>>    	for (pt_idx = from; pt_idx <= to; ++pt_idx) { @@ -2709,6 +2712,7 @@
>> void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>>    int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    		   int vm_context, unsigned int pasid)
>>    {
>> +	bool no_shadow = (vm_context == AMDGPU_VM_CONTEXT_COMPUTE);
>>    	struct amdgpu_bo_param bp;
>>    	struct amdgpu_bo *root;
>>    	int r, i;
>> @@ -2748,7 +2752,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    		  "CPU update of VM recommended only for large BAR system\n");
>>    	vm->last_update = NULL;
>>    
>> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
>> +	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, no_shadow,
>> +			   &bp);
>>    	r = amdgpu_bo_create(adev, &bp, &root);
>>    	if (r)
>>    		goto error_free_sched_entity;

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