[PATCH 0/3] drm/i915: vlv pre_enable/enable callback refactoring

[PATCH 0/3] drm/i915: vlv pre_enable/enable callback refactoring

[Jani Nikula]

Address Daniel's old complaints about ->pre_enable and ->enable called
in the same spot in valleyview_crtc_enable [1].

Rebased on top of Chris' yet-to-be-merged locking patch [2].

Cheers,
Jani.


[1] http://mid.gmane.org/CAKMK7uFs9EMvMW8BnS24e5UNm1D7JrfVg3SD5SDFtVEamGfOOg@mail.gmail.com

[2] http://mid.gmane.org/1374865055-17919-1-git-send-email-chris@chris-wilson.co.uk


Jani Nikula (3):
  drm/i915: rearrange vlv dp enable and pre_enable callbacks
  drm/i915: rearrange vlv hdmi enable and pre_enable callbacks
  drm/i915: move encoder->enable callback later in VLV crtc enable

 drivers/gpu/drm/i915/intel_display.c |    7 ++--
 drivers/gpu/drm/i915/intel_dp.c      |   73 ++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_hdmi.c    |   20 +++++-----
 3 files changed, 53 insertions(+), 47 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] drm/i915: rearrange vlv dp enable and pre_enable callbacks

[Jani Nikula]

Currently ->pre_enable and ->enable are called back to back. Rearrange
the DP callbacks to make it possible to move ->enable call later.

Basically do everything in ->pre_enable on VLV, and make ->enable a NOP.

There should be no functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   73 +++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 928b42f..c6b38a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1691,49 +1691,50 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	intel_dp_complete_link_train(intel_dp);
 	intel_dp_stop_link_train(intel_dp);
 	ironlake_edp_backlight_on(intel_dp);
+}
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_digital_port *dport =
-			enc_to_dig_port(&encoder->base);
-		int channel = vlv_dport_to_channel(dport);
-
-		vlv_wait_port_ready(dev_priv, channel);
-	}
+static void vlv_enable_dp(struct intel_encoder *encoder)
+{
 }
 
 static void intel_pre_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
+
+	if (dport->port == PORT_A)
+		ironlake_edp_pll_on(intel_dp);
+}
+
+static void vlv_pre_enable_dp(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	int port = vlv_dport_to_channel(dport);
+	int pipe = intel_crtc->pipe;
+	u32 val;
 
-	if (dport->port == PORT_A && !IS_VALLEYVIEW(dev))
-		ironlake_edp_pll_on(intel_dp);
+	mutex_lock(&dev_priv->dpio_lock);
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_crtc *intel_crtc =
-			to_intel_crtc(encoder->base.crtc);
-		int port = vlv_dport_to_channel(dport);
-		int pipe = intel_crtc->pipe;
-		u32 val;
-
-		mutex_lock(&dev_priv->dpio_lock);
-		val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
-		val = 0;
-		if (pipe)
-			val |= (1<<21);
-		else
-			val &= ~(1<<21);
-		val |= 0x001000c4;
-		vlv_dpio_write(dev_priv, DPIO_DATA_CHANNEL(port), val);
+	val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
+	val = 0;
+	if (pipe)
+		val |= (1<<21);
+	else
+		val &= ~(1<<21);
+	val |= 0x001000c4;
+	vlv_dpio_write(dev_priv, DPIO_DATA_CHANNEL(port), val);
+	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF0(port), 0x00760018);
+	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port), 0x00400888);
 
-		vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF0(port),
-				 0x00760018);
-		vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
-				 0x00400888);
-		mutex_unlock(&dev_priv->dpio_lock);
-	}
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	intel_enable_dp(encoder);
+
+	vlv_wait_port_ready(dev_priv, port);
 }
 
 static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
@@ -3513,14 +3514,18 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 
 	intel_encoder->compute_config = intel_dp_compute_config;
 	intel_encoder->mode_set = intel_dp_mode_set;
-	intel_encoder->enable = intel_enable_dp;
-	intel_encoder->pre_enable = intel_pre_enable_dp;
 	intel_encoder->disable = intel_disable_dp;
 	intel_encoder->post_disable = intel_post_disable_dp;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
 	intel_encoder->get_config = intel_dp_get_config;
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = intel_dp_pre_pll_enable;
+		intel_encoder->pre_enable = vlv_pre_enable_dp;
+		intel_encoder->enable = vlv_enable_dp;
+	} else {
+		intel_encoder->pre_enable = intel_pre_enable_dp;
+		intel_encoder->enable = intel_enable_dp;
+	}
 
 	intel_dig_port->port = port;
 	intel_dig_port->dp.output_reg = output_reg;
-- 
1.7.10.4

Re: [PATCH 1/3] drm/i915: rearrange vlv dp enable and pre_enable callbacks

[Chris Wilson]

On Tue, Jul 30, 2013 at 11:08:28AM +0300, Jani Nikula wrote:
> Currently ->pre_enable and ->enable are called back to back. Rearrange
> the DP callbacks to make it possible to move ->enable call later.

A one sentence explanation why we want ->enable later would be perfect.
If you can concisely explain why the code was originally written as is,
and why we want to do it differently in the near future, that would be
ideal.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: rearrange vlv dp enable and pre_enable callbacks

[Jani Nikula]

On Tue, 30 Jul 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Jul 30, 2013 at 11:08:28AM +0300, Jani Nikula wrote:
>> Currently ->pre_enable and ->enable are called back to back. Rearrange
>> the DP callbacks to make it possible to move ->enable call later.
>
> A one sentence explanation why we want ->enable later would be perfect.
> If you can concisely explain why the code was originally written as is,
> and why we want to do it differently in the near future, that would be
> ideal.

Good point. How about,

for patches 1&2:

"""
VLV wants encoder enabling before the pipe is up. This is currently
achieved through calling the ->enable callback early, right after the
->pre_enable callback, in valleyview_crtc_enable(). This loses both the
distinction between ->pre_enable and ->enable on VLV and the possibility
to use a hook at the end of the modeset sequence.

Rearrange the DP callbacks to make it possible to move ->enable call
later. Basically do everything in ->pre_enable on VLV, and make ->enable
a NOP.

There should be no functional changes.
"""

for patch 3:

"""
VLV wants encoder enabling before the pipe is up. With the previously
rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
this no longer depends on the early ->enable hook call. Move the
->enable call at the end of the sequence, similar to the crtc enable on
other platforms. This will be needed e.g. for moving the eDP backlight
enabling to the right place in the sequence, currently done too early on
VLV.

There should be no functional changes.
"""

I'll send the revised patches once we agree on the wording.

Thanks,
Jani.

Re: [PATCH 1/3] drm/i915: rearrange vlv dp enable and pre_enable callbacks

[Chris Wilson]

On Tue, Jul 30, 2013 at 12:05:34PM +0300, Jani Nikula wrote:
> On Tue, 30 Jul 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, Jul 30, 2013 at 11:08:28AM +0300, Jani Nikula wrote:
> >> Currently ->pre_enable and ->enable are called back to back. Rearrange
> >> the DP callbacks to make it possible to move ->enable call later.
> >
> > A one sentence explanation why we want ->enable later would be perfect.
> > If you can concisely explain why the code was originally written as is,
> > and why we want to do it differently in the near future, that would be
> > ideal.
> 
> Good point. How about,

Thank you, that was the explanation I was looking for! :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/3] drm/i915: rearrange vlv hdmi enable and pre_enable callbacks

[Jani Nikula]

Currently ->pre_enable and ->enable are called back to back. Rearrange
the HDMI callbacks to make it possible to move ->enable call later.

Basically do everything in ->pre_enable on VLV, and make ->enable a NOP.

There should be no functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a2be6b2..68a27c2 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -718,14 +718,10 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
+}
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_digital_port *dport =
-			enc_to_dig_port(&encoder->base);
-		int channel = vlv_dport_to_channel(dport);
-
-		vlv_wait_port_ready(dev_priv, channel);
-	}
+static void vlv_enable_hdmi(struct intel_encoder *encoder)
+{
 }
 
 static void intel_disable_hdmi(struct intel_encoder *encoder)
@@ -1064,6 +1060,10 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
 	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
 			 0x00400888);
 	mutex_unlock(&dev_priv->dpio_lock);
+
+	intel_enable_hdmi(encoder);
+
+	vlv_wait_port_ready(dev_priv, port);
 }
 
 static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
@@ -1244,14 +1244,16 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 
 	intel_encoder->compute_config = intel_hdmi_compute_config;
 	intel_encoder->mode_set = intel_hdmi_mode_set;
-	intel_encoder->enable = intel_enable_hdmi;
 	intel_encoder->disable = intel_disable_hdmi;
 	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
 	intel_encoder->get_config = intel_hdmi_get_config;
 	if (IS_VALLEYVIEW(dev)) {
-		intel_encoder->pre_enable = intel_hdmi_pre_enable;
 		intel_encoder->pre_pll_enable = intel_hdmi_pre_pll_enable;
+		intel_encoder->pre_enable = intel_hdmi_pre_enable;
+		intel_encoder->enable = vlv_enable_hdmi;
 		intel_encoder->post_disable = intel_hdmi_post_disable;
+	} else {
+		intel_encoder->enable = intel_enable_hdmi;
 	}
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
-- 
1.7.10.4

[PATCH 3/3] drm/i915: move encoder->enable callback later in VLV crtc enable

[Jani Nikula]

Encoder ->pre_enable and ->enable callbacks were moved back to back in
VLV crtc enable sequence, which is not very useful. Move ->enable call
at the end of the sequence.

With the previously rearranged VLV DP and HDMI ->pre_enable and ->enable
callbacks in place, this should not cause any functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7d63d8d..c3c0bf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3663,10 +3663,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
 
-	/* VLV wants encoder enabling _before_ the pipe is up. */
-	for_each_encoder_on_crtc(dev, crtc, encoder)
-		encoder->enable(encoder);
-
 	i9xx_pfit_enable(intel_crtc);
 
 	intel_crtc_load_lut(crtc);
@@ -3677,6 +3673,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_update_cursor(crtc, true);
 
 	intel_update_fbc(dev);
+
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		encoder->enable(encoder);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
-- 
1.7.10.4

[PATCH v3 1/3] drm/i915: rearrange vlv dp enable and pre_enable callbacks

[Jani Nikula]

VLV wants encoder enabling before the pipe is up. This is currently
achieved through calling the ->enable callback early, right after the
->pre_enable callback, in valleyview_crtc_enable(). This loses both the
distinction between ->pre_enable and ->enable on VLV and the possibility
to use a hook at the end of the modeset sequence.

Rearrange the DP callbacks to make it possible to move ->enable call
later. Basically do everything in ->pre_enable on VLV, and make ->enable
a NOP.

There should be no functional changes.

v2: Rebase.

v3: Explain why this is needed in the commit message (Chris).

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   73 +++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 928b42f..c6b38a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1691,49 +1691,50 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	intel_dp_complete_link_train(intel_dp);
 	intel_dp_stop_link_train(intel_dp);
 	ironlake_edp_backlight_on(intel_dp);
+}
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_digital_port *dport =
-			enc_to_dig_port(&encoder->base);
-		int channel = vlv_dport_to_channel(dport);
-
-		vlv_wait_port_ready(dev_priv, channel);
-	}
+static void vlv_enable_dp(struct intel_encoder *encoder)
+{
 }
 
 static void intel_pre_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
+
+	if (dport->port == PORT_A)
+		ironlake_edp_pll_on(intel_dp);
+}
+
+static void vlv_pre_enable_dp(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	int port = vlv_dport_to_channel(dport);
+	int pipe = intel_crtc->pipe;
+	u32 val;
 
-	if (dport->port == PORT_A && !IS_VALLEYVIEW(dev))
-		ironlake_edp_pll_on(intel_dp);
+	mutex_lock(&dev_priv->dpio_lock);
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_crtc *intel_crtc =
-			to_intel_crtc(encoder->base.crtc);
-		int port = vlv_dport_to_channel(dport);
-		int pipe = intel_crtc->pipe;
-		u32 val;
-
-		mutex_lock(&dev_priv->dpio_lock);
-		val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
-		val = 0;
-		if (pipe)
-			val |= (1<<21);
-		else
-			val &= ~(1<<21);
-		val |= 0x001000c4;
-		vlv_dpio_write(dev_priv, DPIO_DATA_CHANNEL(port), val);
+	val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
+	val = 0;
+	if (pipe)
+		val |= (1<<21);
+	else
+		val &= ~(1<<21);
+	val |= 0x001000c4;
+	vlv_dpio_write(dev_priv, DPIO_DATA_CHANNEL(port), val);
+	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF0(port), 0x00760018);
+	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port), 0x00400888);
 
-		vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF0(port),
-				 0x00760018);
-		vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
-				 0x00400888);
-		mutex_unlock(&dev_priv->dpio_lock);
-	}
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	intel_enable_dp(encoder);
+
+	vlv_wait_port_ready(dev_priv, port);
 }
 
 static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
@@ -3513,14 +3514,18 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 
 	intel_encoder->compute_config = intel_dp_compute_config;
 	intel_encoder->mode_set = intel_dp_mode_set;
-	intel_encoder->enable = intel_enable_dp;
-	intel_encoder->pre_enable = intel_pre_enable_dp;
 	intel_encoder->disable = intel_disable_dp;
 	intel_encoder->post_disable = intel_post_disable_dp;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
 	intel_encoder->get_config = intel_dp_get_config;
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = intel_dp_pre_pll_enable;
+		intel_encoder->pre_enable = vlv_pre_enable_dp;
+		intel_encoder->enable = vlv_enable_dp;
+	} else {
+		intel_encoder->pre_enable = intel_pre_enable_dp;
+		intel_encoder->enable = intel_enable_dp;
+	}
 
 	intel_dig_port->port = port;
 	intel_dig_port->dp.output_reg = output_reg;
-- 
1.7.10.4

[PATCH v3 2/3] drm/i915: rearrange vlv hdmi enable and pre_enable callbacks

[Jani Nikula]

VLV wants encoder enabling before the pipe is up. This is currently
achieved through calling the ->enable callback early, right after the
->pre_enable callback, in valleyview_crtc_enable(). This loses both the
distinction between ->pre_enable and ->enable on VLV and the possibility
to use a hook at the end of the modeset sequence.

Rearrange the HDMI callbacks to make it possible to move ->enable call
later. Basically do everything in ->pre_enable on VLV, and make ->enable
a NOP.

There should be no functional changes.

v2: Rebase.

v3: Explain why this is needed in the commit message (Chris).

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a2be6b2..68a27c2 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -718,14 +718,10 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
+}
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_digital_port *dport =
-			enc_to_dig_port(&encoder->base);
-		int channel = vlv_dport_to_channel(dport);
-
-		vlv_wait_port_ready(dev_priv, channel);
-	}
+static void vlv_enable_hdmi(struct intel_encoder *encoder)
+{
 }
 
 static void intel_disable_hdmi(struct intel_encoder *encoder)
@@ -1064,6 +1060,10 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
 	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
 			 0x00400888);
 	mutex_unlock(&dev_priv->dpio_lock);
+
+	intel_enable_hdmi(encoder);
+
+	vlv_wait_port_ready(dev_priv, port);
 }
 
 static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
@@ -1244,14 +1244,16 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 
 	intel_encoder->compute_config = intel_hdmi_compute_config;
 	intel_encoder->mode_set = intel_hdmi_mode_set;
-	intel_encoder->enable = intel_enable_hdmi;
 	intel_encoder->disable = intel_disable_hdmi;
 	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
 	intel_encoder->get_config = intel_hdmi_get_config;
 	if (IS_VALLEYVIEW(dev)) {
-		intel_encoder->pre_enable = intel_hdmi_pre_enable;
 		intel_encoder->pre_pll_enable = intel_hdmi_pre_pll_enable;
+		intel_encoder->pre_enable = intel_hdmi_pre_enable;
+		intel_encoder->enable = vlv_enable_hdmi;
 		intel_encoder->post_disable = intel_hdmi_post_disable;
+	} else {
+		intel_encoder->enable = intel_enable_hdmi;
 	}
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
-- 
1.7.10.4

