| Message ID | 1394233957-3904-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Just because I have a SNB machine and I can easily test it. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 27 +++++++++++++++++++++++++-- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_display.c | 7 +++++++ > 3 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index f75d776..2c62e0c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -832,6 +832,13 @@ static int i915_pm_poweroff(struct device *dev) > return i915_drm_freeze(drm_dev); > } > > +static void snb_runtime_suspend(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + > + intel_runtime_pm_disable_interrupts(dev); > +} Afaics RPS is still enabled here, so if we need it enabled at this point (so that GT stays at RC6 for example) we'd at least need here cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); cancel_work_sync(&dev_priv->rps.work); > + > static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > @@ -840,6 +847,18 @@ static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) > hsw_enable_pc8(dev_priv); > } > > +static void snb_runtime_resume(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + > + intel_runtime_pm_restore_interrupts(dev); > + intel_init_pch_refclk(dev); > + i915_gem_init_swizzling(dev); > + mutex_lock(&dev_priv->rps.hw_lock); > + gen6_update_ring_freq(dev); > + mutex_unlock(&dev_priv->rps.hw_lock); > +} I haven't found any info on what exact state is lost in D3, but in any case documenting this in the code would be great. Here there are some bits restored for GTI in i915_gem_init_swizzling(), but what about the clock gating registers and RPS registers. I'd be easier if those would be also re-inited here, or a confirmation that they are saved/restored by HW. --Imre > + > static void hsw_runtime_resume(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > @@ -859,7 +878,9 @@ static int intel_runtime_suspend(struct device *device) > > DRM_DEBUG_KMS("Suspending device\n"); > > - if (IS_HASWELL(dev)) > + if (IS_GEN6(dev)) > + snb_runtime_suspend(dev_priv); > + else if (IS_HASWELL(dev)) > hsw_runtime_suspend(dev_priv); > > i915_gem_release_all_mmaps(dev_priv); > @@ -893,7 +914,9 @@ static int intel_runtime_resume(struct device *device) > intel_opregion_notify_adapter(dev, PCI_D0); > dev_priv->pm.suspended = false; > > - if (IS_HASWELL(dev)) > + if (IS_GEN6(dev)) > + snb_runtime_resume(dev_priv); > + else if (IS_HASWELL(dev)) > hsw_runtime_resume(dev_priv); > > DRM_DEBUG_KMS("Device resumed\n"); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 493579d..309852d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1998,7 +1998,7 @@ struct drm_i915_cmd_table { > #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) > #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) > #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ > -#define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) > +#define HAS_RUNTIME_PM(dev) (IS_GEN6(dev) || IS_HASWELL(dev)) > > #define INTEL_PCH_DEVICE_ID_MASK 0xff00 > #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index df69866..35f65c1 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6859,6 +6859,11 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) > mutex_unlock(&dev_priv->rps.hw_lock); > } > > +static void snb_modeset_global_resources(struct drm_device *dev) > +{ > + modeset_update_crtc_power_domains(dev); > +} > + > static void haswell_modeset_global_resources(struct drm_device *dev) > { > modeset_update_crtc_power_domains(dev); > @@ -10758,6 +10763,8 @@ static void intel_init_display(struct drm_device *dev) > } else if (IS_GEN6(dev)) { > dev_priv->display.fdi_link_train = gen6_fdi_link_train; > dev_priv->display.write_eld = ironlake_write_eld; > + dev_priv->display.modeset_global_resources = > + snb_modeset_global_resources; > } else if (IS_IVYBRIDGE(dev)) { > /* FIXME: detect B0+ stepping and use auto training */ > dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
2014-04-01 12:34 GMT-03:00 Imre Deak <imre.deak@intel.com>: > On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Just because I have a SNB machine and I can easily test it. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 27 +++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/i915_drv.h | 2 +- >> drivers/gpu/drm/i915/intel_display.c | 7 +++++++ >> 3 files changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index f75d776..2c62e0c 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -832,6 +832,13 @@ static int i915_pm_poweroff(struct device *dev) >> return i915_drm_freeze(drm_dev); >> } >> >> +static void snb_runtime_suspend(struct drm_i915_private *dev_priv) >> +{ >> + struct drm_device *dev = dev_priv->dev; >> + >> + intel_runtime_pm_disable_interrupts(dev); >> +} > > Afaics RPS is still enabled here, so if we need it enabled at this point > (so that GT stays at RC6 for example) we'd at least need here > > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > cancel_work_sync(&dev_priv->rps.work); Congratulations, you've found the only runtime PM bug I can still reproduce on my local tree. But this problem also happens for HSW, not just SNB, so IMHO its fix should go on a separate patch. And it should go to -fixes. > >> + >> static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) >> { >> struct drm_device *dev = dev_priv->dev; >> @@ -840,6 +847,18 @@ static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) >> hsw_enable_pc8(dev_priv); >> } >> >> +static void snb_runtime_resume(struct drm_i915_private *dev_priv) >> +{ >> + struct drm_device *dev = dev_priv->dev; >> + >> + intel_runtime_pm_restore_interrupts(dev); >> + intel_init_pch_refclk(dev); >> + i915_gem_init_swizzling(dev); >> + mutex_lock(&dev_priv->rps.hw_lock); >> + gen6_update_ring_freq(dev); >> + mutex_unlock(&dev_priv->rps.hw_lock); >> +} > > I haven't found any info on what exact state is lost in D3, but in any > case documenting this in the code would be great. Unfortunately we don't have this documentation, and that's why we have so many different tests on the pm_pc8 test suite. This function is just based on the HSW code, and I am kinda trusting the test suite and my manual tests. > > Here there are some bits restored for GTI in i915_gem_init_swizzling(), > but what about the clock gating registers and RPS registers. I'd be > easier if those would be also re-inited here, or a confirmation that > they are saved/restored by HW. The clock gating registers were manually checked a looong time ago on HSW, and I didn't really remember to check them on SNB. We even discussed on the mailing list how to do this check, and I proposed an implementation a long time ago: http://lists.freedesktop.org/archives/intel-gfx/2013-November/036351.html , but I never wrote this code. Also, if you have ideas of IGT tests that could catch these possible problems you mentioned, please feel free to contribute! Either by writing code or by giving me a description of the code to be written :) I'll move this SNB patch to the end of the series. Thanks, Paulo > > --Imre > >> + >> static void hsw_runtime_resume(struct drm_i915_private *dev_priv) >> { >> struct drm_device *dev = dev_priv->dev; >> @@ -859,7 +878,9 @@ static int intel_runtime_suspend(struct device *device) >> >> DRM_DEBUG_KMS("Suspending device\n"); >> >> - if (IS_HASWELL(dev)) >> + if (IS_GEN6(dev)) >> + snb_runtime_suspend(dev_priv); >> + else if (IS_HASWELL(dev)) >> hsw_runtime_suspend(dev_priv); >> >> i915_gem_release_all_mmaps(dev_priv); >> @@ -893,7 +914,9 @@ static int intel_runtime_resume(struct device *device) >> intel_opregion_notify_adapter(dev, PCI_D0); >> dev_priv->pm.suspended = false; >> >> - if (IS_HASWELL(dev)) >> + if (IS_GEN6(dev)) >> + snb_runtime_resume(dev_priv); >> + else if (IS_HASWELL(dev)) >> hsw_runtime_resume(dev_priv); >> >> DRM_DEBUG_KMS("Device resumed\n"); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 493579d..309852d 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1998,7 +1998,7 @@ struct drm_i915_cmd_table { >> #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) >> #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) >> #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ >> -#define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) >> +#define HAS_RUNTIME_PM(dev) (IS_GEN6(dev) || IS_HASWELL(dev)) >> >> #define INTEL_PCH_DEVICE_ID_MASK 0xff00 >> #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00 >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index df69866..35f65c1 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6859,6 +6859,11 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) >> mutex_unlock(&dev_priv->rps.hw_lock); >> } >> >> +static void snb_modeset_global_resources(struct drm_device *dev) >> +{ >> + modeset_update_crtc_power_domains(dev); >> +} >> + >> static void haswell_modeset_global_resources(struct drm_device *dev) >> { >> modeset_update_crtc_power_domains(dev); >> @@ -10758,6 +10763,8 @@ static void intel_init_display(struct drm_device *dev) >> } else if (IS_GEN6(dev)) { >> dev_priv->display.fdi_link_train = gen6_fdi_link_train; >> dev_priv->display.write_eld = ironlake_write_eld; >> + dev_priv->display.modeset_global_resources = >> + snb_modeset_global_resources; >> } else if (IS_IVYBRIDGE(dev)) { >> /* FIXME: detect B0+ stepping and use auto training */ >> dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train; >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f75d776..2c62e0c 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -832,6 +832,13 @@ static int i915_pm_poweroff(struct device *dev) return i915_drm_freeze(drm_dev); } +static void snb_runtime_suspend(struct drm_i915_private *dev_priv) +{ + struct drm_device *dev = dev_priv->dev; + + intel_runtime_pm_disable_interrupts(dev); +} + static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; @@ -840,6 +847,18 @@ static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) hsw_enable_pc8(dev_priv); } +static void snb_runtime_resume(struct drm_i915_private *dev_priv) +{ + struct drm_device *dev = dev_priv->dev; + + intel_runtime_pm_restore_interrupts(dev); + intel_init_pch_refclk(dev); + i915_gem_init_swizzling(dev); + mutex_lock(&dev_priv->rps.hw_lock); + gen6_update_ring_freq(dev); + mutex_unlock(&dev_priv->rps.hw_lock); +} + static void hsw_runtime_resume(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; @@ -859,7 +878,9 @@ static int intel_runtime_suspend(struct device *device) DRM_DEBUG_KMS("Suspending device\n"); - if (IS_HASWELL(dev)) + if (IS_GEN6(dev)) + snb_runtime_suspend(dev_priv); + else if (IS_HASWELL(dev)) hsw_runtime_suspend(dev_priv); i915_gem_release_all_mmaps(dev_priv); @@ -893,7 +914,9 @@ static int intel_runtime_resume(struct device *device) intel_opregion_notify_adapter(dev, PCI_D0); dev_priv->pm.suspended = false; - if (IS_HASWELL(dev)) + if (IS_GEN6(dev)) + snb_runtime_resume(dev_priv); + else if (IS_HASWELL(dev)) hsw_runtime_resume(dev_priv); DRM_DEBUG_KMS("Device resumed\n"); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 493579d..309852d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1998,7 +1998,7 @@ struct drm_i915_cmd_table { #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ -#define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) +#define HAS_RUNTIME_PM(dev) (IS_GEN6(dev) || IS_HASWELL(dev)) #define INTEL_PCH_DEVICE_ID_MASK 0xff00 #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index df69866..35f65c1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6859,6 +6859,11 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) mutex_unlock(&dev_priv->rps.hw_lock); } +static void snb_modeset_global_resources(struct drm_device *dev) +{ + modeset_update_crtc_power_domains(dev); +} + static void haswell_modeset_global_resources(struct drm_device *dev) { modeset_update_crtc_power_domains(dev); @@ -10758,6 +10763,8 @@ static void intel_init_display(struct drm_device *dev) } else if (IS_GEN6(dev)) { dev_priv->display.fdi_link_train = gen6_fdi_link_train; dev_priv->display.write_eld = ironlake_write_eld; + dev_priv->display.modeset_global_resources = + snb_modeset_global_resources; } else if (IS_IVYBRIDGE(dev)) { /* FIXME: detect B0+ stepping and use auto training */ dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;