Re: [PATCH 13/24] drm/{i915,xe}: move more calls inside intel_display_driver_{register,unregister}()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 29, 2026 at 02:03:57PM +0300, Jani Nikula wrote:
> The intel_display_driver_register() and
> intel_display_driver_unregister() calls are followed and preceded by
> intel_display_power_enable() and intel_display_power_disable() calls,
> respectively. Move them inside the register/unregister calls.
> 
> Semantically, this is a weird location, as there's nothing really
> "register" or "unregister" about them, but they retain the existing
> sequence. Add comments to note that.
> 
> There's a slight functional change for !HAS_DISPLAY() in that
> register/unregister return early. Assume this is what we want, and there
> are no cases where display engine is present but all pipes have been
> fused off.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_driver.c | 6 ++++++
>  drivers/gpu/drm/i915/i915_driver.c                  | 2 --
>  drivers/gpu/drm/xe/display/xe_display.c             | 2 --
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 77fa4497b442..7fee9ef88224 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -578,6 +578,9 @@ void intel_display_driver_register(struct intel_display *display)
>  					DISPLAY_RUNTIME_INFO(display), &p);
>  
>  	intel_register_dsm_handler();
> +
> +	/* Semantically out of place, just for the sequence */
> +	intel_display_power_enable(display);
>  }
>  
>  /* part #1: call before irq uninstall */
> @@ -652,6 +655,9 @@ void intel_display_driver_unregister(struct intel_display *display)
>  	if (!HAS_DISPLAY(display))
>  		return;
>  
> +	/* Semantically out of place, just for the sequence */
> +	intel_display_power_disable(display);
> +
>  	intel_unregister_dsm_handler();
>  
>  	drm_client_dev_unregister(display->drm);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 6fd3e8b155b1..b637c4dedf1b 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -661,7 +661,6 @@ static int i915_driver_register(struct drm_i915_private *dev_priv)
>  
>  	intel_display_driver_register(display);
>  
> -	intel_display_power_enable(display);

Maybe these deserve their own display_driver_runtime_pm_{en,dis}able()
things.

Although I'm not sure why we even do this so late. We could probably
do this as soon as the readout is done. And I think ideally we wouldn't
even use the INIT domain for this and would just do something to keep
the already enabled power wells enabled until the readout is done.
But that's a whole different can of worms.

>  	intel_runtime_pm_enable(&dev_priv->runtime_pm);
>  
>  	if (i915_switcheroo_register(dev_priv))
> @@ -683,7 +682,6 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  	i915_switcheroo_unregister(dev_priv);
>  
>  	intel_runtime_pm_disable(&dev_priv->runtime_pm);
> -	intel_display_power_disable(display);
>  
>  	intel_display_driver_unregister(display);
>  
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index bbd4f527d5e3..e17e05a8854c 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -177,7 +177,6 @@ void xe_display_register(struct xe_device *xe)
>  		return;
>  
>  	intel_display_driver_register(display);
> -	intel_display_power_enable(display);
>  }
>  
>  void xe_display_unregister(struct xe_device *xe)
> @@ -187,7 +186,6 @@ void xe_display_unregister(struct xe_device *xe)
>  	if (!xe->info.probe_display)
>  		return;
>  
> -	intel_display_power_disable(display);
>  	intel_display_driver_unregister(display);
>  }
>  
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel