[PATCH 0/5] Panel power sequencing improvements

[PATCH 0/5] Panel power sequencing improvements

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

This series was already posted to the mailing list as part of the Runtime PM
series. Since the runtime PM code touches some eDP code, I originally made this
series on top of runtime PM because I was hoping runtime PM would be merged
earlier, so I wouldn't need to rebase this series.

It seems that these patches actually fix real bugs, so Jesse asked me to send
this series on top of -nightly so we could try to merge it earlier. Here it is.

Besides rebasing, the differences are on patch 4 and 5: they now should address
the review comments from Chris, Ben and Jani.

Looks like this series fixes freedesktop.org bug 69693, and it also helps us
getting closer to fix 72211.

Thanks,
Paulo

Paulo Zanoni (5):
  drm/i915: don't enable VDD just to enable the panel
  drm/i915: don't touch the VDD when disabling the panel
  drm/i915: fix VDD override off wait
  drm/i915: save some time when waiting the eDP timings
  drm/i915: init the DP panel power seq variables earlier

 drivers/gpu/drm/i915/intel_ddi.c     |  3 ---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 48 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  4 +++
 4 files changed, 55 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] drm/i915: don't enable VDD just to enable the panel

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We just don't need this. This saves 250ms from every modeset on my
machine.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index c8a5fb7..432ead5 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1122,9 +1122,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-		ironlake_edp_panel_vdd_on(intel_dp);
 		ironlake_edp_panel_on(intel_dp);
-		ironlake_edp_panel_vdd_off(intel_dp, true);
 	}
 
 	WARN_ON(intel_crtc->ddi_pll_sel == PORT_CLK_SEL_NONE);
-- 
1.8.3.1

[PATCH 2/5] drm/i915: don't touch the VDD when disabling the panel

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

I don't see a reason to touch VDD when we're disabling the panel:
since the panel is enabled, we don't need VDD. This saves a few sleep
calls from the vdd_on and vdd_off functions at every modeset.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69693
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 1 -
 drivers/gpu/drm/i915/intel_dp.c  | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 432ead5..b1b1d9d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1165,7 +1165,6 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-		ironlake_edp_panel_vdd_on(intel_dp);
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 		ironlake_edp_panel_off(intel_dp);
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bda2e1f..fe327ce 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1235,20 +1235,16 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("Turn eDP power off\n");
 
-	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
-
 	pp = ironlake_get_pp_control(intel_dp);
 	/* We need to switch off panel power _and_ force vdd, for otherwise some
 	 * panels get very unhappy and cease to work. */
-	pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
+	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_BLC_ENABLE);
 
 	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
 
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
 
-	intel_dp->want_panel_vdd = false;
-
 	ironlake_wait_panel_off(intel_dp);
 }
 
@@ -1774,7 +1770,6 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 
 	/* Make sure the panel is off before trying to change the mode. But also
 	 * ensure that we have vdd while we switch off the panel. */
-	ironlake_edp_panel_vdd_on(intel_dp);
 	ironlake_edp_backlight_off(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	ironlake_edp_panel_off(intel_dp);
-- 
1.8.3.1

[PATCH 3/5] drm/i915: fix VDD override off wait

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

If we're disabling the VDD override bit and the panel is enabled, we
don't need to wait for anything. If the panel is disabled, then we
need to actually wait for panel_power_cycle_delay, not
panel_power_down_delay, because the power down delay was already
respected when we disabled the panel.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fe327ce..a2aace2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1142,7 +1142,9 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		/* Make sure sequencer is idle before allowing subsequent activity */
 		DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
 		I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
-		msleep(intel_dp->panel_power_down_delay);
+
+		if ((pp & POWER_TARGET_ON) == 0)
+			msleep(intel_dp->panel_power_cycle_delay);
 	}
 }
 
-- 
1.8.3.1

[PATCH 4/5] drm/i915: save some time when waiting the eDP timings

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The eDP spec defines some points where after you do action A, you have
to wait some time before action B. The thing is that in our driver
action B does not happen exactly after action A, but we still use
msleep() calls directly. What this patch does is that we record the
timestamp of when action A happened, then, just before action B, we
look at how much time has passed and only sleep the remaining amount
needed.

With this change, I am able to save about 5-20ms (out of the total
200ms) of the backlight_off delay and completely skip the 1ms
backlight_on delay. The 600ms vdd_off delay doesn't happen during
normal usage anymore due to a previous patch.

v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and
      move it to intel_display.c
    - Fix the msleep call: diff is in jiffies
v3: - Use "tmp_jiffies" so we don't need to worry about the value of
      "jiffies" advancing while we're doing the math.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 26 +++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c59b67..0c238dd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -806,6 +806,24 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
 		DRM_DEBUG_KMS("vblank wait timed out\n");
 }
 
+/* If you need to wait X ms between events A and B, but event B doesn't happen
+ * exactly after event A, you record the timestamp (jiffies) of when event A
+ * happened, then just before event B you call intel_wait_until_after and pass
+ * the timestamp as the first argument, and X as the second argument. */
+void intel_wait_until_after(unsigned long timestamp, int to_wait_ms)
+{
+	unsigned long target = timestamp + msecs_to_jiffies(to_wait_ms);
+	unsigned long diff;
+	/* Don't re-read the value of "jiffies" every time since it may change
+	 * behind our back and break the math. */
+	unsigned long tmp_jiffies = jiffies;
+
+	if (time_after(target, tmp_jiffies)) {
+		diff = (long)target - (long)tmp_jiffies;
+		msleep(jiffies_to_msecs(diff));
+	}
+}
+
 static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a2aace2..79f7ec2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1056,9 +1056,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
 static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp)
 {
 	DRM_DEBUG_KMS("Wait for panel power cycle\n");
+
+	/* When we disable the VDD override bit last we have to do the manual
+	 * wait. */
+	intel_wait_until_after(intel_dp->last_power_cycle,
+			       intel_dp->panel_power_cycle_delay);
+
 	ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
 }
 
+static void ironlake_wait_backlight_on(struct intel_dp *intel_dp)
+{
+	intel_wait_until_after(intel_dp->last_power_on,
+			       intel_dp->backlight_on_delay);
+}
+
+static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
+{
+	intel_wait_until_after(intel_dp->last_backlight_off,
+			       intel_dp->backlight_off_delay);
+}
 
 /* Read the current pp_control value, unlocking the register if it
  * is locked
@@ -1144,7 +1161,7 @@ static void ironlake_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)
-			msleep(intel_dp->panel_power_cycle_delay);
+			intel_dp->last_power_cycle = jiffies;
 	}
 }
 
@@ -1217,6 +1234,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp)
 	POSTING_READ(pp_ctrl_reg);
 
 	ironlake_wait_panel_on(intel_dp);
+	intel_dp->last_power_on = jiffies;
 
 	if (IS_GEN5(dev)) {
 		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
@@ -1237,6 +1255,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("Turn eDP power off\n");
 
+	ironlake_edp_wait_backlight_off(intel_dp);
+
 	pp = ironlake_get_pp_control(intel_dp);
 	/* We need to switch off panel power _and_ force vdd, for otherwise some
 	 * panels get very unhappy and cease to work. */
@@ -1268,7 +1288,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
 	 * link.  So delay a bit to make sure the image is solid before
 	 * allowing it to appear.
 	 */
-	msleep(intel_dp->backlight_on_delay);
+	ironlake_wait_backlight_on(intel_dp);
 	pp = ironlake_get_pp_control(intel_dp);
 	pp |= EDP_BLC_ENABLE;
 
@@ -1300,7 +1320,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
 
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
-	msleep(intel_dp->backlight_off_delay);
+	intel_dp->last_backlight_off = jiffies;
 }
 
 static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea62673..eda8ee8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -484,6 +484,9 @@ struct intel_dp {
 	int backlight_off_delay;
 	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;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
 };
@@ -707,6 +710,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
 void intel_display_set_init_power(struct drm_device *dev, bool enable);
 int valleyview_get_vco(struct drm_i915_private *dev_priv);
+void intel_wait_until_after(unsigned long timestamp, int to_wait_ms);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.8.3.1

[PATCH 5/5] drm/i915: init the DP panel power seq variables earlier

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Our driver has two different ways of waiting for panel power
sequencing delays. One of these ways is through
ironlake_wait_panel_status, which implicitly uses the values written
to our registers. The other way is through the functions that call
intel_wait_until_after, and on this case we do direct msleep() calls
on the intel_dp->xxx_delay variables.

Function intel_dp_init_panel_power_sequencer is responsible for
initializing the _delay variables and deciding which values we need to
write to the registers, but it does not write these values to the
registers. Only at intel_dp_init_panel_power_sequencer_registers we
actually do this write.

Then problem is that when we call intel_dp_i2c_init, we will get some
I2C calls, which will trigger a VDD enable, which will make use of the
panel power sequencing registers and the _delay variables, so we need
to have both ready by this time. Today, when this happens, the _delay
variables are zero (because they were not computed) and the panel
power sequence registers contain whatever values were written by the
BIOS (which are usually correct).

What this patch does is to make sure that function
intel_dp_init_panel_power_sequencer is called earlier, so by the time
we call intel_dp_i2c_init, the _delay variables will already be
initialized. The actual registers won't contain their final values,
but at least they will contain the values set by the BIOS.

The good side is that we were reading the values, but were not using
them for anything (because we were just skipping the msleep(0) calls),
so this "fix" shouldn't fix any real existing bugs. I was only able to
identify the problem because I added some debug code to check how much
time time we were saving with my previous patch.

Regression introduced by:
    commit ed92f0b239ac971edc509169ae3d6955fbe0a188
    Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
    Date:   Wed Jun 12 17:27:24 2013 -0300
        drm/i915: extract intel_edp_init_connector

v2: - Rewrite commit message.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 79f7ec2..9cc5819 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3540,14 +3540,14 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 }
 
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
-				     struct intel_connector *intel_connector)
+				     struct intel_connector *intel_connector,
+				     struct edp_power_seq *power_seq)
 {
 	struct drm_connector *connector = &intel_connector->base;
 	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 drm_display_mode *fixed_mode = NULL;
-	struct edp_power_seq power_seq = { 0 };
 	bool has_dpcd;
 	struct drm_display_mode *scan;
 	struct edid *edid;
@@ -3555,8 +3555,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
-	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-
 	/* Cache DPCD and EDID for edp. */
 	ironlake_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);
@@ -3574,8 +3572,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 
 	/* We now know it's not a ghost, init power sequence regs. */
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-						      &power_seq);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
 
 	edid = drm_get_edid(connector, &intel_dp->adapter);
 	if (edid) {
@@ -3624,6 +3621,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
+	struct edp_power_seq power_seq = { 0 };
 	const char *name = NULL;
 	int type, error;
 
@@ -3707,13 +3705,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		BUG();
 	}
 
+	if (is_edp(intel_dp))
+		intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
+
 	error = intel_dp_i2c_init(intel_dp, intel_connector, name);
 	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
 	     error, port_name(port));
 
 	intel_dp->psr_setup_done = false;
 
-	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
+	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
 		i2c_del_adapter(&intel_dp->adapter);
 		if (is_edp(intel_dp)) {
 			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-- 
1.8.3.1

Re: [PATCH 3/5] drm/i915: fix VDD override off wait

[Jesse Barnes]

On Fri,  6 Dec 2013 17:32:42 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we're disabling the VDD override bit and the panel is enabled, we
> don't need to wait for anything. If the panel is disabled, then we
> need to actually wait for panel_power_cycle_delay, not
> panel_power_down_delay, because the power down delay was already
> respected when we disabled the panel.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fe327ce..a2aace2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1142,7 +1142,9 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  		/* Make sure sequencer is idle before allowing subsequent activity */
>  		DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
>  		I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
> -		msleep(intel_dp->panel_power_down_delay);
> +
> +		if ((pp & POWER_TARGET_ON) == 0)
> +			msleep(intel_dp->panel_power_cycle_delay);
>  	}
>  }
>  

Lemme check the eDP docs on this one...  it's supposed to be T12, which
is the time between power cycles.  Yeah that matches what we're using
elsewhere, so:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: save some time when waiting the eDP timings

[Jesse Barnes]

On Fri,  6 Dec 2013 17:32:43 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The eDP spec defines some points where after you do action A, you have
> to wait some time before action B. The thing is that in our driver
> action B does not happen exactly after action A, but we still use
> msleep() calls directly. What this patch does is that we record the
> timestamp of when action A happened, then, just before action B, we
> look at how much time has passed and only sleep the remaining amount
> needed.
> 
> With this change, I am able to save about 5-20ms (out of the total
> 200ms) of the backlight_off delay and completely skip the 1ms
> backlight_on delay. The 600ms vdd_off delay doesn't happen during
> normal usage anymore due to a previous patch.
> 
> v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and
>       move it to intel_display.c
>     - Fix the msleep call: diff is in jiffies
> v3: - Use "tmp_jiffies" so we don't need to worry about the value of
>       "jiffies" advancing while we're doing the math.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 26 +++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c59b67..0c238dd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -806,6 +806,24 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  		DRM_DEBUG_KMS("vblank wait timed out\n");
>  }
>  
> +/* If you need to wait X ms between events A and B, but event B doesn't happen
> + * exactly after event A, you record the timestamp (jiffies) of when event A
> + * happened, then just before event B you call intel_wait_until_after and pass
> + * the timestamp as the first argument, and X as the second argument. */
> +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms)
> +{
> +	unsigned long target = timestamp + msecs_to_jiffies(to_wait_ms);
> +	unsigned long diff;
> +	/* Don't re-read the value of "jiffies" every time since it may change
> +	 * behind our back and break the math. */
> +	unsigned long tmp_jiffies = jiffies;
> +
> +	if (time_after(target, tmp_jiffies)) {
> +		diff = (long)target - (long)tmp_jiffies;
> +		msleep(jiffies_to_msecs(diff));
> +	}
> +}
> +
>  static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a2aace2..79f7ec2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1056,9 +1056,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
>  static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp)
>  {
>  	DRM_DEBUG_KMS("Wait for panel power cycle\n");
> +
> +	/* When we disable the VDD override bit last we have to do the manual
> +	 * wait. */
> +	intel_wait_until_after(intel_dp->last_power_cycle,
> +			       intel_dp->panel_power_cycle_delay);
> +
>  	ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
>  }
>  
> +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp)
> +{
> +	intel_wait_until_after(intel_dp->last_power_on,
> +			       intel_dp->backlight_on_delay);
> +}
> +
> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> +{
> +	intel_wait_until_after(intel_dp->last_backlight_off,
> +			       intel_dp->backlight_off_delay);
> +}
>  
>  /* Read the current pp_control value, unlocking the register if it
>   * is locked
> @@ -1144,7 +1161,7 @@ static void ironlake_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)
> -			msleep(intel_dp->panel_power_cycle_delay);
> +			intel_dp->last_power_cycle = jiffies;
>  	}
>  }
>  
> @@ -1217,6 +1234,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp)
>  	POSTING_READ(pp_ctrl_reg);
>  
>  	ironlake_wait_panel_on(intel_dp);
> +	intel_dp->last_power_on = jiffies;
>  
>  	if (IS_GEN5(dev)) {
>  		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
> @@ -1237,6 +1255,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
>  
>  	DRM_DEBUG_KMS("Turn eDP power off\n");
>  
> +	ironlake_edp_wait_backlight_off(intel_dp);
> +
>  	pp = ironlake_get_pp_control(intel_dp);
>  	/* We need to switch off panel power _and_ force vdd, for otherwise some
>  	 * panels get very unhappy and cease to work. */
> @@ -1268,7 +1288,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
>  	 * link.  So delay a bit to make sure the image is solid before
>  	 * allowing it to appear.
>  	 */
> -	msleep(intel_dp->backlight_on_delay);
> +	ironlake_wait_backlight_on(intel_dp);
>  	pp = ironlake_get_pp_control(intel_dp);
>  	pp |= EDP_BLC_ENABLE;
>  
> @@ -1300,7 +1320,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
>  
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
> -	msleep(intel_dp->backlight_off_delay);
> +	intel_dp->last_backlight_off = jiffies;
>  }
>  
>  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea62673..eda8ee8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -484,6 +484,9 @@ struct intel_dp {
>  	int backlight_off_delay;
>  	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;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
>  };
> @@ -707,6 +710,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
> +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);

Nice.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/5] drm/i915: init the DP panel power seq variables earlier

[Jesse Barnes]

On Fri,  6 Dec 2013 17:32:44 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Our driver has two different ways of waiting for panel power
> sequencing delays. One of these ways is through
> ironlake_wait_panel_status, which implicitly uses the values written
> to our registers. The other way is through the functions that call
> intel_wait_until_after, and on this case we do direct msleep() calls
> on the intel_dp->xxx_delay variables.
> 
> Function intel_dp_init_panel_power_sequencer is responsible for
> initializing the _delay variables and deciding which values we need to
> write to the registers, but it does not write these values to the
> registers. Only at intel_dp_init_panel_power_sequencer_registers we
> actually do this write.
> 
> Then problem is that when we call intel_dp_i2c_init, we will get some
> I2C calls, which will trigger a VDD enable, which will make use of the
> panel power sequencing registers and the _delay variables, so we need
> to have both ready by this time. Today, when this happens, the _delay
> variables are zero (because they were not computed) and the panel
> power sequence registers contain whatever values were written by the
> BIOS (which are usually correct).
> 
> What this patch does is to make sure that function
> intel_dp_init_panel_power_sequencer is called earlier, so by the time
> we call intel_dp_i2c_init, the _delay variables will already be
> initialized. The actual registers won't contain their final values,
> but at least they will contain the values set by the BIOS.
> 
> The good side is that we were reading the values, but were not using
> them for anything (because we were just skipping the msleep(0) calls),
> so this "fix" shouldn't fix any real existing bugs. I was only able to
> identify the problem because I added some debug code to check how much
> time time we were saving with my previous patch.
> 
> Regression introduced by:
>     commit ed92f0b239ac971edc509169ae3d6955fbe0a188
>     Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>     Date:   Wed Jun 12 17:27:24 2013 -0300
>         drm/i915: extract intel_edp_init_connector
> 
> v2: - Rewrite commit message.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 79f7ec2..9cc5819 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3540,14 +3540,14 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  }
>  
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> -				     struct intel_connector *intel_connector)
> +				     struct intel_connector *intel_connector,
> +				     struct edp_power_seq *power_seq)
>  {
>  	struct drm_connector *connector = &intel_connector->base;
>  	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 drm_display_mode *fixed_mode = NULL;
> -	struct edp_power_seq power_seq = { 0 };
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
> @@ -3555,8 +3555,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> -	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> -
>  	/* Cache DPCD and EDID for edp. */
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	has_dpcd = intel_dp_get_dpcd(intel_dp);
> @@ -3574,8 +3572,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	}
>  
>  	/* We now know it's not a ghost, init power sequence regs. */
> -	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
> -						      &power_seq);
> +	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
>  
>  	edid = drm_get_edid(connector, &intel_dp->adapter);
>  	if (edid) {
> @@ -3624,6 +3621,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_dig_port->port;
> +	struct edp_power_seq power_seq = { 0 };
>  	const char *name = NULL;
>  	int type, error;
>  
> @@ -3707,13 +3705,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		BUG();
>  	}
>  
> +	if (is_edp(intel_dp))
> +		intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> +
>  	error = intel_dp_i2c_init(intel_dp, intel_connector, name);
>  	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>  	     error, port_name(port));
>  
>  	intel_dp->psr_setup_done = false;
>  
> -	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> +	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
>  		i2c_del_adapter(&intel_dp->adapter);
>  		if (is_edp(intel_dp)) {
>  			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);

Yeah, seems reasonable.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: save some time when waiting the eDP timings

[Daniel Vetter]

On Fri, Dec 06, 2013 at 05:32:43PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The eDP spec defines some points where after you do action A, you have
> to wait some time before action B. The thing is that in our driver
> action B does not happen exactly after action A, but we still use
> msleep() calls directly. What this patch does is that we record the
> timestamp of when action A happened, then, just before action B, we
> look at how much time has passed and only sleep the remaining amount
> needed.
> 
> With this change, I am able to save about 5-20ms (out of the total
> 200ms) of the backlight_off delay and completely skip the 1ms
> backlight_on delay. The 600ms vdd_off delay doesn't happen during
> normal usage anymore due to a previous patch.
> 
> v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and
>       move it to intel_display.c
>     - Fix the msleep call: diff is in jiffies
> v3: - Use "tmp_jiffies" so we don't need to worry about the value of
>       "jiffies" advancing while we're doing the math.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 26 +++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c59b67..0c238dd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -806,6 +806,24 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  		DRM_DEBUG_KMS("vblank wait timed out\n");
>  }
>  
> +/* If you need to wait X ms between events A and B, but event B doesn't happen
> + * exactly after event A, you record the timestamp (jiffies) of when event A
> + * happened, then just before event B you call intel_wait_until_after and pass
> + * the timestamp as the first argument, and X as the second argument. */
> +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms)
> +{
> +	unsigned long target = timestamp + msecs_to_jiffies(to_wait_ms);

msec_to_jiffies_timeout is what we want here. Also I nowadays prefer
multiline comments with the /* and */ on their own line for more
consistency ...
-Daniel

> +	unsigned long diff;
> +	/* Don't re-read the value of "jiffies" every time since it may change
> +	 * behind our back and break the math. */
> +	unsigned long tmp_jiffies = jiffies;
> +
> +	if (time_after(target, tmp_jiffies)) {
> +		diff = (long)target - (long)tmp_jiffies;
> +		msleep(jiffies_to_msecs(diff));

This will add one more jiffy again, so for optimal results we need to use
something else here I think.

I've merged the previous 3 patches and signed up Jesse to review the last
one.
-Daniel

> +	}
> +}
> +
>  static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a2aace2..79f7ec2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1056,9 +1056,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
>  static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp)
>  {
>  	DRM_DEBUG_KMS("Wait for panel power cycle\n");
> +
> +	/* When we disable the VDD override bit last we have to do the manual
> +	 * wait. */
> +	intel_wait_until_after(intel_dp->last_power_cycle,
> +			       intel_dp->panel_power_cycle_delay);
> +
>  	ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
>  }
>  
> +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp)
> +{
> +	intel_wait_until_after(intel_dp->last_power_on,
> +			       intel_dp->backlight_on_delay);
> +}
> +
> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> +{
> +	intel_wait_until_after(intel_dp->last_backlight_off,
> +			       intel_dp->backlight_off_delay);
> +}
>  
>  /* Read the current pp_control value, unlocking the register if it
>   * is locked
> @@ -1144,7 +1161,7 @@ static void ironlake_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)
> -			msleep(intel_dp->panel_power_cycle_delay);
> +			intel_dp->last_power_cycle = jiffies;
>  	}
>  }
>  
> @@ -1217,6 +1234,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp)
>  	POSTING_READ(pp_ctrl_reg);
>  
>  	ironlake_wait_panel_on(intel_dp);
> +	intel_dp->last_power_on = jiffies;
>  
>  	if (IS_GEN5(dev)) {
>  		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
> @@ -1237,6 +1255,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
>  
>  	DRM_DEBUG_KMS("Turn eDP power off\n");
>  
> +	ironlake_edp_wait_backlight_off(intel_dp);
> +
>  	pp = ironlake_get_pp_control(intel_dp);
>  	/* We need to switch off panel power _and_ force vdd, for otherwise some
>  	 * panels get very unhappy and cease to work. */
> @@ -1268,7 +1288,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
>  	 * link.  So delay a bit to make sure the image is solid before
>  	 * allowing it to appear.
>  	 */
> -	msleep(intel_dp->backlight_on_delay);
> +	ironlake_wait_backlight_on(intel_dp);
>  	pp = ironlake_get_pp_control(intel_dp);
>  	pp |= EDP_BLC_ENABLE;
>  
> @@ -1300,7 +1320,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
>  
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
> -	msleep(intel_dp->backlight_off_delay);
> +	intel_dp->last_backlight_off = jiffies;
>  }
>  
>  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea62673..eda8ee8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -484,6 +484,9 @@ struct intel_dp {
>  	int backlight_off_delay;
>  	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;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
>  };
> @@ -707,6 +710,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
> +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/5] drm/i915: save some time when waiting the eDP timings

[Daniel Vetter]

On Tue, Dec 10, 2013 at 11:28:59PM +0100, Daniel Vetter wrote:
> On Fri, Dec 06, 2013 at 05:32:43PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > The eDP spec defines some points where after you do action A, you have
> > to wait some time before action B. The thing is that in our driver
> > action B does not happen exactly after action A, but we still use
> > msleep() calls directly. What this patch does is that we record the
> > timestamp of when action A happened, then, just before action B, we
> > look at how much time has passed and only sleep the remaining amount
> > needed.
> > 
> > With this change, I am able to save about 5-20ms (out of the total
> > 200ms) of the backlight_off delay and completely skip the 1ms
> > backlight_on delay. The 600ms vdd_off delay doesn't happen during
> > normal usage anymore due to a previous patch.
> > 
> > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and
> >       move it to intel_display.c
> >     - Fix the msleep call: diff is in jiffies
> > v3: - Use "tmp_jiffies" so we don't need to worry about the value of
> >       "jiffies" advancing while we're doing the math.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 26 +++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
> >  3 files changed, 45 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3c59b67..0c238dd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -806,6 +806,24 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
> >  		DRM_DEBUG_KMS("vblank wait timed out\n");
> >  }
> >  
> > +/* If you need to wait X ms between events A and B, but event B doesn't happen
> > + * exactly after event A, you record the timestamp (jiffies) of when event A
> > + * happened, then just before event B you call intel_wait_until_after and pass
> > + * the timestamp as the first argument, and X as the second argument. */
> > +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms)
> > +{
> > +	unsigned long target = timestamp + msecs_to_jiffies(to_wait_ms);
> 
> msec_to_jiffies_timeout is what we want here. Also I nowadays prefer
> multiline comments with the /* and */ on their own line for more
> consistency ...
> -Daniel
> 
> > +	unsigned long diff;
> > +	/* Don't re-read the value of "jiffies" every time since it may change
> > +	 * behind our back and break the math. */
> > +	unsigned long tmp_jiffies = jiffies;
> > +
> > +	if (time_after(target, tmp_jiffies)) {
> > +		diff = (long)target - (long)tmp_jiffies;
> > +		msleep(jiffies_to_msecs(diff));
> 
> This will add one more jiffy again, so for optimal results we need to use
> something else here I think.

schedule_timeout is what I think we should use here. Also, this function
isn't really anything intel specific, so I think we should drop the intel_
prefix and shovel it as a static inline (it's really small after all) next
to the other generic timeout helpers at the bottom of i915_drv.h.

I'm also a bit unhappy still about the name - we should somehow make it
clearer that the timeout is in ms. A few ideas:

msleep_start_from_jiffies
msleep_form_jiffies
wait_after_until_ms
wait_remaining_ms_from_jiffies

Also mabye rename timestamp to ts_jiffies or so. All this bikeshedding
here is because the mixing of time-units here irks me a bit (and I pretty
much expect someone to botch it eventually). So spending a bit more time
on coming up with a really good name would be good imo.

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

Re: [PATCH 3/5] drm/i915: fix VDD override off wait

[Patrik Jakobsson]

On Fri, Dec 6, 2013 at 8:47 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Fri,  6 Dec 2013 17:32:42 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we're disabling the VDD override bit and the panel is enabled, we
>> don't need to wait for anything. If the panel is disabled, then we
>> need to actually wait for panel_power_cycle_delay, not
>> panel_power_down_delay, because the power down delay was already
>> respected when we disabled the panel.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index fe327ce..a2aace2 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1142,7 +1142,9 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>>               /* Make sure sequencer is idle before allowing subsequent activity */
>>               DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
>>               I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
>> -             msleep(intel_dp->panel_power_down_delay);
>> +
>> +             if ((pp & POWER_TARGET_ON) == 0)
>> +                     msleep(intel_dp->panel_power_cycle_delay);
>>       }
>>  }
>>
>
> Lemme check the eDP docs on this one...  it's supposed to be T12, which
> is the time between power cycles.  Yeah that matches what we're using
> elsewhere, so:
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Starting with this patch I get a blank screen when booting my MacBook Air 6,2.
Reverting only this patch on top of 3.14-rc1 doesn't help so it's most likely
the entire series that needs to be looked at.

Doing a suspend/resume fixes the problem.

Thanks
Patrik Jakobsson

Re: [PATCH 3/5] drm/i915: fix VDD override off wait

[Chris Wilson]

On Thu, Feb 06, 2014 at 06:16:02PM +0100, Patrik Jakobsson wrote:
> On Fri, Dec 6, 2013 at 8:47 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Fri,  6 Dec 2013 17:32:42 -0200
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If we're disabling the VDD override bit and the panel is enabled, we
> >> don't need to wait for anything. If the panel is disabled, then we
> >> need to actually wait for panel_power_cycle_delay, not
> >> panel_power_down_delay, because the power down delay was already
> >> respected when we disabled the panel.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index fe327ce..a2aace2 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1142,7 +1142,9 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
> >>               /* Make sure sequencer is idle before allowing subsequent activity */
> >>               DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
> >>               I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
> >> -             msleep(intel_dp->panel_power_down_delay);
> >> +
> >> +             if ((pp & POWER_TARGET_ON) == 0)
> >> +                     msleep(intel_dp->panel_power_cycle_delay);
> >>       }
> >>  }
> >>
> >
> > Lemme check the eDP docs on this one...  it's supposed to be T12, which
> > is the time between power cycles.  Yeah that matches what we're using
> > elsewhere, so:
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Starting with this patch I get a blank screen when booting my MacBook Air 6,2.
> Reverting only this patch on top of 3.14-rc1 doesn't help so it's most likely
> the entire series that needs to be looked at.
> 
> Doing a suspend/resume fixes the problem.

I've filed this as https://bugs.freedesktop.org/show_bug.cgi?id=74628
Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: fix VDD override off wait

