Re: [Intel-gfx] [PATCH v2 1/6] drm/ttm: Add new callbacks to ttm res mgr

From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>,
	dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
	intel-gfx@lists.freedesktop.org, nouveau@lists.freedesktop.org
Cc: alexander.deucher@amd.com, luben.tuikov@amd.com,
	christian.koenig@amd.com, matthew.auld@intel.com
Subject: Re: [Intel-gfx] [PATCH v2 1/6] drm/ttm: Add new callbacks to ttm res mgr
Date: Mon, 8 Aug 2022 13:28:30 +0200	[thread overview]
Message-ID: <c6fdc73d-8877-b261-6e46-0094d4bb4527@gmail.com> (raw)
In-Reply-To: <20220725114240.4844-1-Arunpravin.PaneerSelvam@amd.com>

Am 25.07.22 um 13:42 schrieb Arunpravin Paneer Selvam:
> We are adding two new callbacks to ttm resource manager
> function to handle intersection and compatibility of
> placement and resources.
>
> v2: move the amdgpu and ttm_range_manager changes to
>      separate patches (Christian)
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_resource.c | 59 ++++++++++++++++++++++++++++++
>   include/drm/ttm/ttm_resource.h     | 39 ++++++++++++++++++++
>   2 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 20f9adcc3235..4cd31d24c3e7 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -253,6 +253,65 @@ void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
>   }
>   EXPORT_SYMBOL(ttm_resource_free);
>   
> +/**
> + * ttm_resource_intersect - test for intersection
> + *
> + * @bdev: TTM device structure
> + * @res: The resource to test
> + * @place: The placement to test
> + * @size: How many bytes the new allocation needs.
> + *
> + * Test if @res intersects with @place and @size. Used for testing if evictions
> + * are valueable or not.
> + *
> + * Returns true if the res placement intersects with @place and @size.
> + */
> +bool ttm_resource_intersect(struct ttm_device *bdev,
> +			    struct ttm_resource *res,
> +			    const struct ttm_place *place,
> +			    size_t size)
> +{
> +	struct ttm_resource_manager *man;
> +
> +	if (!res)
> +		return false;
> +
> +	man = ttm_manager_type(bdev, res->mem_type);
> +	if (!place || !man->func->intersect)
> +		return true;
> +
> +	return man->func->intersect(man, res, place, size);
> +}
> +
> +/**
> + * ttm_resource_compatible - test for compatibility
> + *
> + * @bdev: TTM device structure
> + * @res: The resource to test
> + * @place: The placement to test
> + * @size: How many bytes the new allocation needs.
> + *
> + * Test if @res compatible with @place and @size.
> + *
> + * Returns true if the res placement compatible with @place and @size.
> + */
> +bool ttm_resource_compatible(struct ttm_device *bdev,
> +			     struct ttm_resource *res,
> +			     const struct ttm_place *place,
> +			     size_t size)
> +{
> +	struct ttm_resource_manager *man;
> +
> +	if (!res)
> +		return false;
> +
> +	man = ttm_manager_type(bdev, res->mem_type);
> +	if (!place || !man->func->compatible)

Well that !place is probably misplaced here.

When no placement is given then that's either illegal or the resource is 
never compatible.

Maybe move that check up to the !res if and return false.

> +		return true;
> +
> +	return man->func->compatible(man, res, place, size);
> +}
> +
>   static bool ttm_resource_places_compat(struct ttm_resource *res,
>   				       const struct ttm_place *places,
>   				       unsigned num_placement)
> diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
> index ca89a48c2460..68042e165c40 100644
> --- a/include/drm/ttm/ttm_resource.h
> +++ b/include/drm/ttm/ttm_resource.h
> @@ -88,6 +88,37 @@ struct ttm_resource_manager_func {
>   	void (*free)(struct ttm_resource_manager *man,
>   		     struct ttm_resource *res);
>   
> +	/**
> +	 * struct ttm_resource_manager_func member intersect
> +	 *
> +	 * @man: Pointer to a memory type manager.
> +	 * @res: Pointer to a struct ttm_resource to be checked.
> +	 * @place: Placement to check against.
> +	 * @size: Size of the check.
> +	 *
> +	 * Test if @res intersects with @place + @size. Used to judge if
> +	 * evictions are valueable or not.
> +	 */
> +	bool (*intersect)(struct ttm_resource_manager *man,
> +			  struct ttm_resource *res,
> +			  const struct ttm_place *place,
> +			  size_t size);
> +
> +	/**
> +	 * struct ttm_resource_manager_func member compatible
> +	 *
> +	 * @man: Pointer to a memory type manager.
> +	 * @res: Pointer to a struct ttm_resource to be checked.
> +	 * @place: Placement to check against.
> +	 * @size: Size of the check.
> +	 *
> +	 * Test if @res compatible with @place + @size.

"Used to check of the need to move the backing store or not."

Apart from that looks good to me.

Regards,
Christian.

> +	 */
> +	bool (*compatible)(struct ttm_resource_manager *man,
> +			   struct ttm_resource *res,
> +			   const struct ttm_place *place,
> +			   size_t size);
> +
>   	/**
>   	 * struct ttm_resource_manager_func member debug
>   	 *
> @@ -329,6 +360,14 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo,
>   		       const struct ttm_place *place,
>   		       struct ttm_resource **res);
>   void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);
> +bool ttm_resource_intersect(struct ttm_device *bdev,
> +			    struct ttm_resource *res,
> +			    const struct ttm_place *place,
> +			    size_t size);
> +bool ttm_resource_compatible(struct ttm_device *bdev,
> +			     struct ttm_resource *res,
> +			     const struct ttm_place *place,
> +			     size_t size);
>   bool ttm_resource_compat(struct ttm_resource *res,
>   			 struct ttm_placement *placement);
>   void ttm_resource_set_bo(struct ttm_resource *res,