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

[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 <michel.daenzer at amd.com>
>>>>
>>>> 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 <michel.daenzer at amd.com>
>>
>> [...]
>>
>>>> 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 = &abo->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