| Message ID | 20190806090535.14807-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/22] drm/i915/gem: Make caps.scheduler static | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > We do not notify userspace when the scheduler capabilities are changed > (due to wedging the driver) and as such userspace will expect the caps > to be static and unchanging. Make it so, and so we only need to compute > our caps once during driver registration. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 16 ++-------------- > .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 4 ++-- > drivers/gpu/drm/i915/gt/intel_reset.c | 5 +---- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- > drivers/gpu/drm/i915/i915_gem.c | 13 +++++++++++-- > drivers/gpu/drm/i915/i915_request.c | 2 -- > 7 files changed, 22 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > index 5ab7df53c2a0..edd21d14e64f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > @@ -459,13 +459,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr > return NOTIFY_DONE; > } > > -/** > - * i915_gem_shrinker_register - Register the i915 shrinker > - * @i915: i915 device > - * > - * This function registers and sets up the i915 shrinker and OOM handler. > - */ > -void i915_gem_shrinker_register(struct drm_i915_private *i915) > +void i915_gem_driver_register__shrinker(struct drm_i915_private *i915) > { > i915->mm.shrinker.scan_objects = i915_gem_shrinker_scan; > i915->mm.shrinker.count_objects = i915_gem_shrinker_count; > @@ -480,13 +474,7 @@ void i915_gem_shrinker_register(struct drm_i915_private *i915) > WARN_ON(register_vmap_purge_notifier(&i915->mm.vmap_notifier)); > } > > -/** > - * i915_gem_shrinker_unregister - Unregisters the i915 shrinker > - * @i915: i915 device > - * > - * This function unregisters the i915 shrinker and OOM handler. > - */ > -void i915_gem_shrinker_unregister(struct drm_i915_private *i915) > +void i915_gem_driver_unregister__shrinker(struct drm_i915_private *i915) > { > WARN_ON(unregister_vmap_purge_notifier(&i915->mm.vmap_notifier)); > WARN_ON(unregister_oom_notifier(&i915->mm.oom_notifier)); > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > index 01857c12f12f..50aa7e95124d 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > @@ -382,7 +382,7 @@ static bool assert_mmap_offset(struct drm_i915_private *i915, > > static void disable_retire_worker(struct drm_i915_private *i915) > { > - i915_gem_shrinker_unregister(i915); > + i915_gem_driver_unregister__shrinker(i915); > > intel_gt_pm_get(&i915->gt); > > @@ -398,7 +398,7 @@ static void restore_retire_worker(struct drm_i915_private *i915) > igt_flush_test(i915, I915_WAIT_LOCKED); > mutex_unlock(&i915->drm.struct_mutex); > > - i915_gem_shrinker_register(i915); > + i915_gem_driver_register__shrinker(i915); > } > > static void mmap_offset_lock(struct drm_i915_private *i915) > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c > index 98c071fe532b..cdba6cd29327 100644 > --- a/drivers/gpu/drm/i915/gt/intel_reset.c > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c > @@ -757,11 +757,8 @@ static void __intel_gt_set_wedged(struct intel_gt *gt) > if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display) > __intel_gt_reset(gt, ALL_ENGINES); > > - for_each_engine(engine, gt->i915, id) { > + for_each_engine(engine, gt->i915, id) > engine->submit_request = nop_submit_request; > - engine->schedule = NULL; > - } > - gt->i915->caps.scheduler = 0; > > /* > * Make sure no request can slip through without getting completed by > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index d348d22bf0cf..77dc66d6e88f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1719,7 +1719,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = &dev_priv->drm; > > - i915_gem_shrinker_register(dev_priv); > + i915_gem_driver_register(dev_priv); > i915_pmu_register(dev_priv); > > /* > @@ -1799,7 +1799,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > i915_teardown_sysfs(dev_priv); > drm_dev_unplug(&dev_priv->drm); > > - i915_gem_shrinker_unregister(dev_priv); > + i915_gem_driver_unregister(dev_priv); > } > > static void i915_welcome_messages(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4fabf1a3df68..c241aae07b3e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2488,6 +2488,8 @@ static inline u32 i915_reset_engine_count(struct i915_gpu_error *error, > void i915_gem_init_mmio(struct drm_i915_private *i915); > int __must_check i915_gem_init(struct drm_i915_private *dev_priv); > int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv); > +void i915_gem_driver_register(struct drm_i915_private *i915); > +void i915_gem_driver_unregister(struct drm_i915_private *i915); > void i915_gem_driver_remove(struct drm_i915_private *dev_priv); > void i915_gem_driver_release(struct drm_i915_private *dev_priv); > int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv, > @@ -2587,8 +2589,8 @@ unsigned long i915_gem_shrink(struct drm_i915_private *i915, > #define I915_SHRINK_WRITEBACK BIT(4) > > unsigned long i915_gem_shrink_all(struct drm_i915_private *i915); > -void i915_gem_shrinker_register(struct drm_i915_private *i915); > -void i915_gem_shrinker_unregister(struct drm_i915_private *i915); > +void i915_gem_driver_register__shrinker(struct drm_i915_private *i915); > +void i915_gem_driver_unregister__shrinker(struct drm_i915_private *i915); > void i915_gem_shrinker_taints_mutex(struct drm_i915_private *i915, > struct mutex *mutex); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index eb34f3e5a74d..5ab1ddfef23c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1249,8 +1249,6 @@ int i915_gem_init_hw(struct drm_i915_private *i915) > > intel_mocs_init(gt); > > - intel_engines_set_scheduler_caps(i915); > - > out: > intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); > return ret; > @@ -1599,6 +1597,17 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > return ret; > } > > +void i915_gem_driver_register(struct drm_i915_private *i915) > +{ > + i915_gem_driver_register__shrinker(i915); > + intel_engines_set_scheduler_caps(i915); > +} > + > +void i915_gem_driver_unregister(struct drm_i915_private *i915) > +{ > + i915_gem_driver_unregister__shrinker(i915); > +} > + > void i915_gem_driver_remove(struct drm_i915_private *dev_priv) > { > GEM_BUG_ON(dev_priv->gt.awake); > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 8ac7d14ec8c9..81094f250bdb 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -1198,7 +1198,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) > */ > local_bh_disable(); > i915_sw_fence_commit(&rq->semaphore); > - rcu_read_lock(); /* RCU serialisation for set-wedged protection */ We don't need to protect against attr changes anymore so yes... > if (engine->schedule) { > struct i915_sched_attr attr = rq->gem_context->sched; > > @@ -1228,7 +1227,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) > > engine->schedule(rq, &attr); but will now schedule during wedged. Didn't notice anything that would blowup on reordering but is this intentional? -Mika > } > - rcu_read_unlock(); > i915_sw_fence_commit(&rq->submit); > local_bh_enable(); /* Kick the execlists tasklet if just scheduled */ > > -- > 2.23.0.rc1
Quoting Mika Kuoppala (2019-08-06 11:40:48) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > > index 8ac7d14ec8c9..81094f250bdb 100644 > > --- a/drivers/gpu/drm/i915/i915_request.c > > +++ b/drivers/gpu/drm/i915/i915_request.c > > @@ -1198,7 +1198,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) > > */ > > local_bh_disable(); > > i915_sw_fence_commit(&rq->semaphore); > > - rcu_read_lock(); /* RCU serialisation for set-wedged protection */ > > We don't need to protect against attr changes anymore so yes... > > > if (engine->schedule) { > > struct i915_sched_attr attr = rq->gem_context->sched; > > > > @@ -1228,7 +1227,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) > > > > engine->schedule(rq, &attr); > > but will now schedule during wedged. Didn't notice anything that > would blowup on reordering but is this intentional? How do you think it will blow up? engine->schedule remains untouched over wedged, and all it is doing is traversing the dependency lists (which remain intact) and the scheduler list (which is controlled by locking and cleared inside engine->cancel_requests, after which point it will remain clear as nop_submit_request should not care). I don't think there's a bad interaction there between i915_schedule() and nop_submit_request... -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2019-08-06 11:40:48) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c >> > index 8ac7d14ec8c9..81094f250bdb 100644 >> > --- a/drivers/gpu/drm/i915/i915_request.c >> > +++ b/drivers/gpu/drm/i915/i915_request.c >> > @@ -1198,7 +1198,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) >> > */ >> > local_bh_disable(); >> > i915_sw_fence_commit(&rq->semaphore); >> > - rcu_read_lock(); /* RCU serialisation for set-wedged protection */ >> >> We don't need to protect against attr changes anymore so yes... >> >> > if (engine->schedule) { >> > struct i915_sched_attr attr = rq->gem_context->sched; >> > >> > @@ -1228,7 +1227,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) >> > >> > engine->schedule(rq, &attr); >> >> but will now schedule during wedged. Didn't notice anything that >> would blowup on reordering but is this intentional? > > How do you think it will blow up? engine->schedule remains untouched > over wedged, and all it is doing is traversing the dependency lists > (which remain intact) and the scheduler list (which is controlled by > locking and cleared inside engine->cancel_requests, after which point it > will remain clear as nop_submit_request should not care). > > I don't think there's a bad interaction there between i915_schedule() > and nop_submit_request... Didn't find anything that would blow up. Just a notable change in behaviour so tried to poke around a bit. The less we special case the better so nothing against the idea. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > -Chris
Quoting Patchwork (2019-08-06 21:14:15) > #### Possible regressions #### > > * igt@gem_eio@in-flight-contexts-10ms: > - shard-snb: [PASS][1] -> [FAIL][2] +2 similar issues > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-snb6/igt@gem_eio@in-flight-contexts-10ms.html > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13884/shard-snb1/igt@gem_eio@in-flight-contexts-10ms.html Broke something in the legacy submission code, probably fiddling with the ring/timeline. > * igt@i915_selftest@mock_requests: > - shard-snb: [PASS][9] -> [DMESG-WARN][10] > [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-snb4/igt@i915_selftest@mock_requests.html > [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13884/shard-snb7/igt@i915_selftest@mock_requests.html And a use-after-free in the mock, probably also the ring/timeline fiddling. -Chris
Quoting Chris Wilson (2019-08-06 21:20:31) > Quoting Patchwork (2019-08-06 21:14:15) > > #### Possible regressions #### > > > > * igt@gem_eio@in-flight-contexts-10ms: > > - shard-snb: [PASS][1] -> [FAIL][2] +2 similar issues > > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-snb6/igt@gem_eio@in-flight-contexts-10ms.html > > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13884/shard-snb1/igt@gem_eio@in-flight-contexts-10ms.html > > Broke something in the legacy submission code, probably fiddling with > the ring/timeline. No, it's just the test made an assumption that no longer applies. https://patchwork.freedesktop.org/patch/320400/?series=64381&rev=1 -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c index 5ab7df53c2a0..edd21d14e64f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c @@ -459,13 +459,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr return NOTIFY_DONE; } -/** - * i915_gem_shrinker_register - Register the i915 shrinker - * @i915: i915 device - * - * This function registers and sets up the i915 shrinker and OOM handler. - */ -void i915_gem_shrinker_register(struct drm_i915_private *i915) +void i915_gem_driver_register__shrinker(struct drm_i915_private *i915) { i915->mm.shrinker.scan_objects = i915_gem_shrinker_scan; i915->mm.shrinker.count_objects = i915_gem_shrinker_count; @@ -480,13 +474,7 @@ void i915_gem_shrinker_register(struct drm_i915_private *i915) WARN_ON(register_vmap_purge_notifier(&i915->mm.vmap_notifier)); } -/** - * i915_gem_shrinker_unregister - Unregisters the i915 shrinker - * @i915: i915 device - * - * This function unregisters the i915 shrinker and OOM handler. - */ -void i915_gem_shrinker_unregister(struct drm_i915_private *i915) +void i915_gem_driver_unregister__shrinker(struct drm_i915_private *i915) { WARN_ON(unregister_vmap_purge_notifier(&i915->mm.vmap_notifier)); WARN_ON(unregister_oom_notifier(&i915->mm.oom_notifier)); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index 01857c12f12f..50aa7e95124d 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -382,7 +382,7 @@ static bool assert_mmap_offset(struct drm_i915_private *i915, static void disable_retire_worker(struct drm_i915_private *i915) { - i915_gem_shrinker_unregister(i915); + i915_gem_driver_unregister__shrinker(i915); intel_gt_pm_get(&i915->gt); @@ -398,7 +398,7 @@ static void restore_retire_worker(struct drm_i915_private *i915) igt_flush_test(i915, I915_WAIT_LOCKED); mutex_unlock(&i915->drm.struct_mutex); - i915_gem_shrinker_register(i915); + i915_gem_driver_register__shrinker(i915); } static void mmap_offset_lock(struct drm_i915_private *i915) diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index 98c071fe532b..cdba6cd29327 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -757,11 +757,8 @@ static void __intel_gt_set_wedged(struct intel_gt *gt) if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display) __intel_gt_reset(gt, ALL_ENGINES); - for_each_engine(engine, gt->i915, id) { + for_each_engine(engine, gt->i915, id) engine->submit_request = nop_submit_request; - engine->schedule = NULL; - } - gt->i915->caps.scheduler = 0; /* * Make sure no request can slip through without getting completed by diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d348d22bf0cf..77dc66d6e88f 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1719,7 +1719,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm; - i915_gem_shrinker_register(dev_priv); + i915_gem_driver_register(dev_priv); i915_pmu_register(dev_priv); /* @@ -1799,7 +1799,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) i915_teardown_sysfs(dev_priv); drm_dev_unplug(&dev_priv->drm); - i915_gem_shrinker_unregister(dev_priv); + i915_gem_driver_unregister(dev_priv); } static void i915_welcome_messages(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4fabf1a3df68..c241aae07b3e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2488,6 +2488,8 @@ static inline u32 i915_reset_engine_count(struct i915_gpu_error *error, void i915_gem_init_mmio(struct drm_i915_private *i915); int __must_check i915_gem_init(struct drm_i915_private *dev_priv); int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv); +void i915_gem_driver_register(struct drm_i915_private *i915); +void i915_gem_driver_unregister(struct drm_i915_private *i915); void i915_gem_driver_remove(struct drm_i915_private *dev_priv); void i915_gem_driver_release(struct drm_i915_private *dev_priv); int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv, @@ -2587,8 +2589,8 @@ unsigned long i915_gem_shrink(struct drm_i915_private *i915, #define I915_SHRINK_WRITEBACK BIT(4) unsigned long i915_gem_shrink_all(struct drm_i915_private *i915); -void i915_gem_shrinker_register(struct drm_i915_private *i915); -void i915_gem_shrinker_unregister(struct drm_i915_private *i915); +void i915_gem_driver_register__shrinker(struct drm_i915_private *i915); +void i915_gem_driver_unregister__shrinker(struct drm_i915_private *i915); void i915_gem_shrinker_taints_mutex(struct drm_i915_private *i915, struct mutex *mutex); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index eb34f3e5a74d..5ab1ddfef23c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1249,8 +1249,6 @@ int i915_gem_init_hw(struct drm_i915_private *i915) intel_mocs_init(gt); - intel_engines_set_scheduler_caps(i915); - out: intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); return ret; @@ -1599,6 +1597,17 @@ int i915_gem_init(struct drm_i915_private *dev_priv) return ret; } +void i915_gem_driver_register(struct drm_i915_private *i915) +{ + i915_gem_driver_register__shrinker(i915); + intel_engines_set_scheduler_caps(i915); +} + +void i915_gem_driver_unregister(struct drm_i915_private *i915) +{ + i915_gem_driver_unregister__shrinker(i915); +} + void i915_gem_driver_remove(struct drm_i915_private *dev_priv) { GEM_BUG_ON(dev_priv->gt.awake); diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 8ac7d14ec8c9..81094f250bdb 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1198,7 +1198,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) */ local_bh_disable(); i915_sw_fence_commit(&rq->semaphore); - rcu_read_lock(); /* RCU serialisation for set-wedged protection */ if (engine->schedule) { struct i915_sched_attr attr = rq->gem_context->sched; @@ -1228,7 +1227,6 @@ struct i915_request *__i915_request_commit(struct i915_request *rq) engine->schedule(rq, &attr); } - rcu_read_unlock(); i915_sw_fence_commit(&rq->submit); local_bh_enable(); /* Kick the execlists tasklet if just scheduled */
We do not notify userspace when the scheduler capabilities are changed (due to wedging the driver) and as such userspace will expect the caps to be static and unchanging. Make it so, and so we only need to compute our caps once during driver registration. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 16 ++-------------- .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 4 ++-- drivers/gpu/drm/i915/gt/intel_reset.c | 5 +---- drivers/gpu/drm/i915/i915_drv.c | 4 ++-- drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- drivers/gpu/drm/i915/i915_gem.c | 13 +++++++++++-- drivers/gpu/drm/i915/i915_request.c | 2 -- 7 files changed, 22 insertions(+), 28 deletions(-)