| Message ID | 20210201085715.27435-30-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/57] drm/i915/gt: Restrict the GT clock override to just Icelake | expand |
On 01/02/2021 08:56, Chris Wilson wrote: > Whether a scheduler chooses to implement timeslicing is up to it, and > not an underlying property of the HW engine. The scheduler does depend > on the HW supporting preemption. Therefore, continuing on the comment I made in the previous patch, if we had a helper with which engine would request scheduling (setting the tasklet), if it passed in a pointer to itself, scheduler would then be able to inspect if the engine supports preemption and so set its own internal flag. Makes sense? It would require something like: i915_sched_enable_scheduling(se, engine, tasklet) Or something like that if my memory still holds. Regards, Tvrtko > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_engine.h | 6 ++++++ > drivers/gpu/drm/i915/gt/intel_engine_types.h | 18 ++++-------------- > .../drm/i915/gt/intel_execlists_submission.c | 9 ++++++--- > drivers/gpu/drm/i915/gt/selftest_execlists.c | 2 +- > drivers/gpu/drm/i915/i915_scheduler_types.h | 10 ++++++++++ > 5 files changed, 27 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h > index 4f0163457aed..ca3a9cb06328 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > @@ -279,4 +279,10 @@ intel_engine_flush_scheduler(struct intel_engine_cs *engine) > i915_sched_flush(intel_engine_get_scheduler(engine)); > } > > +static inline bool > +intel_engine_has_timeslices(struct intel_engine_cs *engine) > +{ > + return i915_sched_has_timeslices(intel_engine_get_scheduler(engine)); > +} > + > #endif /* _INTEL_RINGBUFFER_H_ */ > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index a3024a0de1de..96a0aec29672 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > @@ -442,11 +442,10 @@ struct intel_engine_cs { > #define I915_ENGINE_SUPPORTS_STATS BIT(1) > #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_NEEDS_BREADCRUMB_TASKLET BIT(4) > +#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; > > /* > @@ -541,15 +540,6 @@ intel_engine_has_semaphores(const struct intel_engine_cs *engine) > return engine->flags & I915_ENGINE_HAS_SEMAPHORES; > } > > -static inline bool > -intel_engine_has_timeslices(const struct intel_engine_cs *engine) > -{ > - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > - return false; > - > - return engine->flags & I915_ENGINE_HAS_TIMESLICES; > -} > - > static inline bool > intel_engine_needs_breadcrumb_tasklet(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 3217cb4369ad..d4b6d262265a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > @@ -1023,7 +1023,7 @@ static bool needs_timeslice(const struct intel_engine_cs *engine, > { > const struct i915_sched *se = &engine->sched; > > - if (!intel_engine_has_timeslices(engine)) > + if (!i915_sched_has_timeslices(se)) > return false; > > /* If not currently active, or about to switch, wait for next event */ > @@ -2918,8 +2918,6 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) > 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; > } > } > > @@ -2927,6 +2925,11 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) > engine->emit_bb_start = gen8_emit_bb_start; > else > engine->emit_bb_start = gen8_emit_bb_start_noarb; > + > + if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION) && > + intel_engine_has_preemption(engine)) > + __set_bit(I915_SCHED_HAS_TIMESLICES_BIT, > + &engine->sched.flags); > } > > static void logical_ring_default_irqs(struct intel_engine_cs *engine) > diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c > index cfc0f4b9fbc5..147cbfd6dec0 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c > +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c > @@ -3825,7 +3825,7 @@ static unsigned int > __select_siblings(struct intel_gt *gt, > unsigned int class, > struct intel_engine_cs **siblings, > - bool (*filter)(const struct intel_engine_cs *)) > + bool (*filter)(struct intel_engine_cs *)) > { > unsigned int n = 0; > unsigned int inst; > diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h > index cb1eddb7edc8..dfb29b8c2bee 100644 > --- a/drivers/gpu/drm/i915/i915_scheduler_types.h > +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h > @@ -12,12 +12,14 @@ > #include <linux/workqueue.h> > > #include "i915_priolist_types.h" > +#include "i915_utils.h" > > struct drm_printer; > struct i915_request; > > enum { > I915_SCHED_ACTIVE_BIT = 0, > + I915_SCHED_HAS_TIMESLICES_BIT, > }; > > /** > @@ -184,4 +186,12 @@ static inline bool i915_sched_is_active(const struct i915_sched *se) > return test_bit(I915_SCHED_ACTIVE_BIT, &se->flags); > } > > +static inline bool i915_sched_has_timeslices(const struct i915_sched *se) > +{ > + if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + return false; > + > + return test_bit(I915_SCHED_HAS_TIMESLICES_BIT, &se->flags); > +} > + > #endif /* _I915_SCHEDULER_TYPES_H_ */ >
Quoting Tvrtko Ursulin (2021-02-04 15:18:31) > > On 01/02/2021 08:56, Chris Wilson wrote: > > Whether a scheduler chooses to implement timeslicing is up to it, and > > not an underlying property of the HW engine. The scheduler does depend > > on the HW supporting preemption. > > Therefore, continuing on the comment I made in the previous patch, if we > had a helper with which engine would request scheduling (setting the > tasklet), if it passed in a pointer to itself, scheduler would then be > able to inspect if the engine supports preemption and so set its own > internal flag. Makes sense? It would require something like: Actually not keen on pushing the inspection into the core scheduler and would rather have the backend turn it on for itself. Because it's not just about the engine supporting preemption, it's about whether or not the backend wants to bother implement timeslicing itself. If we skip to the end, it looks like this for execlists: i915_sched_init(&el->sched, i915->drm.dev, engine->name, engine->mask, &execlists_ops, engine); if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION) && intel_engine_has_preemption(engine)) __set_bit(I915_SCHED_TIMESLICE_BIT, &el->sched.flags); if (intel_engine_has_preemption(engine)) { __set_bit(I915_SCHED_BUSYWAIT_BIT, &el->sched.flags); __set_bit(I915_SCHED_PREEMPT_RESET_BIT, &el->sched.flags); } with the virtual scheduler: ve->base.sched = i915_sched_create(ve->base.i915->drm.dev, ve->base.name, ve->base.mask, &virtual_ops, ve); if (!ve->base.sched) { err = -ENOMEM; goto err_put; } ve->base.sched->flags |= sched; /* override submission method */ I think the virtual scheduler suggests that we can't rely on the scheduler core to dtrt by itself. And if you are still awake by the time we get to this point, how to avoid ve->base.sched->flags |= sched are welcome. -Chris
On 04/02/2021 16:11, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2021-02-04 15:18:31) >> >> On 01/02/2021 08:56, Chris Wilson wrote: >>> Whether a scheduler chooses to implement timeslicing is up to it, and >>> not an underlying property of the HW engine. The scheduler does depend >>> on the HW supporting preemption. >> >> Therefore, continuing on the comment I made in the previous patch, if we >> had a helper with which engine would request scheduling (setting the >> tasklet), if it passed in a pointer to itself, scheduler would then be >> able to inspect if the engine supports preemption and so set its own >> internal flag. Makes sense? It would require something like: > > Actually not keen on pushing the inspection into the core scheduler and > would rather have the backend turn it on for itself. Because it's not > just about the engine supporting preemption, it's about whether or not > the backend wants to bother implement timeslicing itself. > > If we skip to the end, it looks like this for execlists: > > i915_sched_init(&el->sched, i915->drm.dev, > engine->name, engine->mask, > &execlists_ops, engine); > > if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION) && > intel_engine_has_preemption(engine)) > __set_bit(I915_SCHED_TIMESLICE_BIT, &el->sched.flags); > > if (intel_engine_has_preemption(engine)) { > __set_bit(I915_SCHED_BUSYWAIT_BIT, &el->sched.flags); > __set_bit(I915_SCHED_PREEMPT_RESET_BIT, &el->sched.flags); > } > > with the virtual scheduler: > > ve->base.sched = > i915_sched_create(ve->base.i915->drm.dev, > ve->base.name, > ve->base.mask, > &virtual_ops, ve); > if (!ve->base.sched) { > err = -ENOMEM; > goto err_put; > } > > ve->base.sched->flags |= sched; /* override submission method */ > > I think the virtual scheduler suggests that we can't rely on the > scheduler core to dtrt by itself. And if you are still awake by the time > we get to this point, how to avoid ve->base.sched->flags |= sched are > welcome. Not at the moment. Since it is details lets finish first and then see is my thinking. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index 4f0163457aed..ca3a9cb06328 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -279,4 +279,10 @@ intel_engine_flush_scheduler(struct intel_engine_cs *engine) i915_sched_flush(intel_engine_get_scheduler(engine)); } +static inline bool +intel_engine_has_timeslices(struct intel_engine_cs *engine) +{ + return i915_sched_has_timeslices(intel_engine_get_scheduler(engine)); +} + #endif /* _INTEL_RINGBUFFER_H_ */ diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a3024a0de1de..96a0aec29672 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -442,11 +442,10 @@ struct intel_engine_cs { #define I915_ENGINE_SUPPORTS_STATS BIT(1) #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_NEEDS_BREADCRUMB_TASKLET BIT(4) +#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; /* @@ -541,15 +540,6 @@ intel_engine_has_semaphores(const struct intel_engine_cs *engine) return engine->flags & I915_ENGINE_HAS_SEMAPHORES; } -static inline bool -intel_engine_has_timeslices(const struct intel_engine_cs *engine) -{ - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) - return false; - - return engine->flags & I915_ENGINE_HAS_TIMESLICES; -} - static inline bool intel_engine_needs_breadcrumb_tasklet(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 3217cb4369ad..d4b6d262265a 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1023,7 +1023,7 @@ static bool needs_timeslice(const struct intel_engine_cs *engine, { const struct i915_sched *se = &engine->sched; - if (!intel_engine_has_timeslices(engine)) + if (!i915_sched_has_timeslices(se)) return false; /* If not currently active, or about to switch, wait for next event */ @@ -2918,8 +2918,6 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) 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; } } @@ -2927,6 +2925,11 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) engine->emit_bb_start = gen8_emit_bb_start; else engine->emit_bb_start = gen8_emit_bb_start_noarb; + + if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION) && + intel_engine_has_preemption(engine)) + __set_bit(I915_SCHED_HAS_TIMESLICES_BIT, + &engine->sched.flags); } static void logical_ring_default_irqs(struct intel_engine_cs *engine) diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c index cfc0f4b9fbc5..147cbfd6dec0 100644 --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c @@ -3825,7 +3825,7 @@ static unsigned int __select_siblings(struct intel_gt *gt, unsigned int class, struct intel_engine_cs **siblings, - bool (*filter)(const struct intel_engine_cs *)) + bool (*filter)(struct intel_engine_cs *)) { unsigned int n = 0; unsigned int inst; diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h index cb1eddb7edc8..dfb29b8c2bee 100644 --- a/drivers/gpu/drm/i915/i915_scheduler_types.h +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h @@ -12,12 +12,14 @@ #include <linux/workqueue.h> #include "i915_priolist_types.h" +#include "i915_utils.h" struct drm_printer; struct i915_request; enum { I915_SCHED_ACTIVE_BIT = 0, + I915_SCHED_HAS_TIMESLICES_BIT, }; /** @@ -184,4 +186,12 @@ static inline bool i915_sched_is_active(const struct i915_sched *se) return test_bit(I915_SCHED_ACTIVE_BIT, &se->flags); } +static inline bool i915_sched_has_timeslices(const struct i915_sched *se) +{ + if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + return false; + + return test_bit(I915_SCHED_HAS_TIMESLICES_BIT, &se->flags); +} + #endif /* _I915_SCHEDULER_TYPES_H_ */
Whether a scheduler chooses to implement timeslicing is up to it, and not an underlying property of the HW engine. The scheduler does depend on the HW supporting preemption. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_engine.h | 6 ++++++ drivers/gpu/drm/i915/gt/intel_engine_types.h | 18 ++++-------------- .../drm/i915/gt/intel_execlists_submission.c | 9 ++++++--- drivers/gpu/drm/i915/gt/selftest_execlists.c | 2 +- drivers/gpu/drm/i915/i915_scheduler_types.h | 10 ++++++++++ 5 files changed, 27 insertions(+), 18 deletions(-)