Re: [PATCH 12/27] drm/i915: Only report a wakeup if the waiter was truly asleep

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/04/2017 10:41, 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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@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
> +
>  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;
> +
> +		if (wake_up_process(wait->tsk))
> +			result |= ENGINE_WAKEUP_SUCCESS;

The rough idea I had of atomic_inc(&b->wakeup_cnt) here with 
unconditional wake_up_process, coupled with atomic_dec_and_test in the 
signaler would not work? I was thinking that would avoid signaler losing 
the wakeup and avoid us having to touch the low level scheduler data.

Or what you meant last time by not sure it was worth it was referring to 
the above?

Regards,

Tvrtko

>  	}
>
>  	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 00d36aa4e26d..d25b88467e5e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -668,6 +668,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);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx