drm/i915: get power domain in case the BIOS enabled eDP VDD

PATCH In review Paulo Zanoni 5 participants 18 messages Apr 9, 2014 – Jul 14, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2014-04-09 21:47 UTC #0 lore raw

PATCH 1/1 drm/i915: get power domain in case the BIOS enabled eDP VDD

From: Paulo Zanoni <[email protected]>

If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero.

The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit.

Signed-off-by Paulo Zanoni <[email protected]>

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

drivers/gpu/drm/i915/intel_dp.c +10 -1

index e48d47c..a432904 100644
@@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	bool has_dpcd;

@@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/* The VDD bit needs a power domain reference, so if the bit is already
+	 * enabled when we boot, grab this reference. */
+	if (edp_have_panel_vdd(intel_dp)) {
+		enum intel_display_power_domain power_domain;
+		power_domain = intel_display_port_power_domain(intel_encoder);
+		intel_display_power_get(dev_priv, power_domain);
+	}
+
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);

Signature · 1 line

1.9.0

Daniel Vetter

2014-04-10 07:21 UTC #1 lore raw

Quoted 9 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero.

Quoted 7 lines

The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit.
Signed-off-by Paulo Zanoni <[email protected]>

Is this the bug reported by Chris on irc yesterday? Chris? -Daniel

Quoted 40 lines

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

drivers/gpu/drm/i915/intel_dp.c +10 -1

index e48d47c..a432904 100644
@@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	bool has_dpcd;

@@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/* The VDD bit needs a power domain reference, so if the bit is already
+	 * enabled when we boot, grab this reference. */
+	if (edp_have_panel_vdd(intel_dp)) {
+		enum intel_display_power_domain power_domain;
+		power_domain = intel_display_port_power_domain(intel_encoder);
+		intel_display_power_get(dev_priv, power_domain);
+	}
+
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

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

Chris Wilson

Tested-by

2014-04-10 07:36 UTC #2 lore raw

Quoted 16 lines · Daniel Vetter

Quoted 15 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero.
The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit.

Quoted 3 lines

Quoted 1 line
Signed-off-by Paulo Zanoni <[email protected]>
Is this the bug reported by Chris on irc yesterday? Chris?

Looks likely...

Tested-by Chris Wilson <[email protected]>

Bikeshed requests:

That chunk is repeated a few times, please refactor it into a small function.

The caller of intel_edp_init_connector() has a redundant if(is_edp) in its error handling path.
-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

2014-04-11 13:21 UTC #3 lore raw

In reply to Paulo Zanoni

Quoted 9 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero.

Quoted 7 lines

The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit.
Signed-off-by Paulo Zanoni <[email protected]>

Probably also

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76987

Paulo can you please polish the patch as requested by Chris and resubmit? Please also repoke the bug.

Thanks, Daniel

Quoted 40 lines

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

drivers/gpu/drm/i915/intel_dp.c +10 -1

index e48d47c..a432904 100644
@@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	bool has_dpcd;

@@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/* The VDD bit needs a power domain reference, so if the bit is already
+	 * enabled when we boot, grab this reference. */
+	if (edp_have_panel_vdd(intel_dp)) {
+		enum intel_display_power_domain power_domain;
+		power_domain = intel_display_port_power_domain(intel_encoder);
+		intel_display_power_get(dev_priv, power_domain);
+	}
+
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

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

Paulo Zanoni

author

2014-04-22 22:56 UTC #4 lore raw

PATCH 1/4 drm/i915: get power domain in case the BIOS enabled eDP VDD

From: Paulo Zanoni <[email protected]>

v2: - Rebase

Tested-by Chris Wilson <[email protected]> (v1)

Signed-off-by Paulo Zanoni <[email protected]>

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

drivers/gpu/drm/i915/intel_dp.c +10 -1

index 80e5598..44df493 100644
@@ -3785,7 +3785,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	struct drm_display_mode *downclock_mode = NULL;

