[RFC 0/6] Rearranging PPS related code

[RFC 0/6] 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. To accomodate software delays where applicable, placeholders have been 
created in i915_display_funcs, but have not been defined yet.
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. 

Vandana Kannan (6):
  drm/i915: Create PPS related struct and func pointers
  drm/i915: Define PPS setup functions
  drm/i915: Program PPS registers
  drm/i915: Removing refs to intel_dp_panel_power_sequencer
  drm/i915: Replace all refs to intel_dp delays
  drm/i915: Modify refs to intel dp timestamps

 drivers/gpu/drm/i915/i915_drv.h      |  15 ++
 drivers/gpu/drm/i915/intel_display.c |   1 +
 drivers/gpu/drm/i915/intel_dp.c      | 275 ++++++-----------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  32 +++-
 drivers/gpu/drm/i915/intel_panel.c   | 260 +++++++++++++++++++++++++++++++++
 5 files changed, 346 insertions(+), 237 deletions(-)

-- 
2.0.1

[RFC 1/6] drm/i915: Create PPS related struct and func pointers

[Vandana Kannan]

Creating generic functions and data related to panel power sequencing in
intel_panel. The functions have been added equivalent to
intel_dp_panel_power_sequencer() and intel_dp_panel_power_sequencer_registers()

These changes have been made to move PPS related data from intel_dp or
other panel types to generic intel_panel.

Note:- Although edp_power_seq is used as a return type in the functions,
the structure can be made generic for all panels.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 15 +++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  9 +++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ed6cf10..295c724 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -508,6 +508,21 @@ 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);
+	void (*set_pps_panel_on)(struct intel_connector *connector);
+	void (*set_pps_panel_off)(struct intel_connector *connector);
+	void (*set_pps_backlight_on)(struct intel_connector *connector);
+	void (*set_pps_backlight_off)(struct intel_connector *connector);
+	struct edp_power_seq (*get_pps_registers)(
+				struct intel_connector *connector,
+				u32 pp_ctrl_reg, u32 pp_on_reg,
+				u32 pp_off_reg, u32 pp_div_reg);
+	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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 072e69f..ddf839d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -183,6 +183,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 {
-- 
2.0.1

[RFC 2/6] drm/i915: Define PPS setup functions

[Vandana Kannan]

Defining functions equivalent to intel_dp_panel_power_sequencer.
In the setup part, the differnce between vlv and other platforms is only
w.r.t registers. Other parts like reading VBT are common.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   1 +
 drivers/gpu/drm/i915/intel_dp.c      |  31 ++-----
 drivers/gpu/drm/i915/intel_drv.h     |   8 ++
 drivers/gpu/drm/i915/intel_panel.c   | 153 +++++++++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8488a8..22529b9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12632,6 +12632,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 9113497..861b634 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);
@@ -419,13 +419,12 @@ vlv_initial_pps_pipe(struct drm_i915_private *dev_priv,
 	return INVALID_PIPE;
 }
 
-static void
-vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
+void
+vlv_initial_power_sequencer_setup(struct intel_digital_port *intel_dig_port)
 {
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct edp_power_seq power_seq;
 	enum port port = intel_dig_port->port;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
@@ -452,10 +451,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_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
 }
 
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv)
@@ -1366,7 +1361,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;
@@ -5034,9 +5029,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		return false;
 	}
 
-	/* We now know it's not a ghost, init power sequence regs. */
 	pps_lock(intel_dp);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
+	intel_dp_init_panel_power_timestamps(intel_dp);
+	intel_panel_setup_panel_power_sequencer(intel_connector);
 	pps_unlock(intel_dp);
 
 	mutex_lock(&dev->mode_config.mutex);
@@ -5176,18 +5171,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 ddf839d..8f4332e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -989,7 +989,12 @@ 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_initial_power_sequencer_setup(
+				struct intel_digital_port *intel_dig_port);
 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);
@@ -1089,6 +1094,9 @@ 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);
+void intel_panel_init_pps_funcs(struct drm_device *dev);
 
 /* intel_pm.c */
 void intel_init_clock_gating(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 543e0f1..cfb6e9d 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1344,6 +1344,159 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
 	}
 }
 
+
+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 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;
+	u32 pp_on, pp_off, pp_div, pp;
+
+	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);
+
+	/* 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);
+
+	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);
+
+	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 drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_panel *panel = &connector->panel;
+	struct edp_power_seq cur, spec, vbt, final;
+
+	/* Get chip specific register values */
+	cur = dev_priv->display.setup_panel_power_seq(connector);
+
+	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);
+
+}
+
+/* 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 3/6] 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    |   4 +-
 drivers/gpu/drm/i915/intel_drv.h   |   3 ++
 drivers/gpu/drm/i915/intel_panel.c | 102 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 861b634..a40d494 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;
@@ -5005,6 +5005,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;
 
@@ -5032,6 +5033,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_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 8f4332e..687d581 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -994,6 +994,7 @@ void vlv_initial_power_sequencer_setup(
 				struct intel_digital_port *intel_dig_port);
 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);
@@ -1096,6 +1097,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_pm.c */
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cfb6e9d..9f7cb7a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1486,15 +1486,113 @@ void intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
 
 }
 
+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);
+	enum pipe pipe = PIPE_A;
+
+	pipe = vlv_power_sequencer_pipe(&intel_dig_port->dp);
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	*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);
+}
+
+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;
+	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
+	u32 pp_on, pp_off, pp_div;
+	u32 port_sel = 0, div;
+
+	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
+	 * 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);
+
+	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));
+
+}
+
 /* 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))
+	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;
+	}
 }
 
 int intel_panel_init(struct intel_panel *panel,
-- 
2.0.1

[RFC 4/6] drm/i915: Removing refs to intel_dp_panel_power_sequencer

[Vandana Kannan]

intel_dp_panel_power_sequencer and intel_dp_panel_power_sequence_registers
have been replaced by equivalent code in intel_panel.c
Making changes to remove these functions and refs to it.

vlv_power_sequencer_pipe() should return only pipe. panel power
sequencer functions need not be called from here ?

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 189 +---------------------------------------
 1 file changed, 2 insertions(+), 187 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a40d494..79ccf5a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -281,15 +281,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);
-
 static void pps_lock(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -330,7 +321,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 +357,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;
 }
 
@@ -2590,10 +2575,10 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	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);
 
@@ -2621,9 +2606,7 @@ static void vlv_init_panel_power_sequencer(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);
+	intel_panel_set_pps_registers(intel_connector, intel_dig_port->port);
 }
 
 static void vlv_pre_enable_dp(struct intel_encoder *encoder)
@@ -4663,174 +4646,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(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;
-	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 = (seq->t1_t3 << 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);
-	/* 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)
-			<< 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;
-- 
2.0.1

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

[Vandana Kannan]

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 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 79ccf5a..36c6b3b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -496,6 +496,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;
 
@@ -515,7 +516,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);
@@ -1320,26 +1321,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
@@ -1370,6 +1374,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;
@@ -1409,7 +1414,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;
@@ -1489,6 +1494,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;
 
 	/*
@@ -1496,7 +1502,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);
 }
 
@@ -3659,6 +3666,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;
@@ -3717,7 +3725,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
-- 
2.0.1

[RFC 6/6] 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    | 31 ++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h   | 12 ++++++++----
 drivers/gpu/drm/i915/intel_panel.c |  9 +++++++++
 3 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 36c6b3b..4959c86 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1326,7 +1326,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);
@@ -1335,14 +1336,18 @@ 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);
 }
 
@@ -1448,6 +1453,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;
@@ -1475,7 +1481,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);
@@ -1552,6 +1558,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;
 
@@ -1586,7 +1593,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 */
@@ -1602,6 +1609,7 @@ void intel_edp_panel_off(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;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum intel_display_power_domain power_domain;
@@ -1630,7 +1638,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. */
@@ -1687,6 +1695,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;
 
@@ -1705,7 +1714,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);
 }
 
@@ -4647,13 +4656,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;
@@ -4854,7 +4856,6 @@ 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_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 687d581..1718510 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -191,6 +191,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;
 };
 
@@ -578,16 +582,16 @@ 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_up_delay;
 	int panel_power_down_delay;
 	int panel_power_cycle_delay;
 	int backlight_on_delay;
-	int backlight_off_delay;
+	int backlight_off_delay;*/
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
-	unsigned long last_power_cycle;
+	/*unsigned long last_power_cycle;
 	unsigned long last_power_on;
-	unsigned long last_backlight_off;
+	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 9f7cb7a..6104273 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1427,6 +1427,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)
 {
 	struct drm_device *dev = connector->base.dev;
@@ -1434,6 +1441,8 @@ void intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
 	struct intel_panel *panel = &connector->panel;
 	struct edp_power_seq cur, spec, vbt, 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 0/6] Rearranging PPS related code

[Daniel Vetter]

On Tue, Oct 07, 2014 at 08:39:38PM +0530, Vandana Kannan wrote:
> 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. To accomodate software delays where applicable, placeholders have been
> created in i915_display_funcs, but have not been defined yet.
> 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.

I haven't taken a  in-depth look at your patches since I'm travelling.
But one thing I've noticed while scrolling through them is that you
first add most of the new code in a few patches, and then remove the
old one in follow-up patches. This makes reviewing patches a lot
harder.

A better way to split refactoring patch series is to do the split
per-function. So for this series here maybe have a patch for the pps
init changes, the functions to enable/disable power, and so on. And
the important part is that you add/remove/change the code in one patch
for a given function so that the actual change can be reviewed. So the
new vtable functions should grow out of the existing code.

And if you need to split code this should always be done in 2 steps:
First make a verbatim copy with the new names, then refactor both
copies to be platform specific. Of course if the code you copy is just
a few lines that can be done in one step, but as soon as you can't
read both functions completely in the diff context you should do the
split. Jani has definitely overstretched the limit with his bachlight
patches, but occasionally I let things slip through ;-)

Yours, Daniel

>
> Vandana Kannan (6):
>   drm/i915: Create PPS related struct and func pointers
>   drm/i915: Define PPS setup functions
>   drm/i915: Program PPS registers
>   drm/i915: Removing refs to intel_dp_panel_power_sequencer
>   drm/i915: Replace all refs to intel_dp delays
>   drm/i915: Modify refs to intel dp timestamps
>
>  drivers/gpu/drm/i915/i915_drv.h      |  15 ++
>  drivers/gpu/drm/i915/intel_display.c |   1 +
>  drivers/gpu/drm/i915/intel_dp.c      | 275 ++++++-----------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  32 +++-
>  drivers/gpu/drm/i915/intel_panel.c   | 260 +++++++++++++++++++++++++++++++++
>  5 files changed, 346 insertions(+), 237 deletions(-)
>
> --
> 2.0.1
>

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

Re: [RFC 0/6] Rearranging PPS related code

[Kannan, Vandana]

On 09-Oct-14 9:17 PM, Daniel Vetter wrote:
> On Tue, Oct 07, 2014 at 08:39:38PM +0530, Vandana Kannan wrote:
>> 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. To accomodate software delays where applicable, placeholders have been
>> created in i915_display_funcs, but have not been defined yet.
>> 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.
>
> I haven't taken a  in-depth look at your patches since I'm travelling.
> But one thing I've noticed while scrolling through them is that you
> first add most of the new code in a few patches, and then remove the
> old one in follow-up patches. This makes reviewing patches a lot
> harder.
>
> A better way to split refactoring patch series is to do the split
> per-function. So for this series here maybe have a patch for the pps
> init changes, the functions to enable/disable power, and so on. And
> the important part is that you add/remove/change the code in one patch
> for a given function so that the actual change can be reviewed. So the
> new vtable functions should grow out of the existing code.
>
> And if you need to split code this should always be done in 2 steps:
> First make a verbatim copy with the new names, then refactor both
> copies to be platform specific. Of course if the code you copy is just
> a few lines that can be done in one step, but as soon as you can't
> read both functions completely in the diff context you should do the
> split. Jani has definitely overstretched the limit with his bachlight
> patches, but occasionally I let things slip through ;-)
>
> Yours, Daniel
>
Hi Daniel,

Thanks for your inputs.
Agree with your comments, will make changes to the patches accordingly 
and resend.

Thanks,
Vandana
>>
>> Vandana Kannan (6):
>>    drm/i915: Create PPS related struct and func pointers
>>    drm/i915: Define PPS setup functions
>>    drm/i915: Program PPS registers
>>    drm/i915: Removing refs to intel_dp_panel_power_sequencer
>>    drm/i915: Replace all refs to intel_dp delays
>>    drm/i915: Modify refs to intel dp timestamps
>>
>>   drivers/gpu/drm/i915/i915_drv.h      |  15 ++
>>   drivers/gpu/drm/i915/intel_display.c |   1 +
>>   drivers/gpu/drm/i915/intel_dp.c      | 275 ++++++-----------------------------
>>   drivers/gpu/drm/i915/intel_drv.h     |  32 +++-
>>   drivers/gpu/drm/i915/intel_panel.c   | 260 +++++++++++++++++++++++++++++++++
>>   5 files changed, 346 insertions(+), 237 deletions(-)
>>
>> --
>> 2.0.1
>>
>