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.d...@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
>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
