* [PATCH 1/2] drm/amdgpu: reverse PDBs order
@ 2017-12-08 10:56 Chunming Zhou
[not found] ` <20171208105610.26728-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Chunming Zhou @ 2017-12-08 10:56 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Chunming Zhou
The hiberachy of page table is as below, which aligns hw names.
PDB2->PDB1->PDB0->PTB, accordingly:
level3 --- PDB2
level2 --- PDB1
level1 --- PDB0
level0 --- PTB
Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 ++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3ecdbdfb04dd..8904ccf78fc9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
unsigned level)
{
- if (level != adev->vm_manager.num_level)
- return 9 * (adev->vm_manager.num_level - level - 1) +
+ if (level != 0)
+ return 9 * (level - 1) +
adev->vm_manager.block_size;
else
/* For the page tables on the leaves */
@@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
* @adev: amdgpu_device pointer
*
* Calculate the number of entries in a page directory or page table.
+ * The hiberachy of page table is as below, which aligns hw names.
+ * PDB2->PDB1->PDB0->PTB, accordingly:
+ * level3 --- PDB2
+ * level2 --- PDB1
+ * level1 --- PDB0
+ * level0 --- PTB
*/
static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
unsigned level)
{
- unsigned shift = amdgpu_vm_level_shift(adev, 0);
+ unsigned shift = amdgpu_vm_level_shift(adev,
+ adev->vm_manager.num_level);
- if (level == 0)
+ if (level == adev->vm_manager.num_level)
/* For the root directory */
return round_up(adev->vm_manager.max_pfn, 1 << shift) >> shift;
- else if (level != adev->vm_manager.num_level)
+ else if (level != 0)
/* Everything in between */
return 512;
else
- /* For the page tables on the leaves */
+ /* For the page tables on the leaves(PTB) */
return AMDGPU_VM_PTE_COUNT(adev);
}
@@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
u64 flags;
uint64_t init_value = 0;
+ BUG_ON(level > adev->vm_manager.num_level);
+
if (!parent->entries) {
unsigned num_entries = amdgpu_vm_num_entries(adev, level);
@@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
if (to > parent->last_entry_used)
parent->last_entry_used = to;
- ++level;
+ level--;
saddr = saddr & ((1 << shift) - 1);
eaddr = eaddr & ((1 << shift) - 1);
@@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
if (vm->pte_support_ats) {
init_value = AMDGPU_PTE_DEFAULT_ATC;
- if (level != adev->vm_manager.num_level - 1)
+ /* != PDB0 */
+ if (level != 1)
init_value |= AMDGPU_PDE_PTE;
}
@@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
entry->addr = 0;
}
- if (level < adev->vm_manager.num_level) {
+ if (level > 0) {
uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
((1 << shift) - 1);
@@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
saddr /= AMDGPU_GPU_PAGE_SIZE;
eaddr /= AMDGPU_GPU_PAGE_SIZE;
- return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr, 0);
+ return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
+ adev->vm_manager.num_level);
}
/**
@@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
struct amdgpu_vm_pt **entry,
struct amdgpu_vm_pt **parent)
{
- unsigned level = 0;
+ unsigned level = p->adev->vm_manager.num_level;
*parent = NULL;
*entry = &p->vm->root;
while ((*entry)->entries) {
- unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
+ unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
*parent = *entry;
*entry = &(*entry)->entries[idx];
}
- if (level != p->adev->vm_manager.num_level)
+ if (level != 0)
*entry = NULL;
}
--
2.14.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/amdgpu: fix pte index calculation
[not found] ` <20171208105610.26728-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
@ 2017-12-08 10:56 ` Chunming Zhou
[not found] ` <20171208105610.26728-2-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2017-12-08 14:06 ` [PATCH 1/2] drm/amdgpu: reverse PDBs order Christian König
2017-12-08 14:58 ` Alex Deucher
2 siblings, 1 reply; 11+ messages in thread
From: Chunming Zhou @ 2017-12-08 10:56 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Chunming Zhou
Change-Id: I40ecf31ad4b51022a2c0c076ae45188b6e9d63de
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8904ccf78fc9..affe64e42cef 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1335,11 +1335,13 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
*parent = NULL;
*entry = &p->vm->root;
while ((*entry)->entries) {
- unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
+ unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level);
- idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
+ idx %= amdgpu_vm_num_entries(p->adev, level);
*parent = *entry;
*entry = &(*entry)->entries[idx];
+ if (level)
+ level--;
}
if (level != 0)
--
2.14.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: fix pte index calculation
[not found] ` <20171208105610.26728-2-david1.zhou-5C7GfCeVMHo@public.gmane.org>
@ 2017-12-08 14:00 ` Christian König
[not found] ` <32026195-0596-404f-deb5-018d16c19a75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2017-12-08 14:00 UTC (permalink / raw)
To: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
What is wrong with the old approach?
I would rather say that the address should be limited by the level shift
instead. This way we avoid the modulo altogether.
Christian.
Am 08.12.2017 um 11:56 schrieb Chunming Zhou:
> Change-Id: I40ecf31ad4b51022a2c0c076ae45188b6e9d63de
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8904ccf78fc9..affe64e42cef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1335,11 +1335,13 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
> *parent = NULL;
> *entry = &p->vm->root;
> while ((*entry)->entries) {
> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level);
>
> - idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
> + idx %= amdgpu_vm_num_entries(p->adev, level);
> *parent = *entry;
> *entry = &(*entry)->entries[idx];
> + if (level)
> + level--;
> }
>
> if (level != 0)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <20171208105610.26728-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2017-12-08 10:56 ` [PATCH 2/2] drm/amdgpu: fix pte index calculation Chunming Zhou
@ 2017-12-08 14:06 ` Christian König
[not found] ` <957cec12-f5a6-16db-12d3-e3c39638afa6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-12-08 14:58 ` Alex Deucher
2 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2017-12-08 14:06 UTC (permalink / raw)
To: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Am 08.12.2017 um 11:56 schrieb Chunming Zhou:
> The hiberachy of page table is as below, which aligns hw names.
> PDB2->PDB1->PDB0->PTB, accordingly:
> level3 --- PDB2
> level2 --- PDB1
> level1 --- PDB0
> level0 --- PTB
>
> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
Yeah, thought about that as well. But I'm not sure if that is a good
idea or not.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 ++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3ecdbdfb04dd..8904ccf78fc9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
> unsigned level)
> {
> - if (level != adev->vm_manager.num_level)
> - return 9 * (adev->vm_manager.num_level - level - 1) +
> + if (level != 0)
> + return 9 * (level - 1) +
> adev->vm_manager.block_size;
> else
> /* For the page tables on the leaves */
> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
> * @adev: amdgpu_device pointer
> *
> * Calculate the number of entries in a page directory or page table.
> + * The hiberachy of page table is as below, which aligns hw names.
> + * PDB2->PDB1->PDB0->PTB, accordingly:
> + * level3 --- PDB2
> + * level2 --- PDB1
> + * level1 --- PDB0
> + * level0 --- PTB
> */
> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
> unsigned level)
> {
> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
> + unsigned shift = amdgpu_vm_level_shift(adev,
> + adev->vm_manager.num_level);
>
> - if (level == 0)
> + if (level == adev->vm_manager.num_level)
> /* For the root directory */
> return round_up(adev->vm_manager.max_pfn, 1 << shift) >> shift;
> - else if (level != adev->vm_manager.num_level)
> + else if (level != 0)
> /* Everything in between */
> return 512;
> else
> - /* For the page tables on the leaves */
> + /* For the page tables on the leaves(PTB) */
> return AMDGPU_VM_PTE_COUNT(adev);
> }
>
> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> u64 flags;
> uint64_t init_value = 0;
>
> + BUG_ON(level > adev->vm_manager.num_level);
> +
> if (!parent->entries) {
> unsigned num_entries = amdgpu_vm_num_entries(adev, level);
>
> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> if (to > parent->last_entry_used)
> parent->last_entry_used = to;
>
> - ++level;
> + level--;
Post decrement/increment please.
> saddr = saddr & ((1 << shift) - 1);
> eaddr = eaddr & ((1 << shift) - 1);
>
> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>
> if (vm->pte_support_ats) {
> init_value = AMDGPU_PTE_DEFAULT_ATC;
> - if (level != adev->vm_manager.num_level - 1)
BTW: If I'm not completely mistaken that code is incorrect and should
rather be "level != adev->vm_manager.num_level".
> + /* != PDB0 */
> + if (level != 1)
Which would that make it level != 0.
Regards,
Christian.
> init_value |= AMDGPU_PDE_PTE;
>
> }
> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> entry->addr = 0;
> }
>
> - if (level < adev->vm_manager.num_level) {
> + if (level > 0) {
> uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
> ((1 << shift) - 1);
> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> saddr /= AMDGPU_GPU_PAGE_SIZE;
> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>
> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr, 0);
> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
> + adev->vm_manager.num_level);
> }
>
> /**
> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
> struct amdgpu_vm_pt **entry,
> struct amdgpu_vm_pt **parent)
> {
> - unsigned level = 0;
> + unsigned level = p->adev->vm_manager.num_level;
>
> *parent = NULL;
> *entry = &p->vm->root;
> while ((*entry)->entries) {
> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
>
> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
> *parent = *entry;
> *entry = &(*entry)->entries[idx];
> }
>
> - if (level != p->adev->vm_manager.num_level)
> + if (level != 0)
> *entry = NULL;
> }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <20171208105610.26728-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2017-12-08 10:56 ` [PATCH 2/2] drm/amdgpu: fix pte index calculation Chunming Zhou
2017-12-08 14:06 ` [PATCH 1/2] drm/amdgpu: reverse PDBs order Christian König
@ 2017-12-08 14:58 ` Alex Deucher
[not found] ` <CADnq5_NmHKLi1kSa6NDC4feoHUZcmViML9eQvEX_M9UW-8Azww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2 siblings, 1 reply; 11+ messages in thread
From: Alex Deucher @ 2017-12-08 14:58 UTC (permalink / raw)
To: Chunming Zhou; +Cc: amd-gfx list
On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.zhou@amd.com> wrote:
> The hiberachy of page table is as below, which aligns hw names.
> PDB2->PDB1->PDB0->PTB, accordingly:
> level3 --- PDB2
> level2 --- PDB1
> level1 --- PDB0
> level0 --- PTB
What's the advantage of this change? It's not clear from the commit message.
Alex
>
> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 ++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3ecdbdfb04dd..8904ccf78fc9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
> unsigned level)
> {
> - if (level != adev->vm_manager.num_level)
> - return 9 * (adev->vm_manager.num_level - level - 1) +
> + if (level != 0)
> + return 9 * (level - 1) +
> adev->vm_manager.block_size;
> else
> /* For the page tables on the leaves */
> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
> * @adev: amdgpu_device pointer
> *
> * Calculate the number of entries in a page directory or page table.
> + * The hiberachy of page table is as below, which aligns hw names.
> + * PDB2->PDB1->PDB0->PTB, accordingly:
> + * level3 --- PDB2
> + * level2 --- PDB1
> + * level1 --- PDB0
> + * level0 --- PTB
> */
> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
> unsigned level)
> {
> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
> + unsigned shift = amdgpu_vm_level_shift(adev,
> + adev->vm_manager.num_level);
>
> - if (level == 0)
> + if (level == adev->vm_manager.num_level)
> /* For the root directory */
> return round_up(adev->vm_manager.max_pfn, 1 << shift) >> shift;
> - else if (level != adev->vm_manager.num_level)
> + else if (level != 0)
> /* Everything in between */
> return 512;
> else
> - /* For the page tables on the leaves */
> + /* For the page tables on the leaves(PTB) */
> return AMDGPU_VM_PTE_COUNT(adev);
> }
>
> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> u64 flags;
> uint64_t init_value = 0;
>
> + BUG_ON(level > adev->vm_manager.num_level);
> +
> if (!parent->entries) {
> unsigned num_entries = amdgpu_vm_num_entries(adev, level);
>
> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> if (to > parent->last_entry_used)
> parent->last_entry_used = to;
>
> - ++level;
> + level--;
> saddr = saddr & ((1 << shift) - 1);
> eaddr = eaddr & ((1 << shift) - 1);
>
> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>
> if (vm->pte_support_ats) {
> init_value = AMDGPU_PTE_DEFAULT_ATC;
> - if (level != adev->vm_manager.num_level - 1)
> + /* != PDB0 */
> + if (level != 1)
> init_value |= AMDGPU_PDE_PTE;
>
> }
> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> entry->addr = 0;
> }
>
> - if (level < adev->vm_manager.num_level) {
> + if (level > 0) {
> uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
> ((1 << shift) - 1);
> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> saddr /= AMDGPU_GPU_PAGE_SIZE;
> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>
> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr, 0);
> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
> + adev->vm_manager.num_level);
> }
>
> /**
> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
> struct amdgpu_vm_pt **entry,
> struct amdgpu_vm_pt **parent)
> {
> - unsigned level = 0;
> + unsigned level = p->adev->vm_manager.num_level;
>
> *parent = NULL;
> *entry = &p->vm->root;
> while ((*entry)->entries) {
> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
>
> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
> *parent = *entry;
> *entry = &(*entry)->entries[idx];
> }
>
> - if (level != p->adev->vm_manager.num_level)
> + if (level != 0)
> *entry = NULL;
> }
>
> --
> 2.14.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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <CADnq5_NmHKLi1kSa6NDC4feoHUZcmViML9eQvEX_M9UW-8Azww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-12-08 15:37 ` Christian König
2017-12-11 3:01 ` Chunming Zhou
1 sibling, 0 replies; 11+ messages in thread
From: Christian König @ 2017-12-08 15:37 UTC (permalink / raw)
To: Alex Deucher, Chunming Zhou; +Cc: amd-gfx list
Am 08.12.2017 um 15:58 schrieb Alex Deucher:
> On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.zhou@amd.com> wrote:
>> The hiberachy of page table is as below, which aligns hw names.
>> PDB2->PDB1->PDB0->PTB, accordingly:
>> level3 --- PDB2
>> level2 --- PDB1
>> level1 --- PDB0
>> level0 --- PTB
> What's the advantage of this change? It's not clear from the commit message.
Well you align a bit better with how our hardware guys tend to describe
the levels.
But level0 becomes PTB and level1 becomes PDB0 etc..., so I don't think
we win much here.
Christian.
>
> Alex
>
>> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 ++++++++++++++++++++++------------
>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3ecdbdfb04dd..8904ccf78fc9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
>> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>> unsigned level)
>> {
>> - if (level != adev->vm_manager.num_level)
>> - return 9 * (adev->vm_manager.num_level - level - 1) +
>> + if (level != 0)
>> + return 9 * (level - 1) +
>> adev->vm_manager.block_size;
>> else
>> /* For the page tables on the leaves */
>> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>> * @adev: amdgpu_device pointer
>> *
>> * Calculate the number of entries in a page directory or page table.
>> + * The hiberachy of page table is as below, which aligns hw names.
>> + * PDB2->PDB1->PDB0->PTB, accordingly:
>> + * level3 --- PDB2
>> + * level2 --- PDB1
>> + * level1 --- PDB0
>> + * level0 --- PTB
>> */
>> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>> unsigned level)
>> {
>> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
>> + unsigned shift = amdgpu_vm_level_shift(adev,
>> + adev->vm_manager.num_level);
>>
>> - if (level == 0)
>> + if (level == adev->vm_manager.num_level)
>> /* For the root directory */
>> return round_up(adev->vm_manager.max_pfn, 1 << shift) >> shift;
>> - else if (level != adev->vm_manager.num_level)
>> + else if (level != 0)
>> /* Everything in between */
>> return 512;
>> else
>> - /* For the page tables on the leaves */
>> + /* For the page tables on the leaves(PTB) */
>> return AMDGPU_VM_PTE_COUNT(adev);
>> }
>>
>> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>> u64 flags;
>> uint64_t init_value = 0;
>>
>> + BUG_ON(level > adev->vm_manager.num_level);
>> +
>> if (!parent->entries) {
>> unsigned num_entries = amdgpu_vm_num_entries(adev, level);
>>
>> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>> if (to > parent->last_entry_used)
>> parent->last_entry_used = to;
>>
>> - ++level;
>> + level--;
>> saddr = saddr & ((1 << shift) - 1);
>> eaddr = eaddr & ((1 << shift) - 1);
>>
>> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>>
>> if (vm->pte_support_ats) {
>> init_value = AMDGPU_PTE_DEFAULT_ATC;
>> - if (level != adev->vm_manager.num_level - 1)
>> + /* != PDB0 */
>> + if (level != 1)
>> init_value |= AMDGPU_PDE_PTE;
>>
>> }
>> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>> entry->addr = 0;
>> }
>>
>> - if (level < adev->vm_manager.num_level) {
>> + if (level > 0) {
>> uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
>> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
>> ((1 << shift) - 1);
>> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> saddr /= AMDGPU_GPU_PAGE_SIZE;
>> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>
>> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr, 0);
>> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>> + adev->vm_manager.num_level);
>> }
>>
>> /**
>> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
>> struct amdgpu_vm_pt **entry,
>> struct amdgpu_vm_pt **parent)
>> {
>> - unsigned level = 0;
>> + unsigned level = p->adev->vm_manager.num_level;
>>
>> *parent = NULL;
>> *entry = &p->vm->root;
>> while ((*entry)->entries) {
>> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
>> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
>>
>> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>> *parent = *entry;
>> *entry = &(*entry)->entries[idx];
>> }
>>
>> - if (level != p->adev->vm_manager.num_level)
>> + if (level != 0)
>> *entry = NULL;
>> }
>>
>> --
>> 2.14.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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <CADnq5_NmHKLi1kSa6NDC4feoHUZcmViML9eQvEX_M9UW-8Azww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-12-08 15:37 ` Christian König
@ 2017-12-11 3:01 ` Chunming Zhou
[not found] ` <0321277c-e7f0-8d47-2951-21b9870194e2-5C7GfCeVMHo@public.gmane.org>
1 sibling, 1 reply; 11+ messages in thread
From: Chunming Zhou @ 2017-12-11 3:01 UTC (permalink / raw)
To: Alex Deucher, Chunming Zhou; +Cc: amd-gfx list
On 2017年12月08日 22:58, Alex Deucher wrote:
> On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.zhou@amd.com> wrote:
>> The hiberachy of page table is as below, which aligns hw names.
>> PDB2->PDB1->PDB0->PTB, accordingly:
>> level3 --- PDB2
>> level2 --- PDB1
>> level1 --- PDB0
>> level0 --- PTB
> What's the advantage of this change? It's not clear from the commit message.
Hi Alex,
previous the level is increasing, the root pdb is level0, not only me,
also many people thought the root PDB is PDB0, but our many hw documents
assume PDB0 is pointing to PTB, that is said, which bring us unnecessary
trouble on understanding our VM hiberachy implementation, especially for
ramp up beginner.
For last Christian BLOCK_SIZE patch example, the spec said it only
effects the PTB pointed by PDB0, but we initially thought it effects all
levels, which mainly because of confuse 'PDB0'.
Above is my starting point.
Regards,
David Zhou
>
> Alex
>
>> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 ++++++++++++++++++++++------------
>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3ecdbdfb04dd..8904ccf78fc9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
>> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>> unsigned level)
>> {
>> - if (level != adev->vm_manager.num_level)
>> - return 9 * (adev->vm_manager.num_level - level - 1) +
>> + if (level != 0)
>> + return 9 * (level - 1) +
>> adev->vm_manager.block_size;
>> else
>> /* For the page tables on the leaves */
>> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>> * @adev: amdgpu_device pointer
>> *
>> * Calculate the number of entries in a page directory or page table.
>> + * The hiberachy of page table is as below, which aligns hw names.
>> + * PDB2->PDB1->PDB0->PTB, accordingly:
>> + * level3 --- PDB2
>> + * level2 --- PDB1
>> + * level1 --- PDB0
>> + * level0 --- PTB
>> */
>> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>> unsigned level)
>> {
>> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
>> + unsigned shift = amdgpu_vm_level_shift(adev,
>> + adev->vm_manager.num_level);
>>
>> - if (level == 0)
>> + if (level == adev->vm_manager.num_level)
>> /* For the root directory */
>> return round_up(adev->vm_manager.max_pfn, 1 << shift) >> shift;
>> - else if (level != adev->vm_manager.num_level)
>> + else if (level != 0)
>> /* Everything in between */
>> return 512;
>> else
>> - /* For the page tables on the leaves */
>> + /* For the page tables on the leaves(PTB) */
>> return AMDGPU_VM_PTE_COUNT(adev);
>> }
>>
>> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>> u64 flags;
>> uint64_t init_value = 0;
>>
>> + BUG_ON(level > adev->vm_manager.num_level);
>> +
>> if (!parent->entries) {
>> unsigned num_entries = amdgpu_vm_num_entries(adev, level);
>>
>> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>> if (to > parent->last_entry_used)
>> parent->last_entry_used = to;
>>
>> - ++level;
>> + level--;
>> saddr = saddr & ((1 << shift) - 1);
>> eaddr = eaddr & ((1 << shift) - 1);
>>
>> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>>
>> if (vm->pte_support_ats) {
>> init_value = AMDGPU_PTE_DEFAULT_ATC;
>> - if (level != adev->vm_manager.num_level - 1)
>> + /* != PDB0 */
>> + if (level != 1)
>> init_value |= AMDGPU_PDE_PTE;
>>
>> }
>> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>> entry->addr = 0;
>> }
>>
>> - if (level < adev->vm_manager.num_level) {
>> + if (level > 0) {
>> uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
>> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
>> ((1 << shift) - 1);
>> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> saddr /= AMDGPU_GPU_PAGE_SIZE;
>> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>
>> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr, 0);
>> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>> + adev->vm_manager.num_level);
>> }
>>
>> /**
>> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
>> struct amdgpu_vm_pt **entry,
>> struct amdgpu_vm_pt **parent)
>> {
>> - unsigned level = 0;
>> + unsigned level = p->adev->vm_manager.num_level;
>>
>> *parent = NULL;
>> *entry = &p->vm->root;
>> while ((*entry)->entries) {
>> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
>> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
>>
>> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>> *parent = *entry;
>> *entry = &(*entry)->entries[idx];
>> }
>>
>> - if (level != p->adev->vm_manager.num_level)
>> + if (level != 0)
>> *entry = NULL;
>> }
>>
>> --
>> 2.14.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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <0321277c-e7f0-8d47-2951-21b9870194e2-5C7GfCeVMHo@public.gmane.org>
@ 2017-12-11 3:55 ` Alex Deucher
[not found] ` <CADnq5_NYVOVqK1f_Re0ibyR7ROHxW9sP+i6Vnt5F8OEGxSxVrA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Alex Deucher @ 2017-12-11 3:55 UTC (permalink / raw)
To: Chunming Zhou; +Cc: Chunming Zhou, amd-gfx list
On Sun, Dec 10, 2017 at 10:01 PM, Chunming Zhou <zhoucm1@amd.com> wrote:
>
>
> On 2017年12月08日 22:58, Alex Deucher wrote:
>>
>> On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.zhou@amd.com> wrote:
>>>
>>> The hiberachy of page table is as below, which aligns hw names.
>>> PDB2->PDB1->PDB0->PTB, accordingly:
>>> level3 --- PDB2
>>> level2 --- PDB1
>>> level1 --- PDB0
>>> level0 --- PTB
>>
>> What's the advantage of this change? It's not clear from the commit
>> message.
>
> Hi Alex,
>
> previous the level is increasing, the root pdb is level0, not only me, also
> many people thought the root PDB is PDB0, but our many hw documents assume
> PDB0 is pointing to PTB, that is said, which bring us unnecessary trouble on
> understanding our VM hiberachy implementation, especially for ramp up
> beginner.
> For last Christian BLOCK_SIZE patch example, the spec said it only effects
> the PTB pointed by PDB0, but we initially thought it effects all levels,
> which mainly because of confuse 'PDB0'.
>
> Above is my starting point.
Thanks for clarifying. Please include that information in the commit
message for clarity.
Alex
>
> Regards,
> David Zhou
>
>>
>> Alex
>>
>>> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37
>>> ++++++++++++++++++++++------------
>>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 3ecdbdfb04dd..8904ccf78fc9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
>>> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>>> unsigned level)
>>> {
>>> - if (level != adev->vm_manager.num_level)
>>> - return 9 * (adev->vm_manager.num_level - level - 1) +
>>> + if (level != 0)
>>> + return 9 * (level - 1) +
>>> adev->vm_manager.block_size;
>>> else
>>> /* For the page tables on the leaves */
>>> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct
>>> amdgpu_device *adev,
>>> * @adev: amdgpu_device pointer
>>> *
>>> * Calculate the number of entries in a page directory or page table.
>>> + * The hiberachy of page table is as below, which aligns hw names.
>>> + * PDB2->PDB1->PDB0->PTB, accordingly:
>>> + * level3 --- PDB2
>>> + * level2 --- PDB1
>>> + * level1 --- PDB0
>>> + * level0 --- PTB
>>> */
>>> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>>> unsigned level)
>>> {
>>> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
>>> + unsigned shift = amdgpu_vm_level_shift(adev,
>>> +
>>> adev->vm_manager.num_level);
>>>
>>> - if (level == 0)
>>> + if (level == adev->vm_manager.num_level)
>>> /* For the root directory */
>>> return round_up(adev->vm_manager.max_pfn, 1 << shift) >>
>>> shift;
>>> - else if (level != adev->vm_manager.num_level)
>>> + else if (level != 0)
>>> /* Everything in between */
>>> return 512;
>>> else
>>> - /* For the page tables on the leaves */
>>> + /* For the page tables on the leaves(PTB) */
>>> return AMDGPU_VM_PTE_COUNT(adev);
>>> }
>>>
>>> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct
>>> amdgpu_device *adev,
>>> u64 flags;
>>> uint64_t init_value = 0;
>>>
>>> + BUG_ON(level > adev->vm_manager.num_level);
>>> +
>>> if (!parent->entries) {
>>> unsigned num_entries = amdgpu_vm_num_entries(adev,
>>> level);
>>>
>>> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct
>>> amdgpu_device *adev,
>>> if (to > parent->last_entry_used)
>>> parent->last_entry_used = to;
>>>
>>> - ++level;
>>> + level--;
>>> saddr = saddr & ((1 << shift) - 1);
>>> eaddr = eaddr & ((1 << shift) - 1);
>>>
>>> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct
>>> amdgpu_device *adev,
>>>
>>> if (vm->pte_support_ats) {
>>> init_value = AMDGPU_PTE_DEFAULT_ATC;
>>> - if (level != adev->vm_manager.num_level - 1)
>>> + /* != PDB0 */
>>> + if (level != 1)
>>> init_value |= AMDGPU_PDE_PTE;
>>>
>>> }
>>> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct
>>> amdgpu_device *adev,
>>> entry->addr = 0;
>>> }
>>>
>>> - if (level < adev->vm_manager.num_level) {
>>> + if (level > 0) {
>>> uint64_t sub_saddr = (pt_idx == from) ? saddr :
>>> 0;
>>> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
>>> ((1 << shift) - 1);
>>> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> saddr /= AMDGPU_GPU_PAGE_SIZE;
>>> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>
>>> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>>> 0);
>>> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>>> + adev->vm_manager.num_level);
>>> }
>>>
>>> /**
>>> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct
>>> amdgpu_pte_update_params *p, uint64_t addr,
>>> struct amdgpu_vm_pt **entry,
>>> struct amdgpu_vm_pt **parent)
>>> {
>>> - unsigned level = 0;
>>> + unsigned level = p->adev->vm_manager.num_level;
>>>
>>> *parent = NULL;
>>> *entry = &p->vm->root;
>>> while ((*entry)->entries) {
>>> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev,
>>> level++);
>>> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev,
>>> level--);
>>>
>>> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>>> *parent = *entry;
>>> *entry = &(*entry)->entries[idx];
>>> }
>>>
>>> - if (level != p->adev->vm_manager.num_level)
>>> + if (level != 0)
>>> *entry = NULL;
>>> }
>>>
>>> --
>>> 2.14.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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: fix pte index calculation
[not found] ` <32026195-0596-404f-deb5-018d16c19a75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-12-11 7:34 ` Chunming Zhou
0 siblings, 0 replies; 11+ messages in thread
From: Chunming Zhou @ 2017-12-11 7:34 UTC (permalink / raw)
To: christian.koenig-5C7GfCeVMHo, Chunming Zhou,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 2017年12月08日 22:00, Christian König wrote:
> What is wrong with the old approach?
When the num entries are less than one page(512), the old approach will
result in error. e.g. when we enable translate further feature, the ptb
will only have 32 entries with 64KB---16 * 4KB selection.
>
> I would rather say that the address should be limited by the level
> shift instead. This way we avoid the modulo altogether.
already in your patch set.
Regards,
David Zhou
>
> Christian.
>
> Am 08.12.2017 um 11:56 schrieb Chunming Zhou:
>> Change-Id: I40ecf31ad4b51022a2c0c076ae45188b6e9d63de
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8904ccf78fc9..affe64e42cef 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1335,11 +1335,13 @@ void amdgpu_vm_get_entry(struct
>> amdgpu_pte_update_params *p, uint64_t addr,
>> *parent = NULL;
>> *entry = &p->vm->root;
>> while ((*entry)->entries) {
>> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
>> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level);
>> - idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>> + idx %= amdgpu_vm_num_entries(p->adev, level);
>> *parent = *entry;
>> *entry = &(*entry)->entries[idx];
>> + if (level)
>> + level--;
>> }
>> if (level != 0)
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <957cec12-f5a6-16db-12d3-e3c39638afa6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-12-11 7:49 ` Chunming Zhou
0 siblings, 0 replies; 11+ messages in thread
From: Chunming Zhou @ 2017-12-11 7:49 UTC (permalink / raw)
To: christian.koenig-5C7GfCeVMHo, Chunming Zhou,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 2017年12月08日 22:06, Christian König wrote:
> Am 08.12.2017 um 11:56 schrieb Chunming Zhou:
>> The hiberachy of page table is as below, which aligns hw names.
>> PDB2->PDB1->PDB0->PTB, accordingly:
>> level3 --- PDB2
>> level2 --- PDB1
>> level1 --- PDB0
>> level0 --- PTB
>>
>> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>
> Yeah, thought about that as well. But I'm not sure if that is a good
> idea or not.
We should correct the confusing PDB0 concept, even spec directly uses it.
>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37
>> ++++++++++++++++++++++------------
>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3ecdbdfb04dd..8904ccf78fc9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
>> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>> unsigned level)
>> {
>> - if (level != adev->vm_manager.num_level)
>> - return 9 * (adev->vm_manager.num_level - level - 1) +
>> + if (level != 0)
>> + return 9 * (level - 1) +
>> adev->vm_manager.block_size;
>> else
>> /* For the page tables on the leaves */
>> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct
>> amdgpu_device *adev,
>> * @adev: amdgpu_device pointer
>> *
>> * Calculate the number of entries in a page directory or page table.
>> + * The hiberachy of page table is as below, which aligns hw names.
>> + * PDB2->PDB1->PDB0->PTB, accordingly:
>> + * level3 --- PDB2
>> + * level2 --- PDB1
>> + * level1 --- PDB0
>> + * level0 --- PTB
>> */
>> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>> unsigned level)
>> {
>> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
>> + unsigned shift = amdgpu_vm_level_shift(adev,
>> + adev->vm_manager.num_level);
>> - if (level == 0)
>> + if (level == adev->vm_manager.num_level)
>> /* For the root directory */
>> return round_up(adev->vm_manager.max_pfn, 1 << shift) >>
>> shift;
>> - else if (level != adev->vm_manager.num_level)
>> + else if (level != 0)
>> /* Everything in between */
>> return 512;
>> else
>> - /* For the page tables on the leaves */
>> + /* For the page tables on the leaves(PTB) */
>> return AMDGPU_VM_PTE_COUNT(adev);
>> }
>> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>> u64 flags;
>> uint64_t init_value = 0;
>> + BUG_ON(level > adev->vm_manager.num_level);
>> +
>> if (!parent->entries) {
>> unsigned num_entries = amdgpu_vm_num_entries(adev, level);
>> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>> if (to > parent->last_entry_used)
>> parent->last_entry_used = to;
>> - ++level;
>> + level--;
>
> Post decrement/increment please.
it's OK, but what the difference of `level--;` and `--level;` is? which
isn't in expression, but in alone line.
>
>> saddr = saddr & ((1 << shift) - 1);
>> eaddr = eaddr & ((1 << shift) - 1);
>> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>> if (vm->pte_support_ats) {
>> init_value = AMDGPU_PTE_DEFAULT_ATC;
>> - if (level != adev->vm_manager.num_level - 1)
>
> BTW: If I'm not completely mistaken that code is incorrect and should
> rather be "level != adev->vm_manager.num_level".
will separate a fix patch.
Regards,
David Zhou
>
>> + /* != PDB0 */
>> + if (level != 1)
>
> Which would that make it level != 0.
>
> Regards,
> Christian.
>
>> init_value |= AMDGPU_PDE_PTE;
>> }
>> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>> entry->addr = 0;
>> }
>> - if (level < adev->vm_manager.num_level) {
>> + if (level > 0) {
>> uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
>> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
>> ((1 << shift) - 1);
>> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> saddr /= AMDGPU_GPU_PAGE_SIZE;
>> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr,
>> eaddr, 0);
>> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>> + adev->vm_manager.num_level);
>> }
>> /**
>> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct
>> amdgpu_pte_update_params *p, uint64_t addr,
>> struct amdgpu_vm_pt **entry,
>> struct amdgpu_vm_pt **parent)
>> {
>> - unsigned level = 0;
>> + unsigned level = p->adev->vm_manager.num_level;
>> *parent = NULL;
>> *entry = &p->vm->root;
>> while ((*entry)->entries) {
>> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
>> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
>> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>> *parent = *entry;
>> *entry = &(*entry)->entries[idx];
>> }
>> - if (level != p->adev->vm_manager.num_level)
>> + if (level != 0)
>> *entry = NULL;
>> }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: reverse PDBs order
[not found] ` <CADnq5_NYVOVqK1f_Re0ibyR7ROHxW9sP+i6Vnt5F8OEGxSxVrA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-12-11 9:58 ` Christian König
0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2017-12-11 9:58 UTC (permalink / raw)
To: Alex Deucher, Chunming Zhou; +Cc: Chunming Zhou, amd-gfx list
Am 11.12.2017 um 04:55 schrieb Alex Deucher:
> On Sun, Dec 10, 2017 at 10:01 PM, Chunming Zhou <zhoucm1@amd.com> wrote:
>>
>> On 2017年12月08日 22:58, Alex Deucher wrote:
>>> On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.zhou@amd.com> wrote:
>>>> The hiberachy of page table is as below, which aligns hw names.
>>>> PDB2->PDB1->PDB0->PTB, accordingly:
>>>> level3 --- PDB2
>>>> level2 --- PDB1
>>>> level1 --- PDB0
>>>> level0 --- PTB
>>> What's the advantage of this change? It's not clear from the commit
>>> message.
>> Hi Alex,
>>
>> previous the level is increasing, the root pdb is level0, not only me, also
>> many people thought the root PDB is PDB0, but our many hw documents assume
>> PDB0 is pointing to PTB, that is said, which bring us unnecessary trouble on
>> understanding our VM hiberachy implementation, especially for ramp up
>> beginner.
>> For last Christian BLOCK_SIZE patch example, the spec said it only effects
>> the PTB pointed by PDB0, but we initially thought it effects all levels,
>> which mainly because of confuse 'PDB0'.
>>
>> Above is my starting point.
> Thanks for clarifying. Please include that information in the commit
> message for clarity.
We should document this, but I would keep the code as it is cause
mapping level1 to PDB0 doesn't make it any clearer if you ask me.
And additional to that as you said as well most people assume that zero
is the root page table.
The only people who doesn't do that are our hardware developers (and I
honestly doesn't understand why, cause this causes confusion all over
the place even internally to AMD).
Another problem are the page directory start value which is written into
the registers on Vega10. Those reassemble a page directory entry and I
referenced them as level -1 in the latest patches.
Ok we could change that to 0xff as well, but I find that even more
confusing.
Christian.
>
> Alex
>
>> Regards,
>> David Zhou
>>
>>> Alex
>>>
>>>> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37
>>>> ++++++++++++++++++++++------------
>>>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 3ecdbdfb04dd..8904ccf78fc9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
>>>> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>>>> unsigned level)
>>>> {
>>>> - if (level != adev->vm_manager.num_level)
>>>> - return 9 * (adev->vm_manager.num_level - level - 1) +
>>>> + if (level != 0)
>>>> + return 9 * (level - 1) +
>>>> adev->vm_manager.block_size;
>>>> else
>>>> /* For the page tables on the leaves */
>>>> @@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct
>>>> amdgpu_device *adev,
>>>> * @adev: amdgpu_device pointer
>>>> *
>>>> * Calculate the number of entries in a page directory or page table.
>>>> + * The hiberachy of page table is as below, which aligns hw names.
>>>> + * PDB2->PDB1->PDB0->PTB, accordingly:
>>>> + * level3 --- PDB2
>>>> + * level2 --- PDB1
>>>> + * level1 --- PDB0
>>>> + * level0 --- PTB
>>>> */
>>>> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>>>> unsigned level)
>>>> {
>>>> - unsigned shift = amdgpu_vm_level_shift(adev, 0);
>>>> + unsigned shift = amdgpu_vm_level_shift(adev,
>>>> +
>>>> adev->vm_manager.num_level);
>>>>
>>>> - if (level == 0)
>>>> + if (level == adev->vm_manager.num_level)
>>>> /* For the root directory */
>>>> return round_up(adev->vm_manager.max_pfn, 1 << shift) >>
>>>> shift;
>>>> - else if (level != adev->vm_manager.num_level)
>>>> + else if (level != 0)
>>>> /* Everything in between */
>>>> return 512;
>>>> else
>>>> - /* For the page tables on the leaves */
>>>> + /* For the page tables on the leaves(PTB) */
>>>> return AMDGPU_VM_PTE_COUNT(adev);
>>>> }
>>>>
>>>> @@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct
>>>> amdgpu_device *adev,
>>>> u64 flags;
>>>> uint64_t init_value = 0;
>>>>
>>>> + BUG_ON(level > adev->vm_manager.num_level);
>>>> +
>>>> if (!parent->entries) {
>>>> unsigned num_entries = amdgpu_vm_num_entries(adev,
>>>> level);
>>>>
>>>> @@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct
>>>> amdgpu_device *adev,
>>>> if (to > parent->last_entry_used)
>>>> parent->last_entry_used = to;
>>>>
>>>> - ++level;
>>>> + level--;
>>>> saddr = saddr & ((1 << shift) - 1);
>>>> eaddr = eaddr & ((1 << shift) - 1);
>>>>
>>>> @@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct
>>>> amdgpu_device *adev,
>>>>
>>>> if (vm->pte_support_ats) {
>>>> init_value = AMDGPU_PTE_DEFAULT_ATC;
>>>> - if (level != adev->vm_manager.num_level - 1)
>>>> + /* != PDB0 */
>>>> + if (level != 1)
>>>> init_value |= AMDGPU_PDE_PTE;
>>>>
>>>> }
>>>> @@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct
>>>> amdgpu_device *adev,
>>>> entry->addr = 0;
>>>> }
>>>>
>>>> - if (level < adev->vm_manager.num_level) {
>>>> + if (level > 0) {
>>>> uint64_t sub_saddr = (pt_idx == from) ? saddr :
>>>> 0;
>>>> uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
>>>> ((1 << shift) - 1);
>>>> @@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>> saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>> eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>
>>>> - return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>>>> 0);
>>>> + return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
>>>> + adev->vm_manager.num_level);
>>>> }
>>>>
>>>> /**
>>>> @@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct
>>>> amdgpu_pte_update_params *p, uint64_t addr,
>>>> struct amdgpu_vm_pt **entry,
>>>> struct amdgpu_vm_pt **parent)
>>>> {
>>>> - unsigned level = 0;
>>>> + unsigned level = p->adev->vm_manager.num_level;
>>>>
>>>> *parent = NULL;
>>>> *entry = &p->vm->root;
>>>> while ((*entry)->entries) {
>>>> - unsigned idx = addr >> amdgpu_vm_level_shift(p->adev,
>>>> level++);
>>>> + unsigned idx = addr >> amdgpu_vm_level_shift(p->adev,
>>>> level--);
>>>>
>>>> idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>>>> *parent = *entry;
>>>> *entry = &(*entry)->entries[idx];
>>>> }
>>>>
>>>> - if (level != p->adev->vm_manager.num_level)
>>>> + if (level != 0)
>>>> *entry = NULL;
>>>> }
>>>>
>>>> --
>>>> 2.14.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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-11 9:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-08 10:56 [PATCH 1/2] drm/amdgpu: reverse PDBs order Chunming Zhou
[not found] ` <20171208105610.26728-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2017-12-08 10:56 ` [PATCH 2/2] drm/amdgpu: fix pte index calculation Chunming Zhou
[not found] ` <20171208105610.26728-2-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2017-12-08 14:00 ` Christian König
[not found] ` <32026195-0596-404f-deb5-018d16c19a75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-12-11 7:34 ` Chunming Zhou
2017-12-08 14:06 ` [PATCH 1/2] drm/amdgpu: reverse PDBs order Christian König
[not found] ` <957cec12-f5a6-16db-12d3-e3c39638afa6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-12-11 7:49 ` Chunming Zhou
2017-12-08 14:58 ` Alex Deucher
[not found] ` <CADnq5_NmHKLi1kSa6NDC4feoHUZcmViML9eQvEX_M9UW-8Azww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-12-08 15:37 ` Christian König
2017-12-11 3:01 ` Chunming Zhou
[not found] ` <0321277c-e7f0-8d47-2951-21b9870194e2-5C7GfCeVMHo@public.gmane.org>
2017-12-11 3:55 ` Alex Deucher
[not found] ` <CADnq5_NYVOVqK1f_Re0ibyR7ROHxW9sP+i6Vnt5F8OEGxSxVrA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-12-11 9:58 ` Christian König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.