[PATCH 3/4] drm/i915: make sure VDD is turned off during system suspend

[PATCH 3/4] drm/i915: make sure VDD is turned off during system suspend

[Imre Deak]

Atm we may leave eDP VDD enabled during system suspend after the CRTCs
are disabled through an HPD->DPCD read event. So disable VDD during
suspend at a point when no HPDs can occur.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 15 +++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 13 +++++++++++--
 drivers/gpu/drm/i915/intel_drv.h |  6 ++++++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0653761..1c7979e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -501,6 +501,19 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 	cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
 }
 
+static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_encoder *intel_encoder;
+
+	drm_modeset_lock_all(dev);
+	for_each_intel_encoder(dev, intel_encoder) {
+		if (intel_encoder->suspend)
+			intel_encoder->suspend(intel_encoder);
+	}
+	drm_modeset_unlock_all(dev);
+}
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -547,6 +560,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 		intel_runtime_pm_disable_interrupts(dev);
 		intel_hpd_cancel_work(dev_priv);
 
+		intel_suspend_encoders(dev_priv);
+
 		intel_suspend_gt_powersave(dev);
 
 		intel_modeset_suspend_hw(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 34e3c47..d7f5d0a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1291,8 +1291,6 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 	if (!is_edp(intel_dp))
 		return;
 
-	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
-
 	intel_dp->want_panel_vdd = false;
 
 	if (sync)
@@ -4003,6 +4001,16 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_dig_port);
 }
 
+void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
+
+	if (!is_edp(intel_dp))
+		return;
+
+	edp_panel_vdd_off(intel_dp, true);
+}
+
 static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 {
 	intel_edp_panel_vdd_sanitize(to_intel_encoder(encoder));
@@ -4722,6 +4730,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	intel_encoder->disable = intel_disable_dp;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
 	intel_encoder->get_config = intel_dp_get_config;
+	intel_encoder->suspend = intel_dp_encoder_suspend;
 	if (IS_CHERRYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
 		intel_encoder->pre_enable = chv_pre_enable_dp;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1b3d1d7..cefd337 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -153,6 +153,12 @@ struct intel_encoder {
 	 * be set correctly before calling this function. */
 	void (*get_config)(struct intel_encoder *,
 			   struct intel_crtc_config *pipe_config);
+	/*
+	 * Called during system suspend after all pending requests for the
+	 * encoder are flushed (for example for DP AUX transactions) and
+	 * device interrupts are disabled.
+	 */
+	void (*suspend)(struct intel_encoder *);
 	int crtc_mask;
 	enum hpd_pin hpd_pin;
 };
-- 
1.8.4

[PATCH 4/4] drm/i915: don't try to retrain a DP link on an inactive CRTC

[Imre Deak]

Atm we may retrain the DP link even if the CRTC is inactive through
HPD work->intel_dp_check_link_status(). This in turn can lock up the PHY
(at least on BYT), since the DP port is disabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81948
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d7f5d0a..49de9be 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3551,6 +3551,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (WARN_ON(!intel_encoder->base.crtc))
 		return;
 
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
-- 
1.8.4

Re: [PATCH 4/4] drm/i915: don't try to retrain a DP link on an inactive CRTC

[Ville Syrjälä]

On Mon, Aug 11, 2014 at 10:04:51PM +0300, Imre Deak wrote:
> Atm we may retrain the DP link even if the CRTC is inactive through
> HPD work->intel_dp_check_link_status(). This in turn can lock up the PHY
> (at least on BYT), since the DP port is disabled.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81948
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d7f5d0a..49de9be 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3551,6 +3551,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	if (WARN_ON(!intel_encoder->base.crtc))
>  		return;
>  
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;

Yeah the connectors_active check isn't enough since we don't clear that
when we disable the crtcs for system suspend. Maybe we should do that
instead?

Anyway this check seems fine to me regardless of how we deal with
connectors_active during system suspend, so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		return;
> -- 
> 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 3/4] drm/i915: make sure VDD is turned off during system suspend

[Ville Syrjälä]

On Mon, Aug 11, 2014 at 10:04:50PM +0300, Imre Deak wrote:
> Atm we may leave eDP VDD enabled during system suspend after the CRTCs
> are disabled through an HPD->DPCD read event. So disable VDD during
> suspend at a point when no HPDs can occur.

And vdd itself holds a runtime pm ref so this problem can't occur there.

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  | 15 +++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 13 +++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h |  6 ++++++
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0653761..1c7979e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -501,6 +501,19 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
>  	cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
>  }
>  
> +static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_encoder *intel_encoder;
> +
> +	drm_modeset_lock_all(dev);
> +	for_each_intel_encoder(dev, intel_encoder) {
> +		if (intel_encoder->suspend)
> +			intel_encoder->suspend(intel_encoder);
> +	}
> +	drm_modeset_unlock_all(dev);
> +}
> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -547,6 +560,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		intel_runtime_pm_disable_interrupts(dev);
>  		intel_hpd_cancel_work(dev_priv);
>  
> +		intel_suspend_encoders(dev_priv);
> +
>  		intel_suspend_gt_powersave(dev);
>  
>  		intel_modeset_suspend_hw(dev);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 34e3c47..d7f5d0a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1291,8 +1291,6 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  	if (!is_edp(intel_dp))
>  		return;
>  
> -	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
> -

