From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/4] drm/i915: make sure VDD is turned off during system suspend Date: Tue, 12 Aug 2014 15:34:47 +0300 Message-ID: <20140812123447.GY4193@intel.com> References: <1407783891-9406-1-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id DDD196E05D for ; Tue, 12 Aug 2014 05:34:53 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1407783891-9406-1-git-send-email-imre.deak@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Imre Deak Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > --- > 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 =3D 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 =3D 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 *inte= l_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 =3D 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 =3D 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_re= g, enum port port) > intel_encoder->disable =3D intel_disable_dp; > intel_encoder->get_hw_state =3D intel_dp_get_hw_state; > intel_encoder->get_config =3D intel_dp_get_config; > + intel_encoder->suspend =3D intel_dp_encoder_suspend; > if (IS_CHERRYVIEW(dev)) { > intel_encoder->pre_pll_enable =3D chv_dp_pre_pll_enable; > intel_encoder->pre_enable =3D chv_pre_enable_dp; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_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=E4l=E4 Intel OTC