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

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

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>

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

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