On 06/05/2021 20:13, Matthew Brost wrote:

From: Chris Wilson chris@chris-wilson.co.uk

Now that we no longer switch back and forth between guc and execlists, we no longer need to restore the backend's vfunc and can leave them set after initialisation. The only catch is that we lose the submission on wedging and still need to reset the submit_request vfunc on unwedging.

Signed-off-by: Matthew Brost matthew.brost@intel.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com

This patch had my r-b already so I'll repeat it:

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

---

.../drm/i915/gt/intel_execlists_submission.c | 46 ++++++++--------- .../gpu/drm/i915/gt/intel_ring_submission.c | 4 -- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 50 ++++++++----------- 3 files changed, 44 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index de124870af44..1108c193ab65 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -3076,29 +3076,6 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) engine->submit_request = execlists_submit_request; engine->schedule = i915_schedule; engine->execlists.tasklet.callback = execlists_submission_tasklet;

• engine->reset.prepare = execlists_reset_prepare;

• engine->reset.rewind = execlists_reset_rewind;

• engine->reset.cancel = execlists_reset_cancel;

• engine->reset.finish = execlists_reset_finish;

• engine->park = execlists_park;

• engine->unpark = NULL;

• engine->flags |= I915_ENGINE_SUPPORTS_STATS;

• if (!intel_vgpu_active(engine->i915)) {

• engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
  

• if (can_preempt(engine)) {
  

• 	engine->flags |= I915_ENGINE_HAS_PREEMPTION;
  

• 	if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
  

• 		engine->flags |= I915_ENGINE_HAS_TIMESLICES;
  

• }
  

• }

• if (intel_engine_has_preemption(engine))

• engine->emit_bb_start = gen8_emit_bb_start;
  

• else

• engine->emit_bb_start = gen8_emit_bb_start_noarb;
  

  }

  static void execlists_shutdown(struct intel_engine_cs *engine)

@@ -3129,6 +3106,14 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) engine->cops = &execlists_context_ops; engine->request_alloc = execlists_request_alloc;

• engine->reset.prepare = execlists_reset_prepare;
• engine->reset.rewind = execlists_reset_rewind;
• engine->reset.cancel = execlists_reset_cancel;
• engine->reset.finish = execlists_reset_finish;
• engine->park = execlists_park;
• engine->unpark = NULL;
• engine->emit_flush = gen8_emit_flush_xcs; engine->emit_init_breadcrumb = gen8_emit_init_breadcrumb; engine->emit_fini_breadcrumb = gen8_emit_fini_breadcrumb_xcs;

@@ -3149,6 +3134,21 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) * until a more refined solution exists. */ }

• engine->flags |= I915_ENGINE_SUPPORTS_STATS;

• if (!intel_vgpu_active(engine->i915)) {

• engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
  

• if (can_preempt(engine)) {
  

• 	engine->flags |= I915_ENGINE_HAS_PREEMPTION;
  

• 	if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
  

• 		engine->flags |= I915_ENGINE_HAS_TIMESLICES;
  

• }
  

• }

• if (intel_engine_has_preemption(engine))

• engine->emit_bb_start = gen8_emit_bb_start;
  

• else

• engine->emit_bb_start = gen8_emit_bb_start_noarb;
  

  }

  static void logical_ring_default_irqs(struct intel_engine_cs *engine)

diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c index 9585546556ee..5f4f7f1df48f 100644 --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c @@ -989,14 +989,10 @@ static void gen6_bsd_submit_request(struct i915_request *request) static void i9xx_set_default_submission(struct intel_engine_cs *engine) { engine->submit_request = i9xx_submit_request;

• engine->park = NULL;

• engine->unpark = NULL; }

  static void gen6_bsd_set_default_submission(struct intel_engine_cs *engine) {

• i9xx_set_default_submission(engine); engine->submit_request = gen6_bsd_submit_request; }

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 92688a9b6717..f72faa0b8339 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -608,35 +608,6 @@ static int guc_resume(struct intel_engine_cs *engine) static void guc_set_default_submission(struct intel_engine_cs *engine) { engine->submit_request = guc_submit_request;

• engine->schedule = i915_schedule;

• engine->execlists.tasklet.callback = guc_submission_tasklet;

• engine->reset.prepare = guc_reset_prepare;

• engine->reset.rewind = guc_reset_rewind;

• engine->reset.cancel = guc_reset_cancel;

• engine->reset.finish = guc_reset_finish;

• engine->flags |= I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;

• engine->flags |= I915_ENGINE_HAS_PREEMPTION;

• /*

• * TODO: GuC supports timeslicing and semaphores as well, but they're
  

• * handled by the firmware so some minor tweaks are required before
  

• * enabling.
  

• *
  

• * engine->flags |= I915_ENGINE_HAS_TIMESLICES;
  

• * engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
  

• */
  

• engine->emit_bb_start = gen8_emit_bb_start;

• /*

• * For the breadcrumb irq to work we need the interrupts to stay
  

• * enabled. However, on all platforms on which we'll have support for
  

• * GuC submission we don't allow disabling the interrupts at runtime, so
  

• * we're always safe with the current flow.
  

• */
  

• GEM_BUG_ON(engine->irq_enable || engine->irq_disable); }

  static void guc_release(struct intel_engine_cs *engine)

@@ -658,6 +629,13 @@ static void guc_default_vfuncs(struct intel_engine_cs *engine) engine->cops = &guc_context_ops; engine->request_alloc = guc_request_alloc;

• engine->schedule = i915_schedule;
• engine->reset.prepare = guc_reset_prepare;
• engine->reset.rewind = guc_reset_rewind;
• engine->reset.cancel = guc_reset_cancel;
• engine->reset.finish = guc_reset_finish;
• engine->emit_flush = gen8_emit_flush_xcs; engine->emit_init_breadcrumb = gen8_emit_init_breadcrumb; engine->emit_fini_breadcrumb = gen8_emit_fini_breadcrumb_xcs;

@@ -666,6 +644,20 @@ static void guc_default_vfuncs(struct intel_engine_cs *engine) engine->emit_flush = gen12_emit_flush_xcs; } engine->set_default_submission = guc_set_default_submission;

• engine->flags |= I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;

• engine->flags |= I915_ENGINE_HAS_PREEMPTION;

• /*

• * TODO: GuC supports timeslicing and semaphores as well, but they're
  

• * handled by the firmware so some minor tweaks are required before
  

• * enabling.
  

• *
  

• * engine->flags |= I915_ENGINE_HAS_TIMESLICES;
  

• * engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
  

• */
  

• engine->emit_bb_start = gen8_emit_bb_start; }

  static void rcs_submission_override(struct intel_engine_cs *engine)

On Thu, May 06, 2021 at 12:13:16PM -0700, Matthew Brost wrote:

From: Chris Wilson chris@chris-wilson.co.uk

Since we setup the submission method for the engines once, it is easy to assign an enum and use that instead of probing into the backends.

Signed-off-by: Matthew Brost matthew.brost@intel.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com

Reviewed-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_engine.h | 8 +++++++- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 ++++++++---- drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 8 -------- drivers/gpu/drm/i915/gt/intel_execlists_submission.h | 3 --- drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 +++++++ drivers/gpu/drm/i915/gt/intel_reset.c | 7 +++---- drivers/gpu/drm/i915/gt/selftest_execlists.c | 2 +- drivers/gpu/drm/i915/gt/selftest_ring_submission.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 ----- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 1 - drivers/gpu/drm/i915/i915_perf.c | 10 +++++----- 11 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index 47ee8578e511..8d9184920c51 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -13,8 +13,9 @@ #include "i915_reg.h" #include "i915_request.h" #include "i915_selftest.h" -#include "gt/intel_timeline.h" #include "intel_engine_types.h" +#include "intel_gt_types.h" +#include "intel_timeline.h" #include "intel_workarounds.h"

