diff mbox

drm/i915: Hide GEM shutdown in i915_gem_fini

Message ID 1455205165-19264-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Feb. 11, 2016, 3:39 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

As we have i915_gem_init, do the reverse in new i915_gem_fini so
the fragile order of doing things is hidden from the outside and
only at one place.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Nick Hoath <nicholas.hoath@intel.com>
Cc: David Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |  6 ++----
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++----------------
 3 files changed, 27 insertions(+), 21 deletions(-)

Comments

Chris Wilson Feb. 11, 2016, 3:57 p.m. UTC | #1 
On Thu, Feb 11, 2016 at 03:39:25PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> As we have i915_gem_init, do the reverse in new i915_gem_fini so
> the fragile order of doing things is hidden from the outside and
> only at one place.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Nick Hoath <nicholas.hoath@intel.com>
> Cc: David Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  6 ++----
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++----------------
>  3 files changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2df2fac04708..beda7dea6814 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -444,8 +444,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  cleanup_gem:
>  	mutex_lock(&dev->struct_mutex);
> -	i915_gem_context_fini(dev);
> -	i915_gem_cleanup_engines(dev);
> +	i915_gem_fini(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  cleanup_irq:
>  	intel_guc_ucode_fini(dev);
> @@ -1256,8 +1255,7 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	intel_guc_ucode_fini(dev);
>  	mutex_lock(&dev->struct_mutex);
> -	i915_gem_context_fini(dev);
> -	i915_gem_cleanup_engines(dev);
> +	i915_gem_fini(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  	intel_fbc_cleanup_cfb(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 090172de81a0..d004dc806670 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3060,11 +3060,11 @@ static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
>  void i915_gem_reset(struct drm_device *dev);
>  bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
>  int __must_check i915_gem_init(struct drm_device *dev);
> +void __must_check i915_gem_fini(struct drm_device *dev);
>  int i915_gem_init_rings(struct drm_device *dev);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
>  int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
>  void i915_gem_init_swizzling(struct drm_device *dev);
> -void i915_gem_cleanup_engines(struct drm_device *dev);
>  int __must_check i915_gpu_idle(struct drm_device *dev);
>  int __must_check i915_gem_suspend(struct drm_device *dev);
>  void __i915_add_request(struct drm_i915_gem_request *req,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c63c3072a8a9..f7e7ab432341 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4834,6 +4834,26 @@ cleanup_render_ring:
>  	return ret;
>  }
>  
> +static void
> +i915_gem_cleanup_engines(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_engine_cs *ring;
> +	int i;
> +
> +	for_each_ring(ring, dev_priv, i)
> +		dev_priv->gt.cleanup_ring(ring);
> +
> +	if (i915.enable_execlists) {
> +		/*
> +		 * Neither the BIOS, ourselves or any other kernel
> +		 * expects the system to be in execlists mode on startup,
> +		 * so we need to reset the GPU back to legacy mode.
> +		 */
> +		intel_gpu_reset(dev);
> +	}

This needs to be before contex-fini. If you move this outside of the
mutex, we can then use i915_gpu_reset() and do the full state cleanup.
Similarly that also moves the mutex handling inside i915_gem_fini as
well.
-Chris
kernel test robot Feb. 11, 2016, 4:57 p.m. UTC | #2 
Hi Tvrtko,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160211]
[cannot apply to v4.5-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Tvrtko-Ursulin/drm-i915-Hide-GEM-shutdown-in-i915_gem_fini/20160211-234159
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-i0-201606 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/i915_drv.c:34:0:
>> drivers/gpu/drm/i915/i915_drv.h:3057:40: warning: 'warn_unused_result' attribute ignored [-Wattributes]
    void __must_check i915_gem_fini(struct drm_device *dev);
                                           ^

vim +/warn_unused_result +3057 drivers/gpu/drm/i915/i915_drv.h

  3041	
  3042	static inline bool i915_stop_ring_allow_ban(struct drm_i915_private *dev_priv)
  3043	{
  3044		return dev_priv->gpu_error.stop_rings == 0 ||
  3045			dev_priv->gpu_error.stop_rings & I915_STOP_RING_ALLOW_BAN;
  3046	}
  3047	
  3048	static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
  3049	{
  3050		return dev_priv->gpu_error.stop_rings == 0 ||
  3051			dev_priv->gpu_error.stop_rings & I915_STOP_RING_ALLOW_WARN;
  3052	}
  3053	
  3054	void i915_gem_reset(struct drm_device *dev);
  3055	bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
  3056	int __must_check i915_gem_init(struct drm_device *dev);
> 3057	void __must_check i915_gem_fini(struct drm_device *dev);
  3058	int i915_gem_init_rings(struct drm_device *dev);
  3059	int __must_check i915_gem_init_hw(struct drm_device *dev);
  3060	int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
  3061	void i915_gem_init_swizzling(struct drm_device *dev);
  3062	int __must_check i915_gpu_idle(struct drm_device *dev);
  3063	int __must_check i915_gem_suspend(struct drm_device *dev);
  3064	void __i915_add_request(struct drm_i915_gem_request *req,
  3065				struct drm_i915_gem_object *batch_obj,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2df2fac04708..beda7dea6814 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -444,8 +444,7 @@  static int i915_load_modeset_init(struct drm_device *dev)
 
 cleanup_gem:
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_context_fini(dev);
-	i915_gem_cleanup_engines(dev);
+	i915_gem_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
 cleanup_irq:
 	intel_guc_ucode_fini(dev);
@@ -1256,8 +1255,7 @@  int i915_driver_unload(struct drm_device *dev)
 
 	intel_guc_ucode_fini(dev);
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_context_fini(dev);
-	i915_gem_cleanup_engines(dev);
+	i915_gem_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
 	intel_fbc_cleanup_cfb(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 090172de81a0..d004dc806670 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3060,11 +3060,11 @@  static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
 void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_init(struct drm_device *dev);
+void __must_check i915_gem_fini(struct drm_device *dev);
 int i915_gem_init_rings(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
-void i915_gem_cleanup_engines(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c63c3072a8a9..f7e7ab432341 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4834,6 +4834,26 @@  cleanup_render_ring:
 	return ret;
 }
 
+static void
+i915_gem_cleanup_engines(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_engine_cs *ring;
+	int i;
+
+	for_each_ring(ring, dev_priv, i)
+		dev_priv->gt.cleanup_ring(ring);
+
+	if (i915.enable_execlists) {
+		/*
+		 * Neither the BIOS, ourselves or any other kernel
+		 * expects the system to be in execlists mode on startup,
+		 * so we need to reset the GPU back to legacy mode.
+		 */
+		intel_gpu_reset(dev);
+	}
+}
+
 int
 i915_gem_init_hw(struct drm_device *dev)
 {
@@ -5011,24 +5031,12 @@  out_unlock:
 	return ret;
 }
 
-void
-i915_gem_cleanup_engines(struct drm_device *dev)
+void i915_gem_fini(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *ring;
-	int i;
-
-	for_each_ring(ring, dev_priv, i)
-		dev_priv->gt.cleanup_ring(ring);
+	lockdep_assert_held(&dev->struct_mutex);
 
-	if (i915.enable_execlists) {
-		/*
-		 * Neither the BIOS, ourselves or any other kernel
-		 * expects the system to be in execlists mode on startup,
-		 * so we need to reset the GPU back to legacy mode.
-		 */
-		intel_gpu_reset(dev);
-	}
+	i915_gem_context_fini(dev);
+	i915_gem_cleanup_engines(dev);
 }
 
 static void