* [PATCH 2/3] drm/amdgpu: only move VM BOs in the LRU during validation
[not found] ` <1499952313-2398-1-git-send-email-deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
@ 2017-07-13 13:25 ` Christian König
[not found] ` <1499952313-2398-2-git-send-email-deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-07-13 13:25 ` [PATCH 3/3] drm/amdgpu: only bind VM shadows after validation Christian König
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2017-07-13 13:25 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
From: Christian König <christian.koenig@amd.com>
This should save us a bunch of command submission overhead.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 15 +++------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 56 ++++++----------------------------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 --
3 files changed, 14 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 44ec11d..652425c8 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 3f5863e..ea419ec 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;
@@ -170,6 +171,10 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
return r;
}
+ spin_lock(&glob->lru_lock);
+ ttm_bo_move_to_lru_tail(&parent->bo->tbo);
+ spin_unlock(&glob->lru_lock);
+
if (!parent->entries)
return 0;
@@ -189,7 +194,7 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
* 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;
}
@@ -221,54 +226,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 c3dfd3e..46e030c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -215,8 +215,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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/3] drm/amdgpu: only bind VM shadows after validation
[not found] ` <1499952313-2398-1-git-send-email-deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-07-13 13:25 ` [PATCH 2/3] drm/amdgpu: only move VM BOs in the LRU during validation Christian König
@ 2017-07-13 13:25 ` Christian König
[not found] ` <1499952313-2398-3-git-send-email-deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-07-13 15:57 ` [PATCH 1/3] drm/amdgpu: track if a PD needs an update Felix Kuehling
2017-07-14 2:13 ` zhoucm1
3 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2017-07-13 13:25 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
From: Christian König <christian.koenig@amd.com>
No need to do this on every CS.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ea419ec..e1641f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -171,6 +171,18 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
return 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;
+
+ spin_lock(&glob->lru_lock);
+ ttm_bo_move_to_lru_tail(&shadow->tbo);
+ spin_unlock(&glob->lru_lock);
+ }
+
spin_lock(&glob->lru_lock);
ttm_bo_move_to_lru_tail(&parent->bo->tbo);
spin_unlock(&glob->lru_lock);
@@ -1031,11 +1043,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);
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: track if a PD needs an update
[not found] ` <1499952313-2398-1-git-send-email-deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-07-13 13:25 ` [PATCH 2/3] drm/amdgpu: only move VM BOs in the LRU during validation Christian König
2017-07-13 13:25 ` [PATCH 3/3] drm/amdgpu: only bind VM shadows after validation Christian König
@ 2017-07-13 15:57 ` Felix Kuehling
2017-07-14 2:13 ` zhoucm1
3 siblings, 0 replies; 7+ messages in thread
From: Felix Kuehling @ 2017-07-13 15:57 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 17-07-13 09:25 AM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> That should reduce our command submission overhead quite a bit.
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 55d1c7f..3f5863e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -173,6 +173,7 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
> if (!parent->entries)
> return 0;
>
> + parent->needs_update = true;
This means that after amdgpu_vm_validate_pt_bos, all vm_pts will have
needs_update set, even if the last eviction didn't evict any page
tables. That will create unnecessary overhead in amdgpu_vm_update_level.
You could probably add another check in the loop below for whether
validate actually moved any child page tables.
Regards,
Felix
> for (i = 0; i <= parent->last_entry_used; ++i) {
> struct amdgpu_vm_pt *entry = &parent->entries[i];
>
> @@ -351,6 +352,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>
> entry->bo = pt;
> entry->addr = 0;
> + parent->needs_update = true;
> }
>
> if (level < adev->vm_manager.num_level) {
> @@ -361,6 +363,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> sub_eaddr, level);
> if (r)
> return r;
> +
> + parent->needs_update |= entry->needs_update;
> }
> }
>
> @@ -1050,7 +1054,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>
> int r;
>
> - if (!parent->entries)
> + if (!parent->entries || !parent->needs_update)
> return 0;
>
> memset(¶ms, 0, sizeof(params));
> @@ -1196,6 +1200,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
> return r;
> }
>
> + parent->needs_update = false;
> return 0;
>
> error_free:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 3441ec5..c3dfd3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -102,6 +102,7 @@ struct amdgpu_vm_pt {
> /* array of page tables, one for each directory entry */
> struct amdgpu_vm_pt *entries;
> unsigned last_entry_used;
> + bool needs_update;
> };
>
> struct amdgpu_vm {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: track if a PD needs an update
[not found] ` <1499952313-2398-1-git-send-email-deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
` (2 preceding siblings ...)
2017-07-13 15:57 ` [PATCH 1/3] drm/amdgpu: track if a PD needs an update Felix Kuehling
@ 2017-07-14 2:13 ` zhoucm1
3 siblings, 0 replies; 7+ messages in thread
From: zhoucm1 @ 2017-07-14 2:13 UTC (permalink / raw)
To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
'if (parent->entries[pt_idx].addr == pt)' is good enough for condition
of need_update, adding previous Felix's optimization patch, then the
whole picture looks better than this one, IIRC.
Regards,
David Zhou
On 2017年07月13日 21:25, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> That should reduce our command submission overhead quite a bit.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 55d1c7f..3f5863e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -173,6 +173,7 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
> if (!parent->entries)
> return 0;
>
> + parent->needs_update = true;
> for (i = 0; i <= parent->last_entry_used; ++i) {
> struct amdgpu_vm_pt *entry = &parent->entries[i];
>
> @@ -351,6 +352,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>
> entry->bo = pt;
> entry->addr = 0;
> + parent->needs_update = true;
> }
>
> if (level < adev->vm_manager.num_level) {
> @@ -361,6 +363,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> sub_eaddr, level);
> if (r)
> return r;
> +
> + parent->needs_update |= entry->needs_update;
> }
> }
>
> @@ -1050,7 +1054,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>
> int r;
>
> - if (!parent->entries)
> + if (!parent->entries || !parent->needs_update)
> return 0;
>
> memset(¶ms, 0, sizeof(params));
> @@ -1196,6 +1200,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
> return r;
> }
>
> + parent->needs_update = false;
> return 0;
>
> error_free:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 3441ec5..c3dfd3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -102,6 +102,7 @@ struct amdgpu_vm_pt {
> /* array of page tables, one for each directory entry */
> struct amdgpu_vm_pt *entries;
> unsigned last_entry_used;
> + bool needs_update;
> };
>
> struct amdgpu_vm {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread