On Wed, 02 Sep 2020, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
