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

[Michel Dänzer]

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).


> +	    flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> +		bo->placement.num_placement = 1;
> +		bo->placement.num_busy_placement = 1;
> +		bo->placement.busy_placement = &bo->placement.placement[1];
> +	}

The original placements set by amdgpu_fill_placement_to_bo need to be
restored before returning from this function, otherwise I suspect such
BOs which end up being created in GTT will stay there permanently.

Does the patch still help for Dying Light if you fix this?

The patch as is doesn't seem to make any difference for my dirt-rally
test case.


> @@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>  	memset(&placements, 0,
>  	       (AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
>  
> +
> +	/* New CPU-visible BOs will have GTT set as their busy placement */
> +	if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
> +	    flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {

Make this

	if ((domain & AMDGPU_GEM_DOMAIN_VRAM) &&
	    (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)) {

to match the coding style.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer