Re: [Intel-gfx] [PATCH v2] drm/i915: remove user GTT mappings early during runtime suspend

2014-05-22 Thread Robert Beckett 

On 07/05/2014 17:57, Imre Deak wrote:

Currently user space can access GEM buffers mapped to GTT through
existing mappings concurrently while the platform specific suspend
handlers are running. Since these handlers may change the HW state in a
way that would break such accesses, remove the mappings before calling
the handlers. Spotted by Ville.

Also Chris pointed out that the lists that i915_gem_release_all_mmaps()
walks through need dev-struct_mutex, so take this lock. There is a
potential deadlock against a concurrent RPM resume, resolve this by
aborting and rescheduling the suspend (Daniel).

v2:
- take struct_mutex around i915_gem_release_all_mmaps() (Chris, Daniel)

Signed-off-by: Imre Deak imre.d...@intel.com
---
  drivers/gpu/drm/i915/i915_drv.c | 27 +--
  1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4024e16..0c9858c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -36,6 +36,7 @@

  #include linux/console.h
  #include linux/module.h
+#include linux/pm_runtime.h
  #include drm/drm_crtc_helper.h

  static struct drm_driver driver;
@@ -1315,6 +1316,30 @@ static int intel_runtime_suspend(struct device *device)
DRM_DEBUG_KMS(Suspending device\n);

/*
+* We could deadlock here in case another thread holding struct_mutex
+* calls RPM suspend concurrently, since the RPM suspend will wait
+* first for this RPM suspend to finish. In this case the concurrent
+* RPM resume will be followed by its RPM suspend counterpart. Still
+* for consistency return -EAGAIN, which will reschedule this suspend.
+*/
+   if (!mutex_trylock(dev-struct_mutex)) {
+   DRM_DEBUG_KMS(device lock contention, deffering suspend\n);
+   /*
+* Bump the expiration timestamp, otherwise the suspend won't
+* be rescheduled.
+*/
+   pm_runtime_mark_last_busy(device);
+
+   return -EAGAIN;
+   }
+   /*
+* We are safe here against re-faults, since the fault handler takes
+* an RPM reference.
+*/
+   i915_gem_release_all_mmaps(dev_priv);
+   mutex_unlock(dev-struct_mutex);
+
+   /*
 * rps.work can't be rearmed here, since we get here only after making
 * sure the GPU is idle and the RPS freq is set to the minimum. See
 * intel_mark_idle().
@@ -1340,8 +1365,6 @@ static int intel_runtime_suspend(struct device *device)
return ret;
}

-   i915_gem_release_all_mmaps(dev_priv);
-
del_timer_sync(dev_priv-gpu_error.hangcheck_timer);
dev_priv-pm.suspended = true;




Looks good to me.

Reviewed-by: Robert Beckett robert.beck...@intel.com
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: remove user GTT mappings early during runtime suspend

2014-05-07 Thread Imre Deak 
Currently user space can access GEM buffers mapped to GTT through
existing mappings concurrently while the platform specific suspend
handlers are running. Since these handlers may change the HW state in a
way that would break such accesses, remove the mappings before calling
the handlers. Spotted by Ville.

Also Chris pointed out that the lists that i915_gem_release_all_mmaps()
walks through need dev-struct_mutex, so take this lock. There is a
potential deadlock against a concurrent RPM resume, resolve this by
aborting and rescheduling the suspend (Daniel).

v2:
- take struct_mutex around i915_gem_release_all_mmaps() (Chris, Daniel)

Signed-off-by: Imre Deak imre.d...@intel.com
---
 drivers/gpu/drm/i915/i915_drv.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4024e16..0c9858c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -36,6 +36,7 @@
 
 #include linux/console.h
 #include linux/module.h
+#include linux/pm_runtime.h
 #include drm/drm_crtc_helper.h
 
 static struct drm_driver driver;
@@ -1315,6 +1316,30 @@ static int intel_runtime_suspend(struct device *device)
DRM_DEBUG_KMS(Suspending device\n);
 
/*
+* We could deadlock here in case another thread holding struct_mutex
+* calls RPM suspend concurrently, since the RPM suspend will wait
+* first for this RPM suspend to finish. In this case the concurrent
+* RPM resume will be followed by its RPM suspend counterpart. Still
+* for consistency return -EAGAIN, which will reschedule this suspend.
+*/
+   if (!mutex_trylock(dev-struct_mutex)) {
+   DRM_DEBUG_KMS(device lock contention, deffering suspend\n);
+   /*
+* Bump the expiration timestamp, otherwise the suspend won't
+* be rescheduled.
+*/
+   pm_runtime_mark_last_busy(device);
+
+   return -EAGAIN;
+   }
+   /*
+* We are safe here against re-faults, since the fault handler takes
+* an RPM reference.
+*/
+   i915_gem_release_all_mmaps(dev_priv);
+   mutex_unlock(dev-struct_mutex);
+
+   /*
 * rps.work can't be rearmed here, since we get here only after making
 * sure the GPU is idle and the RPS freq is set to the minimum. See
 * intel_mark_idle().
@@ -1340,8 +1365,6 @@ static int intel_runtime_suspend(struct device *device)
return ret;
}
 
-   i915_gem_release_all_mmaps(dev_priv);
-
del_timer_sync(dev_priv-gpu_error.hangcheck_timer);
dev_priv-pm.suspended = true;
 
-- 
1.8.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: remove user GTT mappings early during runtime suspend

2014-05-07 Thread Imre Deak 
On Wed, 2014-05-07 at 19:57 +0300, Imre Deak wrote:
 Currently user space can access GEM buffers mapped to GTT through
 existing mappings concurrently while the platform specific suspend
 handlers are running. Since these handlers may change the HW state in a
 way that would break such accesses, remove the mappings before calling
 the handlers. Spotted by Ville.
 
 Also Chris pointed out that the lists that i915_gem_release_all_mmaps()
 walks through need dev-struct_mutex, so take this lock. There is a
 potential deadlock against a concurrent RPM resume, resolve this by
 aborting and rescheduling the suspend (Daniel).
 
 v2:
 - take struct_mutex around i915_gem_release_all_mmaps() (Chris, Daniel)
 
 Signed-off-by: Imre Deak imre.d...@intel.com
 ---
  drivers/gpu/drm/i915/i915_drv.c | 27 +--
  1 file changed, 25 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
 index 4024e16..0c9858c 100644
 --- a/drivers/gpu/drm/i915/i915_drv.c
 +++ b/drivers/gpu/drm/i915/i915_drv.c
 @@ -36,6 +36,7 @@
  
  #include linux/console.h
  #include linux/module.h
 +#include linux/pm_runtime.h
  #include drm/drm_crtc_helper.h
  
  static struct drm_driver driver;
 @@ -1315,6 +1316,30 @@ static int intel_runtime_suspend(struct device *device)
   DRM_DEBUG_KMS(Suspending device\n);
  
   /*
 +  * We could deadlock here in case another thread holding struct_mutex
 +  * calls RPM suspend concurrently, since the RPM suspend will wait
   resume^ resume^
 +  * first for this RPM suspend to finish. In this case the concurrent
 +  * RPM resume will be followed by its RPM suspend counterpart. Still
 +  * for consistency return -EAGAIN, which will reschedule this suspend.
 +  */
 + if (!mutex_trylock(dev-struct_mutex)) {
 + DRM_DEBUG_KMS(device lock contention, deffering suspend\n);
 + /*
 +  * Bump the expiration timestamp, otherwise the suspend won't
 +  * be rescheduled.
 +  */
 + pm_runtime_mark_last_busy(device);
 +
 + return -EAGAIN;
 + }
 + /*
 +  * We are safe here against re-faults, since the fault handler takes
 +  * an RPM reference.
 +  */
 + i915_gem_release_all_mmaps(dev_priv);
 + mutex_unlock(dev-struct_mutex);
 +
 + /*
* rps.work can't be rearmed here, since we get here only after making
* sure the GPU is idle and the RPS freq is set to the minimum. See
* intel_mark_idle().
 @@ -1340,8 +1365,6 @@ static int intel_runtime_suspend(struct device *device)
   return ret;
   }
  
 - i915_gem_release_all_mmaps(dev_priv);
 -
   del_timer_sync(dev_priv-gpu_error.hangcheck_timer);
   dev_priv-pm.suspended = true;
  



signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx