[PATCH 0/6] eDP panel power sequencing optimizations

[PATCH 0/6] eDP panel power sequencing optimizations

[Paulo Zanoni]

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

Hi

These patches save a lot of time on our eDP panel power sequencing times. We
were doing a lot of needless waits, so now our load times should be much
shorter.

This series should improve a little bit our driver load time, and it should
greatly improve our suspend time. The most relevant thing is that when we were
waiting for the "panel off" timeout (50ms on my machine), we were not just
waiting for it, but also waiting for the "backlight off" timeout (200ms) and the
"panel power cycle" timeout (500ms). QA did some measurements of a branch
containing these 6 patches + two patches removing the "wait for vblank timeout"
problems we have on Haswell, and the results are reported here:
https://bugs.freedesktop.org/show_bug.cgi?id=72211.

One of my worries about this series is that, due to the extremely smaller wait
times, we may get regression reports saying "my panel is now black!" due to
broken VBT/BIOS/panel/whatever. If this happens, IMHO instead of just reverting
the current patches, we should add some extra msleep() calls surrounded by FIXME
comments explicitly saying why we are waiting more than we should, because the
current code was silently hiding the fact that we were waiting more than we
should.

Thanks,
Paulo

Paulo Zanoni (6):
  drm/i915: init the DP panel power seq variables earlier
  drm/i915: save some time when waiting the eDP timings
  drm/i915: reset eDP timestamps on resume
  drm/i915: remove a column of zeros from the eDP wait definitions
  drm/i915: don't wait for power cycle when waiting for power off
  drm/i915: set the backlight panel delays registers to 1

 drivers/gpu/drm/i915/i915_drv.c      |  2 +
 drivers/gpu/drm/i915/i915_drv.h      | 29 ++++++++++++
 drivers/gpu/drm/i915/intel_ddi.c     |  1 +
 drivers/gpu/drm/i915/intel_display.c | 11 +++++
 drivers/gpu/drm/i915/intel_dp.c      | 88 ++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  6 +++
 6 files changed, 118 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] 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.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
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 7df5085..9d96447 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3532,14 +3532,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;
@@ -3547,8 +3547,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);
@@ -3566,8 +3564,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) {
@@ -3616,6 +3613,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;
 
@@ -3699,13 +3697,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 1/6] drm/i915: init the DP panel power seq variables earlier

[Jesse Barnes]

On Thu, 19 Dec 2013 14:29:39 -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.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 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 7df5085..9d96447 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3532,14 +3532,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;
> @@ -3547,8 +3547,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);
> @@ -3566,8 +3564,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) {
> @@ -3616,6 +3613,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;
>  
> @@ -3699,13 +3697,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);

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

[PATCH 2/6] 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.
v4: - Rename function again.
    - Move function to i915_drv.h.
    - Store last_power_cycle at edp_panel_off too.
    - Use msecs_to_jiffies_timeout, then replace the msleep with an
      open-coded version that avoids the extra +1 jiffy.
    - Try to add units to every variable name so we don't confuse
      jiffies with milliseconds.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cc8afff..7e9b436 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
 	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
 }
 
+/*
+ * If you need to wait X milliseconds 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 this function and
+ * pass the timestamp as the first argument, and X as the second argument.
+ */
+static inline void
+wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
+{
+	unsigned long target_jiffies, tmp_jiffies;
+	unsigned int remaining_ms;
+
+	/*
+	 * Don't re-read the value of "jiffies" every time since it may change
+	 * behind our back and break the math.
+	 */
+	tmp_jiffies = jiffies;
+	target_jiffies = timestamp_jiffies +
+			 msecs_to_jiffies_timeout(to_wait_ms);
+
+	if (time_after(target_jiffies, tmp_jiffies)) {
+		remaining_ms = jiffies_to_msecs((long)target_jiffies -
+						(long)tmp_jiffies);
+		while (remaining_ms)
+			remaining_ms =
+				schedule_timeout_uninterruptible(remaining_ms);
+	}
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9d96447..2f82af4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1057,9 +1057,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. */
+	wait_remaining_ms_from_jiffies(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)
+{
+	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
+				       intel_dp->backlight_on_delay);
+}
+
+static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
+{
+	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
+				       intel_dp->backlight_off_delay);
+}
 
 /* Read the current pp_control value, unlocking the register if it
  * is locked
@@ -1147,7 +1164,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;
 
 		intel_runtime_pm_put(dev_priv);
 	}
@@ -1222,6 +1239,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 */
@@ -1242,6 +1260,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. */
@@ -1252,6 +1272,7 @@ void ironlake_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;
 	ironlake_wait_panel_off(intel_dp);
 }
 
@@ -1273,7 +1294,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;
 
@@ -1305,7 +1326,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 46aea6c..92de688 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -485,6 +485,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;
 };
-- 
1.8.3.1

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

[Jesse Barnes]

On Thu, 19 Dec 2013 14:29:40 -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.
> v4: - Rename function again.
>     - Move function to i915_drv.h.
>     - Store last_power_cycle at edp_panel_off too.
>     - Use msecs_to_jiffies_timeout, then replace the msleep with an
>       open-coded version that avoids the extra +1 jiffy.
>     - Try to add units to every variable name so we don't confuse
>       jiffies with milliseconds.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  3 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc8afff..7e9b436 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +/*
> + * If you need to wait X milliseconds 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 this function and
> + * pass the timestamp as the first argument, and X as the second argument.
> + */
> +static inline void
> +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> +{
> +	unsigned long target_jiffies, tmp_jiffies;
> +	unsigned int remaining_ms;
> +
> +	/*
> +	 * Don't re-read the value of "jiffies" every time since it may change
> +	 * behind our back and break the math.
> +	 */
> +	tmp_jiffies = jiffies;
> +	target_jiffies = timestamp_jiffies +
> +			 msecs_to_jiffies_timeout(to_wait_ms);
> +
> +	if (time_after(target_jiffies, tmp_jiffies)) {
> +		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> +						(long)tmp_jiffies);
> +		while (remaining_ms)
> +			remaining_ms =
> +				schedule_timeout_uninterruptible(remaining_ms);
> +	}
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9d96447..2f82af4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1057,9 +1057,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. */
> +	wait_remaining_ms_from_jiffies(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)
> +{
> +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> +				       intel_dp->backlight_on_delay);
> +}
> +
> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> +{
> +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> +				       intel_dp->backlight_off_delay);
> +}
>  
>  /* Read the current pp_control value, unlocking the register if it
>   * is locked
> @@ -1147,7 +1164,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;
>  
>  		intel_runtime_pm_put(dev_priv);
>  	}
> @@ -1222,6 +1239,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 */
> @@ -1242,6 +1260,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. */
> @@ -1252,6 +1272,7 @@ void ironlake_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;
>  	ironlake_wait_panel_off(intel_dp);
>  }
>  
> @@ -1273,7 +1294,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;
>  
> @@ -1305,7 +1326,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 46aea6c..92de688 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -485,6 +485,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;
>  };

We could bikeshed the name all day, but this looks like a great
improvement.

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

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

[Jani Nikula]

On Thu, 19 Dec 2013, 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.
> v4: - Rename function again.
>     - Move function to i915_drv.h.
>     - Store last_power_cycle at edp_panel_off too.
>     - Use msecs_to_jiffies_timeout, then replace the msleep with an
>       open-coded version that avoids the extra +1 jiffy.
>     - Try to add units to every variable name so we don't confuse
>       jiffies with milliseconds.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  3 files changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc8afff..7e9b436 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +/*
> + * If you need to wait X milliseconds 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 this function and
> + * pass the timestamp as the first argument, and X as the second argument.
> + */
> +static inline void
> +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> +{
> +	unsigned long target_jiffies, tmp_jiffies;
> +	unsigned int remaining_ms;
> +
> +	/*
> +	 * Don't re-read the value of "jiffies" every time since it may change
> +	 * behind our back and break the math.
> +	 */
> +	tmp_jiffies = jiffies;
> +	target_jiffies = timestamp_jiffies +
> +			 msecs_to_jiffies_timeout(to_wait_ms);

Is it possible this gets called before the last_* timestamp fields are
initialized, resulting to timestamp_jiffies being 0? Do we need to
special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0)
so we maintain this precondition.

> +
> +	if (time_after(target_jiffies, tmp_jiffies)) {
> +		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> +						(long)tmp_jiffies);
> +		while (remaining_ms)
> +			remaining_ms =
> +				schedule_timeout_uninterruptible(remaining_ms);

schedule_timeout_uninterruptible takes jiffies, not ms. I think you
should do the measurements again after fixing this.

> +	}
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9d96447..2f82af4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1057,9 +1057,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. */
> +	wait_remaining_ms_from_jiffies(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)
> +{
> +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> +				       intel_dp->backlight_on_delay);
> +}
> +
> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> +{
> +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> +				       intel_dp->backlight_off_delay);
> +}
>  
>  /* Read the current pp_control value, unlocking the register if it
>   * is locked
> @@ -1147,7 +1164,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;
>  
>  		intel_runtime_pm_put(dev_priv);
>  	}
> @@ -1222,6 +1239,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 */
> @@ -1242,6 +1260,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. */
> @@ -1252,6 +1272,7 @@ void ironlake_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;
>  	ironlake_wait_panel_off(intel_dp);
>  }
>  
> @@ -1273,7 +1294,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;
>  
> @@ -1305,7 +1326,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 46aea6c..92de688 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -485,6 +485,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;

The delays and the timestamps could be grouped into an unnamed struct
within intel_dp, but this can be left as a follow-up patch.

BR,
Jani.

>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
>  };
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Daniel Vetter]

On Fri, Dec 20, 2013 at 11:30:15AM +0200, Jani Nikula wrote:
> On Thu, 19 Dec 2013, 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.
> > v4: - Rename function again.
> >     - Move function to i915_drv.h.
> >     - Store last_power_cycle at edp_panel_off too.
> >     - Use msecs_to_jiffies_timeout, then replace the msleep with an
> >       open-coded version that avoids the extra +1 jiffy.
> >     - Try to add units to every variable name so we don't confuse
> >       jiffies with milliseconds.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_drv.h |  3 +++
> >  3 files changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index cc8afff..7e9b436 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
> >  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> >  }
> >  
> > +/*
> > + * If you need to wait X milliseconds 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 this function and
> > + * pass the timestamp as the first argument, and X as the second argument.
> > + */
> > +static inline void
> > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> > +{
> > +	unsigned long target_jiffies, tmp_jiffies;
> > +	unsigned int remaining_ms;
> > +
> > +	/*
> > +	 * Don't re-read the value of "jiffies" every time since it may change
> > +	 * behind our back and break the math.
> > +	 */
> > +	tmp_jiffies = jiffies;
> > +	target_jiffies = timestamp_jiffies +
> > +			 msecs_to_jiffies_timeout(to_wait_ms);
> 
> Is it possible this gets called before the last_* timestamp fields are
> initialized, resulting to timestamp_jiffies being 0? Do we need to
> special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0)
> so we maintain this precondition.

afaik jiffies are allowed to wrap around to 0 ...
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Paulo Zanoni]

2013/12/20 Jani Nikula <jani.nikula@linux.intel.com>:
> On Thu, 19 Dec 2013, 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.
>> v4: - Rename function again.
>>     - Move function to i915_drv.h.
>>     - Store last_power_cycle at edp_panel_off too.
>>     - Use msecs_to_jiffies_timeout, then replace the msleep with an
>>       open-coded version that avoids the extra +1 jiffy.
>>     - Try to add units to every variable name so we don't confuse
>>       jiffies with milliseconds.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
>>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>>  3 files changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index cc8afff..7e9b436 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>>       return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>>  }
>>
>> +/*
>> + * If you need to wait X milliseconds 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 this function and
>> + * pass the timestamp as the first argument, and X as the second argument.
>> + */
>> +static inline void
>> +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>> +{
>> +     unsigned long target_jiffies, tmp_jiffies;
>> +     unsigned int remaining_ms;
>> +
>> +     /*
>> +      * Don't re-read the value of "jiffies" every time since it may change
>> +      * behind our back and break the math.
>> +      */
>> +     tmp_jiffies = jiffies;
>> +     target_jiffies = timestamp_jiffies +
>> +                      msecs_to_jiffies_timeout(to_wait_ms);
>
> Is it possible this gets called before the last_* timestamp fields are
> initialized, resulting to timestamp_jiffies being 0? Do we need to
> special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0)
> so we maintain this precondition.

The jiffies variable keeps running forever, it can be 0 at any time.
In the worst case we will wait more than we should, and this case is
really rare, so it's fine IMHO. And when booting, jiffies usually has
a super-high value.


>
>> +
>> +     if (time_after(target_jiffies, tmp_jiffies)) {
>> +             remaining_ms = jiffies_to_msecs((long)target_jiffies -
>> +                                             (long)tmp_jiffies);
>> +             while (remaining_ms)
>> +                     remaining_ms =
>> +                             schedule_timeout_uninterruptible(remaining_ms);
>
> schedule_timeout_uninterruptible takes jiffies, not ms. I think you
> should do the measurements again after fixing this.

Thanks for catching it! I have CONFIG_HZ=1000, so I guess 1ms == 1 jiffy here.


>
>> +     }
>> +}
>> +
>>  #endif
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 9d96447..2f82af4 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1057,9 +1057,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. */
>> +     wait_remaining_ms_from_jiffies(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)
>> +{
>> +     wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
>> +                                    intel_dp->backlight_on_delay);
>> +}
>> +
>> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
>> +{
>> +     wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
>> +                                    intel_dp->backlight_off_delay);
>> +}
>>
>>  /* Read the current pp_control value, unlocking the register if it
>>   * is locked
>> @@ -1147,7 +1164,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;
>>
>>               intel_runtime_pm_put(dev_priv);
>>       }
>> @@ -1222,6 +1239,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 */
>> @@ -1242,6 +1260,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. */
>> @@ -1252,6 +1272,7 @@ void ironlake_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;
>>       ironlake_wait_panel_off(intel_dp);
>>  }
>>
>> @@ -1273,7 +1294,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;
>>
>> @@ -1305,7 +1326,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 46aea6c..92de688 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -485,6 +485,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;
>
> The delays and the timestamps could be grouped into an unnamed struct
> within intel_dp, but this can be left as a follow-up patch.
>
> BR,
> Jani.
>
>>       bool psr_setup_done;
>>       struct intel_connector *attached_connector;
>>  };
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Paulo Zanoni

[PATCH 2/6] 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.
v4: - Rename function again.
    - Move function to i915_drv.h.
    - Store last_power_cycle at edp_panel_off too.
    - Use msecs_to_jiffies_timeout, then replace the msleep with an
      open-coded version that avoids the extra +1 jiffy.
    - Try to add units to every variable name so we don't confuse
      jiffies with milliseconds.
v5: - Fix schedule_timeout_interruptible usage: it uses jiffies.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cc8afff..0131ab0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2636,4 +2636,31 @@ timespec_to_jiffies_timeout(const struct timespec *value)
 	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
 }
 
+/*
+ * If you need to wait X milliseconds 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 this function and
+ * pass the timestamp as the first argument, and X as the second argument.
+ */
+static inline void
+wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
+{
+	unsigned long target_jiffies, tmp_jiffies, remaining_jiffies;
+
+	/*
+	 * Don't re-read the value of "jiffies" every time since it may change
+	 * behind our back and break the math.
+	 */
+	tmp_jiffies = jiffies;
+	target_jiffies = timestamp_jiffies +
+			 msecs_to_jiffies_timeout(to_wait_ms);
+
+	if (time_after(target_jiffies, tmp_jiffies)) {
+		remaining_jiffies = (long)target_jiffies - (long)tmp_jiffies;
+		while (remaining_jiffies)
+			remaining_jiffies =
+			    schedule_timeout_uninterruptible(remaining_jiffies);
+	}
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9d96447..2f82af4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1057,9 +1057,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. */
+	wait_remaining_ms_from_jiffies(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)
+{
+	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
+				       intel_dp->backlight_on_delay);
+}
+
+static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
+{
+	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
+				       intel_dp->backlight_off_delay);
+}
 
 /* Read the current pp_control value, unlocking the register if it
  * is locked
@@ -1147,7 +1164,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;
 
 		intel_runtime_pm_put(dev_priv);
 	}
@@ -1222,6 +1239,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 */
@@ -1242,6 +1260,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. */
@@ -1252,6 +1272,7 @@ void ironlake_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;
 	ironlake_wait_panel_off(intel_dp);
 }
 
@@ -1273,7 +1294,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;
 
@@ -1305,7 +1326,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 46aea6c..92de688 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -485,6 +485,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;
 };
-- 
1.8.4.2

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

[Jani Nikula]

On Thu, 19 Dec 2013, 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.
> v4: - Rename function again.
>     - Move function to i915_drv.h.
>     - Store last_power_cycle at edp_panel_off too.
>     - Use msecs_to_jiffies_timeout, then replace the msleep with an
>       open-coded version that avoids the extra +1 jiffy.
>     - Try to add units to every variable name so we don't confuse
>       jiffies with milliseconds.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  3 files changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc8afff..7e9b436 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +/*
> + * If you need to wait X milliseconds 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 this function and
> + * pass the timestamp as the first argument, and X as the second argument.
> + */
> +static inline void
> +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> +{
> +	unsigned long target_jiffies, tmp_jiffies;
> +	unsigned int remaining_ms;
> +
> +	/*
> +	 * Don't re-read the value of "jiffies" every time since it may change
> +	 * behind our back and break the math.
> +	 */
> +	tmp_jiffies = jiffies;
> +	target_jiffies = timestamp_jiffies +
> +			 msecs_to_jiffies_timeout(to_wait_ms);
> +
> +	if (time_after(target_jiffies, tmp_jiffies)) {
> +		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> +						(long)tmp_jiffies);
> +		while (remaining_ms)
> +			remaining_ms =
> +				schedule_timeout_uninterruptible(remaining_ms);
> +	}
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9d96447..2f82af4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1057,9 +1057,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. */
> +	wait_remaining_ms_from_jiffies(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)
> +{
> +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> +				       intel_dp->backlight_on_delay);
> +}
> +
> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> +{
> +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> +				       intel_dp->backlight_off_delay);
> +}

I think the naming here is a bit unfortunate. You call wait_backlight_on()
*before* you enable backlight, but wait_backlight_off() *after* you
disable backlight.

The wait_panel_{on,off} functions wait for some event to happen.

I think ironlake_wait_backlight_on() *sounds* like something to call
*after* you've enabled backlight. So instead, I think the function
should be called something like wait_power_on()... but then it gets
confusing with the wait_panel_on(). Ugh.

I don't know. I'll just say

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

because I think the patch looks good otherwise, and maybe someone will
come up with the Perfect Naming Scheme and send follow-up patches we can
bikeshed until the end of time...

>  
>  /* Read the current pp_control value, unlocking the register if it
>   * is locked
> @@ -1147,7 +1164,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;
>  
>  		intel_runtime_pm_put(dev_priv);
>  	}
> @@ -1222,6 +1239,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 */
> @@ -1242,6 +1260,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. */
> @@ -1252,6 +1272,7 @@ void ironlake_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;
>  	ironlake_wait_panel_off(intel_dp);
>  }
>  
> @@ -1273,7 +1294,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;
>  
> @@ -1305,7 +1326,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 46aea6c..92de688 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -485,6 +485,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;
>  };
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Daniel Vetter]

On Fri, Jan 17, 2014 at 03:09:58PM +0200, Jani Nikula wrote:
> On Thu, 19 Dec 2013, 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.
> > v4: - Rename function again.
> >     - Move function to i915_drv.h.
> >     - Store last_power_cycle at edp_panel_off too.
> >     - Use msecs_to_jiffies_timeout, then replace the msleep with an
> >       open-coded version that avoids the extra +1 jiffy.
> >     - Try to add units to every variable name so we don't confuse
> >       jiffies with milliseconds.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_drv.h |  3 +++
> >  3 files changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index cc8afff..7e9b436 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
> >  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> >  }
> >  
> > +/*
> > + * If you need to wait X milliseconds 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 this function and
> > + * pass the timestamp as the first argument, and X as the second argument.
> > + */
> > +static inline void
> > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> > +{
> > +	unsigned long target_jiffies, tmp_jiffies;
> > +	unsigned int remaining_ms;
> > +
> > +	/*
> > +	 * Don't re-read the value of "jiffies" every time since it may change
> > +	 * behind our back and break the math.
> > +	 */
> > +	tmp_jiffies = jiffies;
> > +	target_jiffies = timestamp_jiffies +
> > +			 msecs_to_jiffies_timeout(to_wait_ms);
> > +
> > +	if (time_after(target_jiffies, tmp_jiffies)) {
> > +		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> > +						(long)tmp_jiffies);
> > +		while (remaining_ms)
> > +			remaining_ms =
> > +				schedule_timeout_uninterruptible(remaining_ms);
> > +	}
> > +}
> > +
> >  #endif
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 9d96447..2f82af4 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1057,9 +1057,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. */
> > +	wait_remaining_ms_from_jiffies(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)
> > +{
> > +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> > +				       intel_dp->backlight_on_delay);
> > +}
> > +
> > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> > +{
> > +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> > +				       intel_dp->backlight_off_delay);
> > +}
> 
> I think the naming here is a bit unfortunate. You call wait_backlight_on()
> *before* you enable backlight, but wait_backlight_off() *after* you
> disable backlight.
> 
> The wait_panel_{on,off} functions wait for some event to happen.
> 
> I think ironlake_wait_backlight_on() *sounds* like something to call
> *after* you've enabled backlight. So instead, I think the function
> should be called something like wait_power_on()... but then it gets
> confusing with the wait_panel_on(). Ugh.
> 
> I don't know. I'll just say
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> because I think the patch looks good otherwise, and maybe someone will
> come up with the Perfect Naming Scheme and send follow-up patches we can
> bikeshed until the end of time...

I think a bikeshed patch on top to drop the ironlake_ prefixes would be
good, since this isn't anything hw specific at all. I'll do that when
applying. Otherwise no Clever Ideas for the actual names atm.
-Daniel

> 
> >  
> >  /* Read the current pp_control value, unlocking the register if it
> >   * is locked
> > @@ -1147,7 +1164,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;
> >  
> >  		intel_runtime_pm_put(dev_priv);
> >  	}
> > @@ -1222,6 +1239,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 */
> > @@ -1242,6 +1260,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. */
> > @@ -1252,6 +1272,7 @@ void ironlake_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;
> >  	ironlake_wait_panel_off(intel_dp);
> >  }
> >  
> > @@ -1273,7 +1294,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;
> >  
> > @@ -1305,7 +1326,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 46aea6c..92de688 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -485,6 +485,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;
> >  };
> > -- 
> > 1.8.3.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> 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 2/6] drm/i915: save some time when waiting the eDP timings

[Daniel Vetter]

On Fri, Jan 17, 2014 at 02:29:51PM +0100, Daniel Vetter wrote:
> On Fri, Jan 17, 2014 at 03:09:58PM +0200, Jani Nikula wrote:
> > On Thu, 19 Dec 2013, 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.
> > > v4: - Rename function again.
> > >     - Move function to i915_drv.h.
> > >     - Store last_power_cycle at edp_panel_off too.
> > >     - Use msecs_to_jiffies_timeout, then replace the msleep with an
> > >       open-coded version that avoids the extra +1 jiffy.
> > >     - Try to add units to every variable name so we don't confuse
> > >       jiffies with milliseconds.
> > >
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
> > >  drivers/gpu/drm/i915/intel_drv.h |  3 +++
> > >  3 files changed, 56 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index cc8afff..7e9b436 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
> > >  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> > >  }
> > >  
> > > +/*
> > > + * If you need to wait X milliseconds 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 this function and
> > > + * pass the timestamp as the first argument, and X as the second argument.
> > > + */
> > > +static inline void
> > > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> > > +{
> > > +	unsigned long target_jiffies, tmp_jiffies;
> > > +	unsigned int remaining_ms;
> > > +
> > > +	/*
> > > +	 * Don't re-read the value of "jiffies" every time since it may change
> > > +	 * behind our back and break the math.
> > > +	 */
> > > +	tmp_jiffies = jiffies;
> > > +	target_jiffies = timestamp_jiffies +
> > > +			 msecs_to_jiffies_timeout(to_wait_ms);
> > > +
> > > +	if (time_after(target_jiffies, tmp_jiffies)) {
> > > +		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> > > +						(long)tmp_jiffies);
> > > +		while (remaining_ms)
> > > +			remaining_ms =
> > > +				schedule_timeout_uninterruptible(remaining_ms);
> > > +	}
> > > +}
> > > +
> > >  #endif
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 9d96447..2f82af4 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1057,9 +1057,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. */
> > > +	wait_remaining_ms_from_jiffies(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)
> > > +{
> > > +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> > > +				       intel_dp->backlight_on_delay);
> > > +}
> > > +
> > > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> > > +{
> > > +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> > > +				       intel_dp->backlight_off_delay);
> > > +}
> > 
> > I think the naming here is a bit unfortunate. You call wait_backlight_on()
> > *before* you enable backlight, but wait_backlight_off() *after* you
> > disable backlight.
> > 
> > The wait_panel_{on,off} functions wait for some event to happen.
> > 
> > I think ironlake_wait_backlight_on() *sounds* like something to call
> > *after* you've enabled backlight. So instead, I think the function
> > should be called something like wait_power_on()... but then it gets
> > confusing with the wait_panel_on(). Ugh.
> > 
> > I don't know. I'll just say
> > 
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> > 
> > because I think the patch looks good otherwise, and maybe someone will
> > come up with the Perfect Naming Scheme and send follow-up patches we can
> > bikeshed until the end of time...
> 
> I think a bikeshed patch on top to drop the ironlake_ prefixes would be
> good, since this isn't anything hw specific at all. I'll do that when
> applying. Otherwise no Clever Ideas for the actual names atm.

I've merged the first two patches for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

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

The eDP code records a few timestamps containing the last time we took
some actions, because we need to wait before doing some other actions.
The problem is that if we store a timestamp when suspending and then
look at it when resuming, we'll ignore the unknown amount of time we
actually were suspended.

This happens with the panel power cycle delay: it's 500ms on my
machine, and it's delaying the resume sequence by 200ms due to a
timestamp we recorded before suspending. This patch should solve this
problem by resetting the timestamps, creating encoder->resume()
callbacks.

Q: Why don't we use DRM's connector->reset() callback?
A: Because it is also called then the driver is loaded, and it's on a point
where we have already used the panel, so we can't just reset any of the
timtestamps there.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c     |  1 +
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 5 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 61fb9fc..4aa42e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -649,6 +649,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 		mutex_lock(&dev->struct_mutex);
 
+		intel_encoder_resume(dev);
+
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1488b28..527e89c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1606,6 +1606,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
+	intel_encoder->resume = intel_dp_resume;
 
 	intel_dig_port->port = port;
 	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d1357a..869be78 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11096,6 +11096,17 @@ void i915_redisable_vga(struct drm_device *dev)
 	}
 }
 
+void intel_encoder_resume(struct drm_device *dev)
+{
+	struct intel_encoder *encoder;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (encoder->resume)
+			encoder->resume(encoder);
+	}
+}
+
 static void intel_modeset_readout_hw_state(struct drm_device *dev)
 {
 	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 2f82af4..a291977 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3393,6 +3393,26 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	}
 }
 
+void intel_dp_resume(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	unsigned long tmp_jiffies = jiffies;
+
+	if (!is_edp(intel_dp))
+		return;
+
+	/*
+	 * Reset everything, otherwise when suspend/resume gets very fast, we
+	 * delay the resume based on the values that were set when we were still
+	 * suspending.
+	 */
+	intel_dp->last_power_on = tmp_jiffies - intel_dp->backlight_on_delay;
+	intel_dp->last_power_cycle = tmp_jiffies -
+				     intel_dp->panel_power_cycle_delay;
+	intel_dp->last_backlight_off = tmp_jiffies -
+				       intel_dp->backlight_off_delay;
+}
+
 static void
 intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 				    struct intel_dp *intel_dp,
@@ -3784,6 +3804,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	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;
+	intel_encoder->resume = intel_dp_resume;
 	if (IS_VALLEYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = vlv_dp_pre_pll_enable;
 		intel_encoder->pre_enable = vlv_pre_enable_dp;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 92de688..8fb6dbe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -149,6 +149,7 @@ struct intel_encoder {
 	 * be set correctly before calling this function. */
 	void (*get_config)(struct intel_encoder *,
 			   struct intel_crtc_config *pipe_config);
+	void (*resume)(struct intel_encoder *);
 	int crtc_mask;
 	enum hpd_pin hpd_pin;
 };
@@ -711,6 +712,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_encoder_resume(struct drm_device *dev);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
@@ -734,6 +736,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
+void intel_dp_resume(struct intel_encoder *encoder);
 
 
 /* intel_dsi.c */
-- 
1.8.3.1

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Jesse Barnes]

On Thu, 19 Dec 2013 14:29:41 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> +	 * Reset everything, otherwise when suspend/resume gets very fast, we
> +	 * delay the resume based on the values that were set when we were still
> +	 * suspending.
> +	 */
> +	intel_dp->last_power_on = tmp_jiffies - intel_dp->backlight_on_delay;
> +	intel_dp->last_power_cycle = tmp_jiffies -
> +				     intel_dp->panel_power_cycle_delay;
> +	intel_dp->last_backlight_off = tmp_jiffies -
> +				       intel_dp->backlight_off_delay;

I'm not clear on this; aren't the _delay fields in ms?  Here you're
subtracting ms units from jiffies units.  Don't you have to convert?
Or did I miss a unit change in an earlier patch?

Thanks,
Jesse

[PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

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

The eDP code records a few timestamps containing the last time we took
some actions, because we need to wait before doing some other actions.
The problem is that if we store a timestamp when suspending and then
look at it when resuming, we'll ignore the unknown amount of time we
actually were suspended.

This happens with the panel power cycle delay: it's 500ms on my
machine, and it's delaying the resume sequence by 200ms due to a
timestamp we recorded before suspending. This patch should solve this
problem by resetting the timestamps, creating encoder->resume()
callbacks.

Q: Why don't we use DRM's connector->reset() callback?
A: Because it is also called then the driver is loaded, and it's on a point
where we have already used the panel, so we can't just reset any of the
timtestamps there.

v2: - Fix the madatory jiffies/milliseconds bug.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c     |  1 +
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 5 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 61fb9fc..4aa42e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -649,6 +649,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 		mutex_lock(&dev->struct_mutex);
 
+		intel_encoder_resume(dev);
+
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1488b28..527e89c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1606,6 +1606,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
+	intel_encoder->resume = intel_dp_resume;
 
 	intel_dig_port->port = port;
 	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d1357a..869be78 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11096,6 +11096,17 @@ void i915_redisable_vga(struct drm_device *dev)
 	}
 }
 
+void intel_encoder_resume(struct drm_device *dev)
+{
+	struct intel_encoder *encoder;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (encoder->resume)
+			encoder->resume(encoder);
+	}
+}
+
 static void intel_modeset_readout_hw_state(struct drm_device *dev)
 {
 	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 2f82af4..dafb204 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3393,6 +3393,27 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	}
 }
 
+void intel_dp_resume(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	unsigned long tmp_jiffies = jiffies;
+
+	if (!is_edp(intel_dp))
+		return;
+
+	/*
+	 * Reset everything, otherwise when suspend/resume gets very fast, we
+	 * delay the resume based on the values that were set when we were still
+	 * suspending.
+	 */
+	intel_dp->last_power_on = tmp_jiffies -
+			msecs_to_jiffies(intel_dp->backlight_on_delay);
+	intel_dp->last_power_cycle = tmp_jiffies -
+			msecs_to_jiffies(intel_dp->panel_power_cycle_delay);
+	intel_dp->last_backlight_off = tmp_jiffies -
+			msecs_to_jiffies(intel_dp->backlight_off_delay);
+}
+
 static void
 intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 				    struct intel_dp *intel_dp,
@@ -3784,6 +3805,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	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;
+	intel_encoder->resume = intel_dp_resume;
 	if (IS_VALLEYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = vlv_dp_pre_pll_enable;
 		intel_encoder->pre_enable = vlv_pre_enable_dp;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 92de688..8fb6dbe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -149,6 +149,7 @@ struct intel_encoder {
 	 * be set correctly before calling this function. */
 	void (*get_config)(struct intel_encoder *,
 			   struct intel_crtc_config *pipe_config);
+	void (*resume)(struct intel_encoder *);
 	int crtc_mask;
 	enum hpd_pin hpd_pin;
 };
@@ -711,6 +712,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_encoder_resume(struct drm_device *dev);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
@@ -734,6 +736,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
+void intel_dp_resume(struct intel_encoder *encoder);
 
 
 /* intel_dsi.c */
-- 
1.8.4.2

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Jesse Barnes]

On Fri,  3 Jan 2014 17:46:40 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The eDP code records a few timestamps containing the last time we took
> some actions, because we need to wait before doing some other actions.
> The problem is that if we store a timestamp when suspending and then
> look at it when resuming, we'll ignore the unknown amount of time we
> actually were suspended.
> 
> This happens with the panel power cycle delay: it's 500ms on my
> machine, and it's delaying the resume sequence by 200ms due to a
> timestamp we recorded before suspending. This patch should solve this
> problem by resetting the timestamps, creating encoder->resume()
> callbacks.
> 
> Q: Why don't we use DRM's connector->reset() callback?
> A: Because it is also called then the driver is loaded, and it's on a point
> where we have already used the panel, so we can't just reset any of the
> timtestamps there.
> 
> v2: - Fix the madatory jiffies/milliseconds bug.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c     |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  5 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 61fb9fc..4aa42e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -649,6 +649,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  
>  		mutex_lock(&dev->struct_mutex);
>  
> +		intel_encoder_resume(dev);
> +
>  		error = i915_gem_init_hw(dev);
>  		mutex_unlock(&dev->struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1488b28..527e89c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1606,6 +1606,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	intel_encoder->post_disable = intel_ddi_post_disable;
>  	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
>  	intel_encoder->get_config = intel_ddi_get_config;
> +	intel_encoder->resume = intel_dp_resume;
>  
>  	intel_dig_port->port = port;
>  	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d1357a..869be78 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11096,6 +11096,17 @@ void i915_redisable_vga(struct drm_device *dev)
>  	}
>  }
>  
> +void intel_encoder_resume(struct drm_device *dev)
> +{
> +	struct intel_encoder *encoder;
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +			    base.head) {
> +		if (encoder->resume)
> +			encoder->resume(encoder);
> +	}
> +}
> +
>  static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  {
>  	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 2f82af4..dafb204 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3393,6 +3393,27 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>  	}
>  }
>  
> +void intel_dp_resume(struct intel_encoder *encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	unsigned long tmp_jiffies = jiffies;
> +
> +	if (!is_edp(intel_dp))
> +		return;
> +
> +	/*
> +	 * Reset everything, otherwise when suspend/resume gets very fast, we
> +	 * delay the resume based on the values that were set when we were still
> +	 * suspending.
> +	 */
> +	intel_dp->last_power_on = tmp_jiffies -
> +			msecs_to_jiffies(intel_dp->backlight_on_delay);
> +	intel_dp->last_power_cycle = tmp_jiffies -
> +			msecs_to_jiffies(intel_dp->panel_power_cycle_delay);
> +	intel_dp->last_backlight_off = tmp_jiffies -
> +			msecs_to_jiffies(intel_dp->backlight_off_delay);
> +}
> +
>  static void
>  intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>  				    struct intel_dp *intel_dp,
> @@ -3784,6 +3805,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	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;
> +	intel_encoder->resume = intel_dp_resume;
>  	if (IS_VALLEYVIEW(dev)) {
>  		intel_encoder->pre_pll_enable = vlv_dp_pre_pll_enable;
>  		intel_encoder->pre_enable = vlv_pre_enable_dp;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 92de688..8fb6dbe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -149,6 +149,7 @@ struct intel_encoder {
>  	 * be set correctly before calling this function. */
>  	void (*get_config)(struct intel_encoder *,
>  			   struct intel_crtc_config *pipe_config);
> +	void (*resume)(struct intel_encoder *);
>  	int crtc_mask;
>  	enum hpd_pin hpd_pin;
>  };
> @@ -711,6 +712,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_encoder_resume(struct drm_device *dev);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> @@ -734,6 +736,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  void intel_edp_psr_update(struct drm_device *dev);
> +void intel_dp_resume(struct intel_encoder *encoder);
>  
>  
>  /* intel_dsi.c */

Looks good.  I wonder if the resume call could be put into
modeset_init_hw somehow, or maybe a new modeset_resume_hw routine or
something, since the resume placement above the gem init seems odd, but
that's not a blocker by any means.

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

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Daniel Vetter]

