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

[Robert Beckett <robert.beckett@intel.com>]

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.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
> +	 * 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.beckett@intel.com>