Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: Move display power initialization during driver probing later

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 14 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
> On Tue, Feb 14, 2023 at 11:25:04AM +0200, Jani Nikula wrote:
>> On Wed, 08 Feb 2023, Imre Deak <imre.deak@intel.com> 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@intel.com>
>> 
>> 
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  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