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

[Kristen Carlson Accardi <kristen.c.accardi@intel.com>]

On Fri, 23 Aug 2013 13:44:17 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2013/8/23 Jani Nikula <jani.nikula@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.  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?

> 
> > Signed-off-by: Jani Nikula <jani.nikula@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
> >
> 
> 
>