@@ -3798,6 +3799,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/* The VDD bit needs a power domain reference, so if the bit is already
+	 * enabled when we boot, grab this reference. */
+	if (edp_have_panel_vdd(intel_dp)) {
+		enum intel_display_power_domain power_domain;
+		power_domain = intel_display_port_power_domain(intel_encoder);
+		intel_display_power_get(dev_priv, power_domain);
+	}
+
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);

Signature · 1 line

1.9.0

Paulo Zanoni

author

2014-04-22 22:56 UTC #5 lore raw

PATCH 2/4 drm/i915: add intel_dp_power_get/put

From: Paulo Zanoni <[email protected]>

This was suggested by Chris on his review to the first version of "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, at least that's what I understood from his comment :)

The downside of this patch is that in a few places we will call intel_display_port_power_domain() twice instead of once.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 61 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +29 ~~-32~~

index 44df493..b385b03 100644
@@ -1063,13 +1063,30 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
 	return control;
 }
 
+static void intel_dp_power_get(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_get(dev_priv, power_domain);
+}
+
+static void intel_dp_power_put(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_put(dev_priv, power_domain);
+}
+
 static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
 	bool need_to_disable = !intel_dp->want_panel_vdd;

@@ -1082,8 +1099,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (edp_have_panel_vdd(intel_dp))
 		return need_to_disable;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_dp_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("Turning eDP VDD on\n");

@@ -1133,7 +1149,6 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		struct intel_digital_port *intel_dig_port =
 						dp_to_dig_port(intel_dp);
 		struct intel_encoder *intel_encoder = &intel_dig_port->base;
-		enum intel_display_power_domain power_domain;
 
 		DRM_DEBUG_KMS("Turning eDP VDD off\n");

@@ -1153,8 +1168,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		if ((pp & POWER_TARGET_ON) == 0)
 			intel_dp->last_power_cycle = jiffies;
 
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_put(dev_priv, power_domain);
+		intel_dp_power_put(intel_encoder);
 	}
 }

@@ -1242,7 +1256,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_ctrl_reg;

@@ -1272,8 +1285,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	wait_panel_off(intel_dp);
 
 	/* We got a reference when we enabled the VDD. */
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_encoder);
 }
 
 void intel_edp_backlight_on(struct intel_dp *intel_dp)

@@ -3161,13 +3173,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
-	enum intel_display_power_domain power_domain;
 	struct edid *edid = NULL;
 
 	intel_runtime_pm_get(dev_priv);
-
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_dp_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, drm_get_connector_name(connector));

@@ -3199,8 +3208,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	status = connector_status_connected;
 
 out:
-	intel_display_power_put(dev_priv, power_domain);
-
+	intel_dp_power_put(intel_encoder);
 	intel_runtime_pm_put(dev_priv);
 
 	return status;

@@ -3213,18 +3221,14 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	int ret;
 
 	/* We should parse the EDID data and find out if it has an audio sink
 	 */
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
+	intel_dp_power_get(intel_encoder);
 	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_encoder);
 	if (ret)
 		return ret;

@@ -3247,14 +3251,10 @@ intel_dp_detect_audio(struct drm_connector *connector)
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	struct edid *edid;
 	bool has_audio = false;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_dp_power_get(intel_encoder);
 
 	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {

@@ -3262,7 +3262,7 @@ intel_dp_detect_audio(struct drm_connector *connector)
 		kfree(edid);
 	}
 
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_encoder);
 
 	return has_audio;
 }

@@ -3801,11 +3801,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* The VDD bit needs a power domain reference, so if the bit is already
 	 * enabled when we boot, grab this reference. */
-	if (edp_have_panel_vdd(intel_dp)) {
-		enum intel_display_power_domain power_domain;
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_get(dev_priv, power_domain);
-	}
+	if (edp_have_panel_vdd(intel_dp))
+		intel_dp_power_get(intel_encoder);
 
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);

Signature · 1 line

1.9.0

Ville Syrjälä

2014-04-23 09:45 UTC #6 lore raw

Re: [PATCH 2/4] drm/i915: add intel_dp_power_get/put

Quoted 29 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
This was suggested by Chris on his review to the first version of "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, at least that's what I understood from his comment :)
The downside of this patch is that in a few places we will call intel_display_port_power_domain() twice instead of once.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 61 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 32 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +7 -0
index 44df493..b385b03 100644
@@ -1063,13 +1063,30 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
 	return control;
 }
 
+static void intel_dp_power_get(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_get(dev_priv, power_domain);

drivers/gpu/drm/i915/intel_dp.c static u32 ironlake_get_pp_control(struct intel_dp *intel_dp) · line 1082

Quoted 10 lines

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

+}
+
+static void intel_dp_power_put(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_put(dev_priv, power_domain);
+}

There's nothing DP specific about these functions, so they could be moved somewhere else and used for other output types as well.

Quoted 149 lines

drivers/gpu/drm/i915/intel_dp.c +12 ~~-32~~

+
 static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
 	bool need_to_disable = !intel_dp->want_panel_vdd;

@@ -1082,8 +1099,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (edp_have_panel_vdd(intel_dp))
 		return need_to_disable;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_dp_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("Turning eDP VDD on\n");

@@ -1133,7 +1149,6 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		struct intel_digital_port *intel_dig_port =
 						dp_to_dig_port(intel_dp);
 		struct intel_encoder *intel_encoder = &intel_dig_port->base;
-		enum intel_display_power_domain power_domain;
 
 		DRM_DEBUG_KMS("Turning eDP VDD off\n");

@@ -1153,8 +1168,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		if ((pp & POWER_TARGET_ON) == 0)
 			intel_dp->last_power_cycle = jiffies;
 
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_put(dev_priv, power_domain);
+		intel_dp_power_put(intel_encoder);
 	}
 }

@@ -1242,7 +1256,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_ctrl_reg;

@@ -1272,8 +1285,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	wait_panel_off(intel_dp);
 
 	/* We got a reference when we enabled the VDD. */
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_encoder);
 }
 
 void intel_edp_backlight_on(struct intel_dp *intel_dp)

@@ -3161,13 +3173,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
-	enum intel_display_power_domain power_domain;
 	struct edid *edid = NULL;
 
 	intel_runtime_pm_get(dev_priv);
-
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_dp_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, drm_get_connector_name(connector));

@@ -3199,8 +3208,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	status = connector_status_connected;
 
 out:
-	intel_display_power_put(dev_priv, power_domain);
-
+	intel_dp_power_put(intel_encoder);
 	intel_runtime_pm_put(dev_priv);
 
 	return status;

@@ -3213,18 +3221,14 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	int ret;
 
 	/* We should parse the EDID data and find out if it has an audio sink
 	 */
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
+	intel_dp_power_get(intel_encoder);
 	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_encoder);
 	if (ret)
 		return ret;

@@ -3247,14 +3251,10 @@ intel_dp_detect_audio(struct drm_connector *connector)
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	struct edid *edid;
 	bool has_audio = false;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_dp_power_get(intel_encoder);
 
 	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {

@@ -3262,7 +3262,7 @@ intel_dp_detect_audio(struct drm_connector *connector)
 		kfree(edid);
 	}
 
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_encoder);
 
 	return has_audio;
 }

@@ -3801,11 +3801,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* The VDD bit needs a power domain reference, so if the bit is already
 	 * enabled when we boot, grab this reference. */
-	if (edp_have_panel_vdd(intel_dp)) {
-		enum intel_display_power_domain power_domain;
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_get(dev_priv, power_domain);
-	}
+	if (edp_have_panel_vdd(intel_dp))
+		intel_dp_power_get(intel_encoder);
 
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Ville Syrjälä
Intel OTC

Paulo Zanoni

author

2014-04-24 13:49 UTC #7 lore raw

PATCH 2/4 drm/i915: add intel_encoder_power_get/put

From: Paulo Zanoni <[email protected]>

This was suggested by Chris on his review to the first version of "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, at least that's what I understood from his comment :)

v2: - Make it intel_encoder_power_get/put
- Don't call the new functions on functions where we need both get() and put(), so we don't end up calling intel_display_port_power_domain() more than needed.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 19 +++++--------------
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 14 deletions(-)

I am not really sure if this patch is worth it... I certainly won't be mad if we drop it. The diffstat is not good.

drivers/gpu/drm/i915/intel_dp.c +5 ~~-14~~

index a421c81..8201b60 100644
@@ -1069,7 +1069,6 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
 	bool need_to_disable = !intel_dp->want_panel_vdd;

@@ -1082,8 +1081,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (edp_have_panel_vdd(intel_dp))
 		return need_to_disable;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_encoder_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("Turning eDP VDD on\n");

@@ -1133,7 +1131,6 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		struct intel_digital_port *intel_dig_port =
 						dp_to_dig_port(intel_dp);
 		struct intel_encoder *intel_encoder = &intel_dig_port->base;
-		enum intel_display_power_domain power_domain;
 
 		DRM_DEBUG_KMS("Turning eDP VDD off\n");

@@ -1153,8 +1150,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		if ((pp & POWER_TARGET_ON) == 0)
 			intel_dp->last_power_cycle = jiffies;
 
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_put(dev_priv, power_domain);
+		intel_encoder_power_put(intel_encoder);
 	}
 }

@@ -1242,7 +1238,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_ctrl_reg;

@@ -1272,8 +1267,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	wait_panel_off(intel_dp);
 
 	/* We got a reference when we enabled the VDD. */
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_put(dev_priv, power_domain);
+	intel_encoder_power_put(intel_encoder);
 }
 
 void intel_edp_backlight_on(struct intel_dp *intel_dp)

@@ -3798,11 +3792,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* The VDD bit needs a power domain reference, so if the bit is already
 	 * enabled when we boot, grab this reference. */
-	if (edp_have_panel_vdd(intel_dp)) {
-		enum intel_display_power_domain power_domain;
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_get(dev_priv, power_domain);
-	}
+	if (edp_have_panel_vdd(intel_dp))
+		intel_encoder_power_get(intel_encoder);
 
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);

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

index b885df1..5f27b51 100644
@@ -920,6 +920,8 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+void intel_encoder_power_get(struct intel_encoder *encoder);
+void intel_encoder_power_put(struct intel_encoder *encoder);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
 void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);

drivers/gpu/drm/i915/intel_pm.c +24 -0

index 75c1c76..8ed8d53 100644
@@ -5734,6 +5734,30 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	intel_runtime_pm_put(dev_priv);
 }
 
+/*
+ * Short version for when you don't need to use both get() and put() on the same
+ * function. If you need both, consider calling intel_display_power_get/put()
+ * directly so you will save a call to intel_display_port_power_domain().
+ */
+void intel_encoder_power_get(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_get(dev_priv, power_domain);
+}
+
+/* See the comment above intel_encoder_power_get(). */
+void intel_encoder_power_put(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_put(dev_priv, power_domain);
+}
+
 static struct i915_power_domains *hsw_pwr;
 
 /* Display audio driver power well request */

Signature · 1 line

1.9.0

Ville Syrjälä

Reviewed-by on v1 2/4

2014-04-24 14:11 UTC #8 lore raw

Re: [PATCH 2/4] drm/i915: add intel_encoder_power_get/put

Quoted 19 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
This was suggested by Chris on his review to the first version of "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, at least that's what I understood from his comment :)
v2: - Make it intel_encoder_power_get/put
- Don't call the new functions on functions where we need both get() and put(), so we don't end up calling intel_display_port_power_domain() more than needed.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c  | 19 +++++--------------
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 14 deletions(-)

Quoted 2 lines

I am not really sure if this patch is worth it... I certainly won't be mad if we drop it. The diffstat is not good.

I'm not sure either. In fact I had this same idea already when I saw Imre's power well patches for the first time, but then I thought that it doesn't help that much and decided to keep the idea to myself.

The patch itself looks correct however, and it doesn't make the code any worse IMHO so:

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 129 lines

drivers/gpu/drm/i915/intel_dp.c +5 ~~-14~~

index a421c81..8201b60 100644
@@ -1069,7 +1069,6 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
 	bool need_to_disable = !intel_dp->want_panel_vdd;

@@ -1082,8 +1081,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (edp_have_panel_vdd(intel_dp))
 		return need_to_disable;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_encoder_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("Turning eDP VDD on\n");

@@ -1133,7 +1131,6 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		struct intel_digital_port *intel_dig_port =
 						dp_to_dig_port(intel_dp);
 		struct intel_encoder *intel_encoder = &intel_dig_port->base;
-		enum intel_display_power_domain power_domain;
 
 		DRM_DEBUG_KMS("Turning eDP VDD off\n");

@@ -1153,8 +1150,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		if ((pp & POWER_TARGET_ON) == 0)
 			intel_dp->last_power_cycle = jiffies;
 
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_put(dev_priv, power_domain);
+		intel_encoder_power_put(intel_encoder);
 	}
 }

@@ -1242,7 +1238,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
 	u32 pp;
 	u32 pp_ctrl_reg;

@@ -1272,8 +1267,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	wait_panel_off(intel_dp);
 
 	/* We got a reference when we enabled the VDD. */
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_put(dev_priv, power_domain);
+	intel_encoder_power_put(intel_encoder);
 }
 
 void intel_edp_backlight_on(struct intel_dp *intel_dp)

@@ -3798,11 +3792,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	/* The VDD bit needs a power domain reference, so if the bit is already
 	 * enabled when we boot, grab this reference. */
-	if (edp_have_panel_vdd(intel_dp)) {
-		enum intel_display_power_domain power_domain;
-		power_domain = intel_display_port_power_domain(intel_encoder);
-		intel_display_power_get(dev_priv, power_domain);
-	}
+	if (edp_have_panel_vdd(intel_dp))
+		intel_encoder_power_get(intel_encoder);
 
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);

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

index b885df1..5f27b51 100644
@@ -920,6 +920,8 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+void intel_encoder_power_get(struct intel_encoder *encoder);
+void intel_encoder_power_put(struct intel_encoder *encoder);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
 void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);

drivers/gpu/drm/i915/intel_pm.c +24 -0

index 75c1c76..8ed8d53 100644
@@ -5734,6 +5734,30 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	intel_runtime_pm_put(dev_priv);
 }
 
+/*
+ * Short version for when you don't need to use both get() and put() on the same
+ * function. If you need both, consider calling intel_display_power_get/put()
+ * directly so you will save a call to intel_display_port_power_domain().
+ */
+void intel_encoder_power_get(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_get(dev_priv, power_domain);
+}
+
+/* See the comment above intel_encoder_power_get(). */
+void intel_encoder_power_put(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_put(dev_priv, power_domain);
+}
+
 static struct i915_power_domains *hsw_pwr;
 
 /* Display audio driver power well request */

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Ville Syrjälä
Intel OTC

Daniel Vetter

2014-04-24 15:14 UTC #9 lore raw

Re: [PATCH 2/4] drm/i915: add intel_encoder_power_get/put

Quoted 20 lines · Ville Syrjälä

Quoted 19 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
This was suggested by Chris on his review to the first version of "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, at least that's what I understood from his comment :)
v2: - Make it intel_encoder_power_get/put
- Don't call the new functions on functions where we need both get() and put(), so we don't end up calling intel_display_port_power_domain() more than needed.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c  | 19 +++++--------------
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 14 deletions(-)

Quoted 6 lines

Quoted 2 lines
I am not really sure if this patch is worth it... I certainly won't be mad if we drop it. The diffstat is not good.
I'm not sure either. In fact I had this same idea already when I saw Imre's power well patches for the first time, but then I thought that it doesn't help that much and decided to keep the idea to myself.

If you guys ask about my color choices I have to admit that I'm not a huge fan of intel_display_port_power_domain. In roughly object-oriented code functions with giant switch statements which use the object class to decide what to do tend to be a red flag ...

In case you care for a reference see the switch statement code smell in Martin Fowler's refactoring.

But that was just my bikeshed, tbh I don't care strongly enough here. -Daniel

Signature · 3 lines

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

Paulo Zanoni

author

2014-04-22 22:56 UTC #10 lore raw

PATCH 3/4 drm/i915: remove redundant is_edp() check

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

If intel_edp_init_connector() returns false, then we know that is_edp() is true because of the early return at intel_edp_init_connector(). So remove the redundant check.

Change proposed by Chris on his review to "drm/i915: get power domain in case the BIOS enabled eDP VDD".

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +4 -6

index b385b03..a25f708 100644
@@ -3958,12 +3958,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 
 	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
 		drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
-		if (is_edp(intel_dp)) {
-			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-			mutex_lock(&dev->mode_config.mutex);
-			edp_panel_vdd_off_sync(intel_dp);
-			mutex_unlock(&dev->mode_config.mutex);
-		}
+		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		mutex_lock(&dev->mode_config.mutex);
+		edp_panel_vdd_off_sync(intel_dp);
+		mutex_unlock(&dev->mode_config.mutex);
 		drm_sysfs_connector_remove(connector);
 		drm_connector_cleanup(connector);
 		return false;

Signature · 1 line

1.9.0

Paulo Zanoni

author

2014-04-22 22:56 UTC #11 lore raw

PATCH 4/4 drm/i915: remove useless runtime PM get call

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We already call intel_dp_power_get, which will get a power domain, and every power domain should get a runtime PM reference, which will wake up the machine.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 3 ---
 1 file changed, 3 deletions(-)

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

index a25f708..a0b1cda 100644
@@ -3171,11 +3171,9 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
 	struct edid *edid = NULL;
 
-	intel_runtime_pm_get(dev_priv);
 	intel_dp_power_get(intel_encoder);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",

@@ -3209,7 +3207,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_dp_power_put(intel_encoder);
-	intel_runtime_pm_put(dev_priv);
 
 	return status;
 }

Signature · 1 line

1.9.0

Ville Syrjälä

2014-04-23 09:53 UTC #12 lore raw

Re: [PATCH 4/4] drm/i915: remove useless runtime PM get call

Quoted 5 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
We already call intel_dp_power_get, which will get a power domain, and every power domain should get a runtime PM reference, which will wake up the machine.

intel_crt_detect() could use the same treatment

Quoted 39 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 3 ---
 1 file changed, 3 deletions(-)

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

index a25f708..a0b1cda 100644
@@ -3171,11 +3171,9 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
 	struct edid *edid = NULL;
 
-	intel_runtime_pm_get(dev_priv);
 	intel_dp_power_get(intel_encoder);

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

 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",

@@ -3209,7 +3207,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_dp_power_put(intel_encoder);
-	intel_runtime_pm_put(dev_priv);
 
 	return status;
 }

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Ville Syrjälä
Intel OTC

Paulo Zanoni

author

2014-04-24 13:51 UTC #13 lore raw

PATCH 4/4 drm/i915: remove useless runtime PM get calls

From: Paulo Zanoni <[email protected]>

We already call intel_dp_power_get, which will get a power domain, and every power domain should get a runtime PM reference, which will wake up the machine.

v2: - Also touch intel_crt_detect() (Ville).

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c | 4 ----
 drivers/gpu/drm/i915/intel_dp.c  | 5 -----
 2 files changed, 9 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +0 -4

index aa5a3dc..4c673d0 100644
@@ -646,8 +646,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status status;
 	struct intel_load_detect_pipe tmp;
 
-	intel_runtime_pm_get(dev_priv);
-
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, drm_get_connector_name(connector),
 		      force);

@@ -699,8 +697,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
-	intel_runtime_pm_put(dev_priv);
-
 	return status;
 }

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

index 66d202c..104998e 100644
@@ -3155,8 +3155,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	enum intel_display_power_domain power_domain;
 	struct edid *edid = NULL;
 
-	intel_runtime_pm_get(dev_priv);
-
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);

@@ -3191,9 +3189,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
-
-	intel_runtime_pm_put(dev_priv);
-
 	return status;
 }

Signature · 1 line

1.9.0

Ville Syrjälä

Reviewed-by on v1 4/4

2014-04-24 14:06 UTC #14 lore raw

Re: [PATCH 4/4] drm/i915: remove useless runtime PM get calls

Quoted 3 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
We already call intel_dp_power_get, which will get a power domain, and

                  ^^^^^^^^^^^^^^^^^^

Commit message needs a small fix. Otherwise looks good.

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 63 lines

every power domain should get a runtime PM reference, which will wake up the machine.

v2: - Also touch intel_crt_detect() (Ville).

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c | 4 ----
 drivers/gpu/drm/i915/intel_dp.c  | 5 -----
 2 files changed, 9 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +0 -4

index aa5a3dc..4c673d0 100644
@@ -646,8 +646,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status status;
 	struct intel_load_detect_pipe tmp;
 
-	intel_runtime_pm_get(dev_priv);
-
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, drm_get_connector_name(connector),
 		      force);

@@ -699,8 +697,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
-	intel_runtime_pm_put(dev_priv);
-
 	return status;
 }

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

index 66d202c..104998e 100644
@@ -3155,8 +3155,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	enum intel_display_power_domain power_domain;
 	struct edid *edid = NULL;
 
-	intel_runtime_pm_get(dev_priv);
-
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);

@@ -3191,9 +3189,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
-
-	intel_runtime_pm_put(dev_priv);
-
 	return status;
 }

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Ville Syrjälä
Intel OTC

Daniel Vetter

Applied

2014-07-14 15:59 UTC #15 lore raw

Re: [PATCH 4/4] drm/i915: remove useless runtime PM get calls

Quoted 6 lines · Ville Syrjälä

Quoted 3 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We already call intel_dp_power_get, which will get a power domain, and
                  ^^^^^^^^^^^^^^^^^^
Commit message needs a small fix. Otherwise looks good.

Fixed.

Quoted 2 lines

Reviewed-by Ville Syrjälä <[email protected]>

Queued for -next, thanks for the patch.
-Daniel

Signature · 3 lines

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

Daniel Vetter

Reviewed-by on v1 1/4

2014-04-23 07:00 UTC #16 lore raw

In reply to Paulo Zanoni

Quoted 15 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero.
The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit.

Quoted 4 lines

v2: - Rebase
Tested-by Chris Wilson <[email protected]> (v1)
Signed-off-by Paulo Zanoni <[email protected]>

This regression was introduced in

commit e9cb81a22841908b1c075156b409a538d09c8466

Author: Paulo Zanoni <[email protected]>
Date:   Thu Nov 21 13:47:23 2013 -0200

drm/i915: get a runtime PM reference when the panel VDD is on

Please don't forget the regression notice! With that this is

Reviewed-by Daniel Vetter <[email protected]>

Cc [email protected] (v3.13+)

Quoted 40 lines

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

drivers/gpu/drm/i915/intel_dp.c +10 -1

index 80e5598..44df493 100644
@@ -3785,7 +3785,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	struct drm_display_mode *downclock_mode = NULL;

@@ -3798,6 +3799,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/* The VDD bit needs a power domain reference, so if the bit is already
+	 * enabled when we boot, grab this reference. */
+	if (edp_have_panel_vdd(intel_dp)) {
+		enum intel_display_power_domain power_domain;
+		power_domain = intel_display_port_power_domain(intel_encoder);
+		intel_display_power_get(dev_priv, power_domain);
+	}
+
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

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

Jani Nikula

maintainer

2014-04-23 11:02 UTC #17 lore raw

In reply to Paulo Zanoni

Quoted 15 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero.
The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit.

Quoted 4 lines

v2: - Rebase
Tested-by Chris Wilson <[email protected]> (v1)
Signed-off-by Paulo Zanoni <[email protected]>

Patch 1/4 pushed to -fixes.

BR,
Jani.

Quoted 40 lines

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

drivers/gpu/drm/i915/intel_dp.c +10 -1

index 80e5598..44df493 100644
@@ -3785,7 +3785,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	struct drm_display_mode *downclock_mode = NULL;

@@ -3798,6 +3799,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/* The VDD bit needs a power domain reference, so if the bit is already
+	 * enabled when we boot, grab this reference. */
+	if (edp_have_panel_vdd(intel_dp)) {
+		enum intel_display_power_domain power_domain;
+		power_domain = intel_display_port_power_domain(intel_encoder);
+		intel_display_power_get(dev_priv, power_domain);
+	}
+
 	/* Cache DPCD and EDID for edp. */
 	intel_edp_panel_vdd_on(intel_dp);
 	has_dpcd = intel_dp_get_dpcd(intel_dp);

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center