Re: [PATCH 1/2] drm/xe/tile: Set default memory placement for BOs

[Nirmoy Das <nirmoy.das@linux.intel.com>]

Hi Lucas,

On 4/2/2024 11:54 PM, Lucas De Marchi wrote:
> During initialization, record what should be the default placement for
> BOs, unless otherwise specified via flags when BO is created. This will
> allow to remove XE_BO_FLAG_VRAM_IF_DGFX() that is spread all throughout
> the code.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_bo.c           |  9 +++++++++
>   drivers/gpu/drm/xe/xe_bo.h           | 11 +++++------
>   drivers/gpu/drm/xe/xe_device_types.h |  9 +++++++++
>   drivers/gpu/drm/xe/xe_tile.c         |  6 ++++++
>   4 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 6166bc715656..71446688c643 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1368,6 +1368,15 @@ __xe_bo_create_locked(struct xe_device *xe,
>   	if (vm)
>   		xe_vm_assert_held(vm);
>   
> +	if (!(flags & XE_BO_FLAG_PL_MASK)) {
> +		struct xe_tile *tile_pl = tile;
> +
> +		if (!tile_pl)
> +			tile_pl = xe_device_get_root_tile(xe);
> +
> +		flags |= tile_pl->mem.default_pl_flag;
> +	}
> +
>   	if (start || end != ~0ULL) {
>   		bo = xe_bo_alloc();
>   		if (IS_ERR(bo))
> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> index bae042b35fa8..8781ee15c253 100644
> --- a/drivers/gpu/drm/xe/xe_bo.h
> +++ b/drivers/gpu/drm/xe/xe_bo.h
> @@ -23,17 +23,16 @@
>   
>   #define XE_DEFAULT_GTT_SIZE_MB          3072ULL /* 3GB by default */
>   
> -#define XE_BO_FLAG_USER		BIT(0)
> +#define XE_BO_FLAG_USER			BIT(0)
>   /* The bits below need to be contiguous, or things break */
>   #define XE_BO_FLAG_SYSTEM		BIT(1)
>   #define XE_BO_FLAG_VRAM0		BIT(2)
>   #define XE_BO_FLAG_VRAM1		BIT(3)
> -#define XE_BO_FLAG_VRAM_MASK		(XE_BO_FLAG_VRAM0 | XE_BO_FLAG_VRAM1)
> -/* -- */
>   #define XE_BO_FLAG_STOLEN		BIT(4)
> -#define XE_BO_FLAG_VRAM_IF_DGFX(tile)	(IS_DGFX(tile_to_xe(tile)) ? \
> -					 XE_BO_FLAG_VRAM0 << (tile)->id : \
> -					 XE_BO_FLAG_SYSTEM)

This should happen in the following patch.  Otherwise this looks good to 
me.

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

Regards,

Nirmoy


> +#define XE_BO_FLAG_VRAM_MASK		(XE_BO_FLAG_VRAM0 | XE_BO_FLAG_VRAM1)
> +#define XE_BO_FLAG_PL_MASK		(XE_BO_FLAG_VRAM_MASK | XE_BO_FLAG_SYSTEM | \
> +					 XE_BO_FLAG_STOLEN)
> +
>   #define XE_BO_FLAG_GGTT			BIT(5)
>   #define XE_BO_FLAG_IGNORE_MIN_PAGE_SIZE BIT(6)
>   #define XE_BO_FLAG_PINNED		BIT(7)
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 47c864f67348..cdcdf27a442a 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -173,6 +173,15 @@ struct xe_tile {
>   		 */
>   		struct xe_mem_region vram;
>   
> +		/**
> +		 * @mem.default_pl_flag: Default placement for BOs
> +		 *
> +		 * Default placement set as one of XE_BO_FLAG_{VRAM*,SYSTEM}.
> +		 * May still be overriden when creating a bo by setting a
> +		 * specific flag.
> +		 */
> +		u32 default_pl_flag;
> +
>   		/** @mem.vram_mgr: VRAM TTM manager */
>   		struct xe_ttm_vram_mgr *vram_mgr;
>   
> diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
> index 0650b2fa75ef..a55157f29c2c 100644
> --- a/drivers/gpu/drm/xe/xe_tile.c
> +++ b/drivers/gpu/drm/xe/xe_tile.c
> @@ -5,6 +5,7 @@
>   
>   #include <drm/drm_managed.h>
>   
> +#include "xe_bo.h"
>   #include "xe_device.h"
>   #include "xe_ggtt.h"
>   #include "xe_gt.h"
> @@ -162,6 +163,11 @@ int xe_tile_init_noalloc(struct xe_tile *tile)
>   
>   	xe_device_mem_access_get(tile_to_xe(tile));
>   
> +	if (IS_DGFX(tile->xe))
> +		tile->mem.default_pl_flag = XE_BO_FLAG_VRAM0 << tile->id;
> +	else
> +		tile->mem.default_pl_flag = XE_BO_FLAG_SYSTEM;
> +
>   	err = tile_ttm_mgr_init(tile);
>   	if (err)
>   		goto err_mem_access;