[Intel-gfx] [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Paulo Zanoni]

2013/8/23 Kristen Carlson Accardi <kristen.c.accardi at intel.com>:
> On Fri, 23 Aug 2013 13:44:17 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
>
>> 2013/8/23 Jani Nikula <jani.nikula at intel.com>:
>>
>> /* Please insert explanation on why we need this and what changes if
>> we do this. */
>>
>> I applied your patches and booted them. I got into PC8, did the PC8
>> test suite and nothing changed. I really don't know what to expect
>> from this series and/or how to check what's improving. Also, see
>> below:
>>
>
> So this is one of these things that will have no visible impact on
> i915, but will impact other parts of the system.  So I think the only
> way to test it is by throwing it on the SIP board and checking the
> power level of the components this impacts (Audio, thermal, KBC/EC,
> LPT).  And without the code which does the actual PCI D3 request from
> i915, nothing will happen.

I was told we could try to verify this by reading PCI config register
0xd4, but for me it's always 0x0, which means we're probably not
really getting into D3.

> Is it possible to get a patch which finds
> some very obvious place to put the controller into D3 so we can check
> to see if the opregion notifies are doing what they are supposed to?

http://cgit.freedesktop.org/~pzanoni/linux/?h=d3-wip

My check using the PCI config register 0xd4 suggests we're probably
not really enabling D3 :(

>
>>
>> > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |    4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index a6df68e..7ed2248 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -6093,6 +6093,8 @@ void hsw_enable_pc8_work(struct work_struct *__work)
>> >         lpt_disable_clkout_dp(dev);
>> >         hsw_pc8_disable_interrupts(dev);
>> >         hsw_disable_lcpll(dev_priv, true, true);
>> > +
>> > +       intel_opregion_notify_adapter(dev, PCI_D1);
>>
>> Why D1? Shouldn't this be D3? I think that's what people having been
>> asking us to implement.
>>
>> On the doc that explains "adapter power state notification", my
>> understanding is that it suggests that we should call this _before_ we
>> go into the lower states and the other chunk should be called _after_
>> we're at the higher power states. So perhaps we should call
>> intel_opregion_notify_adapter before hsw_disable_lcpll, and, on the
>> chunk below, after hsw_restore_lcpll? But this is not 100% clear, I
>> may be wrong.
>>
>> By the way, I modified your patch to implement the suggestions above,
>> and got the same results: no noticeable difference, everything still
>> works. No news is good news?
>>
>>
>> >  }
>> >
>> >  static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>> > @@ -6126,6 +6128,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>> >         if (!dev_priv->pc8.enabled)
>> >                 return;
>> >
>> > +       intel_opregion_notify_adapter(dev, PCI_D0);
>> > +
>> >         DRM_DEBUG_KMS("Disabling package C8+\n");
>> >
>> >         hsw_restore_lcpll(dev_priv);
>> > --
>> > 1.7.9.5
>> >
>>
>>
>>
>



-- 
Paulo Zanoni