[RFC v2 0/7] Rearranging PPS related code

[RFC v2 0/7] Rearranging PPS related code

[Vandana Kannan]

Since panel power sequencing is a feature common to all internal displays, 
moving relevant code to intel_panel.c. This patch series contains changes 
to setup PPS data and program register values as required.

The implementation follows the model used for backlight funcs 
(as suggested by Daniel) which splits the changes based on platform. 
As of now, changes have been made considering only eDP. 

TODO:-
1. Accomodate software delays where applicable.
2. Integrate MIPI PPS delays once 1. is done.
3. PPS delays would be required for LVDS as well. The existing file 
intel_lvds.c does not make use of the delays. 

v2: Splitting the patches per function and then splitting the functions per
platform.

Vandana Kannan (7):
  drm/i915: Move around funcs related to eDP PPS
  drm/i915: Setup PPS in intel_panel
  drm/i915: Split PPS setup code based on platform
  drm/i915: Program PPS registers
  drm/i915: Split PPS reg write func based on platform
  drm/i915: Replace all refs to intel_dp delays
  drm/i915: Modify refs to intel dp timestamps

 drivers/gpu/drm/i915/i915_drv.h      |   7 +
 drivers/gpu/drm/i915/intel_display.c |   1 +
 drivers/gpu/drm/i915/intel_dp.c      | 266 +++++------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  30 ++--
 drivers/gpu/drm/i915/intel_panel.c   | 258 +++++++++++++++++++++++++++++++++
 5 files changed, 324 insertions(+), 238 deletions(-)

-- 
2.0.1

[RFC 1/7] drm/i915: Move around funcs related to eDP PPS

[Vandana Kannan]

Making ironlake_get_pp_control & vlv_power_sequencer_pipe non-static, to be
used in future patches.
vlv_power_sequencer_pipe should return only pipe, need not set PPS
registers again.
Moving calls to init PPS data and registers to edp_init_connector instead of
having a if(edp) in dp_init_connector.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 31 ++++++-------------------------
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..2dfdc26 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -322,7 +322,7 @@ static void pps_unlock(struct intel_dp *intel_dp)
 	intel_display_power_put(dev_priv, power_domain);
 }
 
-static enum pipe
+enum pipe
 vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -330,7 +330,6 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
 	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
-	struct edp_power_seq power_seq;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
@@ -367,11 +366,6 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 		      pipe_name(intel_dp->pps_pipe),
 		      port_name(intel_dig_port->port));
 
-	/* init power sequencer on this pipe and port */
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
-
 	return intel_dp->pps_pipe;
 }
 
@@ -1366,7 +1360,7 @@ static void edp_wait_backlight_off(struct intel_dp *intel_dp)
  * is locked
  */
 
-static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
+u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2620,7 +2614,6 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-	struct edp_power_seq power_seq;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
@@ -2647,10 +2640,6 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("initializing pipe %c power sequencer for port %c\n",
 		      pipe_name(intel_dp->pps_pipe), port_name(intel_dig_port->port));
 
-	/* init power sequencer on this pipe and port */
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
 }
 
 static void vlv_pre_enable_dp(struct intel_encoder *encoder)
@@ -5103,6 +5092,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* We now know it's not a ghost, init power sequence regs. */
 	pps_lock(intel_dp);
+	if (IS_VALLEYVIEW(dev))
+		vlv_initial_power_sequencer_setup(intel_dp);
+	intel_dp_init_panel_power_timestamps(intel_dp);
+	intel_dp_init_panel_power_sequencer(dev, intel_dp, power_seq);
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
 	pps_unlock(intel_dp);
 
@@ -5243,18 +5236,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		BUG();
 	}
 
-	if (is_edp(intel_dp)) {
-		pps_lock(intel_dp);
-		if (IS_VALLEYVIEW(dev)) {
-			vlv_initial_power_sequencer_setup(intel_dp);
-		} else {
-			intel_dp_init_panel_power_timestamps(intel_dp);
-			intel_dp_init_panel_power_sequencer(dev, intel_dp,
-							    &power_seq);
-		}
-		pps_unlock(intel_dp);
-	}
-
 	intel_dp_aux_init(intel_dp, intel_connector);
 
 	/* init MST on ports that can support it */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5ab813c..abe2b00 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -988,7 +988,9 @@ void intel_dp_mst_suspend(struct drm_device *dev);
 void intel_dp_mst_resume(struct drm_device *dev);
 int intel_dp_max_link_bw(struct intel_dp *intel_dp);
 void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
+enum pipe vlv_power_sequencer_pipe(struct intel_dp *intel_dp);
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
+u32 ironlake_get_pp_control(struct intel_dp *intel_dp);
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
-- 
2.0.1

[RFC 2/7] drm/i915: Setup PPS in intel_panel

[Vandana Kannan]

Moving intel_dp_setup_panel_power_sequencer code to intel_panel.c to make
PPS code generic in future patches. This patches substitutes references to
intel_dp_setup_panel_power_sequencer with a new function in intel_panel.c.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c    | 121 +++----------------------------------
 drivers/gpu/drm/i915/intel_drv.h   |  11 ++++
 drivers/gpu/drm/i915/intel_panel.c | 103 +++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2dfdc26..5b5d90a40 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -282,10 +282,6 @@ intel_hrawclk(struct drm_device *dev)
 }
 
 static void
-intel_dp_init_panel_power_sequencer(struct drm_device *dev,
-				    struct intel_dp *intel_dp,
-				    struct edp_power_seq *out);
-static void
 intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 					      struct intel_dp *intel_dp,
 					      struct edp_power_seq *out);
@@ -419,6 +415,7 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct edp_power_seq power_seq;
 	enum port port = intel_dig_port->port;
 
@@ -447,7 +444,7 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("initial power sequencer for port %c: pipe %c\n",
 		      port_name(port), pipe_name(intel_dp->pps_pipe));
 
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
+	intel_panel_setup_panel_power_sequencer(intel_connector);
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
 						      &power_seq);
 }
@@ -4725,113 +4722,13 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
 }
 
 static void
-intel_dp_init_panel_power_sequencer(struct drm_device *dev,
-				    struct intel_dp *intel_dp,
-				    struct edp_power_seq *out)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct edp_power_seq cur, vbt, spec, final;
-	u32 pp_on, pp_off, pp_div, pp;
-	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
-
-	lockdep_assert_held(&dev_priv->pps_mutex);
-
-	if (HAS_PCH_SPLIT(dev)) {
-		pp_ctrl_reg = PCH_PP_CONTROL;
-		pp_on_reg = PCH_PP_ON_DELAYS;
-		pp_off_reg = PCH_PP_OFF_DELAYS;
-		pp_div_reg = PCH_PP_DIVISOR;
-	} else {
-		enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
-
-		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
-		pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
-		pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
-		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
-	}
-
-	/* Workaround: Need to write PP_CONTROL with the unlock key as
-	 * the very first thing. */
-	pp = ironlake_get_pp_control(intel_dp);
-	I915_WRITE(pp_ctrl_reg, pp);
-
-	pp_on = I915_READ(pp_on_reg);
-	pp_off = I915_READ(pp_off_reg);
-	pp_div = I915_READ(pp_div_reg);
-
-	/* Pull timing values out of registers */
-	cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
-		PANEL_POWER_UP_DELAY_SHIFT;
-
-	cur.t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
-		PANEL_LIGHT_ON_DELAY_SHIFT;
-
-	cur.t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
-		PANEL_LIGHT_OFF_DELAY_SHIFT;
-
-	cur.t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
-		PANEL_POWER_DOWN_DELAY_SHIFT;
-
-	cur.t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
-		       PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
-
-	DRM_DEBUG_KMS("cur t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
-		      cur.t1_t3, cur.t8, cur.t9, cur.t10, cur.t11_t12);
-
-	vbt = dev_priv->vbt.edp_pps;
-
-	/* Upper limits from eDP 1.3 spec. Note that we use the clunky units of
-	 * our hw here, which are all in 100usec. */
-	spec.t1_t3 = 210 * 10;
-	spec.t8 = 50 * 10; /* no limit for t8, use t7 instead */
-	spec.t9 = 50 * 10; /* no limit for t9, make it symmetric with t8 */
-	spec.t10 = 500 * 10;
-	/* This one is special and actually in units of 100ms, but zero
-	 * based in the hw (so we need to add 100 ms). But the sw vbt
-	 * table multiplies it with 1000 to make it in units of 100usec,
-	 * too. */
-	spec.t11_t12 = (510 + 100) * 10;
-
-	DRM_DEBUG_KMS("vbt t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
-		      vbt.t1_t3, vbt.t8, vbt.t9, vbt.t10, vbt.t11_t12);
-
-	/* Use the max of the register settings and vbt. If both are
-	 * unset, fall back to the spec limits. */
-#define assign_final(field)	final.field = (max(cur.field, vbt.field) == 0 ? \
-				       spec.field : \
-				       max(cur.field, vbt.field))
-	assign_final(t1_t3);
-	assign_final(t8);
-	assign_final(t9);
-	assign_final(t10);
-	assign_final(t11_t12);
-#undef assign_final
-
-#define get_delay(field)	(DIV_ROUND_UP(final.field, 10))
-	intel_dp->panel_power_up_delay = get_delay(t1_t3);
-	intel_dp->backlight_on_delay = get_delay(t8);
-	intel_dp->backlight_off_delay = get_delay(t9);
-	intel_dp->panel_power_down_delay = get_delay(t10);
-	intel_dp->panel_power_cycle_delay = get_delay(t11_t12);
-#undef get_delay
-
-	DRM_DEBUG_KMS("panel power up delay %d, power down delay %d, power cycle delay %d\n",
-		      intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay,
-		      intel_dp->panel_power_cycle_delay);
-
-	DRM_DEBUG_KMS("backlight on delay %d, off delay %d\n",
-		      intel_dp->backlight_on_delay, intel_dp->backlight_off_delay);
-
-	if (out)
-		*out = final;
-}
-
-static void
 intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 					      struct intel_dp *intel_dp,
 					      struct edp_power_seq *seq)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct intel_panel *panel = &intel_connector->panel;
 	u32 pp_on, pp_off, pp_div, port_sel = 0;
 	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
 	int pp_on_reg, pp_off_reg, pp_div_reg;
@@ -4859,14 +4756,16 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 	 * do the manual sleep, and once when we disable the panel and wait for
 	 * the PP_STATUS bit to become zero.
 	 */
-	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
+	pp_on = (panel->pps.panel_power_up_delay <<
+		PANEL_POWER_UP_DELAY_SHIFT) |
 		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
 	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
-		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
+		 (panel->pps.panel_power_down_delay <<
+		 PANEL_POWER_DOWN_DELAY_SHIFT);
 	/* Compute the divisor for the pp clock, simply match the Bspec
 	 * formula. */
 	pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
-	pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
+	pp_div |= (DIV_ROUND_UP(panel->pps.panel_power_cycle_delay, 1000)
 			<< PANEL_POWER_CYCLE_DELAY_SHIFT);
 
 	/* Haswell doesn't have any port selection bits for the panel
@@ -5095,7 +4994,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (IS_VALLEYVIEW(dev))
 		vlv_initial_power_sequencer_setup(intel_dp);
 	intel_dp_init_panel_power_timestamps(intel_dp);
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, power_seq);
+	intel_panel_setup_panel_power_sequencer(intel_connector);
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
 	pps_unlock(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index abe2b00..76fa3b3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -186,6 +186,15 @@ struct intel_panel {
 	} backlight;
 
 	void (*backlight_power)(struct intel_connector *, bool enable);
+
+	/* PPS */
+	struct {
+		int panel_power_up_delay;
+		int panel_power_down_delay;
+		int panel_power_cycle_delay;
+		int backlight_on_delay;
+		int backlight_off_delay;
+	} pps;
 };
 
 struct intel_connector {
@@ -1090,6 +1099,8 @@ extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_device *dev,
 				struct drm_display_mode *fixed_mode,
 				struct drm_connector *connector);
+void intel_panel_setup_panel_power_sequencer(
+				struct intel_connector *connector);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index d48d1bc..c213ae3 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1346,6 +1346,109 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
 	}
 }
 
+void
+intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
+{
+	struct intel_panel *panel = &connector->panel;
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_encoder *encoder = connector->base.encoder;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	struct edp_power_seq cur, vbt, spec, final;
+	u32 pp_on, pp_off, pp_div, pp;
+	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	if (HAS_PCH_SPLIT(dev)) {
+		pp_ctrl_reg = PCH_PP_CONTROL;
+		pp_on_reg = PCH_PP_ON_DELAYS;
+		pp_off_reg = PCH_PP_OFF_DELAYS;
+		pp_div_reg = PCH_PP_DIVISOR;
+	} else {
+		enum pipe pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
+
+		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
+		pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
+		pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
+		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
+	}
+
+	/* Workaround: Need to write PP_CONTROL with the unlock key as
+	 * the very first thing. */
+	pp = ironlake_get_pp_control(&intel_dig_port->dp);
+	I915_WRITE(pp_ctrl_reg, pp);
+
+	pp_on = I915_READ(pp_on_reg);
+	pp_off = I915_READ(pp_off_reg);
+	pp_div = I915_READ(pp_div_reg);
+
+	/* Pull timing values out of registers */
+	cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
+		PANEL_POWER_UP_DELAY_SHIFT;
+
+	cur.t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
+		PANEL_LIGHT_ON_DELAY_SHIFT;
+
+	cur.t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
+		PANEL_LIGHT_OFF_DELAY_SHIFT;
+
+	cur.t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
+		PANEL_POWER_DOWN_DELAY_SHIFT;
+
+	cur.t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
+		       PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
+
+	DRM_DEBUG_KMS("cur t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
+		      cur.t1_t3, cur.t8, cur.t9, cur.t10, cur.t11_t12);
+
+	vbt = dev_priv->vbt.edp_pps;
+
+	/* Upper limits from eDP 1.3 spec. Note that we use the clunky units of
+	 * our hw here, which are all in 100usec. */
+	spec.t1_t3 = 210 * 10;
+	spec.t8 = 50 * 10; /* no limit for t8, use t7 instead */
+	spec.t9 = 50 * 10; /* no limit for t9, make it symmetric with t8 */
+	spec.t10 = 500 * 10;
+	/* This one is special and actually in units of 100ms, but zero
+	 * based in the hw (so we need to add 100 ms). But the sw vbt
+	 * table multiplies it with 1000 to make it in units of 100usec,
+	 * too. */
+	spec.t11_t12 = (510 + 100) * 10;
+
+	DRM_DEBUG_KMS("vbt t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
+		      vbt.t1_t3, vbt.t8, vbt.t9, vbt.t10, vbt.t11_t12);
+
+	/* Use the max of the register settings and vbt. If both are
+	 * unset, fall back to the spec limits. */
+#define assign_final(field)	final.field = (max(cur.field, vbt.field) == 0 ? \
+				       spec.field : \
+				       max(cur.field, vbt.field))
+	assign_final(t1_t3);
+	assign_final(t8);
+	assign_final(t9);
+	assign_final(t10);
+	assign_final(t11_t12);
+#undef assign_final
+
+#define get_delay(field)	(DIV_ROUND_UP(final.field, 10))
+	panel->pps.panel_power_up_delay = get_delay(t1_t3);
+	panel->pps.backlight_on_delay = get_delay(t8);
+	panel->pps.backlight_off_delay = get_delay(t9);
+	panel->pps.panel_power_down_delay = get_delay(t10);
+	panel->pps.panel_power_cycle_delay = get_delay(t11_t12);
+#undef get_delay
+
+	DRM_DEBUG_KMS("panel power up delay %d, power down delay %d, power cycle delay %d\n",
+		      panel->pps.panel_power_up_delay,
+		      panel->pps.panel_power_down_delay,
+		      panel->pps.panel_power_cycle_delay);
+
+	DRM_DEBUG_KMS("backlight on delay %d, off delay %d\n",
+		      panel->pps.backlight_on_delay,
+		      panel->pps.backlight_off_delay);
+}
+
 int intel_panel_init(struct intel_panel *panel,
 		     struct drm_display_mode *fixed_mode,
 		     struct drm_display_mode *downclock_mode)
-- 
2.0.1

[RFC 3/7] drm/i915: Split PPS setup code based on platform

[Vandana Kannan]

In the setup part, the differnce between vlv and other platforms is only
w.r.t registers. Other parts like reading VBT are common.
Removing calls to setup PPS and registers again through vlv_setup_panel_power
sequencer (this would be redundant).

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   3 ++
 drivers/gpu/drm/i915/intel_display.c |   1 +
 drivers/gpu/drm/i915/intel_dp.c      |   9 +---
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_panel.c   | 101 ++++++++++++++++++++++++++---------
 5 files changed, 82 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ac6232b..1446d02 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -508,6 +508,9 @@ struct drm_i915_display_funcs {
 			      uint32_t level);
 	void (*disable_backlight)(struct intel_connector *connector);
 	void (*enable_backlight)(struct intel_connector *connector);
+
+	struct edp_power_seq (*setup_panel_power_seq)
+				(struct intel_connector *connector);
 };
 
 struct intel_uncore_funcs {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 783a769..4e569ca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12677,6 +12677,7 @@ static void intel_init_display(struct drm_device *dev)
 	}
 
 	intel_panel_init_backlight_funcs(dev);
+	intel_panel_init_pps_funcs(dev);
 
 	mutex_init(&dev_priv->pps_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5b5d90a40..a433c5f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -409,14 +409,12 @@ vlv_initial_pps_pipe(struct drm_i915_private *dev_priv,
 	return INVALID_PIPE;
 }
 
-static void
+void
 vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_connector *intel_connector = intel_dp->attached_connector;
-	struct edp_power_seq power_seq;
 	enum port port = intel_dig_port->port;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
@@ -444,9 +442,6 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("initial power sequencer for port %c: pipe %c\n",
 		      port_name(port), pipe_name(intel_dp->pps_pipe));
 
-	intel_panel_setup_panel_power_sequencer(intel_connector);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
 }
 
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv)
@@ -4991,8 +4986,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* We now know it's not a ghost, init power sequence regs. */
 	pps_lock(intel_dp);
-	if (IS_VALLEYVIEW(dev))
-		vlv_initial_power_sequencer_setup(intel_dp);
 	intel_dp_init_panel_power_timestamps(intel_dp);
 	intel_panel_setup_panel_power_sequencer(intel_connector);
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 76fa3b3..597e09f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -998,6 +998,7 @@ void intel_dp_mst_resume(struct drm_device *dev);
 int intel_dp_max_link_bw(struct intel_dp *intel_dp);
 void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
 enum pipe vlv_power_sequencer_pipe(struct intel_dp *intel_dp);
+void vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp);
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
 u32 ironlake_get_pp_control(struct intel_dp *intel_dp);
 /* intel_dp_mst.c */
@@ -1101,6 +1102,7 @@ extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_connector *connector);
 void intel_panel_setup_panel_power_sequencer(
 				struct intel_connector *connector);
+void intel_panel_init_pps_funcs(struct drm_device *dev);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c213ae3..9b124ef 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1346,43 +1346,27 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
 	}
 }
 
-void
-intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
+static struct edp_power_seq pch_get_pps_registers(
+				struct intel_connector *connector,
+				u32 pp_ctrl_reg, u32 pp_on_reg,
+				u32 pp_off_reg, u32 pp_div_reg)
 {
-	struct intel_panel *panel = &connector->panel;
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_encoder *encoder = connector->base.encoder;
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
-	struct edp_power_seq cur, vbt, spec, final;
+	struct edp_power_seq cur;
 	u32 pp_on, pp_off, pp_div, pp;
-	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
-
-	lockdep_assert_held(&dev_priv->pps_mutex);
 
-	if (HAS_PCH_SPLIT(dev)) {
-		pp_ctrl_reg = PCH_PP_CONTROL;
-		pp_on_reg = PCH_PP_ON_DELAYS;
-		pp_off_reg = PCH_PP_OFF_DELAYS;
-		pp_div_reg = PCH_PP_DIVISOR;
-	} else {
-		enum pipe pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
-
-		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
-		pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
-		pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
-		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
-	}
+	pp_on = I915_READ(pp_on_reg);
+	pp_off = I915_READ(pp_off_reg);
+	pp_div = I915_READ(pp_div_reg);
 
 	/* Workaround: Need to write PP_CONTROL with the unlock key as
 	 * the very first thing. */
 	pp = ironlake_get_pp_control(&intel_dig_port->dp);
 	I915_WRITE(pp_ctrl_reg, pp);
 
-	pp_on = I915_READ(pp_on_reg);
-	pp_off = I915_READ(pp_off_reg);
-	pp_div = I915_READ(pp_div_reg);
-
 	/* Pull timing values out of registers */
 	cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
 		PANEL_POWER_UP_DELAY_SHIFT;
@@ -1397,10 +1381,63 @@ intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
 		PANEL_POWER_DOWN_DELAY_SHIFT;
 
 	cur.t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
-		       PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
+			PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
 
 	DRM_DEBUG_KMS("cur t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
-		      cur.t1_t3, cur.t8, cur.t9, cur.t10, cur.t11_t12);
+			cur.t1_t3, cur.t8, cur.t9, cur.t10, cur.t11_t12);
+
+	return cur;
+}
+
+static struct edp_power_seq pch_setup_pps(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	pp_ctrl_reg = PCH_PP_CONTROL;
+	pp_on_reg = PCH_PP_ON_DELAYS;
+	pp_off_reg = PCH_PP_OFF_DELAYS;
+	pp_div_reg = PCH_PP_DIVISOR;
+
+	return pch_get_pps_registers(connector, pp_ctrl_reg, pp_on_reg,
+			pp_off_reg, pp_div_reg);
+}
+
+static struct edp_power_seq vlv_setup_pps(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_encoder *encoder = connector->base.encoder;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	enum pipe pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
+	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
+
+	vlv_initial_power_sequencer_setup(&intel_dig_port->dp);
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
+	pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
+	pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
+	pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
+
+	return pch_get_pps_registers(connector, pp_ctrl_reg, pp_on_reg,
+			pp_off_reg, pp_div_reg);
+}
+
+void
+intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
+{
+	struct intel_panel *panel = &connector->panel;
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct edp_power_seq cur, vbt, spec, final;
+
+	/* Get chip specific register values */
+	cur = dev_priv->display.setup_panel_power_seq(connector);
 
 	vbt = dev_priv->vbt.edp_pps;
 
@@ -1449,6 +1486,18 @@ intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
 		      panel->pps.backlight_off_delay);
 }
 
+/* Setup chip specific PPS functions */
+void intel_panel_init_pps_funcs(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_VALLEYVIEW(dev))
+		dev_priv->display.setup_panel_power_seq = vlv_setup_pps;
+	else
+		dev_priv->display.setup_panel_power_seq = pch_setup_pps;
+}
+
+	
 int intel_panel_init(struct intel_panel *panel,
 		     struct drm_display_mode *fixed_mode,
 		     struct drm_display_mode *downclock_mode)
-- 
2.0.1

[RFC 4/7] drm/i915: Program PPS registers

[Vandana Kannan]

Actually set values into PPS related registers. This implementation is
equivalent to intel_dp_panel_power_sequencer_registers where the values
saved intially are written into registers.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c    | 80 ++------------------------------------
 drivers/gpu/drm/i915/intel_drv.h   |  3 ++
 drivers/gpu/drm/i915/intel_panel.c | 70 +++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a433c5f..ca11eb1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -248,7 +248,7 @@ unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes)
 }
 
 /* hrawclock is 1/4 the FSB frequency */
-static int
+int
 intel_hrawclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -281,11 +281,6 @@ intel_hrawclk(struct drm_device *dev)
 	}
 }
 
-static void
-intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
-					      struct intel_dp *intel_dp,
-					      struct edp_power_seq *out);
-
 static void pps_lock(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -4716,76 +4711,6 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
 	intel_dp->last_backlight_off = jiffies;
 }
 
-static void
-intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
-					      struct intel_dp *intel_dp,
-					      struct edp_power_seq *seq)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_connector *intel_connector = intel_dp->attached_connector;
-	struct intel_panel *panel = &intel_connector->panel;
-	u32 pp_on, pp_off, pp_div, port_sel = 0;
-	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
-	int pp_on_reg, pp_off_reg, pp_div_reg;
-	enum port port = dp_to_dig_port(intel_dp)->port;
-
-	lockdep_assert_held(&dev_priv->pps_mutex);
-
-	if (HAS_PCH_SPLIT(dev)) {
-		pp_on_reg = PCH_PP_ON_DELAYS;
-		pp_off_reg = PCH_PP_OFF_DELAYS;
-		pp_div_reg = PCH_PP_DIVISOR;
-	} else {
-		enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
-
-		pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
-		pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
-		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
-	}
-
-	/*
-	 * And finally store the new values in the power sequencer. The
-	 * backlight delays are set to 1 because we do manual waits on them. For
-	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
-	 * we'll end up waiting for the backlight off delay twice: once when we
-	 * do the manual sleep, and once when we disable the panel and wait for
-	 * the PP_STATUS bit to become zero.
-	 */
-	pp_on = (panel->pps.panel_power_up_delay <<
-		PANEL_POWER_UP_DELAY_SHIFT) |
-		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
-	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
-		 (panel->pps.panel_power_down_delay <<
-		 PANEL_POWER_DOWN_DELAY_SHIFT);
-	/* Compute the divisor for the pp clock, simply match the Bspec
-	 * formula. */
-	pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
-	pp_div |= (DIV_ROUND_UP(panel->pps.panel_power_cycle_delay, 1000)
-			<< PANEL_POWER_CYCLE_DELAY_SHIFT);
-
-	/* Haswell doesn't have any port selection bits for the panel
-	 * power sequencer any more. */
-	if (IS_VALLEYVIEW(dev)) {
-		port_sel = PANEL_PORT_SELECT_VLV(port);
-	} else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		if (port == PORT_A)
-			port_sel = PANEL_PORT_SELECT_DPA;
-		else
-			port_sel = PANEL_PORT_SELECT_DPD;
-	}
-
-	pp_on |= port_sel;
-
-	I915_WRITE(pp_on_reg, pp_on);
-	I915_WRITE(pp_off_reg, pp_off);
-	I915_WRITE(pp_div_reg, pp_div);
-
-	DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n",
-		      I915_READ(pp_on_reg),
-		      I915_READ(pp_off_reg),
-		      I915_READ(pp_div_reg));
-}
-
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4960,6 +4885,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	bool has_dpcd;
 	struct drm_display_mode *scan;
 	struct edid *edid;
+	enum port port = intel_dig_port->port;
 
 	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
 
@@ -4988,7 +4914,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	pps_lock(intel_dp);
 	intel_dp_init_panel_power_timestamps(intel_dp);
 	intel_panel_setup_panel_power_sequencer(intel_connector);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
+	intel_panel_set_pps_registers(intel_connector, port);
 	pps_unlock(intel_dp);
 
 	mutex_lock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 597e09f..4ae913c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1001,6 +1001,7 @@ enum pipe vlv_power_sequencer_pipe(struct intel_dp *intel_dp);
 void vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp);
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
 u32 ironlake_get_pp_control(struct intel_dp *intel_dp);
+int intel_hrawclk(struct drm_device *dev);
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
@@ -1102,6 +1103,8 @@ extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_connector *connector);
 void intel_panel_setup_panel_power_sequencer(
 				struct intel_connector *connector);
+void intel_panel_set_pps_registers(struct intel_connector *connector,
+				enum port port);
 void intel_panel_init_pps_funcs(struct drm_device *dev);
 
 /* intel_runtime_pm.c */
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 9b124ef..75172ab 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1346,6 +1346,76 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
 	}
 }
 
+void
+intel_panel_set_pps_registers(struct intel_connector *connector,
+				enum port port)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_encoder *encoder = connector->base.encoder;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	struct intel_panel *panel = &connector->panel;
+	u32 pp_on, pp_off, pp_div, port_sel = 0;
+	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
+	int pp_on_reg, pp_off_reg, pp_div_reg;
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	if (HAS_PCH_SPLIT(dev)) {
+		pp_on_reg = PCH_PP_ON_DELAYS;
+		pp_off_reg = PCH_PP_OFF_DELAYS;
+		pp_div_reg = PCH_PP_DIVISOR;
+	} else {
+		enum pipe pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
+
+		pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
+		pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
+		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
+	}
+
+	/*
+	 * And finally store the new values in the power sequencer. The
+	 * backlight delays are set to 1 because we do manual waits on them. For
+	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
+	 * we'll end up waiting for the backlight off delay twice: once when we
+	 * do the manual sleep, and once when we disable the panel and wait for
+	 * the PP_STATUS bit to become zero.
+	 */
+	pp_on = (panel->pps.panel_power_up_delay <<
+			PANEL_POWER_UP_DELAY_SHIFT) |
+		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
+	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
+		(panel->pps.panel_power_down_delay <<
+		 PANEL_POWER_DOWN_DELAY_SHIFT);
+	/* Compute the divisor for the pp clock, simply match the Bspec
+	 * formula. */
+	pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
+	pp_div |= (DIV_ROUND_UP(panel->pps.panel_power_cycle_delay, 1000)
+			<< PANEL_POWER_CYCLE_DELAY_SHIFT);
+
+	/* Haswell doesn't have any port selection bits for the panel
+	 * power sequencer any more. */
+	if (IS_VALLEYVIEW(dev)) {
+		port_sel = PANEL_PORT_SELECT_VLV(port);
+	} else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
+		if (port == PORT_A)
+			port_sel = PANEL_PORT_SELECT_DPA;
+		else
+			port_sel = PANEL_PORT_SELECT_DPD;
+	}
+
+	pp_on |= port_sel;
+
+	I915_WRITE(pp_on_reg, pp_on);
+	I915_WRITE(pp_off_reg, pp_off);
+	I915_WRITE(pp_div_reg, pp_div);
+
+	DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n",
+		      I915_READ(pp_on_reg),
+		      I915_READ(pp_off_reg),
+		      I915_READ(pp_div_reg));
+}
+
 static struct edp_power_seq pch_get_pps_registers(
 				struct intel_connector *connector,
 				u32 pp_ctrl_reg, u32 pp_on_reg,
-- 
2.0.1

Re: [RFC 4/7] drm/i915: Program PPS registers

[Daniel Vetter]

On Mon, Oct 20, 2014 at 06:20:06PM +0530, Vandana Kannan wrote:
> Actually set values into PPS related registers. This implementation is
> equivalent to intel_dp_panel_power_sequencer_registers where the values
> saved intially are written into registers.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c    | 80 ++------------------------------------
>  drivers/gpu/drm/i915/intel_drv.h   |  3 ++
>  drivers/gpu/drm/i915/intel_panel.c | 70 +++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a433c5f..ca11eb1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -248,7 +248,7 @@ unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes)
>  }
>  
>  /* hrawclock is 1/4 the FSB frequency */
> -static int
> +int
>  intel_hrawclk(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -281,11 +281,6 @@ intel_hrawclk(struct drm_device *dev)
>  	}
>  }
>  
> -static void
> -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> -					      struct intel_dp *intel_dp,
> -					      struct edp_power_seq *out);
> -
>  static void pps_lock(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -4716,76 +4711,6 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
>  	intel_dp->last_backlight_off = jiffies;
>  }
>  
> -static void
> -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> -					      struct intel_dp *intel_dp,
> -					      struct edp_power_seq *seq)

Hm, moving this function looks like it would be clearer as part of patch
1?

Otherwise I've done a (very) quick read-through of your series and on a
high level it looks sane I think. So please sign someone up for the
detailed review (and make sure that person is aware of that AR) so I can
merge this.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 4/7] drm/i915: Program PPS registers

[Daniel Vetter]

On Mon, Oct 20, 2014 at 06:08:48PM +0200, Daniel Vetter wrote:
> On Mon, Oct 20, 2014 at 06:20:06PM +0530, Vandana Kannan wrote:
> > Actually set values into PPS related registers. This implementation is
> > equivalent to intel_dp_panel_power_sequencer_registers where the values
> > saved intially are written into registers.
> > 
> > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c    | 80 ++------------------------------------
> >  drivers/gpu/drm/i915/intel_drv.h   |  3 ++
> >  drivers/gpu/drm/i915/intel_panel.c | 70 +++++++++++++++++++++++++++++++++
> >  3 files changed, 76 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index a433c5f..ca11eb1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -248,7 +248,7 @@ unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes)
> >  }
> >  
> >  /* hrawclock is 1/4 the FSB frequency */
> > -static int
> > +int
> >  intel_hrawclk(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -281,11 +281,6 @@ intel_hrawclk(struct drm_device *dev)
> >  	}
> >  }
> >  
> > -static void
> > -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > -					      struct intel_dp *intel_dp,
> > -					      struct edp_power_seq *out);
> > -
> >  static void pps_lock(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > @@ -4716,76 +4711,6 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> >  	intel_dp->last_backlight_off = jiffies;
> >  }
> >  
> > -static void
> > -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > -					      struct intel_dp *intel_dp,
> > -					      struct edp_power_seq *seq)
> 
> Hm, moving this function looks like it would be clearer as part of patch
> 1?

I've meant patch 2 of course, mixed things up.
-Daniel

> 
> Otherwise I've done a (very) quick read-through of your series and on a
> high level it looks sane I think. So please sign someone up for the
> detailed review (and make sure that person is aware of that AR) so I can
> merge this.
> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 4/7] drm/i915: Program PPS registers

[Ville Syrjälä]

On Mon, Oct 20, 2014 at 06:08:48PM +0200, Daniel Vetter wrote:
> On Mon, Oct 20, 2014 at 06:20:06PM +0530, Vandana Kannan wrote:
> > Actually set values into PPS related registers. This implementation is
> > equivalent to intel_dp_panel_power_sequencer_registers where the values
> > saved intially are written into registers.
> > 
> > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c    | 80 ++------------------------------------
> >  drivers/gpu/drm/i915/intel_drv.h   |  3 ++
> >  drivers/gpu/drm/i915/intel_panel.c | 70 +++++++++++++++++++++++++++++++++
> >  3 files changed, 76 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index a433c5f..ca11eb1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -248,7 +248,7 @@ unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes)
> >  }
> >  
> >  /* hrawclock is 1/4 the FSB frequency */
> > -static int
> > +int
> >  intel_hrawclk(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -281,11 +281,6 @@ intel_hrawclk(struct drm_device *dev)
> >  	}
> >  }
> >  
> > -static void
> > -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > -					      struct intel_dp *intel_dp,
> > -					      struct edp_power_seq *out);
> > -
> >  static void pps_lock(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > @@ -4716,76 +4711,6 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> >  	intel_dp->last_backlight_off = jiffies;
> >  }
> >  
> > -static void
> > -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > -					      struct intel_dp *intel_dp,
> > -					      struct edp_power_seq *seq)
> 
> Hm, moving this function looks like it would be clearer as part of patch
> 1?
> 
> Otherwise I've done a (very) quick read-through of your series and on a
> high level it looks sane I think. So please sign someone up for the
> detailed review (and make sure that person is aware of that AR) so I can
> merge this.

Would be nice if this series could be rebased on top of my VLV/CHV PPS
series since I'd rather not redo that thing a third time.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC 4/7] drm/i915: Program PPS registers

[Kannan, Vandana]

On 20-Oct-14 9:38 PM, Daniel Vetter wrote:
> On Mon, Oct 20, 2014 at 06:20:06PM +0530, Vandana Kannan wrote:
>> Actually set values into PPS related registers. This implementation is
>> equivalent to intel_dp_panel_power_sequencer_registers where the values
>> saved intially are written into registers.
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c    | 80 ++------------------------------------
>>   drivers/gpu/drm/i915/intel_drv.h   |  3 ++
>>   drivers/gpu/drm/i915/intel_panel.c | 70 +++++++++++++++++++++++++++++++++
>>   3 files changed, 76 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a433c5f..ca11eb1 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -248,7 +248,7 @@ unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes)
>>   }
>>
>>   /* hrawclock is 1/4 the FSB frequency */
>> -static int
>> +int
>>   intel_hrawclk(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -281,11 +281,6 @@ intel_hrawclk(struct drm_device *dev)
>>   	}
>>   }
>>
>> -static void
>> -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>> -					      struct intel_dp *intel_dp,
>> -					      struct edp_power_seq *out);
>> -
>>   static void pps_lock(struct intel_dp *intel_dp)
>>   {
>>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> @@ -4716,76 +4711,6 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
>>   	intel_dp->last_backlight_off = jiffies;
>>   }
>>
>> -static void
>> -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>> -					      struct intel_dp *intel_dp,
>> -					      struct edp_power_seq *seq)
>
> Hm, moving this function looks like it would be clearer as part of patch
> 1?
>
> Otherwise I've done a (very) quick read-through of your series and on a
> high level it looks sane I think. So please sign someone up for the
> detailed review (and make sure that person is aware of that AR) so I can
> merge this.
>
> Thanks, Daniel
>
Thanks Daniel..
I went through the LVDS and DSI parts. If software delays are used for 
both, would it really add more value to have the delays in a function in 
intel_panel.c and call whenever required ?
I'm thinking directly calling a msleep would be simpler but that would 
mean the PPS part wont be in one place in intel_panel.c.
Let me know what you think about this..

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

Re: [RFC 4/7] drm/i915: Program PPS registers

[Daniel Vetter]

On Mon, Oct 27, 2014 at 02:55:40PM +0530, Kannan, Vandana wrote:
> I went through the LVDS and DSI parts. If software delays are used for both,
> would it really add more value to have the delays in a function in
> intel_panel.c and call whenever required ?
> I'm thinking directly calling a msleep would be simpler but that would mean
> the PPS part wont be in one place in intel_panel.c.
> Let me know what you think about this..

Hm, I don't really follow what you mean? I think for now we can just
rework the pps for dp and leave things aside. Of course I'm not going to
stop you if you want to rework dsi/lvds too. But I'm not sure how valuable
that will be really. dsi maybe ...

Anyway the functions in dp have some niece tricks to only wait the
remaining time. So e.g. if the timeout is 500ms, but we've done the power
switch already 200ms ago then it will only wait the remaining 300ms. So if
you want to unify all this I think it would be best to use the existing
wait functions from dp, since they're more optimized.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 5/7] drm/i915: Split PPS reg write func based on platform

[Vandana Kannan]

The difference between vlv and other platforms is w.r.t registers and port
selection. Splitting the function to get value to be programmed based on 
platform and making the part which writes into registers common.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  4 ++
 drivers/gpu/drm/i915/intel_panel.c | 85 +++++++++++++++++++++++++-------------
 2 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1446d02..da0eede 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -511,6 +511,10 @@ struct drm_i915_display_funcs {
 
 	struct edp_power_seq (*setup_panel_power_seq)
 				(struct intel_connector *connector);
+	void (*set_pps_registers)(struct intel_connector *connector,
+			enum port port, int *pp_ctrl_reg,
+			int *pp_on_reg, int *pp_off_reg,
+			int *pp_div_reg, int *port_sel, int *div);
 };
 
 struct intel_uncore_funcs {
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 75172ab..c5e6c10 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1346,32 +1346,67 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
 	}
 }
 
-void
-intel_panel_set_pps_registers(struct intel_connector *connector,
-				enum port port)
+static void vlv_set_pps_registers(struct intel_connector *connector,
+		enum port port, int *pp_ctrl_reg,
+		int *pp_on_reg, int *pp_off_reg,
+		int *pp_div_reg, int *port_sel, int *div)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_encoder *encoder = connector->base.encoder;
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
-	struct intel_panel *panel = &connector->panel;
-	u32 pp_on, pp_off, pp_div, port_sel = 0;
-	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
-	int pp_on_reg, pp_off_reg, pp_div_reg;
+	enum pipe pipe = PIPE_A;
+
+	pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
-	if (HAS_PCH_SPLIT(dev)) {
-		pp_on_reg = PCH_PP_ON_DELAYS;
-		pp_off_reg = PCH_PP_OFF_DELAYS;
-		pp_div_reg = PCH_PP_DIVISOR;
-	} else {
-		enum pipe pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
+	*pp_ctrl_reg = 0;
+	*pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
+	*pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
+	*pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
+
+	*port_sel = PANEL_PORT_SELECT_VLV(port);
+	*div = intel_hrawclk(dev);
+}
 
-		pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
-		pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
-		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
+static void pch_set_pps_registers(struct intel_connector *connector,
+		enum port port, int *pp_ctrl_reg,
+		int *pp_on_reg, int *pp_off_reg,
+		int *pp_div_reg, int *port_sel, int *div)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	*pp_ctrl_reg = 0;
+	*pp_on_reg = PCH_PP_ON_DELAYS;
+	*pp_off_reg = PCH_PP_OFF_DELAYS;
+	*pp_div_reg = PCH_PP_DIVISOR;
+
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
+		if (port == PORT_A)
+			*port_sel = PANEL_PORT_SELECT_DPA;
+		else
+			*port_sel = PANEL_PORT_SELECT_DPD;
 	}
+	*div = intel_pch_rawclk(dev);
+}
+
+void
+intel_panel_set_pps_registers(struct intel_connector *connector,
+				enum port port)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_panel *panel = &connector->panel;
+	u32 pp_on, pp_off, pp_div, port_sel = 0, div;
+	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
+
+	dev_priv->display.set_pps_registers(connector, port, &pp_ctrl_reg,
+			&pp_on_reg, &pp_off_reg, &pp_div_reg,
+			&port_sel, &div);
 
 	/*
 	 * And finally store the new values in the power sequencer. The
@@ -1393,17 +1428,6 @@ intel_panel_set_pps_registers(struct intel_connector *connector,
 	pp_div |= (DIV_ROUND_UP(panel->pps.panel_power_cycle_delay, 1000)
 			<< PANEL_POWER_CYCLE_DELAY_SHIFT);
 
-	/* Haswell doesn't have any port selection bits for the panel
-	 * power sequencer any more. */
-	if (IS_VALLEYVIEW(dev)) {
-		port_sel = PANEL_PORT_SELECT_VLV(port);
-	} else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		if (port == PORT_A)
-			port_sel = PANEL_PORT_SELECT_DPA;
-		else
-			port_sel = PANEL_PORT_SELECT_DPD;
-	}
-
 	pp_on |= port_sel;
 
 	I915_WRITE(pp_on_reg, pp_on);
@@ -1561,10 +1585,13 @@ void intel_panel_init_pps_funcs(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.setup_panel_power_seq = vlv_setup_pps;
-	else
+		dev_priv->display.set_pps_registers = vlv_set_pps_registers;
+	} else {
 		dev_priv->display.setup_panel_power_seq = pch_setup_pps;
+		dev_priv->display.set_pps_registers = pch_set_pps_registers;
+	}
 }
 
 	
-- 
2.0.1

[RFC 6/7] drm/i915: Replace all refs to intel_dp delays

[Vandana Kannan]

Removing delays from intel_dp.
Replacing intel_dp PPS delays with intel_panel PPS delays.
This is part of removing all refs to PPS in intel_dp and moving it to
PPS in intel_panel.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 -----
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ca11eb1..bdb8317 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -497,6 +497,7 @@ static int edp_notify_handler(struct notifier_block *this, unsigned long code,
 						 edp_notifier);
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	u32 pp_div;
 	u32 pp_ctrl_reg, pp_div_reg;
 
@@ -516,7 +517,7 @@ static int edp_notify_handler(struct notifier_block *this, unsigned long code,
 		/* 0x1F write to PP_DIV_REG sets max cycle delay */
 		I915_WRITE(pp_div_reg, pp_div | 0x1F);
 		I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
-		msleep(intel_dp->panel_power_cycle_delay);
+		msleep(intel_connector->panel.pps.panel_power_cycle_delay);
 	}
 
 	pps_unlock(intel_dp);
@@ -1321,26 +1322,29 @@ static void wait_panel_off(struct intel_dp *intel_dp)
 
 static void wait_panel_power_cycle(struct intel_dp *intel_dp)
 {
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	DRM_DEBUG_KMS("Wait for panel power cycle\n");
 
 	/* When we disable the VDD override bit last we have to do the manual
 	 * wait. */
 	wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
-				       intel_dp->panel_power_cycle_delay);
+			intel_connector->panel.pps.panel_power_cycle_delay);
 
 	wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
 }
 
 static void wait_backlight_on(struct intel_dp *intel_dp)
 {
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
-				       intel_dp->backlight_on_delay);
+			intel_connector->panel.pps.backlight_on_delay);
 }
 
 static void edp_wait_backlight_off(struct intel_dp *intel_dp)
 {
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
-				       intel_dp->backlight_off_delay);
+			intel_connector->panel.pps.backlight_off_delay);
 }
 
 /* Read the current pp_control value, unlocking the register if it
@@ -1371,6 +1375,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum intel_display_power_domain power_domain;
 	u32 pp;
@@ -1410,7 +1415,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 	 */
 	if (!edp_have_panel_power(intel_dp)) {
 		DRM_DEBUG_KMS("eDP was not running\n");
-		msleep(intel_dp->panel_power_up_delay);
+		msleep(intel_connector->panel.pps.panel_power_up_delay);
 	}
 
 	return need_to_disable;
@@ -1490,6 +1495,7 @@ static void edp_panel_vdd_work(struct work_struct *__work)
 
 static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp)
 {
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	unsigned long delay;
 
 	/*
@@ -1497,7 +1503,8 @@ static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp)
 	 * down delay) to keep the panel power up across a sequence of
 	 * operations.
 	 */
-	delay = msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5);
+	delay = msecs_to_jiffies(
+		intel_connector->panel.pps.panel_power_cycle_delay * 5);
 	schedule_delayed_work(&intel_dp->panel_vdd_work, delay);
 }
 
@@ -3702,6 +3709,7 @@ static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	enum port port = intel_dig_port->port;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3760,7 +3768,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	DP &= ~DP_AUDIO_OUTPUT_ENABLE;
 	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
 	POSTING_READ(intel_dp->output_reg);
-	msleep(intel_dp->panel_power_down_delay);
+	msleep(intel_connector->panel.pps.panel_power_down_delay);
 }
 
 static bool
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4ae913c..f99cbc5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -581,11 +581,6 @@ struct intel_dp {
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
-	int panel_power_up_delay;
-	int panel_power_down_delay;
-	int panel_power_cycle_delay;
-	int backlight_on_delay;
-	int backlight_off_delay;
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
 	unsigned long last_power_cycle;
-- 
2.0.1

[RFC 7/7] drm/i915: Modify refs to intel dp timestamps

[Vandana Kannan]

Moving timestamp values to intel_panel as part of moving all refs of PPS to
intel_panel.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c    | 29 ++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h   |  7 ++++---
 drivers/gpu/drm/i915/intel_panel.c |  9 +++++++++
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bdb8317..96296a4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1327,7 +1327,8 @@ static void wait_panel_power_cycle(struct intel_dp *intel_dp)
 
 	/* When we disable the VDD override bit last we have to do the manual
 	 * wait. */
-	wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
+	wait_remaining_ms_from_jiffies(
+			intel_connector->panel.pps.last_power_cycle,
 			intel_connector->panel.pps.panel_power_cycle_delay);
 
 	wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
@@ -1336,14 +1337,16 @@ static void wait_panel_power_cycle(struct intel_dp *intel_dp)
 static void wait_backlight_on(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
-	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
+	wait_remaining_ms_from_jiffies(
+			intel_connector->panel.pps.last_power_on,
 			intel_connector->panel.pps.backlight_on_delay);
 }
 
 static void edp_wait_backlight_off(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
-	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
+	wait_remaining_ms_from_jiffies(
+			intel_connector->panel.pps.last_backlight_off,
 			intel_connector->panel.pps.backlight_off_delay);
 }
 
@@ -1449,6 +1452,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port =
 		dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
@@ -1476,7 +1480,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 	I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
 
 	if ((pp & POWER_TARGET_ON) == 0)
-		intel_dp->last_power_cycle = jiffies;
+		intel_connector->panel.pps.last_power_cycle = jiffies;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_put(dev_priv, power_domain);
@@ -1553,6 +1557,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	u32 pp;
 	u32 pp_ctrl_reg;
 
@@ -1587,7 +1592,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
 	POSTING_READ(pp_ctrl_reg);
 
 	wait_panel_on(intel_dp);
-	intel_dp->last_power_on = jiffies;
+	intel_connector->panel.pps.last_power_on = jiffies;
 
 	if (IS_GEN5(dev)) {
 		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
@@ -1605,6 +1610,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_ctrl_reg;
@@ -1631,7 +1637,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
 
-	intel_dp->last_power_cycle = jiffies;
+	intel_connector->panel.pps.last_power_cycle = jiffies;
 	wait_panel_off(intel_dp);
 
 	/* We got a reference when we enabled the VDD. */
@@ -1688,6 +1694,7 @@ static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	u32 pp;
 	u32 pp_ctrl_reg;
 
@@ -1706,7 +1713,7 @@ static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
 
 	pps_unlock(intel_dp);
 
-	intel_dp->last_backlight_off = jiffies;
+	intel_connector->panel.pps.last_backlight_off = jiffies;
 	edp_wait_backlight_off(intel_dp);
 }
 
@@ -4712,13 +4719,6 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	}
 }
 
-static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
-{
-	intel_dp->last_power_cycle = jiffies;
-	intel_dp->last_power_on = jiffies;
-	intel_dp->last_backlight_off = jiffies;
-}
-
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4920,7 +4920,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* We now know it's not a ghost, init power sequence regs. */
 	pps_lock(intel_dp);
-	intel_dp_init_panel_power_timestamps(intel_dp);
 	intel_panel_setup_panel_power_sequencer(intel_connector);
 	intel_panel_set_pps_registers(intel_connector, port);
 	pps_unlock(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f99cbc5..e6e2925 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -194,6 +194,10 @@ struct intel_panel {
 		int panel_power_cycle_delay;
 		int backlight_on_delay;
 		int backlight_off_delay;
+		/* timestamps */
+		unsigned long last_power_cycle;
+		unsigned long last_power_on;
+		unsigned long last_backlight_off;
 	} pps;
 };
 
@@ -583,9 +587,6 @@ struct intel_dp {
 	uint8_t train_set[4];
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
-	unsigned long last_power_cycle;
-	unsigned long last_power_on;
-	unsigned long last_backlight_off;
 
 	struct notifier_block edp_notifier;
 
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c5e6c10..e6a72dd 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1522,6 +1522,13 @@ static struct edp_power_seq vlv_setup_pps(struct intel_connector *connector)
 			pp_off_reg, pp_div_reg);
 }
 
+static void intel_panel_init_pps_timestamps(struct intel_panel *panel)
+{
+	panel->pps.last_power_cycle = jiffies;
+	panel->pps.last_power_on = jiffies;
+	panel->pps.last_backlight_off = jiffies;
+}
+
 void
 intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
 {
@@ -1530,6 +1537,8 @@ intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_power_seq cur, vbt, spec, final;
 
+	intel_panel_init_pps_timestamps(panel);
+
 	/* Get chip specific register values */
 	cur = dev_priv->display.setup_panel_power_seq(connector);
 
-- 
2.0.1

Re: [RFC 7/7] drm/i915: Modify refs to intel dp timestamps

[Daniel Vetter]

On Mon, Oct 20, 2014 at 06:20:09PM +0530, Vandana Kannan wrote:
> Moving timestamp values to intel_panel as part of moving all refs of PPS to
> intel_panel.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>

On second though, how does the code still work before applying patch 6&7?
Presuming I've not read this the wrong way round it looks like we only
init the new values, but the code still uses the old values. Which means
it's broken, which isn't ok.

I guess the patch 2 should be redone as two steps:
1. Move code to intel_panel.c, no actual code changes (code movement can't
   be reviewed really without completely redoing the patch, so code
   movement should never change a single line).
2. Do all the variable changes.
3. Do the refactoring like you have it already.

Of course you could also exchange steps 2/3.

But the code must work after every patch completely, that's a hard rule in
linux kernel development. Or maybe I completely missed something here.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c    | 29 ++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h   |  7 ++++---
>  drivers/gpu/drm/i915/intel_panel.c |  9 +++++++++
>  3 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bdb8317..96296a4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1327,7 +1327,8 @@ static void wait_panel_power_cycle(struct intel_dp *intel_dp)
>  
>  	/* When we disable the VDD override bit last we have to do the manual
>  	 * wait. */
> -	wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
> +	wait_remaining_ms_from_jiffies(
> +			intel_connector->panel.pps.last_power_cycle,
>  			intel_connector->panel.pps.panel_power_cycle_delay);
>  
>  	wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
> @@ -1336,14 +1337,16 @@ static void wait_panel_power_cycle(struct intel_dp *intel_dp)
>  static void wait_backlight_on(struct intel_dp *intel_dp)
>  {
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> -	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> +	wait_remaining_ms_from_jiffies(
> +			intel_connector->panel.pps.last_power_on,
>  			intel_connector->panel.pps.backlight_on_delay);
>  }
>  
>  static void edp_wait_backlight_off(struct intel_dp *intel_dp)
>  {
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> -	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> +	wait_remaining_ms_from_jiffies(
> +			intel_connector->panel.pps.last_backlight_off,
>  			intel_connector->panel.pps.backlight_off_delay);
>  }
>  
> @@ -1449,6 +1452,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  	struct intel_digital_port *intel_dig_port =
>  		dp_to_dig_port(intel_dp);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	enum intel_display_power_domain power_domain;
>  	u32 pp;
>  	u32 pp_stat_reg, pp_ctrl_reg;
> @@ -1476,7 +1480,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  	I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
>  
>  	if ((pp & POWER_TARGET_ON) == 0)
> -		intel_dp->last_power_cycle = jiffies;
> +		intel_connector->panel.pps.last_power_cycle = jiffies;
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_put(dev_priv, power_domain);
> @@ -1553,6 +1557,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	u32 pp;
>  	u32 pp_ctrl_reg;
>  
> @@ -1587,7 +1592,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
>  	POSTING_READ(pp_ctrl_reg);
>  
>  	wait_panel_on(intel_dp);
> -	intel_dp->last_power_on = jiffies;
> +	intel_connector->panel.pps.last_power_on = jiffies;
>  
>  	if (IS_GEN5(dev)) {
>  		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
> @@ -1605,6 +1610,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	enum intel_display_power_domain power_domain;
>  	u32 pp;
>  	u32 pp_ctrl_reg;
> @@ -1631,7 +1637,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
>  
> -	intel_dp->last_power_cycle = jiffies;
> +	intel_connector->panel.pps.last_power_cycle = jiffies;
>  	wait_panel_off(intel_dp);
>  
>  	/* We got a reference when we enabled the VDD. */
> @@ -1688,6 +1694,7 @@ static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	u32 pp;
>  	u32 pp_ctrl_reg;
>  
> @@ -1706,7 +1713,7 @@ static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
>  
>  	pps_unlock(intel_dp);
>  
> -	intel_dp->last_backlight_off = jiffies;
> +	intel_connector->panel.pps.last_backlight_off = jiffies;
>  	edp_wait_backlight_off(intel_dp);
>  }
>  
> @@ -4712,13 +4719,6 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>  	}
>  }
>  
> -static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> -{
> -	intel_dp->last_power_cycle = jiffies;
> -	intel_dp->last_power_on = jiffies;
> -	intel_dp->last_backlight_off = jiffies;
> -}
> -
>  void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4920,7 +4920,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  
>  	/* We now know it's not a ghost, init power sequence regs. */
>  	pps_lock(intel_dp);
> -	intel_dp_init_panel_power_timestamps(intel_dp);
>  	intel_panel_setup_panel_power_sequencer(intel_connector);
>  	intel_panel_set_pps_registers(intel_connector, port);
>  	pps_unlock(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f99cbc5..e6e2925 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -194,6 +194,10 @@ struct intel_panel {
>  		int panel_power_cycle_delay;
>  		int backlight_on_delay;
>  		int backlight_off_delay;
> +		/* timestamps */
> +		unsigned long last_power_cycle;
> +		unsigned long last_power_on;
> +		unsigned long last_backlight_off;
>  	} pps;
>  };
>  
> @@ -583,9 +587,6 @@ struct intel_dp {
>  	uint8_t train_set[4];
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> -	unsigned long last_power_cycle;
> -	unsigned long last_power_on;
> -	unsigned long last_backlight_off;
>  
>  	struct notifier_block edp_notifier;
>  
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index c5e6c10..e6a72dd 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1522,6 +1522,13 @@ static struct edp_power_seq vlv_setup_pps(struct intel_connector *connector)
>  			pp_off_reg, pp_div_reg);
>  }
>  
> +static void intel_panel_init_pps_timestamps(struct intel_panel *panel)
> +{
> +	panel->pps.last_power_cycle = jiffies;
> +	panel->pps.last_power_on = jiffies;
> +	panel->pps.last_backlight_off = jiffies;
> +}
> +
>  void
>  intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
>  {
> @@ -1530,6 +1537,8 @@ intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct edp_power_seq cur, vbt, spec, final;
>  
> +	intel_panel_init_pps_timestamps(panel);
> +
>  	/* Get chip specific register values */
>  	cur = dev_priv->display.setup_panel_power_seq(connector);
>  
> -- 
> 2.0.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch