Re: [PATCH v2 2/2] drm/xe: Move VRAM manager to struct xe_vram_region

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Feb 03, 2025 at 02:53:59PM +0100, Piórkowski, Piotr wrote:
> From: Piotr Piórkowski <piotr.piorkowski@intel.com>
> 
> VRAM manager is related directly to struct xe_vram_region so it
> should be inside this structure.
> Let's move the VRAM to struct xe_vram_region.
> 
> v2:
>  - remove xe_vram_region pointer from xe_ttm_vram_mgr
>  - stop use dynamic alloaction for xe_ttm_vram_mgr in xe_vram_region
>  - rename struct xe_ttm_vram_mgr vram_mgr to ttm

When I read this I thought it was too much in a single patch,
but looking the patch below I got impressed on how clean that
became.

Thank you so much for addressing that.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bo.c                 | 10 ++++++++--
>  drivers/gpu/drm/xe/xe_device_types.h       |  6 +++---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c |  2 +-
>  drivers/gpu/drm/xe/xe_tile.c               |  6 +-----
>  drivers/gpu/drm/xe/xe_ttm_vram_mgr.c       |  1 -
>  drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h |  4 ----
>  6 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 095794200438..c2724847ae25 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -145,10 +145,13 @@ static struct xe_vram_region *res_to_mem_region(struct ttm_resource *res)
>  {
>  	struct xe_device *xe = ttm_to_xe_device(res->bo->bdev);
>  	struct ttm_resource_manager *mgr;
> +	struct xe_ttm_vram_mgr *vram_mgr;
>  
>  	xe_assert(xe, resource_is_vram(res));
>  	mgr = ttm_manager_type(&xe->ttm, res->mem_type);
> -	return to_xe_ttm_vram_mgr(mgr)->vram;
> +	vram_mgr = to_xe_ttm_vram_mgr(mgr);
> +
> +	return container_of(vram_mgr, struct xe_vram_region, ttm);
>  }
>  
>  static void try_add_system(struct xe_device *xe, struct xe_bo *bo,
> @@ -177,12 +180,15 @@ static void add_vram(struct xe_device *xe, struct xe_bo *bo,
>  		     struct ttm_place *places, u32 bo_flags, u32 mem_type, u32 *c)
>  {
>  	struct ttm_place place = { .mem_type = mem_type };
> +	struct ttm_resource_manager *mgr = ttm_manager_type(&xe->ttm, mem_type);
> +	struct xe_ttm_vram_mgr *vram_mgr = to_xe_ttm_vram_mgr(mgr);
> +
>  	struct xe_vram_region *vram;
>  	u64 io_size;
>  
>  	xe_assert(xe, *c < ARRAY_SIZE(bo->placements));
>  
> -	vram = to_xe_ttm_vram_mgr(ttm_manager_type(&xe->ttm, mem_type))->vram;
> +	vram = container_of(vram_mgr, struct xe_vram_region, ttm);
>  	xe_assert(xe, vram && vram->usable_size);
>  	io_size = vram->io_size;
>  
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index c7c285c6a6c9..6d44de9a31f6 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -23,6 +23,7 @@
>  #include "xe_sriov_types.h"
>  #include "xe_step_types.h"
>  #include "xe_survivability_mode_types.h"
> +#include "xe_ttm_vram_mgr_types.h"
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
>  #define TEST_VM_OPS_ERROR
> @@ -104,6 +105,8 @@ struct xe_vram_region {
>  	resource_size_t actual_physical_size;
>  	/** @mapping: pointer to VRAM mappable space */
>  	void __iomem *mapping;
> +	/** @vram_mgr: VRAM TTM manager */
> +	struct xe_ttm_vram_mgr ttm;
>  };
>  
>  /**
> @@ -198,9 +201,6 @@ struct xe_tile {
>  		 */
>  		struct xe_vram_region vram;
>  
> -		/** @mem.vram_mgr: VRAM TTM manager */
> -		struct xe_ttm_vram_mgr *vram_mgr;
> -
>  		/** @mem.ggtt: Global graphics translation table */
>  		struct xe_ggtt *ggtt;
>  
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index b1d994d65589..af788c147ca8 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -1560,7 +1560,7 @@ static u64 pf_query_free_lmem(struct xe_gt *gt)
>  {
>  	struct xe_tile *tile = gt->tile;
>  
> -	return xe_ttm_vram_get_avail(&tile->mem.vram_mgr->manager);
> +	return xe_ttm_vram_get_avail(&tile->mem.vram.ttm.manager);
>  }
>  
>  static u64 pf_query_max_lmem(struct xe_gt *gt)
> diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
> index 2825553b568f..d9a7a04ff652 100644
> --- a/drivers/gpu/drm/xe/xe_tile.c
> +++ b/drivers/gpu/drm/xe/xe_tile.c
> @@ -94,10 +94,6 @@ static int xe_tile_alloc(struct xe_tile *tile)
>  		return -ENOMEM;
>  	tile->mem.ggtt->tile = tile;
>  
> -	tile->mem.vram_mgr = drmm_kzalloc(drm, sizeof(*tile->mem.vram_mgr), GFP_KERNEL);
> -	if (!tile->mem.vram_mgr)
> -		return -ENOMEM;
> -
>  	return 0;
>  }
>  
> @@ -139,7 +135,7 @@ static int tile_ttm_mgr_init(struct xe_tile *tile)
>  	int err;
>  
>  	if (tile->mem.vram.usable_size) {
> -		err = xe_ttm_vram_mgr_init(tile, tile->mem.vram_mgr);
> +		err = xe_ttm_vram_mgr_init(tile, &tile->mem.vram.ttm);
>  		if (err)
>  			return err;
>  		xe->info.mem_region_mask |= BIT(tile->id) << 1;
> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index a8c37fb4fec0..9e375a40aee9 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -342,7 +342,6 @@ int xe_ttm_vram_mgr_init(struct xe_tile *tile, struct xe_ttm_vram_mgr *mgr)
>  	struct xe_device *xe = tile_to_xe(tile);
>  	struct xe_vram_region *vram = &tile->mem.vram;
>  
> -	mgr->vram = vram;
>  	return __xe_ttm_vram_mgr_init(xe, mgr, XE_PL_VRAM0 + tile->id,
>  				      vram->usable_size, vram->io_size,
>  				      PAGE_SIZE);
> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h b/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
> index 4c52ced4ee44..1144f9232ebb 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
> @@ -9,8 +9,6 @@
>  #include <drm/drm_buddy.h>
>  #include <drm/ttm/ttm_device.h>
>  
> -struct xe_vram_region;
> -
>  /**
>   * struct xe_ttm_vram_mgr - XE TTM VRAM manager
>   *
> @@ -21,8 +19,6 @@ struct xe_ttm_vram_mgr {
>  	struct ttm_resource_manager manager;
>  	/** @mm: DRM buddy allocator which manages the VRAM */
>  	struct drm_buddy mm;
> -	/** @vram: ptr to details of associated VRAM region */
> -	struct xe_vram_region *vram;
>  	/** @visible_size: Proped size of the CPU visible portion */
>  	u64 visible_size;
>  	/** @visible_avail: CPU visible portion still unallocated */
> -- 
> 2.34.1
>