struct drm_printer; @@ -262,6 +263,11 @@ void intel_engine_init_active(struct intel_engine_cs *engine, #define ENGINE_MOCK 1 #define ENGINE_VIRTUAL 2

+static inline bool intel_engine_uses_guc(const struct intel_engine_cs *engine) +{

• return engine->gt->submission_method >= INTEL_SUBMISSION_GUC;

+}

static inline bool intel_engine_has_preempt_reset(const struct intel_engine_cs *engine) { diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 6dbdbde00f14..0618379b68ca 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -909,12 +909,16 @@ int intel_engines_init(struct intel_gt *gt) enum intel_engine_id id; int err;

• if (intel_uc_uses_guc_submission(&gt->uc))

• if (intel_uc_uses_guc_submission(&gt->uc)) {

• gt->submission_method = INTEL_SUBMISSION_GUC;
  

  setup = intel_guc_submission_setup;

• else if (HAS_EXECLISTS(gt->i915))

• } else if (HAS_EXECLISTS(gt->i915)) {

• gt->submission_method = INTEL_SUBMISSION_ELSP;
  

  setup = intel_execlists_submission_setup;

• else

• } else {

• gt->submission_method = INTEL_SUBMISSION_RING;
  

  setup = intel_ring_submission_setup;

• }

  for_each_engine(engine, gt, id) { err = engine_setup_common(engine);

@@ -1479,7 +1483,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, drm_printf(m, "\tIPEHR: 0x%08x\n", ENGINE_READ(engine, IPEHR)); }

• if (intel_engine_in_guc_submission_mode(engine)) {

• if (intel_engine_uses_guc(engine)) { /* nothing to print yet */ } else if (HAS_EXECLISTS(dev_priv)) { struct i915_request * const *port, *rq;

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 1108c193ab65..9d2da5ccaef6 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1768,7 +1768,6 @@ process_csb(struct intel_engine_cs *engine, struct i915_request **inactive) */ GEM_BUG_ON(!tasklet_is_locked(&execlists->tasklet) && !reset_in_progress(execlists));

• GEM_BUG_ON(!intel_engine_in_execlists_submission_mode(engine));

  /*

  • Note that csb_write, csb_status may be either in HWSP or mmio.

@@ -3884,13 +3883,6 @@ void intel_execlists_show_requests(struct intel_engine_cs *engine, spin_unlock_irqrestore(&engine->active.lock, flags); }

-bool -intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine) -{

• return engine->set_default_submission ==

•       execlists_set_default_submission;
  

-}

#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftest_execlists.c" #endif diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h index fd61dae820e9..4ca9b475e252 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h @@ -43,7 +43,4 @@ int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine, const struct intel_engine_cs *master, const struct intel_engine_cs *sibling);

-bool -intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine);

#endif /* __INTEL_EXECLISTS_SUBMISSION_H__ */ diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h index 0caf6ca0a784..fecfacf551d5 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h @@ -31,6 +31,12 @@ struct i915_ggtt; struct intel_engine_cs; struct intel_uncore;

+enum intel_submission_method {

• INTEL_SUBMISSION_RING,
• INTEL_SUBMISSION_ELSP,
• INTEL_SUBMISSION_GUC,

+};

struct intel_gt { struct drm_i915_private *i915; struct intel_uncore *uncore; @@ -118,6 +124,7 @@ struct intel_gt { struct intel_engine_cs *engine[I915_NUM_ENGINES]; struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1] [MAX_ENGINE_INSTANCE + 1];

• enum intel_submission_method submission_method;

  /*

  • Default address space (either GGTT or ppGTT depending on arch).

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index a377c4588aaa..d5094be6d90f 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -1118,7 +1118,6 @@ static int intel_gt_reset_engine(struct intel_engine_cs *engine) int __intel_engine_reset_bh(struct intel_engine_cs *engine, const char *msg) { struct intel_gt *gt = engine->gt;

• bool uses_guc = intel_engine_in_guc_submission_mode(engine); int ret;

  ENGINE_TRACE(engine, "flags=%lx\n", gt->reset.flags);

@@ -1134,10 +1133,10 @@ int __intel_engine_reset_bh(struct intel_engine_cs *engine, const char *msg) "Resetting %s for %s\n", engine->name, msg); atomic_inc(&engine->i915->gpu_error.reset_engine_count[engine->uabi_class]);

• if (!uses_guc)

• ret = intel_gt_reset_engine(engine);
  

• else

• if (intel_engine_uses_guc(engine)) ret = intel_guc_reset_engine(&engine->gt->uc.guc, engine);
• else

• ret = intel_gt_reset_engine(engine);
  

  if (ret) { /* If we fail here, we expect to fallback to a global reset */ ENGINE_TRACE(engine, "Failed to reset, err: %d\n", ret);

diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c index 1081cd36a2bd..1f93591a8c69 100644 --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c @@ -4716,7 +4716,7 @@ int intel_execlists_live_selftests(struct drm_i915_private *i915) SUBTEST(live_virtual_reset), };

• if (!HAS_EXECLISTS(i915))

• if (i915->gt.submission_method != INTEL_SUBMISSION_ELSP) return 0;

  if (intel_gt_is_wedged(&i915->gt))

diff --git a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c index 99609271c3a7..c12e74171b63 100644 --- a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c +++ b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c @@ -291,7 +291,7 @@ int intel_ring_submission_live_selftests(struct drm_i915_private *i915) SUBTEST(live_ctx_switch_wa), };

• if (HAS_EXECLISTS(i915))

• if (i915->gt.submission_method > INTEL_SUBMISSION_RING) return 0;

  return intel_gt_live_subtests(tests, &i915->gt);

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 f72faa0b8339..17b551a0c89f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -745,8 +745,3 @@ void intel_guc_submission_init_early(struct intel_guc *guc) { guc->submission_selected = __guc_submission_selected(guc); }

-bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine) -{

• return engine->set_default_submission == guc_set_default_submission;

-} diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h index 5f7b9e6347d0..3f7005018939 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h @@ -20,7 +20,6 @@ void intel_guc_submission_fini(struct intel_guc *guc); int intel_guc_preempt_work_create(struct intel_guc *guc); void intel_guc_preempt_work_destroy(struct intel_guc *guc); int intel_guc_submission_setup(struct intel_engine_cs *engine); -bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine);

static inline bool intel_guc_submission_is_supported(struct intel_guc *guc) { diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 85ad62dbabfa..66f1f25119b5 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1257,11 +1257,7 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) case 8: case 9: case 10:

• if (intel_engine_in_execlists_submission_mode(ce->engine)) {
  

• 	stream->specific_ctx_id_mask =
  

• 		(1U << GEN8_CTX_ID_WIDTH) - 1;
  

• 	stream->specific_ctx_id = stream->specific_ctx_id_mask;
  

• } else {
  

• if (intel_engine_uses_guc(ce->engine)) {
  /*
   * When using GuC, the context descriptor we write in
   * i915 is read by GuC and rewritten before it's
  

@@ -1280,6 +1276,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) */ stream->specific_ctx_id_mask = (1U << (GEN8_CTX_ID_WIDTH - 1)) - 1;

• } else {
  

• 	stream->specific_ctx_id_mask =
  

• 		(1U << GEN8_CTX_ID_WIDTH) - 1;
  

• 	stream->specific_ctx_id = stream->specific_ctx_id_mask;
  

  } break;

-- 2.28.0

On 06/05/2021 20:13, Matthew Brost wrote:

From: Chris Wilson chris@chris-wilson.co.uk

The different submission backends each have their own preferred behaviour and interrupt setup. Let each handle their own interrupts.

This becomes more useful later as we to extract the use of auxiliary state in the interrupt handler that is backend specific.

Signed-off-by: Matthew Brost matthew.brost@intel.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com

Same, this patch had my r-b already so I'll repeat it:

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

---

drivers/gpu/drm/i915/gt/intel_engine_cs.c | 7 ++ drivers/gpu/drm/i915/gt/intel_engine_types.h | 14 +--- .../drm/i915/gt/intel_execlists_submission.c | 41 ++++++++++ drivers/gpu/drm/i915/gt/intel_gt_irq.c | 82 ++++++------------- drivers/gpu/drm/i915/gt/intel_gt_irq.h | 23 ++++++ .../gpu/drm/i915/gt/intel_ring_submission.c | 8 ++ drivers/gpu/drm/i915/gt/intel_rps.c | 2 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++- drivers/gpu/drm/i915/i915_irq.c | 10 ++- 9 files changed, 124 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 0618379b68ca..828e1669f92c 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -255,6 +255,11 @@ static void intel_engine_sanitize_mmio(struct intel_engine_cs *engine) intel_engine_set_hwsp_writemask(engine, ~0u); }

+static void nop_irq_handler(struct intel_engine_cs *engine, u16 iir) +{

• GEM_DEBUG_WARN_ON(iir);

+}

• static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) { const struct engine_info *info = &intel_engines[id];

@@ -292,6 +297,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->hw_id = info->hw_id; engine->guc_id = MAKE_GUC_ID(info->class, info->instance);

• engine->irq_handler = nop_irq_handler;
• engine->class = info->class; engine->instance = info->instance; __sprint_engine_name(engine);

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 883bafc44902..9ef349cd5cea 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -402,6 +402,7 @@ struct intel_engine_cs { u32 irq_enable_mask; /* bitmask to enable ring interrupt */ void (*irq_enable)(struct intel_engine_cs *engine); void (*irq_disable)(struct intel_engine_cs *engine);

• void (*irq_handler)(struct intel_engine_cs *engine, u16 iir);

  void (*sanitize)(struct intel_engine_cs *engine); int (*resume)(struct intel_engine_cs *engine);

@@ -481,10 +482,9 @@ struct intel_engine_cs { #define I915_ENGINE_HAS_PREEMPTION BIT(2) #define I915_ENGINE_HAS_SEMAPHORES BIT(3) #define I915_ENGINE_HAS_TIMESLICES BIT(4) -#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(5) -#define I915_ENGINE_IS_VIRTUAL BIT(6) -#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(7) -#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(8) +#define I915_ENGINE_IS_VIRTUAL BIT(5) +#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(6) +#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(7) unsigned int flags;

/* @@ -593,12 +593,6 @@ intel_engine_has_timeslices(const struct intel_engine_cs *engine) return engine->flags & I915_ENGINE_HAS_TIMESLICES; }

-static inline bool -intel_engine_needs_breadcrumb_tasklet(const struct intel_engine_cs *engine) -{

• return engine->flags & I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;

-}

• static inline bool intel_engine_is_virtual(const struct intel_engine_cs *engine) {

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 9d2da5ccaef6..8db200422950 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -118,6 +118,7 @@ #include "intel_engine_stats.h" #include "intel_execlists_submission.h" #include "intel_gt.h" +#include "intel_gt_irq.h" #include "intel_gt_pm.h" #include "intel_gt_requests.h" #include "intel_lrc.h" @@ -2384,6 +2385,45 @@ static void execlists_submission_tasklet(struct tasklet_struct *t) rcu_read_unlock(); }

+static void execlists_irq_handler(struct intel_engine_cs *engine, u16 iir) +{

• bool tasklet = false;
• if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {

• u32 eir;
  

• /* Upper 16b are the enabling mask, rsvd for internal errors */
  

• eir = ENGINE_READ(engine, RING_EIR) & GENMASK(15, 0);
  

• ENGINE_TRACE(engine, "CS error: %x\n", eir);
  

• /* Disable the error interrupt until after the reset */
  

• if (likely(eir)) {
  

• 	ENGINE_WRITE(engine, RING_EMR, ~0u);
  

• 	ENGINE_WRITE(engine, RING_EIR, eir);
  

• 	WRITE_ONCE(engine->execlists.error_interrupt, eir);
  

• 	tasklet = true;
  

• }
  

• }
• if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {

• WRITE_ONCE(engine->execlists.yield,
  

• 	   ENGINE_READ_FW(engine, RING_EXECLIST_STATUS_HI));
  

• ENGINE_TRACE(engine, "semaphore yield: %08x\n",
  

• 	     engine->execlists.yield);
  

• if (del_timer(&engine->execlists.timer))
  

• 	tasklet = true;
  

• }
• if (iir & GT_CONTEXT_SWITCH_INTERRUPT)

• tasklet = true;
  

• if (iir & GT_RENDER_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(engine);
  

• if (tasklet)

• tasklet_hi_schedule(&engine->execlists.tasklet);
  

+}

• static void __execlists_kick(struct intel_engine_execlists *execlists) { /* Kick the tasklet for some interrupt coalescing and reset handling */

@@ -3133,6 +3173,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) * until a more refined solution exists. */ }

• intel_engine_set_irq_handler(engine, execlists_irq_handler);

  engine->flags |= I915_ENGINE_SUPPORTS_STATS; if (!intel_vgpu_active(engine->i915)) {

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c index 9fc6c912a4e5..d29126c458ba 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c @@ -20,48 +20,6 @@ static void guc_irq_handler(struct intel_guc *guc, u16 iir) intel_guc_to_host_event_handler(guc); }

-static void -cs_irq_handler(struct intel_engine_cs *engine, u32 iir) -{

• bool tasklet = false;
• if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {

• u32 eir;
  

• /* Upper 16b are the enabling mask, rsvd for internal errors */
  

• eir = ENGINE_READ(engine, RING_EIR) & GENMASK(15, 0);
  

• ENGINE_TRACE(engine, "CS error: %x\n", eir);
  

• /* Disable the error interrupt until after the reset */
  

• if (likely(eir)) {
  

• 	ENGINE_WRITE(engine, RING_EMR, ~0u);
  

• 	ENGINE_WRITE(engine, RING_EIR, eir);
  

• 	WRITE_ONCE(engine->execlists.error_interrupt, eir);
  

• 	tasklet = true;
  

• }
  

• }
• if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {

• WRITE_ONCE(engine->execlists.yield,
  

• 	   ENGINE_READ_FW(engine, RING_EXECLIST_STATUS_HI));
  

• ENGINE_TRACE(engine, "semaphore yield: %08x\n",
  

• 	     engine->execlists.yield);
  

• if (del_timer(&engine->execlists.timer))
  

• 	tasklet = true;
  

• }
• if (iir & GT_CONTEXT_SWITCH_INTERRUPT)

• tasklet = true;
  

• if (iir & GT_RENDER_USER_INTERRUPT) {

• intel_engine_signal_breadcrumbs(engine);
  

• tasklet |= intel_engine_needs_breadcrumb_tasklet(engine);
  

• }
• if (tasklet)

• tasklet_hi_schedule(&engine->execlists.tasklet);
  

-}

• static u32 gen11_gt_engine_identity(struct intel_gt *gt, const unsigned int bank, const unsigned int bit)

@@ -122,7 +80,7 @@ gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, engine = NULL;

if (likely(engine))

• return cs_irq_handler(engine, iir);
  

• return intel_engine_cs_irq(engine, iir);
  
  

  WARN_ONCE(1, "unhandled engine interrupt class=0x%x, instance=0x%x\n", class, instance);

@@ -275,9 +233,12 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) void gen5_gt_irq_handler(struct intel_gt *gt, u32 gt_iir) { if (gt_iir & GT_RENDER_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(gt->engine_class[RENDER_CLASS][0]);
  

• intel_engine_cs_irq(gt->engine_class[RENDER_CLASS][0],
  

• 		    gt_iir);
  

• if (gt_iir & ILK_BSD_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(gt->engine_class[VIDEO_DECODE_CLASS][0]);
  

• intel_engine_cs_irq(gt->engine_class[VIDEO_DECODE_CLASS][0],
  

• 		    gt_iir);
  

  }

  static void gen7_parity_error_irq_handler(struct intel_gt *gt, u32 iir)

@@ -301,11 +262,16 @@ static void gen7_parity_error_irq_handler(struct intel_gt *gt, u32 iir) void gen6_gt_irq_handler(struct intel_gt *gt, u32 gt_iir) { if (gt_iir & GT_RENDER_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(gt->engine_class[RENDER_CLASS][0]);
  

• intel_engine_cs_irq(gt->engine_class[RENDER_CLASS][0],
  

• 		    gt_iir);
  

• if (gt_iir & GT_BSD_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(gt->engine_class[VIDEO_DECODE_CLASS][0]);
  

• intel_engine_cs_irq(gt->engine_class[VIDEO_DECODE_CLASS][0],
  

• 		    gt_iir >> 12);
  

• if (gt_iir & GT_BLT_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(gt->engine_class[COPY_ENGINE_CLASS][0]);
  

• intel_engine_cs_irq(gt->engine_class[COPY_ENGINE_CLASS][0],
  

• 		    gt_iir >> 22);
  
  

  if (gt_iir & (GT_BLT_CS_ERROR_INTERRUPT | GT_BSD_CS_ERROR_INTERRUPT |

@@ -324,10 +290,10 @@ void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl) if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) { iir = raw_reg_read(regs, GEN8_GT_IIR(0)); if (likely(iir)) {

• 	cs_irq_handler(gt->engine_class[RENDER_CLASS][0],
  

• 		       iir >> GEN8_RCS_IRQ_SHIFT);
  

• 	cs_irq_handler(gt->engine_class[COPY_ENGINE_CLASS][0],
  

• 		       iir >> GEN8_BCS_IRQ_SHIFT);
  

• 	intel_engine_cs_irq(gt->engine_class[RENDER_CLASS][0],
  

• 			    iir >> GEN8_RCS_IRQ_SHIFT);
  

• 	intel_engine_cs_irq(gt->engine_class[COPY_ENGINE_CLASS][0],
  

• 			    iir >> GEN8_BCS_IRQ_SHIFT);
  raw_reg_write(regs, GEN8_GT_IIR(0), iir);
  

  } }

@@ -335,10 +301,10 @@ void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl) if (master_ctl & (GEN8_GT_VCS0_IRQ | GEN8_GT_VCS1_IRQ)) { iir = raw_reg_read(regs, GEN8_GT_IIR(1)); if (likely(iir)) {

• 	cs_irq_handler(gt->engine_class[VIDEO_DECODE_CLASS][0],
  

• 		       iir >> GEN8_VCS0_IRQ_SHIFT);
  

• 	cs_irq_handler(gt->engine_class[VIDEO_DECODE_CLASS][1],
  

• 		       iir >> GEN8_VCS1_IRQ_SHIFT);
  

• 	intel_engine_cs_irq(gt->engine_class[VIDEO_DECODE_CLASS][0],
  

• 			    iir >> GEN8_VCS0_IRQ_SHIFT);
  

• 	intel_engine_cs_irq(gt->engine_class[VIDEO_DECODE_CLASS][1],
  

• 			    iir >> GEN8_VCS1_IRQ_SHIFT);
  raw_reg_write(regs, GEN8_GT_IIR(1), iir);
  

  } }

@@ -346,8 +312,8 @@ void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl) if (master_ctl & GEN8_GT_VECS_IRQ) { iir = raw_reg_read(regs, GEN8_GT_IIR(3)); if (likely(iir)) {

• 	cs_irq_handler(gt->engine_class[VIDEO_ENHANCEMENT_CLASS][0],
  

• 		       iir >> GEN8_VECS_IRQ_SHIFT);
  

• 	intel_engine_cs_irq(gt->engine_class[VIDEO_ENHANCEMENT_CLASS][0],
  

• 			    iir >> GEN8_VECS_IRQ_SHIFT);
  raw_reg_write(regs, GEN8_GT_IIR(3), iir);
  

  } }

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.h b/drivers/gpu/drm/i915/gt/intel_gt_irq.h index f667e976fb2b..41cad38668c5 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.h @@ -8,6 +8,8 @@

#include <linux/types.h>

+#include "intel_engine_types.h"

• struct intel_gt;

  #define GEN8_GT_IRQS (GEN8_GT_RCS_IRQ | \

@@ -39,4 +41,25 @@ void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl); void gen8_gt_irq_reset(struct intel_gt *gt); void gen8_gt_irq_postinstall(struct intel_gt *gt);

+static inline void intel_engine_cs_irq(struct intel_engine_cs *engine, u16 iir) +{

• if (iir)

• engine->irq_handler(engine, iir);
  

+}

+static inline void +intel_engine_set_irq_handler(struct intel_engine_cs *engine,

• 	     void (*fn)(struct intel_engine_cs *engine,
  

• 			u16 iir))
  

+{

• /*

• * As the interrupt is live as allocate and setup the engines,
  

• * err on the side of caution and apply barriers to updating
  

• * the irq handler callback. This assures that when we do use
  

• * the engine, we will receive interrupts only to ourselves,
  

• * and not lose any.
  

• */
  

• smp_store_mb(engine->irq_handler, fn);

+}

• #endif /* INTEL_GT_IRQ_H */

diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c index 5f4f7f1df48f..2b6dffcc2262 100644 --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c @@ -12,6 +12,7 @@ #include "intel_breadcrumbs.h" #include "intel_context.h" #include "intel_gt.h" +#include "intel_gt_irq.h" #include "intel_reset.h" #include "intel_ring.h" #include "shmem_utils.h" @@ -1017,10 +1018,17 @@ static void ring_release(struct intel_engine_cs *engine) intel_timeline_put(engine->legacy.timeline); }

+static void irq_handler(struct intel_engine_cs *engine, u16 iir) +{

• intel_engine_signal_breadcrumbs(engine);

+}

• static void setup_irq(struct intel_engine_cs *engine) { struct drm_i915_private *i915 = engine->i915;

• intel_engine_set_irq_handler(engine, irq_handler);

• if (INTEL_GEN(i915) >= 6) { engine->irq_enable = gen6_irq_enable; engine->irq_disable = gen6_irq_disable;

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 405d814e9040..97cab1b99871 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -1774,7 +1774,7 @@ void gen6_rps_irq_handler(struct intel_rps *rps, u32 pm_iir) return;

if (pm_iir & PM_VEBOX_USER_INTERRUPT)

• intel_engine_signal_breadcrumbs(gt->engine[VECS0]);
  

• intel_engine_cs_irq(gt->engine[VECS0], pm_iir >> 10);
  
  

  if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) DRM_DEBUG("Command parser error, pm_iir 0x%08x\n", pm_iir);

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 17b551a0c89f..335719f17490 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -11,6 +11,7 @@ #include "gt/intel_context.h" #include "gt/intel_engine_pm.h" #include "gt/intel_gt.h" +#include "gt/intel_gt_irq.h" #include "gt/intel_gt_pm.h" #include "gt/intel_lrc.h" #include "gt/intel_mocs.h" @@ -264,6 +265,14 @@ static void guc_submission_tasklet(struct tasklet_struct *t) spin_unlock_irqrestore(&engine->active.lock, flags); }

+static void cs_irq_handler(struct intel_engine_cs *engine, u16 iir) +{

• if (iir & GT_RENDER_USER_INTERRUPT) {

• intel_engine_signal_breadcrumbs(engine);
  

• tasklet_hi_schedule(&engine->execlists.tasklet);
  

• }

+}

• static void guc_reset_prepare(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists;

@@ -645,7 +654,6 @@ static void guc_default_vfuncs(struct intel_engine_cs *engine) } engine->set_default_submission = guc_set_default_submission;

• engine->flags |= I915_ENGINE_NEEDS_BREADCRUMB_TASKLET; engine->flags |= I915_ENGINE_HAS_PREEMPTION;

  /*

@@ -681,6 +689,7 @@ static void rcs_submission_override(struct intel_engine_cs *engine) static inline void guc_default_irqs(struct intel_engine_cs *engine) { engine->irq_keep_mask = GT_RENDER_USER_INTERRUPT;

• intel_engine_set_irq_handler(engine, cs_irq_handler); }

  int intel_guc_submission_setup(struct intel_engine_cs *engine)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index f6967a93ec7a..d58118806299 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4014,7 +4014,7 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg) intel_uncore_write16(&dev_priv->uncore, GEN2_IIR, iir);

if (iir & I915_USER_INTERRUPT)

• 	intel_engine_signal_breadcrumbs(dev_priv->gt.engine[RCS0]);
  

• 	intel_engine_cs_irq(dev_priv->gt.engine[RCS0], iir);
  
  

  if (iir & I915_MASTER_ERROR_INTERRUPT) i8xx_error_irq_handler(dev_priv, eir, eir_stuck);

@@ -4122,7 +4122,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg) intel_uncore_write(&dev_priv->uncore, GEN2_IIR, iir);

if (iir & I915_USER_INTERRUPT)

• 	intel_engine_signal_breadcrumbs(dev_priv->gt.engine[RCS0]);
  

• 	intel_engine_cs_irq(dev_priv->gt.engine[RCS0], iir);
  
  

  if (iir & I915_MASTER_ERROR_INTERRUPT) i9xx_error_irq_handler(dev_priv, eir, eir_stuck);

@@ -4267,10 +4267,12 @@ static irqreturn_t i965_irq_handler(int irq, void *arg) intel_uncore_write(&dev_priv->uncore, GEN2_IIR, iir);

if (iir & I915_USER_INTERRUPT)

• 	intel_engine_signal_breadcrumbs(dev_priv->gt.engine[RCS0]);
  

• 	intel_engine_cs_irq(dev_priv->gt.engine[RCS0],
  

• 			    iir);
  
  

  if (iir & I915_BSD_USER_INTERRUPT)

• 	intel_engine_signal_breadcrumbs(dev_priv->gt.engine[VCS0]);
  

• 	intel_engine_cs_irq(dev_priv->gt.engine[VCS0],
  

• 			    iir >> 25);
  
  

  if (iir & I915_MASTER_ERROR_INTERRUPT) i9xx_error_irq_handler(dev_priv, eir, eir_stuck);

On Thu, May 06, 2021 at 12:13:18PM -0700, Matthew Brost wrote:

From: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

If we're about to sanitize the GuC, something might have going wrong beforehand, so we should avoid trying to talk to it. Even if GuC is still running fine, the sanitize will reset its internal state and clear the CTB registration, so there is still no need to explicitly do so.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/2469 Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Michal Wajdeczko michal.wajdeczko@intel.com Cc: John Harrison John.C.Harrison@Intel.com

Reviewed-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 6abb8f2dc33d..892c1315ce49 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -504,7 +504,7 @@ static int __uc_init_hw(struct intel_uc *uc)

ret = intel_guc_sample_forcewake(guc); if (ret)

• goto err_communication;
  

• goto err_log_capture;
  
  

  if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_enable(guc);

@@ -529,8 +529,6 @@ static int __uc_init_hw(struct intel_uc *uc) /* * We've failed to load the firmware :( */ -err_communication:

• guc_disable_communication(guc);

err_log_capture: __uc_capture_load_err_log(uc); err_out: @@ -558,9 +556,6 @@ static void __uc_fini_hw(struct intel_uc *uc) if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_disable(guc);

• if (guc_communication_enabled(guc))

• guc_disable_communication(guc);
  

• __uc_sanitize(uc);

}

@@ -577,7 +572,6 @@ void intel_uc_reset_prepare(struct intel_uc *uc) if (!intel_guc_is_ready(guc)) return;

• guc_disable_communication(guc); __uc_sanitize(uc);

}

-- 2.28.0

On 06.05.2021 21:13, Matthew Brost wrote:

From: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

We have a couple of failure injection points in the CT enablement path, so we need to use i915_probe_error() to select the appropriate log level. A new macro (CT_PROBE_ERROR) has been added to the set of CT logging macros to be used in this scenario and upcoming ones.

While adding the new macros, fix the underlying logging mechanics used by the existing ones (DRM_DEV_* -> drm_*) and move the inlines to before they're used inside the macros.

Signed-off-by: Matthew Brost matthew.brost@intel.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 48 ++++++++++++----------- 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index fa9e048cc65f..25618649048f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -7,14 +7,36 @@ #include "intel_guc_ct.h" #include "gt/intel_gt.h"

+static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct) +{

• return container_of(ct, struct intel_guc, ct);

+}

+static inline struct intel_gt *ct_to_gt(struct intel_guc_ct *ct) +{

• return guc_to_gt(ct_to_guc(ct));

+}

+static inline struct drm_i915_private *ct_to_i915(struct intel_guc_ct *ct) +{

• return ct_to_gt(ct)->i915;

+}

+static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct) +{

• return &ct_to_i915(ct)->drm;

+}

#define CT_ERROR(_ct, _fmt, ...) \

• DRM_DEV_ERROR(ct_to_dev(_ct), "CT: " _fmt, ##__VA_ARGS__)

• drm_err(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)

#ifdef CONFIG_DRM_I915_DEBUG_GUC #define CT_DEBUG(_ct, _fmt, ...) \

• DRM_DEV_DEBUG_DRIVER(ct_to_dev(_ct), "CT: " _fmt, ##__VA_ARGS__)

• drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)

#else #define CT_DEBUG(...) do { } while (0) #endif +#define CT_PROBE_ERROR(_ct, _fmt, ...) \

• i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__);

struct ct_request { struct list_head link; @@ -47,26 +69,6 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct) INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func); }

-static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct) -{

• return container_of(ct, struct intel_guc, ct);

-}

-static inline struct intel_gt *ct_to_gt(struct intel_guc_ct *ct) -{

• return guc_to_gt(ct_to_guc(ct));

-}

-static inline struct drm_i915_private *ct_to_i915(struct intel_guc_ct *ct) -{

• return ct_to_gt(ct)->i915;

-}

-static inline struct device *ct_to_dev(struct intel_guc_ct *ct) -{

• return ct_to_i915(ct)->drm.dev;

-}

static inline const char *guc_ct_buffer_type_to_str(u32 type) { switch (type) { @@ -264,7 +266,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) err_deregister: ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); err_out:

