On pe, 2016-06-03 at 17:36 +0100, Chris Wilson wrote:
> The retire worker is a low frequency task that makes sure we retire
> outstanding requests if userspace is being lax. We only need to start it
> once as it remains active until the GPU is idle, so do a cheap test
> before the more expensive queue_work(). A consequence of this is that we
> need correct locking in the worker to make the hot path of request
> submission cheap. To keep the symmetry and keep hangcheck strictly bound
> by the GPU's wakelock, we move the cancel_sync(hangcheck) to the idle
> worker before dropping the wakelock.
>
> v2: Guard against RCU fouling the breadcrumbs bottom-half whilst we kick
> the waiter.
> v3: Remove the wakeref assertion squelching (now we hold a wakeref for
> the hangcheck, any rpm error there is genuine).
> v4: To prevent excess work when retiring requests, we split the busy
> flag into two, a boolean to denote whether we hold the wakeref and a
> bitmask of active engines.
>
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=88437
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.c | 2 -
> drivers/gpu/drm/i915/i915_drv.h | 56 +++++++-------
> drivers/gpu/drm/i915/i915_gem.c | 114
> ++++++++++++++++++-----------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++
> drivers/gpu/drm/i915/i915_irq.c | 15 +---
> drivers/gpu/drm/i915/intel_display.c | 26 -------
> drivers/gpu/drm/i915/intel_pm.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +-
> 9 files changed, 115 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 72dae6fb0aa2..dd6cf222e8f5 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2437,7 +2437,8 @@ static int i915_rps_boost_info(struct seq_file *m, void
> *data)
> struct drm_file *file;
>
> seq_printf(m, "RPS enabled? %d\n", dev_priv->rps.enabled);
> - seq_printf(m, "GPU busy? %d\n", dev_priv->mm.busy);
> + seq_printf(m, "GPU busy? %s [%x]\n",
> + yesno(dev_priv->gt.awake), dev_priv->gt.active_engines);
> seq_printf(m, "CPU waiting? %d\n", count_irq_waiters(dev_priv));
> seq_printf(m, "Frequency requested %d; min hard:%d, soft:%d; max
> soft:%d, hard:%d\n",
> intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq),
> @@ -2777,7 +2778,7 @@ static int i915_runtime_pm_status(struct seq_file *m,
> void *unused)
> if (!HAS_RUNTIME_PM(dev_priv))
> seq_puts(m, "Runtime power management not supported\n");
>
> - seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
> + seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
> seq_printf(m, "IRQs disabled: %s\n",
> yesno(!intel_irqs_enabled(dev_priv)));
> #ifdef CONFIG_PM
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3c8c75c77574..5f7208d2fdbf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2697,8 +2697,6 @@ static int intel_runtime_suspend(struct device *device)
> i915_gem_release_all_mmaps(dev_priv);
> mutex_unlock(&dev->struct_mutex);
>
> - cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> -
> intel_guc_suspend(dev);
>
> intel_suspend_gt_powersave(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 88d9242398ce..3f075adf9e84 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1305,37 +1305,11 @@ struct i915_gem_mm {
> struct list_head fence_list;
>
> /**
> - * We leave the user IRQ off as much as possible,
> - * but this means that requests will finish and never
> - * be retired once the system goes idle. Set a timer to
> - * fire periodically while the ring is running. When it
> - * fires, go retire requests.
> - */
> - struct delayed_work retire_work;
> -
> - /**
> - * When we detect an idle GPU, we want to turn on
> - * powersaving features. So once we see that there
> - * are no more requests outstanding and no more
> - * arrive within a small period of time, we fire
> - * off the idle_work.
> - */
> - struct delayed_work idle_work;
> -
> - /**
> * Are we in a non-interruptible section of code like
> * modesetting?
> */
> bool interruptible;
>
> - /**
> - * Is the GPU currently considered idle, or busy executing userspace
> - * requests? Whilst idle, we attempt to power down the hardware and
> - * display clocks. In order to reduce the effect on performance, there
> - * is a slight delay before we do so.
> - */
> - bool busy;
> -
> /* the indicator for dispatch video commands on two BSD rings */
> unsigned int bsd_ring_dispatch_index;
>
> @@ -2034,6 +2008,34 @@ struct drm_i915_private {
> int (*init_engines)(struct drm_device *dev);
> void (*cleanup_engine)(struct intel_engine_cs *engine);
> void (*stop_engine)(struct intel_engine_cs *engine);
> +
> + /**
> + * Is the GPU currently considered idle, or busy executing
> + * userspace requests? Whilst idle, we allow runtime power
> + * management to power down the hardware and display clocks.
> + * In order to reduce the effect on performance, there
> + * is a slight delay before we do so.
> + */
> + unsigned active_engines;
> + bool awake;
> +
> + /**
> + * We leave the user IRQ off as much as possible,
> + * but this means that requests will finish and never
> + * be retired once the system goes idle. Set a timer to
> + * fire periodically while the ring is running. When it
> + * fires, go retire requests.
> + */
> + struct delayed_work retire_work;
> +
> + /**
> + * When we detect an idle GPU, we want to turn on
> + * powersaving features. So once we see that there
> + * are no more requests outstanding and no more
> + * arrive within a small period of time, we fire
> + * off the idle_work.
> + */
> + struct delayed_work idle_work;
Code motion would be cool in separate patches, but well it's a 62 patch
series already.
> } gt;
>
> /* perform PHY state sanity checks? */
> @@ -3247,7 +3249,7 @@ int __must_check i915_gem_set_seqno(struct drm_device
> *dev, u32 seqno);
> struct drm_i915_gem_request *
> i915_gem_find_active_request(struct intel_engine_cs *engine);
>
> -bool i915_gem_retire_requests(struct drm_i915_private *dev_priv);
> +void i915_gem_retire_requests(struct drm_i915_private *dev_priv);
> void i915_gem_retire_requests_ring(struct intel_engine_cs *engine);
>
> static inline u32 i915_reset_counter(struct i915_gpu_error *error)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f4e550ddaa5d..5a7131b749a2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2554,6 +2554,26 @@ i915_gem_get_seqno(struct drm_i915_private *dev_priv,
> u32 *seqno)
> return 0;
> }
>
> +static void i915_gem_mark_busy(struct drm_i915_private *dev_priv,
> + const struct intel_engine_cs *engine)
> +{
> + dev_priv->gt.active_engines |= intel_engine_flag(engine);
> + if (dev_priv->gt.awake)
> + return;
> +
> + intel_runtime_pm_get_noresume(dev_priv);
> + dev_priv->gt.awake = true;
> +
> + intel_enable_gt_powersave(dev_priv);
> + i915_update_gfx_val(dev_priv);
> + if (INTEL_INFO(dev_priv)->gen >= 6)
> + gen6_rps_busy(dev_priv);
> +
> + queue_delayed_work(dev_priv->wq,
> + &dev_priv->gt.retire_work,
> + round_jiffies_up_relative(HZ));
> +}
> +
> /*
> * NB: This function is not allowed to fail. Doing so would mean the the
> * request is not being tracked for completion but the work itself is
> @@ -2640,12 +2660,6 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
> }
> /* Not allowed to fail! */
> WARN(ret, "emit|add_request failed: %d!\n", ret);
> -
> - queue_delayed_work(dev_priv->wq,
> - &dev_priv->mm.retire_work,
> - round_jiffies_up_relative(HZ));
> - intel_mark_busy(dev_priv);
> -
> /* Sanity check that the reserved size was large enough. */
> ret = intel_ring_get_tail(ringbuf) - request_start;
> if (ret < 0)
> @@ -2654,6 +2668,8 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
> "Not enough space reserved (%d bytes) "
> "for adding the request (%d bytes)\n",
> reserved_tail, ret);
> +
> + i915_gem_mark_busy(dev_priv, engine);
> }
>
> static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
> @@ -2968,46 +2984,47 @@ i915_gem_retire_requests_ring(struct intel_engine_cs
> *engine)
> WARN_ON(i915_verify_lists(engine->dev));
> }
>
> -bool
> -i915_gem_retire_requests(struct drm_i915_private *dev_priv)
> +void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> - bool idle = true;
> +
> + if (dev_priv->gt.active_engines == 0)
> + return;
> +
> + GEM_BUG_ON(!dev_priv->gt.awake);
>
> for_each_engine(engine, dev_priv) {
> i915_gem_retire_requests_ring(engine);
> - idle &= list_empty(&engine->request_list);
> - if (i915.enable_execlists) {
> - spin_lock_bh(&engine->execlist_lock);
> - idle &= list_empty(&engine->execlist_queue);
> - spin_unlock_bh(&engine->execlist_lock);
> - }
As discussed in IRC, this disappearing could be mentioned in the commit
message.
> + if (list_empty(&engine->request_list))
> + dev_priv->gt.active_engines &=
> ~intel_engine_flag(engine);
> }
>
> - if (idle)
> + if (dev_priv->gt.active_engines == 0)
> mod_delayed_work(dev_priv->wq,
> - &dev_priv->mm.idle_work,
> + &dev_priv->gt.idle_work,
> msecs_to_jiffies(100));
> -
> - return idle;
> }
>
> static void
> i915_gem_retire_work_handler(struct work_struct *work)
> {
> struct drm_i915_private *dev_priv =
> - container_of(work, typeof(*dev_priv), mm.retire_work.work);
> + container_of(work, typeof(*dev_priv), gt.retire_work.work);
> struct drm_device *dev = dev_priv->dev;
> - bool idle;
>
> /* Come back later if the device is busy... */
> - idle = false;
> if (mutex_trylock(&dev->struct_mutex)) {
> - idle = i915_gem_retire_requests(dev_priv);
> + i915_gem_retire_requests(dev_priv);
> mutex_unlock(&dev->struct_mutex);
> }
> - if (!idle)
> - queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
> +
> + /* Keep the retire handler running until we are finally idle.
> + * We do not need to do this test under locking as in the worst-case
> + * we queue the retire worker once too often.
> + */
> + if (READ_ONCE(dev_priv->gt.awake))
This is the only occurrance in this function, so don't think we need
READ_ONCE. Not sure if READ_ONCE is good documentation of "read outside
lock", comment might be better.
> + queue_delayed_work(dev_priv->wq,
> + &dev_priv->gt.retire_work,
> round_jiffies_up_relative(HZ));
> }
>
> @@ -3015,25 +3032,36 @@ static void
> i915_gem_idle_work_handler(struct work_struct *work)
> {
> struct drm_i915_private *dev_priv =
> - container_of(work, typeof(*dev_priv), mm.idle_work.work);
> + container_of(work, typeof(*dev_priv), gt.idle_work.work);
> struct drm_device *dev = dev_priv->dev;
> struct intel_engine_cs *engine;
>
> - for_each_engine(engine, dev_priv)
> - if (!list_empty(&engine->request_list))
> - return;
> + if (!READ_ONCE(dev_priv->gt.awake))
> + return;
>
> - /* we probably should sync with hangcheck here, using cancel_work_sync.
> - * Also locking seems to be fubar here, engine->request_list is
> protected
> - * by dev->struct_mutex. */
> + mutex_lock(&dev->struct_mutex);
> + if (dev_priv->gt.active_engines)
> + goto out;
>
> - intel_mark_idle(dev_priv);
> + for_each_engine(engine, dev_priv)
> + i915_gem_batch_pool_fini(&engine->batch_pool);
>
> - if (mutex_trylock(&dev->struct_mutex)) {
> - for_each_engine(engine, dev_priv)
> - i915_gem_batch_pool_fini(&engine->batch_pool);
> + GEM_BUG_ON(!dev_priv->gt.awake);
> + dev_priv->gt.awake = false;
>
> - mutex_unlock(&dev->struct_mutex);
> + if (INTEL_INFO(dev_priv)->gen >= 6)
> + gen6_rps_idle(dev_priv);
> + intel_runtime_pm_put(dev_priv);
> +out:
> + mutex_unlock(&dev->struct_mutex);
> +
> + if (!dev_priv->gt.awake &&
No READ_ONCE here, even we just unlocked the mutex. So lacks some
consistency.
Also, this assumes we might be pre-empted between unlocking mutex and
making this test, so I'm little bit confused. Do you want to optimize
by avoiding calling cancel_delayed_work_sync?
> + cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work)) {
> + unsigned stuck = intel_kick_waiters(dev_priv);
> + if (unlikely(stuck)) {
> + DRM_DEBUG_DRIVER("kicked stuck waiters...missed irq\n");
> + dev_priv->gpu_error.missed_irq_rings |= stuck;
> + }
> }
> }
>
> @@ -4154,7 +4182,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct
> drm_file *file)
>
> ret = __i915_wait_request(target, true, NULL, NULL);
> if (ret == 0)
> - queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
> + queue_delayed_work(dev_priv->wq, &dev_priv->gt.retire_work, 0);
>
> i915_gem_request_unreference(target);
>
> @@ -4672,13 +4700,13 @@ i915_gem_suspend(struct drm_device *dev)
> mutex_unlock(&dev->struct_mutex);
>
> cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> - cancel_delayed_work_sync(&dev_priv->mm.retire_work);
> - flush_delayed_work(&dev_priv->mm.idle_work);
> + cancel_delayed_work_sync(&dev_priv->gt.retire_work);
> + flush_delayed_work(&dev_priv->gt.idle_work);
>
> /* Assert that we sucessfully flushed all the work and
> * reset the GPU back to its idle, low power state.
> */
> - WARN_ON(dev_priv->mm.busy);
> + WARN_ON(dev_priv->gt.awake);
>
> return 0;
>
> @@ -4982,9 +5010,9 @@ i915_gem_load_init(struct drm_device *dev)
> init_engine_lists(&dev_priv->engine[i]);
> for (i = 0; i < I915_MAX_NUM_FENCES; i++)
> INIT_LIST_HEAD(&dev_priv->fence_regs[i].lru_list);
> - INIT_DELAYED_WORK(&dev_priv->mm.retire_work,
> + INIT_DELAYED_WORK(&dev_priv->gt.retire_work,
> i915_gem_retire_work_handler);
> - INIT_DELAYED_WORK(&dev_priv->mm.idle_work,
> + INIT_DELAYED_WORK(&dev_priv->gt.idle_work,
> i915_gem_idle_work_handler);
> init_waitqueue_head(&dev_priv->gpu_error.wait_queue);
> init_waitqueue_head(&dev_priv->gpu_error.reset_queue);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 8097698b9622..d3297dab0298 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1477,6 +1477,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
> *data,
> dispatch_flags |= I915_DISPATCH_RS;
> }
>
> + /* Take a local wakeref for preparing to dispatch the execbuf as
> + * we expect to access the hardware fairly frequently in the
> + * process. Upon first dispatch, we acquire another prolonged
> + * wakeref that we hold until the GPU has been idle for at least
> + * 100ms.
> + */
> intel_runtime_pm_get(dev_priv);
>
> ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f74f5727ea77..7a2dc8f1f64e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3102,12 +3102,8 @@ static void i915_hangcheck_elapsed(struct work_struct
> *work)
> if (!i915.enable_hangcheck)
> return;
>
> - /*
> - * The hangcheck work is synced during runtime suspend, we don't
> - * require a wakeref. TODO: instead of disabling the asserts make
> - * sure that we hold a reference when this work is running.
> - */
> - DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
> + if (!READ_ONCE(dev_priv->gt.awake))
> + return;
>
> /* As enabling the GPU requires fairly extensive mmio access,
> * periodically arm the mmio checker to see if we are triggering
> @@ -3215,17 +3211,12 @@ static void i915_hangcheck_elapsed(struct work_struct
> *work)
> }
> }
>
> - if (rings_hung) {
> + if (rings_hung)
> i915_handle_error(dev_priv, rings_hung, "Engine(s) hung");
> - goto out;
> - }
>
> /* Reset timer in case GPU hangs without another request being added */
> if (busy_count)
> i915_queue_hangcheck(dev_priv);
> -
> -out:
> - ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
> }
>
> static void ibx_irq_reset(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index bb09ee6d1a3f..14e41fdd8112 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10969,32 +10969,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct
> drm_device *dev,
> return mode;
> }
>
> -void intel_mark_busy(struct drm_i915_private *dev_priv)
> -{
> - if (dev_priv->mm.busy)
> - return;
> -
> - intel_runtime_pm_get(dev_priv);
> - intel_enable_gt_powersave(dev_priv);
> - i915_update_gfx_val(dev_priv);
> - if (INTEL_GEN(dev_priv) >= 6)
> - gen6_rps_busy(dev_priv);
> - dev_priv->mm.busy = true;
> -}
> -
> -void intel_mark_idle(struct drm_i915_private *dev_priv)
> -{
> - if (!dev_priv->mm.busy)
> - return;
> -
> - dev_priv->mm.busy = false;
> -
> - if (INTEL_GEN(dev_priv) >= 6)
> - gen6_rps_idle(dev_priv);
> -
> - intel_runtime_pm_put(dev_priv);
> -}
> -
> static void intel_crtc_destroy(struct drm_crtc *crtc)
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 712bd0debb91..35bb9a23cd2d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4850,7 +4850,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> /* This is intentionally racy! We peek at the state here, then
> * validate inside the RPS worker.
> */
> - if (!(dev_priv->mm.busy &&
> + if (!(dev_priv->gt.awake &&
> dev_priv->rps.enabled &&
> dev_priv->rps.cur_freq < dev_priv->rps.max_freq_softlimit))
> return;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 166f1a3829b0..d0cd9a1aa80e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -372,13 +372,13 @@ struct intel_engine_cs {
> };
>
> static inline bool
> -intel_engine_initialized(struct intel_engine_cs *engine)
> +intel_engine_initialized(const struct intel_engine_cs *engine)
> {
> return engine->i915 != NULL;
> }
>
> static inline unsigned
> -intel_engine_flag(struct intel_engine_cs *engine)
> +intel_engine_flag(const struct intel_engine_cs *engine)
> {
> return 1 << engine->id;
> }
I think majority of our functions are not const-correct, I remember
some grunting on the subject when I tried to change some to be. But I'm
all for it myself.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
