On Tue, Feb 14, 2023 at 04:17:41PM +0200, Jani Nikula wrote:
> On Tue, 14 Feb 2023, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > 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:
>
> Seems fine to me; doesn't need to be part of this series.
Ok, can follow up with that.
Thanks for the review, patchset is pushed CCing stable on the first
patch and fixing a typo in patch 3.
>
> BR,
> Jani.
>
> >
> > 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
>
> --
> Jani Nikula, Intel Open Source Graphics Center
