Re: [Intel-xe] [PATCH] drm/xe: fix pvc unload issue

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

On Tue, Apr 04, 2023 at 03:44:18AM +0000, Matthew Brost wrote:
> On Mon, Apr 03, 2023 at 10:20:31PM +0000, Chang, Bruce wrote:
> > Currently, unload pvc driver will generate a null dereference
> > and the call stack is as below.
> > 
> > [ 4850.618000] Call Trace:
> > [ 4850.620740]  <TASK>
> > [ 4850.623134]  ttm_bo_cleanup_memtype_use+0x3f/0x50 [ttm]
> > [ 4850.628661]  ttm_bo_release+0x154/0x2c0 [ttm]
> > [ 4850.633317]  ? drm_buddy_fini+0x62/0x80 [drm_buddy]
> > [ 4850.638487]  ? __kmem_cache_free+0x27d/0x2c0
> > [ 4850.643054]  ttm_bo_put+0x38/0x60 [ttm]
> > [ 4850.647190]  xe_gem_object_free+0x1f/0x30 [xe]
> > [ 4850.651945]  drm_gem_object_free+0x1e/0x30 [drm]
> > [ 4850.656904]  ggtt_fini_noalloc+0x9d/0xe0 [xe]
> > [ 4850.661574]  drm_managed_release+0xb5/0x150 [drm]
> > [ 4850.666617]  drm_dev_release+0x30/0x50 [drm]
> > [ 4850.671209]  devm_drm_dev_init_release+0x3c/0x60 [drm]
> > 
> > There are a couple issues, but the main one is due to TTM has only
> > one TTM_PL_TT region, but since pvc has 2 tiles and tries to setup
> > 1 TTM_PL_TT each tile. The second will overwrite the first one.
> > 
> > During unload time, the first tile will reset the TTM_PL_TT manger
> > and when the second tile is trying to free Bo and it will generate
> > the null reference since the TTM manage is already got reset to 0.
> > 
> > The fix is to use one global TTM_PL_TT manager.
> > 
> > v2: make gtt mgr global and change the name to sys_mgr
> > 
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Signed-off-by: Bruce Chang <yu.bruce.chang@intel.com>
> 
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>

pushed to drm-xe-next. Thanks for the patch and review.

