Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[Michel Dänzer]

On 20/05/17 12:21 PM, John Brooks wrote:
> On Sat, May 20, 2017 at 10:27:14AM +0900, Michel Dänzer wrote:
>> On 20/05/17 04:23 AM, John Brooks wrote:
>>> On Fri, May 19, 2017 at 04:03:28PM +0900, Michel Dänzer wrote:
 On 19/05/17 12:04 PM, John Brooks wrote:
> Set GTT as the busy placement for newly created BOs that have the
> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
> established BOs to be evicted from visible VRAM.

 This is an interesting idea, but there are some issues with this patch.
>>
>> [...]
>>
> + flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> + bo->placement.num_placement = 1;
> + bo->placement.num_busy_placement = 1;
> + bo->placement.busy_placement = >placement.placement[1];
> + }

 The original placements set by amdgpu_fill_placement_to_bo need to be
 restored before returning from this function, otherwise I suspect such
 BOs which end up being created in GTT will stay there permanently.

>>>
>>> I'm curious, what makes you think that the BOs cannot move back to VRAM
>>> otherwise? VRAM is still in the placements and prefered/allowed domains, 
>>> just
>>> not in the busy placements.
>>
>> If there is not enough free space when the BO is created, there probably
>> won't be either when it's validated for GPU command streams later.
>>
>>
 Does the patch still help for Dying Light if you fix this?
>>
>> Please test this. The result should tell us whether the problem with
>> Dying Light is really pressure on CPU visible VRAM, or something else.
>>
> 
> I did some tests. The patch still helps if I restore the old placement values
> after ttm_bo_init_reserved. But while doing this, I made another observation
> that throws a wrench into things: It *does* kill performance if I remove
> AMDGPU_GEM_DOMAIN_GTT from bo->prefered_domains. I think that GTT getting into
> prefered_domains was what "fixed" the game this whole time and the BO creation
> was irrelevant.

Ah, right, so the problem I was thinking of was actually caused by the
second hunk, not the first one. :) amdgpu_cs_bo_validate re-generates
the placements for each command stream according to bo->prefered_domains
or bo->allowed_domains.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[Michel Dänzer]

On 20/05/17 04:23 AM, John Brooks wrote:
> On Fri, May 19, 2017 at 04:03:28PM +0900, Michel Dänzer wrote:
>> On 19/05/17 12:04 PM, John Brooks wrote:
>>> Set GTT as the busy placement for newly created BOs that have the
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
>>> established BOs to be evicted from visible VRAM.
>>
>> This is an interesting idea, but there are some issues with this patch.

[...]

>>> +   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
>>> +   bo->placement.num_placement = 1;
>>> +   bo->placement.num_busy_placement = 1;
>>> +   bo->placement.busy_placement = >placement.placement[1];
>>> +   }
>>
>> The original placements set by amdgpu_fill_placement_to_bo need to be
>> restored before returning from this function, otherwise I suspect such
>> BOs which end up being created in GTT will stay there permanently.
>>
> 
> I'm curious, what makes you think that the BOs cannot move back to VRAM
> otherwise? VRAM is still in the placements and prefered/allowed domains, just
> not in the busy placements.

If there is not enough free space when the BO is created, there probably
won't be either when it's validated for GPU command streams later.


>> Does the patch still help for Dying Light if you fix this?

Please test this. The result should tell us whether the problem with
Dying Light is really pressure on CPU visible VRAM, or something else.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[Michel Dänzer]

