| Message ID | 20230217223308.3449737-2-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up some GuC related failure paths | expand |
On 2/17/2023 2:33 PM, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The stats worker thread management was mis-matched between > enable/disable call sites. Fix those up. Also, abstract the > cancel/enable code into a helper function rather than replicating in > multiple places. > > v2: Rename the helpers and wrap the enable as well as the cancel > (review feedback from Daniele). > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele > --- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 38 +++++++++++-------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index be495e657d66b..a04d7049a2c2f 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1352,6 +1352,16 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs *engine, ktime_t *now) > return ns_to_ktime(total); > } > > +static void guc_enable_busyness_worker(struct intel_guc *guc) > +{ > + mod_delayed_work(system_highpri_wq, &guc->timestamp.work, guc->timestamp.ping_delay); > +} > + > +static void guc_cancel_busyness_worker(struct intel_guc *guc) > +{ > + cancel_delayed_work_sync(&guc->timestamp.work); > +} > + > static void __reset_guc_busyness_stats(struct intel_guc *guc) > { > struct intel_gt *gt = guc_to_gt(guc); > @@ -1360,7 +1370,7 @@ static void __reset_guc_busyness_stats(struct intel_guc *guc) > unsigned long flags; > ktime_t unused; > > - cancel_delayed_work_sync(&guc->timestamp.work); > + guc_cancel_busyness_worker(guc); > > spin_lock_irqsave(&guc->timestamp.lock, flags); > > @@ -1416,8 +1426,7 @@ static void guc_timestamp_ping(struct work_struct *wrk) > > intel_gt_reset_unlock(gt, srcu); > > - mod_delayed_work(system_highpri_wq, &guc->timestamp.work, > - guc->timestamp.ping_delay); > + guc_enable_busyness_worker(guc); > } > > static int guc_action_enable_usage_stats(struct intel_guc *guc) > @@ -1436,16 +1445,15 @@ static void guc_init_engine_stats(struct intel_guc *guc) > { > struct intel_gt *gt = guc_to_gt(guc); > intel_wakeref_t wakeref; > + int ret; > > - mod_delayed_work(system_highpri_wq, &guc->timestamp.work, > - guc->timestamp.ping_delay); > - > - with_intel_runtime_pm(>->i915->runtime_pm, wakeref) { > - int ret = guc_action_enable_usage_stats(guc); > + with_intel_runtime_pm(>->i915->runtime_pm, wakeref) > + ret = guc_action_enable_usage_stats(guc); > > - if (ret) > - guc_err(guc, "Failed to enable usage stats: %pe\n", ERR_PTR(ret)); > - } > + if (ret) > + guc_err(guc, "Failed to enable usage stats: %pe\n", ERR_PTR(ret)); > + else > + guc_enable_busyness_worker(guc); > } > > void intel_guc_busyness_park(struct intel_gt *gt) > @@ -1460,7 +1468,7 @@ void intel_guc_busyness_park(struct intel_gt *gt) > * and causes an unclaimed register access warning. Cancel the worker > * synchronously here. > */ > - cancel_delayed_work_sync(&guc->timestamp.work); > + guc_cancel_busyness_worker(guc); > > /* > * Before parking, we should sample engine busyness stats if we need to. > @@ -1487,8 +1495,7 @@ void intel_guc_busyness_unpark(struct intel_gt *gt) > spin_lock_irqsave(&guc->timestamp.lock, flags); > guc_update_pm_timestamp(guc, &unused); > spin_unlock_irqrestore(&guc->timestamp.lock, flags); > - mod_delayed_work(system_highpri_wq, &guc->timestamp.work, > - guc->timestamp.ping_delay); > + guc_enable_busyness_worker(guc); > } > > static inline bool > @@ -4408,11 +4415,12 @@ void intel_guc_submission_enable(struct intel_guc *guc) > guc_init_global_schedule_policy(guc); > } > > +/* Note: By the time we're here, GuC may have already been reset */ > void intel_guc_submission_disable(struct intel_guc *guc) > { > struct intel_gt *gt = guc_to_gt(guc); > > - /* Note: By the time we're here, GuC may have already been reset */ > + guc_cancel_busyness_worker(guc); > > /* Disable and route to host */ > if (GRAPHICS_VER(gt->i915) >= 12)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index be495e657d66b..a04d7049a2c2f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1352,6 +1352,16 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs *engine, ktime_t *now) return ns_to_ktime(total); } +static void guc_enable_busyness_worker(struct intel_guc *guc) +{ + mod_delayed_work(system_highpri_wq, &guc->timestamp.work, guc->timestamp.ping_delay); +} + +static void guc_cancel_busyness_worker(struct intel_guc *guc) +{ + cancel_delayed_work_sync(&guc->timestamp.work); +} + static void __reset_guc_busyness_stats(struct intel_guc *guc) { struct intel_gt *gt = guc_to_gt(guc); @@ -1360,7 +1370,7 @@ static void __reset_guc_busyness_stats(struct intel_guc *guc) unsigned long flags; ktime_t unused; - cancel_delayed_work_sync(&guc->timestamp.work); + guc_cancel_busyness_worker(guc); spin_lock_irqsave(&guc->timestamp.lock, flags); @@ -1416,8 +1426,7 @@ static void guc_timestamp_ping(struct work_struct *wrk) intel_gt_reset_unlock(gt, srcu); - mod_delayed_work(system_highpri_wq, &guc->timestamp.work, - guc->timestamp.ping_delay); + guc_enable_busyness_worker(guc); } static int guc_action_enable_usage_stats(struct intel_guc *guc) @@ -1436,16 +1445,15 @@ static void guc_init_engine_stats(struct intel_guc *guc) { struct intel_gt *gt = guc_to_gt(guc); intel_wakeref_t wakeref; + int ret; - mod_delayed_work(system_highpri_wq, &guc->timestamp.work, - guc->timestamp.ping_delay); - - with_intel_runtime_pm(>->i915->runtime_pm, wakeref) { - int ret = guc_action_enable_usage_stats(guc); + with_intel_runtime_pm(>->i915->runtime_pm, wakeref) + ret = guc_action_enable_usage_stats(guc); - if (ret) - guc_err(guc, "Failed to enable usage stats: %pe\n", ERR_PTR(ret)); - } + if (ret) + guc_err(guc, "Failed to enable usage stats: %pe\n", ERR_PTR(ret)); + else + guc_enable_busyness_worker(guc); } void intel_guc_busyness_park(struct intel_gt *gt) @@ -1460,7 +1468,7 @@ void intel_guc_busyness_park(struct intel_gt *gt) * and causes an unclaimed register access warning. Cancel the worker * synchronously here. */ - cancel_delayed_work_sync(&guc->timestamp.work); + guc_cancel_busyness_worker(guc); /* * Before parking, we should sample engine busyness stats if we need to. @@ -1487,8 +1495,7 @@ void intel_guc_busyness_unpark(struct intel_gt *gt) spin_lock_irqsave(&guc->timestamp.lock, flags); guc_update_pm_timestamp(guc, &unused); spin_unlock_irqrestore(&guc->timestamp.lock, flags); - mod_delayed_work(system_highpri_wq, &guc->timestamp.work, - guc->timestamp.ping_delay); + guc_enable_busyness_worker(guc); } static inline bool @@ -4408,11 +4415,12 @@ void intel_guc_submission_enable(struct intel_guc *guc) guc_init_global_schedule_policy(guc); } +/* Note: By the time we're here, GuC may have already been reset */ void intel_guc_submission_disable(struct intel_guc *guc) { struct intel_gt *gt = guc_to_gt(guc); - /* Note: By the time we're here, GuC may have already been reset */ + guc_cancel_busyness_worker(guc); /* Disable and route to host */ if (GRAPHICS_VER(gt->i915) >= 12)