[Intel-gfx] [PATCH 2/2] drm/i915: Reset hangcheck timeouts upon idling

[Chris Wilson]

On Wed, Apr 19, 2017 at 05:09:46PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > If we have a long period of idleness, we turn off the hangcheck timer
> > and stop polling the hardware. Before we restart the hangcheck, we
> > should clear the previous timestamps to prevent us thinking that the
> > engine was stalled for a long time, if the seqno were manipulated
> > carefully (such as the repeating patterns in gem_exec_whisper).
> >
> > It should have no impact upon normal use.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hangcheck.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
> > index b0ca0c4c70d9..a74decca5109 100644
> > --- a/drivers/gpu/drm/i915/intel_hangcheck.c
> > +++ b/drivers/gpu/drm/i915/intel_hangcheck.c
> > @@ -409,13 +409,13 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> >  	int busy_count = 0;
> >  
> >  	if (!i915.enable_hangcheck)
> > -		return;
> > +		goto disarm_hangcheck;
> >  
> >  	if (!READ_ONCE(dev_priv->gt.awake))
> > -		return;
> > +		goto disarm_hangcheck;
> >  
> >  	if (i915_terminally_wedged(&dev_priv->gpu_error))
> > -		return;
> > +		goto disarm_hangcheck;
> >  
> >  	/* As enabling the GPU requires fairly extensive mmio access,
> >  	 * periodically arm the mmio checker to see if we are triggering
> > @@ -446,8 +446,14 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> >  		hangcheck_declare_hang(dev_priv, hung, stuck);
> >  
> >  	/* Reset timer in case GPU hangs without another request being added */
> > -	if (busy_count)
> > +	if (busy_count) {
> >  		i915_queue_hangcheck(dev_priv);
> 
> Now if we don't have a waiter, we always init hangcheck. And thus
> we never detect a hang if so. As demonstrated by the
> gem_busy at basic-default-hang.
> 
> I suggest we decouple the waiters completely from hangcheck:
> 
> -               const bool busy = intel_engine_has_waiter(engine);
> +               const bool busy = engine->timeline->inflight_seqnos;

inflight seqnos isn't a good choice either, as that doesn't mean the
engine is active yet. The only issue with this patch was resetting the
hangcheck.seqno nerfed the waiterless hangcheck. Turned out to be a very
bad idea.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre