[Intel-gfx] [PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Apr 6 17:40:10 UTC 2017
On 06/04/2017 10:30, Chris Wilson wrote:
> If we attempt to wake up a waiter, who is currently checking the seqno
> it will be in the TASK_INTERRUPTIBLE state and ttwu will report success.
> However, it is actually awake and functioning -- so delay reporting the
> actual wake up until it sleeps.
>
> v2: Defend against !CONFIG_SMP
> v3: Don't filter out calls to wake_up_process
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 9ccbf26124c6..808d3a3cda0a 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -27,6 +27,12 @@
>
> #include "i915_drv.h"
>
> +#ifdef CONFIG_SMP
> +#define task_asleep(tsk) (!(tsk)->on_cpu)
> +#else
> +#define task_asleep(tsk) ((tsk) != current)
> +#endif
It really bothers to fish into this low level info which probably isn't
intended to be used from the outside.
> +
> static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> {
> struct intel_wait *wait;
> @@ -37,8 +43,16 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> wait = b->irq_wait;
> if (wait) {
> result = ENGINE_WAKEUP_WAITER;
> - if (wake_up_process(wait->tsk))
> +
> + /* Be careful not to report a successful wakeup if the waiter
> + * is currently processing the seqno, where it will have
> + * already called set_task_state(TASK_INTERRUPTIBLE).
> + */
> + if (task_asleep(wait->tsk))
> result |= ENGINE_WAKEUP_ASLEEP;
And this still has the problem of not being atomic between reporting the
two flags. So the reported status can be false which also bothers me.
I will need to take some more time thinking about this.
Regards,
Tvrtko
> +
> + if (wake_up_process(wait->tsk))
> + result |= ENGINE_WAKEUP_SUCCESS;
> }
>
> return result;
> @@ -98,7 +112,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
> * but we still have a waiter. Assuming all batches complete within
> * DRM_I915_HANGCHECK_JIFFIES [1.5s]!
> */
> - if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
> + if (intel_engine_wakeup(engine) == ENGINE_WAKEUP) {
> missed_breadcrumb(engine);
> mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> } else {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index cbe61d3f31da..974a5928bec9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -663,6 +663,10 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
> unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
> #define ENGINE_WAKEUP_WAITER BIT(0)
> #define ENGINE_WAKEUP_ASLEEP BIT(1)
> +#define ENGINE_WAKEUP_SUCCESS BIT(2)
> +#define ENGINE_WAKEUP (ENGINE_WAKEUP_WAITER | \
> + ENGINE_WAKEUP_ASLEEP | \
> + ENGINE_WAKEUP_SUCCESS)
>
> void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
> void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>
More information about the Intel-gfx
mailing list