[Intel-gfx] [PATCH 07/19] drm/i915: add runtime put/get calls at the basic places

[Paulo Zanoni]

2013/12/10 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Nov 29, 2013 at 11:03:38AM -0200, Rodrigo Vivi wrote:
>> On Wed, Nov 27, 2013 at 06:20:34PM -0200, Paulo Zanoni wrote:
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index 2715600..70c4cef 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -2477,6 +2477,12 @@ static void i915_hangcheck_elapsed(unsigned long data)
>> >     if (!i915_enable_hangcheck)
>> >             return;
>> >
>> > +   /* Just postpone in case we're completely idle... */
>> > +   if (dev_priv->pm.suspended) {
>> > +           i915_queue_hangcheck(dev);
>> > +           return;
>> > +   }
>>
>> I got the idea here, just not sure if it fits on this patch or it is just the commit message.
>
> This hunk here is the wrong thing to do: If we're suspended and the
> hangcheck fires, we'll just delay it. But if we keep on being suspended
> the hangcheck will fire at the normal recheck rate (but never do anything)
> which wreaks utter havoc with deep sleep residency. The simples solution
> would be to synchronously cancel the timer in the idle work (where we
> already put the execbuf runtime ref) I think.
>

The problem is that if I do this, then we won't end up running the
hangcheck (which we always run today), so we won't check for hangs,
which is probably not good. I could add "i915_force_hangcheck" what
would be called at intel_mark_idle if you would like, but I'm not too
sure what are the downsides of doing this.

I also tried getting/putting a refcount on the hangcheck function, but
I get complaints that it's sleeping from an invalid context.

Which approach is the best?



> I've dropped this hunk from the patch when merging.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni