[PATCH 1/3] drm/i915: add PWM and BLC assertion checks

[PATCH 1/3] drm/i915: add PWM and BLC assertion checks

[Jesse Barnes]

To make sure we properly follow the enable/disable sequences.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_panel.c |  5 ++-
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bf73771..b6f7087 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
 		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
 }
 
+static void assert_pwm(struct intel_connector *connector,
+		       bool expected_state)
+{
+	bool state;
+
+	state = intel_panel_get_backlight(connector);
+
+	WARN(state != expected_state, "pwm state failure, expected %d, found "
+	     "%d\n", expected_state, state);
+}
+
+#define assert_pwm_enabled(c) assert_pwm((c), true)
+#define assert_pwm_disabled(c) assert_pwm((c), false)
+
 static bool edp_have_panel_power(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode;
 
+	assert_pwm_disabled(intel_dp->attached_connector);
+
 	/*
 	 * There are four kinds of DP registers:
 	 *
@@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 	}
 }
 
+/*
+ * For this and the disable sequence below, Google for actual eDP LCD timing
+ * diagrams or check the eDP spec.  Below is for reference on asserts and
+ * does not contain Tx values for delays between steps.
+ *
+ * For panel on, the sequence should be:
+ * - LCD power supply on (PP regs or VDD AUX)
+ *   - eDP should display black at this point
+ * - HPD (if present) should go high
+ * - AUX channel becomes available
+ * - link training begins
+ * - LED backlight power on
+ * - LED PWM_EN goes high, duty cycle >min (PWM regs)
+ * - link training completes
+ * - LED_EN goes high (PP BLC_EN bit)
+ */
+
 void intel_edp_panel_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
 	}
 }
 
+/*
+ * For panel off the sequence should be:
+ * - LED_EN goes low (BLC_EN in our PP regs)
+ * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0)
+ *   - eDP should display black video at this point
+ * - LED VCCS goes low (power for backlight)
+ * - DP link goes to idle or off
+ * - AUX goes down
+ * - HPD line (if present) drops to low
+ * - eDP black video stops
+ * - LCD power supply shuts down (PP regs and VDD AUX)
+ */
+
 void intel_edp_panel_off(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 
 	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
 
+	/* By this time the PWM and BLC bits should be off already */
+	assert_pwm_disabled(intel_dp->attached_connector);
+
 	pp = ironlake_get_pp_control(intel_dp);
+
+	WARN(pp & EDP_BLC_ENABLE,
+	     "backlight controller still on at panel off time\n");
+
 	/* We need to switch off panel power _and_ force vdd, for otherwise some
 	 * panels get very unhappy and cease to work. */
-	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD |
-		EDP_BLC_ENABLE);
+	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD);
 
 	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
 
@@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
 	 * allowing it to appear.
 	 */
 	wait_backlight_on(intel_dp);
+
+	assert_pwm_enabled(intel_dp->attached_connector);
+
 	pp = ironlake_get_pp_control(intel_dp);
 	pp |= EDP_BLC_ENABLE;
 
@@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp))
 		return;
 
+	/* PWM must still be enabled here */
+	assert_pwm_enabled(intel_dp->attached_connector);
+
 	intel_panel_disable_backlight(intel_dp->attached_connector);
 
 	DRM_DEBUG_KMS("\n");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9002e77..0e91c40 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
 			      int fitting_mode);
 void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
 			       u32 max);
+u32 intel_panel_get_backlight(struct intel_connector *connector);
 int intel_panel_setup_backlight(struct drm_connector *connector);
 void intel_panel_enable_backlight(struct intel_connector *connector);
 void intel_panel_disable_backlight(struct intel_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cb05840..21c5e6f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
+		return 0;
+
 	return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK;
 }
 
@@ -395,7 +398,7 @@ static u32 vlv_get_backlight(struct intel_connector *connector)
 	return _vlv_get_backlight(dev, pipe);
 }
 
-static u32 intel_panel_get_backlight(struct intel_connector *connector)
+u32 intel_panel_get_backlight(struct intel_connector *connector)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.8.4.2

[PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Jesse Barnes]

With the new checks in place, we can see we're doing things backwards,
so fix them up per the spec.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b6f7087..d540fbe 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("Turn eDP power off\n");
 
-	edp_wait_backlight_off(intel_dp);
-
 	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
 
 	/* By this time the PWM and BLC bits should be off already */
@@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
 		return;
 
 	DRM_DEBUG_KMS("\n");
+
+	intel_panel_enable_backlight(intel_dp->attached_connector);
+
 	/*
 	 * If we enable the backlight right away following a panel power
 	 * on, we may see slight flicker as the panel syncs with the eDP
@@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
 
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
-
-	intel_panel_enable_backlight(intel_dp->attached_connector);
 }
 
 void intel_edp_backlight_off(struct intel_dp *intel_dp)
@@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
 	/* PWM must still be enabled here */
 	assert_pwm_enabled(intel_dp->attached_connector);
 
-	intel_panel_disable_backlight(intel_dp->attached_connector);
-
 	DRM_DEBUG_KMS("\n");
 	pp = ironlake_get_pp_control(intel_dp);
 	pp &= ~EDP_BLC_ENABLE;
@@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
 	intel_dp->last_backlight_off = jiffies;
+
+	edp_wait_backlight_off(intel_dp);
+
+	intel_panel_disable_backlight(intel_dp->attached_connector);
 }
 
 static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
-- 
1.8.4.2

[PATCH 3/3] drm/i915/vlv: use min brightness from VBT

[Jesse Barnes]

Going below the minimum value may affect the BLC_EN line, so try to use
the VBT provided minimum where possible, otherwise use an experimentally
derived value to prevent the panel from coming up.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_bios.c  |  3 ++-
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_panel.c | 10 +++++++++-
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff02225..3c40dcb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1148,6 +1148,7 @@ struct intel_vbt_data {
 	struct {
 		u16 pwm_freq_hz;
 		bool active_low_pwm;
+		u8 min_brightness;
 	} backlight;
 
 	/* MIPI DSI */
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 4867f4c..e8dedf5 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -301,11 +301,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 
 	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
 	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
+	dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
 	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
 		      "active %s, min brightness %u, level %u\n",
 		      dev_priv->vbt.backlight.pwm_freq_hz,
 		      dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
-		      entry->min_brightness,
+		      dev_priv->vbt.backlight.min_brightness,
 		      backlight_data->level[panel_type]);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0e91c40..053a968 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -166,6 +166,7 @@ struct intel_panel {
 		bool present;
 		u32 level;
 		u32 max;
+		u32 min;
 		bool enabled;
 		bool combination_mode;	/* gen 2/4 only */
 		bool active_low_pwm;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 21c5e6f..27d7508 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -510,6 +510,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
 	else
 		level = freq / max * level;
 
+	if (level < panel->backlight.min)
+		level = panel->backlight.min;
+
 	panel->backlight.level = level;
 	if (panel->backlight.device)
 		panel->backlight.device->props.brightness = level;
@@ -1047,7 +1050,12 @@ static int vlv_setup_backlight(struct intel_connector *connector)
 
 	ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A));
 	panel->backlight.max = ctl >> 16;
-	if (!panel->backlight.max)
+	panel->backlight.min = dev_priv->vbt.backlight.min_brightness;
+	/* sane (i.e. checked on scope) default */
+	if (!panel->backlight.min)
+		panel->backlight.min = 64;
+	if (!panel->backlight.max ||
+	    panel->backlight.max < panel->backlight.min)
 		return -ENODEV;
 
 	val = _vlv_get_backlight(dev, PIPE_A);
-- 
1.8.4.2

Re: [PATCH 3/3] drm/i915/vlv: use min brightness from VBT

[Daniel Vetter]

On Mon, Mar 31, 2014 at 11:13:57AM -0700, Jesse Barnes wrote:
> Going below the minimum value may affect the BLC_EN line, so try to use
> the VBT provided minimum where possible, otherwise use an experimentally
> derived value to prevent the panel from coming up.

"to prevent the panel form failing to come up" I hope?
-Daniel

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_bios.c  |  3 ++-
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_panel.c | 10 +++++++++-
>  4 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff02225..3c40dcb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1148,6 +1148,7 @@ struct intel_vbt_data {
>  	struct {
>  		u16 pwm_freq_hz;
>  		bool active_low_pwm;
> +		u8 min_brightness;
>  	} backlight;
>  
>  	/* MIPI DSI */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 4867f4c..e8dedf5 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -301,11 +301,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  
>  	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
>  	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> +	dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
>  	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
>  		      "active %s, min brightness %u, level %u\n",
>  		      dev_priv->vbt.backlight.pwm_freq_hz,
>  		      dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> -		      entry->min_brightness,
> +		      dev_priv->vbt.backlight.min_brightness,
>  		      backlight_data->level[panel_type]);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0e91c40..053a968 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -166,6 +166,7 @@ struct intel_panel {
>  		bool present;
>  		u32 level;
>  		u32 max;
> +		u32 min;
>  		bool enabled;
>  		bool combination_mode;	/* gen 2/4 only */
>  		bool active_low_pwm;
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 21c5e6f..27d7508 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -510,6 +510,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>  	else
>  		level = freq / max * level;
>  
> +	if (level < panel->backlight.min)
> +		level = panel->backlight.min;
> +
>  	panel->backlight.level = level;
>  	if (panel->backlight.device)
>  		panel->backlight.device->props.brightness = level;
> @@ -1047,7 +1050,12 @@ static int vlv_setup_backlight(struct intel_connector *connector)
>  
>  	ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A));
>  	panel->backlight.max = ctl >> 16;
> -	if (!panel->backlight.max)
> +	panel->backlight.min = dev_priv->vbt.backlight.min_brightness;
> +	/* sane (i.e. checked on scope) default */
> +	if (!panel->backlight.min)
> +		panel->backlight.min = 64;
> +	if (!panel->backlight.max ||
> +	    panel->backlight.max < panel->backlight.min)
>  		return -ENODEV;
>  
>  	val = _vlv_get_backlight(dev, PIPE_A);
> -- 
> 1.8.4.2
> 
> _______________________________________________
> 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/3] drm/i915/vlv: use min brightness from VBT

[Jesse Barnes]

On Mon, 31 Mar 2014 21:07:04 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Mar 31, 2014 at 11:13:57AM -0700, Jesse Barnes wrote:
> > Going below the minimum value may affect the BLC_EN line, so try to use
> > the VBT provided minimum where possible, otherwise use an experimentally
> > derived value to prevent the panel from coming up.
> 
> "to prevent the panel form failing to come up" I hope?

Yes I suppose that makes more sense.  I'm not trying to deliberately
sabotage things, even though it may appear that way at times. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: add PWM and BLC assertion checks

[Jani Nikula]

On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> To make sure we properly follow the enable/disable sequences.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_panel.c |  5 ++-
>  3 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bf73771..b6f7087 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
>  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
>  }
>  
> +static void assert_pwm(struct intel_connector *connector,
> +		       bool expected_state)
> +{
> +	bool state;
> +
> +	state = intel_panel_get_backlight(connector);

If the duty cycle is regarded as a binary on/off, I'd rather add an
additional "is enabled" call to intel_panel.c. Especially so because the
duty cycle value returned by intel_panel_get_backlight is meaningless
without the max value.

> +
> +	WARN(state != expected_state, "pwm state failure, expected %d, found "
> +	     "%d\n", expected_state, state);
> +}
> +
> +#define assert_pwm_enabled(c) assert_pwm((c), true)
> +#define assert_pwm_disabled(c) assert_pwm((c), false)
> +
>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode;
>  
> +	assert_pwm_disabled(intel_dp->attached_connector);
> +
>  	/*
>  	 * There are four kinds of DP registers:
>  	 *
> @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  	}
>  }
>  
> +/*
> + * For this and the disable sequence below, Google for actual eDP LCD timing
> + * diagrams or check the eDP spec.  Below is for reference on asserts and
> + * does not contain Tx values for delays between steps.
> + *
> + * For panel on, the sequence should be:
> + * - LCD power supply on (PP regs or VDD AUX)
> + *   - eDP should display black at this point
> + * - HPD (if present) should go high
> + * - AUX channel becomes available
> + * - link training begins
> + * - LED backlight power on
> + * - LED PWM_EN goes high, duty cycle >min (PWM regs)
> + * - link training completes
> + * - LED_EN goes high (PP BLC_EN bit)
> + */
> +
>  void intel_edp_panel_on(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
>  	}
>  }
>  
> +/*
> + * For panel off the sequence should be:
> + * - LED_EN goes low (BLC_EN in our PP regs)
> + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0)
> + *   - eDP should display black video at this point
> + * - LED VCCS goes low (power for backlight)
> + * - DP link goes to idle or off
> + * - AUX goes down
> + * - HPD line (if present) drops to low
> + * - eDP black video stops
> + * - LCD power supply shuts down (PP regs and VDD AUX)
> + */
> +
>  void intel_edp_panel_off(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  
>  	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
>  
> +	/* By this time the PWM and BLC bits should be off already */
> +	assert_pwm_disabled(intel_dp->attached_connector);
> +
>  	pp = ironlake_get_pp_control(intel_dp);
> +
> +	WARN(pp & EDP_BLC_ENABLE,
> +	     "backlight controller still on at panel off time\n");
> +
>  	/* We need to switch off panel power _and_ force vdd, for otherwise some
>  	 * panels get very unhappy and cease to work. */
> -	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD |
> -		EDP_BLC_ENABLE);
> +	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD);
>  
>  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>  
> @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
>  	 * allowing it to appear.
>  	 */
>  	wait_backlight_on(intel_dp);
> +
> +	assert_pwm_enabled(intel_dp->attached_connector);
> +
>  	pp = ironlake_get_pp_control(intel_dp);
>  	pp |= EDP_BLC_ENABLE;
>  
> @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
>  	if (!is_edp(intel_dp))
>  		return;
>  
> +	/* PWM must still be enabled here */
> +	assert_pwm_enabled(intel_dp->attached_connector);
> +
>  	intel_panel_disable_backlight(intel_dp->attached_connector);
>  
>  	DRM_DEBUG_KMS("\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9002e77..0e91c40 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
>  			      int fitting_mode);
>  void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>  			       u32 max);
> +u32 intel_panel_get_backlight(struct intel_connector *connector);
>  int intel_panel_setup_backlight(struct drm_connector *connector);
>  void intel_panel_enable_backlight(struct intel_connector *connector);
>  void intel_panel_disable_backlight(struct intel_connector *connector);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb05840..21c5e6f 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
> +		return 0;
> +

If our internal state is consistent, I don't think this should be
necessary. And if our internal state isn't consistent, we should fix
that and maybe add internal asserts within intel_panel.c.

>  	return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK;
>  }
>  
> @@ -395,7 +398,7 @@ static u32 vlv_get_backlight(struct intel_connector *connector)
>  	return _vlv_get_backlight(dev, pipe);
>  }
>  
> -static u32 intel_panel_get_backlight(struct intel_connector *connector)
> +u32 intel_panel_get_backlight(struct intel_connector *connector)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 1.8.4.2
>
> _______________________________________________
> 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 3/3] drm/i915/vlv: use min brightness from VBT

[Jani Nikula]

On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Going below the minimum value may affect the BLC_EN line, so try to use
> the VBT provided minimum where possible, otherwise use an experimentally
> derived value to prevent the panel from coming up.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_bios.c  |  3 ++-
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_panel.c | 10 +++++++++-
>  4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff02225..3c40dcb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1148,6 +1148,7 @@ struct intel_vbt_data {
>  	struct {
>  		u16 pwm_freq_hz;
>  		bool active_low_pwm;
> +		u8 min_brightness;
>  	} backlight;
>  
>  	/* MIPI DSI */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 4867f4c..e8dedf5 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -301,11 +301,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  
>  	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
>  	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> +	dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
>  	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
>  		      "active %s, min brightness %u, level %u\n",
>  		      dev_priv->vbt.backlight.pwm_freq_hz,
>  		      dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> -		      entry->min_brightness,
> +		      dev_priv->vbt.backlight.min_brightness,
>  		      backlight_data->level[panel_type]);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0e91c40..053a968 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -166,6 +166,7 @@ struct intel_panel {
>  		bool present;
>  		u32 level;
>  		u32 max;
> +		u32 min;
>  		bool enabled;
>  		bool combination_mode;	/* gen 2/4 only */
>  		bool active_low_pwm;
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 21c5e6f..27d7508 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -510,6 +510,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>  	else
>  		level = freq / max * level;
>  
> +	if (level < panel->backlight.min)
> +		level = panel->backlight.min;

Are you sure the VBT minimum is the *absolute* minimum duty cycle value?
The spec I have says, "This field specifies the minimum brightness",
which could be an absolute value, percentage, multiplier, anything
really. I most doubt it's an absolute value because you have to go out
of your way to figure out what the max is because it's defined in terms
of the PWM frequency in Hz.

No matter what the unit is, this is wrong for any gen 2/4 platform where
combination mode is enabled.

---

I'm also hesitant about the change. First, what does it mean for the
userspace that 0 no longer switches off the backlight? (Which I realize
was sort of broken due to not really disabling the PWM since gen4, but
that's what the perceived effect was anyway.) This may not be a big
issue since 0 is not necessarily off for acpi backlight.

Second, what's the usability implication of backlight change requests
potentially having no effect at the low values? Imagine maybe 10-20
levels of brightness in the UI but no change between the lowest
levels. I fear we might get regression reports for this. The only way to
handle this would be scaling of some sort I think.

I suppose our original thinking was mechanism not policy, and just
exposed the full range to userspace. This is very different from the
acpi backlight approach, which looks at vbt/opregion and exposes I think
up to 20 discrete levels with 0 being min, not necessarily off. The
opregion tables may also have a non-linear mapping from backlight
percentage to duty cycle. Maybe we should bite the bullet and follow
suit?


BR,
Jani.



> +
>  	panel->backlight.level = level;
>  	if (panel->backlight.device)
>  		panel->backlight.device->props.brightness = level;
> @@ -1047,7 +1050,12 @@ static int vlv_setup_backlight(struct intel_connector *connector)
>  
>  	ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A));
>  	panel->backlight.max = ctl >> 16;
> -	if (!panel->backlight.max)
> +	panel->backlight.min = dev_priv->vbt.backlight.min_brightness;
> +	/* sane (i.e. checked on scope) default */
> +	if (!panel->backlight.min)
> +		panel->backlight.min = 64;
> +	if (!panel->backlight.max ||
> +	    panel->backlight.max < panel->backlight.min)
>  		return -ENODEV;
>  
>  	val = _vlv_get_backlight(dev, PIPE_A);
> -- 
> 1.8.4.2
>
> _______________________________________________
> 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 1/3] drm/i915: add PWM and BLC assertion checks

[Jani Nikula]

On Tue, 01 Apr 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> To make sure we properly follow the enable/disable sequences.
>>
>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>>  drivers/gpu/drm/i915/intel_panel.c |  5 ++-
>>  3 files changed, 65 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index bf73771..b6f7087 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
>>  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
>>  }
>>  
>> +static void assert_pwm(struct intel_connector *connector,
>> +		       bool expected_state)
>> +{
>> +	bool state;
>> +
>> +	state = intel_panel_get_backlight(connector);
>
> If the duty cycle is regarded as a binary on/off, I'd rather add an
> additional "is enabled" call to intel_panel.c. Especially so because the
> duty cycle value returned by intel_panel_get_backlight is meaningless
> without the max value.
>
>> +
>> +	WARN(state != expected_state, "pwm state failure, expected %d, found "
>> +	     "%d\n", expected_state, state);
>> +}
>> +
>> +#define assert_pwm_enabled(c) assert_pwm((c), true)
>> +#define assert_pwm_disabled(c) assert_pwm((c), false)
>> +
>>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>  {
>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
>>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>>  	struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode;
>>  
>> +	assert_pwm_disabled(intel_dp->attached_connector);
>> +
>>  	/*
>>  	 * There are four kinds of DP registers:
>>  	 *
>> @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>>  	}
>>  }
>>  
>> +/*
>> + * For this and the disable sequence below, Google for actual eDP LCD timing
>> + * diagrams or check the eDP spec.  Below is for reference on asserts and
>> + * does not contain Tx values for delays between steps.
>> + *
>> + * For panel on, the sequence should be:
>> + * - LCD power supply on (PP regs or VDD AUX)
>> + *   - eDP should display black at this point
>> + * - HPD (if present) should go high
>> + * - AUX channel becomes available
>> + * - link training begins
>> + * - LED backlight power on
>> + * - LED PWM_EN goes high, duty cycle >min (PWM regs)
>> + * - link training completes
>> + * - LED_EN goes high (PP BLC_EN bit)
>> + */
>> +
>>  void intel_edp_panel_on(struct intel_dp *intel_dp)
>>  {
>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
>>  	}
>>  }
>>  
>> +/*
>> + * For panel off the sequence should be:
>> + * - LED_EN goes low (BLC_EN in our PP regs)
>> + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0)
>> + *   - eDP should display black video at this point
>> + * - LED VCCS goes low (power for backlight)
>> + * - DP link goes to idle or off
>> + * - AUX goes down
>> + * - HPD line (if present) drops to low
>> + * - eDP black video stops
>> + * - LCD power supply shuts down (PP regs and VDD AUX)
>> + */
>> +
>>  void intel_edp_panel_off(struct intel_dp *intel_dp)
>>  {
>>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>>  
>>  	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
>>  
>> +	/* By this time the PWM and BLC bits should be off already */
>> +	assert_pwm_disabled(intel_dp->attached_connector);
>> +
>>  	pp = ironlake_get_pp_control(intel_dp);
>> +
>> +	WARN(pp & EDP_BLC_ENABLE,
>> +	     "backlight controller still on at panel off time\n");
>> +
>>  	/* We need to switch off panel power _and_ force vdd, for otherwise some
>>  	 * panels get very unhappy and cease to work. */
>> -	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD |
>> -		EDP_BLC_ENABLE);
>> +	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD);
>>  
>>  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>>  
>> @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
>>  	 * allowing it to appear.
>>  	 */
>>  	wait_backlight_on(intel_dp);
>> +
>> +	assert_pwm_enabled(intel_dp->attached_connector);
>> +
>>  	pp = ironlake_get_pp_control(intel_dp);
>>  	pp |= EDP_BLC_ENABLE;
>>  
>> @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
>>  	if (!is_edp(intel_dp))
>>  		return;
>>  
>> +	/* PWM must still be enabled here */
>> +	assert_pwm_enabled(intel_dp->attached_connector);
>> +
>>  	intel_panel_disable_backlight(intel_dp->attached_connector);
>>  
>>  	DRM_DEBUG_KMS("\n");
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9002e77..0e91c40 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
>>  			      int fitting_mode);
>>  void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>>  			       u32 max);
>> +u32 intel_panel_get_backlight(struct intel_connector *connector);
>>  int intel_panel_setup_backlight(struct drm_connector *connector);
>>  void intel_panel_enable_backlight(struct intel_connector *connector);
>>  void intel_panel_disable_backlight(struct intel_connector *connector);
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index cb05840..21c5e6f 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> +	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
>> +		return 0;
>> +
>
> If our internal state is consistent, I don't think this should be
> necessary. And if our internal state isn't consistent, we should fix
> that and maybe add internal asserts within intel_panel.c.

I wrote this with the bigger picture in mind, but your check above is
also doubly wrong!

BR,
Jani.


>
>>  	return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK;
>>  }
>>  
>> @@ -395,7 +398,7 @@ static u32 vlv_get_backlight(struct intel_connector *connector)
>>  	return _vlv_get_backlight(dev, pipe);
>>  }
>>  
>> -static u32 intel_panel_get_backlight(struct intel_connector *connector)
>> +u32 intel_panel_get_backlight(struct intel_connector *connector)
>>  {
>>  	struct drm_device *dev = connector->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -- 
>> 1.8.4.2
>>
>> _______________________________________________
>> 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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: add PWM and BLC assertion checks

[Jesse Barnes]

On Tue, 01 Apr 2014 10:19:29 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > To make sure we properly follow the enable/disable sequences.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  drivers/gpu/drm/i915/intel_panel.c |  5 ++-
> >  3 files changed, 65 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index bf73771..b6f7087 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
> >  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> >  }
> >  
> > +static void assert_pwm(struct intel_connector *connector,
> > +		       bool expected_state)
> > +{
> > +	bool state;
> > +
> > +	state = intel_panel_get_backlight(connector);
> 
> If the duty cycle is regarded as a binary on/off, I'd rather add an
> additional "is enabled" call to intel_panel.c. Especially so because the
> duty cycle value returned by intel_panel_get_backlight is meaningless
> without the max value.

Hm I guess that would be cleaner; for my purposes I thought any
non-zero PWM duty cycle would be sufficient, but of course other checks
are needed as well, like whether the PWM enable bit is on, and checks
against the BLC_EN bit in the PP regs, but those are logically
separate.  is_enabled might better map back to the PWM_EN bit rather
than a non-zero duty cycle though.

> >  
> > +	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
> > +		return 0;
> > +
> 
> If our internal state is consistent, I don't think this should be
> necessary. And if our internal state isn't consistent, we should fix
> that and maybe add internal asserts within intel_panel.c.

Yeah this could be covered with other asserts as long as we have them
in all the right places.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: add PWM and BLC assertion checks

[Jesse Barnes]

On Tue, 01 Apr 2014 12:27:43 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Tue, 01 Apr 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> To make sure we properly follow the enable/disable sequences.
> >>
> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
> >>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >>  drivers/gpu/drm/i915/intel_panel.c |  5 ++-
> >>  3 files changed, 65 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index bf73771..b6f7087 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
> >>  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> >>  }
> >>  
> >> +static void assert_pwm(struct intel_connector *connector,
> >> +		       bool expected_state)
> >> +{
> >> +	bool state;
> >> +
> >> +	state = intel_panel_get_backlight(connector);
> >
> > If the duty cycle is regarded as a binary on/off, I'd rather add an
> > additional "is enabled" call to intel_panel.c. Especially so because the
> > duty cycle value returned by intel_panel_get_backlight is meaningless
> > without the max value.
> >
> >> +
> >> +	WARN(state != expected_state, "pwm state failure, expected %d, found "
> >> +	     "%d\n", expected_state, state);
> >> +}
> >> +
> >> +#define assert_pwm_enabled(c) assert_pwm((c), true)
> >> +#define assert_pwm_disabled(c) assert_pwm((c), false)
> >> +
> >>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >>  {
> >>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
> >>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> >>  	struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode;
> >>  
> >> +	assert_pwm_disabled(intel_dp->attached_connector);
> >> +
> >>  	/*
> >>  	 * There are four kinds of DP registers:
> >>  	 *
> >> @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
> >>  	}
> >>  }
> >>  
> >> +/*
> >> + * For this and the disable sequence below, Google for actual eDP LCD timing
> >> + * diagrams or check the eDP spec.  Below is for reference on asserts and
> >> + * does not contain Tx values for delays between steps.
> >> + *
> >> + * For panel on, the sequence should be:
> >> + * - LCD power supply on (PP regs or VDD AUX)
> >> + *   - eDP should display black at this point
> >> + * - HPD (if present) should go high
> >> + * - AUX channel becomes available
> >> + * - link training begins
> >> + * - LED backlight power on
> >> + * - LED PWM_EN goes high, duty cycle >min (PWM regs)
> >> + * - link training completes
> >> + * - LED_EN goes high (PP BLC_EN bit)
> >> + */
> >> +
> >>  void intel_edp_panel_on(struct intel_dp *intel_dp)
> >>  {
> >>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
> >>  	}
> >>  }
> >>  
> >> +/*
> >> + * For panel off the sequence should be:
> >> + * - LED_EN goes low (BLC_EN in our PP regs)
> >> + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0)
> >> + *   - eDP should display black video at this point
> >> + * - LED VCCS goes low (power for backlight)
> >> + * - DP link goes to idle or off
> >> + * - AUX goes down
> >> + * - HPD line (if present) drops to low
> >> + * - eDP black video stops
> >> + * - LCD power supply shuts down (PP regs and VDD AUX)
> >> + */
> >> +
> >>  void intel_edp_panel_off(struct intel_dp *intel_dp)
> >>  {
> >>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >> @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
> >>  
> >>  	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
> >>  
> >> +	/* By this time the PWM and BLC bits should be off already */
> >> +	assert_pwm_disabled(intel_dp->attached_connector);
> >> +
> >>  	pp = ironlake_get_pp_control(intel_dp);
> >> +
> >> +	WARN(pp & EDP_BLC_ENABLE,
> >> +	     "backlight controller still on at panel off time\n");
> >> +
> >>  	/* We need to switch off panel power _and_ force vdd, for otherwise some
> >>  	 * panels get very unhappy and cease to work. */
> >> -	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD |
> >> -		EDP_BLC_ENABLE);
> >> +	pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD);
> >>  
> >>  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
> >>  
> >> @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
> >>  	 * allowing it to appear.
> >>  	 */
> >>  	wait_backlight_on(intel_dp);
> >> +
> >> +	assert_pwm_enabled(intel_dp->attached_connector);
> >> +
> >>  	pp = ironlake_get_pp_control(intel_dp);
> >>  	pp |= EDP_BLC_ENABLE;
> >>  
> >> @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
> >>  	if (!is_edp(intel_dp))
> >>  		return;
> >>  
> >> +	/* PWM must still be enabled here */
> >> +	assert_pwm_enabled(intel_dp->attached_connector);
> >> +
> >>  	intel_panel_disable_backlight(intel_dp->attached_connector);
> >>  
> >>  	DRM_DEBUG_KMS("\n");
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 9002e77..0e91c40 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
> >>  			      int fitting_mode);
> >>  void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
> >>  			       u32 max);
> >> +u32 intel_panel_get_backlight(struct intel_connector *connector);
> >>  int intel_panel_setup_backlight(struct drm_connector *connector);
> >>  void intel_panel_enable_backlight(struct intel_connector *connector);
> >>  void intel_panel_disable_backlight(struct intel_connector *connector);
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index cb05840..21c5e6f 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  
> >> +	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
> >> +		return 0;
> >> +
> >
> > If our internal state is consistent, I don't think this should be
> > necessary. And if our internal state isn't consistent, we should fix
> > that and maybe add internal asserts within intel_panel.c.
> 
> I wrote this with the bigger picture in mind, but your check above is
> also doubly wrong!

Hah it is!  I had it correct in one tree but must have copy & pasted
wrong into this one. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915/vlv: use min brightness from VBT

[Jesse Barnes]

On Tue, 01 Apr 2014 11:08:13 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Going below the minimum value may affect the BLC_EN line, so try to use
> > the VBT provided minimum where possible, otherwise use an experimentally
> > derived value to prevent the panel from coming up.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
> >  drivers/gpu/drm/i915/intel_bios.c  |  3 ++-
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  drivers/gpu/drm/i915/intel_panel.c | 10 +++++++++-
> >  4 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ff02225..3c40dcb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1148,6 +1148,7 @@ struct intel_vbt_data {
> >  	struct {
> >  		u16 pwm_freq_hz;
> >  		bool active_low_pwm;
> > +		u8 min_brightness;
> >  	} backlight;
> >  
> >  	/* MIPI DSI */
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 4867f4c..e8dedf5 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -301,11 +301,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
> >  
> >  	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
> >  	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> > +	dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> >  	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
> >  		      "active %s, min brightness %u, level %u\n",
> >  		      dev_priv->vbt.backlight.pwm_freq_hz,
> >  		      dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> > -		      entry->min_brightness,
> > +		      dev_priv->vbt.backlight.min_brightness,
> >  		      backlight_data->level[panel_type]);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0e91c40..053a968 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -166,6 +166,7 @@ struct intel_panel {
> >  		bool present;
> >  		u32 level;
> >  		u32 max;
> > +		u32 min;
> >  		bool enabled;
> >  		bool combination_mode;	/* gen 2/4 only */
> >  		bool active_low_pwm;
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 21c5e6f..27d7508 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -510,6 +510,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
> >  	else
> >  		level = freq / max * level;
> >  
> > +	if (level < panel->backlight.min)
> > +		level = panel->backlight.min;
> 
> Are you sure the VBT minimum is the *absolute* minimum duty cycle value?
> The spec I have says, "This field specifies the minimum brightness",
> which could be an absolute value, percentage, multiplier, anything
> really. I most doubt it's an absolute value because you have to go out
> of your way to figure out what the max is because it's defined in terms
> of the PWM frequency in Hz.
> 
> No matter what the unit is, this is wrong for any gen 2/4 platform where
> combination mode is enabled.
> 
> ---
> 
> I'm also hesitant about the change. First, what does it mean for the
> userspace that 0 no longer switches off the backlight? (Which I realize
> was sort of broken due to not really disabling the PWM since gen4, but
> that's what the perceived effect was anyway.) This may not be a big
> issue since 0 is not necessarily off for acpi backlight.
> 
> Second, what's the usability implication of backlight change requests
> potentially having no effect at the low values? Imagine maybe 10-20
> levels of brightness in the UI but no change between the lowest
> levels. I fear we might get regression reports for this. The only way to
> handle this would be scaling of some sort I think.
> 
> I suppose our original thinking was mechanism not policy, and just
> exposed the full range to userspace. This is very different from the
> acpi backlight approach, which looks at vbt/opregion and exposes I think
> up to 20 discrete levels with 0 being min, not necessarily off. The
> opregion tables may also have a non-linear mapping from backlight
> percentage to duty cycle. Maybe we should bite the bullet and follow
> suit?

Yeah I should have marked this one as RFC; the VBT usage is new and we
really do need to scale things.

The main bug fix here is to prevent the BYT duty cycle from dropping
below ~64.  As you say, to do that we really shouldn't expose those
lower 64 values to userspace as they'll do nothing.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Jesse Barnes]

Jani, can you review this one?  It's still needed for us to conform to
the eDP timing spec.

Thanks,
Jesse

On Mon, 31 Mar 2014 11:13:56 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> With the new checks in place, we can see we're doing things backwards,
> so fix them up per the spec.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b6f7087..d540fbe 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  
>  	DRM_DEBUG_KMS("Turn eDP power off\n");
>  
> -	edp_wait_backlight_off(intel_dp);
> -
>  	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
>  
>  	/* By this time the PWM and BLC bits should be off already */
> @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
>  		return;
>  
>  	DRM_DEBUG_KMS("\n");
> +
> +	intel_panel_enable_backlight(intel_dp->attached_connector);
> +
>  	/*
>  	 * If we enable the backlight right away following a panel power
>  	 * on, we may see slight flicker as the panel syncs with the eDP
> @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
>  
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
> -
> -	intel_panel_enable_backlight(intel_dp->attached_connector);
>  }
>  
>  void intel_edp_backlight_off(struct intel_dp *intel_dp)
> @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
>  	/* PWM must still be enabled here */
>  	assert_pwm_enabled(intel_dp->attached_connector);
>  
> -	intel_panel_disable_backlight(intel_dp->attached_connector);
> -
>  	DRM_DEBUG_KMS("\n");
>  	pp = ironlake_get_pp_control(intel_dp);
>  	pp &= ~EDP_BLC_ENABLE;
> @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
>  	intel_dp->last_backlight_off = jiffies;
> +
> +	edp_wait_backlight_off(intel_dp);
> +
> +	intel_panel_disable_backlight(intel_dp->attached_connector);
>  }
>  
>  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Jani Nikula]

