Re: [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup()

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

On 24/02/2017 18:01, Chris Wilson wrote:
> The two users of the return value from intel_engine_wakeup() are
> expecting different results. In the breadcrumbs hangcheck, we are using
> it to determine whether wake_up_process() detected the waiter was
> currently running (and if so we presume that it hasn't yet missed the
> interrupt). However, in the fake_irq path, we are using the return value
> as a check as to whether there are any waiters, and so we may
> incorrectly stop the fake-irq if that waiter was currently running.
>
> To handle the two different needs, return both bits of information!
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 28 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h  | 26 +++-----------------------
>  2 files changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 027c93e34c97..64e1b0c2d8b6 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -26,6 +26,32 @@
>
>  #include "i915_drv.h"
>
> +unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)

How come you decided to un-inline it? Changes shouldn't have made a big 
difference to its size I would have thought.

> +{
> +	unsigned int ret = 0;

Optional bikeshed - maybe call it res or something, since ret is usually 
used for < 0 || 0.

> +
> +	/* Note that for this not to dangerously chase a dangling pointer,
> +	 * we must hold the rcu_read_lock here.
> +	 *
> +	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
> +	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
> +	 * is unlikely to be beneficial.
> +	 */
> +	if (intel_engine_has_waiter(engine)) {
> +		struct task_struct *tsk;
> +
> +		ret = ENGINE_WAKEUP_WAITER;
> +
> +		rcu_read_lock();
> +		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> +		if (tsk && !wake_up_process(tsk))
> +			ret |= ENGINE_WAKEUP_ACTIVE;
> +		rcu_read_unlock();
> +	}
> +
> +	return ret;
> +}
> +
>  static unsigned long wait_timeout(void)
>  {
>  	return round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES);
> @@ -49,7 +75,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	 * to process the pending interrupt (e.g, low priority task on a loaded
>  	 * system) and wait until it sleeps before declaring a missed interrupt.
>  	 */
> -	if (!intel_engine_wakeup(engine)) {
> +	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
>  		mod_timer(&b->hangcheck, wait_timeout());
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 0f29e07a9581..7d753dc1b89d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -642,29 +642,9 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
>  	return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
>  }
>
> -static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
> -{
> -	bool wakeup = false;
> -
> -	/* Note that for this not to dangerously chase a dangling pointer,
> -	 * we must hold the rcu_read_lock here.
> -	 *
> -	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
> -	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
> -	 * is unlikely to be beneficial.
> -	 */
> -	if (intel_engine_has_waiter(engine)) {
> -		struct task_struct *tsk;
> -
> -		rcu_read_lock();
> -		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> -		if (tsk)
> -			wakeup = wake_up_process(tsk);
> -		rcu_read_unlock();
> -	}
> -
> -	return wakeup;
> -}
> +unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
> +#define ENGINE_WAKEUP_WAITER BIT(0)
> +#define ENGINE_WAKEUP_ACTIVE BIT(1)
>
>  void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx