Re: [PATCH 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread Michel Dänzer 
On 19/05/17 10:43 AM, John Brooks wrote:
> On Fri, May 19, 2017 at 10:20:37AM +0900, Michel Dänzer wrote:
>> On 19/05/17 12:43 AM, John Brooks wrote:
>>> On Thu, May 18, 2017 at 06:08:09PM +0900, Michel Dänzer wrote:
 From: Michel Dänzer 

 In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
 flag set to CPU visible VRAM with more force.

 For other BOs, this gives another chance to stay in VRAM if they
 happened to lie in the CPU visible part and another BO needs to go
 there.

 This should allow BOs to stay in VRAM longer in some cases.

 Signed-off-by: Michel Dänzer 
>>
>> [...]
>>
 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
 b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
 index 57789b860768..d5ed85026542 100644
 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
 @@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct 
 ttm_buffer_object *bo,
adev->mman.buffer_funcs_ring &&
adev->mman.buffer_funcs_ring->ready == false) {
amdgpu_ttm_placement_from_domain(abo, 
 AMDGPU_GEM_DOMAIN_CPU);
 +  } else if (adev->mc.visible_vram_size < 
 adev->mc.real_vram_size) {
 +  unsigned fpfn = adev->mc.visible_vram_size >> 
 PAGE_SHIFT;
 +  struct drm_mm_node *node = bo->mem.mm_node;
 +  unsigned long pages_left;
 +
 +  for (pages_left = bo->mem.num_pages;
 +   pages_left;
 +   pages_left -= node->size, node++) {
 +  if (node->start < fpfn)
 +  break;
 +  }
 +
 +  if (!pages_left)
 +  goto gtt;
 +
 +  /* Try evicting to the CPU inaccessible part of VRAM
 +   * first, but only set GTT as busy placement, so this
 +   * BO will be evicted to GTT rather than causing other
 +   * BOs to be evicted from VRAM
 +   */
 +  amdgpu_ttm_placement_from_domain(abo, 
 AMDGPU_GEM_DOMAIN_VRAM |
 +   AMDGPU_GEM_DOMAIN_GTT);
 +  abo->placements[0].fpfn = fpfn;
 +  abo->placements[0].lpfn = 0;
 +  abo->placement.busy_placement = >placements[1];
>>>
>>> Are you sure you want to hardcode the placements index? It'll be dependent 
>>> on
>>> the order set up in amdgpu_ttm_placement_init.
>>
>> Yes, see patch 1. Looping over the placements and testing their contents
>> is silly when we know exactly how they were set up. 
> 
> Agreed
> 
>> Or do you mean this code shouldn't call amdgpu_ttm_placement_from_domain at
>> all and just set up the placements itself?
> 
> Calling amdgpu_ttm_placement_from_domain makes sense. I was just imagining a
> scenario where code like this that makes assumptions about the ordering of
> placements in the array would break silently if that order were changed, and
> you'd have to go about finding the places where integer literals were used to
> address specific placements.

Right, if we make changes to amdgpu_ttm_placement_init, we'll need to
audit and possibly update its callers. I think that's reasonable, though
others might disagree. :)

Thanks for your feedback.


