Re: [PATCH 08/13] drm/xe/sa: Allow creating suballocator with custom guard size

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Dec 12, 2024 at 02:01:36AM +0100, Michal Wajdeczko wrote:
> Actual xe_sa_manager implementation uses hardcoded 4K to exclude
> it from making suballocations but in upcoming patch we want to
> reuse the xe_sa_manager where such 4K guard is not needed. Add
> another variant of the xe_sa_bo_manager_init() function that
> accepts arbitrary guard size.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_sa.c | 18 ++++++++++++++++--
>  drivers/gpu/drm/xe/xe_sa.h |  9 +++++++--
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_sa.c b/drivers/gpu/drm/xe/xe_sa.c
> index d92d9836fa91..94f05e704a43 100644
> --- a/drivers/gpu/drm/xe/xe_sa.c
> +++ b/drivers/gpu/drm/xe/xe_sa.c
> @@ -32,14 +32,28 @@ static void sa_bo_manager_fini_bo(void *arg)
>  	sa_manager->bo = NULL;
>  }
>  
> -struct xe_sa_manager *xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u32 align)
> +/**
> + * __xe_sa_bo_manager_init() - Create and initialize the suballocator
> + * @tile: the &xe_tile where allocate
> + * @size: number of bytes to allocate
> + * @guard: number of bytes to exclude from suballocations
> + * @align: alignment for each suballocated chunk
> + *
> + * Prepares the suballocation manager for suballocations.
> + *
> + * Return: a pointer to the &xe_sa_manager or an ERR_PTR on failure.
> + */
> +struct xe_sa_manager *__xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u32 guard, u32 align)
>  {
>  	struct xe_device *xe = tile_to_xe(tile);
>  	struct xe_sa_manager *sa_manager;
> -	u32 managed_size = size - SZ_4K;
> +	u32 managed_size;
>  	struct xe_bo *bo;
>  	int ret;
>  
> +	xe_tile_assert(tile, size > guard);
> +	managed_size = size - guard;
> +
>  	sa_manager = drmm_kzalloc(&xe->drm, sizeof(*sa_manager), GFP_KERNEL);
>  	if (!sa_manager)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/gpu/drm/xe/xe_sa.h b/drivers/gpu/drm/xe/xe_sa.h
> index bf55d8c3b5eb..9328b38eff6a 100644
> --- a/drivers/gpu/drm/xe/xe_sa.h
> +++ b/drivers/gpu/drm/xe/xe_sa.h
> @@ -5,6 +5,7 @@
>  #ifndef _XE_SA_H_
>  #define _XE_SA_H_
>  
> +#include <linux/sizes.h>
>  #include <linux/types.h>
>  #include "xe_sa_types.h"
>  
> @@ -12,10 +13,14 @@ struct dma_fence;
>  struct xe_bo;
>  struct xe_tile;
>  
> -struct xe_sa_manager *xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u32 align);
> -
> +struct xe_sa_manager *__xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u32 guard, u32 align);
>  struct drm_suballoc *__xe_sa_bo_new(struct xe_sa_manager *sa_manager, u32 size, gfp_t gfp);
>  
> +static inline struct xe_sa_manager *xe_sa_bo_manager_init(struct xe_tile *tile, u32 size, u32 align)
> +{
> +	return __xe_sa_bo_manager_init(tile, size, SZ_4K, align);

So why is this guard needed? I'm reasoning this was for CS prefetches
but all current uses of the SA (xe_bb.c) do suballocations with CS
prefetch padding. We likely don't need to do this in both places.

I'd say drop prefetch padding from xe_bb.c and move the prefetch
calculation size to here.

Can be done in a follow up but before RBing let's make sure we
understand exactly why this is needed and develop a follow up plan as
the code as is doesn't look right / necessary.

Matt

> +}
> +
>  static inline struct drm_suballoc *xe_sa_bo_new(struct xe_sa_manager *sa_manager, u32 size)
>  {
>  	return __xe_sa_bo_new(sa_manager, size, GFP_KERNEL);
> -- 
> 2.47.1
>