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

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

On Mon, 04 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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?

I keep thinking we should have preconditions within the functions to
ensure we don't inadvertently break anything. We have some comments,
like above, but nowhere near enough, and they don't jump out at you if
something goes wrong.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx