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

[Imre Deak <imre.deak@intel.com>]

[-- 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