[RFC 0/5] Discussion around eviction improvements

[RFC 0/5] Discussion around eviction improvements

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Last few days I was looking at the situation with VRAM over subscription, what
happens versus what perhaps should happen. Browsing through the driver and
running some simple experiments.

I ended up with this patch series which, as a disclaimer, may be completely
wrong but as I found some suspicious things, to me at least, I thought it was a
good point to stop and request some comments.

To perhaps summarise what are the main issues I think I found:

 * Migration rate limiting does not bother knowing if actual migration happened
   and so can over-account and unfairly penalise.

 * Migration rate limiting does not even work, at least not for the common case
   where userspace configures VRAM+GTT. It thinks it can stop migration attempts
   by playing with bo->allowed_domains vs bo->preferred domains but, both from
   the code, and from empirical experiments, I see that not working at all. Both
   masks are identical so fiddling with them achieves nothing.

 * Idea of the fallback placement only works when VRAM has free space. As soon
   as it does not, ttm_resource_compatible is happy to leave the buffers in the
   secondary placement forever.

 * Driver thinks it will be re-validating evicted buffers on the next submission
   but it does not for the very common case of VRAM+GTT because it only checks
   if current placement is *none* of the preferred placements.

All those problems are addressed in individual patches.

End result of this series appears to be driver which will try harder to move
buffers back into VRAM, but will be (more) correctly throttled in doing so by
the existing rate limiting logic.

I have run a quick benchmark of Cyberpunk 2077 and cannot say that I saw a
change but that could be a good thing too. At least I did not break anything,
perhaps.. On one occassion I did see the rate limiting logic get confused while
for a period of few minutes it went to a mode where it was constantly giving a
high migration budget. But that recovered itself when I switched clients and did
not come back so I don't know. If there is something wrong there I don't think
it would be caused by any patches in this series.

Series is probably rough but should be good enough for dicsussion. I am curious
to hear if I identified at least something correctly as a real problem.

It would also be good to hear what are the suggested games to check and see
whether there is any improvement.

Cc: Christian König <christian.koenig@amd.com>
Cc: Friedrich Vock <friedrich.vock@gmx.de>

Tvrtko Ursulin (5):
  drm/amdgpu: Fix migration rate limiting accounting
  drm/amdgpu: Actually respect buffer migration budget
  drm/ttm: Add preferred placement flag
  drm/amdgpu: Use preferred placement for VRAM+GTT
  drm/amdgpu: Re-validate evicted buffers

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 38 +++++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 21 ++++++++++--
 drivers/gpu/drm/ttm/ttm_resource.c         | 13 +++++---
 include/drm/ttm/ttm_placement.h            |  3 ++
 5 files changed, 65 insertions(+), 18 deletions(-)

-- 
2.44.0

[RFC 1/5] drm/amdgpu: Fix migration rate limiting accounting

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

The logic assumed any migration attempt worked and therefore would over-
account the amount of data migrated during buffer re-validation. As a
consequence client can be unfairly penalised by incorrectly considering
its migration budget spent.

Fix it by looking at the before and after buffer object backing store and
only account if there was a change.

FIXME:
I think this needs a better solution to account for migrations between
VRAM visible and non-visible portions.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Friedrich Vock <friedrich.vock@gmx.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index ec888fc6ead8..22708954ae68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -784,12 +784,15 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
 		.no_wait_gpu = false,
 		.resv = bo->tbo.base.resv
 	};
+	struct ttm_resource *old_res;
 	uint32_t domain;
 	int r;
 
 	if (bo->tbo.pin_count)
 		return 0;
 
+	old_res = bo->tbo.resource;
+
 	/* Don't move this buffer if we have depleted our allowance
 	 * to move it. Don't move anything if the threshold is zero.
 	 */
@@ -817,16 +820,29 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
 	amdgpu_bo_placement_from_domain(bo, domain);
 	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
 
-	p->bytes_moved += ctx.bytes_moved;
-	if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
-	    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
-		p->bytes_moved_vis += ctx.bytes_moved;
-
 	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
 		domain = bo->allowed_domains;
 		goto retry;
 	}
 
+	if (!r) {
+		struct ttm_resource *new_res = bo->tbo.resource;
+		bool moved = true;
+
+		if (old_res == new_res)
+			moved = false;
+		else if (old_res && new_res &&
+			 old_res->mem_type == new_res->mem_type)
+			moved = false;
+
+		if (moved) {
+			p->bytes_moved += ctx.bytes_moved;
+			if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
+			    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
+				p->bytes_moved_vis += ctx.bytes_moved;
+		}
+	}
+
 	return r;
 }
 
-- 
2.44.0

[RFC 2/5] drm/amdgpu: Actually respect buffer migration budget

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Current code appears to live in a misconception that playing with buffer
allowed and preferred placements can control the decision on whether
backing store migration will be attempted or not.

Both from code inspection and from empirical experiments I see that not
being true, and that both allowed and preferred placement are typically
set to the same bitmask.

As such, when the code decides to throttle the migration for a client, it
is in fact not achieving anything. Buffers can still be either migrated or
not migrated based on the external (to this function and facility) logic.

Fix it by not changing the buffer object placements if the migration
budget has been spent.

FIXME:
Is it still required to call validate is the question..

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Friedrich Vock <friedrich.vock@gmx.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 22708954ae68..d07a1dd7c880 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -784,6 +784,7 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
 		.no_wait_gpu = false,
 		.resv = bo->tbo.base.resv
 	};
+	bool migration_allowed = true;
 	struct ttm_resource *old_res;
 	uint32_t domain;
 	int r;
@@ -805,19 +806,24 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
 			 * visible VRAM if we've depleted our allowance to do
 			 * that.
 			 */
-			if (p->bytes_moved_vis < p->bytes_moved_vis_threshold)
+			if (p->bytes_moved_vis < p->bytes_moved_vis_threshold) {
 				domain = bo->preferred_domains;
-			else
+			} else {
 				domain = bo->allowed_domains;
+				migration_allowed = false;
+			}
 		} else {
 			domain = bo->preferred_domains;
 		}
 	} else {
 		domain = bo->allowed_domains;
+		migration_allowed = false;
 	}
 
 retry:
-	amdgpu_bo_placement_from_domain(bo, domain);
+	if (migration_allowed)
+		amdgpu_bo_placement_from_domain(bo, domain);
+
 	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
 
 	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
-- 
2.44.0

[RFC 3/5] drm/ttm: Add preferred placement flag

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Currently the fallback placement flag can achieve a hint that buffer
should be migrated back to the non-fallback placement, however that only
works while there is no memory pressure. As soon as we reach full VRAM
utilisation, or worse overcommit, the logic is happy to leave buffers in
the fallback placement. Consequence of this is that once buffers are
evicted they never get considered to be migrated back until the memory
pressure subsides, leaving a potentially active client not able to bring
its buffers back in.

Add a "preferred" placement flag which drivers can set when they want some
extra effort to be attempted for bringing a buffer back in.

QQQ:
Is the current "desired" flag unfortunately named perhaps? I ended up
understanding it as more like "would be nice if possible but absolutely
don't bother under memory pressure".

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Friedrich Vock <friedrich.vock@gmx.de>
---
 drivers/gpu/drm/ttm/ttm_resource.c | 13 +++++++++----
 include/drm/ttm/ttm_placement.h    |  3 +++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 4a66b851b67d..59f3d1bcc11f 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -305,6 +305,8 @@ bool ttm_resource_compatible(struct ttm_resource *res,
 			     struct ttm_placement *placement,
 			     bool evicting)
 {
+	const u32 incompatible_flag = evicting ? TTM_PL_FLAG_DESIRED :
+						 TTM_PL_FLAG_FALLBACK;
 	struct ttm_buffer_object *bo = res->bo;
 	struct ttm_device *bdev = bo->bdev;
 	unsigned i;
@@ -316,11 +318,14 @@ bool ttm_resource_compatible(struct ttm_resource *res,
 		const struct ttm_place *place = &placement->placement[i];
 		struct ttm_resource_manager *man;
 
-		if (res->mem_type != place->mem_type)
-			continue;
+		if (res->mem_type != place->mem_type) {
+			if (place->flags & TTM_PL_FLAG_PREFERRED)
+				return false;
+			else
+				continue;
+		}
 
-		if (place->flags & (evicting ? TTM_PL_FLAG_DESIRED :
-				    TTM_PL_FLAG_FALLBACK))
+		if (place->flags & incompatible_flag)
 			continue;
 
 		if (place->flags & TTM_PL_FLAG_CONTIGUOUS &&
diff --git a/include/drm/ttm/ttm_placement.h b/include/drm/ttm/ttm_placement.h
index b510a4812609..8ea0865e9cc8 100644
--- a/include/drm/ttm/ttm_placement.h
+++ b/include/drm/ttm/ttm_placement.h
@@ -70,6 +70,9 @@
 /* Placement is only used during eviction */
 #define TTM_PL_FLAG_FALLBACK	(1 << 4)
 
+/* Placement is only used during eviction */
+#define TTM_PL_FLAG_PREFERRED	(1 << 5)
+
 /**
  * struct ttm_place
  *
-- 
2.44.0

[RFC 4/5] drm/amdgpu: Use preferred placement for VRAM+GTT

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Now that TTM has the preferred placement flag, extend the current
workaround which assumes the GTT placement as fallback in the presence of
the additional VRAM placement.

By marking the VRAM placement as preferred we will make the buffer re-
validation phase actually attempt to migrate them back to VRAM.

Without it, TTM core logic is happy to leave them in GTT placement
"forever".

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Friedrich Vock <friedrich.vock@gmx.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 50b7e7c0ce50..9be767357e86 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -128,8 +128,8 @@ void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
 	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
 	struct ttm_placement *placement = &abo->placement;
 	struct ttm_place *places = abo->placements;
+	int c = 0, vram_index = -1;
 	u64 flags = abo->flags;
-	u32 c = 0;
 
 	if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
 		unsigned int visible_pfn = adev->gmc.visible_vram_size >> PAGE_SHIFT;
@@ -158,7 +158,7 @@ void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
 		    flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)
 			places[c].flags |= TTM_PL_FLAG_CONTIGUOUS;
 
-		c++;
+		vram_index = c++;
 	}
 
 	if (domain & AMDGPU_GEM_DOMAIN_DOORBELL) {
@@ -180,8 +180,10 @@ void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
 		 * When GTT is just an alternative to VRAM make sure that we
 		 * only use it as fallback and still try to fill up VRAM first.
 		 */
-		if (domain & abo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM)
+		if (vram_index >= 0) {
 			places[c].flags |= TTM_PL_FLAG_FALLBACK;
+			places[vram_index].flags |= TTM_PL_FLAG_PREFERRED;
+		}
 		c++;
 	}
 
-- 
2.44.0

[RFC 5/5] drm/amdgpu: Re-validate evicted buffers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Currently the driver appears to be thinking that it will be attempting to
re-validate the evicted buffers on the next submission if they are not in
their preferred placement.

That however appears not to be true for the very common case of buffers
with allowed placements of VRAM+GTT. Simply because the check can only
detect if the current placement is *none* of the preferred ones, happily
leaving VRAM+GTT buffers in the GTT placement "forever".

Fix it by extending the VRAM+GTT special case to the re-validation logic.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Friedrich Vock <friedrich.vock@gmx.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 6bddd43604bc..e53ff914b62e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1248,10 +1248,25 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 	 * next command submission.
 	 */
 	if (amdgpu_vm_is_bo_always_valid(vm, bo)) {
-		uint32_t mem_type = bo->tbo.resource->mem_type;
+		unsigned current_domain =
+			amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
+		bool move_to_evict = false;
 
-		if (!(bo->preferred_domains &
-		      amdgpu_mem_type_to_domain(mem_type)))
+		if (!(bo->preferred_domains & current_domain)) {
+			move_to_evict = true;
+		} else if ((bo->preferred_domains & AMDGPU_GEM_DOMAIN_MASK) ==
+			   (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
+			   current_domain != AMDGPU_GEM_DOMAIN_VRAM) {
+			/*
+			 * If userspace has provided a list of possible
+			 * placements equal to VRAM+GTT, we assume VRAM is *the*
+			 * preferred placement and so try to move it back there
+			 * on the next submission.
+			 */
+			move_to_evict = true;
+		}
+
+		if (move_to_evict)
 			amdgpu_vm_bo_evicted(&bo_va->base);
 		else
 			amdgpu_vm_bo_idle(&bo_va->base);
-- 
2.44.0

Re: [RFC 1/5] drm/amdgpu: Fix migration rate limiting accounting

[Friedrich Vock]

On 08.05.24 20:09, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> The logic assumed any migration attempt worked and therefore would over-
> account the amount of data migrated during buffer re-validation. As a
> consequence client can be unfairly penalised by incorrectly considering
> its migration budget spent.

If the migration failed but data was still moved (which I think could be
the case when we try evicting everything but it still doesn't work?),
shouldn't the eviction movements count towards the ratelimit too?

>
> Fix it by looking at the before and after buffer object backing store and
> only account if there was a change.
>
> FIXME:
> I think this needs a better solution to account for migrations between
> VRAM visible and non-visible portions.

FWIW, I have some WIP patches (not posted on any MLs yet though) that
attempt to solve this issue (+actually enforcing ratelimits) by moving
the ratelimit accounting/enforcement to TTM entirely.

By moving the accounting to TTM we can count moved bytes when we move
them, and don't have to rely on comparing resources to determine whether
moving actually happened. This should address your FIXME as well.

Regards,
Friedrich

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Friedrich Vock <friedrich.vock@gmx.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index ec888fc6ead8..22708954ae68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -784,12 +784,15 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>   		.no_wait_gpu = false,
>   		.resv = bo->tbo.base.resv
>   	};
> +	struct ttm_resource *old_res;
>   	uint32_t domain;
>   	int r;
>
>   	if (bo->tbo.pin_count)
>   		return 0;
>
> +	old_res = bo->tbo.resource;
> +
>   	/* Don't move this buffer if we have depleted our allowance
>   	 * to move it. Don't move anything if the threshold is zero.
>   	 */
> @@ -817,16 +820,29 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>   	amdgpu_bo_placement_from_domain(bo, domain);
>   	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>
> -	p->bytes_moved += ctx.bytes_moved;
> -	if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> -	    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> -		p->bytes_moved_vis += ctx.bytes_moved;
> -
>   	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>   		domain = bo->allowed_domains;
>   		goto retry;
>   	}
>
> +	if (!r) {
> +		struct ttm_resource *new_res = bo->tbo.resource;
> +		bool moved = true;
> +
> +		if (old_res == new_res)
> +			moved = false;
> +		else if (old_res && new_res &&
> +			 old_res->mem_type == new_res->mem_type)
> +			moved = false;
> +
> +		if (moved) {
> +			p->bytes_moved += ctx.bytes_moved;
> +			if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> +			    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> +				p->bytes_moved_vis += ctx.bytes_moved;
> +		}
> +	}
> +
>   	return r;
>   }
>

Re: [RFC 1/5] drm/amdgpu: Fix migration rate limiting accounting

[Tvrtko Ursulin]

On 08/05/2024 20:08, Friedrich Vock wrote:
> On 08.05.24 20:09, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> The logic assumed any migration attempt worked and therefore would over-
>> account the amount of data migrated during buffer re-validation. As a
>> consequence client can be unfairly penalised by incorrectly considering
>> its migration budget spent.
> 
> If the migration failed but data was still moved (which I think could be
> the case when we try evicting everything but it still doesn't work?),
> shouldn't the eviction movements count towards the ratelimit too?

Possibly, which path would that be?

I mean there are definitely more migration which *should not* be counted 
which I think your mini-series approaches more accurately. What this 
patch achieves, in its current RFC form, is reduces the "false-positive" 
migration budget depletions.

So larger improvements aside, point of the series was to illustrate that 
even the things which were said to be working do not seem to. See cover 
letter to see what I thought does not work either well or at all.
>> Fix it by looking at the before and after buffer object backing store and
>> only account if there was a change.
>>
>> FIXME:
>> I think this needs a better solution to account for migrations between
>> VRAM visible and non-visible portions.
> 
> FWIW, I have some WIP patches (not posted on any MLs yet though) that
> attempt to solve this issue (+actually enforcing ratelimits) by moving
> the ratelimit accounting/enforcement to TTM entirely.
> 
> By moving the accounting to TTM we can count moved bytes when we move
> them, and don't have to rely on comparing resources to determine whether
> moving actually happened. This should address your FIXME as well.

Yep, I've seen them. They are not necessarily conflicting with this 
series, potentialy TTM placement flag aside. *If* something like this 
can be kept small and still manage to fix up a few simple things which 
do not appear to work at all at the moment.

For the larger re-work it is quite, well, large and it is not easy to be 
certain the end result would work as expected. IMO it would be best to 
sketch out a larger series which brings some practical and masurable 
change in behaviour before commiting to merge things piecemeal.

For instance I have a niggling feeling the runtime games driver plays 
with placements and domains are not great and wonder if things could be 
cleaner if simplified by letting TTM manage things more, more 
explicitly, and having the list of placements more static. Thinking 
about it seems a step too far for now though.

Regards,

Tvrtko

> 
> Regards,
> Friedrich
> 
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index ec888fc6ead8..22708954ae68 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -784,12 +784,15 @@ static int amdgpu_cs_bo_validate(void *param, 
>> struct amdgpu_bo *bo)
>>           .no_wait_gpu = false,
>>           .resv = bo->tbo.base.resv
>>       };
>> +    struct ttm_resource *old_res;
>>       uint32_t domain;
>>       int r;
>>
>>       if (bo->tbo.pin_count)
>>           return 0;
>>
>> +    old_res = bo->tbo.resource;
>> +
>>       /* Don't move this buffer if we have depleted our allowance
>>        * to move it. Don't move anything if the threshold is zero.
>>        */
>> @@ -817,16 +820,29 @@ static int amdgpu_cs_bo_validate(void *param, 
>> struct amdgpu_bo *bo)
>>       amdgpu_bo_placement_from_domain(bo, domain);
>>       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>
>> -    p->bytes_moved += ctx.bytes_moved;
>> -    if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
>> -        amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>> -        p->bytes_moved_vis += ctx.bytes_moved;
>> -
>>       if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>>           domain = bo->allowed_domains;
>>           goto retry;
>>       }
>>
>> +    if (!r) {
>> +        struct ttm_resource *new_res = bo->tbo.resource;
>> +        bool moved = true;
>> +
>> +        if (old_res == new_res)
>> +            moved = false;
>> +        else if (old_res && new_res &&
>> +             old_res->mem_type == new_res->mem_type)
>> +            moved = false;
>> +
>> +        if (moved) {
>> +            p->bytes_moved += ctx.bytes_moved;
>> +            if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
>> +                amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>> +                p->bytes_moved_vis += ctx.bytes_moved;
>> +        }
>> +    }
>> +
>>       return r;
>>   }
>>

Re: [RFC 0/5] Discussion around eviction improvements

[Tvrtko Ursulin]

On 08/05/2024 19:09, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> Last few days I was looking at the situation with VRAM over subscription, what
> happens versus what perhaps should happen. Browsing through the driver and
> running some simple experiments.
> 
> I ended up with this patch series which, as a disclaimer, may be completely
> wrong but as I found some suspicious things, to me at least, I thought it was a
> good point to stop and request some comments.
> 
> To perhaps summarise what are the main issues I think I found:
> 
>   * Migration rate limiting does not bother knowing if actual migration happened
>     and so can over-account and unfairly penalise.
> 
>   * Migration rate limiting does not even work, at least not for the common case
>     where userspace configures VRAM+GTT. It thinks it can stop migration attempts
>     by playing with bo->allowed_domains vs bo->preferred domains but, both from
>     the code, and from empirical experiments, I see that not working at all. Both
>     masks are identical so fiddling with them achieves nothing.
> 
>   * Idea of the fallback placement only works when VRAM has free space. As soon
>     as it does not, ttm_resource_compatible is happy to leave the buffers in the
>     secondary placement forever.
> 
>   * Driver thinks it will be re-validating evicted buffers on the next submission
>     but it does not for the very common case of VRAM+GTT because it only checks
>     if current placement is *none* of the preferred placements.
> 
> All those problems are addressed in individual patches.
> 
> End result of this series appears to be driver which will try harder to move
> buffers back into VRAM, but will be (more) correctly throttled in doing so by
> the existing rate limiting logic.
> 
> I have run a quick benchmark of Cyberpunk 2077 and cannot say that I saw a
> change but that could be a good thing too. At least I did not break anything,
> perhaps.. On one occassion I did see the rate limiting logic get confused while
> for a period of few minutes it went to a mode where it was constantly giving a
> high migration budget. But that recovered itself when I switched clients and did
> not come back so I don't know. If there is something wrong there I don't think
> it would be caused by any patches in this series.

Since yesterday I also briefly tested with Far Cry New Dawn. One run 
each so possibly doesn't mean anything apart that there isn't a 
regression aka migration throttling is keeping things at bay even with 
increased requests to migrate things back to VRAM:
			
		     before		 after
min/avg/max fps	    36/44/54		37/45/55

Cyberpunk 2077 from yesterday was similarly close:

		26.96/29.59/30.40	29.70/30.00/30.32

I guess the real story is proper DGPU where misplaced buffers have a 
real cost.

Regards,

Tvrtko

> Series is probably rough but should be good enough for dicsussion. I am curious
> to hear if I identified at least something correctly as a real problem.
> 
> It would also be good to hear what are the suggested games to check and see
> whether there is any improvement.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Friedrich Vock <friedrich.vock@gmx.de>
> 
> Tvrtko Ursulin (5):
>    drm/amdgpu: Fix migration rate limiting accounting
>    drm/amdgpu: Actually respect buffer migration budget
>    drm/ttm: Add preferred placement flag
>    drm/amdgpu: Use preferred placement for VRAM+GTT
>    drm/amdgpu: Re-validate evicted buffers
> 
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 38 +++++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 21 ++++++++++--
>   drivers/gpu/drm/ttm/ttm_resource.c         | 13 +++++---
>   include/drm/ttm/ttm_placement.h            |  3 ++
>   5 files changed, 65 insertions(+), 18 deletions(-)
> 

Re: [RFC 0/5] Discussion around eviction improvements

[Christian König]

Just FYI, I've been on sick leave for a while and now trying to catch up.

It will probably be at least week until I can look into this again.

Sorry,
Christian.

Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Last few days I was looking at the situation with VRAM over subscription, what
> happens versus what perhaps should happen. Browsing through the driver and
> running some simple experiments.
>
> I ended up with this patch series which, as a disclaimer, may be completely
> wrong but as I found some suspicious things, to me at least, I thought it was a
> good point to stop and request some comments.
>
> To perhaps summarise what are the main issues I think I found:
>
>   * Migration rate limiting does not bother knowing if actual migration happened
>     and so can over-account and unfairly penalise.
>
>   * Migration rate limiting does not even work, at least not for the common case
>     where userspace configures VRAM+GTT. It thinks it can stop migration attempts
>     by playing with bo->allowed_domains vs bo->preferred domains but, both from
>     the code, and from empirical experiments, I see that not working at all. Both
>     masks are identical so fiddling with them achieves nothing.
>
>   * Idea of the fallback placement only works when VRAM has free space. As soon
>     as it does not, ttm_resource_compatible is happy to leave the buffers in the
>     secondary placement forever.
>
>   * Driver thinks it will be re-validating evicted buffers on the next submission
>     but it does not for the very common case of VRAM+GTT because it only checks
>     if current placement is *none* of the preferred placements.
>
> All those problems are addressed in individual patches.
>
> End result of this series appears to be driver which will try harder to move
> buffers back into VRAM, but will be (more) correctly throttled in doing so by
> the existing rate limiting logic.
>
> I have run a quick benchmark of Cyberpunk 2077 and cannot say that I saw a
> change but that could be a good thing too. At least I did not break anything,
> perhaps.. On one occassion I did see the rate limiting logic get confused while
> for a period of few minutes it went to a mode where it was constantly giving a
> high migration budget. But that recovered itself when I switched clients and did
> not come back so I don't know. If there is something wrong there I don't think
> it would be caused by any patches in this series.
>
> Series is probably rough but should be good enough for dicsussion. I am curious
> to hear if I identified at least something correctly as a real problem.
>
> It would also be good to hear what are the suggested games to check and see
> whether there is any improvement.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>
> Tvrtko Ursulin (5):
>    drm/amdgpu: Fix migration rate limiting accounting
>    drm/amdgpu: Actually respect buffer migration budget
>    drm/ttm: Add preferred placement flag
>    drm/amdgpu: Use preferred placement for VRAM+GTT
>    drm/amdgpu: Re-validate evicted buffers
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 38 +++++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 21 ++++++++++--
>   drivers/gpu/drm/ttm/ttm_resource.c         | 13 +++++---
>   include/drm/ttm/ttm_placement.h            |  3 ++
>   5 files changed, 65 insertions(+), 18 deletions(-)
>

Re: [RFC 0/5] Discussion around eviction improvements

[Tvrtko Ursulin]

On 09/05/2024 13:40, Tvrtko Ursulin wrote:
> 
> On 08/05/2024 19:09, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> Last few days I was looking at the situation with VRAM over 
>> subscription, what
>> happens versus what perhaps should happen. Browsing through the driver 
>> and
>> running some simple experiments.
>>
>> I ended up with this patch series which, as a disclaimer, may be 
>> completely
>> wrong but as I found some suspicious things, to me at least, I thought 
>> it was a
>> good point to stop and request some comments.
>>
>> To perhaps summarise what are the main issues I think I found:
>>
>>   * Migration rate limiting does not bother knowing if actual 
>> migration happened
>>     and so can over-account and unfairly penalise.
>>
>>   * Migration rate limiting does not even work, at least not for the 
>> common case
>>     where userspace configures VRAM+GTT. It thinks it can stop 
>> migration attempts
>>     by playing with bo->allowed_domains vs bo->preferred domains but, 
>> both from
>>     the code, and from empirical experiments, I see that not working 
>> at all. Both
>>     masks are identical so fiddling with them achieves nothing.
>>
>>   * Idea of the fallback placement only works when VRAM has free 
>> space. As soon
>>     as it does not, ttm_resource_compatible is happy to leave the 
>> buffers in the
>>     secondary placement forever.
>>
>>   * Driver thinks it will be re-validating evicted buffers on the next 
>> submission
>>     but it does not for the very common case of VRAM+GTT because it 
>> only checks
>>     if current placement is *none* of the preferred placements.
>>
>> All those problems are addressed in individual patches.
>>
>> End result of this series appears to be driver which will try harder 
>> to move
>> buffers back into VRAM, but will be (more) correctly throttled in 
>> doing so by
>> the existing rate limiting logic.
>>
>> I have run a quick benchmark of Cyberpunk 2077 and cannot say that I 
>> saw a
>> change but that could be a good thing too. At least I did not break 
>> anything,
>> perhaps.. On one occassion I did see the rate limiting logic get 
>> confused while
>> for a period of few minutes it went to a mode where it was constantly 
>> giving a
>> high migration budget. But that recovered itself when I switched 
>> clients and did
>> not come back so I don't know. If there is something wrong there I 
>> don't think
>> it would be caused by any patches in this series.
> 
> Since yesterday I also briefly tested with Far Cry New Dawn. One run 
> each so possibly doesn't mean anything apart that there isn't a 
> regression aka migration throttling is keeping things at bay even with 
> increased requests to migrate things back to VRAM:
> 
>               before         after
> min/avg/max fps        36/44/54        37/45/55
> 
> Cyberpunk 2077 from yesterday was similarly close:
> 
>          26.96/29.59/30.40    29.70/30.00/30.32
> 
> I guess the real story is proper DGPU where misplaced buffers have a 
> real cost.

I found one game which regresses spectacularly badly with this series - 
Assasin's Creed Valhalla. The built-in benchmark at least. The game 
appears to have a working set much larger than the other games I tested, 
around 5GiB total during the benchmark. And for some reason migration 
throttling totally fails to put it in check. I will be investigating 
this shortly.

Regards,

Tvrtko

>> Series is probably rough but should be good enough for dicsussion. I 
>> am curious
>> to hear if I identified at least something correctly as a real problem.
>>
>> It would also be good to hear what are the suggested games to check 
>> and see
>> whether there is any improvement.
>>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>>
>> Tvrtko Ursulin (5):
>>    drm/amdgpu: Fix migration rate limiting accounting
>>    drm/amdgpu: Actually respect buffer migration budget
>>    drm/ttm: Add preferred placement flag
>>    drm/amdgpu: Use preferred placement for VRAM+GTT
>>    drm/amdgpu: Re-validate evicted buffers
>>
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 38 +++++++++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 21 ++++++++++--
>>   drivers/gpu/drm/ttm/ttm_resource.c         | 13 +++++---
>>   include/drm/ttm/ttm_placement.h            |  3 ++
>>   5 files changed, 65 insertions(+), 18 deletions(-)
>>

Re: [RFC 1/5] drm/amdgpu: Fix migration rate limiting accounting

[Friedrich Vock]

On 09.05.24 11:19, Tvrtko Ursulin wrote:
>
> On 08/05/2024 20:08, Friedrich Vock wrote:
>> On 08.05.24 20:09, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>
>>> The logic assumed any migration attempt worked and therefore would
>>> over-
>>> account the amount of data migrated during buffer re-validation. As a
>>> consequence client can be unfairly penalised by incorrectly considering
>>> its migration budget spent.
>>
>> If the migration failed but data was still moved (which I think could be
>> the case when we try evicting everything but it still doesn't work?),
>> shouldn't the eviction movements count towards the ratelimit too?
>
> Possibly, which path would that be?
>
Thinking about it more, the only case where allocation still won't
succeed after evicting everything from a place is the edge case when the
buffer is larger than the place's size.

The most likely condition for this to happen (without the submission
failing entirely because the buffer just doesn't fit anywhere) would be
if the app tries creating a 256MB+ visible-VRAM buffer if resizeable BAR
is disabled.
This case could potentially trigger an allocation failure when trying
with preferred_domains, but retrying with allowed_domains, which
includes GTT, could subsequently work.

> I mean there are definitely more migration which *should not* be
> counted which I think your mini-series approaches more accurately.
> What this patch achieves, in its current RFC form, is reduces the
> "false-positive" migration budget depletions.
>
> So larger improvements aside, point of the series was to illustrate
> that even the things which were said to be working do not seem to. See
> cover letter to see what I thought does not work either well or at all.
Fair point. If this patchset does "wrong"/inaccurate accounting in a
different way that improves the experience, then it's still an improvement.
>>> Fix it by looking at the before and after buffer object backing
>>> store and
>>> only account if there was a change.
>>>
>>> FIXME:
>>> I think this needs a better solution to account for migrations between
>>> VRAM visible and non-visible portions.
>>
>> FWIW, I have some WIP patches (not posted on any MLs yet though) that
>> attempt to solve this issue (+actually enforcing ratelimits) by moving
>> the ratelimit accounting/enforcement to TTM entirely.
>>
>> By moving the accounting to TTM we can count moved bytes when we move
>> them, and don't have to rely on comparing resources to determine whether
>> moving actually happened. This should address your FIXME as well.
>
> Yep, I've seen them. They are not necessarily conflicting with this
> series, potentialy TTM placement flag aside. *If* something like this
> can be kept small and still manage to fix up a few simple things which
> do not appear to work at all at the moment.
>
> For the larger re-work it is quite, well, large and it is not easy to
> be certain the end result would work as expected. IMO it would be best
> to sketch out a larger series which brings some practical and
> masurable change in behaviour before commiting to merge things piecemeal.
>
Yeah, fully agree. Getting something working and iterating on that based
on the results you get seems like the best way forward, that's what I'll
be focusing on for now.

Thanks,
Friedrich

> For instance I have a niggling feeling the runtime games driver plays
> with placements and domains are not great and wonder if things could
> be cleaner if simplified by letting TTM manage things more, more
> explicitly, and having the list of placements more static. Thinking
> about it seems a step too far for now though.
>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Friedrich
>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 26
>>> +++++++++++++++++++++-----
>>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index ec888fc6ead8..22708954ae68 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -784,12 +784,15 @@ static int amdgpu_cs_bo_validate(void *param,
>>> struct amdgpu_bo *bo)
>>>           .no_wait_gpu = false,
>>>           .resv = bo->tbo.base.resv
>>>       };
>>> +    struct ttm_resource *old_res;
>>>       uint32_t domain;
>>>       int r;
>>>
>>>       if (bo->tbo.pin_count)
>>>           return 0;
>>>
>>> +    old_res = bo->tbo.resource;
>>> +
>>>       /* Don't move this buffer if we have depleted our allowance
>>>        * to move it. Don't move anything if the threshold is zero.
>>>        */
>>> @@ -817,16 +820,29 @@ static int amdgpu_cs_bo_validate(void *param,
>>> struct amdgpu_bo *bo)
>>>       amdgpu_bo_placement_from_domain(bo, domain);
>>>       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>>
>>> -    p->bytes_moved += ctx.bytes_moved;
>>> -    if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
>>> -        amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>>> -        p->bytes_moved_vis += ctx.bytes_moved;
>>> -
>>>       if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>>>           domain = bo->allowed_domains;
>>>           goto retry;
>>>       }
>>>
>>> +    if (!r) {
>>> +        struct ttm_resource *new_res = bo->tbo.resource;
>>> +        bool moved = true;
>>> +
>>> +        if (old_res == new_res)
>>> +            moved = false;
>>> +        else if (old_res && new_res &&
>>> +             old_res->mem_type == new_res->mem_type)
>>> +            moved = false;
>>> +
>>> +        if (moved) {
>>> +            p->bytes_moved += ctx.bytes_moved;
>>> +            if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
>>> +                amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>>> +                p->bytes_moved_vis += ctx.bytes_moved;
>>> +        }
>>> +    }
>>> +
>>>       return r;
>>>   }
>>>

Re: [RFC 0/5] Discussion around eviction improvements

[Tvrtko Ursulin]

On 13/05/2024 14:49, Tvrtko Ursulin wrote:
> 
> On 09/05/2024 13:40, Tvrtko Ursulin wrote:
>>
>> On 08/05/2024 19:09, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>
>>> Last few days I was looking at the situation with VRAM over 
>>> subscription, what
>>> happens versus what perhaps should happen. Browsing through the 
>>> driver and
>>> running some simple experiments.
>>>
>>> I ended up with this patch series which, as a disclaimer, may be 
>>> completely
>>> wrong but as I found some suspicious things, to me at least, I 
>>> thought it was a
>>> good point to stop and request some comments.
>>>
>>> To perhaps summarise what are the main issues I think I found:
>>>
>>>   * Migration rate limiting does not bother knowing if actual 
>>> migration happened
>>>     and so can over-account and unfairly penalise.
>>>
>>>   * Migration rate limiting does not even work, at least not for the 
>>> common case
>>>     where userspace configures VRAM+GTT. It thinks it can stop 
>>> migration attempts
>>>     by playing with bo->allowed_domains vs bo->preferred domains but, 
>>> both from
>>>     the code, and from empirical experiments, I see that not working 
>>> at all. Both
>>>     masks are identical so fiddling with them achieves nothing.
>>>
>>>   * Idea of the fallback placement only works when VRAM has free 
>>> space. As soon
>>>     as it does not, ttm_resource_compatible is happy to leave the 
>>> buffers in the
>>>     secondary placement forever.
>>>
>>>   * Driver thinks it will be re-validating evicted buffers on the 
>>> next submission
>>>     but it does not for the very common case of VRAM+GTT because it 
>>> only checks
>>>     if current placement is *none* of the preferred placements.
>>>
>>> All those problems are addressed in individual patches.
>>>
>>> End result of this series appears to be driver which will try harder 
>>> to move
>>> buffers back into VRAM, but will be (more) correctly throttled in 
>>> doing so by
>>> the existing rate limiting logic.
>>>
>>> I have run a quick benchmark of Cyberpunk 2077 and cannot say that I 
>>> saw a
>>> change but that could be a good thing too. At least I did not break 
>>> anything,
>>> perhaps.. On one occassion I did see the rate limiting logic get 
>>> confused while
>>> for a period of few minutes it went to a mode where it was constantly 
>>> giving a
>>> high migration budget. But that recovered itself when I switched 
>>> clients and did
>>> not come back so I don't know. If there is something wrong there I 
>>> don't think
>>> it would be caused by any patches in this series.
>>
>> Since yesterday I also briefly tested with Far Cry New Dawn. One run 
>> each so possibly doesn't mean anything apart that there isn't a 
>> regression aka migration throttling is keeping things at bay even with 
>> increased requests to migrate things back to VRAM:
>>
>>               before         after
>> min/avg/max fps        36/44/54        37/45/55
>>
>> Cyberpunk 2077 from yesterday was similarly close:
>>
>>          26.96/29.59/30.40    29.70/30.00/30.32
>>
>> I guess the real story is proper DGPU where misplaced buffers have a 
>> real cost.
> 
> I found one game which regresses spectacularly badly with this series - 
> Assasin's Creed Valhalla. The built-in benchmark at least. The game 
> appears to have a working set much larger than the other games I tested, 
> around 5GiB total during the benchmark. And for some reason migration 
> throttling totally fails to put it in check. I will be investigating 
> this shortly.

I think that the conclusion is everything I attempted to add relating to 
TTM_PL_PREFERRED does not really work as I initially thought it did. 
Therefore please imagine this series as only containing patches 1, 2 and 5.

(And FWIW it was quite annoying to get to the bottom of since for some 
reason the system exibits some sort of a latching behaviour, where on 
some boots and/or some minutes of runtime things were fine, and then it 
would latch onto a mode where the TTM_PL_PREFERRED induced breakage 
would show. And sometimes this breakage would appear straight away. Odd.)

I still need to test though if the subset of patches manage to achieve 
some positive improvement on their own. It is possible, as patch 5 marks 
more buffers for re-validation so once overcommit subsides they would 
get promoted to preferred placement straight away. And 1&2 are 
notionally fixes for migration throttling so at least in broad sense 
should be still valid as discussion points.

Regards,

Tvrtko

>>> Series is probably rough but should be good enough for dicsussion. I 
>>> am curious
>>> to hear if I identified at least something correctly as a real problem.
>>>
>>> It would also be good to hear what are the suggested games to check 
>>> and see
>>> whether there is any improvement.
>>>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>>>
>>> Tvrtko Ursulin (5):
>>>    drm/amdgpu: Fix migration rate limiting accounting
>>>    drm/amdgpu: Actually respect buffer migration budget
>>>    drm/ttm: Add preferred placement flag
>>>    drm/amdgpu: Use preferred placement for VRAM+GTT
>>>    drm/amdgpu: Re-validate evicted buffers
>>>
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 38 +++++++++++++++++-----
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 21 ++++++++++--
>>>   drivers/gpu/drm/ttm/ttm_resource.c         | 13 +++++---
>>>   include/drm/ttm/ttm_placement.h            |  3 ++
>>>   5 files changed, 65 insertions(+), 18 deletions(-)
>>>

Re: [RFC 0/5] Discussion around eviction improvements

[Christian König]

Am 14.05.24 um 17:14 schrieb Tvrtko Ursulin:
>
> On 13/05/2024 14:49, Tvrtko Ursulin wrote:
>>
>> On 09/05/2024 13:40, Tvrtko Ursulin wrote:
>>>
>>> On 08/05/2024 19:09, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>>
>>>> Last few days I was looking at the situation with VRAM over 
>>>> subscription, what
>>>> happens versus what perhaps should happen. Browsing through the 
>>>> driver and
>>>> running some simple experiments.
>>>>
>>>> I ended up with this patch series which, as a disclaimer, may be 
>>>> completely
>>>> wrong but as I found some suspicious things, to me at least, I 
>>>> thought it was a
>>>> good point to stop and request some comments.
>>>>
>>>> To perhaps summarise what are the main issues I think I found:
>>>>
>>>>   * Migration rate limiting does not bother knowing if actual 
>>>> migration happened
>>>>     and so can over-account and unfairly penalise.
>>>>
>>>>   * Migration rate limiting does not even work, at least not for 
>>>> the common case
>>>>     where userspace configures VRAM+GTT. It thinks it can stop 
>>>> migration attempts
>>>>     by playing with bo->allowed_domains vs bo->preferred domains 
>>>> but, both from
>>>>     the code, and from empirical experiments, I see that not 
>>>> working at all. Both
>>>>     masks are identical so fiddling with them achieves nothing.
>>>>
>>>>   * Idea of the fallback placement only works when VRAM has free 
>>>> space. As soon
>>>>     as it does not, ttm_resource_compatible is happy to leave the 
>>>> buffers in the
>>>>     secondary placement forever.
>>>>
>>>>   * Driver thinks it will be re-validating evicted buffers on the 
>>>> next submission
>>>>     but it does not for the very common case of VRAM+GTT because it 
>>>> only checks
>>>>     if current placement is *none* of the preferred placements.
>>>>
>>>> All those problems are addressed in individual patches.
>>>>
>>>> End result of this series appears to be driver which will try 
>>>> harder to move
>>>> buffers back into VRAM, but will be (more) correctly throttled in 
>>>> doing so by
>>>> the existing rate limiting logic.
>>>>
>>>> I have run a quick benchmark of Cyberpunk 2077 and cannot say that 
>>>> I saw a
>>>> change but that could be a good thing too. At least I did not break 
>>>> anything,
>>>> perhaps.. On one occassion I did see the rate limiting logic get 
>>>> confused while
>>>> for a period of few minutes it went to a mode where it was 
>>>> constantly giving a
>>>> high migration budget. But that recovered itself when I switched 
>>>> clients and did
>>>> not come back so I don't know. If there is something wrong there I 
>>>> don't think
>>>> it would be caused by any patches in this series.
>>>
>>> Since yesterday I also briefly tested with Far Cry New Dawn. One run 
>>> each so possibly doesn't mean anything apart that there isn't a 
>>> regression aka migration throttling is keeping things at bay even 
>>> with increased requests to migrate things back to VRAM:
>>>
>>>               before         after
>>> min/avg/max fps        36/44/54        37/45/55
>>>
>>> Cyberpunk 2077 from yesterday was similarly close:
>>>
>>>          26.96/29.59/30.40    29.70/30.00/30.32
>>>
>>> I guess the real story is proper DGPU where misplaced buffers have a 
>>> real cost.
>>
>> I found one game which regresses spectacularly badly with this series 
>> - Assasin's Creed Valhalla. The built-in benchmark at least. The game 
>> appears to have a working set much larger than the other games I 
>> tested, around 5GiB total during the benchmark. And for some reason 
>> migration throttling totally fails to put it in check. I will be 
>> investigating this shortly.
>
> I think that the conclusion is everything I attempted to add relating 
> to TTM_PL_PREFERRED does not really work as I initially thought it 
> did. Therefore please imagine this series as only containing patches 
> 1, 2 and 5.

Noted (and I had just started to wrap my head around that idea).

>
> (And FWIW it was quite annoying to get to the bottom of since for some 
> reason the system exibits some sort of a latching behaviour, where on 
> some boots and/or some minutes of runtime things were fine, and then 
> it would latch onto a mode where the TTM_PL_PREFERRED induced breakage 
> would show. And sometimes this breakage would appear straight away. Odd.)

Welcome to my world. You improve one use case and four other get a 
penalty. Even when you know the code and potential use cases inside out 
it's really hard to predict how some applications and the core memory 
management behave sometimes.

>
> I still need to test though if the subset of patches manage to achieve 
> some positive improvement on their own. It is possible, as patch 5 
> marks more buffers for re-validation so once overcommit subsides they 
> would get promoted to preferred placement straight away. And 1&2 are 
> notionally fixes for migration throttling so at least in broad sense 
> should be still valid as discussion points.

Yeah, especially 5 kind of makes sense but could potentially lead to 
higher overhead. Need to see how we can better handle that.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>>> Series is probably rough but should be good enough for dicsussion. 
>>>> I am curious
>>>> to hear if I identified at least something correctly as a real 
>>>> problem.
>>>>
>>>> It would also be good to hear what are the suggested games to check 
>>>> and see
>>>> whether there is any improvement.
>>>>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>>>>
>>>> Tvrtko Ursulin (5):
>>>>    drm/amdgpu: Fix migration rate limiting accounting
>>>>    drm/amdgpu: Actually respect buffer migration budget
>>>>    drm/ttm: Add preferred placement flag
>>>>    drm/amdgpu: Use preferred placement for VRAM+GTT
>>>>    drm/amdgpu: Re-validate evicted buffers
>>>>
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 38 
>>>> +++++++++++++++++-----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++--
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 21 ++++++++++--
>>>>   drivers/gpu/drm/ttm/ttm_resource.c         | 13 +++++---
>>>>   include/drm/ttm/ttm_placement.h            |  3 ++
>>>>   5 files changed, 65 insertions(+), 18 deletions(-)
>>>>

Re: [RFC 1/5] drm/amdgpu: Fix migration rate limiting accounting

[Christian König]

Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> The logic assumed any migration attempt worked and therefore would over-
> account the amount of data migrated during buffer re-validation. As a
> consequence client can be unfairly penalised by incorrectly considering
> its migration budget spent.
>
> Fix it by looking at the before and after buffer object backing store and
> only account if there was a change.
>
> FIXME:
> I think this needs a better solution to account for migrations between
> VRAM visible and non-visible portions.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Friedrich Vock <friedrich.vock@gmx.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index ec888fc6ead8..22708954ae68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -784,12 +784,15 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>   		.no_wait_gpu = false,
>   		.resv = bo->tbo.base.resv
>   	};
> +	struct ttm_resource *old_res;
>   	uint32_t domain;
>   	int r;
>   
>   	if (bo->tbo.pin_count)
>   		return 0;
>   
> +	old_res = bo->tbo.resource;
> +
>   	/* Don't move this buffer if we have depleted our allowance
>   	 * to move it. Don't move anything if the threshold is zero.
>   	 */
> @@ -817,16 +820,29 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>   	amdgpu_bo_placement_from_domain(bo, domain);
>   	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>   
> -	p->bytes_moved += ctx.bytes_moved;
> -	if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> -	    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> -		p->bytes_moved_vis += ctx.bytes_moved;
> -
>   	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>   		domain = bo->allowed_domains;
>   		goto retry;
>   	}
>   
> +	if (!r) {
> +		struct ttm_resource *new_res = bo->tbo.resource;
> +		bool moved = true;
> +
> +		if (old_res == new_res)
> +			moved = false;
> +		else if (old_res && new_res &&
> +			 old_res->mem_type == new_res->mem_type)
> +			moved = false;

The old resource might already be destroyed after you return from 
validation. So this here won't work.

Apart from that even when a migration attempt fails the moved bytes 
should be accounted.

When the validation attempt doesn't caused any moves then the bytecount 
here would be zero.

So as far as I can see that is as fair as you can get.

Regards,
Christian.

PS: Looks like our mail servers are once more not very reliable.

If you get mails from me multiple times please just ignore it.

> +
> +		if (moved) {
> +			p->bytes_moved += ctx.bytes_moved;
> +			if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> +			    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> +				p->bytes_moved_vis += ctx.bytes_moved;
> +		}
> +	}
> +
>   	return r;
>   }
>   

Re: [RFC 2/5] drm/amdgpu: Actually respect buffer migration budget

[Christian König]

Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Current code appears to live in a misconception that playing with buffer
> allowed and preferred placements can control the decision on whether
> backing store migration will be attempted or not.
>
> Both from code inspection and from empirical experiments I see that not
> being true, and that both allowed and preferred placement are typically
> set to the same bitmask.

That's not correct for the use case handled here, but see below.

>
> As such, when the code decides to throttle the migration for a client, it
> is in fact not achieving anything. Buffers can still be either migrated or
> not migrated based on the external (to this function and facility) logic.
>
> Fix it by not changing the buffer object placements if the migration
> budget has been spent.
>
> FIXME:
> Is it still required to call validate is the question..
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Friedrich Vock <friedrich.vock@gmx.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 22708954ae68..d07a1dd7c880 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -784,6 +784,7 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>   		.no_wait_gpu = false,
>   		.resv = bo->tbo.base.resv
>   	};
> +	bool migration_allowed = true;
>   	struct ttm_resource *old_res;
>   	uint32_t domain;
>   	int r;
> @@ -805,19 +806,24 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>   			 * visible VRAM if we've depleted our allowance to do
>   			 * that.
>   			 */
> -			if (p->bytes_moved_vis < p->bytes_moved_vis_threshold)
> +			if (p->bytes_moved_vis < p->bytes_moved_vis_threshold) {
>   				domain = bo->preferred_domains;
> -			else
> +			} else {
>   				domain = bo->allowed_domains;
> +				migration_allowed = false;
> +			}
>   		} else {
>   			domain = bo->preferred_domains;
>   		}
>   	} else {
>   		domain = bo->allowed_domains;
> +		migration_allowed = false;
>   	}
>   
>   retry:
> -	amdgpu_bo_placement_from_domain(bo, domain);
> +	if (migration_allowed)
> +		amdgpu_bo_placement_from_domain(bo, domain);

That's completely invalid. Calling amdgpu_bo_placement_from_domain() is 
a mandatory prerequisite for calling ttm_bo_validate();

E.g. the usually code fow is:

/* This initializes bo->placement */
amdgpu_bo_placement_from_domain()

/* Eventually modify bo->placement to fit special requirements */
....

/* Apply the placement to the BO */
ttm_bo_validate(&bo->tbo, &bo->placement, &ctx)

To sum it up bo->placement should be a variable on the stack instead, 
but we never bothered to clean that up.

Regards,
Christian.

