On 20/05/17 12:52 AM, Marek Olšák wrote: > On Fri, May 19, 2017 at 9:03 AM, Michel Dänzer <michel at daenzer.net> 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
