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.
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
