[PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[Michel Dänzer]

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