From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time Date: Mon, 2 Jun 2014 10:43:46 +0200 Message-ID: <20140602084346.GL19050@phenom.ffwll.local> References: <1401397897-4655-1-git-send-email-jbarnes@virtuousgeek.org> <1401397897-4655-2-git-send-email-jbarnes@virtuousgeek.org> <1401454477.24060.32.camel@intelbox> <20140530083220.08b9b541@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by gabe.freedesktop.org (Postfix) with ESMTP id F0B3F6E4C2 for ; Mon, 2 Jun 2014 01:43:50 -0700 (PDT) Received: by mail-wg0-f52.google.com with SMTP id l18so4768890wgh.35 for ; Mon, 02 Jun 2014 01:43:50 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140530083220.08b9b541@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org, kristen@linux.intel.com List-Id: intel-gfx@lists.freedesktop.org On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote: > On Fri, 30 May 2014 15:54:37 +0300 > Imre Deak wrote: > > > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote: > > > From: Kristen Carlson Accardi > > > > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > Signed-off-by: Jesse Barnes > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 3 --- > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 66c6ffb..433bdfa 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev) > > > drm_irq_uninstall(dev); > > > dev_priv->enable_hotplug_processing = false; > > > > > > - intel_disable_gt_powersave(dev); > > > - > > > > I wonder what was the reason for this call. One possibility is that > > i915_save_state() depends on it to save the correct registers, but it > > would be good to clarify this. save_state needs to die. Pretty much because it's fragile like you've just pointed out. > > It also cancels some deferred works which we do need here. But we could > > also add that to intel_enable_gt_powersave_sync() in this patch. > > Yeah I was worried about that too, but then we do the reset on resume > anyway, and I didn't see anything in my logs in testing... > > But I can split that out if there's a reason to. Seems like we do a > bit too much teardown at suspend these days (like tearing down opregion > state), I'd like to trim it back if possible and share between runtime > and system suspend/freeze. Yeah, that's the direction I'm pushing towards, too - we should only stop timers, work, interrupts and stuff like that, but never tear down structures. So if you can use this opportunity to fix a few of the offenders (like opregion) I'd be very happy. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch