Re: [Intel-gfx] [PATCH v4 3/3] drm/i915: No TLB invalidation on wedged or suspended GT
On 03/10/2023 22:01, 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
Signed-off-by: Jonathan Cavitt
CC: John Harrison
---
drivers/gpu/drm/i915/gt/intel_gt.h| 26
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 1 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 41 +++
drivers/gpu/drm/i915/i915_driver.c| 5 +++
4 files changed, 64 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 970bedf6b78a7..71a0e376ded40 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -9,6 +9,7 @@
#include "intel_engine_types.h"
#include "intel_gt_types.h"
#include "intel_reset.h"
+#include "i915_drv.h"
struct drm_i915_private;
struct drm_printer;
@@ -179,4 +180,29 @@ 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);
+
+static inline void intel_tlb_suspend(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);
+}
+
+static inline void intel_tlb_resume(struct drm_i915_private *i915)
+{
+ struct intel_gt *gt;
+ int i;
+
+ if (!HAS_GUC_TLB_INVALIDATION(i915))
+ return;
+ for_each_gt(gt, i915, i) {
+ intel_guc_invalidate_tlb_full(>->uc.guc);
+ intel_guc_invalidate_tlb(>->uc.guc);
Why full and not full? At least the naming suggestes first to be
superset of the second.
+ }
+}
I see no justification for static inlines and I see no reason not to
have this in intel_tlb.h|c.
Also, I suggested having them called from intel_gt_suspend(_*) and
intel_gt_resume. Or there is also intel_uc_suspend/resume. 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.
Regards,
Tvrtko
+
#endif /* __INTEL_GT_H__ */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 636edf598946c..e2491f489f1bc 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 872014a801c7e..20e9076cf099e 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;
+
+ xa_for_each(&guc->tlb_lookup, i, wait) {
+ /* Barrier to ensure the store is observed by the woken thread
*/
+ smp_store_mb(wait->busy, 0);
+ 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,11 +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) {
- /* Barrier to ensure the store is observed by the woken thread
*/
- smp_store_mb(wait->busy, 0);
-
Re: [Intel-gfx] [PATCH v4 3/3] drm/i915: No TLB invalidation on wedged or suspended GT
On Tue, 03 Oct 2023, 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
> Signed-off-by: Jonathan Cavitt
> CC: John Harrison
> ---
> drivers/gpu/drm/i915/gt/intel_gt.h| 26
> drivers/gpu/drm/i915/gt/uc/intel_guc.h| 1 +
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 41 +++
> drivers/gpu/drm/i915/i915_driver.c| 5 +++
> 4 files changed, 64 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 970bedf6b78a7..71a0e376ded40 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -9,6 +9,7 @@
> #include "intel_engine_types.h"
> #include "intel_gt_types.h"
> #include "intel_reset.h"
> +#include "i915_drv.h"
Do not include headers from headers, especially do not include
i915_drv.h from headers.
I thought I just explained this in a previous thread.
>
> struct drm_i915_private;
> struct drm_printer;
> @@ -179,4 +180,29 @@ 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);
> +
> +static inline void intel_tlb_suspend(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);
> +}
> +
> +static inline void intel_tlb_resume(struct drm_i915_private *i915)
> +{
> + struct intel_gt *gt;
> + int i;
> +
> + if (!HAS_GUC_TLB_INVALIDATION(i915))
> + return;
> + for_each_gt(gt, i915, i) {
> + intel_guc_invalidate_tlb_full(>->uc.guc);
> + intel_guc_invalidate_tlb(>->uc.guc);
> + }
> +}
Do not add static inlines that require you to pull in other headers, at
least not without justification. I don't think this one could be
justified.
I thought I just explained this in a prevous thread.
I honestly don't know what you think you'll achieve by using static
inlines here, but *always* err on the side of making them proper
functions instead.
Side note on naming, functions beginning with intel_tlb_ prefix belong
in intel_tlb.[ch]. If these don't belong there, then the prefix is
wrong.
BR,
Jani.
> +
> #endif /* __INTEL_GT_H__ */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 636edf598946c..e2491f489f1bc 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 872014a801c7e..20e9076cf099e 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;
> +
> + xa_for_each(&guc->tlb_lookup, i, wait) {
> + /* Barrier to ensure the store is observed by the woken thread
> */
> + smp_store_mb(wait->busy, 0);
> + 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,11 +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) {
> - /* Barrier to ensure the store is observed by the woken thread
> */
> - smp_store_mb(w
[Intel-gfx] [PATCH v4 3/3] drm/i915: No TLB invalidation on wedged or suspended GT
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
Signed-off-by: Jonathan Cavitt
CC: John Harrison
---
drivers/gpu/drm/i915/gt/intel_gt.h| 26
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 1 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 41 +++
drivers/gpu/drm/i915/i915_driver.c| 5 +++
4 files changed, 64 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 970bedf6b78a7..71a0e376ded40 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -9,6 +9,7 @@
#include "intel_engine_types.h"
#include "intel_gt_types.h"
#include "intel_reset.h"
+#include "i915_drv.h"
struct drm_i915_private;
struct drm_printer;
@@ -179,4 +180,29 @@ 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);
+
+static inline void intel_tlb_suspend(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);
+}
+
+static inline void intel_tlb_resume(struct drm_i915_private *i915)
+{
+ struct intel_gt *gt;
+ int i;
+
+ if (!HAS_GUC_TLB_INVALIDATION(i915))
+ return;
+ for_each_gt(gt, i915, i) {
+ intel_guc_invalidate_tlb_full(>->uc.guc);
+ intel_guc_invalidate_tlb(>->uc.guc);
+ }
+}
+
#endif /* __INTEL_GT_H__ */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 636edf598946c..e2491f489f1bc 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 872014a801c7e..20e9076cf099e 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;
+
+ xa_for_each(&guc->tlb_lookup, i, wait) {
+ /* Barrier to ensure the store is observed by the woken thread
*/
+ smp_store_mb(wait->busy, 0);
+ 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,11 +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) {
- /* Barrier to ensure the store is observed by the woken thread
*/
- smp_store_mb(wait->busy, 0);
- wake_up(&wait->wq);
- }
+ wake_up_all_tlb_invalidate(guc);
}
static void guc_cancel_context_requests(struct intel_context *ce)
@@ -1933,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)
@@ -4740,6 +4753,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 (i