• CT_ERROR(ct, "Failed to open open CT channel (err=%d)\n", err);

• CT_PROBE_ERROR(ct, "Failed to open channel (err=%d)\n", err);

nit: while here we can start using %pe to print error

Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com

return err; }

On Thu, May 06, 2021 at 12:13:20PM -0700, Matthew Brost wrote:

From: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

In GuC submission mode the CS is owned by the GuC FW, so all CS status interrupts are handled by it. We only need the user interrupt as that signals request completion.

Since we're now starting the engines directly in GuC submission mode when selected, we can stop switching back and forth between the execlists and the GuC programming and select directly the correct interrupt mask.

Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: Matthew Brost matthew.brost@intel.com

Cc: John Harrison john.c.harrison@intel.com Cc: Michal Wajdeczko michal.wajdeczko@intel.com

---

drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- 2 files changed, 11 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c index d29126c458ba..f88c10366e58 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c @@ -194,14 +194,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt)

void gen11_gt_irq_postinstall(struct intel_gt *gt) {

• const u32 irqs =

• GT_CS_MASTER_ERROR_INTERRUPT |
  

• GT_RENDER_USER_INTERRUPT |
  

• GT_CONTEXT_SWITCH_INTERRUPT |
  

• GT_WAIT_SEMAPHORE_INTERRUPT;
  

  struct intel_uncore *uncore = gt->uncore;
• const u32 dmask = irqs << 16 | irqs;
• const u32 smask = irqs << 16;

• u32 irqs = GT_RENDER_USER_INTERRUPT;

• u32 dmask;

• u32 smask;

• if (!intel_uc_wants_guc_submission(&gt->uc))

• irqs |= GT_CS_MASTER_ERROR_INTERRUPT |
  

• 	GT_CONTEXT_SWITCH_INTERRUPT |
  

• 	GT_WAIT_SEMAPHORE_INTERRUPT;
  

• dmask = irqs << 16 | irqs;

• smask = irqs << 16;

  BUILD_BUG_ON(irqs & 0xffff0000);

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 335719f17490..38cda5d599a6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -432,32 +432,6 @@ void intel_guc_submission_fini(struct intel_guc *guc) } }

