drm/i915: run intel_uncore_early_sanitize earlier on resume on non-VLV
Subsystems (2)
PATCH 1/1 drm/i915: run intel_uncore_early_sanitize earlier on resume on non-VLV
From: Paulo Zanoni <[email protected]>
As far as I understand, intel_uncore_early_sanitize() was supposed to be ran before any register access, but currently intel_resume_prepare() is ran earlier, and it does register access. I don't think it should be safe to be calling I915_{READ,WRITE} without calling intel_uncore_early_sanitize() first.
One of the problems we currently have is that when we suspend/resume BDW, the FPGA_DBG_RM_NOCLAIM bit becomes 1, so we end up printing an "unclaimed register" message on resume, but this message doesn't really seem to have been triggered by our driver or user space, since the bit was not there before suspending, and gets there just after resuming, before any of our own register accesses. So calling intel_uncore_early_sanitize() as a first thing will allow us to stop printing the error message, fixing the "bug".
v2: VLV is an exception to the early_sanitize() rule: it needs to do stuff before calling early_sanitize(), so instead of calling it earlier for every platform, we call it earlier for non-VLV by adding the early_sanitize() call inside intel_resume_prepare(). This doesn't look like the most-beautiful-solution-ever, but, well, at least it fixes the bug. (Imre)
Cc: Chris Wilson <[email protected]>
Cc: Imre Deak <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094
--- drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_drv.c +8 -1
@@ -669,7 +669,6 @@ static int i915_drm_thaw_early(struct drm_device *dev)if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);
@@ -1049,6 +1048,8 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,if (rpm_resume)intel_init_pch_refclk (dev); +else +intel_uncore_early_sanitize (dev, true);return 0 ; }
@@ -1056,6 +1057,9 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,static int hsw_resume_prepare (struct drm_i915_private *dev_priv,bool rpm_resume) { +if (!rpm_resume) +intel_uncore_early_sanitize (dev_priv->dev, true); +hsw_disable_pc8 (dev_priv);return 0 ;
@@ -1421,6 +1425,9 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,i915_gem_restore_fences (dev); } +if (!rpm_resume) +intel_uncore_early_sanitize (dev, true); +return ret; }
Signature · 1 line
2.1.1
Quoted 64 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
As far as I understand,
intel_uncore_early_sanitize()was supposed to be ran before any register access, but currentlyintel_resume_prepare()is ran earlier, and it does register access. I don't think it should be safe to be calling I915_{READ,WRITE} without callingintel_uncore_early_sanitize()first.One of the problems we currently have is that when we suspend/resume BDW, the
FPGA_DBG_RM_NOCLAIMbit becomes 1, so we end up printing an "unclaimed register" message on resume, but this message doesn't really seem to have been triggered by our driver or user space, since the bit was not there before suspending, and gets there just after resuming, before any of our own register accesses. So callingintel_uncore_early_sanitize()as a first thing will allow us to stop printing the error message, fixing the "bug".v2: VLV is an exception to the
early_sanitize()rule: it needs to do stuff before callingearly_sanitize(), so instead of calling it earlier for every platform, we call it earlier for non-VLV by adding theearly_sanitize()call insideintel_resume_prepare(). This doesn't look like the most-beautiful-solution-ever, but, well, at least it fixes the bug. (Imre)Cc: Chris Wilson <[email protected]>
Cc: Imre Deak <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/i915_drv.c +5
-1@@ -669,7 +669,6 @@ static int i915_drm_thaw_early(struct drm_device *dev)if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);@@ -1049,6 +1048,8 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,if (rpm_resume)intel_init_pch_refclk (dev); +else +intel_uncore_early_sanitize (dev, true);return 0 ; }@@ -1056,6 +1057,9 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,static int hsw_resume_prepare (struct drm_i915_private *dev_priv,bool rpm_resume) { +if (!rpm_resume) +intel_uncore_early_sanitize (dev_priv->dev, true); +hsw_disable_pc8 (dev_priv);return 0 ;
drivers/gpu/drm/i915/i915_drv.c static int vlv_resume_prepare(struct drm_i915_private *dev_priv, · line 1433
Quoted 9 lines
drivers/gpu/drm/i915/i915_drv.c +3
-0@@ -1421,6 +1425,9 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,i915_gem_restore_fences (dev); } +if (!rpm_resume) +intel_uncore_early_sanitize (dev, true); +return ret; }
You also need to call intel_uncore_early_sanitize() from intel_resume_prepare() for the rest of the platforms. With that fixed:
Looking at the result, I agree it's not the nicest, so yet another way to reduce the clutter would be to have the following instead in i915_drm_thaw_early():
intel_resume_early_prepare()intel_uncore_early_sanitize()intel_resume_prepare()
and do the early steps for VLV in intel_resume_early_prepare(). I'm ok with both solutions.
--Imre
Quoted 83 lines · Imre Deak
Quoted 64 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
As far as I understand,
intel_uncore_early_sanitize()was supposed to be ran before any register access, but currentlyintel_resume_prepare()is ran earlier, and it does register access. I don't think it should be safe to be calling I915_{READ,WRITE} without callingintel_uncore_early_sanitize()first.One of the problems we currently have is that when we suspend/resume BDW, the
FPGA_DBG_RM_NOCLAIMbit becomes 1, so we end up printing an "unclaimed register" message on resume, but this message doesn't really seem to have been triggered by our driver or user space, since the bit was not there before suspending, and gets there just after resuming, before any of our own register accesses. So callingintel_uncore_early_sanitize()as a first thing will allow us to stop printing the error message, fixing the "bug".v2: VLV is an exception to the
early_sanitize()rule: it needs to do stuff before callingearly_sanitize(), so instead of calling it earlier for every platform, we call it earlier for non-VLV by adding theearly_sanitize()call insideintel_resume_prepare(). This doesn't look like the most-beautiful-solution-ever, but, well, at least it fixes the bug. (Imre)Cc: Chris Wilson <[email protected]>
Cc: Imre Deak <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/i915_drv.c +5
-1@@ -669,7 +669,6 @@ static int i915_drm_thaw_early(struct drm_device *dev)if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);@@ -1049,6 +1048,8 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,if (rpm_resume)intel_init_pch_refclk (dev); +else +intel_uncore_early_sanitize (dev, true);return 0 ; }@@ -1056,6 +1057,9 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,static int hsw_resume_prepare (struct drm_i915_private *dev_priv,bool rpm_resume) { +if (!rpm_resume) +intel_uncore_early_sanitize (dev_priv->dev, true); +hsw_disable_pc8 (dev_priv);return 0 ;drivers/gpu/drm/i915/i915_drv.c static int vlv_resume_prepare(struct drm_i915_private *dev_priv, · line 1433
Quoted 9 lines
drivers/gpu/drm/i915/i915_drv.c +3
-0@@ -1421,6 +1425,9 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,i915_gem_restore_fences (dev); } +if (!rpm_resume) +intel_uncore_early_sanitize (dev, true); +return ret; }You also need to call
intel_uncore_early_sanitize()fromintel_resume_prepare()for the rest of the platforms. With that fixed:Reviewed-by Imre Deak <[email protected]>Looking at the result, I agree it's not the nicest, so yet another way to reduce the clutter would be to have the following instead in
i915_drm_thaw_early():
Quoted 6 lines
intel_resume_early_prepare()intel_uncore_early_sanitize()intel_resume_prepare()and do the early steps for VLV in
intel_resume_early_prepare(). I'm ok with both solutions.
This honestly starts to smell like a giant maintenance nightmare. We kinda started off into the wrong direction with vlv rpm and it seems to get worse by the day. And it looks like the situation is messy enough that we can't even look down the ordering with copious amounts of warnings ...
But I also don't see any real solution, so just ranting for now. I'd appreciate though if the revised version comes with a bunch of comments attached in the code.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 91 lines · Daniel Vetter
Quoted 83 lines · Imre Deak
Quoted 64 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
As far as I understand,
intel_uncore_early_sanitize()was supposed to be ran before any register access, but currentlyintel_resume_prepare()is ran earlier, and it does register access. I don't think it should be safe to be calling I915_{READ,WRITE} without callingintel_uncore_early_sanitize()first.One of the problems we currently have is that when we suspend/resume BDW, the
FPGA_DBG_RM_NOCLAIMbit becomes 1, so we end up printing an "unclaimed register" message on resume, but this message doesn't really seem to have been triggered by our driver or user space, since the bit was not there before suspending, and gets there just after resuming, before any of our own register accesses. So callingintel_uncore_early_sanitize()as a first thing will allow us to stop printing the error message, fixing the "bug".v2: VLV is an exception to the
early_sanitize()rule: it needs to do stuff before callingearly_sanitize(), so instead of calling it earlier for every platform, we call it earlier for non-VLV by adding theearly_sanitize()call insideintel_resume_prepare(). This doesn't look like the most-beautiful-solution-ever, but, well, at least it fixes the bug. (Imre)Cc: Chris Wilson <[email protected]>
Cc: Imre Deak <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/i915_drv.c +5
-1@@ -669,7 +669,6 @@ static int i915_drm_thaw_early(struct drm_device *dev)if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);@@ -1049,6 +1048,8 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,if (rpm_resume)intel_init_pch_refclk (dev); +else +intel_uncore_early_sanitize (dev, true);return 0 ; }@@ -1056,6 +1057,9 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,static int hsw_resume_prepare (struct drm_i915_private *dev_priv,bool rpm_resume) { +if (!rpm_resume) +intel_uncore_early_sanitize (dev_priv->dev, true); +hsw_disable_pc8 (dev_priv);return 0 ;drivers/gpu/drm/i915/i915_drv.c static int vlv_resume_prepare(struct drm_i915_private *dev_priv, · line 1433
Quoted 9 lines
drivers/gpu/drm/i915/i915_drv.c +3
-0@@ -1421,6 +1425,9 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,i915_gem_restore_fences (dev); } +if (!rpm_resume) +intel_uncore_early_sanitize (dev, true); +return ret; }You also need to call
intel_uncore_early_sanitize()fromintel_resume_prepare()for the rest of the platforms. With that fixed:Reviewed-by Imre Deak <[email protected]>Looking at the result, I agree it's not the nicest, so yet another way to reduce the clutter would be to have the following instead in
i915_drm_thaw_early():Quoted 6 lines
intel_resume_early_prepare()intel_uncore_early_sanitize()intel_resume_prepare()and do the early steps for VLV in
intel_resume_early_prepare(). I'm ok with both solutions.
Quoted 8 lines
This honestly starts to smell like a giant maintenance nightmare. We kinda started off into the wrong direction with vlv rpm and it seems to get worse by the day. And it looks like the situation is messy enough that we can't even look down the ordering with copious amounts of warnings ...
But I also don't see any real solution, so just ranting for now. I'd appreciate though if the revised version comes with a bunch of comments attached in the code.
I blame it on the HW people. :) Seriously, the VLV PM code differs from the rest of PM code in that we save/restore some HW state instead of reinitializing it. That's where the above special casing of the ordering stems from. I agree that it's not ideal, but I think having started with that solution and moving towards the ideal was not that bad. In fact s0ix doesn't yet work in the upstream kernel for reasons independent of i915 (or at least I couldn't make it work), but we would need it to fully validate all the suspend/resume paths.
--Imre
2014-10-22 9:20 GMT-02:00 Imre Deak <[email protected]>:
Quoted 101 lines · Daniel Vetter
Quoted 91 lines · Imre Deak
Quoted 83 lines · Paulo Zanoni
Quoted 64 lines
From: Paulo Zanoni <[email protected]>
As far as I understand,
intel_uncore_early_sanitize()was supposed to be ran before any register access, but currentlyintel_resume_prepare()is ran earlier, and it does register access. I don't think it should be safe to be calling I915_{READ,WRITE} without callingintel_uncore_early_sanitize()first.One of the problems we currently have is that when we suspend/resume BDW, the
FPGA_DBG_RM_NOCLAIMbit becomes 1, so we end up printing an "unclaimed register" message on resume, but this message doesn't really seem to have been triggered by our driver or user space, since the bit was not there before suspending, and gets there just after resuming, before any of our own register accesses. So callingintel_uncore_early_sanitize()as a first thing will allow us to stop printing the error message, fixing the "bug".v2: VLV is an exception to the
early_sanitize()rule: it needs to do stuff before callingearly_sanitize(), so instead of calling it earlier for every platform, we call it earlier for non-VLV by adding theearly_sanitize()call insideintel_resume_prepare(). This doesn't look like the most-beautiful-solution-ever, but, well, at least it fixes the bug. (Imre)Cc: Chris Wilson <[email protected]>
Cc: Imre Deak <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/i915_drv.c +5
-1@@ -669,7 +669,6 @@ static int i915_drm_thaw_early(struct drm_device *dev)if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);@@ -1049,6 +1048,8 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,if (rpm_resume)intel_init_pch_refclk (dev); +else +intel_uncore_early_sanitize (dev, true);return 0 ; }@@ -1056,6 +1057,9 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,static int hsw_resume_prepare (struct drm_i915_private *dev_priv,bool rpm_resume) { +if (!rpm_resume) +intel_uncore_early_sanitize (dev_priv->dev, true); +hsw_disable_pc8 (dev_priv);return 0 ;drivers/gpu/drm/i915/i915_drv.c static int vlv_resume_prepare(struct drm_i915_private *dev_priv, · line 1433
Quoted 9 lines
drivers/gpu/drm/i915/i915_drv.c +3
-0@@ -1421,6 +1425,9 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,i915_gem_restore_fences (dev); } +if (!rpm_resume) +intel_uncore_early_sanitize (dev, true); +return ret; }You also need to call
intel_uncore_early_sanitize()fromintel_resume_prepare()for the rest of the platforms. With that fixed:Reviewed-by Imre Deak <[email protected]>Looking at the result, I agree it's not the nicest, so yet another way to reduce the clutter would be to have the following instead in
i915_drm_thaw_early():Quoted 6 lines
intel_resume_early_prepare()intel_uncore_early_sanitize()intel_resume_prepare()and do the early steps for VLV in
intel_resume_early_prepare(). I'm ok with both solutions.Quoted 8 lines
This honestly starts to smell like a giant maintenance nightmare. We kinda started off into the wrong direction with vlv rpm and it seems to get worse by the day. And it looks like the situation is messy enough that we can't even look down the ordering with copious amounts of warnings ...
But I also don't see any real solution, so just ranting for now. I'd appreciate though if the revised version comes with a bunch of comments attached in the code.
Quoted 8 lines
I blame it on the HW people. :) Seriously, the VLV PM code differs from the rest of PM code in that we save/restore some HW state instead of reinitializing it. That's where the above special casing of the ordering stems from. I agree that it's not ideal, but I think having started with that solution and moving towards the ideal was not that bad. In fact s0ix doesn't yet work in the upstream kernel for reasons independent of i915 (or at least I couldn't make it work), but we would need it to fully validate all the suspend/resume paths.
On a side note, even igt/pm_rpm/rte (the basic subtest) seems to be broken on BYT since forever (at least according to QA, bug #82939), so do we even want RPM enabled on BYT?
Quoted 3 lines
--Imre
Signature · 1 line
Paulo Zanoni
PATCH 1/2 drm/i915: kill intel_resume_prepare()
From: Paulo Zanoni <[email protected]>
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!
--- drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +15 -48
@@ -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_freeze (struct drm_device *dev) {
@@ -663,12 +663,16 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)static int i915_drm_thaw_early (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; -int ret; +int ret =0 ; - 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); +intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);
@@ -1051,25 +1055,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
@@ -1527,7 +1512,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 ;
@@ -1537,7 +1522,12 @@ 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).
@@ -1575,29 +1565,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 = { .suspend = i915_pm_suspend, .suspend_late = i915_pm_suspend_late,
Signature · 1 line
2.1.1
PATCH 2/2 drm/i915: run hsw_disable_pc8() later on resume
From: Paulo Zanoni <[email protected]>
We want to run intel_uncore_early_sanitize() before we touch any registers, because on BDW, when we resume, the FPGA_DBG_RM_NOCLAIM bit is set, so we need to clear it - through intel_uncore_early_sanitize()
- before we do anything else. With the current code, we don't clear the bit before our first register access, so we print a WARN complaining about an unclaimed register error.
v1: Was called "drm/i915: run intel_uncore_early_sanitize earlier on resume"
v2: Was called "drm/i915: run intel_uncore_early_sanitize earlier on resume on non-VLV"
v3: This one, on top of the intel_resume_prepare() rework.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094
--- drivers/gpu/drm/i915/i915_drv.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +5 -4
@@ -665,15 +665,16 @@ 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_priv) ||IS_BROADWELL (dev_priv)) -hsw_disable_pc8 (dev_priv); -else if (IS_VALLEYVIEW (dev_priv)) +if (IS_VALLEYVIEW (dev_priv)) ret =vlv_resume_prepare (dev_priv, false);if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true); + +if (IS_HASWELL (dev_priv) ||IS_BROADWELL (dev_priv)) +hsw_disable_pc8 (dev_priv); +intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);
Signature · 1 line
2.1.1
Re: [PATCH 2/2] drm/i915: run hsw_disable_pc8() later on resume
Quoted 9 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We want to run
intel_uncore_early_sanitize()before we touch any registers, because on BDW, when we resume, theFPGA_DBG_RM_NOCLAIMbit is set, so we need to clear it - throughintel_uncore_early_sanitize()
- before we do anything else. With the current code, we don't clear the bit before our first register access, so we print a WARN complaining about an unclaimed register error.
Quoted 8 lines
v1: Was called "drm/i915: run
intel_uncore_early_sanitizeearlier on resume"
v2: Was called "drm/i915: runintel_uncore_early_sanitizeearlier on resume on non-VLV"
v3: This one, on top of theintel_resume_prepare()rework.Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094
Signed-off-by Paulo Zanoni <[email protected]>
Yeah, I think this looks better. I'll keep on being grumpy about vlv doing all the register save/restore stuff it really shouldn't be doing though ;-) But that's definitely and entirely different thing.
Cheers, Daniel
Quoted 36 lines
--- drivers/gpu/drm/i915/i915_drv.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/gpu/drm/i915/i915_drv.c +5
-4@@ -665,15 +665,16 @@ 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_priv) ||IS_BROADWELL (dev_priv)) -hsw_disable_pc8 (dev_priv); -else if (IS_VALLEYVIEW (dev_priv)) +if (IS_VALLEYVIEW (dev_priv)) ret =vlv_resume_prepare (dev_priv, false);if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true); + +if (IS_HASWELL (dev_priv) ||IS_BROADWELL (dev_priv)) +hsw_disable_pc8 (dev_priv); +intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 1/2 drm/i915: kill intel_resume_prepare()
From: Paulo Zanoni <[email protected]>
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!
v2: - Rebase.
--- drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +15 -48
@@ -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,
Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/2 drm/i915: run hsw_disable_pc8() later on resume
From: Paulo Zanoni <[email protected]>
We want to run intel_uncore_early_sanitize() before we touch any registers, because on BDW, when we resume, the FPGA_DBG_RM_NOCLAIM bit is set, so we need to clear it - through intel_uncore_early_sanitize()
- before we do anything else. With the current code, we don't clear the bit before our first register access, so we print a WARN complaining about an unclaimed register error.
v1: Was called "drm/i915: run intel_uncore_early_sanitize earlier on resume"
v2: Was called "drm/i915: run intel_uncore_early_sanitize earlier on resume on non-VLV"
v3: This one, on top of the intel_resume_prepare() rework.
v4: Rebase.
--- drivers/gpu/drm/i915/i915_drv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +5 -3
@@ -760,14 +760,16 @@ static int i915_drm_resume_early(struct drm_device *dev)pci_set_master (dev->pdev); -if (IS_HASWELL (dev_priv) ||IS_BROADWELL (dev_priv)) -hsw_disable_pc8 (dev_priv); -else if (IS_VALLEYVIEW (dev_priv)) +if (IS_VALLEYVIEW (dev_priv)) ret =vlv_resume_prepare (dev_priv, false);if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret);intel_uncore_early_sanitize (dev, true); + +if (IS_HASWELL (dev_priv) ||IS_BROADWELL (dev_priv)) +hsw_disable_pc8 (dev_priv); +intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);
Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 1/2] drm/i915: kill intel_resume_prepare()
In reply to Paulo ZanoniQuoted 10 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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
Quoted 114 lines
v2: - Rebase.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)drivers/gpu/drm/i915/i915_drv.c +15
-41@@ -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))
drivers/gpu/drm/i915/i915_drv.c static int intel_suspend_complete(struct drm_i915_private *dev_priv) · line 1505
Quoted 10 lines
drivers/gpu/drm/i915/i915_drv.c +0
-7- 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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 1/2] drm/i915: kill intel_resume_prepare()
2014-10-28 11:12 GMT-02:00 Imre Deak <[email protected]>:
Quoted 10 lines · Paulo Zanoni
Quoted 9 lines
From: Paulo Zanoni <[email protected]>
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.
Quoted 5 lines
Quoted 1 line
Also, look at the diffstat!
The reason for
intel_resume_prepare()andintel_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().
Quoted 5 lines
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 callingintel_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...
Quoted 4 lines
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.
Quoted 132 lines
--Imre
[1]
http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.htmlQuoted 124 lines
v2: - Rebase.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)drivers/gpu/drm/i915/i915_drv.c +15
-48@@ -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,
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak
Reviewed-byRe: [PATCH 1/2] drm/i915: kill intel_resume_prepare()
Quoted 17 lines · Paulo Zanoni
2014-10-28 11:12 GMT-02:00 Imre Deak <[email protected]>:
Quoted 10 lines · Paulo Zanoni
Quoted 9 lines
From: Paulo Zanoni <[email protected]>
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.Quoted 5 lines
Quoted 1 line
Also, look at the diffstat!
The reason for
intel_resume_prepare()andintel_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].
Quoted 5 lines
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.
Quoted 6 lines
I know that adding more
IS_VLVchecks 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 killingintel_resume_prepare().
Quoted 8 lines
Quoted 5 lines
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 callingintel_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.
Quoted 9 lines
Quoted 3 lines
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:
Quoted 125 lines
Quoted 125 lines
--Imre
[1]
http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.htmlQuoted 118 lines
v2: - Rebase.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)drivers/gpu/drm/i915/i915_drv.c +15
-45@@ -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 ; -
Quoted 11 lines
Quoted 8 lines
drivers/gpu/drm/i915/i915_drv.c static int intel_suspend_complete(struct drm_i915_private *dev_priv) · line 1505
Quoted 6 lines
drivers/gpu/drm/i915/i915_drv.c +0
-3-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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 1/2] drm/i915: kill intel_resume_prepare()
Quoted 63 lines · Imre Deak
Quoted 17 lines · Paulo Zanoni
2014-10-28 11:12 GMT-02:00 Imre Deak <[email protected]>:
Quoted 10 lines · Paulo Zanoni
Quoted 9 lines
From: Paulo Zanoni <[email protected]>
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.Quoted 5 lines
Quoted 1 line
Also, look at the diffstat!
The reason for
intel_resume_prepare()andintel_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].Quoted 5 lines
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()insnb_resume_prepare()could be just as well called for both paths.Quoted 6 lines
I know that adding more
IS_VLVchecks 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 killingintel_resume_prepare().Quoted 8 lines
Quoted 5 lines
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 callingintel_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.
Quoted 9 lines
Quoted 3 lines
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.
Quoted 6 lines
It's confusing to call
vlv_resume_prepare(),hsw_disable_pc8()fromi915_drm_resume_early()directly and have to jump over some wrappers to find the correspondingvlv_suspend_complete(),hsw_enable_pc8()ini915_drm_suspend_complete(). But this is somewhat bikeshedding, so either way feel free to add on both patches:Reviewed-by Imre Deak <[email protected]>
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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 111 lines · Paulo Zanoni
2014-10-22 9:20 GMT-02:00 Imre Deak <[email protected]>:
Quoted 101 lines · Daniel Vetter
Quoted 91 lines · Imre Deak
Quoted 83 lines · Paulo Zanoni
Quoted 64 lines
From: Paulo Zanoni <[email protected]>
As far as I understand,
intel_uncore_early_sanitize()was supposed to be ran before any register access, but currentlyintel_resume_prepare()is ran earlier, and it does register access. I don't think it should be safe to be calling I915_{READ,WRITE} without callingintel_uncore_early_sanitize()first.One of the problems we currently have is that when we suspend/resume BDW, the
FPGA_DBG_RM_NOCLAIMbit becomes 1, so we end up printing an "unclaimed register" message on resume, but this message doesn't really seem to have been triggered by our driver or user space, since the bit was not there before suspending, and gets there just after resuming, before any of our own register accesses. So callingintel_uncore_early_sanitize()as a first thing will allow us to stop printing the error message, fixing the "bug".v2: VLV is an exception to the
early_sanitize()rule: it needs to do stuff before callingearly_sanitize(), so instead of calling it earlier for every platform, we call it earlier for non-VLV by adding theearly_sanitize()call insideintel_resume_prepare(). This doesn't look like the most-beautiful-solution-ever, but, well, at least it fixes the bug. (Imre)Cc: Chris Wilson <[email protected]>
Cc: Imre Deak <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83094Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/i915_drv.c +5
-1@@ -669,7 +669,6 @@ static int i915_drm_thaw_early(struct drm_device *dev)if (ret)DRM_ERROR ("Resume prepare failed: %d,Continuing resume\n" , ret); -intel_uncore_early_sanitize (dev, true);intel_uncore_sanitize (dev);intel_power_domains_init_hw (dev_priv);@@ -1049,6 +1048,8 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,if (rpm_resume)intel_init_pch_refclk (dev); +else +intel_uncore_early_sanitize (dev, true);return 0 ; }@@ -1056,6 +1057,9 @@ static int snb_resume_prepare(struct drm_i915_private *dev_priv,static int hsw_resume_prepare (struct drm_i915_private *dev_priv,bool rpm_resume) { +if (!rpm_resume) +intel_uncore_early_sanitize (dev_priv->dev, true); +hsw_disable_pc8 (dev_priv);return 0 ;drivers/gpu/drm/i915/i915_drv.c static int vlv_resume_prepare(struct drm_i915_private *dev_priv, · line 1433
Quoted 9 lines
drivers/gpu/drm/i915/i915_drv.c +3
-0@@ -1421,6 +1425,9 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,i915_gem_restore_fences (dev); } +if (!rpm_resume) +intel_uncore_early_sanitize (dev, true); +return ret; }You also need to call
intel_uncore_early_sanitize()fromintel_resume_prepare()for the rest of the platforms. With that fixed:Reviewed-by Imre Deak <[email protected]>Looking at the result, I agree it's not the nicest, so yet another way to reduce the clutter would be to have the following instead in
i915_drm_thaw_early():Quoted 6 lines
intel_resume_early_prepare()intel_uncore_early_sanitize()intel_resume_prepare()and do the early steps for VLV in
intel_resume_early_prepare(). I'm ok with both solutions.Quoted 8 lines
This honestly starts to smell like a giant maintenance nightmare. We kinda started off into the wrong direction with vlv rpm and it seems to get worse by the day. And it looks like the situation is messy enough that we can't even look down the ordering with copious amounts of warnings ...
But I also don't see any real solution, so just ranting for now. I'd appreciate though if the revised version comes with a bunch of comments attached in the code.
Quoted 8 lines
I blame it on the HW people. :) Seriously, the VLV PM code differs from the rest of PM code in that we save/restore some HW state instead of reinitializing it. That's where the above special casing of the ordering stems from. I agree that it's not ideal, but I think having started with that solution and moving towards the ideal was not that bad. In fact s0ix doesn't yet work in the upstream kernel for reasons independent of i915 (or at least I couldn't make it work), but we would need it to fully validate all the suspend/resume paths.
Quoted 3 lines
On a side note, even igt/pm_rpm/rte (the basic subtest) seems to be broken on BYT since forever (at least according to QA, bug #82939), so do we even want RPM enabled on BYT?
If it's really broken and not just the test being pedantic about something, then yes please submit the revert. Submitting reverts is the best way to make sure PM and engineers are aware that something didn't go as it should have, and it's the quickest way to rack up good bug team stats. So please bring them on.
This is btw true for regressions in general: If you don't see action on a bug, then just send in the revert. Either the original author can come up with fix within a few days, or I'll just merge the revert. In either case, bug stats improve.
Cheers, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch