On Thu, May 23, 2019 at 10:26:37PM +0300, Ville Syrjälä wrote:
> On Thu, May 23, 2019 at 12:19:35PM -0700, Rodrigo Vivi wrote:
> > Suspend resume is broken if we try to enable/disable dc9 on
> > cases with disabled displays.
> >
> > v2: Make checkpatch happy:
> > -:78: WARNING:BRACES: braces {} are not necessary for single statement blocks
> >
> > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 104 ++++++++++++++++++++++----------
> > 1 file changed, 71 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 83d2eb9e74cb..0100c7e940c6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -2118,6 +2118,15 @@ get_suspend_mode(struct drm_i915_private *dev_priv, bool hibernate)
> > return I915_DRM_SUSPEND_MEM;
> > }
> >
> > +static void intel_display_suspend_late(struct drm_i915_private *dev_priv)
> > +{
> > + if (!HAS_DISPLAY(dev_priv))
> > + return;
> > +
> > + if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> > + bxt_enable_dc9(dev_priv);
> > +}
> > +
> > static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -2133,10 +2142,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > intel_power_domains_suspend(dev_priv,
> > get_suspend_mode(dev_priv, hibernation));
> >
> > + intel_display_suspend_late(dev_priv);
> > +
> > ret = 0;
> > - if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv))
> > - bxt_enable_dc9(dev_priv);
> > - else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > hsw_enable_pc8(dev_priv);
>
> The pc8 stuff is also about display.
Well, I was considering to move that along, but Package-C state
term itself goes beyond display so I decided to leave it here.
However in the end this "Package-C" sequence just moved to DMC right?!
Called after we get to certain Display C state...
so yeap I agree it makes more sense to move everything...
regardless the terms used...
>
> > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > ret = vlv_suspend_complete(dev_priv);
> > @@ -2266,6 +2275,17 @@ static int i915_drm_resume(struct drm_device *dev)
> > return 0;
> > }
> >
> > +static void intel_display_resume_early(struct drm_i915_private *dev_priv)
> > +{
> > + if (!HAS_DISPLAY(dev_priv))
> > + return;
> > +
> > + if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> > + gen9_sanitize_dc_state(dev_priv);
> > + bxt_disable_dc9(dev_priv);
> > + }
> > +}
> > +
> > static int i915_drm_resume_early(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -2328,12 +2348,10 @@ static int i915_drm_resume_early(struct drm_device *dev)
> >
> > i915_check_and_clear_faults(dev_priv);
> >
> > - if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> > - gen9_sanitize_dc_state(dev_priv);
> > - bxt_disable_dc9(dev_priv);
> > - } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > + intel_display_resume_early(dev_priv);
> > +
> > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > hsw_disable_pc8(dev_priv);
> > - }
> >
> > intel_uncore_sanitize(dev_priv);
> >
> > @@ -2869,6 +2887,20 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
> > return ret;
> > }
> >
> > +static void intel_runtime_display_suspend(struct drm_i915_private *dev_priv)
> > +{
> > + if (!HAS_DISPLAY(dev_priv))
> > + return;
> > +
> > + if (INTEL_GEN(dev_priv) >= 11) {
> > + icl_display_core_uninit(dev_priv);
> > + bxt_enable_dc9(dev_priv);
> > + } else if (IS_GEN9_LP(dev_priv)) {
> > + bxt_display_core_uninit(dev_priv);
> > + bxt_enable_dc9(dev_priv);
> > + }
> > +}
> > +
> > static int intel_runtime_suspend(struct device *kdev)
> > {
> > struct pci_dev *pdev = to_pci_dev(kdev);
> > @@ -2898,14 +2930,10 @@ static int intel_runtime_suspend(struct device *kdev)
> >
> > intel_uncore_suspend(&dev_priv->uncore);
> >
> > + intel_runtime_display_suspend(dev_priv);
> > +
> > ret = 0;
> > - if (INTEL_GEN(dev_priv) >= 11) {
> > - icl_display_core_uninit(dev_priv);
> > - bxt_enable_dc9(dev_priv);
> > - } else if (IS_GEN9_LP(dev_priv)) {
> > - bxt_display_core_uninit(dev_priv);
> > - bxt_enable_dc9(dev_priv);
> > - } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > hsw_enable_pc8(dev_priv);
> > } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > ret = vlv_suspend_complete(dev_priv);
> > @@ -2967,6 +2995,31 @@ static int intel_runtime_suspend(struct device *kdev)
> > return 0;
> > }
> >
> > +static void intel_runtime_display_resume(struct drm_i915_private *dev_priv)
> > +{
> > + if (!HAS_DISPLAY(dev_priv))
> > + return;
> > +
> > + if (INTEL_GEN(dev_priv) >= 11) {
> > + bxt_disable_dc9(dev_priv);
> > + icl_display_core_init(dev_priv, true);
> > + if (dev_priv->csr.dmc_payload) {
> > + if (dev_priv->csr.allowed_dc_mask &
> > + DC_STATE_EN_UPTO_DC6)
> > + skl_enable_dc6(dev_priv);
> > + else if (dev_priv->csr.allowed_dc_mask &
> > + DC_STATE_EN_UPTO_DC5)
> > + gen9_enable_dc5(dev_priv);
> > + }
> > + } else if (IS_GEN9_LP(dev_priv)) {
> > + bxt_disable_dc9(dev_priv);
> > + bxt_display_core_init(dev_priv, true);
> > + if (dev_priv->csr.dmc_payload &&
> > + (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
> > + gen9_enable_dc5(dev_priv);
> > + }
> > +}
> > +
> > static int intel_runtime_resume(struct device *kdev)
> > {
> > struct pci_dev *pdev = to_pci_dev(kdev);
> > @@ -2987,24 +3040,9 @@ static int intel_runtime_resume(struct device *kdev)
> > if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> > DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
> >
> > - if (INTEL_GEN(dev_priv) >= 11) {
> > - bxt_disable_dc9(dev_priv);
> > - icl_display_core_init(dev_priv, true);
> > - if (dev_priv->csr.dmc_payload) {
> > - if (dev_priv->csr.allowed_dc_mask &
> > - DC_STATE_EN_UPTO_DC6)
> > - skl_enable_dc6(dev_priv);
> > - else if (dev_priv->csr.allowed_dc_mask &
> > - DC_STATE_EN_UPTO_DC5)
> > - gen9_enable_dc5(dev_priv);
> > - }
> > - } else if (IS_GEN9_LP(dev_priv)) {
> > - bxt_disable_dc9(dev_priv);
> > - bxt_display_core_init(dev_priv, true);
> > - if (dev_priv->csr.dmc_payload &&
> > - (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
> > - gen9_enable_dc5(dev_priv);
> > - } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > + intel_runtime_display_resume(dev_priv);
> > +
> > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > hsw_disable_pc8(dev_priv);
> > } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > ret = vlv_resume_prepare(dev_priv, true);
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
