From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume Date: Fri, 30 May 2014 14:05:59 -0700 Message-ID: <20140530140559.0e2bda1a@jbarnes-desktop> References: <1401397897-4655-1-git-send-email-jbarnes@virtuousgeek.org> <1401457073.24060.48.camel@intelbox> <20140530112915.5160cbef@jbarnes-desktop> <13618057.iipR8eHAlm@vostro.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pd0-f181.google.com (mail-pd0-f181.google.com [209.85.192.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 7F26D6E1BF for ; Fri, 30 May 2014 14:05:41 -0700 (PDT) Received: by mail-pd0-f181.google.com with SMTP id z10so1341432pdj.12 for ; Fri, 30 May 2014 14:05:41 -0700 (PDT) In-Reply-To: <13618057.iipR8eHAlm@vostro.rjw.lan> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Rafael J. Wysocki" Cc: intel-gfx@lists.freedesktop.org, kristen@linux.intel.com List-Id: intel-gfx@lists.freedesktop.org On Fri, 30 May 2014 23:12:45 +0200 "Rafael J. Wysocki" wrote: > On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote: > > On Fri, 30 May 2014 16:37:53 +0300 > > Imre Deak wrote: > > > > > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote: > > > > From: Kristen Carlson Accardi > > > > > > > > This matches the runtime suspend paths and allows the system to enter > > > > the lowest power mode at freeze time. > > > > > > > > Signed-off-by: Kristen Carlson Accardi > > > > Signed-off-by: Jesse Barnes > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index b6211d7..24dc856 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > > + hsw_enable_pc8(dev_priv); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > > > { > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > > + hsw_disable_pc8(dev_priv); > > > > > > I would put this before we access any of the HW regs in thaw_early() and > > > correspondingly the above call to hsw_enable_pc8() to suspend_late() > > > before we call pci_disable_device(). > > > > > > With that change this is: > > > Reviewed-by: Imre Deak > > > > For the thaw side I think that makes sense. > > > > But for the freeze side, putting it in suspend_late won't get us the > > freeze behavior we want. I think Rafael and Kristen are thinking of > > re-using the freeze infrastructure for some kind of connected standby > > feature, where most stuff is frozen, but the system isn't in S3 or S4, > > so we need the enable_pc8 call in the _freeze path as well. > > > > Rafael, is that correct? > > No, it isn't. The .freeze()/.thaw() callbacks are hibernation-specific. > There are no plans for using this in PM beyond hibernation. > > What we're going to use are .suspend/_late/_noirq() and the corresponding > resume callbacks and runtime PM. > > > I'll add a late_freeze and put it there instead, so it doesn't pollute > > the S3 suspend path. > > The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the > device to prevent it from doing DMA etc and then bring it back to life. Ok thanks. Kristen corrected me on IRC too. The latest patch I sent should do what we want then, now that I've removed the freeze_late function and put our PC8 enable in suspend_late like Imre suggested initially. -- Jesse Barnes, Intel Open Source Technology Center