> 
> > ---
> >  drivers/gpu/drm/xe/Makefile                   |  2 +-
> >  drivers/gpu/drm/xe/xe_device.c                |  2 +
> >  drivers/gpu/drm/xe/xe_device.h                |  1 +
> >  drivers/gpu/drm/xe/xe_device_types.h          |  2 +
> >  drivers/gpu/drm/xe/xe_gt.c                    | 18 -----
> >  drivers/gpu/drm/xe/xe_gt_types.h              |  2 -
> >  drivers/gpu/drm/xe/xe_ttm_gtt_mgr.h           | 16 -----
> >  drivers/gpu/drm/xe/xe_ttm_gtt_mgr_types.h     | 18 -----
> >  .../xe/{xe_ttm_gtt_mgr.c => xe_ttm_sys_mgr.c} | 67 +++++++------------
> >  9 files changed, 31 insertions(+), 97 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/xe/xe_ttm_gtt_mgr.h
> >  delete mode 100644 drivers/gpu/drm/xe/xe_ttm_gtt_mgr_types.h
> >  rename drivers/gpu/drm/xe/{xe_ttm_gtt_mgr.c => xe_ttm_sys_mgr.c} (52%)
> > 
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 669c7b6f552c..7e19a15af3dc 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -83,7 +83,7 @@ xe-y += xe_bb.o \
> >  	xe_step.o \
> >  	xe_sync.o \
> >  	xe_trace.o \
> > -	xe_ttm_gtt_mgr.o \
> > +	xe_ttm_sys_mgr.o \
> >  	xe_ttm_stolen_mgr.o \
> >  	xe_ttm_vram_mgr.o \
> >  	xe_tuning.o \
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index a79f934e3d2d..4fb9aff27686 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -279,6 +279,8 @@ int xe_device_probe(struct xe_device *xe)
> >  	if (err)
> >  		goto err_irq_shutdown;
> >  
> > +	xe_ttm_sys_mgr_init(xe);
> > +
> >  	for_each_gt(gt, xe, id) {
> >  		err = xe_gt_init_noalloc(gt);
> >  		if (err)
> > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> > index d277f8985f7b..d9d1b09a8e38 100644
> > --- a/drivers/gpu/drm/xe/xe_device.h
> > +++ b/drivers/gpu/drm/xe/xe_device.h
> > @@ -116,4 +116,5 @@ static inline bool xe_device_has_flat_ccs(struct xe_device *xe)
> >  }
> >  
> >  u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size);
> > +int xe_ttm_sys_mgr_init(struct xe_device *xe);
> >  #endif
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 88f863edc41c..6fb53a54913f 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -195,6 +195,8 @@ struct xe_device {
> >  			/** @mapping: pointer to VRAM mappable space */
> >  			void *__iomem mapping;
> >  		} vram;
> > +		/** @sys_mgr: system TTM manager */
> > +		struct ttm_resource_manager sys_mgr;
> >  	} mem;
> >  
> >  	/** @usm: unified memory state */
> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > index fd7a5b43ba3e..a6ea138a7877 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.c
> > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > @@ -36,7 +36,6 @@
> >  #include "xe_ring_ops.h"
> >  #include "xe_sa.h"
> >  #include "xe_sched_job.h"
> > -#include "xe_ttm_gtt_mgr.h"
> >  #include "xe_ttm_vram_mgr.h"
> >  #include "xe_tuning.h"
> >  #include "xe_uc.h"
> > @@ -77,16 +76,11 @@ int xe_gt_alloc(struct xe_device *xe, struct xe_gt *gt)
> >  		if (!gt->mem.vram_mgr)
> >  			return -ENOMEM;
> >  
> > -		gt->mem.gtt_mgr = drmm_kzalloc(drm, sizeof(*gt->mem.gtt_mgr),
> > -					       GFP_KERNEL);
> > -		if (!gt->mem.gtt_mgr)
> > -			return -ENOMEM;
> >  	} else {
> >  		struct xe_gt *full_gt = xe_find_full_gt(gt);
> >  
> >  		gt->mem.ggtt = full_gt->mem.ggtt;
> >  		gt->mem.vram_mgr = full_gt->mem.vram_mgr;
> > -		gt->mem.gtt_mgr = full_gt->mem.gtt_mgr;
> >  	}
> >  
> >  	gt->ordered_wq = alloc_ordered_workqueue("gt-ordered-wq", 0);
> > @@ -98,26 +92,14 @@ static int gt_ttm_mgr_init(struct xe_gt *gt)
> >  {
> >  	struct xe_device *xe = gt_to_xe(gt);
> >  	int err;
> > -	struct sysinfo si;
> > -	u64 gtt_size;
> > -
> > -	si_meminfo(&si);
> > -	gtt_size = (u64)si.totalram * si.mem_unit * 3/4;
> >  
> >  	if (gt->mem.vram.size) {
> >  		err = xe_ttm_vram_mgr_init(gt, gt->mem.vram_mgr);
> >  		if (err)
> >  			return err;
> > -		gtt_size = min(max((XE_DEFAULT_GTT_SIZE_MB << 20),
> > -				   (u64)gt->mem.vram.size),
> > -			       gtt_size);
> >  		xe->info.mem_region_mask |= BIT(gt->info.vram_id) << 1;
> >  	}
> >  
> > -	err = xe_ttm_gtt_mgr_init(gt, gt->mem.gtt_mgr, gtt_size);
> > -	if (err)
> > -		return err;
> > -
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> > index 8f29aba455e0..9d3117fad2e4 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_types.h
> > +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> > @@ -162,8 +162,6 @@ struct xe_gt {
> >  		} vram;
> >  		/** @vram_mgr: VRAM TTM manager */
> >  		struct xe_ttm_vram_mgr *vram_mgr;
> > -		/** @gtt_mr: GTT TTM manager */
> > -		struct xe_ttm_gtt_mgr *gtt_mgr;
> >  		/** @ggtt: Global graphics translation table */
> >  		struct xe_ggtt *ggtt;
> >  	} mem;
> > diff --git a/drivers/gpu/drm/xe/xe_ttm_gtt_mgr.h b/drivers/gpu/drm/xe/xe_ttm_gtt_mgr.h
> > deleted file mode 100644
> > index d1d57cb9c2b8..000000000000
> > --- a/drivers/gpu/drm/xe/xe_ttm_gtt_mgr.h
> > +++ /dev/null
> > @@ -1,16 +0,0 @@
> > -/* SPDX-License-Identifier: MIT */
> > -/*
> > - * Copyright © 2022 Intel Corporation
> > - */
> > -
> > -#ifndef _XE_TTGM_GTT_MGR_H_
> > -#define _XE_TTGM_GTT_MGR_H_
> > -
> > -#include "xe_ttm_gtt_mgr_types.h"
> > -
> > -struct xe_gt;
> > -
> > -int xe_ttm_gtt_mgr_init(struct xe_gt *gt, struct xe_ttm_gtt_mgr *mgr,
> > -			u64 gtt_size);
> > -
> > -#endif
> > diff --git a/drivers/gpu/drm/xe/xe_ttm_gtt_mgr_types.h b/drivers/gpu/drm/xe/xe_ttm_gtt_mgr_types.h
> > deleted file mode 100644
> > index c66737488326..000000000000
> > --- a/drivers/gpu/drm/xe/xe_ttm_gtt_mgr_types.h
> > +++ /dev/null
> > @@ -1,18 +0,0 @@
> > -/* SPDX-License-Identifier: MIT */
> > -/*
> > - * Copyright © 2022 Intel Corporation
> > - */
> > -
> > -#ifndef _XE_TTM_GTT_MGR_TYPES_H_
> > -#define _XE_TTM_GTT_MGR_TYPES_H_
> > -
> > -#include <drm/ttm/ttm_device.h>
> > -
> > -struct xe_gt;
> > -
> > -struct xe_ttm_gtt_mgr {
> > -	struct xe_gt *gt;
> > -	struct ttm_resource_manager manager;
> > -};
> > -
> > -#endif
> > diff --git a/drivers/gpu/drm/xe/xe_ttm_gtt_mgr.c b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
> > similarity index 52%
> > rename from drivers/gpu/drm/xe/xe_ttm_gtt_mgr.c
> > rename to drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
> > index 8075781070f2..cf5f4f73d4dc 100644
> > --- a/drivers/gpu/drm/xe/xe_ttm_gtt_mgr.c
> > +++ b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
> > @@ -12,31 +12,24 @@
> >  
> >  #include "xe_bo.h"
> >  #include "xe_gt.h"
> > -#include "xe_ttm_gtt_mgr.h"
> >  
> > -struct xe_ttm_gtt_node {
> > +struct xe_ttm_sys_node {
> >  	struct ttm_buffer_object *tbo;
> >  	struct ttm_range_mgr_node base;
> >  };
> >  
> > -static inline struct xe_ttm_gtt_mgr *
> > -to_gtt_mgr(struct ttm_resource_manager *man)
> > +static inline struct xe_ttm_sys_node *
> > +to_xe_ttm_sys_node(struct ttm_resource *res)
> >  {
> > -	return container_of(man, struct xe_ttm_gtt_mgr, manager);
> > +	return container_of(res, struct xe_ttm_sys_node, base.base);
> >  }
> >  
> > -static inline struct xe_ttm_gtt_node *
> > -to_xe_ttm_gtt_node(struct ttm_resource *res)
> > -{
> > -	return container_of(res, struct xe_ttm_gtt_node, base.base);
> > -}
> > -
> > -static int xe_ttm_gtt_mgr_new(struct ttm_resource_manager *man,
> > +static int xe_ttm_sys_mgr_new(struct ttm_resource_manager *man,
> >  			      struct ttm_buffer_object *tbo,
> >  			      const struct ttm_place *place,
> >  			      struct ttm_resource **res)
> >  {
> > -	struct xe_ttm_gtt_node *node;
> > +	struct xe_ttm_sys_node *node;
> >  	int r;
> >  
> >  	node = kzalloc(struct_size(node, base.mm_nodes, 1), GFP_KERNEL);
> > @@ -66,32 +59,31 @@ static int xe_ttm_gtt_mgr_new(struct ttm_resource_manager *man,
> >  	return r;
> >  }
> >  
> > -static void xe_ttm_gtt_mgr_del(struct ttm_resource_manager *man,
> > +static void xe_ttm_sys_mgr_del(struct ttm_resource_manager *man,
> >  			       struct ttm_resource *res)
> >  {
> > -	struct xe_ttm_gtt_node *node = to_xe_ttm_gtt_node(res);
> > +	struct xe_ttm_sys_node *node = to_xe_ttm_sys_node(res);
> >  
> >  	ttm_resource_fini(man, res);
> >  	kfree(node);
> >  }
> >  
> > -static void xe_ttm_gtt_mgr_debug(struct ttm_resource_manager *man,
> > +static void xe_ttm_sys_mgr_debug(struct ttm_resource_manager *man,
> >  				 struct drm_printer *printer)
> >  {
> >  
> >  }
> >  
> > -static const struct ttm_resource_manager_func xe_ttm_gtt_mgr_func = {
> > -	.alloc = xe_ttm_gtt_mgr_new,
> > -	.free = xe_ttm_gtt_mgr_del,
> > -	.debug = xe_ttm_gtt_mgr_debug
> > +static const struct ttm_resource_manager_func xe_ttm_sys_mgr_func = {
> > +	.alloc = xe_ttm_sys_mgr_new,
> > +	.free = xe_ttm_sys_mgr_del,
> > +	.debug = xe_ttm_sys_mgr_debug
> >  };
> >  
> > -static void ttm_gtt_mgr_fini(struct drm_device *drm, void *arg)
> > +static void ttm_sys_mgr_fini(struct drm_device *drm, void *arg)
> >  {
> > -	struct xe_ttm_gtt_mgr *mgr = arg;
> > -	struct xe_device *xe = gt_to_xe(mgr->gt);
> > -	struct ttm_resource_manager *man = &mgr->manager;
> > +	struct xe_device *xe = (struct xe_device *)arg;
> > +	struct ttm_resource_manager *man = &xe->mem.sys_mgr;
> >  	int err;
> >  
> >  	ttm_resource_manager_set_used(man, false);
> > @@ -104,27 +96,18 @@ static void ttm_gtt_mgr_fini(struct drm_device *drm, void *arg)
> >  	ttm_set_driver_manager(&xe->ttm, XE_PL_TT, NULL);
> >  }
> >  
> > -int xe_ttm_gtt_mgr_init(struct xe_gt *gt, struct xe_ttm_gtt_mgr *mgr,
> > -			u64 gtt_size)
> > +int xe_ttm_sys_mgr_init(struct xe_device *xe)
> >  {
> > -	struct xe_device *xe = gt_to_xe(gt);
> > -	struct ttm_resource_manager *man = &mgr->manager;
> > -	int err;
> > -
> > -	XE_BUG_ON(xe_gt_is_media_type(gt));
> > +	struct ttm_resource_manager *man = &xe->mem.sys_mgr;
> > +	struct sysinfo si;
> > +	u64 gtt_size;
> >  
> > -	mgr->gt = gt;
> > +	si_meminfo(&si);
> > +	gtt_size = (u64)si.totalram * si.mem_unit * 3/4;
> >  	man->use_tt = true;
> > -	man->func = &xe_ttm_gtt_mgr_func;
> > -
> > +	man->func = &xe_ttm_sys_mgr_func;
> >  	ttm_resource_manager_init(man, &xe->ttm, gtt_size >> PAGE_SHIFT);
> > -
> > -	ttm_set_driver_manager(&xe->ttm, XE_PL_TT, &mgr->manager);
> > +	ttm_set_driver_manager(&xe->ttm, XE_PL_TT, man);
> >  	ttm_resource_manager_set_used(man, true);
> > -
> > -	err = drmm_add_action_or_reset(&xe->drm, ttm_gtt_mgr_fini, mgr);
> > -	if (err)
> > -		return err;
> > -
> > -	return 0;
> > +	return drmm_add_action_or_reset(&xe->drm, ttm_sys_mgr_fini, xe);
> >  }
> > -- 
> > 2.25.1
> >