-static void guc_interrupts_capture(struct intel_gt *gt) -{

• struct intel_uncore *uncore = gt->uncore;
• u32 irqs = GT_CONTEXT_SWITCH_INTERRUPT;
• u32 dmask = irqs << 16 | irqs;
• GEM_BUG_ON(INTEL_GEN(gt->i915) < 11);
• /* Don't handle the ctx switch interrupt in GuC submission mode */
• intel_uncore_rmw(uncore, GEN11_RENDER_COPY_INTR_ENABLE, dmask, 0);
• intel_uncore_rmw(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask, 0);

-}

-static void guc_interrupts_release(struct intel_gt *gt) -{

• struct intel_uncore *uncore = gt->uncore;
• u32 irqs = GT_CONTEXT_SWITCH_INTERRUPT;
• u32 dmask = irqs << 16 | irqs;
• GEM_BUG_ON(INTEL_GEN(gt->i915) < 11);
• /* Handle ctx switch interrupts again */
• intel_uncore_rmw(uncore, GEN11_RENDER_COPY_INTR_ENABLE, 0, dmask);
• intel_uncore_rmw(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0, dmask);

-}

static int guc_context_alloc(struct intel_context *ce) { return lrc_alloc(ce, ce->engine); @@ -722,9 +696,6 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine) void intel_guc_submission_enable(struct intel_guc *guc) { guc_stage_desc_init(guc);

• /* Take over from manual control of ELSP (execlists) */
• guc_interrupts_capture(guc_to_gt(guc));

}

void intel_guc_submission_disable(struct intel_guc *guc) @@ -735,8 +706,6 @@ void intel_guc_submission_disable(struct intel_guc *guc)

/* Note: By the time we're here, GuC may have already been reset */

• guc_interrupts_release(gt);
• guc_stage_desc_fini(guc);

}

-- 2.28.0

On 06.05.2021 21:13, Matthew Brost wrote:

From: Rodrigo Vivi rodrigo.vivi@intel.com

This action is no-op in the GuC side for a few versions already and it is getting entirely removed soon, in an upcoming version.

Time to remove before we face communication issues.

Cc: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

Acked-by: Michal Wajdeczko michal.wajdeczko@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.c | 16 ---------------- drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 - drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 4 ---- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 4 ---- 4 files changed, 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index adae04c47aab..ab2c8fe8cdfa 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -469,22 +469,6 @@ int intel_guc_to_host_process_recv_msg(struct intel_guc *guc, return 0; }

-int intel_guc_sample_forcewake(struct intel_guc *guc) -{

• struct drm_i915_private *dev_priv = guc_to_gt(guc)->i915;
• u32 action[2];
• action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
• /* WaRsDisableCoarsePowerGating:skl,cnl */
• if (!HAS_RC6(dev_priv) || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))

• action[1] = 0;
  

• else

• /* bit 0 and 1 are for Render and Media domain separately */
  

• action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
  

• return intel_guc_send(guc, action, ARRAY_SIZE(action));

-}

/**

• intel_guc_auth_huc() - Send action to GuC to authenticate HuC ucode
• @guc: intel_guc structure

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index bc2ba7d0626c..c20f3839de12 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -128,7 +128,6 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len, u32 *response_buf, u32 response_buf_size); int intel_guc_to_host_process_recv_msg(struct intel_guc *guc, const u32 *payload, u32 len); -int intel_guc_sample_forcewake(struct intel_guc *guc); int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset); int intel_guc_suspend(struct intel_guc *guc); int intel_guc_resume(struct intel_guc *guc); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h index 79c560d9c0b6..0f9afcde1d0b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h @@ -302,9 +302,6 @@ struct guc_ct_buffer_desc { #define GUC_CT_MSG_ACTION_SHIFT 16 #define GUC_CT_MSG_ACTION_MASK 0xFFFF

-#define GUC_FORCEWAKE_RENDER (1 << 0) -#define GUC_FORCEWAKE_MEDIA (1 << 1)

#define GUC_POWER_UNSPECIFIED 0 #define GUC_POWER_D0 1 #define GUC_POWER_D1 2 @@ -558,7 +555,6 @@ enum intel_guc_action { INTEL_GUC_ACTION_ENTER_S_STATE = 0x501, INTEL_GUC_ACTION_EXIT_S_STATE = 0x502, INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003,

• INTEL_GUC_ACTION_SAMPLE_FORCEWAKE = 0x3005, INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000, INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505, INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 892c1315ce49..ab0789d66e06 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -502,10 +502,6 @@ static int __uc_init_hw(struct intel_uc *uc)

intel_huc_auth(huc);

• ret = intel_guc_sample_forcewake(guc);
• if (ret)

• goto err_log_capture;
  

• if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_enable(guc);

On 25.05.2021 04:47, Matthew Brost wrote:

On Thu, May 06, 2021 at 12:13:25PM -0700, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

In upcoming GuC firmware, CTB size will be removed from the CTB descriptor so we must keep it locally for any calculations.

While around, improve some debug messages and helpers.

desc->size is still used in the patch and really shouldn't be per this

goal of this patch was to start using local ctb->size for any calculations, not to drop it completely, as at this point (GuC 49) desc->size is still part of the CTB protocol

comment but a patch later in the series drops it. Seeing as this patch and that patch are going to squashed into a single patch upgrading the GuC firmware I think that is ok.

no need to squash this patch

in fact all CTB patches up to and including 20/97 ("drm/i915/guc: Introduce unified HXG messages") are compatible with current GuC 49.0.1 and IMHO *should* be merged separately

only patches from 21/97 ("drm/i915/guc: Update MMIO based communication") to 29/97 ("drm/i915/guc: Update firmware to v60.1.2") must be squashed during merge, as this is were communication protocol is changing (from GuC 49 to GuC 60) and we may break existing functionality (HuC authentication)

Michal

With that: Reviewed-by: Matthew Brost matthew.brost@intel.com

...

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 55 +++++++++++++++++------ drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 2 + 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 4cc8c0b71699..dbece569fbe4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -90,6 +90,24 @@ static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc, desc->owner = CTB_OWNER_HOST; }

+static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb, u32 cmds_addr) +{

• guc_ct_buffer_desc_init(ctb->desc, cmds_addr, ctb->size);

+}

+static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,

• 	       struct guc_ct_buffer_desc *desc,
  

• 	       u32 *cmds, u32 size)
  

+{

• GEM_BUG_ON(size % 4);
• ctb->desc = desc;
• ctb->cmds = cmds;
• ctb->size = size;
• guc_ct_buffer_reset(ctb, 0);

+}

static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 desc_addr, u32 type) @@ -148,7 +166,10 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int intel_guc_ct_init(struct intel_guc_ct *ct) { struct intel_guc *guc = ct_to_guc(ct);

• struct guc_ct_buffer_desc *desc;
• u32 blob_size; void *blob;
• u32 *cmds; int err; int i;

@@ -176,19 +197,24 @@ int intel_guc_ct_init(struct intel_guc_ct *ct) * other code will need updating as well. */

• err = intel_guc_allocate_and_map_vma(guc, PAGE_SIZE, &ct->vma, &blob);

• blob_size = PAGE_SIZE;
• err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob); if (unlikely(err)) {

• CT_ERROR(ct, "Failed to allocate CT channel (err=%d)\n", err);
  

• CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
  

• 	       blob_size, ERR_PTR(err));
  

  return err; }

• CT_DEBUG(ct, "vma base=%#x\n", intel_guc_ggtt_offset(guc, ct->vma));

• CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma), blob_size);

  /* store pointers to desc and cmds */ for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) { GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));

• ct->ctbs[i].desc = blob + PAGE_SIZE/4 * i;
  

• ct->ctbs[i].cmds = blob + PAGE_SIZE/4 * i + PAGE_SIZE/2;
  

• desc = blob + PAGE_SIZE / 4 * i;
  

• cmds = blob + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
  

• guc_ct_buffer_init(&ct->ctbs[i], desc, cmds, PAGE_SIZE / 4);
  

  }

  return 0;

@@ -217,7 +243,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct) int intel_guc_ct_enable(struct intel_guc_ct *ct) { struct intel_guc *guc = ct_to_guc(ct);

• u32 base, cmds, size;

• u32 base, cmds; int err; int i;

@@ -232,10 +258,11 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) */ for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) { GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));

• cmds = base + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;

• size = PAGE_SIZE / 4;
  

• CT_DEBUG(ct, "%d: addr=%#x size=%u\n", i, cmds, size);
  

• guc_ct_buffer_desc_init(ct->ctbs[i].desc, cmds, size);
  

• CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
  

• guc_ct_buffer_reset(&ct->ctbs[i], cmds);
  

  }

  /*

@@ -259,7 +286,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) err_deregister: ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); err_out:

• CT_PROBE_ERROR(ct, "Failed to open channel (err=%d)\n", err);

• CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err)); return err;

}

@@ -314,7 +341,7 @@ static int ct_write(struct intel_guc_ct *ct, struct guc_ct_buffer_desc *desc = ctb->desc; u32 head = desc->head; u32 tail = desc->tail;

• u32 size = desc->size;

• u32 size = ctb->size; u32 used; u32 header; u32 *cmds = ctb->cmds;

@@ -323,7 +350,7 @@ static int ct_write(struct intel_guc_ct *ct, if (unlikely(desc->is_in_error)) return -EPIPE;

• if (unlikely(!IS_ALIGNED(head | tail | size, 4) ||

• if (unlikely(!IS_ALIGNED(head | tail, 4) || (tail | head) >= size)) goto corrupted;

@@ -530,7 +557,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data) struct guc_ct_buffer_desc *desc = ctb->desc; u32 head = desc->head; u32 tail = desc->tail;

• u32 size = desc->size;

• u32 size = ctb->size; u32 *cmds = ctb->cmds; s32 available; unsigned int len;

@@ -539,7 +566,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data) if (unlikely(desc->is_in_error)) return -EPIPE;

• if (unlikely(!IS_ALIGNED(head | tail | size, 4) ||

• if (unlikely(!IS_ALIGNED(head | tail, 4) || (tail | head) >= size)) goto corrupted;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h index 494a51a5200f..4009e2dd0de4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -29,10 +29,12 @@ struct intel_guc;

• @desc: pointer to the buffer descriptor
• @cmds: pointer to the commands buffer

• • @size: size of the commands buffer
  */

