All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
@ 2019-05-20  4:56 Rodrigo Vivi
  2019-05-20  8:01 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Rodrigo Vivi @ 2019-05-20  4:56 UTC (permalink / raw)
  To: intel-gfx

Suspend resume is broken if we try to enable/disable dc9 on
cases with disabled displays.

Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 103 ++++++++++++++++++++++----------
 1 file changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2c7a4318d13c..90693327065a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2117,6 +2117,15 @@ get_suspend_mode(struct drm_i915_private *dev_priv, bool hibernate)
 	return I915_DRM_SUSPEND_MEM;
 }
 
+static void intel_display_suspend_late(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
+		bxt_enable_dc9(dev_priv);
+}
+
 static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2132,10 +2141,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	intel_power_domains_suspend(dev_priv,
 				    get_suspend_mode(dev_priv, hibernation));
 
+	intel_display_suspend_late(dev_priv);
+
 	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
-		bxt_enable_dc9(dev_priv);
-	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		hsw_enable_pc8(dev_priv);
 	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_suspend_complete(dev_priv);
@@ -2265,6 +2274,17 @@ static int i915_drm_resume(struct drm_device *dev)
 	return 0;
 }
 
+static void intel_display_resume_early(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
+		gen9_sanitize_dc_state(dev_priv);
+		bxt_disable_dc9(dev_priv);
+	}
+}
+
 static int i915_drm_resume_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2327,10 +2347,9 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	i915_check_and_clear_faults(dev_priv);
 
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
-		gen9_sanitize_dc_state(dev_priv);
-		bxt_disable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+	intel_display_resume_early(dev_priv);
+
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		hsw_disable_pc8(dev_priv);
 	}
 
@@ -2868,6 +2887,20 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
+static void intel_runtime_display_suspend(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11) {
+		icl_display_core_uninit(dev_priv);
+		bxt_enable_dc9(dev_priv);
+	} else if (IS_GEN9_LP(dev_priv)) {
+		bxt_display_core_uninit(dev_priv);
+		bxt_enable_dc9(dev_priv);
+	}
+}
+
 static int intel_runtime_suspend(struct device *kdev)
 {
 	struct pci_dev *pdev = to_pci_dev(kdev);
@@ -2897,14 +2930,10 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	intel_uncore_suspend(&dev_priv->uncore);
 
+	intel_runtime_display_suspend(dev_priv);
+
 	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11) {
-		icl_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		hsw_enable_pc8(dev_priv);
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		ret = vlv_suspend_complete(dev_priv);
@@ -2966,6 +2995,31 @@ static int intel_runtime_suspend(struct device *kdev)
 	return 0;
 }
 
+static void intel_runtime_display_resume(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11) {
+		bxt_disable_dc9(dev_priv);
+		icl_display_core_init(dev_priv, true);
+		if (dev_priv->csr.dmc_payload) {
+			if (dev_priv->csr.allowed_dc_mask &
+			    DC_STATE_EN_UPTO_DC6)
+				skl_enable_dc6(dev_priv);
+			else if (dev_priv->csr.allowed_dc_mask &
+				 DC_STATE_EN_UPTO_DC5)
+				gen9_enable_dc5(dev_priv);
+		}
+	} else if (IS_GEN9_LP(dev_priv)) {
+		bxt_disable_dc9(dev_priv);
+		bxt_display_core_init(dev_priv, true);
+		if (dev_priv->csr.dmc_payload &&
+		    (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
+			gen9_enable_dc5(dev_priv);
+	}
+}
+
 static int intel_runtime_resume(struct device *kdev)
 {
 	struct pci_dev *pdev = to_pci_dev(kdev);
@@ -2986,24 +3040,9 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
-	if (INTEL_GEN(dev_priv) >= 11) {
-		bxt_disable_dc9(dev_priv);
-		icl_display_core_init(dev_priv, true);
-		if (dev_priv->csr.dmc_payload) {
-			if (dev_priv->csr.allowed_dc_mask &
-			    DC_STATE_EN_UPTO_DC6)
-				skl_enable_dc6(dev_priv);
-			else if (dev_priv->csr.allowed_dc_mask &
-				 DC_STATE_EN_UPTO_DC5)
-				gen9_enable_dc5(dev_priv);
-		}
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_disable_dc9(dev_priv);
-		bxt_display_core_init(dev_priv, true);
-		if (dev_priv->csr.dmc_payload &&
-		    (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
-			gen9_enable_dc5(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+	intel_runtime_display_resume(dev_priv);
+
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		hsw_disable_pc8(dev_priv);
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		ret = vlv_resume_prepare(dev_priv, true);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-05-20  4:56 [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY Rodrigo Vivi
@ 2019-05-20  8:01 ` Patchwork
  2019-05-20  8:21 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2019-05-20  8:01 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
URL   : https://patchwork.freedesktop.org/series/60839/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
810402289a3a drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
-:78: WARNING:BRACES: braces {} are not necessary for single statement blocks
#78: FILE: drivers/gpu/drm/i915/i915_drv.c:2352:
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		hsw_disable_pc8(dev_priv);
 	}

total: 0 errors, 1 warnings, 0 checks, 153 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-05-20  4:56 [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY Rodrigo Vivi
  2019-05-20  8:01 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2019-05-20  8:21 ` Patchwork
  2019-05-20  9:47 ` ✓ Fi.CI.IGT: " Patchwork
  2019-05-20 21:35 ` [PATCH] " Souza, Jose
  3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2019-05-20  8:21 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
URL   : https://patchwork.freedesktop.org/series/60839/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6097 -> Patchwork_13040
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/

Known issues
------------

  Here are the changes found in Patchwork_13040 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-kbl-7500u:       [PASS][1] -> [DMESG-WARN][2] ([fdo#105128] / [fdo#107139])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-kbl-7500u/igt@gem_exec_suspend@basic-s4-devices.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-kbl-7500u/igt@gem_exec_suspend@basic-s4-devices.html
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       [PASS][5] -> [DMESG-WARN][6] ([fdo#107709])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-bsw-kefka/igt@i915_selftest@live_evict.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-bsw-kefka/igt@i915_selftest@live_evict.html

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         [PASS][7] -> [INCOMPLETE][8] ([fdo#103927] / [fdo#109720])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-apl-guc/igt@i915_selftest@live_execlists.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-apl-guc/igt@i915_selftest@live_execlists.html

  
#### Possible fixes ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       [DMESG-WARN][9] ([fdo#108965]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       [INCOMPLETE][11] ([fdo#108602] / [fdo#108744]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
    - fi-apl-guc:         [FAIL][13] ([fdo#110623]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-apl-guc/igt@i915_selftest@live_hangcheck.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-apl-guc/igt@i915_selftest@live_hangcheck.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-apl-guc:         [FAIL][15] ([fdo#110622]) -> [FAIL][16] ([fdo#108622] / [fdo#109720])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-apl-guc/igt@runner@aborted.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/fi-apl-guc/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105128]: https://bugs.freedesktop.org/show_bug.cgi?id=105128
  [fdo#107139]: https://bugs.freedesktop.org/show_bug.cgi?id=107139
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#110622]: https://bugs.freedesktop.org/show_bug.cgi?id=110622
  [fdo#110623]: https://bugs.freedesktop.org/show_bug.cgi?id=110623


Participating hosts (52 -> 43)
------------------------------

  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-icl-u3 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6097 -> Patchwork_13040

  CI_DRM_6097: 3f2d6a47d9eec66594887b1e9718bc1a29aa6a77 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4996: 6fe5d254ec1b9b47d61408e1b49a7339876bf1e7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13040: 810402289a3aba3bb84f19c666a1413bc5140ebf @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

810402289a3a drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-05-20  4:56 [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY Rodrigo Vivi
  2019-05-20  8:01 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
  2019-05-20  8:21 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-05-20  9:47 ` Patchwork
  2019-05-20 21:35 ` [PATCH] " Souza, Jose
  3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2019-05-20  9:47 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
URL   : https://patchwork.freedesktop.org/series/60839/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6097_full -> Patchwork_13040_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_13040_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@debugfs-forcewake-user:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2] ([fdo#107807])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl2/igt@i915_pm_rpm@debugfs-forcewake-user.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-skl5/igt@i915_pm_rpm@debugfs-forcewake-user.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-skl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#109507])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl8/igt@kms_flip@flip-vs-suspend-interruptible.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-skl10/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
    - shard-iclb:         [PASS][5] -> [FAIL][6] ([fdo#103167]) +5 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html

  * igt@kms_lease@setcrtc_implicit_plane:
    - shard-snb:          [PASS][7] -> [SKIP][8] ([fdo#109271])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-snb5/igt@kms_lease@setcrtc_implicit_plane.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-snb4/igt@kms_lease@setcrtc_implicit_plane.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([fdo#108566]) +3 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-apl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-apl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [PASS][11] -> [FAIL][12] ([fdo#103166])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb6/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb6/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [PASS][13] -> [SKIP][14] ([fdo#109441]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb2/igt@kms_psr@psr2_dpms.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb4/igt@kms_psr@psr2_dpms.html

  * igt@perf_pmu@rc6:
    - shard-kbl:          [PASS][15] -> [SKIP][16] ([fdo#109271])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-kbl1/igt@perf_pmu@rc6.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-kbl7/igt@perf_pmu@rc6.html

  
#### Possible fixes ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [DMESG-WARN][17] ([fdo#108686]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-glk7/igt@gem_tiled_swapping@non-threaded.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-glk6/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_pm_rpm@debugfs-read:
    - shard-skl:          [INCOMPLETE][19] ([fdo#107807]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl3/igt@i915_pm_rpm@debugfs-read.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-skl4/igt@i915_pm_rpm@debugfs-read.html

  * igt@i915_pm_rpm@gem-execbuf:
    - shard-skl:          [INCOMPLETE][21] ([fdo#107803] / [fdo#107807]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl4/igt@i915_pm_rpm@gem-execbuf.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-skl5/igt@i915_pm_rpm@gem-execbuf.html

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
    - shard-hsw:          [FAIL][23] ([fdo#105767]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-hsw1/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-hsw4/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          [FAIL][25] ([fdo#102887]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-glk8/igt@kms_flip@2x-flip-vs-expired-vblank.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-apl:          [DMESG-WARN][27] ([fdo#108566]) -> [PASS][28] +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-apl7/igt@kms_flip@flip-vs-suspend.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-apl1/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [FAIL][29] ([fdo#103167]) -> [PASS][30] +2 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][31] ([fdo#108145]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl5/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [FAIL][33] ([fdo#103166]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb3/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [SKIP][35] ([fdo#109441]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb1/igt@kms_psr@psr2_sprite_plane_move.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][37] ([fdo#99912]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-kbl7/igt@kms_setmode@basic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-kbl2/igt@kms_setmode@basic.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [FAIL][39] ([fdo#100047]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb2/igt@kms_sysfs_edid_timing.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb4/igt@kms_sysfs_edid_timing.html

  * igt@syncobj_wait@invalid-signal-illegal-handle:
    - shard-iclb:         [INCOMPLETE][41] ([fdo#107713]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb7/igt@syncobj_wait@invalid-signal-illegal-handle.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb2/igt@syncobj_wait@invalid-signal-illegal-handle.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-xy:
    - shard-iclb:         [INCOMPLETE][43] ([fdo#107713] / [fdo#109100]) -> [TIMEOUT][44] ([fdo#109673])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb3/igt@gem_mmap_gtt@forked-big-copy-xy.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-iclb5/igt@gem_mmap_gtt@forked-big-copy-xy.html

  * igt@i915_pm_rpm@pc8-residency:
    - shard-skl:          [INCOMPLETE][45] ([fdo#107807]) -> [SKIP][46] ([fdo#109271])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl4/igt@i915_pm_rpm@pc8-residency.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/shard-skl3/igt@i915_pm_rpm@pc8-residency.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6097 -> Patchwork_13040

  CI_DRM_6097: 3f2d6a47d9eec66594887b1e9718bc1a29aa6a77 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4996: 6fe5d254ec1b9b47d61408e1b49a7339876bf1e7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13040: 810402289a3aba3bb84f19c666a1413bc5140ebf @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13040/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-05-20  4:56 [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY Rodrigo Vivi
                   ` (2 preceding siblings ...)
  2019-05-20  9:47 ` ✓ Fi.CI.IGT: " Patchwork
@ 2019-05-20 21:35 ` Souza, Jose
  3 siblings, 0 replies; 16+ messages in thread
From: Souza, Jose @ 2019-05-20 21:35 UTC (permalink / raw)
  To: Vivi, Rodrigo, intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 6382 bytes --]

On Sun, 2019-05-19 at 21:56 -0700, Rodrigo Vivi wrote:
> Suspend resume is broken if we try to enable/disable dc9 on
> cases with disabled displays.
> 

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 103 ++++++++++++++++++++++------
> ----
>  1 file changed, 71 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 2c7a4318d13c..90693327065a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2117,6 +2117,15 @@ get_suspend_mode(struct drm_i915_private
> *dev_priv, bool hibernate)
>  	return I915_DRM_SUSPEND_MEM;
>  }
>  
> +static void intel_display_suspend_late(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> +		bxt_enable_dc9(dev_priv);
> +}
> +
>  static int i915_drm_suspend_late(struct drm_device *dev, bool
> hibernation)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2132,10 +2141,10 @@ static int i915_drm_suspend_late(struct
> drm_device *dev, bool hibernation)
>  	intel_power_domains_suspend(dev_priv,
>  				    get_suspend_mode(dev_priv,
> hibernation));
>  
> +	intel_display_suspend_late(dev_priv);
> +
>  	ret = 0;
> -	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> -		bxt_enable_dc9(dev_priv);
> -	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		hsw_enable_pc8(dev_priv);
>  	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		ret = vlv_suspend_complete(dev_priv);
> @@ -2265,6 +2274,17 @@ static int i915_drm_resume(struct drm_device
> *dev)
>  	return 0;
>  }
>  
> +static void intel_display_resume_early(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> +		gen9_sanitize_dc_state(dev_priv);
> +		bxt_disable_dc9(dev_priv);
> +	}
> +}
> +
>  static int i915_drm_resume_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2327,10 +2347,9 @@ static int i915_drm_resume_early(struct
> drm_device *dev)
>  
>  	i915_check_and_clear_faults(dev_priv);
>  
> -	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> -		gen9_sanitize_dc_state(dev_priv);
> -		bxt_disable_dc9(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +	intel_display_resume_early(dev_priv);
> +
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		hsw_disable_pc8(dev_priv);
>  	}
>  
> @@ -2868,6 +2887,20 @@ static int vlv_resume_prepare(struct
> drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> +static void intel_runtime_display_suspend(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		icl_display_core_uninit(dev_priv);
> +		bxt_enable_dc9(dev_priv);
> +	} else if (IS_GEN9_LP(dev_priv)) {
> +		bxt_display_core_uninit(dev_priv);
> +		bxt_enable_dc9(dev_priv);
> +	}
> +}
> +
>  static int intel_runtime_suspend(struct device *kdev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(kdev);
> @@ -2897,14 +2930,10 @@ static int intel_runtime_suspend(struct
> device *kdev)
>  
>  	intel_uncore_suspend(&dev_priv->uncore);
>  
> +	intel_runtime_display_suspend(dev_priv);
> +
>  	ret = 0;
> -	if (INTEL_GEN(dev_priv) >= 11) {
> -		icl_display_core_uninit(dev_priv);
> -		bxt_enable_dc9(dev_priv);
> -	} else if (IS_GEN9_LP(dev_priv)) {
> -		bxt_display_core_uninit(dev_priv);
> -		bxt_enable_dc9(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		hsw_enable_pc8(dev_priv);
>  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> {
>  		ret = vlv_suspend_complete(dev_priv);
> @@ -2966,6 +2995,31 @@ static int intel_runtime_suspend(struct device
> *kdev)
>  	return 0;
>  }
>  
> +static void intel_runtime_display_resume(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		bxt_disable_dc9(dev_priv);
> +		icl_display_core_init(dev_priv, true);
> +		if (dev_priv->csr.dmc_payload) {
> +			if (dev_priv->csr.allowed_dc_mask &
> +			    DC_STATE_EN_UPTO_DC6)
> +				skl_enable_dc6(dev_priv);
> +			else if (dev_priv->csr.allowed_dc_mask &
> +				 DC_STATE_EN_UPTO_DC5)
> +				gen9_enable_dc5(dev_priv);
> +		}
> +	} else if (IS_GEN9_LP(dev_priv)) {
> +		bxt_disable_dc9(dev_priv);
> +		bxt_display_core_init(dev_priv, true);
> +		if (dev_priv->csr.dmc_payload &&
> +		    (dev_priv->csr.allowed_dc_mask &
> DC_STATE_EN_UPTO_DC5))
> +			gen9_enable_dc5(dev_priv);
> +	}
> +}
> +
>  static int intel_runtime_resume(struct device *kdev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(kdev);
> @@ -2986,24 +3040,9 @@ static int intel_runtime_resume(struct device
> *kdev)
>  	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
>  		DRM_DEBUG_DRIVER("Unclaimed access during suspend,
> bios?\n");
>  
> -	if (INTEL_GEN(dev_priv) >= 11) {
> -		bxt_disable_dc9(dev_priv);
> -		icl_display_core_init(dev_priv, true);
> -		if (dev_priv->csr.dmc_payload) {
> -			if (dev_priv->csr.allowed_dc_mask &
> -			    DC_STATE_EN_UPTO_DC6)
> -				skl_enable_dc6(dev_priv);
> -			else if (dev_priv->csr.allowed_dc_mask &
> -				 DC_STATE_EN_UPTO_DC5)
> -				gen9_enable_dc5(dev_priv);
> -		}
> -	} else if (IS_GEN9_LP(dev_priv)) {
> -		bxt_disable_dc9(dev_priv);
> -		bxt_display_core_init(dev_priv, true);
> -		if (dev_priv->csr.dmc_payload &&
> -		    (dev_priv->csr.allowed_dc_mask &
> DC_STATE_EN_UPTO_DC5))
> -			gen9_enable_dc5(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +	intel_runtime_display_resume(dev_priv);
> +
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		hsw_disable_pc8(dev_priv);
>  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> {
>  		ret = vlv_resume_prepare(dev_priv, true);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-08-06 12:02         ` Jani Nikula
@ 2019-08-06 17:01           ` Rodrigo Vivi
  0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Vivi @ 2019-08-06 17:01 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Tue, Aug 06, 2019 at 03:02:31PM +0300, Jani Nikula wrote:
> On Thu, 18 Jul 2019, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Thu, Jul 18, 2019 at 10:25:51PM +0100, Chris Wilson wrote:
> >> Quoting Rodrigo Vivi (2019-07-18 22:14:45)
> >> > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote:
> >> > > Quoting Rodrigo Vivi (2019-07-18 21:49:12)
> >> > > > +void intel_display_power_resume_early(struct drm_i915_private *i915)
> >> > > > +{
> >> > > > +       if (!HAS_DISPLAY(i915))
> >> > > > +               return;
> >> > > > +
> >> > > > +       if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
> >> > > > +               gen9_sanitize_dc_state(i915);
> >> > > 
> >> > > Are you sure that whatever state you are resuming from agrees with your
> >> > > notion of !display? The sanitize routines are supposed to be about
> >> > > cleaning up after third parties who don't play by the same rules.
> >> > 
> >> > I don't expect any function setting any kind of dc states when we don't
> >> > have display. Besides the path that sets DC_STATE_EN is and neeeds to
> >> > be sanitized is also covered by this patch and this shouldn't happen.
> >> > 
> >> > Or am I missing something else?
> >> 
> >> It's not about us, it's about whatever else runs in between. And
> >> remember !HAS_DISPLAY() is also a user setting, not merely a reflection
> >> of probed hw.
> >
> > ouch, we need to get rid of those runtime writes to info struct :/
> >
> > I wonder if it worth to add a intel_display_sanitize to be called
> > when toggling info-num_pipes to 0 along with that DRM_ERROR...
> >
> > or just call it before !HAS_DISPLAY with a XXX comment...
> >
> > other ideas?
> 
> Let's say we only supported user specified display disable via:
> 
> # modprobe i915
> # modprobe -r i915
> # modprobe i915 disable_display=1
> 
> i.e. by having i915 take over and disable everything first. Would that
> be enough?
> 
> Alternatively, could we do display disable by first probing almost
> everything as we would with disable_display=0, then throwing
> -EPROBE_DEFER and having the error handling code clean up the hardware
> after us. The subsequent probe retry would then proceed assuming no
> display hardware.
> 
> Thoughts?

It makes sense for me. Would we need to detach disable_display from
num_pipes for that?

Anyways, taking another quick look to the only case we modify num_pipes
out of platform definition I don't see now how it would suddenly
become 0:

 if (enabled_mask == 0
 ...
                else
                        info->num_pipes = hweight8(enabled_mask);

so maybe just go with my previous version is safe after all?!

But indeed making it more solid is a good plan.

> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-07-18 21:38       ` Rodrigo Vivi
@ 2019-08-06 12:02         ` Jani Nikula
  2019-08-06 17:01           ` Rodrigo Vivi
  0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2019-08-06 12:02 UTC (permalink / raw)
  To: Rodrigo Vivi, Chris Wilson; +Cc: intel-gfx

On Thu, 18 Jul 2019, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Thu, Jul 18, 2019 at 10:25:51PM +0100, Chris Wilson wrote:
>> Quoting Rodrigo Vivi (2019-07-18 22:14:45)
>> > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote:
>> > > Quoting Rodrigo Vivi (2019-07-18 21:49:12)
>> > > > +void intel_display_power_resume_early(struct drm_i915_private *i915)
>> > > > +{
>> > > > +       if (!HAS_DISPLAY(i915))
>> > > > +               return;
>> > > > +
>> > > > +       if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
>> > > > +               gen9_sanitize_dc_state(i915);
>> > > 
>> > > Are you sure that whatever state you are resuming from agrees with your
>> > > notion of !display? The sanitize routines are supposed to be about
>> > > cleaning up after third parties who don't play by the same rules.
>> > 
>> > I don't expect any function setting any kind of dc states when we don't
>> > have display. Besides the path that sets DC_STATE_EN is and neeeds to
>> > be sanitized is also covered by this patch and this shouldn't happen.
>> > 
>> > Or am I missing something else?
>> 
>> It's not about us, it's about whatever else runs in between. And
>> remember !HAS_DISPLAY() is also a user setting, not merely a reflection
>> of probed hw.
>
> ouch, we need to get rid of those runtime writes to info struct :/
>
> I wonder if it worth to add a intel_display_sanitize to be called
> when toggling info-num_pipes to 0 along with that DRM_ERROR...
>
> or just call it before !HAS_DISPLAY with a XXX comment...
>
> other ideas?

Let's say we only supported user specified display disable via:

# modprobe i915
# modprobe -r i915
# modprobe i915 disable_display=1

i.e. by having i915 take over and disable everything first. Would that
be enough?

Alternatively, could we do display disable by first probing almost
everything as we would with disable_display=0, then throwing
-EPROBE_DEFER and having the error handling code clean up the hardware
after us. The subsequent probe retry would then proceed assuming no
display hardware.

Thoughts?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-07-18 21:25     ` Chris Wilson
@ 2019-07-18 21:38       ` Rodrigo Vivi
  2019-08-06 12:02         ` Jani Nikula
  0 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2019-07-18 21:38 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Jani Nikula, intel-gfx

On Thu, Jul 18, 2019 at 10:25:51PM +0100, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2019-07-18 22:14:45)
> > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote:
> > > Quoting Rodrigo Vivi (2019-07-18 21:49:12)
> > > > +void intel_display_power_resume_early(struct drm_i915_private *i915)
> > > > +{
> > > > +       if (!HAS_DISPLAY(i915))
> > > > +               return;
> > > > +
> > > > +       if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
> > > > +               gen9_sanitize_dc_state(i915);
> > > 
> > > Are you sure that whatever state you are resuming from agrees with your
> > > notion of !display? The sanitize routines are supposed to be about
> > > cleaning up after third parties who don't play by the same rules.
> > 
> > I don't expect any function setting any kind of dc states when we don't
> > have display. Besides the path that sets DC_STATE_EN is and neeeds to
> > be sanitized is also covered by this patch and this shouldn't happen.
> > 
> > Or am I missing something else?
> 
> It's not about us, it's about whatever else runs in between. And
> remember !HAS_DISPLAY() is also a user setting, not merely a reflection
> of probed hw.

ouch, we need to get rid of those runtime writes to info struct :/

I wonder if it worth to add a intel_display_sanitize to be called
when toggling info-num_pipes to 0 along with that DRM_ERROR...

or just call it before !HAS_DISPLAY with a XXX comment...

other ideas?

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-07-18 21:14   ` Rodrigo Vivi
@ 2019-07-18 21:25     ` Chris Wilson
  2019-07-18 21:38       ` Rodrigo Vivi
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2019-07-18 21:25 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: Jani Nikula, intel-gfx

Quoting Rodrigo Vivi (2019-07-18 22:14:45)
> On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote:
> > Quoting Rodrigo Vivi (2019-07-18 21:49:12)
> > > +void intel_display_power_resume_early(struct drm_i915_private *i915)
> > > +{
> > > +       if (!HAS_DISPLAY(i915))
> > > +               return;
> > > +
> > > +       if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
> > > +               gen9_sanitize_dc_state(i915);
> > 
> > Are you sure that whatever state you are resuming from agrees with your
> > notion of !display? The sanitize routines are supposed to be about
> > cleaning up after third parties who don't play by the same rules.
> 
> I don't expect any function setting any kind of dc states when we don't
> have display. Besides the path that sets DC_STATE_EN is and neeeds to
> be sanitized is also covered by this patch and this shouldn't happen.
> 
> Or am I missing something else?

It's not about us, it's about whatever else runs in between. And
remember !HAS_DISPLAY() is also a user setting, not merely a reflection
of probed hw.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-07-18 20:58 ` Chris Wilson
@ 2019-07-18 21:14   ` Rodrigo Vivi
  2019-07-18 21:25     ` Chris Wilson
  0 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2019-07-18 21:14 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Jani Nikula, intel-gfx

On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2019-07-18 21:49:12)
> > +void intel_display_power_resume_early(struct drm_i915_private *i915)
> > +{
> > +       if (!HAS_DISPLAY(i915))
> > +               return;
> > +
> > +       if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
> > +               gen9_sanitize_dc_state(i915);
> 
> Are you sure that whatever state you are resuming from agrees with your
> notion of !display? The sanitize routines are supposed to be about
> cleaning up after third parties who don't play by the same rules.

I don't expect any function setting any kind of dc states when we don't
have display. Besides the path that sets DC_STATE_EN is and neeeds to
be sanitized is also covered by this patch and this shouldn't happen.

Or am I missing something else?

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-07-18 20:49 Rodrigo Vivi
@ 2019-07-18 20:58 ` Chris Wilson
  2019-07-18 21:14   ` Rodrigo Vivi
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2019-07-18 20:58 UTC (permalink / raw)
  To: Rodrigo Vivi, intel-gfx; +Cc: Jani Nikula

Quoting Rodrigo Vivi (2019-07-18 21:49:12)
> +void intel_display_power_resume_early(struct drm_i915_private *i915)
> +{
> +       if (!HAS_DISPLAY(i915))
> +               return;
> +
> +       if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
> +               gen9_sanitize_dc_state(i915);

Are you sure that whatever state you are resuming from agrees with your
notion of !display? The sanitize routines are supposed to be about
cleaning up after third parties who don't play by the same rules.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
@ 2019-07-18 20:49 Rodrigo Vivi
  2019-07-18 20:58 ` Chris Wilson
  0 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2019-07-18 20:49 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula

Suspend resume is broken if we try to enable/disable dc9 on
cases with disabled displays.

v2: Make checkpatch happy:
    - braces {} are not necessary for single statement blocks
v3: Also move hsw/bdw PC8 sequences since they are related to
    display PM anyways. (Ville)
v4: Rebase after a long time, plus Move functions to the new
    intel_display_power so we can stop exporting platform specific
    functions as pointed by Jani.
v5: Remove unnecessary braces.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 67 +++++++++++++++++++
 .../drm/i915/display/intel_display_power.h    | 17 ++---
 drivers/gpu/drm/i915/i915_drv.c               | 58 ++++------------
 3 files changed, 84 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 93a148684c53..530b119a3a3b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -5189,3 +5189,70 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
 }
 
 #endif
+
+void intel_display_power_suspend_late(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915))
+		bxt_enable_dc9(i915);
+	else if (IS_HASWELL(i915) || IS_BROADWELL(i915))
+		hsw_enable_pc8(i915);
+}
+
+void intel_display_power_resume_early(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
+		gen9_sanitize_dc_state(i915);
+		bxt_disable_dc9(i915);
+	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
+		hsw_disable_pc8(i915);
+	}
+}
+
+void intel_display_power_suspend(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11) {
+		icl_display_core_uninit(i915);
+		bxt_enable_dc9(i915);
+	} else if (IS_GEN9_LP(i915)) {
+		bxt_display_core_uninit(i915);
+		bxt_enable_dc9(i915);
+	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
+		hsw_enable_pc8(i915);
+	}
+}
+
+void intel_display_power_resume(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11) {
+		bxt_disable_dc9(i915);
+		icl_display_core_init(i915, true);
+		if (i915->csr.dmc_payload) {
+			if (i915->csr.allowed_dc_mask &
+			    DC_STATE_EN_UPTO_DC6)
+				skl_enable_dc6(i915);
+			else if (i915->csr.allowed_dc_mask &
+				 DC_STATE_EN_UPTO_DC5)
+				gen9_enable_dc5(i915);
+		}
+	} else if (IS_GEN9_LP(i915)) {
+		bxt_disable_dc9(i915);
+		bxt_display_core_init(i915, true);
+		if (i915->csr.dmc_payload &&
+		    (i915->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
+			gen9_enable_dc5(i915);
+	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
+		hsw_disable_pc8(i915);
+	}
+}
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index e4d2c1ba24b0..97f2562fc5d3 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -232,27 +232,20 @@ struct i915_power_domains {
 	for_each_power_well_reverse(__dev_priv, __power_well)		        \
 		for_each_if((__power_well)->desc->domains & (__domain_mask))
 
-void skl_enable_dc6(struct drm_i915_private *dev_priv);
-void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv);
-void bxt_enable_dc9(struct drm_i915_private *dev_priv);
-void bxt_disable_dc9(struct drm_i915_private *dev_priv);
-void gen9_enable_dc5(struct drm_i915_private *dev_priv);
-
 int intel_power_domains_init(struct drm_i915_private *dev_priv);
 void intel_power_domains_cleanup(struct drm_i915_private *dev_priv);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume);
 void intel_power_domains_driver_remove(struct drm_i915_private *dev_priv);
-void icl_display_core_init(struct drm_i915_private *dev_priv, bool resume);
-void icl_display_core_uninit(struct drm_i915_private *dev_priv);
 void intel_power_domains_enable(struct drm_i915_private *dev_priv);
 void intel_power_domains_disable(struct drm_i915_private *dev_priv);
 void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 				 enum i915_drm_suspend_mode);
 void intel_power_domains_resume(struct drm_i915_private *dev_priv);
-void hsw_enable_pc8(struct drm_i915_private *dev_priv);
-void hsw_disable_pc8(struct drm_i915_private *dev_priv);
-void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume);
-void bxt_display_core_uninit(struct drm_i915_private *dev_priv);
+
+void intel_display_power_suspend_late(struct drm_i915_private *i915);
+void intel_display_power_resume_early(struct drm_i915_private *i915);
+void intel_display_power_suspend(struct drm_i915_private *i915);
+void intel_display_power_resume(struct drm_i915_private *i915);
 
 const char *
 intel_display_power_domain_str(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7c209743e478..35b1883b55a4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2166,7 +2166,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
-	int ret;
+	int ret = 0;
 
 	disable_rpm_wakeref_asserts(rpm);
 
@@ -2177,12 +2177,9 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	intel_power_domains_suspend(dev_priv,
 				    get_suspend_mode(dev_priv, hibernation));
 
-	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
-		bxt_enable_dc9(dev_priv);
-	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-		hsw_enable_pc8(dev_priv);
-	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+	intel_display_power_suspend_late(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_suspend_complete(dev_priv);
 
 	if (ret) {
@@ -2372,12 +2369,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	intel_gt_check_and_clear_faults(&dev_priv->gt);
 
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
-		gen9_sanitize_dc_state(dev_priv);
-		bxt_disable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_disable_pc8(dev_priv);
-	}
+	intel_display_power_resume_early(dev_priv);
 
 	intel_sanitize_gt_powersave(dev_priv);
 
@@ -2921,7 +2913,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
-	int ret;
+	int ret = 0;
 
 	if (WARN_ON_ONCE(!(dev_priv->gt_pm.rc6.enabled && HAS_RC6(dev_priv))))
 		return -ENODEV;
@@ -2945,18 +2937,10 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	intel_uncore_suspend(&dev_priv->uncore);
 
-	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11) {
-		icl_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_enable_pc8(dev_priv);
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+	intel_display_power_suspend(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_suspend_complete(dev_priv);
-	}
 
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
@@ -3035,28 +3019,10 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
-	if (INTEL_GEN(dev_priv) >= 11) {
-		bxt_disable_dc9(dev_priv);
-		icl_display_core_init(dev_priv, true);
-		if (dev_priv->csr.dmc_payload) {
-			if (dev_priv->csr.allowed_dc_mask &
-			    DC_STATE_EN_UPTO_DC6)
-				skl_enable_dc6(dev_priv);
-			else if (dev_priv->csr.allowed_dc_mask &
-				 DC_STATE_EN_UPTO_DC5)
-				gen9_enable_dc5(dev_priv);
-		}
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_disable_dc9(dev_priv);
-		bxt_display_core_init(dev_priv, true);
-		if (dev_priv->csr.dmc_payload &&
-		    (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
-			gen9_enable_dc5(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_disable_pc8(dev_priv);
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+	intel_display_power_resume(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_resume_prepare(dev_priv, true);
-	}
 
 	intel_uncore_runtime_resume(&dev_priv->uncore);
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
@ 2019-07-18 20:47 Rodrigo Vivi
  0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Vivi @ 2019-07-18 20:47 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula

Suspend resume is broken if we try to enable/disable dc9 on
cases with disabled displays.

v2: Make checkpatch happy:
    - braces {} are not necessary for single statement blocks
v3: Also move hsw/bdw PC8 sequences since they are related to
    display PM anyways. (Ville)
v4: Rebase after a long time, plus Move functions to the new
    intel_display_power so we can stop exporting platform specific
    functions as pointed by Jani.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 67 +++++++++++++++++++
 .../drm/i915/display/intel_display_power.h    | 17 ++---
 drivers/gpu/drm/i915/i915_drv.c               | 56 ++++------------
 3 files changed, 84 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 93a148684c53..530b119a3a3b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -5189,3 +5189,70 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
 }
 
 #endif
+
+void intel_display_power_suspend_late(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915))
+		bxt_enable_dc9(i915);
+	else if (IS_HASWELL(i915) || IS_BROADWELL(i915))
+		hsw_enable_pc8(i915);
+}
+
+void intel_display_power_resume_early(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
+		gen9_sanitize_dc_state(i915);
+		bxt_disable_dc9(i915);
+	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
+		hsw_disable_pc8(i915);
+	}
+}
+
+void intel_display_power_suspend(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11) {
+		icl_display_core_uninit(i915);
+		bxt_enable_dc9(i915);
+	} else if (IS_GEN9_LP(i915)) {
+		bxt_display_core_uninit(i915);
+		bxt_enable_dc9(i915);
+	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
+		hsw_enable_pc8(i915);
+	}
+}
+
+void intel_display_power_resume(struct drm_i915_private *i915)
+{
+	if (!HAS_DISPLAY(i915))
+		return;
+
+	if (INTEL_GEN(i915) >= 11) {
+		bxt_disable_dc9(i915);
+		icl_display_core_init(i915, true);
+		if (i915->csr.dmc_payload) {
+			if (i915->csr.allowed_dc_mask &
+			    DC_STATE_EN_UPTO_DC6)
+				skl_enable_dc6(i915);
+			else if (i915->csr.allowed_dc_mask &
+				 DC_STATE_EN_UPTO_DC5)
+				gen9_enable_dc5(i915);
+		}
+	} else if (IS_GEN9_LP(i915)) {
+		bxt_disable_dc9(i915);
+		bxt_display_core_init(i915, true);
+		if (i915->csr.dmc_payload &&
+		    (i915->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
+			gen9_enable_dc5(i915);
+	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
+		hsw_disable_pc8(i915);
+	}
+}
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index e4d2c1ba24b0..97f2562fc5d3 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -232,27 +232,20 @@ struct i915_power_domains {
 	for_each_power_well_reverse(__dev_priv, __power_well)		        \
 		for_each_if((__power_well)->desc->domains & (__domain_mask))
 
-void skl_enable_dc6(struct drm_i915_private *dev_priv);
-void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv);
-void bxt_enable_dc9(struct drm_i915_private *dev_priv);
-void bxt_disable_dc9(struct drm_i915_private *dev_priv);
-void gen9_enable_dc5(struct drm_i915_private *dev_priv);
-
 int intel_power_domains_init(struct drm_i915_private *dev_priv);
 void intel_power_domains_cleanup(struct drm_i915_private *dev_priv);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume);
 void intel_power_domains_driver_remove(struct drm_i915_private *dev_priv);
-void icl_display_core_init(struct drm_i915_private *dev_priv, bool resume);
-void icl_display_core_uninit(struct drm_i915_private *dev_priv);
 void intel_power_domains_enable(struct drm_i915_private *dev_priv);
 void intel_power_domains_disable(struct drm_i915_private *dev_priv);
 void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 				 enum i915_drm_suspend_mode);
 void intel_power_domains_resume(struct drm_i915_private *dev_priv);
-void hsw_enable_pc8(struct drm_i915_private *dev_priv);
-void hsw_disable_pc8(struct drm_i915_private *dev_priv);
-void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume);
-void bxt_display_core_uninit(struct drm_i915_private *dev_priv);
+
+void intel_display_power_suspend_late(struct drm_i915_private *i915);
+void intel_display_power_resume_early(struct drm_i915_private *i915);
+void intel_display_power_suspend(struct drm_i915_private *i915);
+void intel_display_power_resume(struct drm_i915_private *i915);
 
 const char *
 intel_display_power_domain_str(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7c209743e478..ced61fc4bc3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2166,7 +2166,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
-	int ret;
+	int ret = 0;
 
 	disable_rpm_wakeref_asserts(rpm);
 
@@ -2177,12 +2177,9 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	intel_power_domains_suspend(dev_priv,
 				    get_suspend_mode(dev_priv, hibernation));
 
-	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
-		bxt_enable_dc9(dev_priv);
-	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-		hsw_enable_pc8(dev_priv);
-	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+	intel_display_power_suspend_late(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_suspend_complete(dev_priv);
 
 	if (ret) {
@@ -2372,12 +2369,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	intel_gt_check_and_clear_faults(&dev_priv->gt);
 
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
-		gen9_sanitize_dc_state(dev_priv);
-		bxt_disable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_disable_pc8(dev_priv);
-	}
+	intel_display_power_resume_early(dev_priv);
 
 	intel_sanitize_gt_powersave(dev_priv);
 
@@ -2921,7 +2913,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
-	int ret;
+	int ret = 0;
 
 	if (WARN_ON_ONCE(!(dev_priv->gt_pm.rc6.enabled && HAS_RC6(dev_priv))))
 		return -ENODEV;
@@ -2945,16 +2937,9 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	intel_uncore_suspend(&dev_priv->uncore);
 
-	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11) {
-		icl_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_enable_pc8(dev_priv);
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+	intel_display_power_suspend(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		ret = vlv_suspend_complete(dev_priv);
 	}
 
@@ -3035,26 +3020,9 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
-	if (INTEL_GEN(dev_priv) >= 11) {
-		bxt_disable_dc9(dev_priv);
-		icl_display_core_init(dev_priv, true);
-		if (dev_priv->csr.dmc_payload) {
-			if (dev_priv->csr.allowed_dc_mask &
-			    DC_STATE_EN_UPTO_DC6)
-				skl_enable_dc6(dev_priv);
-			else if (dev_priv->csr.allowed_dc_mask &
-				 DC_STATE_EN_UPTO_DC5)
-				gen9_enable_dc5(dev_priv);
-		}
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_disable_dc9(dev_priv);
-		bxt_display_core_init(dev_priv, true);
-		if (dev_priv->csr.dmc_payload &&
-		    (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
-			gen9_enable_dc5(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_disable_pc8(dev_priv);
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+	intel_display_power_resume(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		ret = vlv_resume_prepare(dev_priv, true);
 	}
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-05-23 23:17 Rodrigo Vivi
  2019-05-28 12:47 ` Jani Nikula
@ 2019-07-10 23:29 ` Souza, Jose
  1 sibling, 0 replies; 16+ messages in thread
From: Souza, Jose @ 2019-07-10 23:29 UTC (permalink / raw)
  To: Vivi, Rodrigo, intel-gfx

On Thu, 2019-05-23 at 16:17 -0700, Rodrigo Vivi wrote:
> Suspend resume is broken if we try to enable/disable dc9 on
> cases with disabled displays.
> 
> v2: Make checkpatch happy:
>     - braces {} are not necessary for single statement blocks
> 
> v3: Also move hsw/bdw PC8 sequences since they are related to
>     display PM anyways. (Ville)

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1)
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 117 +++++++++++++++++++++---------
> --
>  1 file changed, 76 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 83d2eb9e74cb..bd73ce57741a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2118,6 +2118,17 @@ get_suspend_mode(struct drm_i915_private
> *dev_priv, bool hibernate)
>  	return I915_DRM_SUSPEND_MEM;
>  }
>  
> +static void intel_display_suspend_late(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> +		bxt_enable_dc9(dev_priv);
> +	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +		hsw_enable_pc8(dev_priv);
> +}
> +
>  static int i915_drm_suspend_late(struct drm_device *dev, bool
> hibernation)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2133,12 +2144,10 @@ static int i915_drm_suspend_late(struct
> drm_device *dev, bool hibernation)
>  	intel_power_domains_suspend(dev_priv,
>  				    get_suspend_mode(dev_priv,
> hibernation));
>  
> +	intel_display_suspend_late(dev_priv);
> +
>  	ret = 0;
> -	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> -		bxt_enable_dc9(dev_priv);
> -	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -		hsw_enable_pc8(dev_priv);
> -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		ret = vlv_suspend_complete(dev_priv);
>  
>  	if (ret) {
> @@ -2266,6 +2275,19 @@ static int i915_drm_resume(struct drm_device
> *dev)
>  	return 0;
>  }
>  
> +static void intel_display_resume_early(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> +		gen9_sanitize_dc_state(dev_priv);
> +		bxt_disable_dc9(dev_priv);
> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +		hsw_disable_pc8(dev_priv);
> +	}
> +}
> +
>  static int i915_drm_resume_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2328,12 +2350,7 @@ static int i915_drm_resume_early(struct
> drm_device *dev)
>  
>  	i915_check_and_clear_faults(dev_priv);
>  
> -	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> -		gen9_sanitize_dc_state(dev_priv);
> -		bxt_disable_dc9(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		hsw_disable_pc8(dev_priv);
> -	}
> +	intel_display_resume_early(dev_priv);
>  
>  	intel_uncore_sanitize(dev_priv);
>  
> @@ -2869,6 +2886,22 @@ static int vlv_resume_prepare(struct
> drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> +static void intel_runtime_display_suspend(struct drm_i915_private
> *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		icl_display_core_uninit(dev_priv);
> +		bxt_enable_dc9(dev_priv);
> +	} else if (IS_GEN9_LP(dev_priv)) {
> +		bxt_display_core_uninit(dev_priv);
> +		bxt_enable_dc9(dev_priv);
> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +		hsw_enable_pc8(dev_priv);
> +	}
> +}
> +
>  static int intel_runtime_suspend(struct device *kdev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(kdev);
> @@ -2898,18 +2931,11 @@ static int intel_runtime_suspend(struct
> device *kdev)
>  
>  	intel_uncore_suspend(&dev_priv->uncore);
>  
> +	intel_runtime_display_suspend(dev_priv);
> +
>  	ret = 0;
> -	if (INTEL_GEN(dev_priv) >= 11) {
> -		icl_display_core_uninit(dev_priv);
> -		bxt_enable_dc9(dev_priv);
> -	} else if (IS_GEN9_LP(dev_priv)) {
> -		bxt_display_core_uninit(dev_priv);
> -		bxt_enable_dc9(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		hsw_enable_pc8(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		ret = vlv_suspend_complete(dev_priv);
> -	}
>  
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it
> (%d)\n", ret);
> @@ -2967,25 +2993,10 @@ static int intel_runtime_suspend(struct
> device *kdev)
>  	return 0;
>  }
>  
> -static int intel_runtime_resume(struct device *kdev)
> +static void intel_runtime_display_resume(struct drm_i915_private
> *dev_priv)
>  {
> -	struct pci_dev *pdev = to_pci_dev(kdev);
> -	struct drm_device *dev = pci_get_drvdata(pdev);
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int ret = 0;
> -
> -	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
> -		return -ENODEV;
> -
> -	DRM_DEBUG_KMS("Resuming device\n");
> -
> -	WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
> -	disable_rpm_wakeref_asserts(dev_priv);
> -
> -	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> -	dev_priv->runtime_pm.suspended = false;
> -	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> -		DRM_DEBUG_DRIVER("Unclaimed access during suspend,
> bios?\n");
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
>  		bxt_disable_dc9(dev_priv);
> @@ -3006,9 +3017,33 @@ static int intel_runtime_resume(struct device
> *kdev)
>  			gen9_enable_dc5(dev_priv);
>  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		hsw_disable_pc8(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> {
> -		ret = vlv_resume_prepare(dev_priv, true);
>  	}
> +}
> +
> +static int intel_runtime_resume(struct device *kdev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kdev);
> +	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int ret = 0;
> +
> +	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
> +		return -ENODEV;
> +
> +	DRM_DEBUG_KMS("Resuming device\n");
> +
> +	WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
> +	disable_rpm_wakeref_asserts(dev_priv);
> +
> +	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> +	dev_priv->runtime_pm.suspended = false;
> +	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> +		DRM_DEBUG_DRIVER("Unclaimed access during suspend,
> bios?\n");
> +
> +	intel_runtime_display_resume(dev_priv);
> +
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		ret = vlv_resume_prepare(dev_priv, true);
>  
>  	intel_uncore_runtime_resume(&dev_priv->uncore);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
  2019-05-23 23:17 Rodrigo Vivi
@ 2019-05-28 12:47 ` Jani Nikula
  2019-07-10 23:29 ` Souza, Jose
  1 sibling, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2019-05-28 12:47 UTC (permalink / raw)
  To: Rodrigo Vivi, intel-gfx

On Thu, 23 May 2019, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Suspend resume is broken if we try to enable/disable dc9 on
> cases with disabled displays.
>
> v2: Make checkpatch happy:
>     - braces {} are not necessary for single statement blocks
>
> v3: Also move hsw/bdw PC8 sequences since they are related to
>     display PM anyways. (Ville)

Most if not all the new functions belong in intel_runtime_pm.c, and you
can make a bunch of functions static there as well. This is nicely in
line with not exposing platform specific functions from .c files.

BR,
Jani.


>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1)
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 117 +++++++++++++++++++++-----------
>  1 file changed, 76 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 83d2eb9e74cb..bd73ce57741a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2118,6 +2118,17 @@ get_suspend_mode(struct drm_i915_private *dev_priv, bool hibernate)
>  	return I915_DRM_SUSPEND_MEM;
>  }
>  
> +static void intel_display_suspend_late(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> +		bxt_enable_dc9(dev_priv);
> +	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +		hsw_enable_pc8(dev_priv);
> +}
> +
>  static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2133,12 +2144,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  	intel_power_domains_suspend(dev_priv,
>  				    get_suspend_mode(dev_priv, hibernation));
>  
> +	intel_display_suspend_late(dev_priv);
> +
>  	ret = 0;
> -	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> -		bxt_enable_dc9(dev_priv);
> -	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -		hsw_enable_pc8(dev_priv);
> -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		ret = vlv_suspend_complete(dev_priv);
>  
>  	if (ret) {
> @@ -2266,6 +2275,19 @@ static int i915_drm_resume(struct drm_device *dev)
>  	return 0;
>  }
>  
> +static void intel_display_resume_early(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> +		gen9_sanitize_dc_state(dev_priv);
> +		bxt_disable_dc9(dev_priv);
> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +		hsw_disable_pc8(dev_priv);
> +	}
> +}
> +
>  static int i915_drm_resume_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2328,12 +2350,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  
>  	i915_check_and_clear_faults(dev_priv);
>  
> -	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> -		gen9_sanitize_dc_state(dev_priv);
> -		bxt_disable_dc9(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		hsw_disable_pc8(dev_priv);
> -	}
> +	intel_display_resume_early(dev_priv);
>  
>  	intel_uncore_sanitize(dev_priv);
>  
> @@ -2869,6 +2886,22 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> +static void intel_runtime_display_suspend(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		icl_display_core_uninit(dev_priv);
> +		bxt_enable_dc9(dev_priv);
> +	} else if (IS_GEN9_LP(dev_priv)) {
> +		bxt_display_core_uninit(dev_priv);
> +		bxt_enable_dc9(dev_priv);
> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +		hsw_enable_pc8(dev_priv);
> +	}
> +}
> +
>  static int intel_runtime_suspend(struct device *kdev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(kdev);
> @@ -2898,18 +2931,11 @@ static int intel_runtime_suspend(struct device *kdev)
>  
>  	intel_uncore_suspend(&dev_priv->uncore);
>  
> +	intel_runtime_display_suspend(dev_priv);
> +
>  	ret = 0;
> -	if (INTEL_GEN(dev_priv) >= 11) {
> -		icl_display_core_uninit(dev_priv);
> -		bxt_enable_dc9(dev_priv);
> -	} else if (IS_GEN9_LP(dev_priv)) {
> -		bxt_display_core_uninit(dev_priv);
> -		bxt_enable_dc9(dev_priv);
> -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		hsw_enable_pc8(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		ret = vlv_suspend_complete(dev_priv);
> -	}
>  
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> @@ -2967,25 +2993,10 @@ static int intel_runtime_suspend(struct device *kdev)
>  	return 0;
>  }
>  
> -static int intel_runtime_resume(struct device *kdev)
> +static void intel_runtime_display_resume(struct drm_i915_private *dev_priv)
>  {
> -	struct pci_dev *pdev = to_pci_dev(kdev);
> -	struct drm_device *dev = pci_get_drvdata(pdev);
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int ret = 0;
> -
> -	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
> -		return -ENODEV;
> -
> -	DRM_DEBUG_KMS("Resuming device\n");
> -
> -	WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
> -	disable_rpm_wakeref_asserts(dev_priv);
> -
> -	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> -	dev_priv->runtime_pm.suspended = false;
> -	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> -		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
> +	if (!HAS_DISPLAY(dev_priv))
> +		return;
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
>  		bxt_disable_dc9(dev_priv);
> @@ -3006,9 +3017,33 @@ static int intel_runtime_resume(struct device *kdev)
>  			gen9_enable_dc5(dev_priv);
>  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		hsw_disable_pc8(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		ret = vlv_resume_prepare(dev_priv, true);
>  	}
> +}
> +
> +static int intel_runtime_resume(struct device *kdev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kdev);
> +	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int ret = 0;
> +
> +	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
> +		return -ENODEV;
> +
> +	DRM_DEBUG_KMS("Resuming device\n");
> +
> +	WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
> +	disable_rpm_wakeref_asserts(dev_priv);
> +
> +	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> +	dev_priv->runtime_pm.suspended = false;
> +	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> +		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
> +
> +	intel_runtime_display_resume(dev_priv);
> +
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		ret = vlv_resume_prepare(dev_priv, true);
>  
>  	intel_uncore_runtime_resume(&dev_priv->uncore);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY
@ 2019-05-23 23:17 Rodrigo Vivi
  2019-05-28 12:47 ` Jani Nikula
  2019-07-10 23:29 ` Souza, Jose
  0 siblings, 2 replies; 16+ messages in thread
From: Rodrigo Vivi @ 2019-05-23 23:17 UTC (permalink / raw)
  To: intel-gfx

Suspend resume is broken if we try to enable/disable dc9 on
cases with disabled displays.

v2: Make checkpatch happy:
    - braces {} are not necessary for single statement blocks

v3: Also move hsw/bdw PC8 sequences since they are related to
    display PM anyways. (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1)
---
 drivers/gpu/drm/i915/i915_drv.c | 117 +++++++++++++++++++++-----------
 1 file changed, 76 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 83d2eb9e74cb..bd73ce57741a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2118,6 +2118,17 @@ get_suspend_mode(struct drm_i915_private *dev_priv, bool hibernate)
 	return I915_DRM_SUSPEND_MEM;
 }
 
+static void intel_display_suspend_late(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
+		bxt_enable_dc9(dev_priv);
+	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+		hsw_enable_pc8(dev_priv);
+}
+
 static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2133,12 +2144,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	intel_power_domains_suspend(dev_priv,
 				    get_suspend_mode(dev_priv, hibernation));
 
+	intel_display_suspend_late(dev_priv);
+
 	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
-		bxt_enable_dc9(dev_priv);
-	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-		hsw_enable_pc8(dev_priv);
-	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_suspend_complete(dev_priv);
 
 	if (ret) {
@@ -2266,6 +2275,19 @@ static int i915_drm_resume(struct drm_device *dev)
 	return 0;
 }
 
+static void intel_display_resume_early(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
+		gen9_sanitize_dc_state(dev_priv);
+		bxt_disable_dc9(dev_priv);
+	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+		hsw_disable_pc8(dev_priv);
+	}
+}
+
 static int i915_drm_resume_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2328,12 +2350,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	i915_check_and_clear_faults(dev_priv);
 
-	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
-		gen9_sanitize_dc_state(dev_priv);
-		bxt_disable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_disable_pc8(dev_priv);
-	}
+	intel_display_resume_early(dev_priv);
 
 	intel_uncore_sanitize(dev_priv);
 
@@ -2869,6 +2886,22 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
+static void intel_runtime_display_suspend(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_DISPLAY(dev_priv))
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11) {
+		icl_display_core_uninit(dev_priv);
+		bxt_enable_dc9(dev_priv);
+	} else if (IS_GEN9_LP(dev_priv)) {
+		bxt_display_core_uninit(dev_priv);
+		bxt_enable_dc9(dev_priv);
+	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+		hsw_enable_pc8(dev_priv);
+	}
+}
+
 static int intel_runtime_suspend(struct device *kdev)
 {
 	struct pci_dev *pdev = to_pci_dev(kdev);
@@ -2898,18 +2931,11 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	intel_uncore_suspend(&dev_priv->uncore);
 
+	intel_runtime_display_suspend(dev_priv);
+
 	ret = 0;
-	if (INTEL_GEN(dev_priv) >= 11) {
-		icl_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_GEN9_LP(dev_priv)) {
-		bxt_display_core_uninit(dev_priv);
-		bxt_enable_dc9(dev_priv);
-	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		hsw_enable_pc8(dev_priv);
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_suspend_complete(dev_priv);
-	}
 
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
@@ -2967,25 +2993,10 @@ static int intel_runtime_suspend(struct device *kdev)
 	return 0;
 }
 
-static int intel_runtime_resume(struct device *kdev)
+static void intel_runtime_display_resume(struct drm_i915_private *dev_priv)
 {
-	struct pci_dev *pdev = to_pci_dev(kdev);
-	struct drm_device *dev = pci_get_drvdata(pdev);
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	int ret = 0;
-
-	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
-		return -ENODEV;
-
-	DRM_DEBUG_KMS("Resuming device\n");
-
-	WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
-	disable_rpm_wakeref_asserts(dev_priv);
-
-	intel_opregion_notify_adapter(dev_priv, PCI_D0);
-	dev_priv->runtime_pm.suspended = false;
-	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
-		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
+	if (!HAS_DISPLAY(dev_priv))
+		return;
 
 	if (INTEL_GEN(dev_priv) >= 11) {
 		bxt_disable_dc9(dev_priv);
@@ -3006,9 +3017,33 @@ static int intel_runtime_resume(struct device *kdev)
 			gen9_enable_dc5(dev_priv);
 	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		hsw_disable_pc8(dev_priv);
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-		ret = vlv_resume_prepare(dev_priv, true);
 	}
+}
+
+static int intel_runtime_resume(struct device *kdev)
+{
+	struct pci_dev *pdev = to_pci_dev(kdev);
+	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret = 0;
+
+	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
+		return -ENODEV;
+
+	DRM_DEBUG_KMS("Resuming device\n");
+
+	WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
+	disable_rpm_wakeref_asserts(dev_priv);
+
+	intel_opregion_notify_adapter(dev_priv, PCI_D0);
+	dev_priv->runtime_pm.suspended = false;
+	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
+		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
+
+	intel_runtime_display_resume(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		ret = vlv_resume_prepare(dev_priv, true);
 
 	intel_uncore_runtime_resume(&dev_priv->uncore);
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-08-06 17:01 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-20  4:56 [PATCH] drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY Rodrigo Vivi
2019-05-20  8:01 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2019-05-20  8:21 ` ✓ Fi.CI.BAT: success " Patchwork
2019-05-20  9:47 ` ✓ Fi.CI.IGT: " Patchwork
2019-05-20 21:35 ` [PATCH] " Souza, Jose
2019-05-23 23:17 Rodrigo Vivi
2019-05-28 12:47 ` Jani Nikula
2019-07-10 23:29 ` Souza, Jose
2019-07-18 20:47 Rodrigo Vivi
2019-07-18 20:49 Rodrigo Vivi
2019-07-18 20:58 ` Chris Wilson
2019-07-18 21:14   ` Rodrigo Vivi
2019-07-18 21:25     ` Chris Wilson
2019-07-18 21:38       ` Rodrigo Vivi
2019-08-06 12:02         ` Jani Nikula
2019-08-06 17:01           ` Rodrigo Vivi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.