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 <[email protected]> >>>> >>>> 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 <[email protected]> >> >> [...] >> >>>> 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 _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