[PATCH v3 3/3] drm/i915: move encoder->enable callback later in VLV crtc enable

[Jani Nikula]

VLV wants encoder enabling before the pipe is up. With the previously
rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
this no longer depends on the early ->enable hook call. Move the
->enable call at the end of the sequence, similar to the crtc enable on
other platforms. This will be needed e.g. for moving the eDP backlight
enabling to the right place in the sequence, currently done too early on
VLV.

There should be no functional changes.

v2: Rebase.

v3: Explain why this is needed in the commit message (Chris).

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7d63d8d..c3c0bf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3663,10 +3663,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
 
-	/* VLV wants encoder enabling _before_ the pipe is up. */
-	for_each_encoder_on_crtc(dev, crtc, encoder)
-		encoder->enable(encoder);
-
 	i9xx_pfit_enable(intel_crtc);
 
 	intel_crtc_load_lut(crtc);
@@ -3677,6 +3673,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_update_cursor(crtc, true);
 
 	intel_update_fbc(dev);
+
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		encoder->enable(encoder);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
-- 
1.7.10.4

Re: [PATCH v3 3/3] drm/i915: move encoder->enable callback later in VLV crtc enable

[Chris Wilson]

On Tue, Jul 30, 2013 at 12:20:32PM +0300, Jani Nikula wrote:
> VLV wants encoder enabling before the pipe is up. With the previously
> rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
> this no longer depends on the early ->enable hook call. Move the
> ->enable call at the end of the sequence, similar to the crtc enable on
> other platforms. This will be needed e.g. for moving the eDP backlight
> enabling to the right place in the sequence, currently done too early on
> VLV.
> 
> There should be no functional changes.
> 
> v2: Rebase.
> 
> v3: Explain why this is needed in the commit message (Chris).
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

I couldn't spot any functional changes in the 3 patches, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 3/3] drm/i915: move encoder->enable callback later in VLV crtc enable

[Daniel Vetter]

On Tue, Jul 30, 2013 at 11:58:10AM +0100, Chris Wilson wrote:
> On Tue, Jul 30, 2013 at 12:20:32PM +0300, Jani Nikula wrote:
> > VLV wants encoder enabling before the pipe is up. With the previously
> > rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
> > this no longer depends on the early ->enable hook call. Move the
> > ->enable call at the end of the sequence, similar to the crtc enable on
> > other platforms. This will be needed e.g. for moving the eDP backlight
> > enabling to the right place in the sequence, currently done too early on
> > VLV.
> > 
> > There should be no functional changes.
> > 
> > v2: Rebase.
> > 
> > v3: Explain why this is needed in the commit message (Chris).
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> I couldn't spot any functional changes in the 3 patches, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Series merged to dinq, thanks for the patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 3/3] drm/i915: move encoder->enable callback later in VLV crtc enable

[Ville Syrjälä]

On Tue, Jul 30, 2013 at 12:20:32PM +0300, Jani Nikula wrote:
> VLV wants encoder enabling before the pipe is up. With the previously
> rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
> this no longer depends on the early ->enable hook call. Move the
> ->enable call at the end of the sequence, similar to the crtc enable on
> other platforms. This will be needed e.g. for moving the eDP backlight
> enabling to the right place in the sequence, currently done too early on
> VLV.
> 
> There should be no functional changes.
> 
> v2: Rebase.
> 
> v3: Explain why this is needed in the commit message (Chris).
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Looks sane to me as well. For the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Note that I just lost my VLV board so I wasn't able to test this.

I'm assuming there will eventually be some followon patch to move the
backlight stuff to right place for VLV?

Also should we make encoder->enable optional to avoid the stubs for VLV?

And it might make sense to rename all the VLV specific dp/hdmi functions
to vlv_foo instead of the intel_foo that they are called now.

> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7d63d8d..c3c0bf1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3663,10 +3663,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  		if (encoder->pre_enable)
>  			encoder->pre_enable(encoder);
>  
> -	/* VLV wants encoder enabling _before_ the pipe is up. */
> -	for_each_encoder_on_crtc(dev, crtc, encoder)
> -		encoder->enable(encoder);
> -
>  	i9xx_pfit_enable(intel_crtc);
>  
>  	intel_crtc_load_lut(crtc);
> @@ -3677,6 +3673,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	intel_update_fbc(dev);
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		encoder->enable(encoder);
>  }
>  
>  static void i9xx_crtc_enable(struct drm_crtc *crtc)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/3] drm/i915: rearrange vlv dp enable and pre_enable callbacks

[Jani Nikula]

Currently ->pre_enable and ->enable are called back to back. Rearrange
the DP callbacks to make it possible to move ->enable call later.

Basically do everything in ->pre_enable on VLV, and make ->enable a NOP.

There should be no functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   67 ++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a899f93..543cb12 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1405,47 +1405,46 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	intel_dp_complete_link_train(intel_dp);
 	intel_dp_stop_link_train(intel_dp);
 	ironlake_edp_backlight_on(intel_dp);
+}
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_digital_port *dport =
-			enc_to_dig_port(&encoder->base);
-		int channel = vlv_dport_to_channel(dport);
-
-		vlv_wait_port_ready(dev_priv, channel);
-	}
+static void vlv_enable_dp(struct intel_encoder *encoder)
+{
 }
 
 static void intel_pre_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
+
+	if (dport->port == PORT_A)
+		ironlake_edp_pll_on(intel_dp);
+}
+
+static void vlv_pre_enable_dp(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	int port = vlv_dport_to_channel(dport);
+	int pipe = intel_crtc->pipe;
+	u32 val;
 
-	if (dport->port == PORT_A && !IS_VALLEYVIEW(dev))
-		ironlake_edp_pll_on(intel_dp);
+	val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
+	val = 0;
+	if (pipe)
+		val |= (1<<21);
+	else
+		val &= ~(1<<21);
+	val |= 0x001000c4;
+	vlv_dpio_write(dev_priv, DPIO_DATA_CHANNEL(port), val);
+	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF0(port), 0x00760018);
+	vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port), 0x00400888);
 
-	if (IS_VALLEYVIEW(dev)) {
-		struct intel_crtc *intel_crtc =
-			to_intel_crtc(encoder->base.crtc);
-		int port = vlv_dport_to_channel(dport);
-		int pipe = intel_crtc->pipe;
-		u32 val;
-
-		val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
-		val = 0;
-		if (pipe)
-			val |= (1<<21);
-		else
-			val &= ~(1<<21);
-		val |= 0x001000c4;
-		vlv_dpio_write(dev_priv, DPIO_DATA_CHANNEL(port), val);
+	intel_enable_dp(encoder);
 
-		vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF0(port),
-				 0x00760018);
-		vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
-				 0x00400888);
-	}
+	vlv_wait_port_ready(dev_priv, port);
 }
 
 static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
@@ -3187,14 +3186,18 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	drm_encoder_helper_add(&intel_encoder->base, &intel_dp_helper_funcs);
 
 	intel_encoder->compute_config = intel_dp_compute_config;
-	intel_encoder->enable = intel_enable_dp;
-	intel_encoder->pre_enable = intel_pre_enable_dp;
 	intel_encoder->disable = intel_disable_dp;
 	intel_encoder->post_disable = intel_post_disable_dp;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
 	intel_encoder->get_config = intel_dp_get_config;
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
+		intel_encoder->enable = vlv_enable_dp;
+		intel_encoder->pre_enable = vlv_pre_enable_dp;
 		intel_encoder->pre_pll_enable = intel_dp_pre_pll_enable;
+	} else {
+		intel_encoder->enable = intel_enable_dp;
+		intel_encoder->pre_enable = intel_pre_enable_dp;
+	}
 
 	intel_dig_port->port = port;
 	intel_dig_port->dp.output_reg = output_reg;
-- 
1.7.9.5