| Message ID | 1407840731-25081-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 12, 2014 at 04:21:55PM +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble at intel.com> > > v1: > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in > PM suspend and resume path similar to runtime suspend and resume. > > v2: > 1. Keeping GT access, wake, gunit save/restore related helpers static. > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze. > 3. Reusing the sequence in runtime_suspend/resume path at macro level. > > v3: > 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from > D0i3. > 2. Changed commit header. > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Goel, Akash <akash.goel@intel.com> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2 > Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com> Yeah, this looks roughly like what I'd expected, thanks. I think there's some room now to consolidate hw deinit/init code further between system s/r and rpm paths, but at least we've started. Please find one of the rpm domain experts to review this so I can merge it. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++-------------- > 1 file changed, 85 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index ec96f9a..4440722 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > return true; > } > > + > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv); > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend); > + > static int i915_drm_freeze(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work) > static int i915_drm_thaw_early(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + int ret = 0; > > - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > - hsw_disable_pc8(dev_priv); > + /* Restore any platform specific registers/clk state */ > + ret = post_hw_resume_init(dev_priv, false); > > intel_uncore_early_sanitize(dev, true); > intel_uncore_sanitize(dev); > intel_power_domains_init_hw(dev_priv); > > - return 0; > + return ret; > } > > static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev) > struct pci_dev *pdev = to_pci_dev(dev); > struct drm_device *drm_dev = pci_get_drvdata(pdev); > struct drm_i915_private *dev_priv = drm_dev->dev_private; > + int ret = 0; > > /* > * We have a suspedn ordering issue with the snd-hda driver also > @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev) > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > return 0; > > - if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev)) > - hsw_enable_pc8(dev_priv); > + /* Save any platform specific registers/clk state needed post resume */ > + ret = pre_hw_suspend_deinit(dev_priv); > > pci_disable_device(pdev); > pci_set_power_state(pdev, PCI_D3hot); > > - return 0; > + return ret; > } > > static int i915_pm_resume_early(struct device *dev) > @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev) > return i915_drm_freeze(drm_dev); > } > > -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv) > +static int hsw_suspend(struct drm_i915_private *dev_priv) > { > hsw_enable_pc8(dev_priv); > > return 0; > } > > -static int snb_runtime_resume(struct drm_i915_private *dev_priv) > +static int snb_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > { > struct drm_device *dev = dev_priv->dev; > > - intel_init_pch_refclk(dev); > + if (resume_from_rpm_suspend) > + intel_init_pch_refclk(dev); > > return 0; > } > > -static int hsw_runtime_resume(struct drm_i915_private *dev_priv) > +static int hsw_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > { > hsw_disable_pc8(dev_priv); > > @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) > I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); > } > > -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > +static int vlv_suspend(struct drm_i915_private *dev_priv) > { > u32 mask; > - int err; > + int ret = 0; > > /* > * Bspec defines the following GT well on flags as debug only, so > @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > > vlv_check_no_gt_access(dev_priv); > > - err = vlv_force_gfx_clock(dev_priv, true); > - if (err) > + ret = vlv_force_gfx_clock(dev_priv, true); > + if (ret) > goto err1; > > - err = vlv_allow_gt_wake(dev_priv, false); > - if (err) > + ret = vlv_allow_gt_wake(dev_priv, false); > + if (ret) > goto err2; > vlv_save_gunit_s0ix_state(dev_priv); > > - err = vlv_force_gfx_clock(dev_priv, false); > - if (err) > + ret = vlv_force_gfx_clock(dev_priv, false); > + if (ret) > goto err2; > - > - return 0; > + return ret; > > err2: > /* For safety always re-enable waking and disable gfx clock forcing */ > @@ -1332,14 +1341,15 @@ err2: > err1: > vlv_force_gfx_clock(dev_priv, false); > > - return err; > + return ret; > } > > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > +static int vlv_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > { > struct drm_device *dev = dev_priv->dev; > int err; > - int ret; > + int ret = 0; > > /* > * If any of the steps fail just try to continue, that's the best we > @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > vlv_check_no_gt_access(dev_priv); > > - intel_init_clock_gating(dev); > - i915_gem_restore_fences(dev); > + if (resume_from_rpm_suspend) { > + intel_init_clock_gating(dev); > + i915_gem_restore_fences(dev); > + } > > return ret; > } > @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device) > cancel_work_sync(&dev_priv->rps.work); > intel_runtime_pm_disable_interrupts(dev); > > - if (IS_GEN6(dev)) { > - ret = 0; > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > - ret = hsw_runtime_suspend(dev_priv); > - } else if (IS_VALLEYVIEW(dev)) { > - ret = vlv_runtime_suspend(dev_priv); > - } else { > - ret = -ENODEV; > - WARN_ON(1); > - } > + /* Save any platform specific registers/clk state needed post resume */ > + ret = pre_hw_suspend_deinit(dev_priv); > > if (ret) { > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device) > intel_opregion_notify_adapter(dev, PCI_D0); > dev_priv->pm.suspended = false; > > - if (IS_GEN6(dev)) { > - ret = snb_runtime_resume(dev_priv); > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > - ret = hsw_runtime_resume(dev_priv); > - } else if (IS_VALLEYVIEW(dev)) { > - ret = vlv_runtime_resume(dev_priv); > - } else { > - WARN_ON(1); > - ret = -ENODEV; > - } > + /* Restore any platform specific registers/clk state */ > + ret = post_hw_resume_init(dev_priv, true); > > /* > * No point of rolling back things in case of an error, as the best > @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device) > return ret; > } > > +/* This handler is used in system/runtime suspend path to reuse > + * Gfx clock, Wake control, Gunit state save related functionaility for VLV. > + */ > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + int ret = 0; > + > + if (IS_GEN6(dev)) { > + ret = 0; > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret = hsw_suspend(dev_priv); > + } else if (IS_VALLEYVIEW(dev)) { > + ret = vlv_suspend(dev_priv); > + } else { > + ret = -ENODEV; > + WARN_ON(1); > + } > + > + return ret; > +} > + > +/* This handler is used in system/runtime resume path to reuse > + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV. > + */ > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > +{ > + struct drm_device *dev = dev_priv->dev; > + int ret = 0; > + > + if (IS_GEN6(dev)) { > + ret = snb_resume(dev_priv, resume_from_rpm_suspend); > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret = hsw_resume(dev_priv, resume_from_rpm_suspend); > + } else if (IS_VALLEYVIEW(dev)) { > + ret = vlv_resume(dev_priv, resume_from_rpm_suspend); > + } else { > + WARN_ON(1); > + ret = -ENODEV; > + } > + return ret; > +} > + > static const struct dev_pm_ops i915_pm_ops = { > .suspend = i915_pm_suspend, > .suspend_late = i915_pm_suspend_late, > -- > 1.8.5 >
On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble at intel.com> The change is substantial enough that you should add a commit message explaining what it fixes and how. There are further useful guidelines on this topic in Documentation/SubmittingPatches. In the future, you would make the review (and a possible bisection) easier if you split this patch into a refactoring patch without functional change and one that fixes the issue. > v1: > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in > PM suspend and resume path similar to runtime suspend and resume. > > v2: > 1. Keeping GT access, wake, gunit save/restore related helpers static. > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze. > 3. Reusing the sequence in runtime_suspend/resume path at macro level. > > v3: > 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from > D0i3. > 2. Changed commit header. > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Goel, Akash <akash.goel@intel.com> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2 > Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++-------------- > 1 file changed, 85 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index ec96f9a..4440722 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > return true; > } > > + > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv); > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend); Nitpick: Something like intel_suspend_complete, intel_resume_prepare would be more descriptive names. > + > static int i915_drm_freeze(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work) > static int i915_drm_thaw_early(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + int ret = 0; The initialization here is redundant and could suppress complier warnings. There are also a couple more instances below. > - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > - hsw_disable_pc8(dev_priv); > + /* Restore any platform specific registers/clk state */ > + ret = post_hw_resume_init(dev_priv, false); You could print an error message here, noting that we continue resuming despite the error. > intel_uncore_early_sanitize(dev, true); > intel_uncore_sanitize(dev); > intel_power_domains_init_hw(dev_priv); > > - return 0; > + return ret; > } > > static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev) > struct pci_dev *pdev = to_pci_dev(dev); > struct drm_device *drm_dev = pci_get_drvdata(pdev); > struct drm_i915_private *dev_priv = drm_dev->dev_private; > + int ret = 0; > > /* > * We have a suspedn ordering issue with the snd-hda driver also > @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev) > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > return 0; > > - if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev)) > - hsw_enable_pc8(dev_priv); > + /* Save any platform specific registers/clk state needed post resume */ > + ret = pre_hw_suspend_deinit(dev_priv); > > pci_disable_device(pdev); > pci_set_power_state(pdev, PCI_D3hot); In case of error the suspend will be canceled and the resume handler will be called, so we shouldn't disable the device. > > - return 0; > + return ret; > } > > static int i915_pm_resume_early(struct device *dev) > @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev) > return i915_drm_freeze(drm_dev); > } > > -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv) > +static int hsw_suspend(struct drm_i915_private *dev_priv) Based on the above nitpick something like hsw_suspend_prepare would be better. The same goes for the other platform handlers. > { > hsw_enable_pc8(dev_priv); > > return 0; > } > > -static int snb_runtime_resume(struct drm_i915_private *dev_priv) > +static int snb_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more compact. > { > struct drm_device *dev = dev_priv->dev; > > - intel_init_pch_refclk(dev); > + if (resume_from_rpm_suspend) > + intel_init_pch_refclk(dev); > > return 0; > } > > -static int hsw_runtime_resume(struct drm_i915_private *dev_priv) > +static int hsw_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > { > hsw_disable_pc8(dev_priv); > > @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) > I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); > } > > -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > +static int vlv_suspend(struct drm_i915_private *dev_priv) > { > u32 mask; > - int err; > + int ret = 0; > > /* > * Bspec defines the following GT well on flags as debug only, so > @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > > vlv_check_no_gt_access(dev_priv); > > - err = vlv_force_gfx_clock(dev_priv, true); > - if (err) > + ret = vlv_force_gfx_clock(dev_priv, true); > + if (ret) > goto err1; > > - err = vlv_allow_gt_wake(dev_priv, false); > - if (err) > + ret = vlv_allow_gt_wake(dev_priv, false); > + if (ret) > goto err2; > vlv_save_gunit_s0ix_state(dev_priv); > > - err = vlv_force_gfx_clock(dev_priv, false); > - if (err) > + ret = vlv_force_gfx_clock(dev_priv, false); > + if (ret) > goto err2; > - > - return 0; > + return ret; > > err2: > /* For safety always re-enable waking and disable gfx clock forcing */ > @@ -1332,14 +1341,15 @@ err2: > err1: > vlv_force_gfx_clock(dev_priv, false); > > - return err; > + return ret; > } In the above function I can't see any change besides renaming err to ret. There isn't much point in that imo. > > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > +static int vlv_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > { > struct drm_device *dev = dev_priv->dev; > int err; > - int ret; > + int ret = 0; > > /* > * If any of the steps fail just try to continue, that's the best we > @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > vlv_check_no_gt_access(dev_priv); > > - intel_init_clock_gating(dev); > - i915_gem_restore_fences(dev); > + if (resume_from_rpm_suspend) { > + intel_init_clock_gating(dev); > + i915_gem_restore_fences(dev); > + } > > return ret; > } > @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device) > cancel_work_sync(&dev_priv->rps.work); > intel_runtime_pm_disable_interrupts(dev); > > - if (IS_GEN6(dev)) { > - ret = 0; > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > - ret = hsw_runtime_suspend(dev_priv); > - } else if (IS_VALLEYVIEW(dev)) { > - ret = vlv_runtime_suspend(dev_priv); > - } else { > - ret = -ENODEV; > - WARN_ON(1); > - } > + /* Save any platform specific registers/clk state needed post resume */ > + ret = pre_hw_suspend_deinit(dev_priv); > > if (ret) { > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device) > intel_opregion_notify_adapter(dev, PCI_D0); > dev_priv->pm.suspended = false; > > - if (IS_GEN6(dev)) { > - ret = snb_runtime_resume(dev_priv); > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > - ret = hsw_runtime_resume(dev_priv); > - } else if (IS_VALLEYVIEW(dev)) { > - ret = vlv_runtime_resume(dev_priv); > - } else { > - WARN_ON(1); > - ret = -ENODEV; > - } > + /* Restore any platform specific registers/clk state */ > + ret = post_hw_resume_init(dev_priv, true); > > /* > * No point of rolling back things in case of an error, as the best > @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device) > return ret; > } > > +/* This handler is used in system/runtime suspend path to reuse > + * Gfx clock, Wake control, Gunit state save related functionaility for VLV. > + */ The above comment is not precise, the function is used for all platforms. I think it'd be enough to mention that it's the common part of the runtime and system suspend sequence. > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + int ret = 0; > + > + if (IS_GEN6(dev)) { > + ret = 0; > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret = hsw_suspend(dev_priv); > + } else if (IS_VALLEYVIEW(dev)) { > + ret = vlv_suspend(dev_priv); > + } else { > + ret = -ENODEV; > + WARN_ON(1); > + } > + > + return ret; > +} > + > +/* This handler is used in system/runtime resume path to reuse > + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV. > + */ > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > +{ > + struct drm_device *dev = dev_priv->dev; > + int ret = 0; > + > + if (IS_GEN6(dev)) { > + ret = snb_resume(dev_priv, resume_from_rpm_suspend); > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret = hsw_resume(dev_priv, resume_from_rpm_suspend); > + } else if (IS_VALLEYVIEW(dev)) { > + ret = vlv_resume(dev_priv, resume_from_rpm_suspend); > + } else { > + WARN_ON(1); > + ret = -ENODEV; > + } > + return ret; > +} > + > static const struct dev_pm_ops i915_pm_ops = { > .suspend = i915_pm_suspend, > .suspend_late = i915_pm_suspend_late,
On Wed, 2014-08-13 at 16:47 +0300, Imre Deak wrote: > On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@intel.com wrote: > > From: Sagar Kamble <sagar.a.kamble at intel.com> > > The change is substantial enough that you should add a commit message > explaining what it fixes and how. There are further useful guidelines on > this topic in Documentation/SubmittingPatches. > > In the future, you would make the review (and a possible bisection) > easier if you split this patch into a refactoring patch without > functional change and one that fixes the issue. Thanks for the review Imre, Daniel. Agree that I need to split this patch and give more description for the changes. Will fix other reviewed code as well. > > > v1: > > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in > > PM suspend and resume path similar to runtime suspend and resume. > > > > v2: > > 1. Keeping GT access, wake, gunit save/restore related helpers static. > > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze. > > 3. Reusing the sequence in runtime_suspend/resume path at macro level. > > > > v3: > > 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from > > D0i3. > > 2. Changed commit header. > > > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Goel, Akash <akash.goel@intel.com> > > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2 > > Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++-------------- > > 1 file changed, 85 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index ec96f9a..4440722 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > > return true; > > } > > > > + > > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv); > > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > > + bool resume_from_rpm_suspend); > > Nitpick: Something like intel_suspend_complete, intel_resume_prepare > would be more descriptive names. > > > + > > static int i915_drm_freeze(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work) > > static int i915_drm_thaw_early(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > + int ret = 0; > > The initialization here is redundant and could suppress complier > warnings. There are also a couple more instances below. > > > - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > - hsw_disable_pc8(dev_priv); > > + /* Restore any platform specific registers/clk state */ > > + ret = post_hw_resume_init(dev_priv, false); > > You could print an error message here, noting that we continue resuming > despite the error. > > > intel_uncore_early_sanitize(dev, true); > > intel_uncore_sanitize(dev); > > intel_power_domains_init_hw(dev_priv); > > > > - return 0; > > + return ret; > > } > > > > static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev) > > struct pci_dev *pdev = to_pci_dev(dev); > > struct drm_device *drm_dev = pci_get_drvdata(pdev); > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > + int ret = 0; > > > > /* > > * We have a suspedn ordering issue with the snd-hda driver also > > @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev) > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > > return 0; > > > > - if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev)) > > - hsw_enable_pc8(dev_priv); > > + /* Save any platform specific registers/clk state needed post resume */ > > + ret = pre_hw_suspend_deinit(dev_priv); > > > > pci_disable_device(pdev); > > pci_set_power_state(pdev, PCI_D3hot); > > In case of error the suspend will be canceled and the resume handler > will be called, so we shouldn't disable the device. > > > > > - return 0; > > + return ret; > > } > > > > static int i915_pm_resume_early(struct device *dev) > > @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev) > > return i915_drm_freeze(drm_dev); > > } > > > > -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv) > > +static int hsw_suspend(struct drm_i915_private *dev_priv) > > Based on the above nitpick something like hsw_suspend_prepare would be > better. The same goes for the other platform handlers. > > > { > > hsw_enable_pc8(dev_priv); > > > > return 0; > > } > > > > -static int snb_runtime_resume(struct drm_i915_private *dev_priv) > > +static int snb_resume(struct drm_i915_private *dev_priv, > > + bool resume_from_rpm_suspend) > > Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more > compact. > > > { > > struct drm_device *dev = dev_priv->dev; > > > > - intel_init_pch_refclk(dev); > > + if (resume_from_rpm_suspend) > > + intel_init_pch_refclk(dev); > > > > return 0; > > } > > > > -static int hsw_runtime_resume(struct drm_i915_private *dev_priv) > > +static int hsw_resume(struct drm_i915_private *dev_priv, > > + bool resume_from_rpm_suspend) > > { > > hsw_disable_pc8(dev_priv); > > > > @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) > > I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); > > } > > > > -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > > +static int vlv_suspend(struct drm_i915_private *dev_priv) > > { > > u32 mask; > > - int err; > > + int ret = 0; > > > > /* > > * Bspec defines the following GT well on flags as debug only, so > > @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > > > > vlv_check_no_gt_access(dev_priv); > > > > - err = vlv_force_gfx_clock(dev_priv, true); > > - if (err) > > + ret = vlv_force_gfx_clock(dev_priv, true); > > + if (ret) > > goto err1; > > > > - err = vlv_allow_gt_wake(dev_priv, false); > > - if (err) > > + ret = vlv_allow_gt_wake(dev_priv, false); > > + if (ret) > > goto err2; > > vlv_save_gunit_s0ix_state(dev_priv); > > > > - err = vlv_force_gfx_clock(dev_priv, false); > > - if (err) > > + ret = vlv_force_gfx_clock(dev_priv, false); > > + if (ret) > > goto err2; > > - > > - return 0; > > + return ret; > > > > err2: > > /* For safety always re-enable waking and disable gfx clock forcing */ > > @@ -1332,14 +1341,15 @@ err2: > > err1: > > vlv_force_gfx_clock(dev_priv, false); > > > > - return err; > > + return ret; > > } > > In the above function I can't see any change besides renaming err to > ret. There isn't much point in that imo. > > > > > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > +static int vlv_resume(struct drm_i915_private *dev_priv, > > + bool resume_from_rpm_suspend) > > { > > struct drm_device *dev = dev_priv->dev; > > int err; > > - int ret; > > + int ret = 0; > > > > /* > > * If any of the steps fail just try to continue, that's the best we > > @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > > > vlv_check_no_gt_access(dev_priv); > > > > - intel_init_clock_gating(dev); > > - i915_gem_restore_fences(dev); > > + if (resume_from_rpm_suspend) { > > + intel_init_clock_gating(dev); > > + i915_gem_restore_fences(dev); > > + } > > > > return ret; > > } > > @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device) > > cancel_work_sync(&dev_priv->rps.work); > > intel_runtime_pm_disable_interrupts(dev); > > > > - if (IS_GEN6(dev)) { > > - ret = 0; > > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > > - ret = hsw_runtime_suspend(dev_priv); > > - } else if (IS_VALLEYVIEW(dev)) { > > - ret = vlv_runtime_suspend(dev_priv); > > - } else { > > - ret = -ENODEV; > > - WARN_ON(1); > > - } > > + /* Save any platform specific registers/clk state needed post resume */ > > + ret = pre_hw_suspend_deinit(dev_priv); > > > > if (ret) { > > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > > @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device) > > intel_opregion_notify_adapter(dev, PCI_D0); > > dev_priv->pm.suspended = false; > > > > - if (IS_GEN6(dev)) { > > - ret = snb_runtime_resume(dev_priv); > > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > > - ret = hsw_runtime_resume(dev_priv); > > - } else if (IS_VALLEYVIEW(dev)) { > > - ret = vlv_runtime_resume(dev_priv); > > - } else { > > - WARN_ON(1); > > - ret = -ENODEV; > > - } > > + /* Restore any platform specific registers/clk state */ > > + ret = post_hw_resume_init(dev_priv, true); > > > > /* > > * No point of rolling back things in case of an error, as the best > > @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device) > > return ret; > > } > > > > +/* This handler is used in system/runtime suspend path to reuse > > + * Gfx clock, Wake control, Gunit state save related functionaility for VLV. > > + */ > > The above comment is not precise, the function is used for all > platforms. I think it'd be enough to mention that it's the common part > of the runtime and system suspend sequence. > > > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv) > > +{ > > + struct drm_device *dev = dev_priv->dev; > > + int ret = 0; > > + > > + if (IS_GEN6(dev)) { > > + ret = 0; > > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > > + ret = hsw_suspend(dev_priv); > > + } else if (IS_VALLEYVIEW(dev)) { > > + ret = vlv_suspend(dev_priv); > > + } else { > > + ret = -ENODEV; > > + WARN_ON(1); > > + } > > + > > + return ret; > > +} > > + > > +/* This handler is used in system/runtime resume path to reuse > > + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV. > > + */ > > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > > + bool resume_from_rpm_suspend) > > +{ > > + struct drm_device *dev = dev_priv->dev; > > + int ret = 0; > > + > > + if (IS_GEN6(dev)) { > > + ret = snb_resume(dev_priv, resume_from_rpm_suspend); > > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > > + ret = hsw_resume(dev_priv, resume_from_rpm_suspend); > > + } else if (IS_VALLEYVIEW(dev)) { > > + ret = vlv_resume(dev_priv, resume_from_rpm_suspend); > > + } else { > > + WARN_ON(1); > > + ret = -ENODEV; > > + } > > + return ret; > > +} > > + > > static const struct dev_pm_ops i915_pm_ops = { > > .suspend = i915_pm_suspend, > > .suspend_late = i915_pm_suspend_late, >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ec96f9a..4440722 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) return true; } + +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv); +static int post_hw_resume_init(struct drm_i915_private *dev_priv, + bool resume_from_rpm_suspend); + static int i915_drm_freeze(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work) static int i915_drm_thaw_early(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + int ret = 0; - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) - hsw_disable_pc8(dev_priv); + /* Restore any platform specific registers/clk state */ + ret = post_hw_resume_init(dev_priv, false); intel_uncore_early_sanitize(dev, true); intel_uncore_sanitize(dev); intel_power_domains_init_hw(dev_priv); - return 0; + return ret; } static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev) struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); struct drm_i915_private *dev_priv = drm_dev->dev_private; + int ret = 0; /* * We have a suspedn ordering issue with the snd-hda driver also @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev) if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) return 0; - if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev)) - hsw_enable_pc8(dev_priv); + /* Save any platform specific registers/clk state needed post resume */ + ret = pre_hw_suspend_deinit(dev_priv); pci_disable_device(pdev); pci_set_power_state(pdev, PCI_D3hot); - return 0; + return ret; } static int i915_pm_resume_early(struct device *dev) @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev) return i915_drm_freeze(drm_dev); } -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv) +static int hsw_suspend(struct drm_i915_private *dev_priv) { hsw_enable_pc8(dev_priv); return 0; } -static int snb_runtime_resume(struct drm_i915_private *dev_priv) +static int snb_resume(struct drm_i915_private *dev_priv, + bool resume_from_rpm_suspend) { struct drm_device *dev = dev_priv->dev; - intel_init_pch_refclk(dev); + if (resume_from_rpm_suspend) + intel_init_pch_refclk(dev); return 0; } -static int hsw_runtime_resume(struct drm_i915_private *dev_priv) +static int hsw_resume(struct drm_i915_private *dev_priv, + bool resume_from_rpm_suspend) { hsw_disable_pc8(dev_priv); @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); } -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) +static int vlv_suspend(struct drm_i915_private *dev_priv) { u32 mask; - int err; + int ret = 0; /* * Bspec defines the following GT well on flags as debug only, so @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) vlv_check_no_gt_access(dev_priv); - err = vlv_force_gfx_clock(dev_priv, true); - if (err) + ret = vlv_force_gfx_clock(dev_priv, true); + if (ret) goto err1; - err = vlv_allow_gt_wake(dev_priv, false); - if (err) + ret = vlv_allow_gt_wake(dev_priv, false); + if (ret) goto err2; vlv_save_gunit_s0ix_state(dev_priv); - err = vlv_force_gfx_clock(dev_priv, false); - if (err) + ret = vlv_force_gfx_clock(dev_priv, false); + if (ret) goto err2; - - return 0; + return ret; err2: /* For safety always re-enable waking and disable gfx clock forcing */ @@ -1332,14 +1341,15 @@ err2: err1: vlv_force_gfx_clock(dev_priv, false); - return err; + return ret; } -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) +static int vlv_resume(struct drm_i915_private *dev_priv, + bool resume_from_rpm_suspend) { struct drm_device *dev = dev_priv->dev; int err; - int ret; + int ret = 0; /* * If any of the steps fail just try to continue, that's the best we @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) vlv_check_no_gt_access(dev_priv); - intel_init_clock_gating(dev); - i915_gem_restore_fences(dev); + if (resume_from_rpm_suspend) { + intel_init_clock_gating(dev); + i915_gem_restore_fences(dev); + } return ret; } @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device) cancel_work_sync(&dev_priv->rps.work); intel_runtime_pm_disable_interrupts(dev); - if (IS_GEN6(dev)) { - ret = 0; - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { - ret = hsw_runtime_suspend(dev_priv); - } else if (IS_VALLEYVIEW(dev)) { - ret = vlv_runtime_suspend(dev_priv); - } else { - ret = -ENODEV; - WARN_ON(1); - } + /* Save any platform specific registers/clk state needed post resume */ + ret = pre_hw_suspend_deinit(dev_priv); if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device) intel_opregion_notify_adapter(dev, PCI_D0); dev_priv->pm.suspended = false; - if (IS_GEN6(dev)) { - ret = snb_runtime_resume(dev_priv); - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { - ret = hsw_runtime_resume(dev_priv); - } else if (IS_VALLEYVIEW(dev)) { - ret = vlv_runtime_resume(dev_priv); - } else { - WARN_ON(1); - ret = -ENODEV; - } + /* Restore any platform specific registers/clk state */ + ret = post_hw_resume_init(dev_priv, true); /* * No point of rolling back things in case of an error, as the best @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device) return ret; } +/* This handler is used in system/runtime suspend path to reuse + * Gfx clock, Wake control, Gunit state save related functionaility for VLV. + */ +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv) +{ + struct drm_device *dev = dev_priv->dev; + int ret = 0; + + if (IS_GEN6(dev)) { + ret = 0; + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { + ret = hsw_suspend(dev_priv); + } else if (IS_VALLEYVIEW(dev)) { + ret = vlv_suspend(dev_priv); + } else { + ret = -ENODEV; + WARN_ON(1); + } + + return ret; +} + +/* This handler is used in system/runtime resume path to reuse + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV. + */ +static int post_hw_resume_init(struct drm_i915_private *dev_priv, + bool resume_from_rpm_suspend) +{ + struct drm_device *dev = dev_priv->dev; + int ret = 0; + + if (IS_GEN6(dev)) { + ret = snb_resume(dev_priv, resume_from_rpm_suspend); + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { + ret = hsw_resume(dev_priv, resume_from_rpm_suspend); + } else if (IS_VALLEYVIEW(dev)) { + ret = vlv_resume(dev_priv, resume_from_rpm_suspend); + } else { + WARN_ON(1); + ret = -ENODEV; + } + return ret; +} + static const struct dev_pm_ops i915_pm_ops = { .suspend = i915_pm_suspend, .suspend_late = i915_pm_suspend_late,