On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > Because, really, the abstraction is not working for us. It is nice for > VLV, but doesn't add anything useful on SNB/HSW/BDW. We want to change > this code due to a recently-discovered bug, but we can't seem to find > a nice solution that repects the current abstraction. So let's kill > intel_resume_prepare() and its friends, and add an equivalent > implementation to both its callers. > > Also, look at the diffstat! The reason for intel_resume_prepare() and intel_suspend_complete() was to contain platform dependent code in those and to share parts between the system and runtime suspend code, see the discussion at [1]. I still think this is a good idea, but I admit we need to work on it more, by sharing more between the two paths. So for example instead of doing this revert now I would consider calling intel_uncore_early_sanitize() for both system and runtime resume. But if that's not feasible and you want to go ahead with the removal then please also remove intel_suspend_complete(), leaving it in would be confusing imo. --Imre [1] http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.html > > v2: - Rebase. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++------------------------------- > 1 file changed, 15 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 035ec94..33b6fc4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -551,8 +551,8 @@ static void intel_suspend_encoders(struct drm_i915_private *dev_priv) > } > > static int intel_suspend_complete(struct drm_i915_private *dev_priv); > -static int intel_resume_prepare(struct drm_i915_private *dev_priv, > - bool rpm_resume); > +static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > + bool rpm_resume); > > static int i915_drm_suspend(struct drm_device *dev) > { > @@ -744,7 +744,7 @@ static int i915_drm_resume(struct drm_device *dev) > static int i915_drm_resume_early(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - int ret; > + int ret = 0; > > /* > * We have a resume ordering issue with the snd-hda driver also > @@ -760,7 +760,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > > pci_set_master(dev->pdev); > > - ret = intel_resume_prepare(dev_priv, false); > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > + hsw_disable_pc8(dev_priv); > + else if (IS_VALLEYVIEW(dev_priv)) > + ret = vlv_resume_prepare(dev_priv, false); > if (ret) > DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret); > > @@ -986,25 +989,6 @@ static int hsw_suspend_complete(struct drm_i915_private *dev_priv) > return 0; > } > > -static int snb_resume_prepare(struct drm_i915_private *dev_priv, > - bool rpm_resume) > -{ > - struct drm_device *dev = dev_priv->dev; > - > - if (rpm_resume) > - intel_init_pch_refclk(dev); > - > - return 0; > -} > - > -static int hsw_resume_prepare(struct drm_i915_private *dev_priv, > - bool rpm_resume) > -{ > - hsw_disable_pc8(dev_priv); > - > - return 0; > -} > - > /* > * Save all Gunit registers that may be lost after a D3 and a subsequent > * S0i[R123] transition. The list of registers needing a save/restore is > @@ -1462,7 +1446,7 @@ static int intel_runtime_resume(struct device *device) > struct pci_dev *pdev = to_pci_dev(device); > struct drm_device *dev = pci_get_drvdata(pdev); > struct drm_i915_private *dev_priv = dev->dev_private; > - int ret; > + int ret = 0; > > if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev))) > return -ENODEV; > @@ -1472,7 +1456,13 @@ static int intel_runtime_resume(struct device *device) > intel_opregion_notify_adapter(dev, PCI_D0); > dev_priv->pm.suspended = false; > > - ret = intel_resume_prepare(dev_priv, true); > + if (IS_GEN6(dev_priv)) > + intel_init_pch_refclk(dev); > + else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > + hsw_disable_pc8(dev_priv); > + else if (IS_VALLEYVIEW(dev_priv)) > + ret = vlv_resume_prepare(dev_priv, true); > + > /* > * No point of rolling back things in case of an error, as the best > * we can do is to hope that things will still work (and disable RPM). > @@ -1510,29 +1500,6 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv) > return ret; > } > > -/* > - * This function implements common functionality of runtime and system > - * resume sequence. Variable rpm_resume used for implementing different > - * code paths. > - */ > -static int intel_resume_prepare(struct drm_i915_private *dev_priv, > - bool rpm_resume) > -{ > - struct drm_device *dev = dev_priv->dev; > - int ret; > - > - if (IS_GEN6(dev)) > - ret = snb_resume_prepare(dev_priv, rpm_resume); > - else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > - ret = hsw_resume_prepare(dev_priv, rpm_resume); > - else if (IS_VALLEYVIEW(dev)) > - ret = vlv_resume_prepare(dev_priv, rpm_resume); > - else > - ret = 0; > - > - return ret; > -} > - > static const struct dev_pm_ops i915_pm_ops = { > /* > * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