struct intel_guc_ct_buffer { struct guc_ct_buffer_desc *desc; u32 *cmds;

• u32 size;

};

-- 2.28.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Thu, May 06, 2021 at 12:13:26PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

We can retrieve offsets to cmds buffers and descriptor from actual pointers that we already keep locally.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index dbece569fbe4..fbd6bd20f588 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -244,6 +244,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) { struct intel_guc *guc = ct_to_guc(ct); u32 base, cmds;

• void *blob; int err; int i;

@@ -251,15 +252,18 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)

/* vma should be already allocated and map'ed */ GEM_BUG_ON(!ct->vma);

• GEM_BUG_ON(!i915_gem_object_has_pinned_pages(ct->vma->obj));

This doesn't really have anything to do with this patch, but again this patch will be squashed into a large patch updating the GuC firmware, so I think this is fine.

With that: Reviewed-by: Matthew Brost matthew.brost@intel.com

base = intel_guc_ggtt_offset(guc, ct->vma);

• /* (re)initialize descriptors

• * cmds buffers are in the second half of the blob page
  

• */
  

• /* blob should start with send descriptor */
• blob = __px_vaddr(ct->vma->obj);
• GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);
• /* (re)initialize descriptors */ for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) { GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));

• cmds = base + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
  

• cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
  

  CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);

  guc_ct_buffer_reset(&ct->ctbs[i], cmds);

@@ -269,12 +273,12 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) * Register both CT buffers starting with RECV buffer. * Descriptors are in first half of the blob. */

• err = ct_register_buffer(ct, base + PAGE_SIZE / 4 * CTB_RECV,

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob), INTEL_GUC_CT_BUFFER_TYPE_RECV); if (unlikely(err)) goto err_out;

• err = ct_register_buffer(ct, base + PAGE_SIZE / 4 * CTB_SEND,

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob), INTEL_GUC_CT_BUFFER_TYPE_SEND); if (unlikely(err)) goto err_deregister;

-- 2.28.0

On Tue, May 25, 2021 at 03:07:06PM +0200, Michal Wajdeczko wrote:

On 25.05.2021 04:53, Matthew Brost wrote:

...

On Thu, May 06, 2021 at 12:13:26PM -0700, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

We can retrieve offsets to cmds buffers and descriptor from actual pointers that we already keep locally.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index dbece569fbe4..fbd6bd20f588 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -244,6 +244,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) { struct intel_guc *guc = ct_to_guc(ct); u32 base, cmds;

• void *blob; int err; int i;

@@ -251,15 +252,18 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)

/* vma should be already allocated and map'ed */ GEM_BUG_ON(!ct->vma);

• GEM_BUG_ON(!i915_gem_object_has_pinned_pages(ct->vma->obj));

This doesn't really have anything to do with this patch, but again this patch will be squashed into a large patch updating the GuC firmware, so I think this is fine.

again, no need to squash GuC patches up to 20/97

Got it. As discussed I will post patches 4-20 after I done reviewing all of them.

Matt

...

With that: Reviewed-by: Matthew Brost matthew.brost@intel.com

...

base = intel_guc_ggtt_offset(guc, ct->vma);

• /* (re)initialize descriptors

• * cmds buffers are in the second half of the blob page
  

• */
  

• /* blob should start with send descriptor */
• blob = __px_vaddr(ct->vma->obj);
• GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);
• /* (re)initialize descriptors */ for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) { GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));

• cmds = base + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
  

• cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
  

  CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);

  guc_ct_buffer_reset(&ct->ctbs[i], cmds);

@@ -269,12 +273,12 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) * Register both CT buffers starting with RECV buffer. * Descriptors are in first half of the blob. */

• err = ct_register_buffer(ct, base + PAGE_SIZE / 4 * CTB_RECV,

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob), INTEL_GUC_CT_BUFFER_TYPE_RECV); if (unlikely(err)) goto err_out;

• err = ct_register_buffer(ct, base + PAGE_SIZE / 4 * CTB_SEND,

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob), INTEL_GUC_CT_BUFFER_TYPE_SEND); if (unlikely(err)) goto err_deregister;

-- 2.28.0

On Thu, May 06, 2021 at 12:13:27PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

Upcoming GuC firmware will always require just two CTBs and we also plan to configure them with different sizes, so definining them as array is no longer suitable.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 46 ++++++++++++----------- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +++- 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index fbd6bd20f588..c54a29176862 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -168,10 +168,10 @@ int intel_guc_ct_init(struct intel_guc_ct *ct) struct intel_guc *guc = ct_to_guc(ct); struct guc_ct_buffer_desc *desc; u32 blob_size;

• u32 cmds_size; void *blob; u32 *cmds; int err;

• int i;

  GEM_BUG_ON(ct->vma);

@@ -207,15 +207,23 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)

CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma), blob_size);

• /* store pointers to desc and cmds */
• for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {

• GEM_BUG_ON((i !=  CTB_SEND) && (i != CTB_RECV));
  

• /* store pointers to desc and cmds for send ctb */
• desc = blob;
• cmds = blob + PAGE_SIZE / 2;
• cmds_size = PAGE_SIZE / 4;
• CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",

•  ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
  
  

• desc = blob + PAGE_SIZE / 4 * i;
  

• cmds = blob + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
  

• guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);

• guc_ct_buffer_init(&ct->ctbs[i], desc, cmds, PAGE_SIZE / 4);
  

• }

• /* store pointers to desc and cmds for recv ctb */

• desc = blob + PAGE_SIZE / 4;

• cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;

• cmds_size = PAGE_SIZE / 4;

• CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",

•  ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
  

• guc_ct_buffer_init(&ct->ctbs.recv, desc, cmds, cmds_size);

  return 0;

} @@ -246,7 +254,6 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) u32 base, cmds; void *blob; int err;

• int i;

  GEM_BUG_ON(ct->enabled);

@@ -257,28 +264,25 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)

/* blob should start with send descriptor */ blob = __px_vaddr(ct->vma->obj);

• GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);

• GEM_BUG_ON(blob != ct->ctbs.send.desc);

  /* (re)initialize descriptors */

• for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {

• GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
  

• cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);
• guc_ct_buffer_reset(&ct->ctbs.send, cmds);

• cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
  

• CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
  

• guc_ct_buffer_reset(&ct->ctbs[i], cmds);
  

• }

• cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);

• guc_ct_buffer_reset(&ct->ctbs.recv, cmds);

  /*

  • Register both CT buffers starting with RECV buffer.
  • Descriptors are in first half of the blob.

  */

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob),

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob), INTEL_GUC_CT_BUFFER_TYPE_RECV); if (unlikely(err)) goto err_out;

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob),

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob), INTEL_GUC_CT_BUFFER_TYPE_SEND); if (unlikely(err)) goto err_deregister;

@@ -341,7 +345,7 @@ static int ct_write(struct intel_guc_ct *ct, u32 len /* in dwords */, u32 fence) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct guc_ct_buffer_desc *desc = ctb->desc; u32 head = desc->head; u32 tail = desc->tail;

@@ -557,7 +561,7 @@ static inline bool ct_header_is_response(u32 header)

static int ct_read(struct intel_guc_ct *ct, u32 *data) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_RECV];

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.recv; struct guc_ct_buffer_desc *desc = ctb->desc; u32 head = desc->head; u32 tail = desc->tail;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h index 4009e2dd0de4..fc9486779e87 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -47,8 +47,11 @@ struct intel_guc_ct { struct i915_vma *vma; bool enabled;

• /* buffers for sending(0) and receiving(1) commands */
• struct intel_guc_ct_buffer ctbs[2];

• /* buffers for sending and receiving commands */

• struct {

• struct intel_guc_ct_buffer send;
  

• struct intel_guc_ct_buffer recv;
  

• } ctbs;

  struct { u32 last_fence; /* last fence used to send request */

-- 2.28.0

On Thu, May 06, 2021 at 12:13:28PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

Future GuC will require CTB buffers sizes to be multiple of 4K. Make these changes now as this shouldn't impact us too much.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: John Harrison john.c.harrison@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 60 ++++++++++++----------- 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index c54a29176862..c87a0a8bef26 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -38,6 +38,32 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct) #define CT_PROBE_ERROR(_ct, _fmt, ...) \ i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__);

+/**

• • DOC: CTB Blob
• • We allocate single blob to hold both CTB descriptors and buffers:

• •  +--------+-----------------------------------------------+------+
    

• •  | offset | contents                                      | size |
    

• •  +========+===============================================+======+
    

• •  | 0x0000 | H2G `CTB Descriptor`_ (send)                  |      |
    

• •  +--------+-----------------------------------------------+  4K  |
    

• •  | 0x0800 | G2H `CTB Descriptor`_ (recv)                  |      |
    

• •  +--------+-----------------------------------------------+------+
    

• •  | 0x1000 | H2G `CT Buffer`_ (send)                       | n*4K |
    

• •  |        |                                               |      |
    

• •  +--------+-----------------------------------------------+------+
    

• •  | 0x1000 | G2H `CT Buffer`_ (recv)                       | m*4K |
    

• •  | + n*4K |                                               |      |
    

• •  +--------+-----------------------------------------------+------+
    

• • Size of each `CT Buffer`_ must be multiple of 4K.
• • As we don't expect too many messages, for now use minimum sizes.
• */

+#define CTB_DESC_SIZE ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K) +#define CTB_H2G_BUFFER_SIZE (SZ_4K) +#define CTB_G2H_BUFFER_SIZE (SZ_4K)

struct ct_request { struct list_head link; u32 fence; @@ -175,29 +201,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)

GEM_BUG_ON(ct->vma);

• /* We allocate 1 page to hold both descriptors and both buffers.

• *       ___________.....................
  

• *      |desc (SEND)|                   :
  

• *      |___________|                   PAGE/4
  

• *      :___________....................:
  

• *      |desc (RECV)|                   :
  

• *      |___________|                   PAGE/4
  

• *      :_______________________________:
  

• *      |cmds (SEND)                    |
  

• *      |                               PAGE/4
  

• *      |_______________________________|
  

• *      |cmds (RECV)                    |
  

• *      |                               PAGE/4
  

• *      |_______________________________|
  

• *
  

• * Each message can use a maximum of 32 dwords and we don't expect to
  

• * have more than 1 in flight at any time, so we have enough space.
  

• * Some logic further ahead will rely on the fact that there is only 1
  

• * page and that it is always mapped, so if the size is changed the
  

• * other code will need updating as well.
  

• */
  

• blob_size = PAGE_SIZE;

• blob_size = 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE + CTB_G2H_BUFFER_SIZE; err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob); if (unlikely(err)) { CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",

@@ -209,17 +213,17 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)

/* store pointers to desc and cmds for send ctb */ desc = blob;

• cmds = blob + PAGE_SIZE / 2;
• cmds_size = PAGE_SIZE / 4;

• cmds = blob + 2 * CTB_DESC_SIZE;

2 is a magic number here. Think it would be more clear with CTB_NUMBER_DESC define here.

Michal what do you think? We can fix this in the next post of this with your blessing.

With that nit: Reviewed-by: Matthew Brost matthew.brost@intel.com

• cmds_size = CTB_H2G_BUFFER_SIZE; CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send", ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);

  guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);

  /* store pointers to desc and cmds for recv ctb */

