drm/i915: backlight sysfs bl_power and brightness == 0

This series adds support for backlight class sysfs bl_power attribute for eDP panels, which allows switching the backlight on/off. This is done using the eDP panel power control as a sub-state of everything else being enabled. Patch 4 also makes 0 brightness switch off the eDP backlight using the same mechanism. This is all eDP specific, with no support for LVDS.

I'm not particularly fond of these patches, but there appears to be demand for them, particularly since

commit 6dda730e55f412a6dfb181cae6784822ba463847

Author: Jani Nikula <[email protected]>
Date:   Tue Jun 24 18:27:40 2014 +0300

drm/i915: respect the VBT minimum backlight brightness

All the discussions seem to just drag on and on without code to speak about, so here goes. I've tried to make this as simple and unintrusive as possible.

BR,
Jani.

Jani Nikula (4):
drm/i915/dp: split up panel power control from backlight pwm control drm/i915: add some framework for backlight bl_power support drm/i915/dp: make backlight bl_power control power sequencer backlight drm/i915: switch off backlight for backlight class 0 brightness

  drivers/gpu/drm/i915/intel_dp.c    | 61 ++++++++++++++++++++++++++++++--------
  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
  drivers/gpu/drm/i915/intel_panel.c | 27 +++++++++++++++++
  3 files changed, 77 insertions(+), 13 deletions(-)

Make it possible to change panel power control backlight state without touching the PWM. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 39 ++++++++++++++++++++++++++-------------
  1 file changed, 26 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +20 -13

index e5ada4fbe945..d8baf60ff3fd 100644
@@ -1384,7 +1384,8 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
  	intel_display_power_put(dev_priv, power_domain);
  }
 
-void intel_edp_backlight_on(struct intel_dp *intel_dp)
+/* Enable backlight in the panel power control. */
+static void _intel_edp_backlight_on(struct intel_dp *intel_dp)
  {
  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
  	struct drm_device *dev = intel_dig_port->base.base.dev;

@@ -1392,13 +1393,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
  	u32 pp;
  	u32 pp_ctrl_reg;
 
-	if (!is_edp(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

@@ -1415,17 +1409,26 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
  	POSTING_READ(pp_ctrl_reg);
  }
 
-void intel_edp_backlight_off(struct intel_dp *intel_dp)
+/* Enable backlight PWM and backlight PP control. */
+void intel_edp_backlight_on(struct intel_dp *intel_dp)
+{
+	if (!is_edp(intel_dp))
+		return;
+
+	DRM_DEBUG_KMS("\n");
+
+	intel_panel_enable_backlight(intel_dp->attached_connector);
+	_intel_edp_backlight_on(intel_dp);
+}
+
+/* Disable backlight in the panel power control. */
+static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
  {
  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
  	struct drm_i915_private *dev_priv = dev->dev_private;
  	u32 pp;
  	u32 pp_ctrl_reg;
 
-	if (!is_edp(intel_dp))
-		return;
-
-	DRM_DEBUG_KMS("\n");
  	pp = ironlake_get_pp_control(intel_dp);
  	pp &= ~EDP_BLC_ENABLE;
 

@@ -1436,7 +1439,17 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
  	intel_dp->last_backlight_off = jiffies;
 
  	edp_wait_backlight_off(intel_dp);
+}
+
+/* Disable backlight PP control and backlight PWM. */
+void intel_edp_backlight_off(struct intel_dp *intel_dp)

This lets the userspace switch off the backlight using the backlight class sysfs bl_power file. The switch is done using the power sequencer; the backlight PWM, and everything else, remains enabled. The display backlight won't draw power, but for maximum power savings the encoder needs to be switched off.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++
  1 file changed, 22 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +9 -0

index d8baf60ff3fd..f6e3e9a906b0 100644
@@ -1453,6 +1453,27 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
  	intel_panel_disable_backlight(intel_dp->attached_connector);
  }
 
+/*
+ * Hook for controlling the panel power control backlight through the bl_power
+ * sysfs attribute. Take care to handle multiple calls.
+ */
+static void intel_edp_backlight_power(struct intel_connector *connector,
+				      bool enable)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
+	bool is_enabled = ironlake_get_pp_control(intel_dp) & EDP_BLC_ENABLE;

drivers/gpu/drm/i915/intel_dp.c +3 -0

+}
+
  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
  {
  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);

@@ -4579,6 +4600,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
  	}
 
  	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
+	intel_connector->panel.backlight_power = intel_edp_backlight_power;
  	intel_panel_setup_backlight(connector);
 
  	return true;

Quoted 33 lines · Jani Nikula

This lets the userspace switch off the backlight using the backlight class sysfs bl_power file. The switch is done using the power sequencer; the backlight PWM, and everything else, remains enabled. The display backlight won't draw power, but for maximum power savings the encoder needs to be switched off.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++
  1 file changed, 22 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +14 -0

index d8baf60ff3fd..f6e3e9a906b0 100644
@@ -1453,6 +1453,27 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
  	intel_panel_disable_backlight(intel_dp->attached_connector);
  }
 
+/*
+ * Hook for controlling the panel power control backlight through the bl_power
+ * sysfs attribute. Take care to handle multiple calls.
+ */
+static void intel_edp_backlight_power(struct intel_connector *connector,
+				      bool enable)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
+	bool is_enabled = ironlake_get_pp_control(intel_dp) & EDP_BLC_ENABLE;
+
+	if (is_enabled == enable)
+		return;
+
+	DRM_DEBUG_KMS("\n");

drivers/gpu/drm/i915/intel_dp.c void intel_edp_backlight_off(struct intel_dp *intel_dp) · line 1474

Quoted 5 lines

drivers/gpu/drm/i915/intel_dp.c +5 -0

+
+	if (enable)
+		_intel_edp_backlight_on(intel_dp);
+	else
+		_intel_edp_backlight_off(intel_dp);

Is there a good reason why we leave the PWM enabled? There is a small power requirement to leave the PWM enabled.

Quoted 15 lines

drivers/gpu/drm/i915/intel_dp.c +3 -0

+}
+
  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
  {
  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);

@@ -4579,6 +4600,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
  	}
 
  	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
+	intel_connector->panel.backlight_power = intel_edp_backlight_power;
  	intel_panel_setup_backlight(connector);
 
  	return true;

Make it possible to change panel power control backlight state without touching the PWM. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 39 ++++++++++++++++++++++++++-------------
  1 file changed, 26 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +20 -13

index e5ada4fbe945..d8baf60ff3fd 100644
@@ -1384,7 +1384,8 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
  	intel_display_power_put(dev_priv, power_domain);
  }
 
-void intel_edp_backlight_on(struct intel_dp *intel_dp)
+/* Enable backlight in the panel power control. */
+static void _intel_edp_backlight_on(struct intel_dp *intel_dp)
  {
  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
  	struct drm_device *dev = intel_dig_port->base.base.dev;

@@ -1392,13 +1393,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
  	u32 pp;
  	u32 pp_ctrl_reg;
 
-	if (!is_edp(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

@@ -1415,17 +1409,26 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp)
  	POSTING_READ(pp_ctrl_reg);
  }
 
-void intel_edp_backlight_off(struct intel_dp *intel_dp)
+/* Enable backlight PWM and backlight PP control. */
+void intel_edp_backlight_on(struct intel_dp *intel_dp)
+{
+	if (!is_edp(intel_dp))
+		return;
+
+	DRM_DEBUG_KMS("\n");
+
+	intel_panel_enable_backlight(intel_dp->attached_connector);
+	_intel_edp_backlight_on(intel_dp);
+}
+
+/* Disable backlight in the panel power control. */
+static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
  {
  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
  	struct drm_i915_private *dev_priv = dev->dev_private;
  	u32 pp;
  	u32 pp_ctrl_reg;
 
-	if (!is_edp(intel_dp))
-		return;
-
-	DRM_DEBUG_KMS("\n");
  	pp = ironlake_get_pp_control(intel_dp);
  	pp &= ~EDP_BLC_ENABLE;
 

@@ -1436,7 +1439,17 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
  	intel_dp->last_backlight_off = jiffies;
 
  	edp_wait_backlight_off(intel_dp);
+}
+
+/* Disable backlight PP control and backlight PWM. */
+void intel_edp_backlight_off(struct intel_dp *intel_dp)

This lets the userspace switch off the backlight using the backlight class sysfs bl_power file. The switch is done using the power sequencer; the backlight PWM, and everything else, remains enabled. The display backlight won't draw power, but for maximum power savings the encoder needs to be switched off.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++
  1 file changed, 22 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +21 -0

index d8baf60ff3fd..f6e3e9a906b0 100644
@@ -1453,6 +1453,27 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp)
  	intel_panel_disable_backlight(intel_dp->attached_connector);
  }
 
+/*
+ * Hook for controlling the panel power control backlight through the bl_power
+ * sysfs attribute. Take care to handle multiple calls.
+ */
+static void intel_edp_backlight_power(struct intel_connector *connector,
+				      bool enable)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
+	bool is_enabled = ironlake_get_pp_control(intel_dp) & EDP_BLC_ENABLE;
+
+	if (is_enabled == enable)
+		return;
+
+	DRM_DEBUG_KMS("\n");
+
+	if (enable)
+		_intel_edp_backlight_on(intel_dp);
+	else
+		_intel_edp_backlight_off(intel_dp);
+}
+
  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
  {
  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);

Make backlight class sysfs bl_power a sub-state of backlight enabled, if a backlight power connector callback is defined. It's up to the connector callback to handle the sub-state, typically in a way that respects panel power sequencing.

v2: Post the version that does not oops. *facepalm*.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
  drivers/gpu/drm/i915/intel_panel.c | 26 ++++++++++++++++++++++++++
  2 files changed, 28 insertions(+)

drivers/gpu/drm/i915/intel_drv.h +2 -0

index 1b3d1d7e466e..43b7b6609f0e 100644
@@ -173,6 +173,8 @@ struct intel_panel {
  		bool active_low_pwm;
  		struct backlight_device *device;
  	} backlight;
+
+	void (*backlight_power)(struct intel_connector *, bool enable);
  };
 
  struct intel_connector {

drivers/gpu/drm/i915/intel_panel.c +21 -0

index 59b028f0b1e8..af5435634929 100644
@@ -751,6 +751,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
 
  	spin_lock_irqsave(&dev_priv->backlight_lock, flags);
 
+	if (panel->backlight.device)
+		panel->backlight.device->props.power = FB_BLANK_POWERDOWN;
  	panel->backlight.enabled = false;
  	dev_priv->display.disable_backlight(connector);
 

@@ -957,6 +959,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 
  	dev_priv->display.enable_backlight(connector);
  	panel->backlight.enabled = true;
+	if (panel->backlight.device)
+		panel->backlight.device->props.power = FB_BLANK_UNBLANK;
 
  	spin_unlock_irqrestore(&dev_priv->backlight_lock, flags);
  }

@@ -965,6 +969,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
  static int intel_backlight_device_update_status(struct backlight_device *bd)
  {
  	struct intel_connector *connector = bl_get_data(bd);
+	struct intel_panel *panel = &connector->panel;
  	struct drm_device *dev = connector->base.dev;
 
  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);

@@ -972,6 +977,22 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
  		      bd->props.brightness, bd->props.max_brightness);
  	intel_panel_set_backlight(connector, bd->props.brightness,
  				  bd->props.max_brightness);
+
+	/*
+	 * Allow flipping bl_power as a sub-state of enabled. Sadly the
+	 * backlight class device does not make it easy to to differentiate
+	 * between callbacks for brightness and bl_power, so our backlight_power
+	 * callback needs to take this into account.
+	 */
+	if (panel->backlight.enabled) {
+		if (panel->backlight_power) {
+			bool enable = bd->props.power == FB_BLANK_UNBLANK;
+			panel->backlight_power(connector, enable);
+		}
+	} else {
+		bd->props.power = FB_BLANK_POWERDOWN;
+	}
+
  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
  	return 0;
  }

@@ -1023,6 +1044,11 @@ static int intel_backlight_device_register(struct intel_connector *connector)
  					    panel->backlight.level,

Make backlight class sysfs brightness 0 value switch off the backlight for connectors that have the backlight_power callback defined. For eDP, this has the similar caveats regarding power savings as bl_power as only the power sequencer backlight control is switched off.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/intel_panel.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_panel.c +0 -0

index c365f2a57c75..574690afadb3 100644

Quoted 16 lines · Jani Nikula

Make backlight class sysfs brightness 0 value switch off the backlight for connectors that have the backlight_power callback defined. For eDP, this has the similar caveats regarding power savings as bl_power as only the power sequencer backlight control is switched off.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_panel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_panel.c +0 -0

index c365f2a57c75..574690afadb3 100644

drivers/gpu/drm/i915/intel_panel.c static int intel_backlight_device_update_status(struct backlight_device *bd) · line 993

Quoted 9 lines

drivers/gpu/drm/i915/intel_panel.c +2 -1

@@ -986,7 +986,8 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
 	 */
 	if (panel->backlight.enabled) {
 		if (panel->backlight_power) {
-			bool enable = bd->props.power == FB_BLANK_UNBLANK;
+			bool enable = bd->props.power == FB_BLANK_UNBLANK &&
+				bd->props.brightness != 0;
 			panel->backlight_power(connector, enable);
 		}
 	} else {

Didn't get a chance to test this on nightly.

Reviewed_by: Clinton Taylor <[email protected]>