Re: [Intel-gfx] [PATCH 4/4] drm/i915: remove the extra modeset init layer

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

On Wed, 02 Sep 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Sep 02, 2020 at 05:30:23PM +0300, Jani Nikula wrote:
>> Streamline the modeset init by removing the extra init layer.
>> 
>> No functional changes, which means the cleanup path looks hideous.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c | 63 +++++++++++----------------------
>>  1 file changed, 20 insertions(+), 43 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index e332b6fd701d..4d9b61b1a115 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -215,46 +215,6 @@ intel_teardown_mchbar(struct drm_i915_private *dev_priv)
>>  		release_resource(&dev_priv->mch_res);
>>  }
>>  
>> -/* part #1: call before irq install */
>> -static int i915_driver_modeset_probe_noirq(struct drm_i915_private *i915)
>> -{
>> -	return intel_modeset_init_noirq(i915);
>> -}
>> -
>> -/* part #2: call after irq install */
>> -static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>> -{
>> -	int ret;
>> -
>> -	/* Important: The output setup functions called by modeset_init need
>> -	 * working irqs for e.g. gmbus and dp aux transfers. */
>> -	ret = intel_modeset_init_nogem(i915);
>> -	if (ret)
>> -		goto out;
>> -
>> -	ret = i915_gem_init(i915);
>> -	if (ret)
>> -		goto cleanup_modeset;
>> -
>> -	ret = intel_modeset_init(i915);
>> -	if (ret)
>> -		goto cleanup_gem;
>> -
>> -	return 0;
>> -
>> -cleanup_gem:
>> -	i915_gem_suspend(i915);
>> -	i915_gem_driver_remove(i915);
>> -	i915_gem_driver_release(i915);
>> -cleanup_modeset:
>> -	/* FIXME */
>> -	intel_modeset_driver_remove(i915);
>> -	intel_irq_uninstall(i915);
>> -	intel_modeset_driver_remove_noirq(i915);
>> -out:
>> -	return ret;
>> -}
>> -
>>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>>  {
>>  	/*
>> @@ -923,7 +883,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  	if (ret < 0)
>>  		goto out_cleanup_mmio;
>>  
>> -	ret = i915_driver_modeset_probe_noirq(i915);
>> +	ret = intel_modeset_init_noirq(i915);
>>  	if (ret < 0)
>>  		goto out_cleanup_hw;
>>  
>> @@ -931,10 +891,18 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  	if (ret)
>>  		goto out_cleanup_modeset;
>>  
>> -	ret = i915_driver_modeset_probe(i915);
>> -	if (ret < 0)
>> +	ret = intel_modeset_init_nogem(i915);
>> +	if (ret)
>>  		goto out_cleanup_irq;
>>  
>> +	ret = i915_gem_init(i915);
>> +	if (ret)
>> +		goto out_cleanup_modeset2;
>> +
>> +	ret = intel_modeset_init(i915);
>> +	if (ret)
>> +		goto out_cleanup_gem;
>> +
>>  	i915_driver_register(i915);
>>  
>>  	enable_rpm_wakeref_asserts(&i915->runtime_pm);
>> @@ -945,6 +913,15 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  
>>  	return 0;
>>  
>> +out_cleanup_gem:
>> +	i915_gem_suspend(i915);
>> +	i915_gem_driver_remove(i915);
>> +	i915_gem_driver_release(i915);
>> +out_cleanup_modeset2:
>> +	intel_modeset_driver_remove(i915);
>> +	intel_irq_uninstall(i915);
>> +	intel_modeset_driver_remove_noirq(i915);
>> +	goto out_cleanup_modeset;
>
> Looks like we used to do the intel_irq_uninstall() twice? We even
> have a FIXME in there stating as much. With this goto we only do
> it the once I guess. So seems like a slight change in behaviour.
> Though the comment says it gets called twice during driver remove
> as well, which does not seem to be true (at least anymore).
>
> Anyways, fixing that properly likely requires some axctual thought
> wrt. hpd vs. irq vs. other stuff.
>
> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks. Pushed the lot, with a simple FIXME comment added about cleaning
up the error path.

BR,
Jani.


>
>>  out_cleanup_irq:
>>  	intel_irq_uninstall(i915);
>>  out_cleanup_modeset:
>> -- 
>> 2.20.1

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