On Wed, Jan 15, 2014 at 10:21:56AM -0800, Jesse Barnes wrote:
> On Fri,  3 Jan 2014 17:46:40 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > The eDP code records a few timestamps containing the last time we took
> > some actions, because we need to wait before doing some other actions.
> > The problem is that if we store a timestamp when suspending and then
> > look at it when resuming, we'll ignore the unknown amount of time we
> > actually were suspended.
> > 
> > This happens with the panel power cycle delay: it's 500ms on my
> > machine, and it's delaying the resume sequence by 200ms due to a
> > timestamp we recorded before suspending. This patch should solve this
> > problem by resetting the timestamps, creating encoder->resume()
> > callbacks.
> > 
> > Q: Why don't we use DRM's connector->reset() callback?
> > A: Because it is also called then the driver is loaded, and it's on a point
> > where we have already used the panel, so we can't just reset any of the
> > timtestamps there.
> > 
> > v2: - Fix the madatory jiffies/milliseconds bug.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c      |  2 ++
> >  drivers/gpu/drm/i915/intel_ddi.c     |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 22 ++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
> >  5 files changed, 39 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 61fb9fc..4aa42e5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -649,6 +649,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> >  
> >  		mutex_lock(&dev->struct_mutex);
> >  
> > +		intel_encoder_resume(dev);
> > +
> >  		error = i915_gem_init_hw(dev);
> >  		mutex_unlock(&dev->struct_mutex);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1488b28..527e89c 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1606,6 +1606,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
> >  	intel_encoder->post_disable = intel_ddi_post_disable;
> >  	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
> >  	intel_encoder->get_config = intel_ddi_get_config;
> > +	intel_encoder->resume = intel_dp_resume;
> >  
> >  	intel_dig_port->port = port;
> >  	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4d1357a..869be78 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11096,6 +11096,17 @@ void i915_redisable_vga(struct drm_device *dev)
> >  	}
> >  }
> >  
> > +void intel_encoder_resume(struct drm_device *dev)
> > +{
> > +	struct intel_encoder *encoder;
> > +
> > +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> > +			    base.head) {
> > +		if (encoder->resume)
> > +			encoder->resume(encoder);
> > +	}
> > +}
> > +
> >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  {
> >  	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 2f82af4..dafb204 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3393,6 +3393,27 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> >  	}
> >  }
> >  
> > +void intel_dp_resume(struct intel_encoder *encoder)
> > +{
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > +	unsigned long tmp_jiffies = jiffies;
> > +
> > +	if (!is_edp(intel_dp))
> > +		return;
> > +
> > +	/*
> > +	 * Reset everything, otherwise when suspend/resume gets very fast, we
> > +	 * delay the resume based on the values that were set when we were still
> > +	 * suspending.
> > +	 */
> > +	intel_dp->last_power_on = tmp_jiffies -
> > +			msecs_to_jiffies(intel_dp->backlight_on_delay);
> > +	intel_dp->last_power_cycle = tmp_jiffies -
> > +			msecs_to_jiffies(intel_dp->panel_power_cycle_delay);
> > +	intel_dp->last_backlight_off = tmp_jiffies -
> > +			msecs_to_jiffies(intel_dp->backlight_off_delay);
> > +}
> > +
> >  static void
> >  intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> >  				    struct intel_dp *intel_dp,
> > @@ -3784,6 +3805,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >  	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;
> > +	intel_encoder->resume = intel_dp_resume;
> >  	if (IS_VALLEYVIEW(dev)) {
> >  		intel_encoder->pre_pll_enable = vlv_dp_pre_pll_enable;
> >  		intel_encoder->pre_enable = vlv_pre_enable_dp;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 92de688..8fb6dbe 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -149,6 +149,7 @@ struct intel_encoder {
> >  	 * be set correctly before calling this function. */
> >  	void (*get_config)(struct intel_encoder *,
> >  			   struct intel_crtc_config *pipe_config);
> > +	void (*resume)(struct intel_encoder *);
> >  	int crtc_mask;
> >  	enum hpd_pin hpd_pin;
> >  };
> > @@ -711,6 +712,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_encoder_resume(struct drm_device *dev);
> >  
> >  /* intel_dp.c */
> >  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> > @@ -734,6 +736,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
> >  void intel_edp_psr_enable(struct intel_dp *intel_dp);
> >  void intel_edp_psr_disable(struct intel_dp *intel_dp);
> >  void intel_edp_psr_update(struct drm_device *dev);
> > +void intel_dp_resume(struct intel_encoder *encoder);
> >  
> >  
> >  /* intel_dsi.c */
> 
> Looks good.  I wonder if the resume call could be put into
> modeset_init_hw somehow, or maybe a new modeset_resume_hw routine or
> something, since the resume placement above the gem init seems odd, but
> that's not a blocker by any means.

Sorry I've missed this thus far, but we have such a callback already. It's
the helper->reset thing and e.g. already used by the vga code for the
hotplug setup. Adding a new resume callback for encoders shouldn't be
necessary I think.
-Daniel

> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> 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 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

2014/1/15 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jan 15, 2014 at 10:21:56AM -0800, Jesse Barnes wrote:
>> On Fri,  3 Jan 2014 17:46:40 -0200
>> Paulo Zanoni <przanoni@gmail.com> wrote:
>>
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > The eDP code records a few timestamps containing the last time we took
>> > some actions, because we need to wait before doing some other actions.
>> > The problem is that if we store a timestamp when suspending and then
>> > look at it when resuming, we'll ignore the unknown amount of time we
>> > actually were suspended.
>> >
>> > This happens with the panel power cycle delay: it's 500ms on my
>> > machine, and it's delaying the resume sequence by 200ms due to a
>> > timestamp we recorded before suspending. This patch should solve this
>> > problem by resetting the timestamps, creating encoder->resume()
>> > callbacks.
>> >
>> > Q: Why don't we use DRM's connector->reset() callback?
>> > A: Because it is also called then the driver is loaded, and it's on a point
>> > where we have already used the panel, so we can't just reset any of the
>> > timtestamps there.
>> >
>> > v2: - Fix the madatory jiffies/milliseconds bug.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c      |  2 ++
>> >  drivers/gpu/drm/i915/intel_ddi.c     |  1 +
>> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>> >  drivers/gpu/drm/i915/intel_dp.c      | 22 ++++++++++++++++++++++
>> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>> >  5 files changed, 39 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index 61fb9fc..4aa42e5 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -649,6 +649,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>> >
>> >             mutex_lock(&dev->struct_mutex);
>> >
>> > +           intel_encoder_resume(dev);
>> > +
>> >             error = i915_gem_init_hw(dev);
>> >             mutex_unlock(&dev->struct_mutex);
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> > index 1488b28..527e89c 100644
>> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > @@ -1606,6 +1606,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>> >     intel_encoder->post_disable = intel_ddi_post_disable;
>> >     intel_encoder->get_hw_state = intel_ddi_get_hw_state;
>> >     intel_encoder->get_config = intel_ddi_get_config;
>> > +   intel_encoder->resume = intel_dp_resume;
>> >
>> >     intel_dig_port->port = port;
>> >     intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 4d1357a..869be78 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -11096,6 +11096,17 @@ void i915_redisable_vga(struct drm_device *dev)
>> >     }
>> >  }
>> >
>> > +void intel_encoder_resume(struct drm_device *dev)
>> > +{
>> > +   struct intel_encoder *encoder;
>> > +
>> > +   list_for_each_entry(encoder, &dev->mode_config.encoder_list,
>> > +                       base.head) {
>> > +           if (encoder->resume)
>> > +                   encoder->resume(encoder);
>> > +   }
>> > +}
>> > +
>> >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
>> >  {
>> >     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 2f82af4..dafb204 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -3393,6 +3393,27 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>> >     }
>> >  }
>> >
>> > +void intel_dp_resume(struct intel_encoder *encoder)
>> > +{
>> > +   struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>> > +   unsigned long tmp_jiffies = jiffies;
>> > +
>> > +   if (!is_edp(intel_dp))
>> > +           return;
>> > +
>> > +   /*
>> > +    * Reset everything, otherwise when suspend/resume gets very fast, we
>> > +    * delay the resume based on the values that were set when we were still
>> > +    * suspending.
>> > +    */
>> > +   intel_dp->last_power_on = tmp_jiffies -
>> > +                   msecs_to_jiffies(intel_dp->backlight_on_delay);
>> > +   intel_dp->last_power_cycle = tmp_jiffies -
>> > +                   msecs_to_jiffies(intel_dp->panel_power_cycle_delay);
>> > +   intel_dp->last_backlight_off = tmp_jiffies -
>> > +                   msecs_to_jiffies(intel_dp->backlight_off_delay);
>> > +}
>> > +
>> >  static void
>> >  intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>> >                                 struct intel_dp *intel_dp,
>> > @@ -3784,6 +3805,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>> >     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;
>> > +   intel_encoder->resume = intel_dp_resume;
>> >     if (IS_VALLEYVIEW(dev)) {
>> >             intel_encoder->pre_pll_enable = vlv_dp_pre_pll_enable;
>> >             intel_encoder->pre_enable = vlv_pre_enable_dp;
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 92de688..8fb6dbe 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -149,6 +149,7 @@ struct intel_encoder {
>> >      * be set correctly before calling this function. */
>> >     void (*get_config)(struct intel_encoder *,
>> >                        struct intel_crtc_config *pipe_config);
>> > +   void (*resume)(struct intel_encoder *);
>> >     int crtc_mask;
>> >     enum hpd_pin hpd_pin;
>> >  };
>> > @@ -711,6 +712,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_encoder_resume(struct drm_device *dev);
>> >
>> >  /* intel_dp.c */
>> >  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
>> > @@ -734,6 +736,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>> >  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>> >  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>> >  void intel_edp_psr_update(struct drm_device *dev);
>> > +void intel_dp_resume(struct intel_encoder *encoder);
>> >
>> >
>> >  /* intel_dsi.c */
>>
>> Looks good.  I wonder if the resume call could be put into
>> modeset_init_hw somehow, or maybe a new modeset_resume_hw routine or
>> something, since the resume placement above the gem init seems odd, but
>> that's not a blocker by any means.
>
> Sorry I've missed this thus far, but we have such a callback already. It's
> the helper->reset thing and e.g. already used by the vga code for the
> hotplug setup. Adding a new resume callback for encoders shouldn't be
> necessary I think.

I remember you said this on IRC, then I tried it and failed. Please
check the commit message :)


> -Daniel
>
>>
>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>> --
>> Jesse Barnes, Intel Open Source Technology Center
>> _______________________________________________
>> 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



-- 
Paulo Zanoni

[PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

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

The eDP code records a few timestamps containing the last time we took
some actions, because we need to wait before doing some other actions.
The problem is that if we store a timestamp when suspending and then
look at it when resuming, we'll ignore the unknown amount of time we
actually were suspended.

This happens with the panel power cycle delay: it's 500ms on my
machine, and it's delaying the resume sequence by 200ms due to a
timestamp we recorded before suspending. This patch should solve this
problem by resetting the timestamps.

v2: - Fix the madatory jiffies/milliseconds bug.
v3: - We can use drm_connector->reset after Daniel's recent refactor.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 55c43cf..001fb06 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3302,12 +3302,34 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_dig_port);
 }
 
+void intel_dp_reset(struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	unsigned long tmp_jiffies = jiffies;
+
+	if (!is_edp(intel_dp))
+		return;
+
+	/*
+	 * Reset everything, otherwise when suspend/resume gets very fast, we
+	 * delay the resume based on the values that were set when we were still
+	 * suspending.
+	 */
+	intel_dp->last_power_on = tmp_jiffies -
+			msecs_to_jiffies(intel_dp->backlight_on_delay);
+	intel_dp->last_power_cycle = tmp_jiffies -
+			msecs_to_jiffies(intel_dp->panel_power_cycle_delay);
+	intel_dp->last_backlight_off = tmp_jiffies -
+			msecs_to_jiffies(intel_dp->backlight_off_delay);
+}
+
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_set_property,
 	.destroy = intel_dp_connector_destroy,
+	.reset = intel_dp_reset,
 };
 
 static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = {
-- 
1.8.4.2

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Chris Wilson]

On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The eDP code records a few timestamps containing the last time we took
> some actions, because we need to wait before doing some other actions.
> The problem is that if we store a timestamp when suspending and then
> look at it when resuming, we'll ignore the unknown amount of time we
> actually were suspended.
> 
> This happens with the panel power cycle delay: it's 500ms on my
> machine, and it's delaying the resume sequence by 200ms due to a
> timestamp we recorded before suspending. This patch should solve this
> problem by resetting the timestamps.

But you don't explain why this is safe. The code nerfs the timeouts so
that they are ignored, yet the delays are independent. Should this be
based on realtime rather than jiffies?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The eDP code records a few timestamps containing the last time we took
>> some actions, because we need to wait before doing some other actions.
>> The problem is that if we store a timestamp when suspending and then
>> look at it when resuming, we'll ignore the unknown amount of time we
>> actually were suspended.
>>
>> This happens with the panel power cycle delay: it's 500ms on my
>> machine, and it's delaying the resume sequence by 200ms due to a
>> timestamp we recorded before suspending. This patch should solve this
>> problem by resetting the timestamps.
>
> But you don't explain why this is safe. The code nerfs the timeouts so
> that they are ignored, yet the delays are independent. Should this be
> based on realtime rather than jiffies?

I'm not sure I understand your question. What's the problem you see exactly?

Thanks,
Paulo

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Chris Wilson]

On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote:
> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> The eDP code records a few timestamps containing the last time we took
> >> some actions, because we need to wait before doing some other actions.
> >> The problem is that if we store a timestamp when suspending and then
> >> look at it when resuming, we'll ignore the unknown amount of time we
> >> actually were suspended.
> >>
> >> This happens with the panel power cycle delay: it's 500ms on my
> >> machine, and it's delaying the resume sequence by 200ms due to a
> >> timestamp we recorded before suspending. This patch should solve this
> >> problem by resetting the timestamps.
> >
> > But you don't explain why this is safe. The code nerfs the timeouts so
> > that they are ignored, yet the delays are independent. Should this be
> > based on realtime rather than jiffies?
> 
> I'm not sure I understand your question. What's the problem you see exactly?

Given the fast suspend & resume, we will not have waited the required
panel off time before poking it again etc. What makes that safe?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Daniel Vetter]

On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote:
>> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> The eDP code records a few timestamps containing the last time we took
>> >> some actions, because we need to wait before doing some other actions.
>> >> The problem is that if we store a timestamp when suspending and then
>> >> look at it when resuming, we'll ignore the unknown amount of time we
>> >> actually were suspended.
>> >>
>> >> This happens with the panel power cycle delay: it's 500ms on my
>> >> machine, and it's delaying the resume sequence by 200ms due to a
>> >> timestamp we recorded before suspending. This patch should solve this
>> >> problem by resetting the timestamps.
>> >
>> > But you don't explain why this is safe. The code nerfs the timeouts so
>> > that they are ignored, yet the delays are independent. Should this be
>> > based on realtime rather than jiffies?
>>
>> I'm not sure I understand your question. What's the problem you see exactly?
>
> Given the fast suspend & resume, we will not have waited the required
> panel off time before poking it again etc. What makes that safe?

Even worse the kernel might abort the suspend due to some issue and
we'll immediately resume. Also, and immediate thaw operation after
freezing is how hibernate works. Iirc the hw always enforces the full
power off delay after a power reset for exactly this reason (at least
on current platforms afaik). But with the minimal delays in patch 6
that won't help any more, either.

I fear we need a bit more smarts here using a realtime clock source to
figure out whether actually sufficient time elapsed :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Paulo Zanoni]

2014/1/17 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote:
>>> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>:
>>> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
>>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> >>
>>> >> The eDP code records a few timestamps containing the last time we took
>>> >> some actions, because we need to wait before doing some other actions.
>>> >> The problem is that if we store a timestamp when suspending and then
>>> >> look at it when resuming, we'll ignore the unknown amount of time we
>>> >> actually were suspended.
>>> >>
>>> >> This happens with the panel power cycle delay: it's 500ms on my
>>> >> machine, and it's delaying the resume sequence by 200ms due to a
>>> >> timestamp we recorded before suspending. This patch should solve this
>>> >> problem by resetting the timestamps.
>>> >
>>> > But you don't explain why this is safe. The code nerfs the timeouts so
>>> > that they are ignored, yet the delays are independent. Should this be
>>> > based on realtime rather than jiffies?
>>>
>>> I'm not sure I understand your question. What's the problem you see exactly?
>>
>> Given the fast suspend & resume, we will not have waited the required
>> panel off time before poking it again etc. What makes that safe?
>
> Even worse the kernel might abort the suspend due to some issue and
> we'll immediately resume. Also, and immediate thaw operation after
> freezing is how hibernate works. Iirc the hw always enforces the full
> power off delay after a power reset for exactly this reason (at least
> on current platforms afaik).

Oh, now I get it. My bad. A brief experiment here shows that
do_gettimeofday or current_kernel_time can probably be used to fix the
problem. But since that requires properly getting all the timestamps
in the correct units, maybe rewriting some functions, I'll wait a few
days before I come back to this problem.

> But with the minimal delays in patch 6
> that won't help any more, either.

Patch 6 should be independent of this. This patch is just to save us
some time in the resume cases, but patch 6 is to correct the amount of
time we wait while we disable the panel. I don't see a reason to block
patch 6 on this one.

Thanks for the reviews,
Paulo

>
> I fear we need a bit more smarts here using a realtime clock source to
> figure out whether actually sufficient time elapsed :(
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: reset eDP timestamps on resume

[Daniel Vetter]

On Mon, Jan 20, 2014 at 01:47:51PM -0200, Paulo Zanoni wrote:
> 2014/1/17 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote:
> >>> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>:
> >>> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote:
> >>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> >>
> >>> >> The eDP code records a few timestamps containing the last time we took
> >>> >> some actions, because we need to wait before doing some other actions.
> >>> >> The problem is that if we store a timestamp when suspending and then
> >>> >> look at it when resuming, we'll ignore the unknown amount of time we
> >>> >> actually were suspended.
> >>> >>
> >>> >> This happens with the panel power cycle delay: it's 500ms on my
> >>> >> machine, and it's delaying the resume sequence by 200ms due to a
> >>> >> timestamp we recorded before suspending. This patch should solve this
> >>> >> problem by resetting the timestamps.
> >>> >
> >>> > But you don't explain why this is safe. The code nerfs the timeouts so
> >>> > that they are ignored, yet the delays are independent. Should this be
> >>> > based on realtime rather than jiffies?
> >>>
> >>> I'm not sure I understand your question. What's the problem you see exactly?
> >>
> >> Given the fast suspend & resume, we will not have waited the required
> >> panel off time before poking it again etc. What makes that safe?
> >
> > Even worse the kernel might abort the suspend due to some issue and
> > we'll immediately resume. Also, and immediate thaw operation after
> > freezing is how hibernate works. Iirc the hw always enforces the full
> > power off delay after a power reset for exactly this reason (at least
> > on current platforms afaik).
> 
> Oh, now I get it. My bad. A brief experiment here shows that
> do_gettimeofday or current_kernel_time can probably be used to fix the
> problem. But since that requires properly getting all the timestamps
> in the correct units, maybe rewriting some functions, I'll wait a few
> days before I come back to this problem.

A simpler solution might be to grab timestamps in our resume code both
right at the end of our freeze function and at the beginning of thaw. Then
the driver code could convert this once into
dev->jiffies_elapsed_in_suspend or something and the dp->reset function
could simply use that to adjust the timeout values a bit (instead of
completely resetting everything).

The tricky bit should only be how to get the off-by-one stuff right: For
timeouts/waits we need to round up (+1), for this elapsed time we need to
round down one jiffy i.e. -1, but not less than 0 ofc ;-)

So I think you should be able to fix up your patch with very little work
(and no changes to the logic you've already created&tested).

> > But with the minimal delays in patch 6
> > that won't help any more, either.
> 
> Patch 6 should be independent of this. This patch is just to save us
> some time in the resume cases, but patch 6 is to correct the amount of
> time we wait while we disable the panel. I don't see a reason to block
> patch 6 on this one.

Hm yeah, with the new logic (but without the reset on resume) we should
still be save. I'll merge it, together with my prep patch to move the
reset callbacks around a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 4/6] drm/i915: remove a column of zeros from the eDP wait definitions

[Paulo Zanoni]

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

I like how the macros are nicely column-aligned, so we can properly
compare what each macro waits for, but a column full of zeroes doesn't
really help anything: it just makes the lines bigger, and they're
already way past 80 columns. I imagine this column was used in the
past, but IMHO now we can get rid of it.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a291977..a6a4c4f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1008,14 +1008,14 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
 		ironlake_set_pll_cpu_edp(intel_dp);
 }
 
-#define IDLE_ON_MASK		(PP_ON | 0 	  | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
-#define IDLE_ON_VALUE   	(PP_ON | 0 	  | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
+#define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
 
-#define IDLE_OFF_MASK		(PP_ON | 0        | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
-#define IDLE_OFF_VALUE		(0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
 
-#define IDLE_CYCLE_MASK		(PP_ON | 0        | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
-#define IDLE_CYCLE_VALUE	(0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+#define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
+#define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
 
 static void ironlake_wait_panel_status(struct intel_dp *intel_dp,
 				       u32 mask,
-- 
1.8.3.1

Re: [PATCH 4/6] drm/i915: remove a column of zeros from the eDP wait definitions

[Jesse Barnes]

On Thu, 19 Dec 2013 14:29:42 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I like how the macros are nicely column-aligned, so we can properly
> compare what each macro waits for, but a column full of zeroes doesn't
> really help anything: it just makes the lines bigger, and they're
> already way past 80 columns. I imagine this column was used in the
> past, but IMHO now we can get rid of it.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a291977..a6a4c4f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1008,14 +1008,14 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
>  		ironlake_set_pll_cpu_edp(intel_dp);
>  }
>  
> -#define IDLE_ON_MASK		(PP_ON | 0 	  | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> -#define IDLE_ON_VALUE   	(PP_ON | 0 	  | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
> +#define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> +#define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
>  
> -#define IDLE_OFF_MASK		(PP_ON | 0        | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> -#define IDLE_OFF_VALUE		(0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> +#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> +#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
>  
> -#define IDLE_CYCLE_MASK		(PP_ON | 0        | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
> -#define IDLE_CYCLE_VALUE	(0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> +#define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
> +#define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
>  
>  static void ironlake_wait_panel_status(struct intel_dp *intel_dp,
>  				       u32 mask,

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

[PATCH 5/6] drm/i915: don't wait for power cycle when waiting for power off

[Paulo Zanoni]

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

Function ironlake_wait_panel_off should just wait for the power off
delay, while function ironlake_wait_panel_power_cycle should wait for
the panel cycle (that's required after we turn the panel off, before
we enable it again).

The problem is that, currently, ironlake_wait_panel_off is waiting not
just for the panel to be off, but also for the power cycle delay and
the backlight off delay. This function relies on the PP_STATUS bits
3:0, which are not documented and not supposed to be used. A quick
analysis of the values we get while waiting quickly shows that power
off is reached while bits 3:0 are still 0x1, and the time it takes to
become 0x0 is the power cycle delay.

On my system with backlight off delay of 200ms, power down delay of
50ms and power cycle delay of 500ms, this is what I get:
 - Start waiting with value 0x80000008, timestamp 6.429364.
 - Jumps to 0xa0000003, timestamp 6.431360 (time waited: 0.001996)
 - Jumps to 0xa0000002, timestamp 6.631277 (time waited: 0.201913)
 - Jumps to 0x08000001, timestamp 6.681258 (time waited: 0.251894)
 - Jumps to 0x00000000, timestamp 7.192012 (time waited: 0.762648)

As you can see, ironlake_wait_panel_off is sleeping 760ms instead of
the expected 50ms: the first 200ms matches the backlight off delay
(which we should already have waited for!), then the 50ms for the real
panel off delay, then the 500ms for the panel power cycle.

This patch makes is look just at bits 31 and 29:28, which will ignore
the panel power cycle.

And just to be clear: this saves 500ms on my system every time we
disable the panel. But we can still save 200ms more (the backlight off
delay) on the next patches.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a6a4c4f..69d8f1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1011,8 +1011,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
 #define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
 #define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
 
-#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
-#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | 0)
+#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | 0)
 
 #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
 #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
-- 
1.8.3.1

Re: [PATCH 5/6] drm/i915: don't wait for power cycle when waiting for power off

[Jesse Barnes]

On Thu, 19 Dec 2013 14:29:43 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Function ironlake_wait_panel_off should just wait for the power off
> delay, while function ironlake_wait_panel_power_cycle should wait for
> the panel cycle (that's required after we turn the panel off, before
> we enable it again).
> 
> The problem is that, currently, ironlake_wait_panel_off is waiting not
> just for the panel to be off, but also for the power cycle delay and
> the backlight off delay. This function relies on the PP_STATUS bits
> 3:0, which are not documented and not supposed to be used. A quick
> analysis of the values we get while waiting quickly shows that power
> off is reached while bits 3:0 are still 0x1, and the time it takes to
> become 0x0 is the power cycle delay.
> 
> On my system with backlight off delay of 200ms, power down delay of
> 50ms and power cycle delay of 500ms, this is what I get:
>  - Start waiting with value 0x80000008, timestamp 6.429364.
>  - Jumps to 0xa0000003, timestamp 6.431360 (time waited: 0.001996)
>  - Jumps to 0xa0000002, timestamp 6.631277 (time waited: 0.201913)
>  - Jumps to 0x08000001, timestamp 6.681258 (time waited: 0.251894)
>  - Jumps to 0x00000000, timestamp 7.192012 (time waited: 0.762648)
> 
> As you can see, ironlake_wait_panel_off is sleeping 760ms instead of
> the expected 50ms: the first 200ms matches the backlight off delay
> (which we should already have waited for!), then the 50ms for the real
> panel off delay, then the 500ms for the panel power cycle.
> 
> This patch makes is look just at bits 31 and 29:28, which will ignore
> the panel power cycle.
> 
> And just to be clear: this saves 500ms on my system every time we
> disable the panel. But we can still save 200ms more (the backlight off
> delay) on the next patches.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a6a4c4f..69d8f1c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1011,8 +1011,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
>  #define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
>  #define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
>  
> -#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> -#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> +#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | 0)
> +#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | 0)
>  
>  #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
>  #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)

It would be good to note confirmation from the hw guys in the commit
msg, and maybe get some data on an LVDS panel just to be sure, but
otherwise:

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

Re: [PATCH 5/6] drm/i915: don't wait for power cycle when waiting for power off

[Jesse Barnes]

On Thu, 19 Dec 2013 09:38:45 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu, 19 Dec 2013 14:29:43 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Function ironlake_wait_panel_off should just wait for the power off
> > delay, while function ironlake_wait_panel_power_cycle should wait for
> > the panel cycle (that's required after we turn the panel off, before
> > we enable it again).
> > 
> > The problem is that, currently, ironlake_wait_panel_off is waiting not
> > just for the panel to be off, but also for the power cycle delay and
> > the backlight off delay. This function relies on the PP_STATUS bits
> > 3:0, which are not documented and not supposed to be used. A quick
> > analysis of the values we get while waiting quickly shows that power
> > off is reached while bits 3:0 are still 0x1, and the time it takes to
> > become 0x0 is the power cycle delay.
> > 
> > On my system with backlight off delay of 200ms, power down delay of
> > 50ms and power cycle delay of 500ms, this is what I get:
> >  - Start waiting with value 0x80000008, timestamp 6.429364.
> >  - Jumps to 0xa0000003, timestamp 6.431360 (time waited: 0.001996)
> >  - Jumps to 0xa0000002, timestamp 6.631277 (time waited: 0.201913)
> >  - Jumps to 0x08000001, timestamp 6.681258 (time waited: 0.251894)
> >  - Jumps to 0x00000000, timestamp 7.192012 (time waited: 0.762648)
> > 
> > As you can see, ironlake_wait_panel_off is sleeping 760ms instead of
> > the expected 50ms: the first 200ms matches the backlight off delay
> > (which we should already have waited for!), then the 50ms for the real
> > panel off delay, then the 500ms for the panel power cycle.
> > 
> > This patch makes is look just at bits 31 and 29:28, which will ignore
> > the panel power cycle.
> > 
> > And just to be clear: this saves 500ms on my system every time we
> > disable the panel. But we can still save 200ms more (the backlight off
> > delay) on the next patches.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index a6a4c4f..69d8f1c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1011,8 +1011,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
> >  #define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> >  #define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
> >  
> > -#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> > -#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> > +#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | 0)
> > +#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | 0)
> >  
> >  #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
> >  #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> 
> It would be good to note confirmation from the hw guys in the commit
> msg, and maybe get some data on an LVDS panel just to be sure, but
> otherwise:

For the LVDS bit we'd need to apply similar changes on the LVDS side,
but I think that may already be pretty fast, so you can ignore that bit.

[PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Paulo Zanoni]

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

Because we already do the wait in software: see
ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.

For the "backlight on" delay, even BSpec says we need to program 0x1
to PP_ON_DELAYS 12:0.

For the "backlight off" delay, if we don't do the same thing, when we
call ironlake_wait_panel_off we'll end up waiting for the it again.

On my machine the off delay is 200ms, so we save this amount of time
whenever we disable the panel (e.g, suspend).

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 69d8f1c..90ff059 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
 	}
 
-	/* And finally store the new values in the power sequencer. */
+	/*
+	 * 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) |
-		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
-	pp_off = (seq->t9 << PANEL_LIGHT_OFF_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. */
-- 
1.8.3.1

Re: [PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Jesse Barnes]

On Thu, 19 Dec 2013 14:29:44 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because we already do the wait in software: see
> ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> 
> For the "backlight on" delay, even BSpec says we need to program 0x1
> to PP_ON_DELAYS 12:0.
> 
> For the "backlight off" delay, if we don't do the same thing, when we
> call ironlake_wait_panel_off we'll end up waiting for the it again.
> 
> On my machine the off delay is 200ms, so we save this amount of time
> whenever we disable the panel (e.g, suspend).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 69d8f1c..90ff059 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
>  	}
>  
> -	/* And finally store the new values in the power sequencer. */
> +	/*
> +	 * 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) |
> -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_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. */

Yay!

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

Re: [PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Daniel Vetter]

On Thu, Dec 19, 2013 at 09:39:12AM -0800, Jesse Barnes wrote:
> On Thu, 19 Dec 2013 14:29:44 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Because we already do the wait in software: see
> > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> > 
> > For the "backlight on" delay, even BSpec says we need to program 0x1
> > to PP_ON_DELAYS 12:0.
> > 
> > For the "backlight off" delay, if we don't do the same thing, when we
> > call ironlake_wait_panel_off we'll end up waiting for the it again.
> > 
> > On my machine the off delay is 200ms, so we save this amount of time
> > whenever we disable the panel (e.g, suspend).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 69d8f1c..90ff059 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> >  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> >  	}
> >  
> > -	/* And finally store the new values in the power sequencer. */
> > +	/*
> > +	 * 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) |
> > -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> > -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_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. */
> 
> Yay!
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I'll wait with this one until the updated version of patch 3 is finalized,
but I've meanwhile pulled in patches 4&5.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Daniel Vetter]

On Fri, Jan 17, 2014 at 02:57:42PM +0100, Daniel Vetter wrote:
> On Thu, Dec 19, 2013 at 09:39:12AM -0800, Jesse Barnes wrote:
> > On Thu, 19 Dec 2013 14:29:44 -0200
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> > 
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > Because we already do the wait in software: see
> > > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> > > 
> > > For the "backlight on" delay, even BSpec says we need to program 0x1
> > > to PP_ON_DELAYS 12:0.
> > > 
> > > For the "backlight off" delay, if we don't do the same thing, when we
> > > call ironlake_wait_panel_off we'll end up waiting for the it again.
> > > 
> > > On my machine the off delay is 200ms, so we save this amount of time
> > > whenever we disable the panel (e.g, suspend).
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 69d8f1c..90ff059 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > >  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> > >  	}
> > >  
> > > -	/* And finally store the new values in the power sequencer. */
> > > +	/*
> > > +	 * 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) |
> > > -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> > > -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_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. */
> > 
> > Yay!
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> I'll wait with this one until the updated version of patch 3 is finalized,
> but I've meanwhile pulled in patches 4&5.

Paulo convinced me that this is save already, so merged to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Jani Nikula]

On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Because we already do the wait in software: see
> ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
>
> For the "backlight on" delay, even BSpec says we need to program 0x1
> to PP_ON_DELAYS 12:0.
>
> For the "backlight off" delay, if we don't do the same thing, when we
> call ironlake_wait_panel_off we'll end up waiting for the it again.
>
> On my machine the off delay is 200ms, so we save this amount of time
> whenever we disable the panel (e.g, suspend).

Whoa. This appears to fix an eDP black screen with 24->18 bpp dithering:

https://bugs.freedesktop.org/show_bug.cgi?id=73567#c21

Jani.


>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 69d8f1c..90ff059 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
>  	}
>  
> -	/* And finally store the new values in the power sequencer. */
> +	/*
> +	 * 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) |
> -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_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. */
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Daniel Vetter]

On Tue, Jan 28, 2014 at 09:57:57AM +0200, Jani Nikula wrote:
> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Because we already do the wait in software: see
> > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> >
> > For the "backlight on" delay, even BSpec says we need to program 0x1
> > to PP_ON_DELAYS 12:0.
> >
> > For the "backlight off" delay, if we don't do the same thing, when we
> > call ironlake_wait_panel_off we'll end up waiting for the it again.
> >
> > On my machine the off delay is 200ms, so we save this amount of time
> > whenever we disable the panel (e.g, suspend).
> 
> Whoa. This appears to fix an eDP black screen with 24->18 bpp dithering:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=73567#c21

I dunno whether I should cry or laugh ... Problem is that I don't really
see a way to port just this patch to 3.14. So we need the entire series,
which is a bit much imo. At least until we have more users scaling our
walls.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: set the backlight panel delays registers to 1

[Jani Nikula]

On Tue, 28 Jan 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jan 28, 2014 at 09:57:57AM +0200, Jani Nikula wrote:
>> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > Because we already do the wait in software: see
>> > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
>> >
>> > For the "backlight on" delay, even BSpec says we need to program 0x1
>> > to PP_ON_DELAYS 12:0.
>> >
>> > For the "backlight off" delay, if we don't do the same thing, when we
>> > call ironlake_wait_panel_off we'll end up waiting for the it again.
>> >
>> > On my machine the off delay is 200ms, so we save this amount of time
>> > whenever we disable the panel (e.g, suspend).
>> 
>> Whoa. This appears to fix an eDP black screen with 24->18 bpp dithering:
>> 
>> https://bugs.freedesktop.org/show_bug.cgi?id=73567#c21
>
> I dunno whether I should cry or laugh ... Problem is that I don't really
> see a way to port just this patch to 3.14. So we need the entire series,
> which is a bit much imo. At least until we have more users scaling our
> walls.

I also dunno whether I should cry or laugh... but for me it's because
the report has two devices of the same model, and one gets fixed by this
and the other not...

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center