From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: Unifiy GT powersave suspend logic Date: Thu, 12 Jun 2014 09:16:20 -0700 Message-ID: <20140612091620.537f332d@jbarnes-desktop> References: <1402587348-2808-2-git-send-email-jbarnes@virtuousgeek.org> <1402588425-32575-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f42.google.com (mail-pa0-f42.google.com [209.85.220.42]) by gabe.freedesktop.org (Postfix) with ESMTP id AE2666E937 for ; Thu, 12 Jun 2014 09:16:08 -0700 (PDT) Received: by mail-pa0-f42.google.com with SMTP id lj1so1179322pab.29 for ; Thu, 12 Jun 2014 09:16:08 -0700 (PDT) In-Reply-To: <1402588425-32575-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, 12 Jun 2014 17:53:45 +0200 Daniel Vetter wrote: > Jesse's patch to only quiescent our rps work and Imre's fix to address > a race with runtime pm and the forcewake reference held by the used > diverging means to address the same bug: Jesse's patch uses > flush_delayed_work while (since we want to make sure rps is set up) > while Imre's used a cancel+manuel refcount adjustment. > > Unify them again by simply reusing intel_suspend_gt_powersave in > intel_disable_gt_powersave. > > Cc: Imre Deak > Cc: Jesse Barnes > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_pm.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 685b4910eb93..49122204a001 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4706,10 +4706,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) > ironlake_disable_drps(dev); > ironlake_disable_rc6(dev); > } else if (IS_GEN6(dev) || IS_GEN7(dev) || IS_BROADWELL(dev)) { > - if (cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work)) > - intel_runtime_pm_put(dev_priv); > + intel_suspend_gt_powersave(dev); > > - cancel_work_sync(&dev_priv->rps.work); > mutex_lock(&dev_priv->rps.hw_lock); > if (IS_VALLEYVIEW(dev)) > valleyview_disable_rps(dev); Yeah looks good, though we may end up toggling RC6 on rather than canceling an oustanding enable, but that doesn't matter much on the disable path. We can do some more unification of the various freeze/suspend/resume paths too, though we'll want lots of testing... Reviewed-by: Jesse Barnes -- Jesse Barnes, Intel Open Source Technology Center