| Message ID | 20190718204912.24149-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: We don't need display's suspend/resume operations when !HAS_DISPLAY | expand |
Quoting Rodrigo Vivi (2019-07-18 21:49:12) > +void intel_display_power_resume_early(struct drm_i915_private *i915) > +{ > + if (!HAS_DISPLAY(i915)) > + return; > + > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { > + gen9_sanitize_dc_state(i915); Are you sure that whatever state you are resuming from agrees with your notion of !display? The sanitize routines are supposed to be about cleaning up after third parties who don't play by the same rules. -Chris
On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote: > Quoting Rodrigo Vivi (2019-07-18 21:49:12) > > +void intel_display_power_resume_early(struct drm_i915_private *i915) > > +{ > > + if (!HAS_DISPLAY(i915)) > > + return; > > + > > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { > > + gen9_sanitize_dc_state(i915); > > Are you sure that whatever state you are resuming from agrees with your > notion of !display? The sanitize routines are supposed to be about > cleaning up after third parties who don't play by the same rules. I don't expect any function setting any kind of dc states when we don't have display. Besides the path that sets DC_STATE_EN is and neeeds to be sanitized is also covered by this patch and this shouldn't happen. Or am I missing something else? > -Chris
Quoting Rodrigo Vivi (2019-07-18 22:14:45) > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote: > > Quoting Rodrigo Vivi (2019-07-18 21:49:12) > > > +void intel_display_power_resume_early(struct drm_i915_private *i915) > > > +{ > > > + if (!HAS_DISPLAY(i915)) > > > + return; > > > + > > > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { > > > + gen9_sanitize_dc_state(i915); > > > > Are you sure that whatever state you are resuming from agrees with your > > notion of !display? The sanitize routines are supposed to be about > > cleaning up after third parties who don't play by the same rules. > > I don't expect any function setting any kind of dc states when we don't > have display. Besides the path that sets DC_STATE_EN is and neeeds to > be sanitized is also covered by this patch and this shouldn't happen. > > Or am I missing something else? It's not about us, it's about whatever else runs in between. And remember !HAS_DISPLAY() is also a user setting, not merely a reflection of probed hw. -Chris
On Thu, Jul 18, 2019 at 10:25:51PM +0100, Chris Wilson wrote: > Quoting Rodrigo Vivi (2019-07-18 22:14:45) > > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote: > > > Quoting Rodrigo Vivi (2019-07-18 21:49:12) > > > > +void intel_display_power_resume_early(struct drm_i915_private *i915) > > > > +{ > > > > + if (!HAS_DISPLAY(i915)) > > > > + return; > > > > + > > > > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { > > > > + gen9_sanitize_dc_state(i915); > > > > > > Are you sure that whatever state you are resuming from agrees with your > > > notion of !display? The sanitize routines are supposed to be about > > > cleaning up after third parties who don't play by the same rules. > > > > I don't expect any function setting any kind of dc states when we don't > > have display. Besides the path that sets DC_STATE_EN is and neeeds to > > be sanitized is also covered by this patch and this shouldn't happen. > > > > Or am I missing something else? > > It's not about us, it's about whatever else runs in between. And > remember !HAS_DISPLAY() is also a user setting, not merely a reflection > of probed hw. ouch, we need to get rid of those runtime writes to info struct :/ I wonder if it worth to add a intel_display_sanitize to be called when toggling info-num_pipes to 0 along with that DRM_ERROR... or just call it before !HAS_DISPLAY with a XXX comment... other ideas? > -Chris
On Thu, 18 Jul 2019, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Thu, Jul 18, 2019 at 10:25:51PM +0100, Chris Wilson wrote: >> Quoting Rodrigo Vivi (2019-07-18 22:14:45) >> > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote: >> > > Quoting Rodrigo Vivi (2019-07-18 21:49:12) >> > > > +void intel_display_power_resume_early(struct drm_i915_private *i915) >> > > > +{ >> > > > + if (!HAS_DISPLAY(i915)) >> > > > + return; >> > > > + >> > > > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { >> > > > + gen9_sanitize_dc_state(i915); >> > > >> > > Are you sure that whatever state you are resuming from agrees with your >> > > notion of !display? The sanitize routines are supposed to be about >> > > cleaning up after third parties who don't play by the same rules. >> > >> > I don't expect any function setting any kind of dc states when we don't >> > have display. Besides the path that sets DC_STATE_EN is and neeeds to >> > be sanitized is also covered by this patch and this shouldn't happen. >> > >> > Or am I missing something else? >> >> It's not about us, it's about whatever else runs in between. And >> remember !HAS_DISPLAY() is also a user setting, not merely a reflection >> of probed hw. > > ouch, we need to get rid of those runtime writes to info struct :/ > > I wonder if it worth to add a intel_display_sanitize to be called > when toggling info-num_pipes to 0 along with that DRM_ERROR... > > or just call it before !HAS_DISPLAY with a XXX comment... > > other ideas? Let's say we only supported user specified display disable via: # modprobe i915 # modprobe -r i915 # modprobe i915 disable_display=1 i.e. by having i915 take over and disable everything first. Would that be enough? Alternatively, could we do display disable by first probing almost everything as we would with disable_display=0, then throwing -EPROBE_DEFER and having the error handling code clean up the hardware after us. The subsequent probe retry would then proceed assuming no display hardware. Thoughts? BR, Jani.
On Tue, Aug 06, 2019 at 03:02:31PM +0300, Jani Nikula wrote: > On Thu, 18 Jul 2019, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > On Thu, Jul 18, 2019 at 10:25:51PM +0100, Chris Wilson wrote: > >> Quoting Rodrigo Vivi (2019-07-18 22:14:45) > >> > On Thu, Jul 18, 2019 at 09:58:16PM +0100, Chris Wilson wrote: > >> > > Quoting Rodrigo Vivi (2019-07-18 21:49:12) > >> > > > +void intel_display_power_resume_early(struct drm_i915_private *i915) > >> > > > +{ > >> > > > + if (!HAS_DISPLAY(i915)) > >> > > > + return; > >> > > > + > >> > > > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { > >> > > > + gen9_sanitize_dc_state(i915); > >> > > > >> > > Are you sure that whatever state you are resuming from agrees with your > >> > > notion of !display? The sanitize routines are supposed to be about > >> > > cleaning up after third parties who don't play by the same rules. > >> > > >> > I don't expect any function setting any kind of dc states when we don't > >> > have display. Besides the path that sets DC_STATE_EN is and neeeds to > >> > be sanitized is also covered by this patch and this shouldn't happen. > >> > > >> > Or am I missing something else? > >> > >> It's not about us, it's about whatever else runs in between. And > >> remember !HAS_DISPLAY() is also a user setting, not merely a reflection > >> of probed hw. > > > > ouch, we need to get rid of those runtime writes to info struct :/ > > > > I wonder if it worth to add a intel_display_sanitize to be called > > when toggling info-num_pipes to 0 along with that DRM_ERROR... > > > > or just call it before !HAS_DISPLAY with a XXX comment... > > > > other ideas? > > Let's say we only supported user specified display disable via: > > # modprobe i915 > # modprobe -r i915 > # modprobe i915 disable_display=1 > > i.e. by having i915 take over and disable everything first. Would that > be enough? > > Alternatively, could we do display disable by first probing almost > everything as we would with disable_display=0, then throwing > -EPROBE_DEFER and having the error handling code clean up the hardware > after us. The subsequent probe retry would then proceed assuming no > display hardware. > > Thoughts? It makes sense for me. Would we need to detach disable_display from num_pipes for that? Anyways, taking another quick look to the only case we modify num_pipes out of platform definition I don't see now how it would suddenly become 0: if (enabled_mask == 0 ... else info->num_pipes = hweight8(enabled_mask); so maybe just go with my previous version is safe after all?! But indeed making it more solid is a good plan. > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 93a148684c53..530b119a3a3b 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -5189,3 +5189,70 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915) } #endif + +void intel_display_power_suspend_late(struct drm_i915_private *i915) +{ + if (!HAS_DISPLAY(i915)) + return; + + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) + bxt_enable_dc9(i915); + else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) + hsw_enable_pc8(i915); +} + +void intel_display_power_resume_early(struct drm_i915_private *i915) +{ + if (!HAS_DISPLAY(i915)) + return; + + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) { + gen9_sanitize_dc_state(i915); + bxt_disable_dc9(i915); + } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) { + hsw_disable_pc8(i915); + } +} + +void intel_display_power_suspend(struct drm_i915_private *i915) +{ + if (!HAS_DISPLAY(i915)) + return; + + if (INTEL_GEN(i915) >= 11) { + icl_display_core_uninit(i915); + bxt_enable_dc9(i915); + } else if (IS_GEN9_LP(i915)) { + bxt_display_core_uninit(i915); + bxt_enable_dc9(i915); + } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) { + hsw_enable_pc8(i915); + } +} + +void intel_display_power_resume(struct drm_i915_private *i915) +{ + if (!HAS_DISPLAY(i915)) + return; + + if (INTEL_GEN(i915) >= 11) { + bxt_disable_dc9(i915); + icl_display_core_init(i915, true); + if (i915->csr.dmc_payload) { + if (i915->csr.allowed_dc_mask & + DC_STATE_EN_UPTO_DC6) + skl_enable_dc6(i915); + else if (i915->csr.allowed_dc_mask & + DC_STATE_EN_UPTO_DC5) + gen9_enable_dc5(i915); + } + } else if (IS_GEN9_LP(i915)) { + bxt_disable_dc9(i915); + bxt_display_core_init(i915, true); + if (i915->csr.dmc_payload && + (i915->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)) + gen9_enable_dc5(i915); + } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) { + hsw_disable_pc8(i915); + } +} diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h index e4d2c1ba24b0..97f2562fc5d3 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.h +++ b/drivers/gpu/drm/i915/display/intel_display_power.h @@ -232,27 +232,20 @@ struct i915_power_domains { for_each_power_well_reverse(__dev_priv, __power_well) \ for_each_if((__power_well)->desc->domains & (__domain_mask)) -void skl_enable_dc6(struct drm_i915_private *dev_priv); -void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv); -void bxt_enable_dc9(struct drm_i915_private *dev_priv); -void bxt_disable_dc9(struct drm_i915_private *dev_priv); -void gen9_enable_dc5(struct drm_i915_private *dev_priv); - int intel_power_domains_init(struct drm_i915_private *dev_priv); void intel_power_domains_cleanup(struct drm_i915_private *dev_priv); void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume); void intel_power_domains_driver_remove(struct drm_i915_private *dev_priv); -void icl_display_core_init(struct drm_i915_private *dev_priv, bool resume); -void icl_display_core_uninit(struct drm_i915_private *dev_priv); void intel_power_domains_enable(struct drm_i915_private *dev_priv); void intel_power_domains_disable(struct drm_i915_private *dev_priv); void intel_power_domains_suspend(struct drm_i915_private *dev_priv, enum i915_drm_suspend_mode); void intel_power_domains_resume(struct drm_i915_private *dev_priv); -void hsw_enable_pc8(struct drm_i915_private *dev_priv); -void hsw_disable_pc8(struct drm_i915_private *dev_priv); -void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume); -void bxt_display_core_uninit(struct drm_i915_private *dev_priv); + +void intel_display_power_suspend_late(struct drm_i915_private *i915); +void intel_display_power_resume_early(struct drm_i915_private *i915); +void intel_display_power_suspend(struct drm_i915_private *i915); +void intel_display_power_resume(struct drm_i915_private *i915); const char * intel_display_power_domain_str(struct drm_i915_private *i915, diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 7c209743e478..35b1883b55a4 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2166,7 +2166,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; struct intel_runtime_pm *rpm = &dev_priv->runtime_pm; - int ret; + int ret = 0; disable_rpm_wakeref_asserts(rpm); @@ -2177,12 +2177,9 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) intel_power_domains_suspend(dev_priv, get_suspend_mode(dev_priv, hibernation)); - 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)) - hsw_enable_pc8(dev_priv); - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) + intel_display_power_suspend_late(dev_priv); + + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) ret = vlv_suspend_complete(dev_priv); if (ret) { @@ -2372,12 +2369,7 @@ static int i915_drm_resume_early(struct drm_device *dev) intel_gt_check_and_clear_faults(&dev_priv->gt); - 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)) { - hsw_disable_pc8(dev_priv); - } + intel_display_power_resume_early(dev_priv); intel_sanitize_gt_powersave(dev_priv); @@ -2921,7 +2913,7 @@ static int intel_runtime_suspend(struct device *kdev) struct drm_device *dev = pci_get_drvdata(pdev); struct drm_i915_private *dev_priv = to_i915(dev); struct intel_runtime_pm *rpm = &dev_priv->runtime_pm; - int ret; + int ret = 0; if (WARN_ON_ONCE(!(dev_priv->gt_pm.rc6.enabled && HAS_RC6(dev_priv)))) return -ENODEV; @@ -2945,18 +2937,10 @@ static int intel_runtime_suspend(struct device *kdev) intel_uncore_suspend(&dev_priv->uncore); - 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)) { - hsw_enable_pc8(dev_priv); - } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { + intel_display_power_suspend(dev_priv); + + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) ret = vlv_suspend_complete(dev_priv); - } if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); @@ -3035,28 +3019,10 @@ 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)) { - hsw_disable_pc8(dev_priv); - } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { + intel_display_power_resume(dev_priv); + + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) ret = vlv_resume_prepare(dev_priv, true); - } intel_uncore_runtime_resume(&dev_priv->uncore);
Suspend resume is broken if we try to enable/disable dc9 on cases with disabled displays. v2: Make checkpatch happy: - braces {} are not necessary for single statement blocks v3: Also move hsw/bdw PC8 sequences since they are related to display PM anyways. (Ville) v4: Rebase after a long time, plus Move functions to the new intel_display_power so we can stop exporting platform specific functions as pointed by Jani. v5: Remove unnecessary braces. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- .../drm/i915/display/intel_display_power.c | 67 +++++++++++++++++++ .../drm/i915/display/intel_display_power.h | 17 ++--- drivers/gpu/drm/i915/i915_drv.c | 58 ++++------------ 3 files changed, 84 insertions(+), 58 deletions(-)