| Message ID | 20210105231947.31235-5-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Split GuC submission from execlists submission | expand |
Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:46) > Initialize all required entries from guc_set_default_submission, instead > of calling the execlists function. The previously inherited setup has > been copied over from the execlist code and simplified by removing the > execlists submission-specific parts. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: John Harrison <john.c.harrison@intel.com> > --- > + if (INTEL_GEN(engine->i915) >= 12) > + engine->flags |= I915_ENGINE_HAS_RELATIVE_MMIO; We should probably lift this to intel_engine_setup(). Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 1/5/2021 5:02 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:46) >> Initialize all required entries from guc_set_default_submission, instead >> of calling the execlists function. The previously inherited setup has >> been copied over from the execlist code and simplified by removing the >> execlists submission-specific parts. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: John Harrison <john.c.harrison@intel.com> >> --- >> + if (INTEL_GEN(engine->i915) >= 12) >> + engine->flags |= I915_ENGINE_HAS_RELATIVE_MMIO; > We should probably lift this to intel_engine_setup(). GuC requires a more extensive usage of the relative mmio stuff, given that it picks which engine to submit to when using virtual engines, so I'm not sure if the support is going to look exactly the same for both back-ends. There is an old patch from John H to rework the relative mmio (https://patchwork.freedesktop.org/patch/332558/), which will have to land in some form as part of the GuC submission re-enabling. I'd prefer to wait for that to land before moving this flag. Daniele > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris
Quoting Daniele Ceraolo Spurio (2021-01-06 02:38:09) > > > On 1/5/2021 5:02 PM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:46) > >> Initialize all required entries from guc_set_default_submission, instead > >> of calling the execlists function. The previously inherited setup has > >> been copied over from the execlist code and simplified by removing the > >> execlists submission-specific parts. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Matthew Brost <matthew.brost@intel.com> > >> Cc: John Harrison <john.c.harrison@intel.com> > >> --- > >> + if (INTEL_GEN(engine->i915) >= 12) > >> + engine->flags |= I915_ENGINE_HAS_RELATIVE_MMIO; > > We should probably lift this to intel_engine_setup(). > > GuC requires a more extensive usage of the relative mmio stuff, given > that it picks which engine to submit to when using virtual engines, so > I'm not sure if the support is going to look exactly the same for both > back-ends. There is an old patch from John H to rework the relative mmio > (https://patchwork.freedesktop.org/patch/332558/), which will have to > land in some form as part of the GuC submission re-enabling. I'd prefer > to wait for that to land before moving this flag. Whether or not LR* take the flag is independent of the submission backend. As to when to use the flag, I think that patch needs a lot more refinement. I915_ENGINE_HAS_RELATIVE_MMIO is the odd-one-out in the flags atm as it reflects HW capability. -Chris
On 1/5/2021 7:09 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2021-01-06 02:38:09) >> >> On 1/5/2021 5:02 PM, Chris Wilson wrote: >>> Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:46) >>>> Initialize all required entries from guc_set_default_submission, instead >>>> of calling the execlists function. The previously inherited setup has >>>> been copied over from the execlist code and simplified by removing the >>>> execlists submission-specific parts. >>>> >>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Matthew Brost <matthew.brost@intel.com> >>>> Cc: John Harrison <john.c.harrison@intel.com> >>>> --- >>>> + if (INTEL_GEN(engine->i915) >= 12) >>>> + engine->flags |= I915_ENGINE_HAS_RELATIVE_MMIO; >>> We should probably lift this to intel_engine_setup(). >> GuC requires a more extensive usage of the relative mmio stuff, given >> that it picks which engine to submit to when using virtual engines, so >> I'm not sure if the support is going to look exactly the same for both >> back-ends. There is an old patch from John H to rework the relative mmio >> (https://patchwork.freedesktop.org/patch/332558/), which will have to >> land in some form as part of the GuC submission re-enabling. I'd prefer >> to wait for that to land before moving this flag. > Whether or not LR* take the flag is independent of the submission > backend. As to when to use the flag, I think that patch needs a lot more > refinement. it definitely does. I'll lift this up for now, we can reconsider later if needed. Daniele > I915_ENGINE_HAS_RELATIVE_MMIO is the odd-one-out in the flags atm as it > reflects HW capability. > -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index a5b442683c18..3b2821516b93 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -3082,7 +3082,7 @@ static void execlists_park(struct intel_engine_cs *engine) cancel_timer(&engine->execlists.preempt); } -void intel_execlists_set_default_submission(struct intel_engine_cs *engine) +static void execlists_set_default_submission(struct intel_engine_cs *engine) { engine->submit_request = execlists_submit_request; engine->schedule = i915_schedule; @@ -3150,7 +3150,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) engine->emit_fini_breadcrumb = gen12_emit_fini_breadcrumb_xcs; engine->emit_flush = gen12_emit_flush_xcs; } - engine->set_default_submission = intel_execlists_set_default_submission; + engine->set_default_submission = execlists_set_default_submission; if (INTEL_GEN(engine->i915) < 11) { engine->irq_enable = gen8_logical_ring_enable_irq; @@ -3915,7 +3915,7 @@ bool intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine) { return engine->set_default_submission == - intel_execlists_set_default_submission; + execlists_set_default_submission; } #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h index 0c675bbff351..a8fd7adefd82 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h @@ -22,8 +22,6 @@ enum { int intel_execlists_submission_setup(struct intel_engine_cs *engine); -void intel_execlists_set_default_submission(struct intel_engine_cs *engine); - void intel_execlists_show_requests(struct intel_engine_cs *engine, struct drm_printer *m, void (*show_request)(struct drm_printer *m, 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 2faaa14e6e42..3993f1d75e87 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -10,7 +10,6 @@ #include "gt/intel_breadcrumbs.h" #include "gt/intel_context.h" #include "gt/intel_engine_pm.h" -#include "gt/intel_execlists_submission.h" /* XXX */ #include "gt/intel_gt.h" #include "gt/intel_gt_pm.h" #include "gt/intel_lrc.h" @@ -513,6 +512,34 @@ static int guc_request_alloc(struct i915_request *request) return 0; } +static inline void queue_request(struct intel_engine_cs *engine, + struct i915_request *rq, + int prio) +{ + GEM_BUG_ON(!list_empty(&rq->sched.link)); + list_add_tail(&rq->sched.link, + i915_sched_lookup_priolist(engine, prio)); + set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); +} + +static void guc_submit_request(struct i915_request *rq) +{ + struct intel_engine_cs *engine = rq->engine; + unsigned long flags; + + /* Will be called from irq-context when using foreign fences. */ + spin_lock_irqsave(&engine->active.lock, flags); + + queue_request(engine, rq, rq_prio(rq)); + + GEM_BUG_ON(RB_EMPTY_ROOT(&engine->execlists.queue.rb_root)); + GEM_BUG_ON(list_empty(&rq->sched.link)); + + tasklet_hi_schedule(&engine->execlists.tasklet); + + spin_unlock_irqrestore(&engine->active.lock, flags); +} + static void sanitize_hwsp(struct intel_engine_cs *engine) { struct intel_timeline *tl; @@ -600,31 +627,31 @@ static int guc_resume(struct intel_engine_cs *engine) static void guc_set_default_submission(struct intel_engine_cs *engine) { - /* - * We inherit a bunch of functions from execlists that we'd like - * to keep using: - * - * engine->submit_request = execlists_submit_request; - * engine->cancel_requests = execlists_cancel_requests; - * engine->schedule = execlists_schedule; - * - * But we need to override the actual submission backend in order - * to talk to the GuC. - */ - intel_execlists_set_default_submission(engine); - + engine->submit_request = guc_submit_request; + engine->schedule = i915_schedule; engine->execlists.tasklet.func = guc_submission_tasklet; - /* do not use execlists park/unpark */ - engine->park = engine->unpark = NULL; - 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_SUPPORTS_STATS; 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; + */ + + if (INTEL_GEN(engine->i915) >= 12) + engine->flags |= I915_ENGINE_HAS_RELATIVE_MMIO; + + engine->emit_bb_start = gen8_emit_bb_start; /* * For the breadcrumb irq to work we need the interrupts to stay
Initialize all required entries from guc_set_default_submission, instead of calling the execlists function. The previously inherited setup has been copied over from the execlist code and simplified by removing the execlists submission-specific parts. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Matthew Brost <matthew.brost@intel.com> Cc: John Harrison <john.c.harrison@intel.com> --- .../drm/i915/gt/intel_execlists_submission.c | 6 +- .../drm/i915/gt/intel_execlists_submission.h | 2 - .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 63 +++++++++++++------ 3 files changed, 48 insertions(+), 23 deletions(-)