From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Beckett Subject: Re: [PATCH v2] drm/i915: remove user GTT mappings early during runtime suspend Date: Thu, 22 May 2014 12:48:21 +0100 Message-ID: <537DE405.7010402@intel.com> References: <1399375730-4355-1-git-send-email-imre.deak@intel.com> <1399481869-15239-1-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 37E7A6E412 for ; Thu, 22 May 2014 04:48:45 -0700 (PDT) In-Reply-To: <1399481869-15239-1-git-send-email-imre.deak@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > --- > 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 > #include > +#include > #include > > 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