Re: [Intel-gfx] [PATCH 29/57] drm/i915: Move scheduler flags
Quoting Tvrtko Ursulin (2021-02-04 15:14:20) > > On 01/02/2021 08:56, Chris Wilson wrote: > > Start extracting the scheduling flags from the engine. We begin with its > > own existence. > > > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/i915/gt/intel_engine.h| 6 ++ > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 21 +++ > > .../drm/i915/gt/intel_execlists_submission.c | 6 +- > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- > > drivers/gpu/drm/i915/i915_request.h | 2 +- > > drivers/gpu/drm/i915/i915_scheduler.c | 2 +- > > drivers/gpu/drm/i915/i915_scheduler_types.h | 10 + > > 7 files changed, 31 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h > > b/drivers/gpu/drm/i915/gt/intel_engine.h > > index c530839627bb..4f0163457aed 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > > @@ -261,6 +261,12 @@ intel_engine_has_heartbeat(const struct > > intel_engine_cs *engine) > > return READ_ONCE(engine->props.heartbeat_interval_ms); > > } > > > > +static inline bool > > +intel_engine_has_scheduler(struct intel_engine_cs *engine) > > +{ > > + return i915_sched_is_active(intel_engine_get_scheduler(engine)); > > +} > > + > > static inline void > > intel_engine_kick_scheduler(struct intel_engine_cs *engine) > > { > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h > > b/drivers/gpu/drm/i915/gt/intel_engine_types.h > > index 6b0bde292916..a3024a0de1de 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > > @@ -440,14 +440,13 @@ struct intel_engine_cs { > > > > #define I915_ENGINE_USING_CMD_PARSER BIT(0) > > #define I915_ENGINE_SUPPORTS_STATS BIT(1) > > -#define I915_ENGINE_HAS_SCHEDULERBIT(2) > > -#define I915_ENGINE_HAS_PREEMPTION BIT(3) > > -#define I915_ENGINE_HAS_SEMAPHORES BIT(4) > > -#define I915_ENGINE_HAS_TIMESLICES BIT(5) > > -#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(6) > > -#define I915_ENGINE_IS_VIRTUAL BIT(7) > > -#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(8) > > -#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(9) > > +#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) > > unsigned int flags; > > > > /* > > @@ -530,12 +529,6 @@ intel_engine_supports_stats(const struct > > intel_engine_cs *engine) > > return engine->flags & I915_ENGINE_SUPPORTS_STATS; > > } > > > > -static inline bool > > -intel_engine_has_scheduler(const struct intel_engine_cs *engine) > > -{ > > - return engine->flags & I915_ENGINE_HAS_SCHEDULER; > > -} > > - > > static inline bool > > intel_engine_has_preemption(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 b1007e560527..3217cb4369ad 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > > @@ -2913,7 +2913,6 @@ logical_ring_default_vfuncs(struct intel_engine_cs > > *engine) > >*/ > > } > > > > - engine->flags |= I915_ENGINE_HAS_SCHEDULER; > > engine->flags |= I915_ENGINE_SUPPORTS_STATS; > > if (!intel_vgpu_active(engine->i915)) { > > engine->flags |= I915_ENGINE_HAS_SEMAPHORES; > > @@ -2981,6 +2980,7 @@ int intel_execlists_submission_setup(struct > > intel_engine_cs *engine) > > engine->sched.is_executing = execlists_is_executing; > > engine->sched.show = execlists_show; > > tasklet_setup(&engine->sched.tasklet, execlists_submission_tasklet); > > + __set_bit(I915_SCHED_ACTIVE_BIT, &engine->sched.flags); > > This feels a bit dodgy - does is stay like this, with the engine setting > up both the tasklet and setting the bit directly? Could we say that > setting a tasklet via a helper could turn on the bit? Bare with me for a few more patches. I didn't like it either. I guess I should move that chunk earlier so we don't have the eyesore and rightful questions over my sanity. > > +static inline bool i915_sched_is_active(const struct i915_sched *se) > > +{ > > + return test_bit(I915_SCHED_ACTIVE_BIT, &se->flags); > > +} > > What do you have in mind for the distinction between "has scheduler" and > "scheduler is active"? By the end, we use the i915_scheduler for everything, including legacy ringbuffer submission for which the i915_scheduler does nothing, as it cannot support execution reordering. So there's a scheduler behind every i915_r
Re: [Intel-gfx] [PATCH 29/57] drm/i915: Move scheduler flags
On 01/02/2021 08:56, Chris Wilson wrote: Start extracting the scheduling flags from the engine. We begin with its own existence. Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/gt/intel_engine.h| 6 ++ drivers/gpu/drm/i915/gt/intel_engine_types.h | 21 +++ .../drm/i915/gt/intel_execlists_submission.c | 6 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- drivers/gpu/drm/i915/i915_request.h | 2 +- drivers/gpu/drm/i915/i915_scheduler.c | 2 +- drivers/gpu/drm/i915/i915_scheduler_types.h | 10 + 7 files changed, 31 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index c530839627bb..4f0163457aed 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -261,6 +261,12 @@ intel_engine_has_heartbeat(const struct intel_engine_cs *engine) return READ_ONCE(engine->props.heartbeat_interval_ms); } +static inline bool +intel_engine_has_scheduler(struct intel_engine_cs *engine) +{ + return i915_sched_is_active(intel_engine_get_scheduler(engine)); +} + static inline void intel_engine_kick_scheduler(struct intel_engine_cs *engine) { diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 6b0bde292916..a3024a0de1de 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -440,14 +440,13 @@ struct intel_engine_cs { #define I915_ENGINE_USING_CMD_PARSER BIT(0) #define I915_ENGINE_SUPPORTS_STATS BIT(1) -#define I915_ENGINE_HAS_SCHEDULERBIT(2) -#define I915_ENGINE_HAS_PREEMPTION BIT(3) -#define I915_ENGINE_HAS_SEMAPHORES BIT(4) -#define I915_ENGINE_HAS_TIMESLICES BIT(5) -#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(6) -#define I915_ENGINE_IS_VIRTUAL BIT(7) -#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(8) -#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(9) +#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) unsigned int flags; /* @@ -530,12 +529,6 @@ intel_engine_supports_stats(const struct intel_engine_cs *engine) return engine->flags & I915_ENGINE_SUPPORTS_STATS; } -static inline bool -intel_engine_has_scheduler(const struct intel_engine_cs *engine) -{ - return engine->flags & I915_ENGINE_HAS_SCHEDULER; -} - static inline bool intel_engine_has_preemption(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 b1007e560527..3217cb4369ad 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -2913,7 +2913,6 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) */ } - engine->flags |= I915_ENGINE_HAS_SCHEDULER; engine->flags |= I915_ENGINE_SUPPORTS_STATS; if (!intel_vgpu_active(engine->i915)) { engine->flags |= I915_ENGINE_HAS_SEMAPHORES; @@ -2981,6 +2980,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine) engine->sched.is_executing = execlists_is_executing; engine->sched.show = execlists_show; tasklet_setup(&engine->sched.tasklet, execlists_submission_tasklet); + __set_bit(I915_SCHED_ACTIVE_BIT, &engine->sched.flags); This feels a bit dodgy - does is stay like this, with the engine setting up both the tasklet and setting the bit directly? Could we say that setting a tasklet via a helper could turn on the bit? timer_setup(&engine->execlists.timer, execlists_timeslice, 0); timer_setup(&engine->execlists.preempt, execlists_preempt, 0); @@ -3386,6 +3386,7 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings, unsigned int count) { struct virtual_engine *ve; + unsigned long sched; unsigned int n; int err; @@ -3444,6 +3445,7 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings, goto err_put; } + sched = ~0U; for (n = 0; n < count; n++) { struct intel_engine_cs *sibling = siblings[n]; @@ -3473,6 +3475,7 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings, ve->siblings[ve->num_siblings++] = sibling; ve->base.mask |= sibling->mask; + sched &= sibling->sched.flags; /* * All physical engines must be compatible for their emission @@ -3514,6 +3517,7 @@ intel_execlists_create_virtual(struct intel_engine_cs