[PATCH 0/4] drm/i915: backlight sysfs bl_power and brightness == 0

[PATCH 0/4] drm/i915: backlight sysfs bl_power and brightness == 0

[Jani Nikula]

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 <jani.nikula@intel.com>
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(-)

-- 
1.9.1

[PATCH 1/4] drm/i915/dp: split up panel power control from backlight pwm control

[Jani Nikula]

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

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e5ada4fbe945..d8baf60ff3fd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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)
+{
+	if (!is_edp(intel_dp))
+		return;
+
+	DRM_DEBUG_KMS("\n");
 
+	_intel_edp_backlight_off(intel_dp);
 	intel_panel_disable_backlight(intel_dp->attached_connector);
 }
 
-- 
1.9.1

[PATCH 2/4] drm/i915: add some framework for backlight bl_power support

[Jani Nikula]

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.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 drivers/gpu/drm/i915/intel_panel.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1b3d1d7e466e..43b7b6609f0e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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 {
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 59b028f0b1e8..c365f2a57c75 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -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,
 					    props.max_brightness);
 
+	if (panel->backlight.enabled)
+		panel->backlight.device->props.power = FB_BLANK_UNBLANK;
+	else
+		panel->backlight.device->props.power = FB_BLANK_POWERDOWN;
+
 	/*
 	 * Note: using the same name independent of the connector prevents
 	 * registration of multiple backlight devices in the driver.
-- 
1.9.1

[PATCH 3/4] drm/i915/dp: make backlight bl_power control power sequencer backlight

[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 <jani.nikula@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 d8baf60ff3fd..f6e3e9a906b0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
@@ -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;
-- 
1.9.1

[PATCH 4/4] drm/i915: switch off backlight for backlight class 0 brightness

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c365f2a57c75..574690afadb3 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -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 {
-- 
1.9.1

[PATCH v2 2/4] drm/i915: add some framework for backlight bl_power support

[Jani Nikula]

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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 drivers/gpu/drm/i915/intel_panel.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1b3d1d7e466e..43b7b6609f0e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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 {
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 59b028f0b1e8..af5435634929 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -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,
 					    props.max_brightness);
 
+	if (panel->backlight.enabled)
+		props.power = FB_BLANK_UNBLANK;
+	else
+		props.power = FB_BLANK_POWERDOWN;
+
 	/*
 	 * Note: using the same name independent of the connector prevents
 	 * registration of multiple backlight devices in the driver.
-- 
1.9.1

Re: [PATCH 0/4] drm/i915: backlight sysfs bl_power and brightness == 0

[Clint Taylor]

On 08/12/2014 07:11 AM, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> 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.
>
I have hand merged the first 3 patches of this series into kernel 3.10 
Chromium and the bl_power sysfs entry is functional and turns off 
backlight power independent of the PWM as designed.

The fourth patch is non-functional due to the difference between the 11 
Chromium backlight patches from April 2014 and current upstream 
implementation.

Clint

>
> 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(-)
>

Re: [PATCH 1/4] drm/i915/dp: split up panel power control from backlight pwm control

[Clint Taylor]

On 08/12/2014 07:11 AM, Jani Nikula wrote:
> Make it possible to change panel power control backlight state without
> touching the PWM. No functional changes.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 39 ++++++++++++++++++++++++++-------------
>   1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e5ada4fbe945..d8baf60ff3fd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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)
> +{
> +	if (!is_edp(intel_dp))
> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
>
> +	_intel_edp_backlight_off(intel_dp);
>   	intel_panel_disable_backlight(intel_dp->attached_connector);
>   }
>
>

Patch works as expected.

Some day we need to rename intel_panel_disable_backlight() to 
intel_panel_disable_backlight_pwm() to make this code more readable as 
all intel_panel_backlight_ methods only affect the PWM.

Clint

Re: [PATCH 3/4] drm/i915/dp: make backlight bl_power control power sequencer backlight

[Clint Taylor]

On 08/12/2014 07:11 AM, Jani Nikula wrote:
> 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 <jani.nikula@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 d8baf60ff3fd..f6e3e9a906b0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);

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

> +}
> +
>   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;
>

Re: [PATCH 3/4] drm/i915/dp: make backlight bl_power control power sequencer backlight

[Jani Nikula]

On Mon, 18 Aug 2014, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> On 08/12/2014 07:11 AM, Jani Nikula wrote:
>> 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 <jani.nikula@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 d8baf60ff3fd..f6e3e9a906b0 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -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);
>
> Is there a good reason why we leave the PWM enabled? There is a small 
> power requirement to leave the PWM enabled.

Implementation simplicity. If you want to go for max power savings, you
are supposed to switch everything off using KMS, not just backlight.

BR,
Jani.


>
>> +}
>> +
>>   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;
>>
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915/dp: split up panel power control from backlight pwm control

[Clint Taylor]

On 08/12/2014 07:11 AM, Jani Nikula wrote:
> Make it possible to change panel power control backlight state without
> touching the PWM. No functional changes.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 39 ++++++++++++++++++++++++++-------------
>   1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e5ada4fbe945..d8baf60ff3fd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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)
> +{
> +	if (!is_edp(intel_dp))
> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
>
> +	_intel_edp_backlight_off(intel_dp);
>   	intel_panel_disable_backlight(intel_dp->attached_connector);
>   }
>
>
Reviewed_by: Clinton Taylor <Clinton.A.Taylor@intel.com>
Tested_by: Clinton Taylor <Clinton.A.Taylor@intel.com>

Re: [PATCH 3/4] drm/i915/dp: make backlight bl_power control power sequencer backlight

[Clint Taylor]

On 08/12/2014 07:11 AM, Jani Nikula wrote:
> 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 <jani.nikula@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 d8baf60ff3fd..f6e3e9a906b0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> @@ -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;
>
Reviewed_by: Clinton Taylor <Clinton.A.Taylor@intel.com>
Tested_by: Clinton Taylor <Clinton.A.Taylor@intel.com>

Re: [PATCH v2 2/4] drm/i915: add some framework for backlight bl_power support

[Clint Taylor]

On 08/13/2014 02:10 AM, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>   drivers/gpu/drm/i915/intel_panel.c | 26 ++++++++++++++++++++++++++
>   2 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1b3d1d7e466e..43b7b6609f0e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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 {
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 59b028f0b1e8..af5435634929 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -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,
>   					    props.max_brightness);
>
> +	if (panel->backlight.enabled)
> +		props.power = FB_BLANK_UNBLANK;
> +	else
> +		props.power = FB_BLANK_POWERDOWN;
> +
>   	/*
>   	 * Note: using the same name independent of the connector prevents
>   	 * registration of multiple backlight devices in the driver.
>
Reviewed_by: Clinton Taylor <Clinton.A.Taylor@intel.com>
Tested_by: Clinton Taylor <Clinton.A.Taylor@intel.com>

Re: [PATCH 4/4] drm/i915: switch off backlight for backlight class 0 brightness

[Clint Taylor]

On 08/12/2014 07:11 AM, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_panel.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index c365f2a57c75..574690afadb3 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -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 <Clinton.A.Taylor@intel.com>

Re: [PATCH 4/4] drm/i915: switch off backlight for backlight class 0 brightness

[Daniel Vetter]

On Tue, Aug 19, 2014 at 04:04:50PM -0700, Clint Taylor wrote:
> On 08/12/2014 07:11 AM, Jani Nikula wrote:
> >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 <jani.nikula@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_panel.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >index c365f2a57c75..574690afadb3 100644
> >--- a/drivers/gpu/drm/i915/intel_panel.c
> >+++ b/drivers/gpu/drm/i915/intel_panel.c
> >@@ -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 <Clinton.A.Taylor@intel.com>

All 4 patches merged, thanks.

Aside: I really liked that your review here didn't just amount to an r-b
tag, but had some comments and questions - that gives my a nice warm&fuzzy
feeling that a real review has actually been done and not just some patch
rubber stamping.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch