[PATCH v2 1/2] drm/i915: factor out intel_edp_panel_vdd_sanitize

[PATCH v2 1/2] drm/i915: factor out intel_edp_panel_vdd_sanitize

[Imre Deak]

This will be needed by an upcoming patch too that needs to sanitize the
VDD state during resume. The additional async disabling is only needed
for the resume path, here it doesn't make a difference since we enable
VDD right after the sanitize call.

v2:
- don't set intel_dp ptr for non-eDP encoders (Ville)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 60 ++++++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ea6ff71..71294b5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1285,6 +1285,19 @@ static void edp_panel_vdd_work(struct work_struct *__work)
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
+static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp)
+{
+	unsigned long delay;
+
+	/*
+	 * Queue the timer to fire a long time from now (relative to the power
+	 * down delay) to keep the panel power up across a sequence of
+	 * operations.
+	 */
+	delay = msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5);
+	schedule_delayed_work(&intel_dp->panel_vdd_work, delay);
+}
+
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 {
 	if (!is_edp(intel_dp))
@@ -1294,17 +1307,10 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 
 	intel_dp->want_panel_vdd = false;
 
-	if (sync) {
+	if (sync)
 		edp_panel_vdd_off_sync(intel_dp);
-	} else {
-		/*
-		 * Queue the timer to fire a long
-		 * time from now (relative to the power down delay)
-		 * to keep the panel power up across a sequence of operations
-		 */
-		schedule_delayed_work(&intel_dp->panel_vdd_work,
-				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
-	}
+	else
+		edp_panel_vdd_schedule_off(intel_dp);
 }
 
 void intel_edp_panel_on(struct intel_dp *intel_dp)
@@ -4441,6 +4447,32 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 	return downclock_mode;
 }
 
+void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder)
+{
+	struct drm_device *dev = intel_encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_dp *intel_dp;
+	enum intel_display_power_domain power_domain;
+
+	if (intel_encoder->type != INTEL_OUTPUT_EDP)
+		return;
+
+	intel_dp = enc_to_intel_dp(&intel_encoder->base);
+	if (!edp_have_panel_vdd(intel_dp))
+		return;
+	/*
+	 * The VDD bit needs a power domain reference, so if the bit is
+	 * already enabled when we boot or resume, grab this reference and
+	 * schedule a vdd off, so we don't hold on to the reference
+	 * indefinitely.
+	 */
+	DRM_DEBUG_KMS("VDD left on by BIOS, adjusting state tracking\n");
+	power_domain = intel_display_port_power_domain(intel_encoder);
+	intel_display_power_get(dev_priv, power_domain);
+
+	edp_panel_vdd_schedule_off(intel_dp);
+}
+
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 				     struct intel_connector *intel_connector,
 				     struct edp_power_seq *power_seq)
@@ -4461,13 +4493,7 @@ 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);
-	}
+	intel_edp_panel_vdd_sanitize(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 bdcc4a1..28d185d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -909,6 +909,7 @@ bool intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
 void intel_edp_backlight_on(struct intel_dp *intel_dp);
 void intel_edp_backlight_off(struct intel_dp *intel_dp);
 void intel_edp_panel_vdd_on(struct intel_dp *intel_dp);
+void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder);
 void intel_edp_panel_on(struct intel_dp *intel_dp);
 void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/2] drm/i915: fix VDD state tracking after system resume

[Imre Deak]

Just like during booting the BIOS can leave the VDD bit enabled after
system resume. So apply the same state sanitization there too. This
fixes a problem where after resume the port power domain refcount gets
unbalanced.

Reported-and-tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1edfd1a..fdb5657 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12977,6 +12977,12 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	/* HW state is read out, now we need to sanitize this mess. */
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
 			    base.head) {
+		/*
+		 * Do the following only during resume, since at driver
+		 * loading it's done early when initializing the encoder.
+		 */
+		if (force_restore)
+			intel_edp_panel_vdd_sanitize(encoder);
 		intel_sanitize_encoder(encoder);
 	}
 
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: fix VDD state tracking after system resume

[Daniel Vetter]

On Wed, Jul 30, 2014 at 03:57:32PM +0300, Imre Deak wrote:
> Just like during booting the BIOS can leave the VDD bit enabled after
> system resume. So apply the same state sanitization there too. This
> fixes a problem where after resume the port power domain refcount gets
> unbalanced.
> 
> Reported-and-tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1edfd1a..fdb5657 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12977,6 +12977,12 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	/* HW state is read out, now we need to sanitize this mess. */
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
>  			    base.head) {
> +		/*
> +		 * Do the following only during resume, since at driver
> +		 * loading it's done early when initializing the encoder.
> +		 */
> +		if (force_restore)
> +			intel_edp_panel_vdd_sanitize(encoder);

This should be put into an encoder->reset callback.
-Daniel

>  		intel_sanitize_encoder(encoder);
>  	}
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> 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 v2 1/2] drm/i915: factor out intel_edp_panel_vdd_sanitize

[Daniel Vetter]

On Wed, Jul 30, 2014 at 03:57:31PM +0300, Imre Deak wrote:
> This will be needed by an upcoming patch too that needs to sanitize the
> VDD state during resume. The additional async disabling is only needed
> for the resume path, here it doesn't make a difference since we enable
> VDD right after the sanitize call.
> 
> v2:
> - don't set intel_dp ptr for non-eDP encoders (Ville)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 60 ++++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ea6ff71..71294b5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1285,6 +1285,19 @@ static void edp_panel_vdd_work(struct work_struct *__work)
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
> +static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp)
> +{
> +	unsigned long delay;
> +
> +	/*
> +	 * Queue the timer to fire a long time from now (relative to the power
> +	 * down delay) to keep the panel power up across a sequence of
> +	 * operations.
> +	 */
> +	delay = msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5);
> +	schedule_delayed_work(&intel_dp->panel_vdd_work, delay);
> +}
> +
>  static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  {
>  	if (!is_edp(intel_dp))
> @@ -1294,17 +1307,10 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  
>  	intel_dp->want_panel_vdd = false;
>  
> -	if (sync) {
> +	if (sync)
>  		edp_panel_vdd_off_sync(intel_dp);
> -	} else {
> -		/*
> -		 * Queue the timer to fire a long
> -		 * time from now (relative to the power down delay)
> -		 * to keep the panel power up across a sequence of operations
> -		 */
> -		schedule_delayed_work(&intel_dp->panel_vdd_work,
> -				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
> -	}
> +	else
> +		edp_panel_vdd_schedule_off(intel_dp);
>  }
>  
>  void intel_edp_panel_on(struct intel_dp *intel_dp)
> @@ -4441,6 +4447,32 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  	return downclock_mode;
>  }
>  
> +void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder)
> +{
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_dp *intel_dp;
> +	enum intel_display_power_domain power_domain;
> +
> +	if (intel_encoder->type != INTEL_OUTPUT_EDP)
> +		return;
> +
> +	intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +	if (!edp_have_panel_vdd(intel_dp))
> +		return;
> +	/*
> +	 * The VDD bit needs a power domain reference, so if the bit is
> +	 * already enabled when we boot or resume, grab this reference and
> +	 * schedule a vdd off, so we don't hold on to the reference
> +	 * indefinitely.
> +	 */
> +	DRM_DEBUG_KMS("VDD left on by BIOS, adjusting state tracking\n");
> +	power_domain = intel_display_port_power_domain(intel_encoder);
> +	intel_display_power_get(dev_priv, power_domain);
> +
> +	edp_panel_vdd_schedule_off(intel_dp);
> +}
> +
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  				     struct intel_connector *intel_connector,
>  				     struct edp_power_seq *power_seq)
> @@ -4461,13 +4493,7 @@ 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);
> -	}
> +	intel_edp_panel_vdd_sanitize(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 bdcc4a1..28d185d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -909,6 +909,7 @@ bool intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
>  void intel_edp_backlight_on(struct intel_dp *intel_dp);
>  void intel_edp_backlight_off(struct intel_dp *intel_dp);
>  void intel_edp_panel_vdd_on(struct intel_dp *intel_dp);
> +void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder);
>  void intel_edp_panel_on(struct intel_dp *intel_dp);
>  void intel_edp_panel_off(struct intel_dp *intel_dp);
>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
> -- 
> 1.8.4
> 
> _______________________________________________
> 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 v3 2/2] drm/i915: fix VDD state tracking after system resume

[Imre Deak]

Just like during booting the BIOS can leave the VDD bit enabled after
system resume. So apply the same state sanitization there too. This
fixes a problem where after resume the port power domain refcount gets
unbalanced.

v2:
- unchanged
v3:
- call edp sanitizing from the encoder reset handler (Daniel)

Reported-and-tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 71294b5..8741439 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4002,6 +4002,11 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_dig_port);
 }
 
+static void intel_dp_encoder_reset(struct drm_encoder *encoder)
+{
+	intel_edp_panel_vdd_sanitize(to_intel_encoder(encoder));
+}
+
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
@@ -4017,6 +4022,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
 };
 
 static const struct drm_encoder_funcs intel_dp_enc_funcs = {
+	.reset = intel_dp_encoder_reset,
 	.destroy = intel_dp_encoder_destroy,
 };
 
-- 
1.8.4

Re: [PATCH v3 2/2] drm/i915: fix VDD state tracking after system resume

[Ville Syrjälä]

On Thu, Jul 31, 2014 at 02:03:36PM +0300, Imre Deak wrote:
> Just like during booting the BIOS can leave the VDD bit enabled after
> system resume. So apply the same state sanitization there too. This
> fixes a problem where after resume the port power domain refcount gets
> unbalanced.
> 
> v2:
> - unchanged
> v3:
> - call edp sanitizing from the encoder reset handler (Daniel)

It happens a bit earlier than with the earlier attempt, but if
it works it works.

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

> 
> Reported-and-tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 71294b5..8741439 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4002,6 +4002,11 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	kfree(intel_dig_port);
>  }
>  
> +static void intel_dp_encoder_reset(struct drm_encoder *encoder)
> +{
> +	intel_edp_panel_vdd_sanitize(to_intel_encoder(encoder));
> +}
> +
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
>  	.dpms = intel_connector_dpms,
>  	.detect = intel_dp_detect,
> @@ -4017,6 +4022,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
>  };
>  
>  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> +	.reset = intel_dp_encoder_reset,
>  	.destroy = intel_dp_encoder_destroy,
>  };
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 2/2] drm/i915: fix VDD state tracking after system resume

[Daniel Vetter]

On Fri, Aug 01, 2014 at 02:56:54PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 31, 2014 at 02:03:36PM +0300, Imre Deak wrote:
> > Just like during booting the BIOS can leave the VDD bit enabled after
> > system resume. So apply the same state sanitization there too. This
> > fixes a problem where after resume the port power domain refcount gets
> > unbalanced.
> > 
> > v2:
> > - unchanged
> > v3:
> > - call edp sanitizing from the encoder reset handler (Daniel)
> 
> It happens a bit earlier than with the earlier attempt, but if
> it works it works.
> 
> 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