| Message ID | 1414439673-1994-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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@intel.com> > --- > 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,
2014-10-28 11:12 GMT-02:00 Imre Deak <imre.deak@intel.com>: > On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> 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]. Well, IMHO we are just pretending to "share" the code paths between S3 and RPM because we still have the "bool rpm_resume" argument on the current code. IMHO this is one of the biggest reasons why the code became so complex: take a look at the second version of my patch, it added even more "if (rpm_resume)" checks. I know that adding more IS_VLV checks is not cool, but the S3 and RPM code paths are very different here, and the VLV code even requires a really-weird ordering. I _really_ think the code becomes much easier to read/understand/modify by just killing intel_resume_prepare(). > 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. That still wouldn't solve the ordering problem we have on S3. My goal is just to fix a very simple bug with our function ordering... > > 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. I don't think so. It doesn't have all the "bool rpm_resume" confusion, there's no function ordering issues with it, and both the S3 and RPM versions are actually identical. I don't think killing it is relevant to the bug we're trying to fix here, and I also don't think that removing t would be an improvement to the code base. > > --Imre > > [1] > http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.html > >> >> v2: - Rebase. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> 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, > >
On Tue, 2014-10-28 at 11:43 -0200, Paulo Zanoni wrote: > 2014-10-28 11:12 GMT-02:00 Imre Deak <imre.deak@intel.com>: > > On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> 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]. > > Well, IMHO we are just pretending to "share" the code paths between S3 > and RPM because we still have the "bool rpm_resume" argument on the > current code. IMHO this is one of the biggest reasons why the code > became so complex: take a look at the second version of my patch, it > added even more "if (rpm_resume)" checks. Yes, but that's one more thing we could try to remove at one point. For example intel_init_pch_refclk() in snb_resume_prepare() could be just as well called for both paths. > I know that adding more > IS_VLV checks is not cool, but the S3 and RPM code paths are very > different here, and the VLV code even requires a really-weird > ordering. I _really_ think the code becomes much easier to > read/understand/modify by just killing intel_resume_prepare(). > > > 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. > > That still wouldn't solve the ordering problem we have on S3. My goal > is just to fix a very simple bug with our function ordering... I think it would by having in intel_resume_prepare(): if (IS_VALLEYVIEW(dev_priv)) ret = vlv_resume_prepare(dev_priv, rpm_resume); intel_uncore_early_sanitize(dev,true); if (IS_HASWELL(dev_priv) ||IS_BROADWELL(dev_priv)) hsw_disable_pc8(dev_priv); and call it 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. > > I don't think so. It doesn't have all the "bool rpm_resume" confusion, > there's no function ordering issues with it, and both the S3 and RPM > versions are actually identical. I don't think killing it is relevant > to the bug we're trying to fix here, and I also don't think that > removing t would be an improvement to the code base. It's confusing to call vlv_resume_prepare(), hsw_disable_pc8() from i915_drm_resume_early() directly and have to jump over some wrappers to find the corresponding vlv_suspend_complete(), hsw_enable_pc8() in i915_drm_suspend_complete(). But this is somewhat bikeshedding, so either way feel free to add on both patches: Reviewed-by: Imre Deak <imre.deak@intel.com> > > > > > --Imre > > > > [1] > > http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.html > > > >> > >> v2: - Rebase. > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> 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, > > > > > > >
On Tue, Oct 28, 2014 at 04:47:14PM +0200, Imre Deak wrote: > On Tue, 2014-10-28 at 11:43 -0200, Paulo Zanoni wrote: > > 2014-10-28 11:12 GMT-02:00 Imre Deak <imre.deak@intel.com>: > > > On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote: > > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >> > > >> 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]. > > > > Well, IMHO we are just pretending to "share" the code paths between S3 > > and RPM because we still have the "bool rpm_resume" argument on the > > current code. IMHO this is one of the biggest reasons why the code > > became so complex: take a look at the second version of my patch, it > > added even more "if (rpm_resume)" checks. > > Yes, but that's one more thing we could try to remove at one point. For > example intel_init_pch_refclk() in snb_resume_prepare() could be just as > well called for both paths. > > > I know that adding more > > IS_VLV checks is not cool, but the S3 and RPM code paths are very > > different here, and the VLV code even requires a really-weird > > ordering. I _really_ think the code becomes much easier to > > read/understand/modify by just killing intel_resume_prepare(). > > > > > 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. > > > > That still wouldn't solve the ordering problem we have on S3. My goal > > is just to fix a very simple bug with our function ordering... > > I think it would by having in intel_resume_prepare(): > > if (IS_VALLEYVIEW(dev_priv)) > ret = vlv_resume_prepare(dev_priv, rpm_resume); > intel_uncore_early_sanitize(dev,true); > if (IS_HASWELL(dev_priv) ||IS_BROADWELL(dev_priv)) > hsw_disable_pc8(dev_priv); > > and call it 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. > > > > I don't think so. It doesn't have all the "bool rpm_resume" confusion, > > there's no function ordering issues with it, and both the S3 and RPM > > versions are actually identical. I don't think killing it is relevant > > to the bug we're trying to fix here, and I also don't think that > > removing t would be an improvement to the code base. > > It's confusing to call vlv_resume_prepare(), hsw_disable_pc8() from > i915_drm_resume_early() directly and have to jump over some wrappers to > find the corresponding vlv_suspend_complete(), hsw_enable_pc8() in > i915_drm_suspend_complete(). But this is somewhat bikeshedding, so > either way feel free to add on both patches: > Reviewed-by: Imre Deak <imre.deak@intel.com> Yeah that first attempt at unifying rpm and s3 didn't really pan out at all. I guess we need to retry. Meanwhile merged both patches to dinq. Thanks, Daniel
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,