[Intel-gfx] [PATCH 4/8] drm/i915: avoid waking up from PC8 on DP AUX operations

[Paulo Zanoni]

2013/7/30 Chris Wilson <chris at chris-wilson.co.uk>:
> On Mon, Jul 29, 2013 at 05:48:23PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> If we're already allowing PC8, just don't use the IRQs, so we won't
>> need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
>> when we can.
>
> This raises the question of why we where holding the power well wake
> lock if we weren't using IRQs in the first place.

I can't understand the sentence above. Also notice that the power well
is kinda orthogonal to allowing/disallowing PC8. We need the power
well disabled to allow PC8, but we won't allow PC8 whenever the power
well is disabled. And the GMBUS/DP AUX registers are on the PCH, so
they don't get affected by the state of the power well.

> This tells me the
> intel_aux_display_runtime_get() interface is indequate, imo. It would
> need to be more expressive of why/what part of aux display runtime you
> want. You could even move the state checking there so that GMBUS could
> use the same interface:
>
> unsigned long intel_aux_display_runtime_get(dev_priv, want, need)
>  {
>   if (need) {
>      __intel_aux_display_runtime_get();
>      return need;
>    }
>
>    if (want) {
>         if (hsw_pc8_enabled()) {
>            __intel_aux_display_runtime_get();
>            return want;
>         }
>    }
>
>    return 0;
>  }
>
> And pass the return value to _put for it to decide if it needs to do
> anything (also it can then select its path based on whether or not it
> holds a wakeref). Making the caller decide whether or not to act based
> on information inside pc8 looks very fragile to me, and easy to break in
> future.

Yeah, sounds like a good idea.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni