[PATCH 0/4] drm/i915/pps: hide VLV/CHV PPS pipe stuff inside intel_pps.c

[PATCH 0/4] drm/i915/pps: hide VLV/CHV PPS pipe stuff inside intel_pps.c

[Jani Nikula]

It's bugged me for a long time that we poke the intel_pps->pps members
directly in a number of places, especially around VLV/CHV PPS pipe
handling. Hide it all inside intel_pps.c. The function naming may be a
bit contrived, but IMO better than the status quo.

BR,
Jani.

Jani Nikula (4):
  drm/i915/pps: add intel_pps_dp_init() for all DP init
  drm/i915/pps: move active_pipe set to intel_pps_dp_encoder_reset()
  drm/i915/pps: add intel_pps_dp_power_down()
  drm/i915/pps: add intel_pps_backlight_initial_pipe()

 drivers/gpu/drm/i915/display/g4x_dp.c    | 19 +------
 drivers/gpu/drm/i915/display/intel_ddi.c |  2 +-
 drivers/gpu/drm/i915/display/intel_dp.c  | 23 ++-------
 drivers/gpu/drm/i915/display/intel_pps.c | 63 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_pps.h |  6 ++-
 5 files changed, 73 insertions(+), 40 deletions(-)

-- 
2.39.2

[PATCH 1/4] drm/i915/pps: add intel_pps_dp_init() for all DP init

[Jani Nikula]

We need to track PPS also for non-eDP usage on VLV/CHV. Add new
intel_pps_dp_init() for initializing the related parts, hiding the PPS
pipe details inside PPS code.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c  |  5 +----
 drivers/gpu/drm/i915/display/intel_pps.c | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_pps.h |  2 ++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a1fcedfd404b..d1c02de97f5b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6843,8 +6843,6 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
 		return false;
 
 	intel_dp->reset_link_params = true;
-	intel_dp->pps.pps_pipe = INVALID_PIPE;
-	intel_dp->pps.active_pipe = INVALID_PIPE;
 
 	/* Preserve the current hw state. */
 	intel_dp->DP = intel_de_read(dev_priv, intel_dp->output_reg);
@@ -6871,8 +6869,7 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
 	intel_dp_set_default_sink_rates(intel_dp);
 	intel_dp_set_default_max_sink_lane_count(intel_dp);
 
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		intel_dp->pps.active_pipe = vlv_active_pipe(intel_dp);
+	intel_pps_dp_init(intel_dp);
 
 	intel_dp_aux_init(intel_dp);
 	intel_connector->dp.dsc_decompression_aux = &intel_dp->aux;
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index feddc30e3375..2b71e1bf519f 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -1625,6 +1625,19 @@ void intel_pps_encoder_reset(struct intel_dp *intel_dp)
 	}
 }
 
+/* Call on all DP, not just eDP */
+void intel_pps_dp_init(struct intel_dp *intel_dp)
+{
+	struct intel_display *display = to_intel_display(intel_dp);
+	struct drm_i915_private *i915 = to_i915(display->drm);
+
+	intel_dp->pps.pps_pipe = INVALID_PIPE;
+	intel_dp->pps.active_pipe = INVALID_PIPE;
+
+	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
+		intel_dp->pps.active_pipe = vlv_active_pipe(intel_dp);
+}
+
 bool intel_pps_init(struct intel_dp *intel_dp)
 {
 	intel_wakeref_t wakeref;
diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
index 0c5da83a559e..343798461f49 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.h
+++ b/drivers/gpu/drm/i915/display/intel_pps.h
@@ -42,6 +42,8 @@ void intel_pps_wait_power_cycle(struct intel_dp *intel_dp);
 
 bool intel_pps_init(struct intel_dp *intel_dp);
 void intel_pps_init_late(struct intel_dp *intel_dp);
+
+void intel_pps_dp_init(struct intel_dp *intel_dp);
 void intel_pps_encoder_reset(struct intel_dp *intel_dp);
 void intel_pps_reset_all(struct intel_display *display);
 
-- 
2.39.2

[PATCH 2/4] drm/i915/pps: move active_pipe set to intel_pps_dp_encoder_reset()

[Jani Nikula]

Rename intel_pps_encoder_reset() to intel_pps_dp_encoder_reset() to
highlight it's to be called on all DP, not just eDP. Move the VLV/CHV
active pipe set there from intel_dp_encoder_reset(), hiding the PPS pipe
details inside PPS code.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/g4x_dp.c    | 10 +---------
 drivers/gpu/drm/i915/display/intel_ddi.c |  2 +-
 drivers/gpu/drm/i915/display/intel_pps.c | 10 +++++++++-
 drivers/gpu/drm/i915/display/intel_pps.h |  2 +-
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
index 526c8c4d7b53..1f9812223263 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -1266,21 +1266,13 @@ enum pipe vlv_active_pipe(struct intel_dp *intel_dp)
 static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 {
 	struct intel_display *display = to_intel_display(encoder->dev);
-	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
 	struct intel_dp *intel_dp = enc_to_intel_dp(to_intel_encoder(encoder));
 
 	intel_dp->DP = intel_de_read(display, intel_dp->output_reg);
 
 	intel_dp->reset_link_params = true;
 
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-		intel_wakeref_t wakeref;
-
-		with_intel_pps_lock(intel_dp, wakeref)
-			intel_dp->pps.active_pipe = vlv_active_pipe(intel_dp);
-	}
-
-	intel_pps_encoder_reset(intel_dp);
+	intel_pps_dp_encoder_reset(intel_dp);
 }
 
 static const struct drm_encoder_funcs intel_dp_enc_funcs = {
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 00fbe9f8c03a..fba3be6420cd 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4392,7 +4392,7 @@ static void intel_ddi_encoder_reset(struct drm_encoder *encoder)
 
 	intel_dp->reset_link_params = true;
 
-	intel_pps_encoder_reset(intel_dp);
+	intel_pps_dp_encoder_reset(intel_dp);
 
 	if (intel_encoder_is_tc(&dig_port->base))
 		intel_tc_port_init_mode(dig_port);
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index 2b71e1bf519f..9e54acfea992 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -1599,12 +1599,20 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd
 		    (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK));
 }
 
-void intel_pps_encoder_reset(struct intel_dp *intel_dp)
+/* Call on all DP, not just eDP */
+void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 	struct drm_i915_private *i915 = to_i915(display->drm);
 	intel_wakeref_t wakeref;
 
+	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
+		intel_wakeref_t wakeref;
+
+		with_intel_pps_lock(intel_dp, wakeref)
+			intel_dp->pps.active_pipe = vlv_active_pipe(intel_dp);
+	}
+
 	if (!intel_dp_is_edp(intel_dp))
 		return;
 
diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
index 343798461f49..8dbea05f498d 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.h
+++ b/drivers/gpu/drm/i915/display/intel_pps.h
@@ -44,7 +44,7 @@ bool intel_pps_init(struct intel_dp *intel_dp);
 void intel_pps_init_late(struct intel_dp *intel_dp);
 
 void intel_pps_dp_init(struct intel_dp *intel_dp);
-void intel_pps_encoder_reset(struct intel_dp *intel_dp);
+void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp);
 void intel_pps_reset_all(struct intel_display *display);
 
 void vlv_pps_init(struct intel_encoder *encoder,
-- 
2.39.2

[PATCH 3/4] drm/i915/pps: add intel_pps_dp_power_down()

[Jani Nikula]

Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear
there from intel_dp_link_down(), hiding the PPS pipe details inside PPS
code.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/g4x_dp.c    |  9 +--------
 drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++
 drivers/gpu/drm/i915/display/intel_pps.h |  1 +
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
index 1f9812223263..98405fbd0e04 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder,
 		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 
-	msleep(intel_dp->pps.panel_power_down_delay);
-
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-		intel_wakeref_t wakeref;
-
-		with_intel_pps_lock(intel_dp, wakeref)
-			intel_dp->pps.active_pipe = INVALID_PIPE;
-	}
+	intel_pps_dp_power_down(intel_dp);
 }
 
 static void g4x_dp_audio_enable(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index 9e54acfea992..a7f7e5e1f3aa 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd
 		    (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK));
 }
 
+/* Call on all DP, not just eDP */
+void intel_pps_dp_power_down(struct intel_dp *intel_dp)
+{
+	struct intel_display *display = to_intel_display(intel_dp);
+	struct drm_i915_private *i915 = to_i915(display->drm);
+
+	msleep(intel_dp->pps.panel_power_down_delay);
+
+	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
+		intel_wakeref_t wakeref;
+
+		with_intel_pps_lock(intel_dp, wakeref)
+			intel_dp->pps.active_pipe = INVALID_PIPE;
+	}
+}
+
 /* Call on all DP, not just eDP */
 void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
index 8dbea05f498d..42f0377a93a8 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.h
+++ b/drivers/gpu/drm/i915/display/intel_pps.h
@@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp);
 
 void intel_pps_dp_init(struct intel_dp *intel_dp);
 void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp);
+void intel_pps_dp_power_down(struct intel_dp *intel_dp);
 void intel_pps_reset_all(struct intel_display *display);
 
 void vlv_pps_init(struct intel_encoder *encoder,
-- 
2.39.2

[PATCH 4/4] drm/i915/pps: add intel_pps_backlight_initial_pipe()

[Jani Nikula]

Add intel_pps_backlight_initial_pipe() and move the VLV/CHV initial
backlight pipe logic there, hiding the PPS pipe details inside PPS code.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c  | 18 ++----------------
 drivers/gpu/drm/i915/display/intel_pps.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_pps.h |  1 +
 3 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index d1c02de97f5b..e7e0f574d66e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6602,23 +6602,9 @@ intel_edp_add_properties(struct intel_dp *intel_dp)
 static void intel_edp_backlight_setup(struct intel_dp *intel_dp,
 				      struct intel_connector *connector)
 {
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	enum pipe pipe = INVALID_PIPE;
-
-	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
-		/*
-		 * Figure out the current pipe for the initial backlight setup.
-		 * If the current pipe isn't valid, try the PPS pipe, and if that
-		 * fails just assume pipe A.
-		 */
-		pipe = vlv_active_pipe(intel_dp);
-
-		if (pipe != PIPE_A && pipe != PIPE_B)
-			pipe = intel_dp->pps.pps_pipe;
+	enum pipe pipe;
 
-		if (pipe != PIPE_A && pipe != PIPE_B)
-			pipe = PIPE_A;
-	}
+	pipe = intel_pps_backlight_initial_pipe(intel_dp);
 
 	intel_backlight_setup(connector, pipe);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index a7f7e5e1f3aa..5382a1a0dbac 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -1071,6 +1071,30 @@ void intel_pps_off(struct intel_dp *intel_dp)
 		intel_pps_off_unlocked(intel_dp);
 }
 
+enum pipe intel_pps_backlight_initial_pipe(struct intel_dp *intel_dp)
+{
+	struct intel_display *display = to_intel_display(intel_dp);
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	enum pipe pipe = INVALID_PIPE;
+
+	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
+		/*
+		 * Figure out the current pipe for the initial backlight setup.
+		 * If the current pipe isn't valid, try the PPS pipe, and if that
+		 * fails just assume pipe A.
+		 */
+		pipe = vlv_active_pipe(intel_dp);
+
+		if (pipe != PIPE_A && pipe != PIPE_B)
+			pipe = intel_dp->pps.pps_pipe;
+
+		if (pipe != PIPE_A && pipe != PIPE_B)
+			pipe = PIPE_A;
+	}
+
+	return pipe;
+}
+
 /* Enable backlight in the panel power control. */
 void intel_pps_backlight_on(struct intel_dp *intel_dp)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
index 42f0377a93a8..7a23c9d9aebf 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.h
+++ b/drivers/gpu/drm/i915/display/intel_pps.h
@@ -23,6 +23,7 @@ intel_wakeref_t intel_pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wake
 #define with_intel_pps_lock(dp, wf)						\
 	for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), (wf)))
 
+enum pipe intel_pps_backlight_initial_pipe(struct intel_dp *intel_dp);
 void intel_pps_backlight_on(struct intel_dp *intel_dp);
 void intel_pps_backlight_off(struct intel_dp *intel_dp);
 void intel_pps_backlight_power(struct intel_connector *connector, bool enable);
-- 
2.39.2

✗ Fi.CI.BAT: failure for drm/i915/pps: hide VLV/CHV PPS pipe stuff inside intel_pps.c

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 9451 bytes --]

== Series Details ==

Series: drm/i915/pps: hide VLV/CHV PPS pipe stuff inside intel_pps.c
URL   : https://patchwork.freedesktop.org/series/138210/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15358 -> Patchwork_138210v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_138210v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_138210v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/index.html

Participating hosts (37 -> 39)
------------------------------

  Additional (5): bat-arlh-3 fi-bsw-n3050 fi-tgl-1115g4 fi-cfl-8109u fi-kbl-8809g 
  Missing    (3): fi-kbl-7567u bat-jsl-1 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_138210v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_mocs:
    - bat-arls-2:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15358/bat-arls-2/igt@i915_selftest@live@gt_mocs.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/bat-arls-2/igt@i915_selftest@live@gt_mocs.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][3] ([i915#9318])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@read:
    - bat-arls-1:         [PASS][4] -> [DMESG-WARN][5] ([i915#12102])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15358/bat-arls-1/igt@fbdev@read.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/bat-arls-1/igt@fbdev@read.html

  * igt@gem_huc_copy@huc-copy:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][6] ([i915#2190])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-cfl-8109u/igt@gem_huc_copy@huc-copy.html
    - fi-kbl-8809g:       NOTRUN -> [SKIP][7] ([i915#2190])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-kbl-8809g/igt@gem_huc_copy@huc-copy.html
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][8] ([i915#2190])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][9] ([i915#4613]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-kbl-8809g/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][10] ([i915#4613]) +3 other tests skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@random-engines:
    - fi-bsw-n3050:       NOTRUN -> [SKIP][11] +19 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-bsw-n3050/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][12] ([i915#4613]) +3 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-cfl-8109u/igt@gem_lmem_swapping@verify-random.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-twl-1:          [PASS][13] -> [INCOMPLETE][14] ([i915#10886] / [i915#9413])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15358/bat-twl-1/igt@i915_selftest@live@gt_lrc.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/bat-twl-1/igt@i915_selftest@live@gt_lrc.html

  * igt@i915_selftest@live@hangcheck:
    - bat-arls-1:         [PASS][15] -> [DMESG-WARN][16] ([i915#11349])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15358/bat-arls-1/igt@i915_selftest@live@hangcheck.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/bat-arls-1/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@workarounds:
    - bat-dg2-14:         [PASS][17] -> [DMESG-FAIL][18] ([i915#9500])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15358/bat-dg2-14/igt@i915_selftest@live@workarounds.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/bat-dg2-14/igt@i915_selftest@live@workarounds.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][19] ([i915#4103]) +1 other test skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][20] ([i915#3555] / [i915#3840])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][21] +30 other tests skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-kbl-8809g/igt@kms_force_connector_basic@force-load-detect.html
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][22]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pm_backlight@basic-brightness:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][23] +11 other tests skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-cfl-8109u/igt@kms_pm_backlight@basic-brightness.html
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][24] ([i915#9812])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-sprite-plane-onoff:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][25] ([i915#9732]) +3 other tests skip
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@kms_psr@psr-sprite-plane-onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][26] ([i915#3555])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/fi-tgl-1115g4/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@fbdev@info:
    - bat-arls-1:         [DMESG-WARN][27] ([i915#12102]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15358/bat-arls-1/igt@fbdev@info.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/bat-arls-1/igt@fbdev@info.html

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

  [i915#10196]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10196
  [i915#10886]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10886
  [i915#11343]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11343
  [i915#11346]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11346
  [i915#11349]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11349
  [i915#11666]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11666
  [i915#11671]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11671
  [i915#11681]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11681
  [i915#11723]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11723
  [i915#11724]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11724
  [i915#11725]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11725
  [i915#11726]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11726
  [i915#12102]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12102
  [i915#2190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#3840]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3840
  [i915#4103]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4103
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613
  [i915#8809]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/8809
  [i915#9318]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9318
  [i915#9413]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9413
  [i915#9500]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9500
  [i915#9732]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9732
  [i915#9812]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9812
  [i915#9886]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9886


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

  * Linux: CI_DRM_15358 -> Patchwork_138210v1

  CI-20190529: 20190529
  CI_DRM_15358: c72d3ffc0308b71024de6f80c3596668991c67ea @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8004: 0e443bec0ccfb56c2754437b465fc197ee4fd481 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_138210v1: c72d3ffc0308b71024de6f80c3596668991c67ea @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_138210v1/index.html

[-- Attachment #2: Type: text/html, Size: 10058 bytes --]

Re: [PATCH 3/4] drm/i915/pps: add intel_pps_dp_power_down()

[Ville Syrjälä]

On Wed, Sep 04, 2024 at 05:02:33PM +0300, Jani Nikula wrote:
> Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear
> there from intel_dp_link_down(), hiding the PPS pipe details inside PPS
> code.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/g4x_dp.c    |  9 +--------
>  drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_pps.h |  1 +
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> index 1f9812223263..98405fbd0e04 100644
> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> @@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder,
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  
> -	msleep(intel_dp->pps.panel_power_down_delay);
> -
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		intel_wakeref_t wakeref;
> -
> -		with_intel_pps_lock(intel_dp, wakeref)
> -			intel_dp->pps.active_pipe = INVALID_PIPE;
> -	}
> +	intel_pps_dp_power_down(intel_dp);
>  }
>  
>  static void g4x_dp_audio_enable(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 9e54acfea992..a7f7e5e1f3aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd
>  		    (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK));
>  }
>  
> +/* Call on all DP, not just eDP */
> +void intel_pps_dp_power_down(struct intel_dp *intel_dp)

The name seems to imply this powers down something, which it doesn't.

> +{
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +
> +	msleep(intel_dp->pps.panel_power_down_delay);

I can't actually figure out why this msleep() even exists. We already
waited for the power down delay (by waiting for the pps state machine)
when we turned off the panel power, so this seems completely redundant.

The whole pre-ddi modeset sequence is a bit weird because the port
enable/disable essentially happens on the wrong side of panel power
enable/disable. But I guess that's just how the hw works and the PPS
somehow makes sure things happen in the right order. And I suppose
the magic PPS register write protect thing even prevents doing it
in the opposite order (although IIRC we always disable the write
protect mechanism).

> +
> +	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> +		intel_wakeref_t wakeref;
> +
> +		with_intel_pps_lock(intel_dp, wakeref)
> +			intel_dp->pps.active_pipe = INVALID_PIPE;
> +	}

This part is basically the counterpart to vlv_pps_init(),
so the function naming should probably reflect that somehow.
vlv_pps_port_{enable,disable}() or something like that?

> +}
> +
>  /* Call on all DP, not just eDP */
>  void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
> index 8dbea05f498d..42f0377a93a8 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> @@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp);
>  
>  void intel_pps_dp_init(struct intel_dp *intel_dp);
>  void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp);
> +void intel_pps_dp_power_down(struct intel_dp *intel_dp);
>  void intel_pps_reset_all(struct intel_display *display);
>  
>  void vlv_pps_init(struct intel_encoder *encoder,
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/4] drm/i915/pps: add intel_pps_dp_power_down()

[Jani Nikula]

On Wed, 04 Sep 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Sep 04, 2024 at 05:02:33PM +0300, Jani Nikula wrote:
>> Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear
>> there from intel_dp_link_down(), hiding the PPS pipe details inside PPS
>> code.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/g4x_dp.c    |  9 +--------
>>  drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_pps.h |  1 +
>>  3 files changed, 18 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
>> index 1f9812223263..98405fbd0e04 100644
>> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
>> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
>> @@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder,
>>  		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>>  	}
>>  
>> -	msleep(intel_dp->pps.panel_power_down_delay);
>> -
>> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>> -		intel_wakeref_t wakeref;
>> -
>> -		with_intel_pps_lock(intel_dp, wakeref)
>> -			intel_dp->pps.active_pipe = INVALID_PIPE;
>> -	}
>> +	intel_pps_dp_power_down(intel_dp);
>>  }
>>  
>>  static void g4x_dp_audio_enable(struct intel_encoder *encoder,
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
>> index 9e54acfea992..a7f7e5e1f3aa 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> @@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd
>>  		    (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK));
>>  }
>>  
>> +/* Call on all DP, not just eDP */
>> +void intel_pps_dp_power_down(struct intel_dp *intel_dp)
>
> The name seems to imply this powers down something, which it doesn't.

Agreed.

>
>> +{
>> +	struct intel_display *display = to_intel_display(intel_dp);
>> +	struct drm_i915_private *i915 = to_i915(display->drm);
>> +
>> +	msleep(intel_dp->pps.panel_power_down_delay);
>
> I can't actually figure out why this msleep() even exists. We already
> waited for the power down delay (by waiting for the pps state machine)
> when we turned off the panel power, so this seems completely redundant.
>
> The whole pre-ddi modeset sequence is a bit weird because the port
> enable/disable essentially happens on the wrong side of panel power
> enable/disable. But I guess that's just how the hw works and the PPS
> somehow makes sure things happen in the right order. And I suppose
> the magic PPS register write protect thing even prevents doing it
> in the opposite order (although IIRC we always disable the write
> protect mechanism).
>
>> +
>> +	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
>> +		intel_wakeref_t wakeref;
>> +
>> +		with_intel_pps_lock(intel_dp, wakeref)
>> +			intel_dp->pps.active_pipe = INVALID_PIPE;
>> +	}
>
> This part is basically the counterpart to vlv_pps_init(),
> so the function naming should probably reflect that somehow.
> vlv_pps_port_{enable,disable}() or something like that?

I ended up doing things a bit differently across the entire series,
properly isolating pps_pipe/active_pipe to VLV/CHV code only, and adding
functions for that stuff.

I left the above msleep() where it is now. Didn't dare touch it yet, and
it should be a separate change to remove it.

New version of the whole series at [1].

BR,
Jani.


[1] https://lore.kernel.org/all/cover.1725883885.git.jani.nikula@intel.com/





>
>> +}
>> +
>>  /* Call on all DP, not just eDP */
>>  void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp)
>>  {
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
>> index 8dbea05f498d..42f0377a93a8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.h
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
>> @@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp);
>>  
>>  void intel_pps_dp_init(struct intel_dp *intel_dp);
>>  void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp);
>> +void intel_pps_dp_power_down(struct intel_dp *intel_dp);
>>  void intel_pps_reset_all(struct intel_display *display);
>>  
>>  void vlv_pps_init(struct intel_encoder *encoder,
>> -- 
>> 2.39.2

-- 
Jani Nikula, Intel