[Intel-gfx] [PATCH 3/8] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

2013/7/29 Chris Wilson <chris at chris-wilson.co.uk>:
> On Mon, Jul 29, 2013 at 05:48:22PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> This patch allows PC8+ states on Haswell. These states can only be
>> reached when all the display outputs are disabled, and they allow some
>> more power savings.
>>
>> The fact that the graphics device is allowing PC8+ doesn't mean that
>> the machine will actually enter PC8+: all the other devices also need
>> to allow PC8+.
>>
>> For now this option is disabled by default. You need i915.allow_pc8=1
>> if you want it.
>
> Still dislike the names. hsw_pc8 is good, so use it consistently.

Do you mean i915.allow_hsw_pc8? Or i915.enable_pc8? Or
i915.enable_hsw_pc8? (You suggested to change from "allow" to
"enable").

>
>> +/* Disallows PC8 so we can use the GMBUS and DP AUX interrupts. */
>> +void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
>> +{
>> +     mutex_lock(&dev_priv->pc8.lock);
>> +     hsw_disallow_package_c8(dev_priv->dev);
>> +     mutex_unlock(&dev_priv->pc8.lock);
>> +}
>> +
>> +void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
>> +{
>> +     mutex_lock(&dev_priv->pc8.lock);
>> +     hsw_allow_package_c8(dev_priv->dev);
>> +     mutex_unlock(&dev_priv->pc8.lock);
>> +}
>
> Move the locking from into hsw_(allow|disallow)_package_c8, and rename
> the current hsw_alloc_package_c8 i.e
>
>   void hsw_allow_package_c8(struct drm_i915_private *dev_priv)
>   {
>         mutex_lock(&dev_priv->pc8.lock);
>         __hsw_allow_package_c8(dev_priv);
>         mutex_unlock(&dev_priv->pc8.lock);
>   }
>
> then allow the lock manipulation is close together and the doesn't leak
> across layers. (The locking is definitely not clear atm.) Whilst you are
> at is, I do not see any reason not to call them hsw_pc8_enable() and
> hsw_pc8_disable(), and hsw_set_package_c8() becomes hsw_pc8_update().
>
> Just call forbid_refcnt, forbid_count (though I'm still liking
> wake_count). And replace allowing with display_power_well_active,
> verbosity is good here.

You mean replace dev_priv->pc8.allowing with
dev->priv->pc8.display_power_well_active? That's not good, because
when you have eDP-only the display power well is disabled but you
can't allow PC8, and then you have more than one output the power well
is enabled but you can't allow PC8.

> s/i915_allow_pc8/i915_enable_pc8/ for
> consistency.

I use the word "allow" because even if we allow PC8 it doesn't mean it
will actually be enabled, other drivers also need to allow it. But, of
course, I could change this anyway.


>
> So other than those minor issues, being freaked out by the locking and
> the WARNs, I want at least a paragraph explaining why that is even
> remotely safe, the logic at least looks sane.

I'll try to write something in the log message. The basic idea is that
we set specific values to the IMR registers before we allow PC8, and
when we get disallow PC8 we check if the values changed and print a
nice WARN in case they did. We can't get interrupts (except for HPD)
while we're allowing PC8, we could get machine hangs with this.


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



-- 
Paulo Zanoni