On 20/05/17 12:52 AM, Marek Olšák wrote:
> On Fri, May 19, 2017 at 9:03 AM, Michel Dänzer  wrote:
>> On 19/05/17 12:04 PM, John Brooks wrote:
>>> Set GTT as the busy placement for newly created BOs that have the
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
>>> established BOs to be evicted from visible VRAM.
>>
>> This is an interesting idea, but there are some issues with this patch.
>>
>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 365883d..655718a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device 
>>> *adev,
>>>  #endif
>>>
>>>   amdgpu_fill_placement_to_bo(bo, placement);
>>> +
>>> + /* This is a new BO that wants to be CPU-visible; set GTT as the busy
>>> +  * placement so that it does not evict established BOs from visible 
>>> VRAM.
>>> +  */
>>> + if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
>>
>> Should be something like
>>
>> if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
>>
>> otherwise it would also match e.g. BOs with domain ==
>> AMDGPU_GEM_DOMAIN_GTT and the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
>> flag set (not that this makes sense, but there's nothing to prevent it).
> 
> I think it should be like this, which trivially rules out GTT and
> VRAM|GTT cases:
> if (domain == AMDGPU_GEM_DOMAIN_VRAM &&

That won't work as is due to the second hunk of the patch, which adds in
AMDGPU_GEM_DOMAIN_GTT before calling this function.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[John Brooks]

On Fri, May 19, 2017 at 04:03:28PM +0900, Michel Dänzer wrote:
> On 19/05/17 12:04 PM, John Brooks wrote:
> > Set GTT as the busy placement for newly created BOs that have the
> > AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
> > established BOs to be evicted from visible VRAM.
> 
> This is an interesting idea, but there are some issues with this patch.
> 
> 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 365883d..655718a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device 
> > *adev,
> >  #endif
> >  
> > amdgpu_fill_placement_to_bo(bo, placement);
> > +
> > +   /* This is a new BO that wants to be CPU-visible; set GTT as the busy
> > +* placement so that it does not evict established BOs from visible 
> > VRAM.
> > +*/
> > +   if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
> 
> Should be something like
> 
>   if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
> 
> otherwise it would also match e.g. BOs with domain ==
> AMDGPU_GEM_DOMAIN_GTT and the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
> flag set (not that this makes sense, but there's nothing to prevent it).
> 
> 
> > +   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> > +   bo->placement.num_placement = 1;
> > +   bo->placement.num_busy_placement = 1;
> > +   bo->placement.busy_placement = >placement.placement[1];
> > +   }
> 
> The original placements set by amdgpu_fill_placement_to_bo need to be
> restored before returning from this function, otherwise I suspect such
> BOs which end up being created in GTT will stay there permanently.
> 

I'm curious, what makes you think that the BOs cannot move back to VRAM
otherwise? VRAM is still in the placements and prefered/allowed domains, just
not in the busy placements.

> Does the patch still help for Dying Light if you fix this?
> 
> The patch as is doesn't seem to make any difference for my dirt-rally
> test case.
> 
> 
> > @@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> > memset(, 0,
> >(AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
> >  
> > +
> > +   /* New CPU-visible BOs will have GTT set as their busy placement */
> > +   if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
> > +   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> 
> Make this
> 
>   if ((domain & AMDGPU_GEM_DOMAIN_VRAM) &&
>   (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)) {
> 
> to match the coding style.
> 
> 
> -- 
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer

Thanks very much for your feedback. I'll send an updated patch soon.

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

Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[Michel Dänzer]

On 19/05/17 12:04 PM, John Brooks wrote:
> Set GTT as the busy placement for newly created BOs that have the
> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
> established BOs to be evicted from visible VRAM.

This is an interesting idea, but there are some issues with this patch.


> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 365883d..655718a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device 
> *adev,
>  #endif
>  
>   amdgpu_fill_placement_to_bo(bo, placement);
> +
> + /* This is a new BO that wants to be CPU-visible; set GTT as the busy
> +  * placement so that it does not evict established BOs from visible 
> VRAM.
> +  */
> + if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&

Should be something like

if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&

otherwise it would also match e.g. BOs with domain ==
AMDGPU_GEM_DOMAIN_GTT and the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
flag set (not that this makes sense, but there's nothing to prevent it).


> + flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> + bo->placement.num_placement = 1;
> + bo->placement.num_busy_placement = 1;
> + bo->placement.busy_placement = >placement.placement[1];
> + }

The original placements set by amdgpu_fill_placement_to_bo need to be
restored before returning from this function, otherwise I suspect such
BOs which end up being created in GTT will stay there permanently.

Does the patch still help for Dying Light if you fix this?

The patch as is doesn't seem to make any difference for my dirt-rally
test case.


> @@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>   memset(, 0,
>  (AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
>  
> +
> + /* New CPU-visible BOs will have GTT set as their busy placement */
> + if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
> + flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {

Make this

if ((domain & AMDGPU_GEM_DOMAIN_VRAM) &&
(flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)) {

to match the coding style.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[zhoucm1]

On 2017年05月19日 11:04, John Brooks wrote:

Set GTT as the busy placement for newly created BOs that have the
AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
established BOs to be evicted from visible VRAM.
You this patch is consistent with my opinion I mentioned in Marek thread 
(Plan: BO move throttling for visible VRAM evictions).
"I agree on this opinion, from our performance tuning experiment, this 
case indeed often happen especially under vram memory pressure. 
redirecting to GTT is better than heavy eviction between VRAM and GTT.
But we should get a condition for redirecting (eviction counter?), 
otherwise, BO have no change back to prefer domain."
You're using busy placement to solve it, then BO is trying its prefer 
domain first, so it has chance back to prefer domain when memory is free.
Although it cannot prevent evil program occupying memory, it still looks 
reasonable, since we can gain more from heavy eviction under vram 
pressure in most cases.


Above all, I vote 'Yes' to your idea, but I also want to listen what 
others' voice is.


Thanks,
David Zhou


Signed-off-by: John Brooks 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 365883d..655718a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
  #endif
  
  	amdgpu_fill_placement_to_bo(bo, placement);

+
+   /* This is a new BO that wants to be CPU-visible; set GTT as the busy
+* placement so that it does not evict established BOs from visible 
VRAM.
+*/
+   if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
+   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+   bo->placement.num_placement = 1;
+   bo->placement.num_busy_placement = 1;
+   bo->placement.busy_placement = >placement.placement[1];
+   }
+
/* Kernel allocation are uninterruptible */
  
  	initial_bytes_moved = atomic64_read(>num_bytes_moved);

@@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
memset(, 0,
   (AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
  
+

+   /* New CPU-visible BOs will have GTT set as their busy placement */
+   if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
+   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+   domain |= AMDGPU_GEM_DOMAIN_GTT;
+   }
+
amdgpu_ttm_placement_init(adev, ,
  placements, domain, flags);
  


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

[PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[John Brooks]

Set GTT as the busy placement for newly created BOs that have the
AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
established BOs to be evicted from visible VRAM.

Signed-off-by: John Brooks 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 365883d..655718a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 #endif
 
amdgpu_fill_placement_to_bo(bo, placement);
+
+   /* This is a new BO that wants to be CPU-visible; set GTT as the busy
+* placement so that it does not evict established BOs from visible 
VRAM.
+*/
+   if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
+   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+   bo->placement.num_placement = 1;
+   bo->placement.num_busy_placement = 1;
+   bo->placement.busy_placement = >placement.placement[1];
+   }
+
/* Kernel allocation are uninterruptible */
 
initial_bytes_moved = atomic64_read(>num_bytes_moved);
@@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
memset(, 0,
   (AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
 
+
+   /* New CPU-visible BOs will have GTT set as their busy placement */
+   if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
+   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+   domain |= AMDGPU_GEM_DOMAIN_GTT;
+   }
+
amdgpu_ttm_placement_init(adev, ,
  placements, domain, flags);
 
-- 
2.7.4

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