[Intel-xe] [PATCH] drm/xe: fix pvc unload issue
Matthew Brost
matthew.brost at intel.com
Tue Apr 4 03:44:18 UTC 2023
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 at intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Signed-off-by: Bruce Chang <yu.bruce.chang at intel.com>
Reviewed-by: Matthew Brost <matthew.brost at intel.com>
> ---
> 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
>
More information about the Intel-xe
mailing list