Re: [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Imre Deak <imre.deak@intel.com>]

On to, 2015-11-12 at 17:04 +0000, Chris Wilson wrote:
> On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 825114a..ee3ef69 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2962,6 +2962,9 @@ static void i915_hangcheck_elapsed(struct
> > work_struct *work)
> >  	if (!i915.enable_hangcheck)
> >  		return;
> >  
> > +	assert_rpm_device_not_suspended(dev_priv);
> > +	disable_rpm_asserts(dev_priv);
> > +
> >  	for_each_ring(ring, dev_priv, i) {
> >  		u64 acthd;
> >  		u32 seqno;
> > @@ -3053,13 +3056,18 @@ static void i915_hangcheck_elapsed(struct
> > work_struct *work)
> >  		}
> >  	}
> >  
> > -	if (rings_hung)
> > -		return i915_handle_error(dev, true, "Ring hung");
> > +	if (rings_hung) {
> > +		i915_handle_error(dev, true, "Ring hung");
> > +		goto out;
> > +	}
> >  
> >  	if (busy_count)
> >  		/* Reset timer case chip hangs without another
> > request
> >  		 * being added */
> >  		i915_queue_hangcheck(dev);
> > +
> > +out:
> > +	enable_rpm_asserts(dev_priv);
> 
> Nice catch!

It triggered the new assert easily just before going to runtime
suspend..

> Since the rpm wakelock here is covered by
> intel_mark_busy/intel_mark_idle(), we should be able to do something
> like:
> 
> if (!intel_runtime_pm_tryget()
> 	return;
> 
> where intel_runtime_pm_tryget does something like
> atomic_inc_unless_zero().
> 
> Is something like that possible?

Yea, I could add this, but I'd like to better understand the need, see
below.

> As it stands since we don't actually cancel the hangcheck when we
drop
> the rpm wakelock in intel_mark_idle() it can very well come to pass
> that
> we execute this whilst the device is asleep. However, if the device
> is
> alseep, we now that we are no longer executing.

But how could this run while asleep, since we flush the work in the
runtime suspend handler before turning off the HW? But even if it can't
run your idea would be clearer imo..

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