| Message ID | 20231005193536.1463159-5-jonathan.cavitt@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Subject: [PATCH dii-client v6 0/4] drm/i915: Define and use GuC and CTB TLB invalidation routines | expand |
On 05/10/2023 20:35, Jonathan Cavitt wrote: > In case of GT is suspended or wedged, don't allow submission of new TLB > invalidation request and cancel all pending requests. The TLB entries > will be invalidated either during GuC reload or on system resume. > > Signed-off-by: Fei Yang <fei.yang@intel.com> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > CC: John Harrison <john.c.harrison@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt.h | 1 + > drivers/gpu/drm/i915/gt/intel_tlb.c | 25 ++++++++++++ > drivers/gpu/drm/i915/gt/intel_tlb.h | 3 ++ > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 39 +++++++++++++++---- > drivers/gpu/drm/i915/i915_driver.c | 6 +++ > 6 files changed, 68 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h > index 970bedf6b78a7..4e3bb221d2f4d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h > @@ -179,4 +179,5 @@ enum i915_map_type intel_gt_coherent_map_type(struct intel_gt *gt, > void intel_gt_bind_context_set_ready(struct intel_gt *gt); > void intel_gt_bind_context_set_unready(struct intel_gt *gt); > bool intel_gt_is_bind_context_ready(struct intel_gt *gt); > + > #endif /* __INTEL_GT_H__ */ > diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.c b/drivers/gpu/drm/i915/gt/intel_tlb.c > index a84563c178bc6..c6bb1b34abdaa 100644 > --- a/drivers/gpu/drm/i915/gt/intel_tlb.c > +++ b/drivers/gpu/drm/i915/gt/intel_tlb.c > @@ -166,6 +166,31 @@ void intel_gt_fini_tlb(struct intel_gt *gt) > mutex_destroy(>->tlb.invalidate_lock); > } > > +void intel_gt_tlb_suspend_all(struct drm_i915_private *i915) > +{ > + struct intel_gt *gt; > + int i; > + > + if (!HAS_GUC_TLB_INVALIDATION(i915)) > + return; > + for_each_gt(gt, i915, i) > + wake_up_all_tlb_invalidate(>->uc.guc); > +} > + > +void intel_gt_tlb_resume_all(struct drm_i915_private *i915) > +{ > + struct intel_gt *gt; > + int i; > + > + if (!HAS_GUC_TLB_INVALIDATION(i915)) > + return; > + for_each_gt(gt, i915, i) { > + /* Perform tlb invalidation on both GT and GuC, in that order. */ > + intel_guc_invalidate_tlb_full(>->uc.guc); > + intel_guc_invalidate_tlb(>->uc.guc); > + } > +} 1) With this patch I was suggesting having the new helpers called from intel_gt_suspend(_*) and intel_gt_resume. Or there is also intel_uc_suspend/resume so maybe there. Either of those places would benefit from the existing for_each_gt applied at the GEM level, when it calls into the GT component. Are those at the wrong spot in the suspend/resume sequence so it wouldn't work? Maybe it is a matter of taste but since the implementation is so GT centric (everything added operates strictly on a >->uc.guc pointer) that it really looks to me it should be better placed in one of those two components. To be super clear suggestion is from the appropriate place, either intel_gt_suspend_early/late (or whatsitscalled) add a call to intel_gt_tlb_suspend(gt). Same for resume. Or from uc with uc naming if that works better. for_each_gt loops for those then already exist and patch becomes smaller. 2) New comment. It would be nicer if you could split the suspend/resume and wedged into two patches. > + > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > #include "selftest_tlb.c" > #endif > diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.h b/drivers/gpu/drm/i915/gt/intel_tlb.h > index 337327af92ac4..1a34ec0c447f2 100644 > --- a/drivers/gpu/drm/i915/gt/intel_tlb.h > +++ b/drivers/gpu/drm/i915/gt/intel_tlb.h > @@ -26,4 +26,7 @@ static inline u32 intel_gt_next_invalidate_tlb_full(const struct intel_gt *gt) > return intel_gt_tlb_seqno(gt) | 1; > } > > +void intel_gt_tlb_suspend_all(struct drm_i915_private *i915); > +void intel_gt_tlb_resume_all(struct drm_i915_private *i915); > + > #endif /* INTEL_TLB_H */ > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index d7203ba88b0c0..e76b26095eef2 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -536,4 +536,5 @@ void intel_guc_dump_time_info(struct intel_guc *guc, struct drm_printer *p); > > int intel_guc_sched_disable_gucid_threshold_max(struct intel_guc *guc); > > +void wake_up_all_tlb_invalidate(struct intel_guc *guc); > #endif > 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 7b484d1573f16..7172335199c76 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -32,6 +32,7 @@ > > #include "i915_drv.h" > #include "i915_reg.h" > +#include "i915_irq.h" > #include "i915_trace.h" > > /** > @@ -1796,13 +1797,23 @@ static void __guc_reset_context(struct intel_context *ce, intel_engine_mask_t st > intel_context_put(parent); > } > > -void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled) > +void wake_up_all_tlb_invalidate(struct intel_guc *guc) > { > struct intel_guc_tlb_wait *wait; > + unsigned long i; > + > + if (!HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915)) > + return; > + xa_for_each(&guc->tlb_lookup, i, wait) { > + wake_up(&wait->wq); > + } > +} > + > +void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled) > +{ > struct intel_context *ce; > unsigned long index; > unsigned long flags; > - unsigned long i; > > if (unlikely(!guc_submission_initialized(guc))) { > /* Reset called during driver load? GuC not yet initialised! */ > @@ -1833,9 +1844,7 @@ void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stall > * The full GT reset will have cleared the TLB caches and flushed the > * G2H message queue; we can release all the blocked waiters. > */ > - xa_for_each(&guc->tlb_lookup, i, wait) { > - wake_up(&wait->wq); > - } > + wake_up_all_tlb_invalidate(guc); > } > > static void guc_cancel_context_requests(struct intel_context *ce) > @@ -1931,6 +1940,12 @@ void intel_guc_submission_cancel_requests(struct intel_guc *guc) > > /* GuC is blown away, drop all references to contexts */ > xa_destroy(&guc->context_lookup); > + > + /* > + * Wedged GT won't respond to any TLB invalidation request. Simply > + * release all the blocked waiters. > + */ > + wake_up_all_tlb_invalidate(guc); > } > > void intel_guc_submission_reset_finish(struct intel_guc *guc) > @@ -4746,6 +4761,14 @@ static long must_wait_woken(struct wait_queue_entry *wq_entry, long timeout) > return timeout; > } > > +static bool intel_gt_is_enabled(const struct intel_gt *gt) > +{ > + /* Check if GT is wedged or suspended */ > + if (intel_gt_is_wedged(gt) || !intel_irqs_enabled(gt->i915)) > + return false; > + return true; > +} > + > static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) > { > struct intel_guc_tlb_wait _wq, *wq = &_wq; > @@ -4763,7 +4786,8 @@ static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) > }; > u32 size = ARRAY_SIZE(action); > > - if (!intel_guc_ct_enabled(&guc->ct)) > + if (!intel_gt_is_enabled(gt) || > + !intel_guc_ct_enabled(&guc->ct)) IMO this reads confused but I leave it to the GuC experts to decide what makes sense. Not only that it reads confused but it does inspire confidence that it closes any race, since this state can still change just after this check, and then the invalidation request gets submitted (contrary to what the commit says?). Only thing it does below is skip the wait and the time out error message. Again, I leave it for people who know the GuC state transition flows to bless this part. Regards, Tvrtko > return -EINVAL; > > init_waitqueue_head(&_wq.wq); > @@ -4806,7 +4830,8 @@ static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) > * can be queued in CT buffer. > */ > #define OUTSTANDING_GUC_TIMEOUT_PERIOD (HZ * 2) > - if (!must_wait_woken(&wait, OUTSTANDING_GUC_TIMEOUT_PERIOD)) { > + if (intel_gt_is_enabled(gt) && > + !must_wait_woken(&wait, OUTSTANDING_GUC_TIMEOUT_PERIOD)) { > gt_err(gt, > "TLB invalidation response timed out for seqno %u\n", seqno); > err = -ETIME; > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c > index ccbb2834cde07..0c9d9826d2f41 100644 > --- a/drivers/gpu/drm/i915/i915_driver.c > +++ b/drivers/gpu/drm/i915/i915_driver.c > @@ -72,6 +72,7 @@ > #include "gt/intel_gt.h" > #include "gt/intel_gt_pm.h" > #include "gt/intel_rc6.h" > +#include "gt/intel_tlb.h" > > #include "pxp/intel_pxp.h" > #include "pxp/intel_pxp_debugfs.h" > @@ -1093,6 +1094,9 @@ static int i915_drm_suspend(struct drm_device *dev) > intel_dp_mst_suspend(dev_priv); > > intel_runtime_pm_disable_interrupts(dev_priv); > + > + intel_gt_tlb_suspend_all(dev_priv); > + > intel_hpd_cancel_work(dev_priv); > > intel_suspend_encoders(dev_priv); > @@ -1264,6 +1268,8 @@ static int i915_drm_resume(struct drm_device *dev) > > intel_gvt_resume(dev_priv); > > + intel_gt_tlb_resume_all(dev_priv); > + > enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); > > return 0;
On 10/6/2023 03:23, Tvrtko Ursulin wrote: > > > On 05/10/2023 20:35, Jonathan Cavitt wrote: >> ... >> +static bool intel_gt_is_enabled(const struct intel_gt *gt) >> +{ >> + /* Check if GT is wedged or suspended */ >> + if (intel_gt_is_wedged(gt) || !intel_irqs_enabled(gt->i915)) >> + return false; >> + return true; >> +} >> + >> static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) >> { >> struct intel_guc_tlb_wait _wq, *wq = &_wq; >> @@ -4763,7 +4786,8 @@ static int guc_send_invalidate_tlb(struct >> intel_guc *guc, u32 type) >> }; >> u32 size = ARRAY_SIZE(action); >> - if (!intel_guc_ct_enabled(&guc->ct)) >> + if (!intel_gt_is_enabled(gt) || >> + !intel_guc_ct_enabled(&guc->ct)) > > IMO this reads confused but I leave it to the GuC experts to decide > what makes sense. Not only that it reads confused but it does inspire > confidence that it closes any race, since this state can still change > just after this check, and then the invalidation request gets > submitted (contrary to what the commit says?). Only thing it does > below is skip the wait and the time out error message. Again, I leave > it for people who know the GuC state transition flows to bless this part. > > Regards, > > Tvrtko Regarding confused naming, I personally still think that intel_gt_is_enabled() is a bad name. Even the comment inside the function does not mention 'enable', it says 'wedged or suspended'. One could certainly argue that the GT is also not currently enabled if GuC is in use but the CT channel is down. Regarding race conditions, the only things that can take the CT channel down are driver shutdown, suspend and GT reset. On the submission side, the assumption is that the scheduling levels of the driver are not going to call in to the submission backend without suitable locking held to ensure those operations cannot occur concurrently. Is the same not true here? We have to guard against the situation where the call starts from a 'bad' state, e.g. wedged. But the lowest level of code can't be expected to take higher level locks. From all the way down here, we have no idea what the upper levels may or may not be doing and what locks may or may not have been acquired, and therefore what locks may or may not be safe to acquire. John. > >> return -EINVAL; >> init_waitqueue_head(&_wq.wq); >> @@ -4806,7 +4830,8 @@ static int guc_send_invalidate_tlb(struct >> intel_guc *guc, u32 type) >> * can be queued in CT buffer. >> */ >> #define OUTSTANDING_GUC_TIMEOUT_PERIOD (HZ * 2) >> - if (!must_wait_woken(&wait, OUTSTANDING_GUC_TIMEOUT_PERIOD)) { >> + if (intel_gt_is_enabled(gt) && >> + !must_wait_woken(&wait, OUTSTANDING_GUC_TIMEOUT_PERIOD)) { >> gt_err(gt, >> "TLB invalidation response timed out for seqno >> %u\n", seqno); >> err = -ETIME; >> diff --git a/drivers/gpu/drm/i915/i915_driver.c >> b/drivers/gpu/drm/i915/i915_driver.c >> index ccbb2834cde07..0c9d9826d2f41 100644 >> --- a/drivers/gpu/drm/i915/i915_driver.c >> +++ b/drivers/gpu/drm/i915/i915_driver.c >> @@ -72,6 +72,7 @@ >> #include "gt/intel_gt.h" >> #include "gt/intel_gt_pm.h" >> #include "gt/intel_rc6.h" >> +#include "gt/intel_tlb.h" >> #include "pxp/intel_pxp.h" >> #include "pxp/intel_pxp_debugfs.h" >> @@ -1093,6 +1094,9 @@ static int i915_drm_suspend(struct drm_device >> *dev) >> intel_dp_mst_suspend(dev_priv); >> intel_runtime_pm_disable_interrupts(dev_priv); >> + >> + intel_gt_tlb_suspend_all(dev_priv); >> + >> intel_hpd_cancel_work(dev_priv); >> intel_suspend_encoders(dev_priv); >> @@ -1264,6 +1268,8 @@ static int i915_drm_resume(struct drm_device *dev) >> intel_gvt_resume(dev_priv); >> + intel_gt_tlb_resume_all(dev_priv); >> + >> enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); >> return 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 970bedf6b78a7..4e3bb221d2f4d 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -179,4 +179,5 @@ enum i915_map_type intel_gt_coherent_map_type(struct intel_gt *gt, void intel_gt_bind_context_set_ready(struct intel_gt *gt); void intel_gt_bind_context_set_unready(struct intel_gt *gt); bool intel_gt_is_bind_context_ready(struct intel_gt *gt); + #endif /* __INTEL_GT_H__ */ diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.c b/drivers/gpu/drm/i915/gt/intel_tlb.c index a84563c178bc6..c6bb1b34abdaa 100644 --- a/drivers/gpu/drm/i915/gt/intel_tlb.c +++ b/drivers/gpu/drm/i915/gt/intel_tlb.c @@ -166,6 +166,31 @@ void intel_gt_fini_tlb(struct intel_gt *gt) mutex_destroy(>->tlb.invalidate_lock); } +void intel_gt_tlb_suspend_all(struct drm_i915_private *i915) +{ + struct intel_gt *gt; + int i; + + if (!HAS_GUC_TLB_INVALIDATION(i915)) + return; + for_each_gt(gt, i915, i) + wake_up_all_tlb_invalidate(>->uc.guc); +} + +void intel_gt_tlb_resume_all(struct drm_i915_private *i915) +{ + struct intel_gt *gt; + int i; + + if (!HAS_GUC_TLB_INVALIDATION(i915)) + return; + for_each_gt(gt, i915, i) { + /* Perform tlb invalidation on both GT and GuC, in that order. */ + intel_guc_invalidate_tlb_full(>->uc.guc); + intel_guc_invalidate_tlb(>->uc.guc); + } +} + #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftest_tlb.c" #endif diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.h b/drivers/gpu/drm/i915/gt/intel_tlb.h index 337327af92ac4..1a34ec0c447f2 100644 --- a/drivers/gpu/drm/i915/gt/intel_tlb.h +++ b/drivers/gpu/drm/i915/gt/intel_tlb.h @@ -26,4 +26,7 @@ static inline u32 intel_gt_next_invalidate_tlb_full(const struct intel_gt *gt) return intel_gt_tlb_seqno(gt) | 1; } +void intel_gt_tlb_suspend_all(struct drm_i915_private *i915); +void intel_gt_tlb_resume_all(struct drm_i915_private *i915); + #endif /* INTEL_TLB_H */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index d7203ba88b0c0..e76b26095eef2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -536,4 +536,5 @@ void intel_guc_dump_time_info(struct intel_guc *guc, struct drm_printer *p); int intel_guc_sched_disable_gucid_threshold_max(struct intel_guc *guc); +void wake_up_all_tlb_invalidate(struct intel_guc *guc); #endif 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 7b484d1573f16..7172335199c76 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -32,6 +32,7 @@ #include "i915_drv.h" #include "i915_reg.h" +#include "i915_irq.h" #include "i915_trace.h" /** @@ -1796,13 +1797,23 @@ static void __guc_reset_context(struct intel_context *ce, intel_engine_mask_t st intel_context_put(parent); } -void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled) +void wake_up_all_tlb_invalidate(struct intel_guc *guc) { struct intel_guc_tlb_wait *wait; + unsigned long i; + + if (!HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915)) + return; + xa_for_each(&guc->tlb_lookup, i, wait) { + wake_up(&wait->wq); + } +} + +void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled) +{ struct intel_context *ce; unsigned long index; unsigned long flags; - unsigned long i; if (unlikely(!guc_submission_initialized(guc))) { /* Reset called during driver load? GuC not yet initialised! */ @@ -1833,9 +1844,7 @@ void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stall * The full GT reset will have cleared the TLB caches and flushed the * G2H message queue; we can release all the blocked waiters. */ - xa_for_each(&guc->tlb_lookup, i, wait) { - wake_up(&wait->wq); - } + wake_up_all_tlb_invalidate(guc); } static void guc_cancel_context_requests(struct intel_context *ce) @@ -1931,6 +1940,12 @@ void intel_guc_submission_cancel_requests(struct intel_guc *guc) /* GuC is blown away, drop all references to contexts */ xa_destroy(&guc->context_lookup); + + /* + * Wedged GT won't respond to any TLB invalidation request. Simply + * release all the blocked waiters. + */ + wake_up_all_tlb_invalidate(guc); } void intel_guc_submission_reset_finish(struct intel_guc *guc) @@ -4746,6 +4761,14 @@ static long must_wait_woken(struct wait_queue_entry *wq_entry, long timeout) return timeout; } +static bool intel_gt_is_enabled(const struct intel_gt *gt) +{ + /* Check if GT is wedged or suspended */ + if (intel_gt_is_wedged(gt) || !intel_irqs_enabled(gt->i915)) + return false; + return true; +} + static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) { struct intel_guc_tlb_wait _wq, *wq = &_wq; @@ -4763,7 +4786,8 @@ static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) }; u32 size = ARRAY_SIZE(action); - if (!intel_guc_ct_enabled(&guc->ct)) + if (!intel_gt_is_enabled(gt) || + !intel_guc_ct_enabled(&guc->ct)) return -EINVAL; init_waitqueue_head(&_wq.wq); @@ -4806,7 +4830,8 @@ static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 type) * can be queued in CT buffer. */ #define OUTSTANDING_GUC_TIMEOUT_PERIOD (HZ * 2) - if (!must_wait_woken(&wait, OUTSTANDING_GUC_TIMEOUT_PERIOD)) { + if (intel_gt_is_enabled(gt) && + !must_wait_woken(&wait, OUTSTANDING_GUC_TIMEOUT_PERIOD)) { gt_err(gt, "TLB invalidation response timed out for seqno %u\n", seqno); err = -ETIME; diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index ccbb2834cde07..0c9d9826d2f41 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -72,6 +72,7 @@ #include "gt/intel_gt.h" #include "gt/intel_gt_pm.h" #include "gt/intel_rc6.h" +#include "gt/intel_tlb.h" #include "pxp/intel_pxp.h" #include "pxp/intel_pxp_debugfs.h" @@ -1093,6 +1094,9 @@ static int i915_drm_suspend(struct drm_device *dev) intel_dp_mst_suspend(dev_priv); intel_runtime_pm_disable_interrupts(dev_priv); + + intel_gt_tlb_suspend_all(dev_priv); + intel_hpd_cancel_work(dev_priv); intel_suspend_encoders(dev_priv); @@ -1264,6 +1268,8 @@ static int i915_drm_resume(struct drm_device *dev) intel_gvt_resume(dev_priv); + intel_gt_tlb_resume_all(dev_priv); + enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); return 0;