Re: [PATCH v2 1/1] drm/amdkfd: use allowed domain for vmbo validation

2021-06-08 Thread Das, Nirmoy 


On 6/8/2021 5:28 PM, Zeng, Oak wrote:

Hi Nirmoy,

Why keep a unused parameter: +static int amdgpu_amdkfd_validate_vm_bo(void 
*_unused.


We pass this func to amdgpu_vm_validate_pt_bos() which requires two 
args: int (*validate)(void *p, struct amdgpu_bo *bo)





When I looked the codes, the only logic change is the validate page table bo in 
allowed_domain instead of vram domain. Can you explain why validate page table 
bo in vram domain cause a problem? When I looked at the codes, we only place 
page table in GTT domain when vram size is too small (function 
amdgpu_bo_get_preferred_pin_domain). Is there any other case we place page 
table in GTT?


Yes. I don't think there is any other trigger for placing page-table to 
to GTT.  I haven't observed  any issue without this patch, yet. It is 
just replacing hard-coded domain value with a proper/allowed ones.



Regards,

Nirmoy




Regards,
Oak

  


On 2021-06-08, 8:02 AM, "amd-gfx on behalf of Das, Nirmoy" 
 wrote:


 On 6/8/2021 1:42 PM, Christian König wrote:
 >
 >
 > Am 08.06.21 um 13:27 schrieb Nirmoy Das:
 >> Fixes handling when page tables are in system memory.
 >>
 >> v2: remove unwanted variable.
 >>  change amdgpu_amdkfd_validate instead of amdgpu_amdkfd_bo_validate.
 >>
 >> Signed-off-by: Nirmoy Das 
 >> ---
 >>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 ---
 >>   1 file changed, 4 insertions(+), 11 deletions(-)
 >>
 >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
 >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
 >> index d6cb7cf76623..021f25085760 100644
 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
 >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
 >> @@ -348,11 +348,9 @@ static int amdgpu_amdkfd_bo_validate(struct
 >> amdgpu_bo *bo, uint32_t domain,
 >>   return ret;
 >>   }
 >>
 >> -static int amdgpu_amdkfd_validate(void *param, struct amdgpu_bo *bo)
 >> +static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct
 >> amdgpu_bo *bo)
 >>   {
 >> -struct amdgpu_vm_parser *p = param;
 >
 > The structure define of amdgpu_vm_parser isn't used any more if we
 > drop this as well, isn't it?


 Right, I missed that. I will resend.


 Nirmoy

 >
 > Christian.
 >
 >> -
 >> -return amdgpu_amdkfd_bo_validate(bo, p->domain, p->wait);
 >> +return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
 >>   }
 >>
 >>   /* vm_validate_pt_pd_bos - Validate page table and directory BOs
 >> @@ -366,20 +364,15 @@ static int vm_validate_pt_pd_bos(struct
 >> amdgpu_vm *vm)
 >>   {
 >>   struct amdgpu_bo *pd = vm->root.base.bo;
 >>   struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
 >> -struct amdgpu_vm_parser param;
 >>   int ret;
 >>
 >> -param.domain = AMDGPU_GEM_DOMAIN_VRAM;
 >> -param.wait = false;
 >> -
 >> -ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate,
 >> -);
 >> +ret = amdgpu_vm_validate_pt_bos(adev, vm,
 >> amdgpu_amdkfd_validate_vm_bo, NULL);
 >>   if (ret) {
 >>   pr_err("failed to validate PT BOs\n");
 >>   return ret;
 >>   }
 >>
 >> -ret = amdgpu_amdkfd_validate(, pd);
 >> +ret = amdgpu_amdkfd_validate_vm_bo(NULL, pd);
 >>   if (ret) {
 >>   pr_err("failed to validate PD\n");
 >>   return ret;
 >> --
 >> 2.31.1
 >>
 >> ___
 >> amd-gfx mailing list
 >> amd-gfx@lists.freedesktop.org
 >> 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Coak.zeng%40amd.com%7C62e4b0ccafad4e3d110b08d92a755181%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587505641608251%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oufI2dIYs6Gx0EFuPEPgL0eYk5jrhsNwPbvDf8eBJ%2Bk%3Dreserved=0
 >>
 >
 ___
 amd-gfx mailing list
 amd-gfx@lists.freedesktop.org
 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Coak.zeng%40amd.com%7C62e4b0ccafad4e3d110b08d92a755181%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587505641608251%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oufI2dIYs6Gx0EFuPEPgL0eYk5jrhsNwPbvDf8eBJ%2Bk%3Dreserved=0


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

Re: [PATCH v2 1/1] drm/amdkfd: use allowed domain for vmbo validation

2021-06-08 Thread Zeng, Oak 
Hi Nirmoy,

Why keep a unused parameter: +static int amdgpu_amdkfd_validate_vm_bo(void 
*_unused.

When I looked the codes, the only logic change is the validate page table bo in 
allowed_domain instead of vram domain. Can you explain why validate page table 
bo in vram domain cause a problem? When I looked at the codes, we only place 
page table in GTT domain when vram size is too small (function 
amdgpu_bo_get_preferred_pin_domain). Is there any other case we place page 
table in GTT?

Regards,
Oak 

 

On 2021-06-08, 8:02 AM, "amd-gfx on behalf of Das, Nirmoy" 
 wrote:


On 6/8/2021 1:42 PM, Christian König wrote:
>
>
> Am 08.06.21 um 13:27 schrieb Nirmoy Das:
>> Fixes handling when page tables are in system memory.
>>
>> v2: remove unwanted variable.
>>  change amdgpu_amdkfd_validate instead of amdgpu_amdkfd_bo_validate.
>>
>> Signed-off-by: Nirmoy Das 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 ---
>>   1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index d6cb7cf76623..021f25085760 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -348,11 +348,9 @@ static int amdgpu_amdkfd_bo_validate(struct 
>> amdgpu_bo *bo, uint32_t domain,
>>   return ret;
>>   }
>>
>> -static int amdgpu_amdkfd_validate(void *param, struct amdgpu_bo *bo)
>> +static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct 
>> amdgpu_bo *bo)
>>   {
>> -struct amdgpu_vm_parser *p = param;
>
> The structure define of amdgpu_vm_parser isn't used any more if we 
> drop this as well, isn't it?


Right, I missed that. I will resend.


Nirmoy

>
> Christian.
>
>> -
>> -return amdgpu_amdkfd_bo_validate(bo, p->domain, p->wait);
>> +return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
>>   }
>>
>>   /* vm_validate_pt_pd_bos - Validate page table and directory BOs
>> @@ -366,20 +364,15 @@ static int vm_validate_pt_pd_bos(struct 
>> amdgpu_vm *vm)
>>   {
>>   struct amdgpu_bo *pd = vm->root.base.bo;
>>   struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
>> -struct amdgpu_vm_parser param;
>>   int ret;
>>
>> -param.domain = AMDGPU_GEM_DOMAIN_VRAM;
>> -param.wait = false;
>> -
>> -ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate,
>> -);
>> +ret = amdgpu_vm_validate_pt_bos(adev, vm, 
>> amdgpu_amdkfd_validate_vm_bo, NULL);
>>   if (ret) {
>>   pr_err("failed to validate PT BOs\n");
>>   return ret;
>>   }
>>
>> -ret = amdgpu_amdkfd_validate(, pd);
>> +ret = amdgpu_amdkfd_validate_vm_bo(NULL, pd);
>>   if (ret) {
>>   pr_err("failed to validate PD\n");
>>   return ret;
>> -- 
>> 2.31.1
>>
>> ___
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Coak.zeng%40amd.com%7C62e4b0ccafad4e3d110b08d92a755181%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587505641608251%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oufI2dIYs6Gx0EFuPEPgL0eYk5jrhsNwPbvDf8eBJ%2Bk%3Dreserved=0
 
>>
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Coak.zeng%40amd.com%7C62e4b0ccafad4e3d110b08d92a755181%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587505641608251%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oufI2dIYs6Gx0EFuPEPgL0eYk5jrhsNwPbvDf8eBJ%2Bk%3Dreserved=0

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

Re: [PATCH v2 1/1] drm/amdkfd: use allowed domain for vmbo validation

2021-06-08 Thread Das, Nirmoy 


On 6/8/2021 1:42 PM, Christian König wrote:



Am 08.06.21 um 13:27 schrieb Nirmoy Das:

Fixes handling when page tables are in system memory.

v2: remove unwanted variable.
 change amdgpu_amdkfd_validate instead of amdgpu_amdkfd_bo_validate.

Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 ---
  1 file changed, 4 insertions(+), 11 deletions(-)

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

index d6cb7cf76623..021f25085760 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -348,11 +348,9 @@ static int amdgpu_amdkfd_bo_validate(struct 
amdgpu_bo *bo, uint32_t domain,

  return ret;
  }

-static int amdgpu_amdkfd_validate(void *param, struct amdgpu_bo *bo)
+static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct 
amdgpu_bo *bo)

  {
-    struct amdgpu_vm_parser *p = param;


The structure define of amdgpu_vm_parser isn't used any more if we 
drop this as well, isn't it?



Right, I missed that. I will resend.


Nirmoy



Christian.


-
-    return amdgpu_amdkfd_bo_validate(bo, p->domain, p->wait);
+    return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
  }

  /* vm_validate_pt_pd_bos - Validate page table and directory BOs
@@ -366,20 +364,15 @@ static int vm_validate_pt_pd_bos(struct 
amdgpu_vm *vm)

  {
  struct amdgpu_bo *pd = vm->root.base.bo;
  struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
-    struct amdgpu_vm_parser param;
  int ret;

-    param.domain = AMDGPU_GEM_DOMAIN_VRAM;
-    param.wait = false;
-
-    ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate,
-    );
+    ret = amdgpu_vm_validate_pt_bos(adev, vm, 
amdgpu_amdkfd_validate_vm_bo, NULL);

  if (ret) {
  pr_err("failed to validate PT BOs\n");
  return ret;
  }

-    ret = amdgpu_amdkfd_validate(, pd);
+    ret = amdgpu_amdkfd_validate_vm_bo(NULL, pd);
  if (ret) {
  pr_err("failed to validate PD\n");
  return ret;
--
2.31.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cnirmoy.das%40amd.com%7C595092a055e946bafa8608d92a728034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587493530829021%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=7XWT%2FcJ4SnWG85KEeOawECtwq%2BMcEOqoo88o01S0X5g%3Dreserved=0 




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

[PATCH v2 1/1] drm/amdkfd: use allowed domain for vmbo validation

2021-06-08 Thread Nirmoy Das 
Fixes handling when page tables are in system memory.

v2: remove unwanted variable.
change amdgpu_amdkfd_validate instead of amdgpu_amdkfd_bo_validate.

Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index d6cb7cf76623..021f25085760 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -348,11 +348,9 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, 
uint32_t domain,
return ret;
 }

-static int amdgpu_amdkfd_validate(void *param, struct amdgpu_bo *bo)
+static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)
 {
-   struct amdgpu_vm_parser *p = param;
-
-   return amdgpu_amdkfd_bo_validate(bo, p->domain, p->wait);
+   return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
 }

 /* vm_validate_pt_pd_bos - Validate page table and directory BOs
@@ -366,20 +364,15 @@ static int vm_validate_pt_pd_bos(struct amdgpu_vm *vm)
 {
struct amdgpu_bo *pd = vm->root.base.bo;
struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
-   struct amdgpu_vm_parser param;
int ret;

-   param.domain = AMDGPU_GEM_DOMAIN_VRAM;
-   param.wait = false;
-
-   ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate,
-   );
+   ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate_vm_bo, 
NULL);
if (ret) {
pr_err("failed to validate PT BOs\n");
return ret;
}

-   ret = amdgpu_amdkfd_validate(, pd);
+   ret = amdgpu_amdkfd_validate_vm_bo(NULL, pd);
if (ret) {
pr_err("failed to validate PD\n");
return ret;
--
2.31.1

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

Re: [PATCH v2 1/1] drm/amdkfd: use allowed domain for vmbo validation

2021-06-08 Thread Christian König 




Am 08.06.21 um 13:27 schrieb Nirmoy Das:

Fixes handling when page tables are in system memory.

v2: remove unwanted variable.
 change amdgpu_amdkfd_validate instead of amdgpu_amdkfd_bo_validate.

Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 ---
  1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index d6cb7cf76623..021f25085760 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -348,11 +348,9 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, 
uint32_t domain,
return ret;
  }

-static int amdgpu_amdkfd_validate(void *param, struct amdgpu_bo *bo)
+static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)
  {
-   struct amdgpu_vm_parser *p = param;


The structure define of amdgpu_vm_parser isn't used any more if we drop 
this as well, isn't it?


Christian.


-
-   return amdgpu_amdkfd_bo_validate(bo, p->domain, p->wait);
+   return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
  }

  /* vm_validate_pt_pd_bos - Validate page table and directory BOs
@@ -366,20 +364,15 @@ static int vm_validate_pt_pd_bos(struct amdgpu_vm *vm)
  {
struct amdgpu_bo *pd = vm->root.base.bo;
struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
-   struct amdgpu_vm_parser param;
int ret;

-   param.domain = AMDGPU_GEM_DOMAIN_VRAM;
-   param.wait = false;
-
-   ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate,
-   );
+   ret = amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_amdkfd_validate_vm_bo, 
NULL);
if (ret) {
pr_err("failed to validate PT BOs\n");
return ret;
}

-   ret = amdgpu_amdkfd_validate(, pd);
+   ret = amdgpu_amdkfd_validate_vm_bo(NULL, pd);
if (ret) {
pr_err("failed to validate PD\n");
return ret;
--
2.31.1

___
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