2013/8/23 Kristen Carlson Accardi <kristen.c.accardi@xxxxxxxxx>: > On Fri, 23 Aug 2013 13:44:17 -0300 > Paulo Zanoni <przanoni@xxxxxxxxx> wrote: > >> 2013/8/23 Jani Nikula <jani.nikula@xxxxxxxxx>: >> >> /* 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@xxxxxxxxx> >> > --- >> > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
