Re: [Intel-gfx] [PATCH v3 4/6] drm/i915: Implement intersect/compatible functions

[Matthew Auld <matthew.auld@intel.com>]

On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
> Implemented a new intersect and compatible callback function
> fetching start offset from drm buddy allocator.
> 
> v2: move the bits that are specific to buddy_man (Matthew)
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 39 +-----------
>   drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++
>   2 files changed, 64 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 70e2ed4e99df..54eead15d74b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct ttm_buffer_object *bo,
>   	if (!i915_gem_object_evictable(obj))
>   		return false;
>   
> -	switch (res->mem_type) {
> -	case I915_PL_LMEM0: {
> -		struct ttm_resource_manager *man =
> -			ttm_manager_type(bo->bdev, res->mem_type);
> -		struct i915_ttm_buddy_resource *bman_res =
> -			to_ttm_buddy_resource(res);
> -		struct drm_buddy *mm = bman_res->mm;
> -		struct drm_buddy_block *block;
> -
> -		if (!place->fpfn && !place->lpfn)
> -			return true;
> -
> -		GEM_BUG_ON(!place->lpfn);
> -
> -		/*
> -		 * If we just want something mappable then we can quickly check
> -		 * if the current victim resource is using any of the CPU
> -		 * visible portion.
> -		 */
> -		if (!place->fpfn &&
> -		    place->lpfn == i915_ttm_buddy_man_visible_size(man))
> -			return bman_res->used_visible_size > 0;
> -
> -		/* Real range allocation */
> -		list_for_each_entry(block, &bman_res->blocks, link) {
> -			unsigned long fpfn =
> -				drm_buddy_block_offset(block) >> PAGE_SHIFT;
> -			unsigned long lpfn = fpfn +
> -				(drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
> -
> -			if (place->fpfn < lpfn && place->lpfn > fpfn)
> -				return true;
> -		}
> -		return false;
> -	} default:
> -		break;
> -	}
> +	if (res->mem_type == I915_PL_LMEM0)
> +		return ttm_bo_eviction_valuable(bo, place);

We should be able to drop the mem_type == I915_PL_LMEM0 check here I 
think, and just unconditionally do:

return ttm_bo_eviction_valuable(bo, place);

>   
>   	return true;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> index a5109548abc0..9d2a31154d58 100644
> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man,
>   	kfree(bman_res);
>   }
>   
> +static bool i915_ttm_buddy_man_intersect(struct ttm_resource_manager *man,

Nit: intersects

> +					 struct ttm_resource *res,
> +					 const struct ttm_place *place,
> +					 size_t size)
> +{
> +	struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res);
> +	u32 start, num_pages = PFN_UP(size);
> +	struct drm_buddy_block *block;
> +
> +	if (!place->fpfn && !place->lpfn)
> +		return true;
> +
> +	/*
> +	 * If we just want something mappable then we can quickly check
> +	 * if the current victim resource is using any of the CP
> +	 * visible portion.
> +	 */
> +	if (!place->fpfn &&
> +	    place->lpfn == i915_ttm_buddy_man_visible_size(man))
> +		return bman_res->used_visible_size > 0;
> +
> +	/* Check each drm buddy block individually */
> +	list_for_each_entry(block, &bman_res->blocks, link) {
> +		start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
> +		/* Don't evict BOs outside of the requested placement range */
> +		if (place->fpfn >= (start + num_pages) ||
> +		    (place->lpfn && place->lpfn <= start))
> +			return false;
> +	}
> +
> +	return true;

We need to account for the block size somewhere. Also same bug in the 
amdgpu patch it seems. We also need to do this the other way around and 
keep checking until we find something that overlaps, for example if the 
first block doesn't intersect/overlap we will incorrectly return false 
here, even if one of the other blocks does intersect.

list_for_each_entry() {
     fpfn = drm_buddy_block_size(mm, block) >> PAGE_SHIFT;
     lpfn = fpfn + drm_buddy_block_size(mm, block) >> PAGE_SHIFT);

     if (place->fpfn < lpfn && place->lpfn > fpfn)
         return true;
}

return false;

> +}
> +
> +static bool i915_ttm_buddy_man_compatible(struct ttm_resource_manager *man,
> +					  struct ttm_resource *res,
> +					  const struct ttm_place *place,
> +					  size_t size)
> +{
> +	struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res);
> +	u32 start, num_pages = PFN_UP(size);
> +	struct drm_buddy_block *block;
> +
> +	if (!place->fpfn && !place->lpfn)
> +		return true;
> +
> +	if (!place->fpfn &&
> +	    place->lpfn == i915_ttm_buddy_man_visible_size(man))
> +		return bman_res->used_visible_size == res->num_pages;
> +
> +	/* Check each drm buddy block individually */
> +	list_for_each_entry(block, &bman_res->blocks, link) {
> +		start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
> +		if (start < place->fpfn ||
> +		    (place->lpfn && (start + num_pages) > place->lpfn))

Same here. We need to consider the block size/range.

Otherwise I think looks good.

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>   static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>   				     struct drm_printer *printer)
>   {
> @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>   static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = {
>   	.alloc = i915_ttm_buddy_man_alloc,
>   	.free = i915_ttm_buddy_man_free,
> +	.intersects = i915_ttm_buddy_man_intersect,
> +	.compatible = i915_ttm_buddy_man_compatible,
>   	.debug = i915_ttm_buddy_man_debug,
>   };
>