> +
>   	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>   
>   	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {

Re: [RFC 1/5] drm/amdgpu: Fix migration rate limiting accounting

[Tvrtko Ursulin]

On 15/05/2024 08:14, Christian König wrote:
> Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> The logic assumed any migration attempt worked and therefore would over-
>> account the amount of data migrated during buffer re-validation. As a
>> consequence client can be unfairly penalised by incorrectly considering
>> its migration budget spent.
>>
>> Fix it by looking at the before and after buffer object backing store and
>> only account if there was a change.
>>
>> FIXME:
>> I think this needs a better solution to account for migrations between
>> VRAM visible and non-visible portions.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index ec888fc6ead8..22708954ae68 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -784,12 +784,15 @@ static int amdgpu_cs_bo_validate(void *param, 
>> struct amdgpu_bo *bo)
>>           .no_wait_gpu = false,
>>           .resv = bo->tbo.base.resv
>>       };
>> +    struct ttm_resource *old_res;
>>       uint32_t domain;
>>       int r;
>>       if (bo->tbo.pin_count)
>>           return 0;
>> +    old_res = bo->tbo.resource;
>> +
>>       /* Don't move this buffer if we have depleted our allowance
>>        * to move it. Don't move anything if the threshold is zero.
>>        */
>> @@ -817,16 +820,29 @@ static int amdgpu_cs_bo_validate(void *param, 
>> struct amdgpu_bo *bo)
>>       amdgpu_bo_placement_from_domain(bo, domain);
>>       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>> -    p->bytes_moved += ctx.bytes_moved;
>> -    if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
>> -        amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>> -        p->bytes_moved_vis += ctx.bytes_moved;
>> -
>>       if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>>           domain = bo->allowed_domains;
>>           goto retry;
>>       }
>> +    if (!r) {
>> +        struct ttm_resource *new_res = bo->tbo.resource;
>> +        bool moved = true;
>> +
>> +        if (old_res == new_res)
>> +            moved = false;
>> +        else if (old_res && new_res &&
>> +             old_res->mem_type == new_res->mem_type)
>> +            moved = false;
> 
> The old resource might already be destroyed after you return from 
> validation. So this here won't work.
> 
> Apart from that even when a migration attempt fails the moved bytes 
> should be accounted.
> 
> When the validation attempt doesn't caused any moves then the bytecount 
> here would be zero.
> 
> So as far as I can see that is as fair as you can get.

Right, I think I suffered a bit of tunnel vision here and completely 
ignore the _ctx_.moved_bytes part. Scratch this one too then.

Regards,

Tvrtko

> 
> Regards,
> Christian.
> 
> PS: Looks like our mail servers are once more not very reliable.
> 
> If you get mails from me multiple times please just ignore it.
> 
>> +
>> +        if (moved) {
>> +            p->bytes_moved += ctx.bytes_moved;
>> +            if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
>> +                amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>> +                p->bytes_moved_vis += ctx.bytes_moved;
>> +        }
>> +    }
>> +
>>       return r;
>>   }
> 

Re: [RFC 2/5] drm/amdgpu: Actually respect buffer migration budget

[Tvrtko Ursulin]

On 15/05/2024 08:20, Christian König wrote:
> Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> Current code appears to live in a misconception that playing with buffer
>> allowed and preferred placements can control the decision on whether
>> backing store migration will be attempted or not.
>>
>> Both from code inspection and from empirical experiments I see that not
>> being true, and that both allowed and preferred placement are typically
>> set to the same bitmask.
> 
> That's not correct for the use case handled here, but see below.

Which part is not correct, that bo->preferred_domains and 
bo->allower_domains are the same bitmask?

>>
>> As such, when the code decides to throttle the migration for a client, it
>> is in fact not achieving anything. Buffers can still be either 
>> migrated or
>> not migrated based on the external (to this function and facility) logic.
>>
>> Fix it by not changing the buffer object placements if the migration
>> budget has been spent.
>>
>> FIXME:
>> Is it still required to call validate is the question..
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 22708954ae68..d07a1dd7c880 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -784,6 +784,7 @@ static int amdgpu_cs_bo_validate(void *param, 
>> struct amdgpu_bo *bo)
>>           .no_wait_gpu = false,
>>           .resv = bo->tbo.base.resv
>>       };
>> +    bool migration_allowed = true;
>>       struct ttm_resource *old_res;
>>       uint32_t domain;
>>       int r;
>> @@ -805,19 +806,24 @@ static int amdgpu_cs_bo_validate(void *param, 
>> struct amdgpu_bo *bo)
>>                * visible VRAM if we've depleted our allowance to do
>>                * that.
>>                */
>> -            if (p->bytes_moved_vis < p->bytes_moved_vis_threshold)
>> +            if (p->bytes_moved_vis < p->bytes_moved_vis_threshold) {
>>                   domain = bo->preferred_domains;
>> -            else
>> +            } else {
>>                   domain = bo->allowed_domains;
>> +                migration_allowed = false;
>> +            }
>>           } else {
>>               domain = bo->preferred_domains;
>>           }
>>       } else {
>>           domain = bo->allowed_domains;
>> +        migration_allowed = false;
>>       }
>>   retry:
>> -    amdgpu_bo_placement_from_domain(bo, domain);
>> +    if (migration_allowed)
>> +        amdgpu_bo_placement_from_domain(bo, domain);
> 
> That's completely invalid. Calling amdgpu_bo_placement_from_domain() is 
> a mandatory prerequisite for calling ttm_bo_validate();
> 
> E.g. the usually code fow is:
> 
> /* This initializes bo->placement */
> amdgpu_bo_placement_from_domain()
> 
> /* Eventually modify bo->placement to fit special requirements */
> ....
> 
> /* Apply the placement to the BO */
> ttm_bo_validate(&bo->tbo, &bo->placement, &ctx)
> 
> To sum it up bo->placement should be a variable on the stack instead, 
> but we never bothered to clean that up.

I am not clear if you agree or not that the current method of trying to 
avoid migration doesn't really do anything?

On stack placements sounds plausible to force migration avoidance by 
putting a single current object placement in that list, if that is what 
you have in mind? Or a specialized flag/version of 
amdgpu_bo_placement_from_domain with an bool input like 
"allow_placement_change"?

Regards,

Tvrtko

> 
> Regards,
> Christian.
> 
>> +
>>       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>       if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
> 

Re: [RFC 2/5] drm/amdgpu: Actually respect buffer migration budget

[Christian König]

Am 15.05.24 um 12:59 schrieb Tvrtko Ursulin:
>
> On 15/05/2024 08:20, Christian König wrote:
>> Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>
>>> Current code appears to live in a misconception that playing with 
>>> buffer
>>> allowed and preferred placements can control the decision on whether
>>> backing store migration will be attempted or not.
>>>
>>> Both from code inspection and from empirical experiments I see that not
>>> being true, and that both allowed and preferred placement are typically
>>> set to the same bitmask.
>>
>> That's not correct for the use case handled here, but see below.
>
> Which part is not correct, that bo->preferred_domains and 
> bo->allower_domains are the same bitmask?

Sorry totally forgot to explain that.

This rate limit here was specially made for OpenGL applications which 
over commit VRAM. In those case preferred_domains will be VRAM only and 
allowed_domains will be VRAM|GTT.

RADV always uses VRAM|GTT for both (which is correct).

>
>>>
>>> As such, when the code decides to throttle the migration for a 
>>> client, it
>>> is in fact not achieving anything. Buffers can still be either 
>>> migrated or
>>> not migrated based on the external (to this function and facility) 
>>> logic.
>>>
>>> Fix it by not changing the buffer object placements if the migration
>>> budget has been spent.
>>>
>>> FIXME:
>>> Is it still required to call validate is the question..
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 22708954ae68..d07a1dd7c880 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -784,6 +784,7 @@ static int amdgpu_cs_bo_validate(void *param, 
>>> struct amdgpu_bo *bo)
>>>           .no_wait_gpu = false,
>>>           .resv = bo->tbo.base.resv
>>>       };
>>> +    bool migration_allowed = true;
>>>       struct ttm_resource *old_res;
>>>       uint32_t domain;
>>>       int r;
>>> @@ -805,19 +806,24 @@ static int amdgpu_cs_bo_validate(void *param, 
>>> struct amdgpu_bo *bo)
>>>                * visible VRAM if we've depleted our allowance to do
>>>                * that.
>>>                */
>>> -            if (p->bytes_moved_vis < p->bytes_moved_vis_threshold)
>>> +            if (p->bytes_moved_vis < p->bytes_moved_vis_threshold) {
>>>                   domain = bo->preferred_domains;
>>> -            else
>>> +            } else {
>>>                   domain = bo->allowed_domains;
>>> +                migration_allowed = false;
>>> +            }
>>>           } else {
>>>               domain = bo->preferred_domains;
>>>           }
>>>       } else {
>>>           domain = bo->allowed_domains;
>>> +        migration_allowed = false;
>>>       }
>>>   retry:
>>> -    amdgpu_bo_placement_from_domain(bo, domain);
>>> +    if (migration_allowed)
>>> +        amdgpu_bo_placement_from_domain(bo, domain);
>>
>> That's completely invalid. Calling amdgpu_bo_placement_from_domain() 
>> is a mandatory prerequisite for calling ttm_bo_validate();
>>
>> E.g. the usually code fow is:
>>
>> /* This initializes bo->placement */
>> amdgpu_bo_placement_from_domain()
>>
>> /* Eventually modify bo->placement to fit special requirements */
>> ....
>>
>> /* Apply the placement to the BO */
>> ttm_bo_validate(&bo->tbo, &bo->placement, &ctx)
>>
>> To sum it up bo->placement should be a variable on the stack instead, 
>> but we never bothered to clean that up.
>
> I am not clear if you agree or not that the current method of trying 
> to avoid migration doesn't really do anything?

I totally agree, but the approach you taken to fix it is just quite 
broken. You can't leave bo->placement uninitialized and expect that 
ttm_bo_validate() won't move the BO.

>
> On stack placements sounds plausible to force migration avoidance by 
> putting a single current object placement in that list, if that is 
> what you have in mind? Or a specialized flag/version of 
> amdgpu_bo_placement_from_domain with an bool input like 
> "allow_placement_change"?

A very rough idea with no guarantee that it actually works:

Add a TTM_PL_FLAG_RATE_LIMITED with all the TTM code to actually figure 
out how many bytes have been moved and how many bytes the current 
operation can move etc...

Friedrich's patches actually looked like quite a step into the right 
direction for that already, so I would start from there.

Then always feed amdgpu_bo_placement_from_domain() with the 
allowed_domains in the CS path and VM validation.

Finally extend amdgpu_bo_placement_from_domain() to take a closer look 
at bo->preferred_domains, similar to how we do for the 
TTM_PL_FLAG_FALLBACK already and set the TTM_PL_FLAG_RATE_LIMITED flag 
as appropriate.

Regards,
Christian.

> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Christian.
>>
>>> +
>>>       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>>       if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>>

Re: [RFC 2/5] drm/amdgpu: Actually respect buffer migration budget

[Tvrtko Ursulin]

On 15/05/2024 15:31, Christian König wrote:
> Am 15.05.24 um 12:59 schrieb Tvrtko Ursulin:
>>
>> On 15/05/2024 08:20, Christian König wrote:
>>> Am 08.05.24 um 20:09 schrieb Tvrtko Ursulin:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>>
>>>> Current code appears to live in a misconception that playing with 
>>>> buffer
>>>> allowed and preferred placements can control the decision on whether
>>>> backing store migration will be attempted or not.
>>>>
>>>> Both from code inspection and from empirical experiments I see that not
>>>> being true, and that both allowed and preferred placement are typically
>>>> set to the same bitmask.
>>>
>>> That's not correct for the use case handled here, but see below.
>>
>> Which part is not correct, that bo->preferred_domains and 
>> bo->allower_domains are the same bitmask?
> 
> Sorry totally forgot to explain that.
> 
> This rate limit here was specially made for OpenGL applications which 
> over commit VRAM. In those case preferred_domains will be VRAM only and 
> allowed_domains will be VRAM|GTT.
> 
> RADV always uses VRAM|GTT for both (which is correct).

Got it, thanks!

>>>> As such, when the code decides to throttle the migration for a 
>>>> client, it
>>>> is in fact not achieving anything. Buffers can still be either 
>>>> migrated or
>>>> not migrated based on the external (to this function and facility) 
>>>> logic.
>>>>
>>>> Fix it by not changing the buffer object placements if the migration
>>>> budget has been spent.
>>>>
>>>> FIXME:
>>>> Is it still required to call validate is the question..
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Friedrich Vock <friedrich.vock@gmx.de>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 12 +++++++++---
>>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 22708954ae68..d07a1dd7c880 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -784,6 +784,7 @@ static int amdgpu_cs_bo_validate(void *param, 
>>>> struct amdgpu_bo *bo)
>>>>           .no_wait_gpu = false,
>>>>           .resv = bo->tbo.base.resv
>>>>       };
>>>> +    bool migration_allowed = true;
>>>>       struct ttm_resource *old_res;
>>>>       uint32_t domain;
>>>>       int r;
>>>> @@ -805,19 +806,24 @@ static int amdgpu_cs_bo_validate(void *param, 
>>>> struct amdgpu_bo *bo)
>>>>                * visible VRAM if we've depleted our allowance to do
>>>>                * that.
>>>>                */
>>>> -            if (p->bytes_moved_vis < p->bytes_moved_vis_threshold)
>>>> +            if (p->bytes_moved_vis < p->bytes_moved_vis_threshold) {
>>>>                   domain = bo->preferred_domains;
>>>> -            else
>>>> +            } else {
>>>>                   domain = bo->allowed_domains;
>>>> +                migration_allowed = false;
>>>> +            }
>>>>           } else {
>>>>               domain = bo->preferred_domains;
>>>>           }
>>>>       } else {
>>>>           domain = bo->allowed_domains;
>>>> +        migration_allowed = false;
>>>>       }
>>>>   retry:
>>>> -    amdgpu_bo_placement_from_domain(bo, domain);
>>>> +    if (migration_allowed)
>>>> +        amdgpu_bo_placement_from_domain(bo, domain);
>>>
>>> That's completely invalid. Calling amdgpu_bo_placement_from_domain() 
>>> is a mandatory prerequisite for calling ttm_bo_validate();
>>>
>>> E.g. the usually code fow is:
>>>
>>> /* This initializes bo->placement */
>>> amdgpu_bo_placement_from_domain()
>>>
>>> /* Eventually modify bo->placement to fit special requirements */
>>> ....
>>>
>>> /* Apply the placement to the BO */
>>> ttm_bo_validate(&bo->tbo, &bo->placement, &ctx)
>>>
>>> To sum it up bo->placement should be a variable on the stack instead, 
>>> but we never bothered to clean that up.
>>
>> I am not clear if you agree or not that the current method of trying 
>> to avoid migration doesn't really do anything?
> 
> I totally agree, but the approach you taken to fix it is just quite 
> broken. You can't leave bo->placement uninitialized and expect that 
> ttm_bo_validate() won't move the BO.

Yep, that much was clear, sorry that I did not explicitly acknowledge 
but just moved on to discussing how to fix it properly.

>> On stack placements sounds plausible to force migration avoidance by 
>> putting a single current object placement in that list, if that is 
>> what you have in mind? Or a specialized flag/version of 
>> amdgpu_bo_placement_from_domain with an bool input like 
>> "allow_placement_change"?
> 
> A very rough idea with no guarantee that it actually works:
> 
> Add a TTM_PL_FLAG_RATE_LIMITED with all the TTM code to actually figure 
> out how many bytes have been moved and how many bytes the current 
> operation can move etc...
> 
> Friedrich's patches actually looked like quite a step into the right 
> direction for that already, so I would start from there.
> 
> Then always feed amdgpu_bo_placement_from_domain() with the 
> allowed_domains in the CS path and VM validation.
> 
> Finally extend amdgpu_bo_placement_from_domain() to take a closer look 
> at bo->preferred_domains, similar to how we do for the 
> TTM_PL_FLAG_FALLBACK already and set the TTM_PL_FLAG_RATE_LIMITED flag 
> as appropriate.

Two things which I kind of don't like with the placement flag idea is 
that a) typically two placements are involved in a move so semantics of 
an individual placement being rate limited does not fully fit. Unless we 
view it is a hack to temporarily "de-prioritise" placement. In which 
case I come to b), where the dynamic games with domains and placements 
perhaps feel a bit hacky. Certainly interactions between placement 
selection in ttm_bo_validate (via ttm_resource_compatible) and 
ttm_bo_alloc_resource could be tricky.

Maybe... I will think about it a bit more.

In the meantime what I played with today is the "stack local" single 
placement in amdgpu_cs_bo_validate. If over the migration budget, I find 
the abo->placement matching the current bo->tbo.resource, and pass it 
into ttm_bo_validate instead of the full placement list.

With that patch and also "drm/amdgpu: Re-validate evicted buffers" from 
this series I did three of Assasin's Creed Valhalla on each kernel and 
it does appear to be keeping the migrations lower. It has no effect on 
average fps, but does appear to improve the minimum, 1% low and 0.1% low.

This is on an APU mind you. No idea how that would affect discrete. But 
it is interesting that marking _more_ buffers for re-validations 
combined with throttling migrations correctly, for this use case, it 
seems to do what I expected.

Regards,

Tvrtko

>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +
>>>>       r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>>>       if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
>>>
>