• desc = blob + PAGE_SIZE / 4;
• cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
• cmds_size = PAGE_SIZE / 4;

• desc = blob + CTB_DESC_SIZE;
• cmds = blob + 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE;
• cmds_size = CTB_G2H_BUFFER_SIZE; CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv", ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);

-- 2.28.0

On Thu, May 06, 2021 at 12:13:29PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

In upcoming patch we will allow more CTB requests to be sent in parallel to the GuC for procesing, so we shouldn't assume any more that GuC will always reply without 10ms.

Use bigger value from CONFIG_DRM_I915_HEARTBEAT_INTERVAL instead.

I think this should be its own config option or we combine it with a config option suggested in patch 37.

What do you think Michal? If you agree I can fix this up in the post of these patches.

Matt

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index c87a0a8bef26..a4b2e7fe318b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -436,17 +436,23 @@ static int ct_write(struct intel_guc_ct *ct, */ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) {

• long timeout; int err;

  /*

  • Fast commands should complete in less than 10us, so sample quickly
  • up to that length of time, then switch to a slower sleep-wait loop.
  • No GuC command should ever take longer than 10ms.

• *
  

• * However, there might be other CT requests in flight before this one,
  

• * so use @CONFIG_DRM_I915_HEARTBEAT_INTERVAL as backup timeout value.
  

  */

• timeout = max(10, CONFIG_DRM_I915_HEARTBEAT_INTERVAL);

#define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status)) err = wait_for_us(done, 10); if (err)

• err = wait_for(done, 10);
  

• err = wait_for(done, timeout);
  

#undef done

if (unlikely(err))

2.28.0

On Thu, May 06, 2021 at 12:13:30PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

We want to stop using guc.send_mutex while sending CTB messages so we have to start protecting access to CTB send descriptor.

For completeness protect also CTB send descriptor.

Add spinlock to struct intel_guc_ct_buffer and start using it.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 14 ++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a4b2e7fe318b..bee0958d8bae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -89,6 +89,8 @@ static void ct_incoming_request_worker_func(struct work_struct *w); */ void intel_guc_ct_init_early(struct intel_guc_ct *ct) {

• spin_lock_init(&ct->ctbs.send.lock);
• spin_lock_init(&ct->ctbs.recv.lock); spin_lock_init(&ct->requests.lock); INIT_LIST_HEAD(&ct->requests.pending); INIT_LIST_HEAD(&ct->requests.incoming);

@@ -479,17 +481,22 @@ static int ct_send(struct intel_guc_ct *ct, GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK); GEM_BUG_ON(!response_buf && response_buf_size);

• spin_lock_irqsave(&ct->ctbs.send.lock, flags);
• fence = ct_get_next_fence(ct); request.fence = fence; request.status = 0; request.response_len = response_buf_size; request.response_buf = response_buf;

• spin_lock_irqsave(&ct->requests.lock, flags);

• spin_lock(&ct->requests.lock); list_add_tail(&request.link, &ct->requests.pending);

• spin_unlock_irqrestore(&ct->requests.lock, flags);

• spin_unlock(&ct->requests.lock);

  err = ct_write(ct, action, len, fence);

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

• if (unlikely(err)) goto unlink;

@@ -825,6 +832,7 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg) void intel_guc_ct_event_handler(struct intel_guc_ct *ct) { u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */

• unsigned long flags; int err = 0;

  if (unlikely(!ct->enabled)) {

@@ -833,7 +841,9 @@ void intel_guc_ct_event_handler(struct intel_guc_ct *ct) }

do {

• spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
  

  err = ct_read(ct, msg);

• spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
  

  if (err) break;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h index fc9486779e87..bc52dc479a14 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -27,11 +27,13 @@ struct intel_guc;

• record (command transport buffer descriptor) and the actual buffer which
• holds the commands.

• • @lock: protects access to the commands buffer and buffer descriptor
  • @desc: pointer to the buffer descriptor
  • @cmds: pointer to the commands buffer
  • @size: size of the commands buffer
  */

struct intel_guc_ct_buffer {

• spinlock_t lock; struct guc_ct_buffer_desc *desc; u32 *cmds; u32 size;

-- 2.28.0

On Thu, May 06, 2021 at 12:13:31PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

We are no longer using descriptor to hold G2H replies and we are protecting access to the descriptor and command buffer by the separate spinlock, so we can stop using mutex.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index bee0958d8bae..cb58fa7f970c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -537,7 +537,6 @@ static int ct_send(struct intel_guc_ct *ct, int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, u32 *response_buf, u32 response_buf_size) {

• struct intel_guc *guc = ct_to_guc(ct); u32 status = ~0; /* undefined */ int ret;

@@ -546,8 +545,6 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, return -ENODEV; }

• mutex_lock(&guc->send_mutex);
• ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) { CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",

@@ -557,7 +554,6 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, action[0], ret, ret); }

• mutex_unlock(&guc->send_mutex); return ret;

}

-- 2.28.0

On Thu, May 06, 2021 at 12:13:32PM -0700, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

In irq handler try to receive just single G2H message, let other messages to be received from tasklet.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 67 ++++++++++++++++------- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 3 + 2 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index cb58fa7f970c..d630ec32decf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -81,6 +81,7 @@ enum { CTB_SEND = 0, CTB_RECV = 1 };

enum { CTB_OWNER_HOST = 0 };

+static void ct_receive_tasklet_func(unsigned long data); static void ct_incoming_request_worker_func(struct work_struct *w);

/** @@ -95,6 +96,7 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct) INIT_LIST_HEAD(&ct->requests.pending); INIT_LIST_HEAD(&ct->requests.incoming); INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);

• tasklet_init(&ct->receive_tasklet, ct_receive_tasklet_func, (unsigned long)ct);

This function is deprecated. tasklet_setup should be used. I can fix this up when I post the CTB patches.

}

static inline const char *guc_ct_buffer_type_to_str(u32 type) @@ -244,6 +246,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct) { GEM_BUG_ON(ct->enabled);

• tasklet_kill(&ct->receive_tasklet); i915_vma_unpin_and_release(&ct->vma, I915_VMA_RELEASE_MAP); memset(ct, 0, sizeof(*ct));

} @@ -629,7 +632,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data) CT_DEBUG(ct, "received %*ph\n", 4 * len, data);

desc->head = head * 4;

• return 0;

• return available - len;

corrupted: CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n", @@ -665,10 +668,10 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg) u32 status; u32 datalen; struct ct_request *req;

• unsigned long flags; bool found = false;

  GEM_BUG_ON(!ct_header_is_response(header));

• GEM_BUG_ON(!in_irq());

  /* Response payload shall at least include fence and status */ if (unlikely(len < 2)) {

@@ -688,7 +691,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)

CT_DEBUG(ct, "response fence %u status %#x\n", fence, status);

• spin_lock(&ct->requests.lock);

• spin_lock_irqsave(&ct->requests.lock, flags); list_for_each_entry(req, &ct->requests.pending, link) { if (unlikely(fence != req->fence)) { CT_DEBUG(ct, "request %u awaits response\n",

@@ -707,7 +710,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg) found = true; break; }

• spin_unlock(&ct->requests.lock);

• spin_unlock_irqrestore(&ct->requests.lock, flags);

  if (!found) CT_ERROR(ct, "Unsolicited response %*ph\n", msgsize, msg);

@@ -821,31 +824,55 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg) return 0; }

+static int ct_receive(struct intel_guc_ct *ct) +{

• u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
• unsigned long flags;
• int ret;
• spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
• ret = ct_read(ct, msg);
• spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
• if (ret < 0)

• return ret;
  

• if (ct_header_is_response(msg[0]))

• ct_handle_response(ct, msg);
  

• else

• ct_handle_request(ct, msg);
  

• return ret;

+}

+static void ct_try_receive_message(struct intel_guc_ct *ct) +{

• int ret;
• if (GEM_WARN_ON(!ct->enabled))

• return;
  

• ret = ct_receive(ct);
• if (ret > 0)

• tasklet_hi_schedule(&ct->receive_tasklet);
  

+}

+static void ct_receive_tasklet_func(unsigned long data) +{

As mentioned above tasklet_init is deprecated. The callback now accepts the tasklet structure and ct can be looked up via container_of macro.

Everything else looks good.

With that and changing to the new tasklet API: Reviewed-by: Matthew Brost matthew.brost@intel.com

• struct intel_guc_ct *ct = (struct intel_guc_ct *)data;
• ct_try_receive_message(ct);

+}

