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

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2851 bytes --]

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.deak@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;
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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