Re: [PATCH 4/5] drm/i915: Move rawclck, power_domain and irq un/initialization from modeset functions

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

On Fri, Mar 01, 2019 at 04:49:34PM -0800, José Roberto de Souza wrote:
> The initialization of those componentes is required by the GEM/GT not
> only display so lets move then to a more the appropriate place.
> 
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 39 ++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_display.c |  7 -----
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index cc07259ec946..2b5ce764e694 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -691,24 +691,15 @@ static int i915_modeset_load(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_vga_client;
>  
> -	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
> -	intel_update_rawclk(dev_priv);
> -
> -	intel_power_domains_init_hw(dev_priv, false);
> -
>  	intel_csr_ucode_init(dev_priv);
>  
> -	ret = intel_irq_install(dev_priv);
> -	if (ret)
> -		goto cleanup_csr;
> -
>  	intel_setup_gmbus(dev_priv);
>  
>  	/* Important: The output setup functions called by modeset_init need
>  	 * working irqs for e.g. gmbus and dp aux transfers. */
>  	ret = intel_modeset_init(dev);
>  	if (ret)
> -		goto cleanup_irq;
> +		goto cleanup_gmbus;
>  
>  	ret = i915_gem_init(dev_priv);
>  	if (ret)
> @@ -736,12 +727,9 @@ static int i915_modeset_load(struct drm_device *dev)
>  	i915_gem_fini(dev_priv);
>  cleanup_modeset:
>  	intel_modeset_cleanup(dev);
> -cleanup_irq:
> -	drm_irq_uninstall(dev);
> +cleanup_gmbus:
>  	intel_teardown_gmbus(dev_priv);
> -cleanup_csr:
>  	intel_csr_ucode_fini(dev_priv);
> -	intel_power_domains_fini_hw(dev_priv);
>  	vga_switcheroo_unregister_client(pdev);
>  cleanup_vga_client:
>  	vga_client_register(pdev, NULL, NULL, NULL);
> @@ -1765,9 +1753,18 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (ret < 0)
>  		goto out_cleanup_mmio;
>  
> +	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
> +	intel_update_rawclk(dev_priv);
> +
> +	intel_power_domains_init_hw(dev_priv, false);
> +
> +	ret = intel_irq_install(dev_priv);
> +	if (ret)
> +		goto out_cleanup_power;

What are the steps we're reordering wrt. irq enable and
why is it OK to reorder them?

> +
>  	ret = i915_modeset_load(&dev_priv->drm);
>  	if (ret < 0)
> -		goto out_cleanup_hw;
> +		goto out_cleanup_irq;
>  
>  	i915_driver_register(dev_priv);
>  
> @@ -1777,7 +1774,10 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	return 0;
>  
> -out_cleanup_hw:
> +out_cleanup_irq:
> +	drm_irq_uninstall(&dev_priv->drm);
> +out_cleanup_power:
> +	intel_power_domains_fini_hw(dev_priv);
>  	i915_driver_cleanup_hw(dev_priv);
>  out_cleanup_mmio:
>  	i915_driver_cleanup_mmio(dev_priv);
> @@ -1810,6 +1810,13 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	intel_gvt_cleanup(dev_priv);
>  
> +	/*
> +	 * Interrupts and polling as the first thing to avoid creating havoc.
> +	 * Too much stuff here (turning of connectors, ...) would
> +	 * experience fancy races otherwise.
> +	 */
> +	intel_irq_uninstall(dev_priv);

This too seems slightly questionable considering the flush_workqueue()
etc. in intel_modeset_cleanup(). Can we be sure all modeset activity has
in fact finished sufficiently to no require interrupts?

> +
>  	i915_modeset_unload(dev);
>  
>  	/* Free error state after interrupts are fully disabled. */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7963348f1c64..5158e8ecb9ed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16364,13 +16364,6 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	flush_work(&dev_priv->atomic_helper.free_work);
>  	WARN_ON(!llist_empty(&dev_priv->atomic_helper.free_list));
>  
> -	/*
> -	 * Interrupts and polling as the first thing to avoid creating havoc.
> -	 * Too much stuff here (turning of connectors, ...) would
> -	 * experience fancy races otherwise.
> -	 */
> -	intel_irq_uninstall(dev_priv);
> -
>  	/*
>  	 * Due to the hpd irq storm handling the hotplug work can re-arm the
>  	 * poll handlers. Hence disable polling after hpd handling is shut down.
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx