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

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e48d47c..a432904 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
-- 
1.9.0

Re: [PATCH] drm/i915: get power domain in case the BIOS enabled eDP VDD

[Daniel Vetter]

On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e48d47c..a432904 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: get power domain in case the BIOS enabled eDP VDD

[Chris Wilson]

On Thu, Apr 10, 2014 at 09:21:50AM +0200, Daniel Vetter wrote:
> On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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 <paulo.r.zanoni@intel.com>
> 
> Is this the bug reported by Chris on irc yesterday? Chris?

Looks likely...

Tested-by: Chris Wilson <chris@chris-wilson.co.uk>

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: get power domain in case the BIOS enabled eDP VDD

[Daniel Vetter]

On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e48d47c..a432904 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Paulo Zanoni]

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

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.

v2: - Rebase

Tested-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 80e5598..44df493 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
-- 
1.9.0

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 61 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44df493..b385b03 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
-- 
1.9.0

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b385b03..a25f708 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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;
-- 
1.9.0

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a25f708..a0b1cda 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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;
 }
-- 
1.9.0

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

[Daniel Vetter]

On Tue, Apr 22, 2014 at 07:55:42PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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.
> 
> v2: - Rebase
> 
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

This regression was introduced in

commit e9cb81a22841908b1c075156b409a538d09c8466
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org (v3.13+)

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 80e5598..44df493 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Tue, Apr 22, 2014 at 07:55:43PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 61 ++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 44df493..b385b03 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> +}

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

> +
>  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);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Tue, Apr 22, 2014 at 07:55:45PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a25f708..a0b1cda 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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;
>  }
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Jani Nikula]

On Wed, 23 Apr 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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.
>
> v2: - Rebase
>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Patch 1/4 pushed to -fixes.

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 80e5598..44df493 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> -- 
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 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.


diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a421c81..8201b60 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b885df1..5f27b51 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 75c1c76..8ed8d53 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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 */
-- 
1.9.0

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 4 ----
 drivers/gpu/drm/i915/intel_dp.c  | 5 -----
 2 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index aa5a3dc..4c673d0 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -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;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 66d202c..104998e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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;
 }
 
-- 
1.9.0

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

[Ville Syrjälä]

On Thu, Apr 24, 2014 at 10:50:56AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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ä <ville.syrjala@linux.intel.com>

> 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 <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 4 ----
>  drivers/gpu/drm/i915/intel_dp.c  | 5 -----
>  2 files changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index aa5a3dc..4c673d0 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -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;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 66d202c..104998e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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;
>  }
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Thu, Apr 24, 2014 at 10:49:28AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>
> ---
>  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.

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ä <ville.syrjala@linux.intel.com>

> 
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a421c81..8201b60 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b885df1..5f27b51 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 75c1c76..8ed8d53 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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 */
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

On Thu, Apr 24, 2014 at 05:11:10PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 24, 2014 at 10:49:28AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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 <paulo.r.zanoni@intel.com>
> > ---
> >  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.
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Thu, Apr 24, 2014 at 05:05:21PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 24, 2014 at 10:50:56AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > We already call intel_dp_power_get, which will get a power domain, and
>                   ^^^^^^^^^^^^^^^^^^
> Commit message needs a small fix. Otherwise looks good.

Fixed.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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