On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> With the new checks in place, we can see we're doing things backwards,
> so fix them up per the spec.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b6f7087..d540fbe 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  
>  	DRM_DEBUG_KMS("Turn eDP power off\n");
>  
> -	edp_wait_backlight_off(intel_dp);
> -

Please justify moving this wait to intel_edp_backlight_off. I thought
the point of these wait calls is such that we'll only end up waiting
when we really have to. If this is left as-is, we can do useful stuff
*while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp).

Otherwise this looks good to me, although I didn't find proper
explanations for everything. Do I understand correctly that the
EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for
enabling/disabling the PWM signal to the eDP? So before this patch, we
let the disabled PWM signal through to the panel?

BR,
Jani.


>  	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
>  
>  	/* By this time the PWM and BLC bits should be off already */
> @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
>  		return;
>  
>  	DRM_DEBUG_KMS("\n");
> +
> +	intel_panel_enable_backlight(intel_dp->attached_connector);
> +
>  	/*
>  	 * If we enable the backlight right away following a panel power
>  	 * on, we may see slight flicker as the panel syncs with the eDP
> @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
>  
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
> -
> -	intel_panel_enable_backlight(intel_dp->attached_connector);
>  }
>  
>  void intel_edp_backlight_off(struct intel_dp *intel_dp)
> @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
>  	/* PWM must still be enabled here */
>  	assert_pwm_enabled(intel_dp->attached_connector);
>  
> -	intel_panel_disable_backlight(intel_dp->attached_connector);
> -
>  	DRM_DEBUG_KMS("\n");
>  	pp = ironlake_get_pp_control(intel_dp);
>  	pp &= ~EDP_BLC_ENABLE;
> @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
>  	intel_dp->last_backlight_off = jiffies;
> +
> +	edp_wait_backlight_off(intel_dp);
> +
> +	intel_panel_disable_backlight(intel_dp->attached_connector);
>  }
>  
>  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> -- 
> 1.8.4.2
>
> _______________________________________________
> 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 1/3] drm/i915: add PWM and BLC assertion checks

[Jani Nikula]

On Tue, 01 Apr 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 01 Apr 2014 10:19:29 +0300
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > To make sure we properly follow the enable/disable sequences.
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
>> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>> >  drivers/gpu/drm/i915/intel_panel.c |  5 ++-
>> >  3 files changed, 65 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index bf73771..b6f7087 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
>> >  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
>> >  }
>> >  
>> > +static void assert_pwm(struct intel_connector *connector,
>> > +		       bool expected_state)
>> > +{
>> > +	bool state;
>> > +
>> > +	state = intel_panel_get_backlight(connector);
>> 
>> If the duty cycle is regarded as a binary on/off, I'd rather add an
>> additional "is enabled" call to intel_panel.c. Especially so because the
>> duty cycle value returned by intel_panel_get_backlight is meaningless
>> without the max value.
>
> Hm I guess that would be cleaner; for my purposes I thought any
> non-zero PWM duty cycle would be sufficient, but of course other checks
> are needed as well, like whether the PWM enable bit is on, and checks
> against the BLC_EN bit in the PP regs, but those are logically
> separate.  is_enabled might better map back to the PWM_EN bit rather
> than a non-zero duty cycle though.

We could add intel_panel_backlight_enabled() call that returns
connector->panel.backlight.enabled.

BR,
Jani.






>
>> >  
>> > +	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
>> > +		return 0;
>> > +
>> 
>> If our internal state is consistent, I don't think this should be
>> necessary. And if our internal state isn't consistent, we should fix
>> that and maybe add internal asserts within intel_panel.c.
>
> Yeah this could be covered with other asserts as long as we have them
> in all the right places.
>
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Jesse Barnes]

On Wed, 25 Jun 2014 15:21:12 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > With the new checks in place, we can see we're doing things backwards,
> > so fix them up per the spec.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b6f7087..d540fbe 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
> >  
> >  	DRM_DEBUG_KMS("Turn eDP power off\n");
> >  
> > -	edp_wait_backlight_off(intel_dp);
> > -
> 
> Please justify moving this wait to intel_edp_backlight_off. I thought
> the point of these wait calls is such that we'll only end up waiting
> when we really have to. If this is left as-is, we can do useful stuff
> *while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp).

The wait needs to happen between the BLC disable and the backlight
disable per the eDP timing spec.  We could put the disable into a
delayed work queue if you want to reclaim the time, but it should be
pretty small compared to a full panel power sequence.

The wait here looks like it was to prevent us from re-enabling the
backlight too quickly, but I don't have timing info for that; not sure
if there are specific requirements there or not.

Jesse

> Otherwise this looks good to me, although I didn't find proper
> explanations for everything. Do I understand correctly that the
> EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for
> enabling/disabling the PWM signal to the eDP? So before this patch, we
> let the disabled PWM signal through to the panel?

Right, something like that.  Enabling PWM starts driving power to some
components, but the PP_CONTROL bit controls whether it actually gets
out to the panel meaningfully, and at least according to the scope
readouts we have, doing it in this order corrects the backward ordering
we saw.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Jani Nikula]

On Wed, 25 Jun 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 25 Jun 2014 15:21:12 +0300
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > With the new checks in place, we can see we're doing things backwards,
>> > so fix them up per the spec.
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
>> >  1 file changed, 7 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index b6f7087..d540fbe 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>> >  
>> >  	DRM_DEBUG_KMS("Turn eDP power off\n");
>> >  
>> > -	edp_wait_backlight_off(intel_dp);
>> > -
>> 
>> Please justify moving this wait to intel_edp_backlight_off. I thought
>> the point of these wait calls is such that we'll only end up waiting
>> when we really have to. If this is left as-is, we can do useful stuff
>> *while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp).
>
> The wait needs to happen between the BLC disable and the backlight
> disable per the eDP timing spec.  We could put the disable into a
> delayed work queue if you want to reclaim the time, but it should be
> pretty small compared to a full panel power sequence.

Okay, I'll take your word for it. ;) Do you still want to pursue patch
1?  If not, please pick up the comments from there into this one, and
add the above as a comment in there too.

Thanks,
Jani.


>
> The wait here looks like it was to prevent us from re-enabling the
> backlight too quickly, but I don't have timing info for that; not sure
> if there are specific requirements there or not.
>
> Jesse
>
>> Otherwise this looks good to me, although I didn't find proper
>> explanations for everything. Do I understand correctly that the
>> EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for
>> enabling/disabling the PWM signal to the eDP? So before this patch, we
>> let the disabled PWM signal through to the panel?
>
> Right, something like that.  Enabling PWM starts driving power to some
> components, but the PP_CONTROL bit controls whether it actually gets
> out to the panel meaningfully, and at least according to the scope
> readouts we have, doing it in this order corrects the backward ordering
> we saw.
>
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Daniel Vetter]

On Thu, Jun 19, 2014 at 11:00:20AM -0700, Jesse Barnes wrote:
> Jani, can you review this one?  It's still needed for us to conform to
> the eDP timing spec.

Jani's already goofing off on vacation and I couldn't spot his r-b. Merged
anyway, I guess people will scream fast enough if this breaks stuff ;-)
-Daniel

> 
> Thanks,
> Jesse
> 
> On Mon, 31 Mar 2014 11:13:56 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > With the new checks in place, we can see we're doing things backwards,
> > so fix them up per the spec.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b6f7087..d540fbe 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
> >  
> >  	DRM_DEBUG_KMS("Turn eDP power off\n");
> >  
> > -	edp_wait_backlight_off(intel_dp);
> > -
> >  	WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
> >  
> >  	/* By this time the PWM and BLC bits should be off already */
> > @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
> >  		return;
> >  
> >  	DRM_DEBUG_KMS("\n");
> > +
> > +	intel_panel_enable_backlight(intel_dp->attached_connector);
> > +
> >  	/*
> >  	 * If we enable the backlight right away following a panel power
> >  	 * on, we may see slight flicker as the panel syncs with the eDP
> > @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
> >  
> >  	I915_WRITE(pp_ctrl_reg, pp);
> >  	POSTING_READ(pp_ctrl_reg);
> > -
> > -	intel_panel_enable_backlight(intel_dp->attached_connector);
> >  }
> >  
> >  void intel_edp_backlight_off(struct intel_dp *intel_dp)
> > @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
> >  	/* PWM must still be enabled here */
> >  	assert_pwm_enabled(intel_dp->attached_connector);
> >  
> > -	intel_panel_disable_backlight(intel_dp->attached_connector);
> > -
> >  	DRM_DEBUG_KMS("\n");
> >  	pp = ironlake_get_pp_control(intel_dp);
> >  	pp &= ~EDP_BLC_ENABLE;
> > @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
> >  	I915_WRITE(pp_ctrl_reg, pp);
> >  	POSTING_READ(pp_ctrl_reg);
> >  	intel_dp->last_backlight_off = jiffies;
> > +
> > +	edp_wait_backlight_off(intel_dp);
> > +
> > +	intel_panel_disable_backlight(intel_dp->attached_connector);
> >  }
> >  
> >  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> 
> 
> -- 
> 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