drm/i915: run intel_uncore_early_sanitize earlier on resume
Subsystems (2)
PATCH 1/1 drm/i915: run intel_uncore_early_sanitize earlier on resume
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".
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Maybe we need to move even more code up?
drivers/gpu/drm/i915/i915_drv.c +1 -1
@@ -665,11 +665,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;int ret; +intel_uncore_early_sanitize (dev, true); ret =intel_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);
Signature · 1 line
2.1.1
Quoted 8 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.
Quoted 8 lines
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".
One issue is that intel_uncore_early_sanitize() uses forcewake, which is enabled only in prepare resume for VLV (vlv_allow_gt_wake()). Maybe FPGA_DBG could be reset separately before the rest?
Quoted 28 lines
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Maybe we need to move even more code up?
drivers/gpu/drm/i915/i915_drv.c +1
-1@@ -665,11 +665,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;int ret; +intel_uncore_early_sanitize (dev, true); ret =intel_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);
2014-10-10 8:25 GMT-03:00 Imre Deak <[email protected]>:
Quoted 18 lines · Paulo Zanoni
Quoted 8 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.Quoted 8 lines
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".
Quoted 3 lines
One issue is that
intel_uncore_early_sanitize()uses forcewake, which is enabled only in prepare resume for VLV (vlv_allow_gt_wake()). MaybeFPGA_DBGcould be reset separately before the rest?
It doesn't "use" forcewake, it calls intel_uncore_forcewake_reset(). Shouldn't we move the vlv code (e.g., vlv_allow_gt_wake()) to inside intel_uncore_forcewake_reset() instead?
To my understanding, it doesn't make sense to call anything at all before intel_uncore_early_sanitize()...
Quoted 31 lines
Quoted 29 lines
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Maybe we need to move even more code up?
drivers/gpu/drm/i915/i915_drv.c +1
-1@@ -665,11 +665,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;int ret; +intel_uncore_early_sanitize (dev, true); ret =intel_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);
Signature · 1 line
Paulo Zanoni
Quoted 19 lines · Paulo Zanoni
2014-10-10 8:25 GMT-03:00 Imre Deak <[email protected]>:
Quoted 18 lines · Paulo Zanoni
Quoted 16 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".
Quoted 5 lines
Quoted 3 lines
One issue is that
intel_uncore_early_sanitize()uses forcewake, which is enabled only in prepare resume for VLV (vlv_allow_gt_wake()). MaybeFPGA_DBGcould be reset separately before the rest?It doesn't "use" forcewake, it calls
intel_uncore_forcewake_reset().
Hm, right I only remembered that because of
"drm/i915: check for GT faults during S3 resume and S4 restore too"
on the mailing list, but it's not yet merged.
Quoted 5 lines
Shouldn't we move the vlv code (e.g.,
vlv_allow_gt_wake()) to insideintel_uncore_forcewake_reset()instead?To my understanding, it doesn't make sense to call anything at all before
intel_uncore_early_sanitize()...
On VLV we need to do the steps in the resume_prepare handler first. Also we don't want to call vlv_allow_gt_wake() during driver init. But yes, I see that for other platforms it makes sense to move intel_uncore_early_sanitize() earlier. One way would be to call it from resume_prepare for each platform at the proper place in case of system resume.
Quoted 34 lines
Quoted 31 lines
Quoted 29 lines
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Maybe we need to move even more code up?
drivers/gpu/drm/i915/i915_drv.c +1
-1@@ -665,11 +665,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;int ret; +intel_uncore_early_sanitize (dev, true); ret =intel_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);