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

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

On Fri, Jan 31, 2025 at 11:09:13AM +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.
> 
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device_types.h       | 5 ++---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 2 +-
>  drivers/gpu/drm/xe/xe_tile.c               | 6 +++---
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index c7c285c6a6c9..a4d998add4c3 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -104,6 +104,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 *vram_mgr;
>  };
>  
>  /**
> @@ -198,9 +200,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..98e443faabdc 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.vram_mgr->manager);

I like your series, but I didn't like how this ended here
vram vram
manager manager

Then I went to see if there was something we could do about the
xe_ttm_vram_mgr itself, but then I noticed it has pointer to
xe_vram_region inside it and I disliked this double pointer now.

So, if we really want to take this path we probably need a bigger
refactor, or this can bring more confusion then help.

>  }
>  
>  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..b13a0ec7c296 100644
> --- a/drivers/gpu/drm/xe/xe_tile.c
> +++ b/drivers/gpu/drm/xe/xe_tile.c
> @@ -94,8 +94,8 @@ 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)
> +	tile->mem.vram.vram_mgr = drmm_kzalloc(drm, sizeof(*tile->mem.vram.vram_mgr), GFP_KERNEL);
> +	if (!tile->mem.vram.vram_mgr)
>  		return -ENOMEM;
>  
>  	return 0;
> @@ -139,7 +139,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.vram_mgr);
>  		if (err)
>  			return err;
>  		xe->info.mem_region_mask |= BIT(tile->id) << 1;
> -- 
> 2.34.1
>