| Message ID | 1408651778-2636-3-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 21, 2014 at 05:09:38PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > On BDW we're seeing a problem that after we runtime resume, the > outputs connected to DDI C are not detected: they don't appear in the > SDEISR register and GMBUS transactions don't work. They stop working > at the moment we call intel_opregion_notify_adapter() during runtime > suspend, but they don't go back to work when we call the same function > during runtime resume. They only work after we do a modeset and call > intel_opregion_notify_encoder(), but this point is already too late. > > While debugging, I tried to pass PCI_D3hot which is the value that > matches the spec, and it seems to have solved the problem. I couldn't > find any explanation of why this solves the problem, but there's also > no documented explanation - besides our code and git log - of why > Haswell should use PCI_D1, so keep this for now in order to keep BDW > runtime PM working. > > Also add a comment to point the fact that there's no spec documenting > all the weirdness involved here. > > Cc: kristen.c.accardi@intel.com > Testcase: igt/pm_rpm/drm-resources-equal > Testcase: igt/pm_rpm/i2c > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> There's still investigation to understand what the firmware is doing behind our backs, but the change looks likely to be correct and fixes the problem at hand so: Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
On Thu, Aug 21, 2014 at 10:47:12PM +0100, Damien Lespiau wrote: > On Thu, Aug 21, 2014 at 05:09:38PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > On BDW we're seeing a problem that after we runtime resume, the > > outputs connected to DDI C are not detected: they don't appear in the > > SDEISR register and GMBUS transactions don't work. They stop working > > at the moment we call intel_opregion_notify_adapter() during runtime > > suspend, but they don't go back to work when we call the same function > > during runtime resume. They only work after we do a modeset and call > > intel_opregion_notify_encoder(), but this point is already too late. > > > > While debugging, I tried to pass PCI_D3hot which is the value that > > matches the spec, and it seems to have solved the problem. I couldn't > > find any explanation of why this solves the problem, but there's also > > no documented explanation - besides our code and git log - of why > > Haswell should use PCI_D1, so keep this for now in order to keep BDW > > runtime PM working. > > > > Also add a comment to point the fact that there's no spec documenting > > all the weirdness involved here. > > > > Cc: kristen.c.accardi@intel.com > > Testcase: igt/pm_rpm/drm-resources-equal > > Testcase: igt/pm_rpm/i2c > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > There's still investigation to understand what the firmware is doing > behind our backs, but the change looks likely to be correct and fixes > the problem at hand so: > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > -- > Damien > > > --- > > drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++++++++++++------ > > 1 file changed, 22 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 2dcc0d8..6f0c95f 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1455,13 +1455,29 @@ static int intel_runtime_suspend(struct device *device) > > dev_priv->pm.suspended = true; > > > > /* > > - * current versions of firmware which depend on this opregion > > - * notification have repurposed the D1 definition to mean > > - * "runtime suspended" vs. what you would normally expect (D3) > > - * to distinguish it from notifications that might be sent > > - * via the suspend path. > > + * FIXME: We really should find a document that references the arguments > > + * used below! > > */ > > - intel_opregion_notify_adapter(dev, PCI_D1); > > + if (IS_HASWELL(dev)) { > > + /* > > + * current versions of firmware which depend on this opregion > > + * notification have repurposed the D1 definition to mean > > + * "runtime suspended" vs. what you would normally expect (D3) > > + * to distinguish it from notifications that might be sent via > > + * the suspend path. > > + */ > > + intel_opregion_notify_adapter(dev, PCI_D1); > > + } else { > > + /* > > + * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop > > + * being detected, and the call we do at intel_runtime_resume() > > + * won't be able to restore them. Since PCI_D3hot matches the > > + * actual specification and appears to be working, use it. Let's > > + * assume the other non-Haswell platforms will stay the same as > > + * Broadwell. > > + */ > > + intel_opregion_notify_adapter(dev, PCI_D3hot); I guess we could start to open betting pools whether this will grow until we have all the pci states or not ;-) Anyway, all 3 patches merged to dinq, thanks. -Daniel > > + } > > > > DRM_DEBUG_KMS("Device suspended\n"); > > return 0; > > -- > > 2.0.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2dcc0d8..6f0c95f 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1455,13 +1455,29 @@ static int intel_runtime_suspend(struct device *device) dev_priv->pm.suspended = true; /* - * current versions of firmware which depend on this opregion - * notification have repurposed the D1 definition to mean - * "runtime suspended" vs. what you would normally expect (D3) - * to distinguish it from notifications that might be sent - * via the suspend path. + * FIXME: We really should find a document that references the arguments + * used below! */ - intel_opregion_notify_adapter(dev, PCI_D1); + if (IS_HASWELL(dev)) { + /* + * current versions of firmware which depend on this opregion + * notification have repurposed the D1 definition to mean + * "runtime suspended" vs. what you would normally expect (D3) + * to distinguish it from notifications that might be sent via + * the suspend path. + */ + intel_opregion_notify_adapter(dev, PCI_D1); + } else { + /* + * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop + * being detected, and the call we do at intel_runtime_resume() + * won't be able to restore them. Since PCI_D3hot matches the + * actual specification and appears to be working, use it. Let's + * assume the other non-Haswell platforms will stay the same as + * Broadwell. + */ + intel_opregion_notify_adapter(dev, PCI_D3hot); + } DRM_DEBUG_KMS("Device suspended\n"); return 0;