-- 
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 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread John Brooks 
On Fri, May 19, 2017 at 10:20:37AM +0900, Michel Dänzer wrote:
> On 19/05/17 12:43 AM, John Brooks wrote:
> > On Thu, May 18, 2017 at 06:08:09PM +0900, Michel Dänzer wrote:
> >> From: Michel Dänzer 
> >>
> >> In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
> >> flag set to CPU visible VRAM with more force.
> >>
> >> For other BOs, this gives another chance to stay in VRAM if they
> >> happened to lie in the CPU visible part and another BO needs to go
> >> there.
> >>
> >> This should allow BOs to stay in VRAM longer in some cases.
> >>
> >> Signed-off-by: Michel Dänzer 
> 
> [...]
> 
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> index 57789b860768..d5ed85026542 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> @@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct 
> >> ttm_buffer_object *bo,
> >>adev->mman.buffer_funcs_ring &&
> >>adev->mman.buffer_funcs_ring->ready == false) {
> >>amdgpu_ttm_placement_from_domain(abo, 
> >> AMDGPU_GEM_DOMAIN_CPU);
> >> +  } else if (adev->mc.visible_vram_size < 
> >> adev->mc.real_vram_size) {
> >> +  unsigned fpfn = adev->mc.visible_vram_size >> 
> >> PAGE_SHIFT;
> >> +  struct drm_mm_node *node = bo->mem.mm_node;
> >> +  unsigned long pages_left;
> >> +
> >> +  for (pages_left = bo->mem.num_pages;
> >> +   pages_left;
> >> +   pages_left -= node->size, node++) {
> >> +  if (node->start < fpfn)
> >> +  break;
> >> +  }
> >> +
> >> +  if (!pages_left)
> >> +  goto gtt;
> >> +
> >> +  /* Try evicting to the CPU inaccessible part of VRAM
> >> +   * first, but only set GTT as busy placement, so this
> >> +   * BO will be evicted to GTT rather than causing other
> >> +   * BOs to be evicted from VRAM
> >> +   */
> >> +  amdgpu_ttm_placement_from_domain(abo, 
> >> AMDGPU_GEM_DOMAIN_VRAM |
> >> +   AMDGPU_GEM_DOMAIN_GTT);
> >> +  abo->placements[0].fpfn = fpfn;
> >> +  abo->placements[0].lpfn = 0;
> >> +  abo->placement.busy_placement = >placements[1];
> > 
> > Are you sure you want to hardcode the placements index? It'll be dependent 
> > on
> > the order set up in amdgpu_ttm_placement_init.
> 
> Yes, see patch 1. Looping over the placements and testing their contents
> is silly when we know exactly how they were set up. 

Agreed

> Or do you mean this code shouldn't call amdgpu_ttm_placement_from_domain at
> all and just set up the placements itself?

Calling amdgpu_ttm_placement_from_domain makes sense. I was just imagining a
scenario where code like this that makes assumptions about the ordering of
placements in the array would break silently if that order were changed, and
you'd have to go about finding the places where integer literals were used to
address specific placements.

> 
> -- 
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer

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

Re: [PATCH 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread Michel Dänzer 
On 19/05/17 12:43 AM, John Brooks wrote:
> On Thu, May 18, 2017 at 06:08:09PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer 
>>
>> In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
>> flag set to CPU visible VRAM with more force.
>>
>> For other BOs, this gives another chance to stay in VRAM if they
>> happened to lie in the CPU visible part and another BO needs to go
>> there.
>>
>> This should allow BOs to stay in VRAM longer in some cases.
>>
>> Signed-off-by: Michel Dänzer 

[...]

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 57789b860768..d5ed85026542 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
>> *bo,
>>  adev->mman.buffer_funcs_ring &&
>>  adev->mman.buffer_funcs_ring->ready == false) {
>>  amdgpu_ttm_placement_from_domain(abo, 
>> AMDGPU_GEM_DOMAIN_CPU);
>> +} else if (adev->mc.visible_vram_size < 
>> adev->mc.real_vram_size) {
>> +unsigned fpfn = adev->mc.visible_vram_size >> 
>> PAGE_SHIFT;
>> +struct drm_mm_node *node = bo->mem.mm_node;
>> +unsigned long pages_left;
>> +
>> +for (pages_left = bo->mem.num_pages;
>> + pages_left;
>> + pages_left -= node->size, node++) {
>> +if (node->start < fpfn)
>> +break;
>> +}
>> +
>> +if (!pages_left)
>> +goto gtt;
>> +
>> +/* Try evicting to the CPU inaccessible part of VRAM
>> + * first, but only set GTT as busy placement, so this
>> + * BO will be evicted to GTT rather than causing other
>> + * BOs to be evicted from VRAM
>> + */
>> +amdgpu_ttm_placement_from_domain(abo, 
>> AMDGPU_GEM_DOMAIN_VRAM |
>> + AMDGPU_GEM_DOMAIN_GTT);
>> +abo->placements[0].fpfn = fpfn;
>> +abo->placements[0].lpfn = 0;
>> +abo->placement.busy_placement = >placements[1];
> 
> Are you sure you want to hardcode the placements index? It'll be dependent on
> the order set up in amdgpu_ttm_placement_init.

Yes, see patch 1. Looping over the placements and testing their contents
is silly when we know exactly how they were set up. Or do you mean this
code shouldn't call amdgpu_ttm_placement_from_domain at all and just set
up the placements itself?


-- 
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 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread John Brooks 
On Thu, May 18, 2017 at 06:08:09PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer 
> 
> In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
> flag set to CPU visible VRAM with more force.
> 
> For other BOs, this gives another chance to stay in VRAM if they
> happened to lie in the CPU visible part and another BO needs to go
> there.
> 
> This should allow BOs to stay in VRAM longer in some cases.
> 
> Signed-off-by: Michel Dänzer 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 19 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 27 +++
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 41ee353b22c8..d45c2325c61a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -963,11 +963,20 @@ int amdgpu_bo_fault_reserve_notify(struct 
> ttm_buffer_object *bo)
>AMDGPU_GEM_DOMAIN_GTT);
>   abo->placements[0].lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>  
> - /* Only set GTT as busy placement; if there is no space in CPU visible
> -  * VRAM, move this BO to GTT instead of evicting other BOs
> -  */
> - abo->placement.busy_placement = >placements[1];
> - abo->placement.num_busy_placement = 1;
> + if (abo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> + /* Only set VRAM as normal placement; if there is no space in
> +  * CPU visible VRAM, evict other BOs, only fall back to GTT as
> +  * last resort
> +  */
> + abo->placement.num_placement = 1;
> + } else {
> + /* Only set GTT as busy placement; if there is no space in CPU
> +  * visible VRAM, move this BO to GTT instead of evicting other
> +  * BOs
> +  */
> + abo->placement.busy_placement = >placements[1];
> + abo->placement.num_busy_placement = 1;
> + }
>  
>   return ttm_bo_validate(bo, >placement, false, false);
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 57789b860768..d5ed85026542 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
> *bo,
>   adev->mman.buffer_funcs_ring &&
>   adev->mman.buffer_funcs_ring->ready == false) {
>   amdgpu_ttm_placement_from_domain(abo, 
> AMDGPU_GEM_DOMAIN_CPU);
> + } else if (adev->mc.visible_vram_size < 
> adev->mc.real_vram_size) {
> + unsigned fpfn = adev->mc.visible_vram_size >> 
> PAGE_SHIFT;
> + struct drm_mm_node *node = bo->mem.mm_node;
> + unsigned long pages_left;
> +
> + for (pages_left = bo->mem.num_pages;
> +  pages_left;
> +  pages_left -= node->size, node++) {
> + if (node->start < fpfn)
> + break;
> + }
> +
> + if (!pages_left)
> + goto gtt;
> +
> + /* Try evicting to the CPU inaccessible part of VRAM
> +  * first, but only set GTT as busy placement, so this
> +  * BO will be evicted to GTT rather than causing other
> +  * BOs to be evicted from VRAM
> +  */
> + amdgpu_ttm_placement_from_domain(abo, 
> AMDGPU_GEM_DOMAIN_VRAM |
> +  AMDGPU_GEM_DOMAIN_GTT);
> + abo->placements[0].fpfn = fpfn;
> + abo->placements[0].lpfn = 0;
> + abo->placement.busy_placement = >placements[1];

Are you sure you want to hardcode the placements index? It'll be dependent on
the order set up in amdgpu_ttm_placement_init.

> + abo->placement.num_busy_placement = 1;
>   } else {
> +gtt:
>   amdgpu_ttm_placement_from_domain(abo, 
> AMDGPU_GEM_DOMAIN_GTT);
>   /* Set an upper limit to force directly allocating
>* address space for the BO.
> -- 
> 2.11.0
> 
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[PATCH 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread Michel Dänzer 
From: Michel Dänzer 

In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
flag set to CPU visible VRAM with more force.

For other BOs, this gives another chance to stay in VRAM if they
happened to lie in the CPU visible part and another BO needs to go
there.

This should allow BOs to stay in VRAM longer in some cases.

Signed-off-by: Michel Dänzer 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 19 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 27 +++
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 41ee353b22c8..d45c2325c61a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -963,11 +963,20 @@ int amdgpu_bo_fault_reserve_notify(struct 
ttm_buffer_object *bo)
 AMDGPU_GEM_DOMAIN_GTT);
abo->placements[0].lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
 
-   /* Only set GTT as busy placement; if there is no space in CPU visible
-* VRAM, move this BO to GTT instead of evicting other BOs
-*/
-   abo->placement.busy_placement = >placements[1];
-   abo->placement.num_busy_placement = 1;
+   if (abo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+   /* Only set VRAM as normal placement; if there is no space in
+* CPU visible VRAM, evict other BOs, only fall back to GTT as
+* last resort
+*/
+   abo->placement.num_placement = 1;
+   } else {
+   /* Only set GTT as busy placement; if there is no space in CPU
+* visible VRAM, move this BO to GTT instead of evicting other
+* BOs
+*/
+   abo->placement.busy_placement = >placements[1];
+   abo->placement.num_busy_placement = 1;
+   }
 
return ttm_bo_validate(bo, >placement, false, false);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 57789b860768..d5ed85026542 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
adev->mman.buffer_funcs_ring &&
adev->mman.buffer_funcs_ring->ready == false) {
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_CPU);
+   } else if (adev->mc.visible_vram_size < 
adev->mc.real_vram_size) {
+   unsigned fpfn = adev->mc.visible_vram_size >> 
PAGE_SHIFT;
+   struct drm_mm_node *node = bo->mem.mm_node;
+   unsigned long pages_left;
+
+   for (pages_left = bo->mem.num_pages;
+pages_left;
+pages_left -= node->size, node++) {
+   if (node->start < fpfn)
+   break;
+   }
+
+   if (!pages_left)
+   goto gtt;
+
+   /* Try evicting to the CPU inaccessible part of VRAM
+* first, but only set GTT as busy placement, so this
+* BO will be evicted to GTT rather than causing other
+* BOs to be evicted from VRAM
+*/
+   amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_VRAM |
+AMDGPU_GEM_DOMAIN_GTT);
+   abo->placements[0].fpfn = fpfn;
+   abo->placements[0].lpfn = 0;
+   abo->placement.busy_placement = >placements[1];
+   abo->placement.num_busy_placement = 1;
} else {
+gtt:
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_GTT);
/* Set an upper limit to force directly allocating
 * address space for the BO.
-- 
2.11.0

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