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@xxxxxxxxx>
> ---
> 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:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
