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

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

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@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

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);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx