Re: [PATCH v5 6/6] drm/ttm: Switch to using the new res callback

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 09.08.22 um 11:22 schrieb Arunpravin Paneer Selvam:
> Apply new intersect and compatible callback instead
> of having a generic placement range verfications.
>
> v2: Added a separate callback for compatiblilty
>      checks (Christian)

We need to move parts of this patch earlier or the i915 will temporary 
break the driver until this here is applied.

I suggest to have most of this in the first patch and use the existing 
res->start checks as fallback when the new callbacks are not implemented 
yet.

This here can then be the last patch with cleanups and removal of those 
workarounds.

Apart from that looks like a nice cleanup to me.

Regards,
Christian.

> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++++++------------------
>   drivers/gpu/drm/ttm/ttm_bo.c            |  9 +++--
>   drivers/gpu/drm/ttm/ttm_resource.c      |  5 +--
>   3 files changed, 20 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 170935c294f5..7d25a10395c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device *adev, struct ttm_tt *ttm,
>   static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   					    const struct ttm_place *place)
>   {
> -	unsigned long num_pages = bo->resource->num_pages;
>   	struct dma_resv_iter resv_cursor;
> -	struct amdgpu_res_cursor cursor;
>   	struct dma_fence *f;
>   
> +	if (!amdgpu_bo_is_amdgpu_bo(bo))
> +		return ttm_bo_eviction_valuable(bo, place);
> +
>   	/* Swapout? */
>   	if (bo->resource->mem_type == TTM_PL_SYSTEM)
>   		return true;
> @@ -1351,40 +1352,20 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   			return false;
>   	}
>   
> -	switch (bo->resource->mem_type) {
> -	case AMDGPU_PL_PREEMPT:
> -		/* Preemptible BOs don't own system resources managed by the
> -		 * driver (pages, VRAM, GART space). They point to resources
> -		 * owned by someone else (e.g. pageable memory in user mode
> -		 * or a DMABuf). They are used in a preemptible context so we
> -		 * can guarantee no deadlocks and good QoS in case of MMU
> -		 * notifiers or DMABuf move notifiers from the resource owner.
> -		 */
> +	/* Preemptible BOs don't own system resources managed by the
> +	 * driver (pages, VRAM, GART space). They point to resources
> +	 * owned by someone else (e.g. pageable memory in user mode
> +	 * or a DMABuf). They are used in a preemptible context so we
> +	 * can guarantee no deadlocks and good QoS in case of MMU
> +	 * notifiers or DMABuf move notifiers from the resource owner.
> +	 */
> +	if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
>   		return false;
> -	case TTM_PL_TT:
> -		if (amdgpu_bo_is_amdgpu_bo(bo) &&
> -		    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
> -			return false;
> -		return true;
>   
> -	case TTM_PL_VRAM:
> -		/* Check each drm MM node individually */
> -		amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
> -				 &cursor);
> -		while (cursor.remaining) {
> -			if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
> -			    && !(place->lpfn &&
> -				 place->lpfn <= PFN_DOWN(cursor.start)))
> -				return true;
> -
> -			amdgpu_res_next(&cursor, cursor.size);
> -		}
> +	if (bo->resource->mem_type == TTM_PL_TT &&
> +	    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
>   		return false;
>   
> -	default:
> -		break;
> -	}
> -
>   	return ttm_bo_eviction_valuable(bo, place);
>   }
>   
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index c1bd006a5525..f066e8124c50 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>   bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   			      const struct ttm_place *place)
>   {
> +	struct ttm_resource *res = bo->resource;
> +	struct ttm_device *bdev = bo->bdev;
> +
>   	dma_resv_assert_held(bo->base.resv);
>   	if (bo->resource->mem_type == TTM_PL_SYSTEM)
>   		return true;
> @@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   	/* Don't evict this BO if it's outside of the
>   	 * requested placement range
>   	 */
> -	if (place->fpfn >= (bo->resource->start + bo->resource->num_pages) ||
> -	    (place->lpfn && place->lpfn <= bo->resource->start))
> -		return false;
> -
> -	return true;
> +	return ttm_resource_intersects(bdev, res, place, bo->base.size);
>   }
>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index ea67976f1f60..a729c32a1e48 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -316,6 +316,8 @@ static bool ttm_resource_places_compat(struct ttm_resource *res,
>   				       const struct ttm_place *places,
>   				       unsigned num_placement)
>   {
> +	struct ttm_buffer_object *bo = res->bo;
> +	struct ttm_device *bdev = bo->bdev;
>   	unsigned i;
>   
>   	if (res->placement & TTM_PL_FLAG_TEMPORARY)
> @@ -324,8 +326,7 @@ static bool ttm_resource_places_compat(struct ttm_resource *res,
>   	for (i = 0; i < num_placement; i++) {
>   		const struct ttm_place *heap = &places[i];
>   
> -		if (res->start < heap->fpfn || (heap->lpfn &&
> -		    (res->start + res->num_pages) > heap->lpfn))
> +		if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
>   			continue;
>   
>   		if ((res->mem_type == heap->mem_type) &&