[PATCH 4/4] drm/xe: Introduce the wedged_mode debugfs
Rodrigo Vivi
rodrigo.vivi at intel.com
Wed Apr 17 20:29:26 UTC 2024
On Wed, Apr 17, 2024 at 02:51:05PM -0500, Lucas De Marchi wrote:
> On Tue, Apr 09, 2024 at 06:15:07PM GMT, Rodrigo Vivi wrote:
> > So, the wedged mode can be selected per device at runtime,
> > before the tests or before reproducing the issue.
> >
> > v2: - s/busted/wedged
> > - some locking consistency
> >
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_debugfs.c | 56 ++++++++++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_device.c | 41 ++++++++++++++------
> > drivers/gpu/drm/xe/xe_device.h | 4 +-
> > drivers/gpu/drm/xe/xe_device_types.h | 11 +++++-
> > drivers/gpu/drm/xe/xe_gt.c | 2 +-
> > drivers/gpu/drm/xe/xe_guc.c | 2 +-
> > drivers/gpu/drm/xe/xe_guc_ads.c | 52 +++++++++++++++++++++++++-
> > drivers/gpu/drm/xe/xe_guc_ads.h | 1 +
> > drivers/gpu/drm/xe/xe_guc_submit.c | 28 +++++++-------
> > 9 files changed, 163 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> > index 86150cafe0ff..6ff067ea5a8f 100644
> > --- a/drivers/gpu/drm/xe/xe_debugfs.c
> > +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> > @@ -12,6 +12,7 @@
> > #include "xe_bo.h"
> > #include "xe_device.h"
> > #include "xe_gt_debugfs.h"
> > +#include "xe_guc_ads.h"
> > #include "xe_pm.h"
> > #include "xe_step.h"
> >
> > @@ -106,6 +107,58 @@ static const struct file_operations forcewake_all_fops = {
> > .release = forcewake_release,
> > };
> >
> > +static ssize_t wedged_mode_show(struct file *f, char __user *ubuf,
> > + size_t size, loff_t *pos)
> > +{
> > + struct xe_device *xe = file_inode(f)->i_private;
> > + char buf[32];
> > + int len = 0;
> > +
> > + mutex_lock(&xe->wedged.lock);
>
> what are we trying to synchronize here? multiple threads writing to
> debugfs to change the wedged mode?
well, since it is a debugfs set manually (or in the igt we control) and
that should be set in advance of the exec/hang test, I believe we could
live without any lock indeed.
>
> > + len = scnprintf(buf, sizeof(buf), "%d\n", xe->wedged.mode);
> > + mutex_unlock(&xe->wedged.lock);
> > +
> > + return simple_read_from_buffer(ubuf, size, pos, buf, len);
> > +}
> > +
> > +static ssize_t wedged_mode_set(struct file *f, const char __user *ubuf,
> > + size_t size, loff_t *pos)
> > +{
> > + struct xe_device *xe = file_inode(f)->i_private;
> > + struct xe_gt *gt;
> > + u32 wedged_mode;
> > + ssize_t ret;
> > + u8 id;
> > +
> > + ret = kstrtouint_from_user(ubuf, size, 0, &wedged_mode);
> > + if (ret)
> > + return ret;
> > +
> > + if (wedged_mode > 2)
> > + return -EINVAL;
> > +
> > + mutex_lock(&xe->wedged.lock);
> > + xe->wedged.mode = wedged_mode;
>
> should we return if xe->wedged.mode == wedged_mode already?
indeed. will change.
>
> > + if (wedged_mode == 2) {
>
> this doesn't seem correct.
>
> echo 2 > wedged_mode
> echo 1 > wedged_mode
>
> Second call doesn't fail, driver starts handling it as wedged_mode == 1,
> guc still has the policy to disable reset. Or am I missing anything?
>
> If we are not handling going back to 1, then we should just error
> out above.
indeed. I forgot about that case...
will change.
>
> > + for_each_gt(gt, xe, id) {
> > + ret = xe_guc_ads_scheduler_policy_disable_reset(>->uc.guc.ads);
> > + if (ret) {
> > + drm_err(&xe->drm, "Failed to update GuC ADS scheduler policy. GPU might still reset even on the wedged_mode=2\n");
>
> humn... wouldn't it be better to keep xe->wedged.mode as is and error
> out like:
>
> drm_err(&xe->drm, "Failed to update GuC ADS scheduler policy, can't change wedged_mode\n");
>
> also, we should return error here. dunno which one is appropriate, but
> maybe -ECOMM. Also, maybe unwind the first GTs on error? May not be
> needed and just make matters worse,
yeap, I didn't want to rewind exactly because of the multigt.
what happens if it sets gt0, then fails in gt1, then fails in rewind of gt0?
> but may deserve a comment about that
> conscious decision here.
exactly what I tried to do with:
"Failed to update GuC ADS scheduler policy. GPU might still reset even on the wedged_mode=2\n"
:)
you respect the decision on stopping and wedging on any timeout, but warning
that guc might not respect that selection.
should I put this above sentence as a comment in the code?
>
> Lucas De Marchi
>
> > + break;
> > + }
> > + }
> > + }
> > + mutex_unlock(&xe->wedged.lock);
> > +
> > + return size;
> > +}
> > +
> > +static const struct file_operations wedged_mode_fops = {
> > + .owner = THIS_MODULE,
> > + .read = wedged_mode_show,
> > + .write = wedged_mode_set,
> > +};
> > +
> > void xe_debugfs_register(struct xe_device *xe)
> > {
> > struct ttm_device *bdev = &xe->ttm;
> > @@ -123,6 +176,9 @@ void xe_debugfs_register(struct xe_device *xe)
> > debugfs_create_file("forcewake_all", 0400, root, xe,
> > &forcewake_all_fops);
> >
> > + debugfs_create_file("wedged_mode", 0400, root, xe,
> > + &wedged_mode_fops);
> > +
> > for (mem_type = XE_PL_VRAM0; mem_type <= XE_PL_VRAM1; ++mem_type) {
> > man = ttm_manager_type(bdev, mem_type);
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 7928a5470cee..949fca2f0400 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -445,6 +445,9 @@ int xe_device_probe_early(struct xe_device *xe)
> > if (err)
> > return err;
> >
> > + mutex_init(&xe->wedged.lock);
> > + xe->wedged.mode = xe_modparam.wedged_mode;
> > +
> > return 0;
> > }
> >
> > @@ -787,26 +790,37 @@ u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address)
> > }
> >
> > /**
> > - * xe_device_declare_wedged - Declare device wedged
> > + * xe_device_hint_wedged - Get a hint and possibly declare device as wedged
> > * @xe: xe device instance
> > + * @in_timeout_path: hint coming from a timeout path
> > *
> > - * This is a final state that can only be cleared with a module
> > + * The wedged state is a final on that can only be cleared with a module
> > * re-probe (unbind + bind).
> > * In this state every IOCTL will be blocked so the GT cannot be used.
> > - * In general it will be called upon any critical error such as gt reset
> > - * failure or guc loading failure.
> > - * If xe.wedged module parameter is set to 2, this function will be called
> > - * on every single execution timeout (a.k.a. GPU hang) right after devcoredump
> > - * snapshot capture. In this mode, GT reset won't be attempted so the state of
> > - * the issue is preserved for further debugging.
> > + * In general device will be declared wedged only at critical
> > + * error paths such as gt reset failure or guc loading failure.
> > + * Hints are also expected from every single execution timeout (a.k.a. GPU hang)
> > + * right after devcoredump snapshot capture. Then, device can be declared wedged
> > + * if wedged_mode is set to 2. In this mode, GT reset won't be attempted so the
> > + * state of the issue is preserved for further debugging.
> > + *
> > + * Return: True if device has been just declared wedged. False otherwise.
> > */
> > -void xe_device_declare_wedged(struct xe_device *xe)
> > +bool xe_device_hint_wedged(struct xe_device *xe, bool in_timeout_path)
> > {
> > - if (xe_modparam.wedged_mode == 0)
> > - return;
> > + bool ret = false;
> > +
> > + mutex_lock(&xe->wedged.lock);
> >
> > - if (!atomic_xchg(&xe->wedged, 1)) {
> > + if (xe->wedged.mode == 0)
> > + goto out;
> > +
> > + if (in_timeout_path && xe->wedged.mode != 2)
> > + goto out;
> > +
> > + if (!atomic_xchg(&xe->wedged.flag, 1)) {
> > xe->needs_flr_on_fini = true;
> > + ret = true;
> > drm_err(&xe->drm,
> > "CRITICAL: Xe has declared device %s as wedged.\n"
> > "IOCTLs and executions are blocked until device is probed again with unbind and bind operations:\n"
> > @@ -816,4 +830,7 @@ void xe_device_declare_wedged(struct xe_device *xe)
> > dev_name(xe->drm.dev), dev_name(xe->drm.dev),
> > dev_name(xe->drm.dev));
> > }
> > +out:
> > + mutex_unlock(&xe->wedged.lock);
> > + return ret;
> > }
> > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> > index 0fea5c18f76d..e3ea8a43e7f9 100644
> > --- a/drivers/gpu/drm/xe/xe_device.h
> > +++ b/drivers/gpu/drm/xe/xe_device.h
> > @@ -178,9 +178,9 @@ u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address);
> >
> > static inline bool xe_device_wedged(struct xe_device *xe)
> > {
> > - return atomic_read(&xe->wedged);
> > + return atomic_read(&xe->wedged.flag);
> > }
> >
> > -void xe_device_declare_wedged(struct xe_device *xe);
> > +bool xe_device_hint_wedged(struct xe_device *xe, bool in_timeout_path);
> >
> > #endif
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index b9ef60f21750..0da4787f1087 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -458,8 +458,15 @@ struct xe_device {
> > /** @needs_flr_on_fini: requests function-reset on fini */
> > bool needs_flr_on_fini;
> >
> > - /** @wedged: Xe device faced a critical error and is now blocked. */
> > - atomic_t wedged;
> > + /** @wedged: Struct to control Wedged States and mode */
> > + struct {
> > + /** @wedged.flag: Xe device faced a critical error and is now blocked. */
> > + atomic_t flag;
> > + /** @wedged.mode: Mode controlled by kernel parameter and debugfs */
> > + int mode;
> > + /** @wedged.lock: To protect @wedged.mode */
> > + struct mutex lock;
> > + } wedged;
> >
> > /* private: */
> >
> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > index 0844081b88ef..da16f4273877 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.c
> > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > @@ -688,7 +688,7 @@ static int gt_reset(struct xe_gt *gt)
> > err_fail:
> > xe_gt_err(gt, "reset failed (%pe)\n", ERR_PTR(err));
> >
> > - xe_device_declare_wedged(gt_to_xe(gt));
> > + xe_device_hint_wedged(gt_to_xe(gt), false);
> >
> > return err;
> > }
> > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> > index f1c3e338301d..ee7e0fa4815d 100644
> > --- a/drivers/gpu/drm/xe/xe_guc.c
> > +++ b/drivers/gpu/drm/xe/xe_guc.c
> > @@ -495,7 +495,7 @@ static void guc_wait_ucode(struct xe_guc *guc)
> > xe_gt_err(gt, "GuC firmware exception. EIP: %#x\n",
> > xe_mmio_read32(gt, SOFT_SCRATCH(13)));
> >
> > - xe_device_declare_wedged(gt_to_xe(gt));
> > + xe_device_hint_wedged(gt_to_xe(gt), false);
> > } else {
> > xe_gt_dbg(gt, "GuC successfully loaded\n");
> > }
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> > index dbd88ae20aa3..ad64d5a31239 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> > @@ -9,6 +9,7 @@
> >
> > #include <generated/xe_wa_oob.h>
> >
> > +#include "abi/guc_actions_abi.h"
> > #include "regs/xe_engine_regs.h"
> > #include "regs/xe_gt_regs.h"
> > #include "regs/xe_guc_regs.h"
> > @@ -16,11 +17,11 @@
> > #include "xe_gt.h"
> > #include "xe_gt_ccs_mode.h"
> > #include "xe_guc.h"
> > +#include "xe_guc_ct.h"
> > #include "xe_hw_engine.h"
> > #include "xe_lrc.h"
> > #include "xe_map.h"
> > #include "xe_mmio.h"
> > -#include "xe_module.h"
> > #include "xe_platform_types.h"
> > #include "xe_wa.h"
> >
> > @@ -395,6 +396,7 @@ int xe_guc_ads_init_post_hwconfig(struct xe_guc_ads *ads)
> >
> > static void guc_policies_init(struct xe_guc_ads *ads)
> > {
> > + struct xe_device *xe = ads_to_xe(ads);
> > u32 global_flags = 0;
> >
> > ads_blob_write(ads, policies.dpc_promote_time,
> > @@ -402,8 +404,10 @@ static void guc_policies_init(struct xe_guc_ads *ads)
> > ads_blob_write(ads, policies.max_num_work_items,
> > GLOBAL_POLICY_MAX_NUM_WI);
> >
> > - if (xe_modparam.wedged_mode == 2)
> > + mutex_lock(&xe->wedged.lock);
> > + if (xe->wedged.mode == 2)
> > global_flags |= GLOBAL_POLICY_DISABLE_ENGINE_RESET;
> > + mutex_unlock(&xe->wedged.lock);
> >
> > ads_blob_write(ads, policies.global_flags, global_flags);
> > ads_blob_write(ads, policies.is_valid, 1);
> > @@ -760,3 +764,47 @@ void xe_guc_ads_populate_post_load(struct xe_guc_ads *ads)
> > {
> > guc_populate_golden_lrc(ads);
> > }
> > +
> > +static int guc_ads_action_update_policies(struct xe_guc_ads *ads, u32 policy_offset)
> > +{
> > + struct xe_guc_ct *ct = &ads_to_guc(ads)->ct;
> > + u32 action[] = {
> > + XE_GUC_ACTION_GLOBAL_SCHED_POLICY_CHANGE,
> > + policy_offset
> > + };
> > +
> > + return xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0);
> > +}
> > +
> > +int xe_guc_ads_scheduler_policy_disable_reset(struct xe_guc_ads *ads)
> > +{
> > + struct xe_device *xe = ads_to_xe(ads);
> > + struct xe_gt *gt = ads_to_gt(ads);
> > + struct xe_tile *tile = gt_to_tile(gt);
> > + struct guc_policies *policies;
> > + struct xe_bo *bo;
> > + int ret = 0;
> > +
> > + policies = kmalloc(sizeof(*policies), GFP_KERNEL);
> > + if (!policies)
> > + return -ENOMEM;
> > +
> > + policies->dpc_promote_time = ads_blob_read(ads, policies.dpc_promote_time);
> > + policies->max_num_work_items = ads_blob_read(ads, policies.max_num_work_items);
> > + policies->is_valid = 1;
> > + if (xe->wedged.mode == 2)
> > + policies->global_flags |= GLOBAL_POLICY_DISABLE_ENGINE_RESET;
> > +
> > + bo = xe_managed_bo_create_from_data(xe, tile, policies, sizeof(struct guc_policies),
> > + XE_BO_FLAG_VRAM_IF_DGFX(tile) |
> > + XE_BO_FLAG_GGTT);
> > + if (IS_ERR(bo)) {
> > + ret = PTR_ERR(bo);
> > + goto out;
> > + }
> > +
> > + ret = guc_ads_action_update_policies(ads, xe_bo_ggtt_addr(bo));
> > +out:
> > + kfree(policies);
> > + return ret;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.h b/drivers/gpu/drm/xe/xe_guc_ads.h
> > index 138ef6267671..7c45c40fab34 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ads.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_ads.h
> > @@ -13,5 +13,6 @@ int xe_guc_ads_init_post_hwconfig(struct xe_guc_ads *ads);
> > void xe_guc_ads_populate(struct xe_guc_ads *ads);
> > void xe_guc_ads_populate_minimal(struct xe_guc_ads *ads);
> > void xe_guc_ads_populate_post_load(struct xe_guc_ads *ads);
> > +int xe_guc_ads_scheduler_policy_disable_reset(struct xe_guc_ads *ads);
> >
> > #endif
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 0bea17536659..7de97b90ad00 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -35,7 +35,6 @@
> > #include "xe_macros.h"
> > #include "xe_map.h"
> > #include "xe_mocs.h"
> > -#include "xe_module.h"
> > #include "xe_ring_ops_types.h"
> > #include "xe_sched_job.h"
> > #include "xe_trace.h"
> > @@ -868,26 +867,33 @@ static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue *q)
> > xe_sched_tdr_queue_imm(&q->guc->sched);
> > }
> >
> > -static void guc_submit_wedged(struct xe_guc *guc)
> > +static bool guc_submit_hint_wedged(struct xe_guc *guc)
> > {
> > struct xe_exec_queue *q;
> > unsigned long index;
> > int err;
> >
> > - xe_device_declare_wedged(guc_to_xe(guc));
> > + if (xe_device_wedged(guc_to_xe(guc)))
> > + return true;
> > +
> > + if (!xe_device_hint_wedged(guc_to_xe(guc), true))
> > + return false;
> > +
> > xe_guc_submit_reset_prepare(guc);
> > xe_guc_ct_stop(&guc->ct);
> >
> > err = drmm_add_action_or_reset(&guc_to_xe(guc)->drm,
> > guc_submit_wedged_fini, guc);
> > if (err)
> > - return;
> > + return true; /* Device is wedged anyway */
> >
> > mutex_lock(&guc->submission_state.lock);
> > xa_for_each(&guc->submission_state.exec_queue_lookup, index, q)
> > if (xe_exec_queue_get_unless_zero(q))
> > set_exec_queue_wedged(q);
> > mutex_unlock(&guc->submission_state.lock);
> > +
> > + return true;
> > }
> >
> > static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > @@ -898,15 +904,12 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > struct xe_guc *guc = exec_queue_to_guc(q);
> > struct xe_device *xe = guc_to_xe(guc);
> > struct xe_gpu_scheduler *sched = &ge->sched;
> > - bool wedged = xe_device_wedged(xe);
> > + bool wedged;
> >
> > xe_assert(xe, xe_exec_queue_is_lr(q));
> > trace_xe_exec_queue_lr_cleanup(q);
> >
> > - if (!wedged && xe_modparam.wedged_mode == 2) {
> > - guc_submit_wedged(exec_queue_to_guc(q));
> > - wedged = true;
> > - }
> > + wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
> >
> > /* Kill the run_job / process_msg entry points */
> > xe_sched_submission_stop(sched);
> > @@ -957,7 +960,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > struct xe_device *xe = guc_to_xe(exec_queue_to_guc(q));
> > int err = -ETIME;
> > int i = 0;
> > - bool wedged = xe_device_wedged(xe);
> > + bool wedged;
> >
> > /*
> > * TDR has fired before free job worker. Common if exec queue
> > @@ -981,10 +984,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> >
> > trace_xe_sched_job_timedout(job);
> >
> > - if (!wedged && xe_modparam.wedged_mode == 2) {
> > - guc_submit_wedged(exec_queue_to_guc(q));
> > - wedged = true;
> > - }
> > + wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
> >
> > /* Kill the run_job entry point */
> > xe_sched_submission_stop(sched);
> > --
> > 2.44.0
> >
More information about the Intel-xe
mailing list