On Tue, Feb 14, 2023 at 11:25:04AM +0200, Jani Nikula wrote:
> On Wed, 08 Feb 2023, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > Determining whether the display engine is present on a platform happens
> > only in intel_device_info_runtime_init(). Initializing the display power
> > functionality depends on this condition, so move
> > intel_power_domains_init() later after the runtime init function has
> > been called.
> >
> > The next patch fixing platforms without display, depends on this patch.
> >
>
> It's pretty hard to review we aren't using any of the power domain stuff
> before the intel_power_domains_init() call. What happens if we do?
That shouldn't happen before the HW state is read out in
intel_power_domains_init_hw(). I can't see anything calling the display power
get/put/is_enabled functions before that, but it would be good to ensure this.
So how about also adding:
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 7222502a760cc..7014e1770f57a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -213,6 +213,9 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
struct i915_power_well *power_well;
bool is_enabled;
+ drm_WARN_ON(&dev_priv->drm,
+ dev_priv->display.power.domains.init_state < I915_POWER_DOMAINS_INITIALIZING);
+
if (dev_priv->runtime_pm.suspended)
return false;
@@ -488,6 +491,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
struct i915_power_well *power_well;
+ drm_WARN_ON(&dev_priv->drm,
+ power_domains->init_state < I915_POWER_DOMAINS_INITIALIZING);
+
if (intel_display_power_grab_async_put_ref(dev_priv, domain))
return;
@@ -1880,7 +1886,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *i915, bool resume)
{
struct i915_power_domains *power_domains = &i915->display.power.domains;
- power_domains->initializing = true;
+ power_domains->init_state = I915_POWER_DOMAINS_INITIALIZING;
if (DISPLAY_VER(i915) >= 11) {
icl_display_core_init(i915, resume);
@@ -1924,7 +1930,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *i915, bool resume)
}
intel_power_domains_sync_hw(i915);
- power_domains->initializing = false;
+ power_domains->init_state = I915_POWER_DOMAINS_INITIALIZED;
}
/**
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index 2154d900b1aad..0d9aba94bae01 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -133,7 +133,11 @@ struct i915_power_domains {
* Power wells needed for initialization at driver init and suspend
* time are on. They are kept on until after the first modeset.
*/
- bool initializing;
+ enum {
+ I915_POWER_DOMAINS_UNINITIALIZED,
+ I915_POWER_DOMAINS_INITIALIZING,
+ I915_POWER_DOMAINS_INITIALIZED,
+ } init_state;
bool display_core_suspended;
int power_well_count;
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index 8710dd41ffd4c..66df7a733afb9 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -1208,7 +1208,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
* During driver initialization/resume we can avoid restoring the
* part of the HW/SW state that will be inited anyway explicitly.
*/
- if (dev_priv->display.power.domains.initializing)
+ if (dev_priv->display.power.domains.init_state < I915_POWER_DOMAINS_INITIALIZED)
return;
intel_hpd_init(dev_priv);
> I approve of the change, but I can't in good faith claim I checked this.
>
> Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 5 +++++
> > drivers/gpu/drm/i915/i915_driver.c | 7 -------
> > 2 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 166662ade593c..b3e7ed3866cde 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -8634,6 +8634,10 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
> > goto cleanup_bios;
> >
> > /* FIXME: completely on the wrong abstraction layer */
> > + ret = intel_power_domains_init(i915);
> > + if (ret < 0)
> > + goto cleanup_vga;
> > +
> > intel_power_domains_init_hw(i915, false);
> >
> > if (!HAS_DISPLAY(i915))
> > @@ -8676,6 +8680,7 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
> > cleanup_vga_client_pw_domain_dmc:
> > intel_dmc_ucode_fini(i915);
> > intel_power_domains_driver_remove(i915);
> > +cleanup_vga:
> > intel_vga_unregister(i915);
> > cleanup_bios:
> > intel_bios_driver_remove(i915);
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 13bf4fe52f9fe..fe2870a6ae631 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -251,9 +251,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
> > intel_detect_pch(dev_priv);
> >
> > intel_pm_setup(dev_priv);
> > - ret = intel_power_domains_init(dev_priv);
> > - if (ret < 0)
> > - goto err_gem;
> > intel_irq_init(dev_priv);
> > intel_init_display_hooks(dev_priv);
> > intel_init_clock_gating_hooks(dev_priv);
> > @@ -262,10 +259,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
> >
> > return 0;
> >
> > -err_gem:
> > - i915_gem_cleanup_early(dev_priv);
> > - intel_gt_driver_late_release_all(dev_priv);
> > - i915_drm_clients_fini(&dev_priv->clients);
> > err_rootgt:
> > intel_region_ttm_device_fini(dev_priv);
> > err_ttm:
>
> --
> Jani Nikula, Intel Open Source Graphics Center