[Paulo Zanoni]

2014-02-06 15:22 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Thu, Feb 06, 2014 at 06:16:02PM +0100, Patrik Jakobsson wrote:
>> On Fri, Dec 6, 2013 at 8:47 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > On Fri,  6 Dec 2013 17:32:42 -0200
>> > Paulo Zanoni <przanoni@gmail.com> wrote:
>> >
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> If we're disabling the VDD override bit and the panel is enabled, we
>> >> don't need to wait for anything. If the panel is disabled, then we
>> >> need to actually wait for panel_power_cycle_delay, not
>> >> panel_power_down_delay, because the power down delay was already
>> >> respected when we disabled the panel.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> >> index fe327ce..a2aace2 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> @@ -1142,7 +1142,9 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>> >>               /* Make sure sequencer is idle before allowing subsequent activity */
>> >>               DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
>> >>               I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
>> >> -             msleep(intel_dp->panel_power_down_delay);
>> >> +
>> >> +             if ((pp & POWER_TARGET_ON) == 0)
>> >> +                     msleep(intel_dp->panel_power_cycle_delay);
>> >>       }
>> >>  }
>> >>
>> >
>> > Lemme check the eDP docs on this one...  it's supposed to be T12, which
>> > is the time between power cycles.  Yeah that matches what we're using
>> > elsewhere, so:
>> >
>> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>> Starting with this patch I get a blank screen when booting my MacBook Air 6,2.
>> Reverting only this patch on top of 3.14-rc1 doesn't help so it's most likely
>> the entire series that needs to be looked at.
>>
>> Doing a suspend/resume fixes the problem.

Oh, well, at least we didn't break suspend/resume :)

I just requested some information at the bugzilla entry created by
Chris. Can you please provide the requested files there, and move the
discussion to bugzilla?

>
> I've filed this as https://bugs.freedesktop.org/show_bug.cgi?id=74628

Thanks for doing this! It's too easy to forget about mailing-list-only
bug reports.

> Thanks,
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/5] drm/i915: fix VDD override off wait

[Patrik Jakobsson]

On Thu, Feb 6, 2014 at 8:30 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-02-06 15:22 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> On Thu, Feb 06, 2014 at 06:16:02PM +0100, Patrik Jakobsson wrote:
>>> On Fri, Dec 6, 2013 at 8:47 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>> > On Fri,  6 Dec 2013 17:32:42 -0200
>>> > Paulo Zanoni <przanoni@gmail.com> wrote:
>>> >
>>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> >>
>>> >> If we're disabling the VDD override bit and the panel is enabled, we
>>> >> don't need to wait for anything. If the panel is disabled, then we
>>> >> need to actually wait for panel_power_cycle_delay, not
>>> >> panel_power_down_delay, because the power down delay was already
>>> >> respected when we disabled the panel.
>>> >>
>>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> >> ---
>>> >>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> >> index fe327ce..a2aace2 100644
>>> >> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> >> @@ -1142,7 +1142,9 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>>> >>               /* Make sure sequencer is idle before allowing subsequent activity */
>>> >>               DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
>>> >>               I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
>>> >> -             msleep(intel_dp->panel_power_down_delay);
>>> >> +
>>> >> +             if ((pp & POWER_TARGET_ON) == 0)
>>> >> +                     msleep(intel_dp->panel_power_cycle_delay);
>>> >>       }
>>> >>  }
>>> >>
>>> >
>>> > Lemme check the eDP docs on this one...  it's supposed to be T12, which
>>> > is the time between power cycles.  Yeah that matches what we're using
>>> > elsewhere, so:
>>> >
>>> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>>
>>> Starting with this patch I get a blank screen when booting my MacBook Air 6,2.
>>> Reverting only this patch on top of 3.14-rc1 doesn't help so it's most likely
>>> the entire series that needs to be looked at.
>>>
>>> Doing a suspend/resume fixes the problem.
>
> Oh, well, at least we didn't break suspend/resume :)
>
> I just requested some information at the bugzilla entry created by
> Chris. Can you please provide the requested files there, and move the
> discussion to bugzilla?
>
>>
>> I've filed this as https://bugs.freedesktop.org/show_bug.cgi?id=74628
>
> Thanks for doing this! It's too easy to forget about mailing-list-only
> bug reports.

I was just a bit lazy. The dmesg is up on bugzilla.

Thanks
Patrik