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