Should we perhaps keep this warn here and call the _sync() version directly
from ->suspend()? I think we already do it that way when tearing down
the encoder.

>  	intel_dp->want_panel_vdd = false;
>  
>  	if (sync)
> @@ -4003,6 +4001,16 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	kfree(intel_dig_port);
>  }
>  
> +void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +
> +	if (!is_edp(intel_dp))
> +		return;

edp_panel_vdd_off() already has the same check, but if you convert it
to use the _sync() directly then the check is needed.

> +
> +	edp_panel_vdd_off(intel_dp, true);
> +}
> +
>  static void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  {
>  	intel_edp_panel_vdd_sanitize(to_intel_encoder(encoder));
> @@ -4722,6 +4730,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	intel_encoder->disable = intel_disable_dp;
>  	intel_encoder->get_hw_state = intel_dp_get_hw_state;
>  	intel_encoder->get_config = intel_dp_get_config;
> +	intel_encoder->suspend = intel_dp_encoder_suspend;
>  	if (IS_CHERRYVIEW(dev)) {
>  		intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
>  		intel_encoder->pre_enable = chv_pre_enable_dp;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1b3d1d7..cefd337 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -153,6 +153,12 @@ struct intel_encoder {
>  	 * be set correctly before calling this function. */
>  	void (*get_config)(struct intel_encoder *,
>  			   struct intel_crtc_config *pipe_config);
> +	/*
> +	 * Called during system suspend after all pending requests for the
> +	 * encoder are flushed (for example for DP AUX transactions) and
> +	 * device interrupts are disabled.
> +	 */
> +	void (*suspend)(struct intel_encoder *);
>  	int crtc_mask;
>  	enum hpd_pin hpd_pin;
>  };
> -- 
> 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 3/4] drm/i915: make sure VDD is turned off during system suspend

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 4730 bytes --]

On Tue, 2014-08-12 at 15:34 +0300, Ville Syrjälä wrote:
> On Mon, Aug 11, 2014 at 10:04:50PM +0300, Imre Deak wrote:
> > Atm we may leave eDP VDD enabled during system suspend after the CRTCs
> > are disabled through an HPD->DPCD read event. So disable VDD during
> > suspend at a point when no HPDs can occur.
> 
> And vdd itself holds a runtime pm ref so this problem can't occur there.

Yep, I'll update the commit message.

> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  | 15 +++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 13 +++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h |  6 ++++++
> >  3 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 0653761..1c7979e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -501,6 +501,19 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> >  	cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
> >  }
> >  
> > +static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_encoder *intel_encoder;
> > +
> > +	drm_modeset_lock_all(dev);
> > +	for_each_intel_encoder(dev, intel_encoder) {
> > +		if (intel_encoder->suspend)
> > +			intel_encoder->suspend(intel_encoder);
> > +	}
> > +	drm_modeset_unlock_all(dev);
> > +}
> > +
> >  static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -547,6 +560,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  		intel_runtime_pm_disable_interrupts(dev);
> >  		intel_hpd_cancel_work(dev_priv);
> >  
> > +		intel_suspend_encoders(dev_priv);
> > +
> >  		intel_suspend_gt_powersave(dev);
> >  
> >  		intel_modeset_suspend_hw(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 34e3c47..d7f5d0a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1291,8 +1291,6 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
> >  	if (!is_edp(intel_dp))
> >  		return;
> >  
> > -	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
> > -
> 
> Should we perhaps keep this warn here and call the _sync() version directly
> from ->suspend()? I think we already do it that way when tearing down
> the encoder.

Ok.

> >  	intel_dp->want_panel_vdd = false;
> >  
> >  	if (sync)
> > @@ -4003,6 +4001,16 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
> >  	kfree(intel_dig_port);
> >  }
> >  
> > +void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
> > +{
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> > +
> > +	if (!is_edp(intel_dp))
> > +		return;
> 
> edp_panel_vdd_off() already has the same check, but if you convert it
> to use the _sync() directly then the check is needed.
> 
> > +
> > +	edp_panel_vdd_off(intel_dp, true);
> > +}
> > +
> >  static void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >  {
> >  	intel_edp_panel_vdd_sanitize(to_intel_encoder(encoder));
> > @@ -4722,6 +4730,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >  	intel_encoder->disable = intel_disable_dp;
> >  	intel_encoder->get_hw_state = intel_dp_get_hw_state;
> >  	intel_encoder->get_config = intel_dp_get_config;
> > +	intel_encoder->suspend = intel_dp_encoder_suspend;
> >  	if (IS_CHERRYVIEW(dev)) {
> >  		intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
> >  		intel_encoder->pre_enable = chv_pre_enable_dp;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1b3d1d7..cefd337 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -153,6 +153,12 @@ struct intel_encoder {
> >  	 * be set correctly before calling this function. */
> >  	void (*get_config)(struct intel_encoder *,
> >  			   struct intel_crtc_config *pipe_config);
> > +	/*
> > +	 * Called during system suspend after all pending requests for the
> > +	 * encoder are flushed (for example for DP AUX transactions) and
> > +	 * device interrupts are disabled.
> > +	 */
> > +	void (*suspend)(struct intel_encoder *);
> >  	int crtc_mask;
> >  	enum hpd_pin hpd_pin;
> >  };
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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