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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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