[PATCH] drm/i915: Add some missing steps to i915_driver_load error path

[PATCH] drm/i915: Add some missing steps to i915_driver_load error path

[Chris Wilson]

We missed adding a few cleanup steps for recent additions.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a7a0eb86..2924337 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1527,7 +1527,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ret = i915_gem_gtt_init(dev);
 	if (ret)
-		goto put_bridge;
+		goto out_regs;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_kick_out_firmware_fb(dev_priv);
@@ -1556,7 +1556,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 				     aperture_size);
 	if (dev_priv->gtt.mappable == NULL) {
 		ret = -EIO;
-		goto out_rmmap;
+		goto out_gtt;
 	}
 
 	dev_priv->gtt.mtrr = arch_phys_wc_add(dev_priv->gtt.mappable_base,
@@ -1630,7 +1630,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		ret = i915_load_modeset_init(dev);
 		if (ret < 0) {
 			DRM_ERROR("failed to init modeset\n");
-			goto out_gem_unload;
+			goto out_power_well;
 		}
 	} else {
 		/* Start out suspended in ums mode. */
@@ -1650,6 +1650,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	return 0;
 
+out_power_well:
+	if (HAS_POWER_WELL(dev))
+		i915_remove_power_well(dev);
+	drm_vblank_cleanup(dev);
 out_gem_unload:
 	if (dev_priv->mm.inactive_shrinker.shrink)
 		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
@@ -1663,12 +1667,17 @@ out_gem_unload:
 out_mtrrfree:
 	arch_phys_wc_del(dev_priv->gtt.mtrr);
 	io_mapping_free(dev_priv->gtt.mappable);
+out_gtt:
+	list_del(&dev_priv->gtt.base.global_link);
+	drm_mm_takedown(&dev_priv->gtt.base.mm);
 	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
-out_rmmap:
+out_regs:
 	pci_iounmap(dev->pdev, dev_priv->regs);
 put_bridge:
 	pci_dev_put(dev_priv->bridge_dev);
 free_priv:
+	if (dev_priv->slab)
+		kmem_cache_destroy(dev_priv->slab);
 	kfree(dev_priv);
 	return ret;
 }
@@ -1754,6 +1763,8 @@ int i915_driver_unload(struct drm_device *dev)
 	if (dev_priv->regs != NULL)
 		pci_iounmap(dev->pdev, dev_priv->regs);
 
+	drm_vblank_cleanup(dev);
+
 	intel_teardown_gmbus(dev);
 	intel_teardown_mchbar(dev);
 
-- 
1.8.4.rc2

Re: [PATCH] drm/i915: Add some missing steps to i915_driver_load error path

[Ben Widawsky]

On Sun, Aug 11, 2013 at 12:46:45PM +0100, Chris Wilson wrote:
> We missed adding a few cleanup steps for recent additions.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a7a0eb86..2924337 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1527,7 +1527,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	ret = i915_gem_gtt_init(dev);
>  	if (ret)
> -		goto put_bridge;
> +		goto out_regs;
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		i915_kick_out_firmware_fb(dev_priv);
> @@ -1556,7 +1556,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  				     aperture_size);
>  	if (dev_priv->gtt.mappable == NULL) {
>  		ret = -EIO;
> -		goto out_rmmap;
> +		goto out_gtt;
>  	}
>  
>  	dev_priv->gtt.mtrr = arch_phys_wc_add(dev_priv->gtt.mappable_base,
> @@ -1630,7 +1630,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		ret = i915_load_modeset_init(dev);
>  		if (ret < 0) {
>  			DRM_ERROR("failed to init modeset\n");
> -			goto out_gem_unload;
> +			goto out_power_well;
>  		}
>  	} else {
>  		/* Start out suspended in ums mode. */
> @@ -1650,6 +1650,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	return 0;
>  
> +out_power_well:
> +	if (HAS_POWER_WELL(dev))
> +		i915_remove_power_well(dev);
> +	drm_vblank_cleanup(dev);
>  out_gem_unload:
>  	if (dev_priv->mm.inactive_shrinker.shrink)
>  		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
> @@ -1663,12 +1667,17 @@ out_gem_unload:
>  out_mtrrfree:
>  	arch_phys_wc_del(dev_priv->gtt.mtrr);
>  	io_mapping_free(dev_priv->gtt.mappable);
> +out_gtt:
> +	list_del(&dev_priv->gtt.base.global_link);
> +	drm_mm_takedown(&dev_priv->gtt.base.mm);
>  	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
> -out_rmmap:
> +out_regs:
>  	pci_iounmap(dev->pdev, dev_priv->regs);
>  put_bridge:
>  	pci_dev_put(dev_priv->bridge_dev);
>  free_priv:
> +	if (dev_priv->slab)
> +		kmem_cache_destroy(dev_priv->slab);
>  	kfree(dev_priv);
>  	return ret;
>  }
> @@ -1754,6 +1763,8 @@ int i915_driver_unload(struct drm_device *dev)
>  	if (dev_priv->regs != NULL)
>  		pci_iounmap(dev->pdev, dev_priv->regs);
>  
> +	drm_vblank_cleanup(dev);
> +
>  	intel_teardown_gmbus(dev);
>  	intel_teardown_mchbar(dev);
>  

It looks like we still have no way to restore the firmware fb we've
kicked out? Also, if you wanted to rename put_bridge, and free_priv to
"out_" I wouldn't mind the unification.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center