[PATCH v6 2/4] drm/xe: Don't update wedged mode in case of an error
Laguna, Lukasz
lukasz.laguna at intel.com
Wed Jul 30 13:08:46 UTC 2025
On 5/28/2025 11:17, Michal Wajdeczko wrote:
>
> On 21.05.2025 14:25, Lukasz Laguna wrote:
>> Update driver's internal wedged.mode state only in case of a success or
>> set it to "misconfigured" in case of inconsistent reset policy state
>> between GTs.
>>
>> Fixes: 6b8ef44cc0a9 ("drm/xe: Introduce the wedged_mode debugfs")
>> Signed-off-by: Lukasz Laguna <lukasz.laguna at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_debugfs.c | 34 ++++++++++++++++++++++++----
>> drivers/gpu/drm/xe/xe_device.c | 2 ++
>> drivers/gpu/drm/xe/xe_device_types.h | 1 +
>> drivers/gpu/drm/xe/xe_guc_ads.c | 12 +++++-----
>> drivers/gpu/drm/xe/xe_guc_ads.h | 4 +++-
>> 5 files changed, 41 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
>> index 5dd0ad3f9bae..5bc4fe5aa9a2 100644
>> --- a/drivers/gpu/drm/xe/xe_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
>> @@ -150,6 +150,25 @@ static ssize_t wedged_mode_show(struct file *f, char __user *ubuf,
>> return simple_read_from_buffer(ubuf, size, pos, buf, len);
>> }
>>
>> +static int wedged_mode_set_reset_policy(struct xe_gt *gt, enum xe_wedged_mode mode)
>> +{
>> + struct xe_device *xe = gt_to_xe(gt);
>> + int ret;
>> +
>> + if ((xe->wedged.mode == XE_WEDGED_MODE_NEVER &&
>> + mode == XE_WEDGED_MODE_UPON_CRITICAL_ERROR) ||
>> + (xe->wedged.mode == XE_WEDGED_MODE_UPON_CRITICAL_ERROR &&
>> + mode == XE_WEDGED_MODE_NEVER))
>> + return 0;
> this could be a separate helper:
>
> bool need_policy_change(old_mode, new_mode)
>
> and might be called before we enter the for_each_gt loop
good point
>> +
>> + ret = xe_guc_ads_scheduler_policy_toggle_reset(>->uc.guc.ads,
>> + !(mode == XE_WEDGED_MODE_UPON_ANY_HANG));
>> + if (ret)
>> + xe_gt_err(gt, "Failed to update GuC ADS scheduler policy (%pe)\n", ERR_PTR(ret));
>> +
>> + return ret;
>> +}
>> +
>> static ssize_t wedged_mode_set(struct file *f, const char __user *ubuf,
>> size_t size, loff_t *pos)
>> {
>> @@ -170,19 +189,24 @@ static ssize_t wedged_mode_set(struct file *f, const char __user *ubuf,
>> if (xe->wedged.mode == wedged_mode)
>> return size;
>>
>> - xe->wedged.mode = wedged_mode;
>> -
>> xe_pm_runtime_get(xe);
>> for_each_gt(gt, xe, id) {
>> - ret = xe_guc_ads_scheduler_policy_toggle_reset(>->uc.guc.ads);
>> + ret = wedged_mode_set_reset_policy(gt, wedged_mode);
>> if (ret) {
>> - xe_gt_err(gt, "Failed to update GuC ADS scheduler policy. GuC may still cause engine reset even with wedged_mode=2\n");
>> + if (id > 0) {
>> + xe->wedged.mode = XE_WEDGED_MODE_MISCONFIGURED;
> hmm, I'm not sure that the rest of the driver code will properly handle
> this new unexpected state - do we really need this? can't we just try to
> revert recent change where it was accepted? and if it fails there it
> still not the end of the world
>
> alternatively we can try to track GuC reset policy settings per-GuC, so
> we will know where it was left inconsistent with global 'wedge' mode
>
> or just wait until next mode update or GT reset where likely we will be
> able to restore the consistency
>
> IMO it should be fine just to report inconsistency, and then let the
> user decide what to do next
OK. I'll remove this extra state and will just report the inconsistency.
>
>> + drm_err(&xe->drm, "Inconsistent reset policy state between GTs, setting wedged mode: (%u) %s\n",
>> + xe->wedged.mode,
>> + xe_device_wedged_mode_to_string(xe->wedged.mode));
> this new state is not the one exposed in modparam description, user
> might be confused what it means
>
>> + }
>> xe_pm_runtime_put(xe);
>> - return -EIO;
>> + return ret;
>> }
>> }
>> xe_pm_runtime_put(xe);
>>
>> + xe->wedged.mode = wedged_mode;
>> +
>> return size;
>> }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index fa4f1a3ac1f2..0aea87994541 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -1206,6 +1206,8 @@ const char *xe_device_wedged_mode_to_string(enum xe_wedged_mode mode)
>> return "upon-critical-error";
>> case XE_WEDGED_MODE_UPON_ANY_HANG:
>> return "upon-any-hang";
>> + case XE_WEDGED_MODE_MISCONFIGURED:
>> + return "misconfigured";
>> default:
>> return "<invalid>";
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index a8dfacb951c1..8ec1d891a2ab 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -562,6 +562,7 @@ struct xe_device {
>> XE_WEDGED_MODE_NEVER = 0,
>> XE_WEDGED_MODE_UPON_CRITICAL_ERROR = 1,
>> XE_WEDGED_MODE_UPON_ANY_HANG = 2,
>> + XE_WEDGED_MODE_MISCONFIGURED = 3,
>> XE_WEDGED_MODE_DEFAULT = XE_WEDGED_MODE_UPON_CRITICAL_ERROR,
>> } mode;
>> } wedged;
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>> index ca8bdb0ec08d..f2c4c4e292f6 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> @@ -1008,14 +1008,14 @@ static int guc_ads_update_policies(struct xe_guc_ads *ads, const struct guc_poli
>> /**
>> * xe_guc_ads_scheduler_policy_toggle_reset - Toggle reset policy
>> * @ads: Additional data structures object
>> + * @enable: true to enable engine resets, false otherwise
>> *
>> - * This function update the GuC's engine reset policy based on wedged.mode.
>> + * This function update the GuC's engine reset policy.
>> *
>> * Return: 0 on success, and negative error code otherwise.
>> */
>> -int xe_guc_ads_scheduler_policy_toggle_reset(struct xe_guc_ads *ads)
>> +int xe_guc_ads_scheduler_policy_toggle_reset(struct xe_guc_ads *ads, bool enable)
>> {
>> - struct xe_device *xe = ads_to_xe(ads);
>> struct guc_policies *policies;
>> int ret;
>>
>> @@ -1026,10 +1026,10 @@ int xe_guc_ads_scheduler_policy_toggle_reset(struct xe_guc_ads *ads)
>> 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 == XE_WEDGED_MODE_UPON_ANY_HANG)
>> - policies->global_flags |= GLOBAL_POLICY_DISABLE_ENGINE_RESET;
>> - else
>> + if (enable)
>> policies->global_flags &= ~GLOBAL_POLICY_DISABLE_ENGINE_RESET;
>> + else
>> + policies->global_flags |= GLOBAL_POLICY_DISABLE_ENGINE_RESET;
> btw, do we correctly update policies stored in ADS that likely will be
> used again by the GuC on the subsequent reset ?
>
> adding @John
I've just checked - xe_guc_ads_populate() is called on reset. Then,
inside guc_policies_init() ADS reset policy is configured based on the
xe->wedged.mode.
>>
>> ret = guc_ads_update_policies(ads, policies);
>> kfree(policies);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.h b/drivers/gpu/drm/xe/xe_guc_ads.h
>> index 2e6674c760ff..9879aadd22d6 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ads.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.h
>> @@ -6,6 +6,8 @@
>> #ifndef _XE_GUC_ADS_H_
>> #define _XE_GUC_ADS_H_
>>
>> +#include <linux/types.h>
>> +
>> struct xe_guc_ads;
>>
>> int xe_guc_ads_init(struct xe_guc_ads *ads);
>> @@ -13,6 +15,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_toggle_reset(struct xe_guc_ads *ads);
>> +int xe_guc_ads_scheduler_policy_toggle_reset(struct xe_guc_ads *ads, bool enable);
>>
>> #endif
More information about the Intel-xe
mailing list