/*

• When we're communicating with the GuC over CT, GuC uses events
• to notify us about new messages being posted on the RECV buffer.

*/ void intel_guc_ct_event_handler(struct intel_guc_ct *ct) {

• u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */

• unsigned long flags;

• int err = 0;

• if (unlikely(!ct->enabled)) { WARN(1, "Unexpected GuC event received while CT disabled!\n"); return; }

• do {

• spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
  

• err = ct_read(ct, msg);
  

• spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
  

• if (err)
  

• 	break;
  

• if (ct_header_is_response(msg[0]))
  

• 	err = ct_handle_response(ct, msg);
  

• else
  

• 	err = ct_handle_request(ct, msg);
  

• } while (!err);

• ct_try_receive_message(ct);

} diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h index bc52dc479a14..cb222f202301 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -6,6 +6,7 @@ #ifndef _INTEL_GUC_CT_H_ #define _INTEL_GUC_CT_H_

+#include <linux/interrupt.h> #include <linux/spinlock.h> #include <linux/workqueue.h>

@@ -55,6 +56,8 @@ struct intel_guc_ct { struct intel_guc_ct_buffer recv; } ctbs;

• struct tasklet_struct receive_tasklet;
• struct { u32 last_fence; /* last fence used to send request */

-- 2.28.0

On Tue, May 11, 2021 at 05:16:38PM +0200, Daniel Vetter wrote:

On Thu, May 06, 2021 at 12:13:34PM -0700, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

New GuC firmware will unify format of MMIO and CTB H2G messages. Introduce their definitions now to allow gradual transition of our code to match new changes.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Michał Winiarski michal.winiarski@intel.com

---

.../gpu/drm/i915/gt/uc/abi/guc_messages_abi.h | 226 ++++++++++++++++++ 1 file changed, 226 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h index 775e21f3058c..1c264819aa03 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h @@ -6,6 +6,232 @@ #ifndef _ABI_GUC_MESSAGES_ABI_H #define _ABI_GUC_MESSAGES_ABI_H

+/**

• • DOC: HXG Message

These aren't useful if we don't pull them in somewhere in the Documentation/gpu hierarchy. General comment, and also please check that it all renders correctly still.

Sure. Let me figure this out before my next rev.

btw if you respin a patch not originally by you we generally add a (v1) to the original s-o-b line (or whever the version split was) and explain in the usual changelog in the commit message what was changed.

Still new to this process. Will do.

Matt

This holds for the entire series ofc. -Daniel

...

• • All messages exchanged with GuC are defined using 32 bit dwords.
• • First dword is treated as a message header. Remaining dwords are optional.
• • .. _HXG Message:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | | | |
• • | 0 | 31 | **ORIGIN** - originator of the message |
• • | | | - _`GUC_HXG_ORIGIN_HOST` = 0 |
• • | | | - _`GUC_HXG_ORIGIN_GUC` = 1 |
• • | | | |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | **TYPE** - message type |
• • | | | - _`GUC_HXG_TYPE_REQUEST` = 0 |
• • | | | - _`GUC_HXG_TYPE_EVENT` = 1 |
• • | | | - _`GUC_HXG_TYPE_NO_RESPONSE_BUSY` = 3 |
• • | | | - _`GUC_HXG_TYPE_NO_RESPONSE_RETRY` = 5 |
• • | | | - _`GUC_HXG_TYPE_RESPONSE_FAILURE` = 6 |
• • | | | - _`GUC_HXG_TYPE_RESPONSE_SUCCESS` = 7 |
• • | +-------+--------------------------------------------------------------+
• • | | 27:0 | **AUX** - auxiliary data (depends TYPE) |
• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:0 | optional payload (depends on TYPE) |
• • +---+-------+ |
• • |...| | |
• • +---+-------+ |
• • | n | 31:0 | |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_MSG_MIN_LEN 1u +#define GUC_HXG_MSG_0_ORIGIN (0x1 << 31) +#define GUC_HXG_ORIGIN_HOST 0u +#define GUC_HXG_ORIGIN_GUC 1u +#define GUC_HXG_MSG_0_TYPE (0x7 << 28) +#define GUC_HXG_TYPE_REQUEST 0u +#define GUC_HXG_TYPE_EVENT 1u +#define GUC_HXG_TYPE_NO_RESPONSE_BUSY 3u +#define GUC_HXG_TYPE_NO_RESPONSE_RETRY 5u +#define GUC_HXG_TYPE_RESPONSE_FAILURE 6u +#define GUC_HXG_TYPE_RESPONSE_SUCCESS 7u +#define GUC_HXG_MSG_0_AUX (0xfffffff << 0)

+/**

• • DOC: HXG Request
• • The `HXG Request`_ message should be used to initiate synchronous activity
• • for which confirmation or return data is expected.
• • The recipient of this message shall use `HXG Response`_, `HXG Failure`_
• • or `HXG Retry`_ message as a definite reply, and may use `HXG Busy`_
• • message as a intermediate reply.
• • Format of @DATA0 and all @DATAn fields depends on the @ACTION code.
• • _HXG Request:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_REQUEST_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:16 | **DATA0** - request data (depends on ACTION) |
• • | +-------+--------------------------------------------------------------+
• • | | 15:0 | **ACTION** - requested action code |
• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:0 | **DATA1** - optional data (depends on ACTION) |
• • +---+-------+--------------------------------------------------------------+
• • |...| | |
• • +---+-------+--------------------------------------------------------------+
• • | n | 31:0 | **DATAn** - optional data (depends on ACTION) |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_REQUEST_MSG_MIN_LEN GUC_HXG_MSG_MIN_LEN +#define GUC_HXG_REQUEST_MSG_0_DATA0 (0xfff << 16) +#define GUC_HXG_REQUEST_MSG_0_ACTION (0xffff << 0) +#define GUC_HXG_REQUEST_MSG_n_DATAn (0xffffffff << 0)

+/**

• • DOC: HXG Event
• • The `HXG Event`_ message should be used to initiate asynchronous activity
• • that does not involves immediate confirmation nor data.
• • Format of @DATA0 and all @DATAn fields depends on the @ACTION code.
• • .. _HXG Event:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_EVENT_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:16 | **DATA0** - event data (depends on ACTION) |
• • | +-------+--------------------------------------------------------------+
• • | | 15:0 | **ACTION** - event action code |
• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:0 | **DATA1** - optional event data (depends on ACTION) |
• • +---+-------+--------------------------------------------------------------+
• • |...| | |
• • +---+-------+--------------------------------------------------------------+
• • | n | 31:0 | **DATAn** - optional event data (depends on ACTION) |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_EVENT_MSG_MIN_LEN GUC_HXG_MSG_MIN_LEN +#define GUC_HXG_EVENT_MSG_0_DATA0 (0xfff << 16) +#define GUC_HXG_EVENT_MSG_0_ACTION (0xffff << 0) +#define GUC_HXG_EVENT_MSG_n_DATAn (0xffffffff << 0)

+/**

• • DOC: HXG Busy
• • The `HXG Busy`_ message may be used to acknowledge reception of the `HXG Request`_
• • message if the recipient expects that it processing will be longer than default
• • timeout.
• • The @COUNTER field may be used as a progress indicator.
• • .. _HXG Busy:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_NO_RESPONSE_BUSY_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:0 | **COUNTER** - progress indicator |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_BUSY_MSG_LEN GUC_HXG_MSG_MIN_LEN +#define GUC_HXG_BUSY_MSG_0_COUNTER GUC_HXG_MSG_0_AUX

+/**

• • DOC: HXG Retry
• • The `HXG Retry`_ message should be used by recipient to indicate that the
• • `HXG Request`_ message was dropped and it should be resent again.
• • The @REASON field may be used to provide additional information.
• • .. _HXG Retry:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_NO_RESPONSE_RETRY_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:0 | **REASON** - reason for retry |
• • | | | - _`GUC_HXG_RETRY_REASON_UNSPECIFIED` = 0 |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_RETRY_MSG_LEN GUC_HXG_MSG_MIN_LEN +#define GUC_HXG_RETRY_MSG_0_REASON GUC_HXG_MSG_0_AUX +#define GUC_HXG_RETRY_REASON_UNSPECIFIED 0u

+/**

• • DOC: HXG Failure
• • The `HXG Failure`_ message shall be used as a reply to the `HXG Request`_
• • message that could not be processed due to an error.
• • .. _HXG Failure:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_RESPONSE_FAILURE_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:16 | **HINT** - additional error hint |
• • | +-------+--------------------------------------------------------------+
• • | | 15:0 | **ERROR** - error/result code |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_FAILURE_MSG_LEN GUC_HXG_MSG_MIN_LEN +#define GUC_HXG_FAILURE_MSG_0_HINT (0xfff << 16) +#define GUC_HXG_FAILURE_MSG_0_ERROR (0xffff << 0)

+/**

• • DOC: HXG Response
• • The `HXG Response`_ message SHALL be used as a reply to the `HXG Request`_
• • message that was successfully processed without an error.
• • .. _HXG Response:
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_RESPONSE_SUCCESS_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:0 | **DATA0** - data (depends on ACTION from `HXG Request`_) |
• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:0 | **DATA1** - data (depends on ACTION from `HXG Request`_) |
• • +---+-------+--------------------------------------------------------------+
• • |...| | |
• • +---+-------+--------------------------------------------------------------+
• • | n | 31:0 | **DATAn** - data (depends on ACTION from `HXG Request`_) |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_HXG_RESPONSE_MSG_MIN_LEN GUC_HXG_MSG_MIN_LEN +#define GUC_HXG_RESPONSE_MSG_0_DATA0 GUC_HXG_MSG_0_AUX +#define GUC_HXG_RESPONSE_MSG_n_DATAn (0xffffffff << 0)

+/* deprecated */ #define INTEL_GUC_MSG_TYPE_SHIFT 28 #define INTEL_GUC_MSG_TYPE_MASK (0xF << INTEL_GUC_MSG_TYPE_SHIFT)

#define INTEL_GUC_MSG_DATA_SHIFT 16

2.28.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch